Skip to content

Commit

Permalink
Try match FK properties with the exact name as principal key if they …
Browse files Browse the repository at this point in the history 
…are a proper subset of the dependent PK and don't contain the Id property.

Fixes #15250
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Aug 13, 2019
1 parent 29b269a commit 3055bbb
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 42 deletions.
2 changes: 0 additions & 2 deletions src/EFCore/ChangeTracking/Internal/ChangeDetector.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -299,7 +298,6 @@ private void DetectNavigationChange(InternalEntityEntry entry, INavigation navig
}

var added = new HashSet<object>(ReferenceEqualityComparer.Instance);

if (currentCollection != null)
{
foreach (var entity in currentCollection)
Expand Down
1 change: 0 additions & 1 deletion src/EFCore/ChangeTracking/Internal/NavigationFixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
92 changes: 54 additions & 38 deletions src/EFCore/Metadata/Conventions/ForeignKeyPropertyDiscoveryConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,19 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
/// <summary>
/// <para>
/// 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
/// </para>
/// <para>
/// 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, doesn't represent an ownership,
/// the dependent side is not ambiguous and not derived then if the 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.
/// </para>
/// <para>
///
/// </para>
/// <para>
/// If a match was found, but the property types are not compatible with the principal key types no further matches are searched for.
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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;
Expand Down
68 changes: 67 additions & 1 deletion test/EFCore.Tests/Metadata/Conventions/ForeignKeyPropertyDiscoveryConventionTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public void Does_not_match_PK_name_properties()

var relationshipBuilder = dependentTypeBuilder.HasRelationship(
PrincipalTypeWithCompositeKey,
"NavProp",
nameof(DependentEntityWithCompositeKey.NavProp),
null,
ConfigurationSource.Convention);

Expand All @@ -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()
{
Expand Down

0 comments on commit 3055bbb

Please sign in to comment.