From 66ab359714c804bf0a63b015faef335e74e873af Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Tue, 27 Oct 2020 19:42:36 -0700 Subject: [PATCH 1/2] Don't discover FK properties for an identifying FK created by convention Silently ignore inherited properties in TPT in SqlServerIndexConvention Fixes #23092 --- .../Conventions/SqlServerIndexConvention.cs | 49 +++++++++++++------ .../ForeignKeyPropertyDiscoveryConvention.cs | 9 +++- .../SqlServerModelBuilderGenericTest.cs | 27 ++++++++++ ...reignKeyPropertyDiscoveryConventionTest.cs | 28 +++++++++++ 4 files changed, 96 insertions(+), 17 deletions(-) diff --git a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs index e87910440d2..d2098a6061f 100644 --- a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs +++ b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Linq; using System.Text; using JetBrains.Annotations; @@ -154,12 +155,13 @@ private IConventionIndexBuilder SetIndexFilter(IConventionIndexBuilder indexBuil var index = indexBuilder.Metadata; if (index.IsUnique && index.IsClustered() != true - && index.Properties.Any(property => property.IsColumnNullable())) + && GetNullableColumns(index) is List nullableColumns + && nullableColumns.Count > 0) { if (columnNameChanged || index.GetFilter() == null) { - indexBuilder.HasFilter(CreateIndexFilter(index)); + indexBuilder.HasFilter(CreateIndexFilter(nullableColumns)); } } else @@ -173,20 +175,8 @@ private IConventionIndexBuilder SetIndexFilter(IConventionIndexBuilder indexBuil return indexBuilder; } - private string CreateIndexFilter(IIndex index) + private string CreateIndexFilter(List nullableColumns) { - var tableName = index.DeclaringEntityType.GetTableName(); - if (tableName == null) - { - return null; - } - - var table = StoreObjectIdentifier.Table(tableName, index.DeclaringEntityType.GetSchema()); - var nullableColumns = index.Properties - .Where(property => property.IsColumnNullable(table)) - .Select(property => property.GetColumnName(table)) - .ToList(); - var builder = new StringBuilder(); for (var i = 0; i < nullableColumns.Count; i++) { @@ -202,5 +192,34 @@ private string CreateIndexFilter(IIndex index) return builder.ToString(); } + + private List GetNullableColumns(IIndex index) + { + var tableName = index.DeclaringEntityType.GetTableName(); + if (tableName == null) + { + return null; + } + + var nullableColumns = new List(); + var table = StoreObjectIdentifier.Table(tableName, index.DeclaringEntityType.GetSchema()); + foreach (var property in index.Properties) + { + var columnName = property.GetColumnName(table); + if (columnName == null) + { + return null; + } + + if (!property.IsColumnNullable(table)) + { + continue; + } + + nullableColumns.Add(columnName); + } + + return nullableColumns; + } } } diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs index b5969119aa2..df708483000 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs @@ -130,7 +130,13 @@ private IConventionForeignKeyBuilder DiscoverProperties( IConventionContext context) { var foreignKey = relationshipBuilder.Metadata; - if (!ConfigurationSource.Convention.Overrides(foreignKey.GetPropertiesConfigurationSource())) + var foreignKeyProperties = FindCandidateForeignKeyProperties(relationshipBuilder.Metadata, onDependent: true); + var propertiesConfigurationSource = foreignKey.GetPropertiesConfigurationSource(); + if (!ConfigurationSource.Convention.OverridesStrictly(propertiesConfigurationSource) + && (propertiesConfigurationSource != ConfigurationSource.Convention + || (foreignKey.Properties.All(p => !p.IsImplicitlyCreated()) + && (foreignKeyProperties == null + || !foreignKey.Properties.SequenceEqual(foreignKeyProperties))))) { var batch = context.DelayConventions(); using var foreignKeyReference = batch.Track(foreignKey); @@ -184,7 +190,6 @@ private IConventionForeignKeyBuilder DiscoverProperties( } } - var foreignKeyProperties = FindCandidateForeignKeyProperties(relationshipBuilder.Metadata, onDependent: true); if (foreignKeyProperties == null) { if (invertible diff --git a/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs b/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs index 0670cdf0c13..5929f551921 100644 --- a/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs +++ b/test/EFCore.SqlServer.Tests/ModelBuilding/SqlServerModelBuilderGenericTest.cs @@ -206,6 +206,33 @@ public virtual void TPT_identifying_FK_are_created_only_on_declaring_type() Assert.Single(sesameBunFk.GetMappedConstraints()); } + [ConditionalFact] + public virtual void TPT_index_can_use_inherited_properties() + { + var modelBuilder = CreateModelBuilder(); + modelBuilder.Entity() + .Ignore(b => b.Bun) + .Ignore(b => b.Pickles); + modelBuilder.Entity(b => + { + b.ToTable("Ingredients"); + b.Property("NullableProp"); + b.Ignore(i => i.BigMak); + }); + modelBuilder.Entity(b => + { + b.ToTable("Buns"); + b.HasIndex(bun => bun.BurgerId); + b.HasIndex("NullableProp"); + b.HasOne(i => i.BigMak).WithOne().HasForeignKey(i => i.Id); + }); + + var model = modelBuilder.FinalizeModel(); + + var bunType = model.FindEntityType(typeof(Bun)); + Assert.All(bunType.GetIndexes(), i => Assert.Null(i.GetFilter())); + } + public class Parent { public int Id { get; set; } diff --git a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs index 4956b26322e..5ab4e0fb6fd 100644 --- a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs @@ -577,6 +577,29 @@ public void Does_not_match_dependent_PK_for_self_ref() ValidateModel(); } + [ConditionalFact] + public void Does_not_match_for_convention_identifying_FK() + { + var derivedType = PrincipalType.Builder.ModelBuilder.Entity(typeof(DerivedPrincipalEntity), ConfigurationSource.Convention); + derivedType.HasBaseType(PrincipalType, ConfigurationSource.Convention); + + PrincipalType.Builder.Property(typeof(int), nameof(PrincipalEntity.PrincipalEntityId), ConfigurationSource.Convention); + var relationshipBuilder = derivedType.HasRelationship( + PrincipalType, PrincipalType.FindPrimaryKey().Properties, ConfigurationSource.Convention) + .IsUnique(true, ConfigurationSource.DataAnnotation); + + var newRelationshipBuilder = RunConvention(relationshipBuilder); + Assert.Same(relationshipBuilder, newRelationshipBuilder); + + var fk = (IForeignKey)relationshipBuilder.Metadata; + Assert.Equal(nameof(PrincipalEntity.PeeKay), fk.Properties.Single().Name); + Assert.Same(fk, derivedType.Metadata.GetForeignKeys().Single()); + Assert.True(fk.IsUnique); + Assert.True(fk.IsRequired); + + ValidateModel(); + } + [ConditionalFact] public void Matches_composite_dependent_PK_for_unique_FK() { @@ -1192,6 +1215,7 @@ private class PrincipalEntity public static readonly PropertyInfo DependentEntityKayPeeProperty = typeof(PrincipalEntity).GetProperty("DependentEntityKayPee"); + public int PrincipalEntityId { get; set; } public int PeeKay { get; set; } public int? DependentEntityKayPee { get; set; } public IEnumerable InverseNav { get; set; } @@ -1199,6 +1223,10 @@ private class PrincipalEntity public PrincipalEntity SelfRef { get; set; } } + private class DerivedPrincipalEntity : PrincipalEntity + { + } + private class DependentEntity { public static readonly PropertyInfo SomeNavIDProperty = typeof(DependentEntity).GetProperty("SomeNavID"); From 05d1ddd616fc159d76e6ee52ae9b4994d6b3f340 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Wed, 28 Oct 2020 16:36:54 -0700 Subject: [PATCH 2/2] Add quirk mode for #23092 --- .../Metadata/Conventions/SqlServerIndexConvention.cs | 5 ++++- .../Conventions/ForeignKeyPropertyDiscoveryConvention.cs | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs index d2098a6061f..4ae01369054 100644 --- a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs +++ b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerIndexConvention.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.Linq; using System.Text; @@ -201,12 +202,14 @@ private List GetNullableColumns(IIndex index) return null; } + var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23092_2", out var isEnabled) && isEnabled; var nullableColumns = new List(); var table = StoreObjectIdentifier.Table(tableName, index.DeclaringEntityType.GetSchema()); foreach (var property in index.Properties) { var columnName = property.GetColumnName(table); - if (columnName == null) + if (columnName == null + && !useOldBehavior) { return null; } diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs index df708483000..d1287374a88 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs @@ -129,12 +129,15 @@ private IConventionForeignKeyBuilder DiscoverProperties( IConventionForeignKeyBuilder relationshipBuilder, IConventionContext context) { + var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23092_1", out var isEnabled) && isEnabled; + var foreignKey = relationshipBuilder.Metadata; var foreignKeyProperties = FindCandidateForeignKeyProperties(relationshipBuilder.Metadata, onDependent: true); var propertiesConfigurationSource = foreignKey.GetPropertiesConfigurationSource(); if (!ConfigurationSource.Convention.OverridesStrictly(propertiesConfigurationSource) && (propertiesConfigurationSource != ConfigurationSource.Convention - || (foreignKey.Properties.All(p => !p.IsImplicitlyCreated()) + || (!useOldBehavior + && foreignKey.Properties.All(p => !p.IsImplicitlyCreated()) && (foreignKeyProperties == null || !foreignKey.Properties.SequenceEqual(foreignKeyProperties))))) {