From f875ecd53adfc1c34e4afd206c02caa4cb3d9614 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Tue, 28 Jul 2020 11:15:34 -0700 Subject: [PATCH] Discover self-referencing many-to-many relationships Fixes #21568 --- .../RelationshipDiscoveryConvention.cs | 135 ++++-------------- .../Internal/InternalEntityTypeBuilder.cs | 8 +- .../RelationshipDiscoveryConventionTest.cs | 15 +- 3 files changed, 40 insertions(+), 118 deletions(-) diff --git a/src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs index ba2155aaa40..dad6209e9a6 100644 --- a/src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs @@ -311,10 +311,10 @@ private static IReadOnlyList RemoveIncompatibleWithExisti } var noOtherCompatibleNavigation = true; - foreach (var n in relationshipCandidate.NavigationProperties) + foreach (var otherNavigation in relationshipCandidate.NavigationProperties) { - if (n != navigationProperty - && IsCompatibleInverse(n, compatibleInverse, entityTypeBuilder, targetEntityTypeBuilder)) + if (otherNavigation != navigationProperty + && IsCompatibleInverse(otherNavigation, compatibleInverse, entityTypeBuilder, targetEntityTypeBuilder)) { noOtherCompatibleNavigation = false; break; @@ -524,11 +524,9 @@ private void CreateRelationships( var entityType = entityTypeBuilder.Metadata; var targetEntityType = relationshipCandidate.TargetTypeBuilder.Metadata; var isAmbiguousOnBase = entityType.BaseType != null - && HasAmbiguousNavigationsTo( - entityType.BaseType, targetEntityType.ClrType) - || targetEntityType.BaseType != null - && HasAmbiguousNavigationsTo( - targetEntityType.BaseType, entityType.ClrType); + && HasAmbiguousNavigationsTo(entityType.BaseType, targetEntityType.ClrType) + || (targetEntityType.BaseType != null + && HasAmbiguousNavigationsTo(targetEntityType.BaseType, entityType.ClrType)); var ambiguousOwnership = relationshipCandidate.NavigationProperties.Count == 1 && relationshipCandidate.InverseProperties.Count == 1 @@ -631,15 +629,11 @@ private void CreateRelationships( { if (targetOwned) { - entityTypeBuilder.HasOwnership( - targetEntityType.ClrType, - navigation); + entityTypeBuilder.HasOwnership(targetEntityType.ClrType, navigation); } else { - entityTypeBuilder.HasRelationship( - targetEntityType, - navigation); + entityTypeBuilder.HasRelationship(targetEntityType, navigation); } } else @@ -668,20 +662,14 @@ private void CreateRelationships( if (targetOwned) { - entityTypeBuilder.HasOwnership( - targetEntityType.ClrType, - navigation, - inverse); + entityTypeBuilder.HasOwnership(targetEntityType.ClrType, navigation, inverse); } else { - if (entityTypeBuilder.HasRelationship( - targetEntityType, - navigation, - inverse) == null) + if (entityTypeBuilder.HasRelationship(targetEntityType, navigation, inverse) == null) { HasManyToManyRelationship( - entityTypeBuilder, relationshipCandidate, navigation, inverse); + entityTypeBuilder, relationshipCandidate.TargetTypeBuilder, navigation, inverse); } } } @@ -709,9 +697,7 @@ private void CreateRelationships( continue; } - targetEntityType.Builder.HasRelationship( - entityTypeBuilder.Metadata, - inverse); + targetEntityType.Builder.HasRelationship(entityTypeBuilder.Metadata, inverse); } } } @@ -766,7 +752,7 @@ private void RemoveNavigation( private void HasManyToManyRelationship( IConventionEntityTypeBuilder entityTypeBuilder, - RelationshipCandidate relationshipCandidate, + IConventionEntityTypeBuilder targetEntityTypeBuilder, PropertyInfo navigation, PropertyInfo inverse) { @@ -775,78 +761,39 @@ private void HasManyToManyRelationship( if (navigationTargetType == null || inverseTargetType == null) { - // these are not many-to-many navigations - return; - } - - if (navigationTargetType == inverseTargetType) - { - // do not automatically create many-to-many joins to self - return; - } - - var leftEntityType = entityTypeBuilder.Metadata; - var rightEntityType = relationshipCandidate.TargetTypeBuilder.Metadata; - if (navigationTargetType != rightEntityType.ClrType - || inverseTargetType != leftEntityType.ClrType) - { - // this relationship should be defined between different - // entity types further up at least one of their inheritance trees return; } - var navigationName = navigation.GetSimpleMemberName(); - var inverseName = inverse.GetSimpleMemberName(); - if (((EntityType)leftEntityType) - .FindNavigationsInHierarchy(navigationName).FirstOrDefault() != null - || ((EntityType)rightEntityType) - .FindNavigationsInHierarchy(inverseName).FirstOrDefault() != null) + var entityType = entityTypeBuilder.Metadata; + var targetEntityType = targetEntityTypeBuilder.Metadata; + if (navigationTargetType != targetEntityType.ClrType + || inverseTargetType != entityType.ClrType) { - // navigations have already been configured using these properties return; } - var leftSkipNavigation = leftEntityType.FindSkipNavigation(navigation) ?? - leftEntityType.AddSkipNavigation( - navigationName, navigation, rightEntityType, - collection: true, onDependent: false, fromDataAnnotation: false); - if (leftSkipNavigation == null) + var skipNavigationBuilder = entityTypeBuilder.HasSkipNavigation(navigation, targetEntityType); + if (skipNavigationBuilder == null) { return; } - var rightSkipNavigation = rightEntityType.FindSkipNavigation(inverse) ?? - rightEntityType.AddSkipNavigation( - inverseName, inverse, leftEntityType, - collection: true, onDependent: false, fromDataAnnotation: false); - if (rightSkipNavigation == null) + var inverseSkipNavigationBuilder = targetEntityTypeBuilder.HasSkipNavigation(inverse, entityType); + if (inverseSkipNavigationBuilder == null) { + entityTypeBuilder.HasNoSkipNavigation(skipNavigationBuilder.Metadata); return; } - // also sets rightSkipNavigation's inverse - leftSkipNavigation.Builder.HasInverse(rightSkipNavigation, fromDataAnnotation: false); - - // the implicit many-to-many join entity type will be created - // and configured by ManyToManyAssocationEntityTypeConvention. + skipNavigationBuilder.HasInverse(inverseSkipNavigationBuilder.Metadata, fromDataAnnotation: false); } - /// - /// Called after an entity type is added to the model. - /// - /// The builder for the entity type. - /// Additional information associated with convention execution. + /// public virtual void ProcessEntityTypeAdded( IConventionEntityTypeBuilder entityTypeBuilder, IConventionContext context) => DiscoverRelationships(entityTypeBuilder, context); - /// - /// Called after the base type of an entity type changes. - /// - /// The builder for the entity type. - /// The new base entity type. - /// The old base entity type. - /// Additional information associated with convention execution. + /// public virtual void ProcessEntityTypeBaseTypeChanged( IConventionEntityTypeBuilder entityTypeBuilder, IConventionEntityType newBaseType, @@ -894,14 +841,7 @@ private void ApplyOnRelatedEntityTypes(IConventionEntityType entityType, IConven } } - /// - /// Called after a navigation is removed from the entity type. - /// - /// The builder for the entity type that contained the navigation. - /// The builder for the target entity type of the navigation. - /// The navigation name. - /// The member used for by the navigation. - /// Additional information associated with convention execution. + /// public virtual void ProcessNavigationRemoved( IConventionEntityTypeBuilder sourceEntityTypeBuilder, IConventionEntityTypeBuilder targetEntityTypeBuilder, @@ -947,21 +887,13 @@ private static bool IsCandidateNavigationProperty( && (!sourceEntityTypeBuilder.Metadata.IsKeyless || (memberInfo as PropertyInfo)?.PropertyType.TryGetSequenceType() == null); - /// - /// Called after an entity type is ignored. - /// - /// The builder for the model. - /// The name of the ignored entity type. - /// The ignored entity type. - /// Additional information associated with convention execution. + /// public virtual void ProcessEntityTypeIgnored( IConventionModelBuilder modelBuilder, string name, Type type, IConventionContext context) { foreach (var entityType in modelBuilder.Metadata.GetEntityTypes()) { - // Only run the convention if an ambiguity might have been removed var ambiguityRemoved = RemoveAmbiguous(entityType, type); - if (ambiguityRemoved) { DiscoverRelationships(entityType.Builder, context); @@ -969,12 +901,7 @@ public virtual void ProcessEntityTypeIgnored( } } - /// - /// Called after an entity type member is ignored. - /// - /// The builder for the entity type. - /// The name of the ignored member. - /// Additional information associated with convention execution. + /// public virtual void ProcessEntityTypeMemberIgnored( IConventionEntityTypeBuilder entityTypeBuilder, string name, IConventionContext context) { @@ -1055,11 +982,7 @@ public virtual void ProcessNavigationAdded( } } - /// - /// Called after the ownership value for a foreign key is changed. - /// - /// The builder for the foreign key. - /// Additional information associated with convention execution. + /// public virtual void ProcessForeignKeyOwnershipChanged( IConventionForeignKeyBuilder relationshipBuilder, IConventionContext context) diff --git a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs index 2273266c51d..1ab19b2da9e 100644 --- a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs @@ -3565,12 +3565,8 @@ public virtual InternalSkipNavigationBuilder HasSkipNavigation( } else { - if (!configurationSource.HasValue) - { - return null; - } - - if (IsIgnored(navigationName, configurationSource)) + if (!configurationSource.HasValue + || IsIgnored(navigationName, configurationSource)) { return null; } diff --git a/test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs index 341c34a7675..fb047a9698e 100644 --- a/test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs @@ -239,7 +239,7 @@ public void Many_to_one_bidirectional_is_discovered() } [ConditionalFact] - public void Many_to_many_skip_navigations_are_not_discovered_if_self_join() + public void Many_to_many_skip_navigations_are_discovered_if_self_join() { var modelBuilder = CreateInternalModeBuilder(); var manyToManySelf = modelBuilder.Entity(typeof(ManyToManySelf), ConfigurationSource.Convention); @@ -248,7 +248,13 @@ public void Many_to_many_skip_navigations_are_not_discovered_if_self_join() RunConvention(manyToManySelf); - Assert.Empty(manyToManySelf.Metadata.GetSkipNavigations()); + Assert.Equal(2, manyToManySelf.Metadata.GetSkipNavigations().Count()); + var navigationOnManyToManyFirst = manyToManySelf.Metadata.GetSkipNavigations().First(); + var navigationOnManyToManySecond = manyToManySelf.Metadata.GetSkipNavigations().Last(); + Assert.Equal(nameof(ManyToManySelf.ManyToManySelf1), navigationOnManyToManyFirst.Name); + Assert.Equal(nameof(ManyToManySelf.ManyToManySelf2), navigationOnManyToManySecond.Name); + Assert.Same(navigationOnManyToManyFirst.Inverse, navigationOnManyToManySecond); + Assert.Same(navigationOnManyToManySecond.Inverse, navigationOnManyToManyFirst); } [ConditionalFact] @@ -268,7 +274,7 @@ public void Many_to_many_skip_navigations_are_not_discovered_if_relationship_sho } [ConditionalFact] - public void Many_to_many_bidirectional_sets_up_skip_navigations_but_not_join_entity_type() + public void Many_to_many_bidirectional_sets_up_skip_navigations() { var modelBuilder = CreateInternalModeBuilder(); var manyToManyFirst = modelBuilder.Entity(typeof(ManyToManyFirst), ConfigurationSource.Convention); @@ -285,9 +291,6 @@ public void Many_to_many_bidirectional_sets_up_skip_navigations_but_not_join_ent Assert.Equal("ManyToManyFirsts", navigationOnManyToManySecond.Name); Assert.Same(navigationOnManyToManyFirst.Inverse, navigationOnManyToManySecond); Assert.Same(navigationOnManyToManySecond.Inverse, navigationOnManyToManyFirst); - - Assert.Empty(manyToManyFirst.Metadata.Model.GetEntityTypes() - .Where(et => et.IsImplicitlyCreatedJoinEntityType)); } [ConditionalFact]