From 9dab5d8fc528b3d0b2e64b26ddd01402754efcc7 Mon Sep 17 00:00:00 2001 From: AndriySvyryd Date: Tue, 13 Aug 2019 14:48:38 -0700 Subject: [PATCH] Try match FK properties with the exact name as principal key if they are a proper subset of the dependent PK and don't contain the Id property. Fixes #15250 --- .../ChangeTracking/Internal/ChangeDetector.cs | 2 - .../Internal/NavigationFixer.cs | 1 - .../ForeignKeyPropertyDiscoveryConvention.cs | 92 +++++++++++-------- ...reignKeyPropertyDiscoveryConventionTest.cs | 68 +++++++++++++- 4 files changed, 121 insertions(+), 42 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/ChangeDetector.cs b/src/EFCore/ChangeTracking/Internal/ChangeDetector.cs index 8f07ae9053d..ad547fc13f2 100644 --- a/src/EFCore/ChangeTracking/Internal/ChangeDetector.cs +++ b/src/EFCore/ChangeTracking/Internal/ChangeDetector.cs @@ -144,7 +144,6 @@ public virtual void DetectChanges(IStateManager stateManager) _logger.DetectChangesStarting(stateManager.Context); foreach (var entry in stateManager.ToList()) // Might be too big, but usually _all_ entities are using Snapshot tracking - { if (entry.EntityType.GetChangeTrackingStrategy() == ChangeTrackingStrategy.Snapshot && entry.EntityState != EntityState.Detached) @@ -299,7 +298,6 @@ private void DetectNavigationChange(InternalEntityEntry entry, INavigation navig } var added = new HashSet(ReferenceEqualityComparer.Instance); - if (currentCollection != null) { foreach (var entity in currentCollection) diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index d81c5c620dd..fef003874cb 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs @@ -258,7 +258,6 @@ public virtual void NavigationCollectionChanged( foreach (var newValue in added) { var newTargetEntry = stateManager.GetOrCreateEntry(newValue, targetEntityType); - if (newTargetEntry.EntityState != EntityState.Detached) { try diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs index be00dea8e21..6a6835ab000 100644 --- a/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs @@ -16,14 +16,19 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions /// /// /// A convention that finds foreign key properties for relationships based on their names, ignoring case: - /// * [navigation property name][primary key property name] + /// * [navigation property name][principal key property name] /// * [navigation property name]Id - /// * [principal entity name][primary key property name] + /// * [principal entity name][principal key property name] /// * [principal entity name]Id /// /// - /// If no matching properties were found, the relationship is one-to-one, doesn't represent an ownership, - /// the dependent side is not ambiguous and not derived then the primary key properties are used. + /// If no matching properties were found, the relationship doesn't represent an ownership, + /// the dependent side is not ambiguous and not derived then if the relationship is one-to-one, + /// the primary key properties are used, otherwise the convention tries to match properties with + /// the exact name as principal key properties if they are a proper subset of the dependent PK. + /// + /// + /// /// /// /// If a match was found, but the property types are not compatible with the principal key types no further matches are searched for. @@ -189,41 +194,57 @@ private IConventionRelationshipBuilder DiscoverProperties( } } - if (foreignKey.IsUnique - && foreignKey.DeclaringEntityType.BaseType == null + if (foreignKey.DeclaringEntityType.BaseType == null && !foreignKey.IsSelfReferencing()) { - // Try to use PK properties if principal end is not ambiguous - if (!foreignKey.IsOwnership - && (!ConfigurationSource.Convention.Overrides(foreignKey.GetPrincipalEndConfigurationSource()) - || foreignKey.DeclaringEntityType.DefiningEntityType == foreignKey.PrincipalEntityType)) - { - foreignKeyProperties = GetCompatiblePrimaryKeyProperties( - foreignKey.DeclaringEntityType, - foreignKey.PrincipalEntityType, - foreignKey.PrincipalKey.Properties); - } - else if (invertible) + if (foreignKey.IsUnique) { - foreignKeyProperties = FindCandidateForeignKeyProperties(foreignKey, onDependent: true, matchPk: true); - var candidatePropertiesOnPrincipal = - FindCandidateForeignKeyProperties(foreignKey, onDependent: false, matchPk: true); - if (candidatePropertiesOnPrincipal != null) + // Try to use PK properties if principal end is not ambiguous + if (!foreignKey.IsOwnership + && (!ConfigurationSource.Convention.Overrides(foreignKey.GetPrincipalEndConfigurationSource()) + || foreignKey.DeclaringEntityType.DefiningEntityType == foreignKey.PrincipalEntityType)) { - if (foreignKeyProperties == null) + foreignKeyProperties = GetCompatiblePrimaryKeyProperties( + foreignKey.DeclaringEntityType, + foreignKey.PrincipalEntityType, + foreignKey.PrincipalKey.Properties); + } + else if (invertible) + { + foreignKeyProperties = FindCandidateForeignKeyProperties(foreignKey, onDependent: true, matchPk: true); + var candidatePropertiesOnPrincipal = + FindCandidateForeignKeyProperties(foreignKey, onDependent: false, matchPk: true); + if (candidatePropertiesOnPrincipal != null) { - using (var batch = context.DelayConventions()) + if (foreignKeyProperties == null) { - var invertedRelationshipBuilder = relationshipBuilder - .HasEntityTypes(foreignKey.DeclaringEntityType, foreignKey.PrincipalEntityType); - return batch.Run( - invertedRelationshipBuilder.HasForeignKey(candidatePropertiesOnPrincipal).Metadata) - ?.Builder; + using (var batch = context.DelayConventions()) + { + var invertedRelationshipBuilder = relationshipBuilder + .HasEntityTypes(foreignKey.DeclaringEntityType, foreignKey.PrincipalEntityType); + return batch.Run( + invertedRelationshipBuilder.HasForeignKey(candidatePropertiesOnPrincipal).Metadata) + ?.Builder; + } } - } + foreignKeyProperties = null; + ((ForeignKey)relationshipBuilder.Metadata).SetPrincipalEndConfigurationSource(null); + } + } + } + else + { + // Try match properties with the exact name as principal key if they are a proper subset of the dependent PK + var dependentPk = foreignKey.DeclaringEntityType.FindPrimaryKey(); + if (dependentPk != null + && dependentPk.Properties.Count > foreignKey.PrincipalKey.Properties.Count + && TryFindMatchingProperties(foreignKey, "", onDependent: true, matchPk: false, out foreignKeyProperties) + && foreignKeyProperties != null + && foreignKeyProperties.Any(p => !dependentPk.Properties.Contains(p) + || p.Name.Equals("Id", StringComparison.OrdinalIgnoreCase))) + { foreignKeyProperties = null; - ((ForeignKey)relationshipBuilder.Metadata).SetPrincipalEndConfigurationSource(null); } } } @@ -412,17 +433,12 @@ private bool TryFindMatchingProperties( isKeyContainedInForeignKey = false; break; } - - if (!foreignKey.IsUnique) - { - // Stop searching if match found, but is incompatible - return true; - } } if (isKeyContainedInForeignKey - && key.IsPrimaryKey() - && !matchPk) + && (!foreignKey.IsUnique + || (key.IsPrimaryKey() + && !matchPk))) { // Stop searching if match found, but is incompatible return true; diff --git a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs index 9ef2686dc37..4b8dd84c4ec 100644 --- a/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs @@ -374,7 +374,7 @@ public void Does_not_match_PK_name_properties() var relationshipBuilder = dependentTypeBuilder.HasRelationship( PrincipalTypeWithCompositeKey, - "NavProp", + nameof(DependentEntityWithCompositeKey.NavProp), null, ConfigurationSource.Convention); @@ -390,6 +390,72 @@ public void Does_not_match_PK_name_properties() ValidateModel(); } + [ConditionalFact] + public void Does_not_match_PK_name_properties_if_subset_of_dependent_PK_and_contains_id() + { + var dependentTypeBuilder = DependentTypeWithCompositeKey.Builder; + var pkProperty1 = dependentTypeBuilder.Property( + DependentEntityWithCompositeKey.IdProperty, ConfigurationSource.Convention) + .Metadata; + var pkProperty2 = dependentTypeBuilder.Property( + DependentEntityWithCompositeKey.NameProperty, ConfigurationSource.Convention) + .IsRequired(true, ConfigurationSource.Convention) + .Metadata; + var pkProperty3 = dependentTypeBuilder.Property( + DependentEntityWithCompositeKey.NavPropIdProperty, ConfigurationSource.Convention) + .Metadata; + + dependentTypeBuilder.PrimaryKey(new[] { pkProperty1, pkProperty2, pkProperty3 }, ConfigurationSource.Explicit); + + var relationshipBuilder = dependentTypeBuilder.HasRelationship( + PrincipalTypeWithCompositeKey, + nameof(DependentEntityWithCompositeKey.NavProp), + null, + ConfigurationSource.Convention); + + var newRelationshipBuilder = RunConvention(relationshipBuilder); + + var fk = (IForeignKey)DependentTypeWithCompositeKey.GetForeignKeys().Single(); + Assert.Same(fk, newRelationshipBuilder.Metadata); + Assert.Equal("NavProp" + CompositePrimaryKey[0].Name + "1", fk.Properties[0].Name); + Assert.Equal("NavProp" + CompositePrimaryKey[1].Name + "1", fk.Properties[1].Name); + Assert.Same(CompositePrimaryKey[0], fk.PrincipalKey.Properties[0]); + Assert.Same(CompositePrimaryKey[1], fk.PrincipalKey.Properties[1]); + + ValidateModel(); + } + + [ConditionalFact] + public void Matches_PK_name_properties_if_subset_of_dependent_PK() + { + var dependentTypeBuilder = DependentType.Builder; + dependentTypeBuilder.PrimaryKey(new[] { DependentEntity.PrincipalEntityPeEKaYProperty }, ConfigurationSource.Explicit); + var pkProperty = dependentTypeBuilder.Property( + DependentEntity.IDProperty, ConfigurationSource.Convention) + .IsRequired(true, ConfigurationSource.Convention) + .Metadata; + var fkProperty = dependentTypeBuilder.Property( + DependentEntity.PeEKaYProperty, ConfigurationSource.Convention) + .Metadata; + + dependentTypeBuilder.PrimaryKey(new[] { pkProperty, fkProperty }, ConfigurationSource.Explicit); + + var relationshipBuilder = dependentTypeBuilder.HasRelationship( + PrincipalType, + nameof(DependentEntity.SomeNav), + null, + ConfigurationSource.Convention); + + var newRelationshipBuilder = RunConvention(relationshipBuilder); + + var fk = (IForeignKey)DependentType.GetForeignKeys().Single(); + Assert.Same(fk, newRelationshipBuilder.Metadata); + Assert.Same(fkProperty, fk.Properties[0]); + Assert.Same(PrimaryKey, fk.PrincipalKey.Properties[0]); + + ValidateModel(); + } + [ConditionalFact] public void Matches_dependent_PK_for_unique_FK_set_by_higher_source_than_convention() {