diff --git a/src/EFCore.Relational/EFCore.Relational.baseline.json b/src/EFCore.Relational/EFCore.Relational.baseline.json index 69caefdd664..2bbc7faba18 100644 --- a/src/EFCore.Relational/EFCore.Relational.baseline.json +++ b/src/EFCore.Relational/EFCore.Relational.baseline.json @@ -12486,6 +12486,22 @@ } ] }, + { + "Type": "class Microsoft.EntityFrameworkCore.Metadata.Conventions.RelationalForeignKeyIndexConvention : Microsoft.EntityFrameworkCore.Metadata.Conventions.ForeignKeyIndexConvention, Microsoft.EntityFrameworkCore.Metadata.Conventions.IIndexAnnotationChangedConvention, Microsoft.EntityFrameworkCore.Metadata.Conventions.IConvention", + "Methods": [ + { + "Member": "RelationalForeignKeyIndexConvention(Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure.ProviderConventionSetBuilderDependencies dependencies, Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure.RelationalConventionSetBuilderDependencies relationalDependencies);" + }, + { + "Member": "virtual void ProcessIndexAnnotationChanged(Microsoft.EntityFrameworkCore.Metadata.Builders.IConventionIndexBuilder indexBuilder, string name, Microsoft.EntityFrameworkCore.Metadata.IConventionAnnotation? annotation, Microsoft.EntityFrameworkCore.Metadata.IConventionAnnotation? oldAnnotation, Microsoft.EntityFrameworkCore.Metadata.Conventions.IConventionContext context);" + } + ], + "Properties": [ + { + "Member": "virtual Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure.RelationalConventionSetBuilderDependencies RelationalDependencies { get; }" + } + ] + }, { "Type": "abstract class Microsoft.EntityFrameworkCore.Storage.RelationalGeometryTypeMapping : Microsoft.EntityFrameworkCore.Storage.RelationalTypeMapping", "Methods": [ diff --git a/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs b/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs index b8e49bb22c2..d29dd39c514 100644 --- a/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs +++ b/src/EFCore.Relational/Metadata/Conventions/Infrastructure/RelationalConventionSetBuilder.cs @@ -80,6 +80,9 @@ public override ConventionSet CreateConventionSet() conventionSet.Replace( new RelationalValueGenerationConvention(Dependencies, RelationalDependencies)); + conventionSet.Replace( + new RelationalForeignKeyIndexConvention(Dependencies, RelationalDependencies)); + conventionSet.Replace( new RelationalKeyDiscoveryConvention(Dependencies, RelationalDependencies)); diff --git a/src/EFCore.Relational/Metadata/Conventions/RelationalForeignKeyIndexConvention.cs b/src/EFCore.Relational/Metadata/Conventions/RelationalForeignKeyIndexConvention.cs new file mode 100644 index 00000000000..7931ff6c580 --- /dev/null +++ b/src/EFCore.Relational/Metadata/Conventions/RelationalForeignKeyIndexConvention.cs @@ -0,0 +1,99 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions; + +/// +/// A relational-specific convention inheriting from that takes index filters +/// into account. +/// +/// +/// +/// An index with a filter only includes some of the rows in the table, so it cannot be used in place of the +/// index on the foreign key properties that would otherwise be created by convention. The exception is a filter that +/// was set by convention, such as the filter that SQL Server uses to exclude rows with null values from unique indexes. +/// These filters are not considered when deciding whether an index is needed. Strictly, such a filter can also exclude rows +/// that a lookup on a prefix of the indexed columns needs, but treating it as covering keeps the behavior that models +/// without explicit filters have always had, and so avoids adding indexes to existing models. +/// +/// +/// See Model building conventions for more information and examples. +/// +/// +public class RelationalForeignKeyIndexConvention : ForeignKeyIndexConvention, IIndexAnnotationChangedConvention +{ + /// + /// Creates a new instance of . + /// + /// Parameter object containing dependencies for this convention. + /// Parameter object containing relational dependencies for this convention. + public RelationalForeignKeyIndexConvention( + ProviderConventionSetBuilderDependencies dependencies, + RelationalConventionSetBuilderDependencies relationalDependencies) + : base(dependencies) + => RelationalDependencies = relationalDependencies; + + /// + /// Relational provider-specific dependencies for this service. + /// + protected virtual RelationalConventionSetBuilderDependencies RelationalDependencies { get; } + + /// + /// Called after an annotation is changed on an index. + /// + /// The builder for the index. + /// The annotation name. + /// The new annotation. + /// The old annotation. + /// Additional information associated with convention execution. + public virtual void ProcessIndexAnnotationChanged( + IConventionIndexBuilder indexBuilder, + string name, + IConventionAnnotation? annotation, + IConventionAnnotation? oldAnnotation, + IConventionContext context) + { + if (name == RelationalAnnotationNames.Filter) + { + ProcessIndexCoverageChanged(indexBuilder.Metadata); + } + } + + /// + /// Called when a model is being finalized. + /// + /// + /// Upgrading a filter from being set by convention to being set explicitly, without changing its value, is not reported + /// as an annotation change. This makes sure that foreign key indexes are created for any index that is partial at this point. + /// + /// The builder for the model. + /// Additional information associated with convention execution. + public override void ProcessModelFinalizing( + IConventionModelBuilder modelBuilder, + IConventionContext context) + { + foreach (var entityType in modelBuilder.Metadata.GetEntityTypes()) + { + foreach (var index in entityType.GetDeclaredIndexes().ToList()) + { + if (index.IsInModel + && IsPartial(index)) + { + ProcessIndexCoverageChanged(index); + } + } + } + + base.ProcessModelFinalizing(modelBuilder, context); + } + + /// + /// + /// An index is partial if it has a filter that was not set by convention. Filters set by convention, such as the filter + /// used by SQL Server to exclude rows with null values from unique indexes, are ignored. The filter is read from the index + /// itself, so a filter that only applies to a specific table because of table sharing is not taken into account. + /// + protected override bool IsPartial(IConventionIndex index) + => index.FindAnnotation(RelationalAnnotationNames.Filter) is IConventionAnnotation { Value: not null } filter + && filter.GetConfigurationSource() != ConfigurationSource.Convention; +} diff --git a/src/EFCore/EFCore.baseline.json b/src/EFCore/EFCore.baseline.json index 4371962ccd0..93ea0a2a838 100644 --- a/src/EFCore/EFCore.baseline.json +++ b/src/EFCore/EFCore.baseline.json @@ -8827,6 +8827,9 @@ { "Member": "virtual Microsoft.EntityFrameworkCore.Metadata.IConventionIndex? CreateIndex(System.Collections.Generic.IReadOnlyList properties, bool unique, Microsoft.EntityFrameworkCore.Metadata.Builders.IConventionEntityTypeBuilder entityTypeBuilder);" }, + { + "Member": "virtual bool IsPartial(Microsoft.EntityFrameworkCore.Metadata.IConventionIndex index);" + }, { "Member": "virtual void ProcessEntityTypeBaseTypeChanged(Microsoft.EntityFrameworkCore.Metadata.Builders.IConventionEntityTypeBuilder entityTypeBuilder, Microsoft.EntityFrameworkCore.Metadata.IConventionEntityType? newBaseType, Microsoft.EntityFrameworkCore.Metadata.IConventionEntityType? oldBaseType, Microsoft.EntityFrameworkCore.Metadata.Conventions.IConventionContext context);" }, @@ -8845,6 +8848,9 @@ { "Member": "virtual void ProcessIndexAdded(Microsoft.EntityFrameworkCore.Metadata.Builders.IConventionIndexBuilder indexBuilder, Microsoft.EntityFrameworkCore.Metadata.Conventions.IConventionContext context);" }, + { + "Member": "virtual void ProcessIndexCoverageChanged(Microsoft.EntityFrameworkCore.Metadata.IConventionIndex index);" + }, { "Member": "virtual void ProcessIndexRemoved(Microsoft.EntityFrameworkCore.Metadata.Builders.IConventionEntityTypeBuilder entityTypeBuilder, Microsoft.EntityFrameworkCore.Metadata.IConventionIndex index, Microsoft.EntityFrameworkCore.Metadata.Conventions.IConventionContext context);" }, diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyIndexConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyIndexConvention.cs index 758a13c6d3b..19af8eea612 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyIndexConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyIndexConvention.cs @@ -195,8 +195,7 @@ public virtual void ProcessEntityTypeBaseTypeChanged( } else { - var coveringIndex = baseIndexes!.FirstOrDefault(i => AreIndexedBy( - foreignKey.Properties, foreignKey.IsUnique, i.Properties, i.IsUnique)); + var coveringIndex = baseIndexes!.FirstOrDefault(i => AreIndexedBy(foreignKey.Properties, foreignKey.IsUnique, i)); if (coveringIndex != null) { RemoveIndex(index); @@ -221,6 +220,55 @@ public virtual void ProcessIndexAdded(IConventionIndexBuilder indexBuilder, ICon return; } + RemoveCoveredIndexes(index); + } + + /// + /// Called when something other than the properties or uniqueness of an index changes which may affect whether + /// the index covers the rows of the table. For example, a filter being added to or removed from the index. + /// + /// + /// When the index no longer covers all rows, any foreign key indexes that were removed because they were covered by it + /// are re-created. When the index starts covering all rows, any indexes that it now covers are removed. + /// + /// The index. + protected virtual void ProcessIndexCoverageChanged(IConventionIndex index) + { + if (!index.IsInModel + || index.CollectionIndices is not null) + { + return; + } + + if (IsPartial(index)) + { + CreateCoveredForeignKeyIndexes(index); + } + else + { + RemoveCoveredIndexes(index); + } + } + + /// + /// Returns a value indicating whether the given index only includes a subset of the rows in the table, and so + /// cannot be used in place of an index that includes all rows. + /// + /// + /// This is always by default. Relational providers override this to account for index filters. + /// + /// The index. + /// if the index only includes a subset of the rows. + protected virtual bool IsPartial(IConventionIndex index) + => false; + + private void RemoveCoveredIndexes(IConventionIndex index) + { + if (IsPartial(index)) + { + return; + } + foreach (var otherIndex in index.DeclaringEntityType.GetDerivedTypesInclusive() .SelectMany(t => t.GetDeclaredIndexes()) .Where(i => i != index @@ -231,6 +279,24 @@ public virtual void ProcessIndexAdded(IConventionIndexBuilder indexBuilder, ICon } } + private void CreateCoveredForeignKeyIndexes(IConventionIndex index) + { + foreach (var foreignKey in index.DeclaringEntityType.GetDerivedTypesInclusive() + .SelectMany(t => t.GetDeclaredForeignKeys()) + .Where(fk => AreIndexedBy(fk.Properties, fk.IsUnique, index.Properties, index.IsUnique)).ToList()) + { + CreateIndex(foreignKey.Properties, foreignKey.IsUnique, foreignKey.DeclaringEntityType.Builder); + } + } + + private bool AreIndexedBy( + IReadOnlyList properties, + bool unique, + IConventionIndex coveringIndex) + => coveringIndex.CollectionIndices is null + && !IsPartial(coveringIndex) + && AreIndexedBy(properties, unique, coveringIndex.Properties, coveringIndex.IsUnique); + /// /// Called after an index is removed. /// @@ -248,17 +314,14 @@ public virtual void ProcessIndexRemoved( } // A removed complex index never covered any FK index, so nothing to re-create. - if (index.CollectionIndices is not null) + // Likewise, a partial index never caused a FK index to be removed. + if (index.CollectionIndices is not null + || IsPartial(index)) { return; } - foreach (var foreignKey in index.DeclaringEntityType.GetDerivedTypesInclusive() - .SelectMany(t => t.GetDeclaredForeignKeys()) - .Where(fk => AreIndexedBy(fk.Properties, fk.IsUnique, index.Properties, index.IsUnique))) - { - CreateIndex(foreignKey.Properties, foreignKey.IsUnique, foreignKey.DeclaringEntityType.Builder); - } + CreateCoveredForeignKeyIndexes(index); } /// @@ -292,8 +355,7 @@ public virtual void ProcessForeignKeyUniquenessChanged( } var coveringIndex = foreignKey.DeclaringEntityType.GetIndexes() - .FirstOrDefault(i => i.CollectionIndices is null - && AreIndexedBy(foreignKey.Properties, false, i.Properties, i.IsUnique)); + .FirstOrDefault(i => AreIndexedBy(foreignKey.Properties, false, i)); if (coveringIndex != null) { RemoveIndex(index); @@ -315,7 +377,8 @@ public virtual void ProcessIndexUniquenessChanged( IConventionContext context) { var index = indexBuilder.Metadata; - if (index.CollectionIndices is not null) + if (index.CollectionIndices is not null + || IsPartial(index)) { return; } @@ -366,8 +429,7 @@ public virtual void ProcessIndexUniquenessChanged( foreach (var existingIndex in entityTypeBuilder.Metadata.GetIndexes()) { - if (existingIndex.CollectionIndices is null - && AreIndexedBy(properties, unique, existingIndex.Properties, existingIndex.IsUnique)) + if (AreIndexedBy(properties, unique, existingIndex)) { return null; } @@ -431,10 +493,7 @@ public virtual void ProcessModelFinalizing( foreach (var existingIndex in entityType.GetIndexes()) { - if (existingIndex.CollectionIndices is null - && AreIndexedBy( - declaredForeignKey.Properties, declaredForeignKey.IsUnique, existingIndex.Properties, - existingIndex.IsUnique)) + if (AreIndexedBy(declaredForeignKey.Properties, declaredForeignKey.IsUnique, existingIndex)) { if (declaredForeignKey.Properties.Count != existingIndex.Properties.Count) { diff --git a/test/EFCore.Relational.Specification.Tests/Scaffolding/CompiledModelRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Scaffolding/CompiledModelRelationalTestBase.cs index 7ad4073d04d..8f9ef6d5bcd 100644 --- a/test/EFCore.Relational.Specification.Tests/Scaffolding/CompiledModelRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Scaffolding/CompiledModelRelationalTestBase.cs @@ -911,7 +911,10 @@ protected virtual void AssertTpcSprocs(IModel model) Assert.Equal("PIX", alternateIndex.GetDatabaseName()); Assert.Equal("AlternateId <> NULL", alternateIndex.GetFilter()); - Assert.Equal([alternateIndex], principalBaseId.GetContainingIndexes()); + // The filtered unique index doesn't cover all rows, so the foreign key also has an index of its own. + var foreignKeyIndex = principalBase.FindIndex(principalBaseId)!; + Assert.NotSame(alternateIndex, foreignKeyIndex); + Assert.Equal([foreignKeyIndex, alternateIndex], principalBaseId.GetContainingIndexes()); var insertSproc = principalBase.GetInsertStoredProcedure()!; Assert.Equal("PrincipalBase_Insert", insertSproc.Name); diff --git a/test/EFCore.Relational.Tests/Metadata/Conventions/RelationalForeignKeyIndexFilterTest.cs b/test/EFCore.Relational.Tests/Metadata/Conventions/RelationalForeignKeyIndexFilterTest.cs new file mode 100644 index 00000000000..6f9782c48db --- /dev/null +++ b/test/EFCore.Relational.Tests/Metadata/Conventions/RelationalForeignKeyIndexFilterTest.cs @@ -0,0 +1,335 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// ReSharper disable ClassNeverInstantiated.Local +// ReSharper disable InconsistentNaming + +namespace Microsoft.EntityFrameworkCore.Metadata.Conventions; + +/// +/// Tests for how index filters affect the creation and removal of foreign key indexes. See #24237. +/// +public class RelationalForeignKeyIndexFilterTest +{ + [Fact] + public void Unfiltered_index_covers_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + + Assert.Equal(["PeriodId,CompanyId"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filtered_index_does_not_cover_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Foreign_key_index_is_restored_when_filter_is_added_to_existing_index() + { + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + + Assert.Equal(["PeriodId,CompanyId"], DescribeIndexes(modelBuilder)); + + indexBuilder.HasFilter("x"); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Foreign_key_index_is_removed_again_when_filter_is_removed() + { + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + + indexBuilder.HasFilter(null); + + Assert.Equal(["PeriodId,CompanyId"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filtered_index_declared_before_foreign_key_does_not_prevent_foreign_key_index() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Changing_the_filter_value_does_not_duplicate_the_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + + indexBuilder.HasFilter("y"); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE y"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filter_set_by_convention_still_covers_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var index = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).Metadata; + + ((IConventionIndex)index).Builder.HasFilter("[CompanyId] IS NOT NULL"); + + Assert.Equal(["PeriodId,CompanyId WHERE [CompanyId] IS NOT NULL"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filter_set_from_data_annotation_does_not_cover_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var index = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).Metadata; + + ((IConventionIndex)index).Builder.HasFilter("x", fromDataAnnotation: true); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filter_upgraded_from_convention_to_explicit_with_the_same_value_restores_foreign_key_index() + { + // Only the configuration source of the filter changes here, not its value, so no annotation change is reported. + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + ((IConventionIndex)indexBuilder.Metadata).Builder.HasFilter("x"); + + Assert.Equal(["PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + + indexBuilder.HasFilter("x"); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder, finalize: true)); + } + + [Fact] + public void Foreign_key_index_is_not_created_when_another_unfiltered_index_covers_it() + { + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.Id }); + + indexBuilder.HasFilter("x"); + + Assert.Equal(["PeriodId,CompanyId WHERE x", "PeriodId,Id"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Removing_filter_does_not_remove_explicitly_configured_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + modelBuilder.Entity().HasIndex(i => i.PeriodId); + + indexBuilder.HasFilter(null); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Unnamed_filtered_index_on_exactly_the_foreign_key_columns_is_the_foreign_key_index() + { + // An unnamed index on the same properties is the same index in the model, so there can't be a second one. + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity().HasIndex(i => i.PeriodId).HasFilter("x"); + + Assert.Equal(["PeriodId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Named_filtered_index_on_exactly_the_foreign_key_columns_gets_a_separate_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity().HasIndex(["PeriodId"], "Named").HasFilter("x"); + + var indexes = modelBuilder.Model.FindEntityType(typeof(Invoice))!.GetIndexes().OrderBy(i => i.Name).ToList(); + Assert.Equal(2, indexes.Count); + + Assert.Null(indexes[0].Name); + Assert.Null(indexes[0].GetFilter()); + Assert.False(indexes[0].IsUnique); + + Assert.Equal("Named", indexes[1].Name); + Assert.Equal("x", indexes[1].GetFilter()); + } + + [Fact] + public void Named_unique_filtered_index_on_exactly_the_unique_foreign_key_columns_gets_a_separate_unique_foreign_key_index() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasOne(i => i.Period).WithOne().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(["PeriodId"], "Named").IsUnique().HasFilter("x"); + + var indexes = modelBuilder.Model.FindEntityType(typeof(Invoice))!.GetIndexes().OrderBy(i => i.Name).ToList(); + Assert.Equal(2, indexes.Count); + + Assert.Null(indexes[0].Name); + Assert.Null(indexes[0].GetFilter()); + Assert.True(indexes[0].IsUnique); + + Assert.Equal("Named", indexes[1].Name); + Assert.Equal("x", indexes[1].GetFilter()); + Assert.True(indexes[1].IsUnique); + } + + [Fact] + public void Filtered_unique_index_does_not_cover_foreign_key_index_when_uniqueness_changes() + { + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x").IsUnique(); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Foreign_key_index_covered_only_by_filtered_index_is_kept_when_foreign_key_becomes_non_unique() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasOne(i => i.Period).WithOne().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + + Assert.Equal(["PeriodId", "PeriodId,CompanyId WHERE x"], DescribeIndexes(modelBuilder)); + + var entityType = modelBuilder.Model.FindEntityType(typeof(Invoice))!; + Assert.False(entityType.FindIndex(entityType.FindProperty(nameof(Invoice.PeriodId))!)!.IsUnique); + } + + [Fact] + public void Removing_filtered_index_does_not_duplicate_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var index = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x").Metadata; + + modelBuilder.Entity().Metadata.RemoveIndex(index); + + Assert.Equal(["PeriodId"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Removing_unfiltered_index_restores_foreign_key_index() + { + var modelBuilder = CreateModelBuilder(); + var index = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).Metadata; + + modelBuilder.Entity().Metadata.RemoveIndex(index); + + Assert.Equal(["PeriodId"], DescribeIndexes(modelBuilder)); + } + + [Fact] + public void Filtered_index_on_base_type_does_not_cover_foreign_key_index_on_derived_type() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity(); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + var indexBuilder = modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + + Assert.Empty(DerivedIndexes(modelBuilder)); + + indexBuilder.HasFilter("x"); + + Assert.Equal(["PeriodId"], DerivedIndexes(modelBuilder)); + + indexBuilder.HasFilter(null); + + Assert.Empty(DerivedIndexes(modelBuilder)); + } + + [Fact] + public void Foreign_key_index_is_kept_when_base_type_with_covering_filtered_index_is_set() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + modelBuilder.Entity().HasBaseType((Type?)null); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + + Assert.Equal(["PeriodId"], DerivedIndexes(modelBuilder)); + + modelBuilder.Entity().HasBaseType(); + + Assert.Equal(["PeriodId"], DerivedIndexes(modelBuilder)); + } + + [Fact] + public void Redundant_index_is_not_logged_for_filtered_covering_index() + { + var logger = new TestLogger { EnabledFor = LogLevel.Debug }; + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(modelLogger: logger); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).HasFilter("x"); + + modelBuilder.FinalizeModel(); + + Assert.Null(logger.LoggedAt); + } + + [Fact] + public void Redundant_index_is_logged_for_unfiltered_covering_index() + { + var logger = new TestLogger { EnabledFor = LogLevel.Debug }; + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(modelLogger: logger); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }); + + modelBuilder.FinalizeModel(); + + Assert.Equal(CoreEventId.RedundantIndexRemoved, logger.LoggedEvent); + } + + private static ModelBuilder CreateModelBuilder() + { + var modelBuilder = FakeRelationalTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + return modelBuilder; + } + + private static string[] DescribeIndexes(ModelBuilder modelBuilder, bool finalize = false) + { + IReadOnlyModel model = finalize ? modelBuilder.FinalizeModel() : modelBuilder.Model; + + return model.FindEntityType(typeof(Invoice))!.GetIndexes().Select(Describe).Order(StringComparer.Ordinal).ToArray(); + } + + private static string[] DerivedIndexes(ModelBuilder modelBuilder) + => modelBuilder.Model.FindEntityType(typeof(DerivedInvoice))!.GetDeclaredIndexes().Select(Describe) + .Order(StringComparer.Ordinal).ToArray(); + + private static string Describe(IReadOnlyIndex index) + => string.Join(",", index.Properties.Select(p => p.Name)) + (index.GetFilter() is { } filter ? " WHERE " + filter : ""); + + private class Period + { + public int Id { get; set; } + } + + private class Invoice + { + public int Id { get; set; } + public int PeriodId { get; set; } + public int CompanyId { get; set; } + public Period? Period { get; set; } + } + + private class BaseInvoice + { + public int Id { get; set; } + public int PeriodId { get; set; } + public int CompanyId { get; set; } + } + + private class DerivedInvoice : BaseInvoice + { + public Period? Period { get; set; } + } +} diff --git a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/DbContextModelBuilder.cs b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/DbContextModelBuilder.cs index dcc2f82adc3..8ec2d776015 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/DbContextModelBuilder.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/DbContextModelBuilder.cs @@ -1283,6 +1283,15 @@ private IRelationalModel CreateRelationalModel() pK_PrincipalBase.MappedKeys.Add(pK_PrincipalBaseKey); RelationalModel.GetOrCreateUniqueConstraints(pK_PrincipalBaseKey).Add(pK_PrincipalBase); principalBaseTable.UniqueConstraints.Add("PK_PrincipalBase", pK_PrincipalBase); + var iX_PrincipalBase_PrincipalBaseId = new TableIndex( + "IX_PrincipalBase_PrincipalBaseId", principalBaseTable, new[] { principalBaseIdColumn }, false); + iX_PrincipalBase_PrincipalBaseId.SetRowIndexValueFactory(new SimpleRowIndexValueFactory(iX_PrincipalBase_PrincipalBaseId)); + var iX_PrincipalBase_PrincipalBaseIdIx = RelationalModel.GetIndex(this, + "Microsoft.EntityFrameworkCore.Scaffolding.CompiledModelTestBase+PrincipalBase", + new[] { "PrincipalBaseId" }); + iX_PrincipalBase_PrincipalBaseId.MappedIndexes.Add(iX_PrincipalBase_PrincipalBaseIdIx); + RelationalModel.GetOrCreateTableIndexes(iX_PrincipalBase_PrincipalBaseIdIx).Add(iX_PrincipalBase_PrincipalBaseId); + principalBaseTable.Indexes.Add("IX_PrincipalBase_PrincipalBaseId", iX_PrincipalBase_PrincipalBaseId); var iX_PrincipalBase_PrincipalDerivedDependentBasebyteId = new TableIndex( "IX_PrincipalBase_PrincipalDerived>Id", principalBaseTable, new[] { principalDerivedDependentBasebyteIdColumn }, false); iX_PrincipalBase_PrincipalDerivedDependentBasebyteId.SetRowIndexValueFactory(new SimpleRowIndexValueFactory(iX_PrincipalBase_PrincipalDerivedDependentBasebyteId)); @@ -1307,6 +1316,12 @@ private IRelationalModel CreateRelationalModel() pK_PrincipalDerived.MappedKeys.Add(pK_PrincipalBaseKey); RelationalModel.GetOrCreateUniqueConstraints(pK_PrincipalBaseKey).Add(pK_PrincipalDerived); principalDerivedTable.UniqueConstraints.Add("PK_PrincipalDerived", pK_PrincipalDerived); + var iX_PrincipalDerived_PrincipalBaseId = new TableIndex( + "IX_PrincipalDerived_PrincipalBaseId", principalDerivedTable, new[] { principalBaseIdColumn0 }, false); + iX_PrincipalDerived_PrincipalBaseId.SetRowIndexValueFactory(new SimpleRowIndexValueFactory(iX_PrincipalDerived_PrincipalBaseId)); + iX_PrincipalDerived_PrincipalBaseId.MappedIndexes.Add(iX_PrincipalBase_PrincipalBaseIdIx); + RelationalModel.GetOrCreateTableIndexes(iX_PrincipalBase_PrincipalBaseIdIx).Add(iX_PrincipalDerived_PrincipalBaseId); + principalDerivedTable.Indexes.Add("IX_PrincipalDerived_PrincipalBaseId", iX_PrincipalDerived_PrincipalBaseId); var iX_PrincipalDerived_PrincipalDerivedDependentBasebyteId = new TableIndex( "IX_PrincipalDerived_PrincipalDerived>Id", principalDerivedTable, new[] { principalDerivedDependentBasebyteIdColumn0 }, false); iX_PrincipalDerived_PrincipalDerivedDependentBasebyteId.SetRowIndexValueFactory(new SimpleRowIndexValueFactory(iX_PrincipalDerived_PrincipalDerivedDependentBasebyteId)); diff --git a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/PrincipalBaseEntityType.cs b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/PrincipalBaseEntityType.cs index 2c7cb572ae4..cd6c99f1523 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/PrincipalBaseEntityType.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/Baselines/Tpc_Sprocs/PrincipalBaseEntityType.cs @@ -33,7 +33,7 @@ public static RuntimeEntityType Create(RuntimeModel model, RuntimeEntityType bas propertyCount: 15, navigationCount: 1, foreignKeyCount: 2, - unnamedIndexCount: 1, + unnamedIndexCount: 2, namedIndexCount: 1, keyCount: 1); @@ -739,6 +739,9 @@ public static RuntimeEntityType Create(RuntimeModel model, RuntimeEntityType bas runtimeEntityType.SetPrimaryKey(key); var index = runtimeEntityType.AddIndex( + new[] { principalBaseId }); + + var index0 = runtimeEntityType.AddIndex( new[] { principalDerivedId }); var principalIndex = runtimeEntityType.AddIndex( diff --git a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/CompiledModelSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/CompiledModelSqlServerTest.cs index e34d35ba893..7b5953aeb54 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/CompiledModelSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/CompiledModelSqlServerTest.cs @@ -325,7 +325,10 @@ protected override void AssertTpcSprocs(IModel model) CoreStrings.RuntimeModelMissingData, Assert.Throws(() => alternateIndex.GetDataCompression()).Message); - Assert.Equal([alternateIndex], principalBaseId.GetContainingIndexes()); + // The filtered unique index doesn't cover all rows, so the foreign key also has an index of its own. + var foreignKeyIndex = principalBase.FindIndex(principalBaseId)!; + Assert.NotSame(alternateIndex, foreignKeyIndex); + Assert.Equal([foreignKeyIndex, alternateIndex], principalBaseId.GetContainingIndexes()); } // ConditionalFact does not work on overridden test methods (xunit discovers the override via diff --git a/test/EFCore.SqlServer.Tests/Metadata/Conventions/RelationalForeignKeyIndexConventionTest.cs b/test/EFCore.SqlServer.Tests/Metadata/Conventions/RelationalForeignKeyIndexConventionTest.cs index b4439ca4241..1d8d2c7425b 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/Conventions/RelationalForeignKeyIndexConventionTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/Conventions/RelationalForeignKeyIndexConventionTest.cs @@ -65,6 +65,43 @@ public void Removing_relationship_does_not_remove_conventional_index_if_in_use() Assert.Empty(dependentEntityBuilder.Metadata.GetForeignKeys()); } + [Fact] + public void Filter_added_by_SQL_Server_convention_to_unique_index_still_covers_foreign_key_index() + { + var modelBuilder = SqlServerTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).IsUnique(); + + var index = Assert.Single(modelBuilder.Model.FindEntityType(typeof(FilteredIndexInvoice))!.GetIndexes()); + Assert.Equal("[PeriodId] IS NOT NULL AND [CompanyId] IS NOT NULL", index.GetFilter()); + Assert.Equal(["PeriodId", "CompanyId"], index.Properties.Select(p => p.Name)); + } + + [Fact] + public void Filter_explicitly_set_on_unique_index_does_not_cover_foreign_key_index() + { + var modelBuilder = SqlServerTestHelpers.Instance.CreateConventionBuilder(); + modelBuilder.Entity().HasOne(i => i.Period).WithMany().HasForeignKey(i => i.PeriodId); + modelBuilder.Entity().HasIndex(i => new { i.PeriodId, i.CompanyId }).IsUnique().HasFilter("[CompanyId] > 0"); + + Assert.Equal( + [["PeriodId"], ["PeriodId", "CompanyId"]], + modelBuilder.Model.FindEntityType(typeof(FilteredIndexInvoice))!.GetIndexes().Select(i => i.Properties.Select(p => p.Name).ToArray())); + } + + private class FilteredIndexPeriod + { + public int Id { get; set; } + } + + private class FilteredIndexInvoice + { + public int Id { get; set; } + public int? PeriodId { get; set; } + public int? CompanyId { get; set; } + public FilteredIndexPeriod? Period { get; set; } + } + private static TestLogger CreateTestLogger() => new() { EnabledFor = LogLevel.Warning }; diff --git a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyIndexConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyIndexConventionTest.cs index 3ab38be6114..3175db08920 100644 --- a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyIndexConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyIndexConventionTest.cs @@ -29,6 +29,71 @@ public void Does_not_override_foreign_key_index_uniqueness_when_referenced_key_c Assert.False(index.IsUnique); } + [Fact] + public void Partial_index_does_not_cover_foreign_key_index() + { + var modelBuilder = CreateModelBuilderWithPartialAwareConvention(); + modelBuilder.Entity().HasOne(t => t.Principal).WithMany().HasForeignKey(t => t.PrincipalId); + var index = modelBuilder.Entity().HasIndex(t => new { t.PrincipalId, t.DependentId }).Metadata; + + var entityType = modelBuilder.Model.FindEntityType(typeof(DependentEntity))!; + Assert.Equal([index], entityType.GetIndexes()); + + ((IMutableIndex)index).SetAnnotation(PartialAnnotation, true); + + Assert.Equal(2, entityType.GetIndexes().Count()); + Assert.NotNull(entityType.FindIndex(entityType.FindProperty(nameof(DependentEntity.PrincipalId))!)); + + ((IMutableIndex)index).RemoveAnnotation(PartialAnnotation); + + Assert.Equal([index], entityType.GetIndexes()); + } + + [Fact] + public void Partial_index_declared_before_foreign_key_does_not_prevent_foreign_key_index() + { + var modelBuilder = CreateModelBuilderWithPartialAwareConvention(); + var index = modelBuilder.Entity().HasIndex(t => new { t.PrincipalId, t.DependentId }).Metadata; + ((IMutableIndex)index).SetAnnotation(PartialAnnotation, true); + + modelBuilder.Entity().HasOne(t => t.Principal).WithMany().HasForeignKey(t => t.PrincipalId); + + var entityType = modelBuilder.Model.FindEntityType(typeof(DependentEntity))!; + Assert.Equal(2, entityType.GetIndexes().Count()); + } + + private const string PartialAnnotation = "Test:Partial"; + + private static ModelBuilder CreateModelBuilderWithPartialAwareConvention() + { + var services = InMemoryTestHelpers.Instance.CreateContextServices(); + var dependencies = services.GetRequiredService(); + + // The in-memory provider doesn't use this convention, so it is added here. + return InMemoryTestHelpers.Instance.CreateConventionBuilder( + services, c => c.ConventionSet.Add(new PartialAwareConvention(dependencies))); + } + + private class PartialAwareConvention(ProviderConventionSetBuilderDependencies dependencies) + : ForeignKeyIndexConvention(dependencies), IIndexAnnotationChangedConvention + { + protected override bool IsPartial(IConventionIndex index) + => index.FindAnnotation(PartialAnnotation) != null; + + public void ProcessIndexAnnotationChanged( + IConventionIndexBuilder indexBuilder, + string name, + IConventionAnnotation? annotation, + IConventionAnnotation? oldAnnotation, + IConventionContext context) + { + if (name == PartialAnnotation) + { + ProcessIndexCoverageChanged(indexBuilder.Metadata); + } + } + } + private class PrincipalEntity { public int InitialPrincipalId { get; set; }