Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Discover self-referencing many-to-many relationships #21824

Merged
1 commit merged into from
Jul 28, 2020
Merged

Discover self-referencing many-to-many relationships #21824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 29 additions & 106 deletions src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,10 @@ private static IReadOnlyList<RelationshipCandidate> 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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -709,9 +697,7 @@ private void CreateRelationships(
continue;
}

targetEntityType.Builder.HasRelationship(
entityTypeBuilder.Metadata,
inverse);
targetEntityType.Builder.HasRelationship(entityTypeBuilder.Metadata, inverse);
}
}
}
Expand Down Expand Up @@ -766,7 +752,7 @@ private void RemoveNavigation(

private void HasManyToManyRelationship(
IConventionEntityTypeBuilder entityTypeBuilder,
RelationshipCandidate relationshipCandidate,
IConventionEntityTypeBuilder targetEntityTypeBuilder,
PropertyInfo navigation,
PropertyInfo inverse)
{
Expand All @@ -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);
}

/// <summary>
/// Called after an entity type is added to the model.
/// </summary>
/// <param name="entityTypeBuilder"> The builder for the entity type. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessEntityTypeAdded(
IConventionEntityTypeBuilder entityTypeBuilder, IConventionContext<IConventionEntityTypeBuilder> context)
=> DiscoverRelationships(entityTypeBuilder, context);

/// <summary>
/// Called after the base type of an entity type changes.
/// </summary>
/// <param name="entityTypeBuilder"> The builder for the entity type. </param>
/// <param name="newBaseType"> The new base entity type. </param>
/// <param name="oldBaseType"> The old base entity type. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessEntityTypeBaseTypeChanged(
IConventionEntityTypeBuilder entityTypeBuilder,
IConventionEntityType newBaseType,
Expand Down Expand Up @@ -894,14 +841,7 @@ private void ApplyOnRelatedEntityTypes(IConventionEntityType entityType, IConven
}
}

/// <summary>
/// Called after a navigation is removed from the entity type.
/// </summary>
/// <param name="sourceEntityTypeBuilder"> The builder for the entity type that contained the navigation. </param>
/// <param name="targetEntityTypeBuilder"> The builder for the target entity type of the navigation. </param>
/// <param name="navigationName"> The navigation name. </param>
/// <param name="memberInfo"> The member used for by the navigation. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessNavigationRemoved(
IConventionEntityTypeBuilder sourceEntityTypeBuilder,
IConventionEntityTypeBuilder targetEntityTypeBuilder,
Expand Down Expand Up @@ -947,34 +887,21 @@ private static bool IsCandidateNavigationProperty(
&& (!sourceEntityTypeBuilder.Metadata.IsKeyless
|| (memberInfo as PropertyInfo)?.PropertyType.TryGetSequenceType() == null);

/// <summary>
/// Called after an entity type is ignored.
/// </summary>
/// <param name="modelBuilder"> The builder for the model. </param>
/// <param name="name"> The name of the ignored entity type. </param>
/// <param name="type"> The ignored entity type. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessEntityTypeIgnored(
IConventionModelBuilder modelBuilder, string name, Type type, IConventionContext<string> 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);
}
}
}

/// <summary>
/// Called after an entity type member is ignored.
/// </summary>
/// <param name="entityTypeBuilder"> The builder for the entity type. </param>
/// <param name="name"> The name of the ignored member. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessEntityTypeMemberIgnored(
IConventionEntityTypeBuilder entityTypeBuilder, string name, IConventionContext<string> context)
{
Expand Down Expand Up @@ -1055,11 +982,7 @@ public virtual void ProcessNavigationAdded(
}
}

/// <summary>
/// Called after the ownership value for a foreign key is changed.
/// </summary>
/// <param name="relationshipBuilder"> The builder for the foreign key. </param>
/// <param name="context"> Additional information associated with convention execution. </param>
/// <inheritdoc />
public virtual void ProcessForeignKeyOwnershipChanged(
IConventionForeignKeyBuilder relationshipBuilder,
IConventionContext<bool?> context)
Expand Down
8 changes: 2 additions & 6 deletions src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
15 changes: 9 additions & 6 deletions test/EFCore.Tests/Metadata/Conventions/RelationshipDiscoveryConventionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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]
Expand All @@ -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);
Expand All @@ -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]
Expand Down