Skip to content

Commit

Permalink
Pull down relationship to the derived type when setting a navigation …
Browse files Browse the repository at this point in the history 
…targeting the derived type.

Remove ambiguous navigations when the target type is ignored or a base type is set that has the navigation ignored.
Throw correct exception when the collection navigation type doesn't match the dependent entity type.

Fixes #16762
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Jul 31, 2019
1 parent f500feb commit 8ecbf2f
Show file tree
Hide file tree
Showing 12 changed files with 251 additions and 87 deletions.
25 changes: 25 additions & 0 deletions src/EFCore/Extensions/ConventionEntityTypeExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using JetBrains.Annotations;
Expand Down Expand Up @@ -52,6 +53,30 @@ public static IEnumerable<IConventionEntityType> GetDerivedTypesInclusive([NotNu
public static IEnumerable<IConventionEntityType> GetDirectlyDerivedTypes([NotNull] this IConventionEntityType entityType)
=> ((EntityType)entityType).GetDirectlyDerivedTypes();

/// <summary>
/// Returns all base types of the given <see cref="IEntityType" />, including the type itself, top to bottom.
/// </summary>
/// <param name="entityType"> The entity type. </param>
/// <returns> Base types. </returns>
public static IEnumerable<IConventionEntityType> GetAllBaseTypesInclusive([NotNull] this IConventionEntityType entityType)
=> GetAllBaseTypesInclusiveAscending(entityType).Reverse();

/// <summary>
/// Returns all base types of the given <see cref="IEntityType" />, including the type itself, bottom to top.
/// </summary>
/// <param name="entityType"> The entity type. </param>
/// <returns> Base types. </returns>
public static IEnumerable<IConventionEntityType> GetAllBaseTypesInclusiveAscending([NotNull] this IConventionEntityType entityType)
{
Check.NotNull(entityType, nameof(entityType));

while (entityType != null)
{
yield return entityType;
entityType = entityType.BaseType;
}
}

/// <summary>
/// <para>
/// Gets all keys declared on the given <see cref="IEntityType" />.
Expand Down
14 changes: 2 additions & 12 deletions src/EFCore/Metadata/Builders/ReferenceNavigationBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,23 +178,13 @@ private InternalRelationshipBuilder WithManyBuilder(MemberIdentity collection)
ThrowForConflictingNavigation(foreignKey, collectionName, false);
}

return RelatedEntityType != foreignKey.PrincipalEntityType
? collection.MemberInfo == null && ReferenceMember == null
return collection.MemberInfo == null || ReferenceMember == null
? builder.HasNavigations(
ReferenceName, collection.Name,
(EntityType)RelatedEntityType, (EntityType)DeclaringEntityType, ConfigurationSource.Explicit)
: builder.HasNavigations(
ReferenceMember, collection.MemberInfo,
(EntityType)RelatedEntityType, (EntityType)DeclaringEntityType, ConfigurationSource.Explicit)
: collection.MemberInfo != null
? builder.HasNavigation(
collection.MemberInfo,
pointsToPrincipal: false,
ConfigurationSource.Explicit)
: builder.HasNavigation(
collection.Name,
pointsToPrincipal: false,
ConfigurationSource.Explicit);
(EntityType)RelatedEntityType, (EntityType)DeclaringEntityType, ConfigurationSource.Explicit);
}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public virtual ConventionSet CreateConventionSet()
conventionSet.EntityTypeAddedConventions.Add(new DerivedTypeDiscoveryConvention(Dependencies));

conventionSet.EntityTypeIgnoredConventions.Add(inversePropertyAttributeConvention);
conventionSet.EntityTypeIgnoredConventions.Add(relationshipDiscoveryConvention);

var discriminatorConvention = new DiscriminatorConvention(Dependencies);
conventionSet.EntityTypeRemovedConventions.Add(new OwnedTypesConvention(Dependencies));
Expand Down
101 changes: 69 additions & 32 deletions src/EFCore/Metadata/Conventions/RelationshipDiscoveryConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
/// </summary>
public class RelationshipDiscoveryConvention :
IEntityTypeAddedConvention,
IEntityTypeIgnoredConvention,
IEntityTypeBaseTypeChangedConvention,
INavigationRemovedConvention,
IEntityTypeMemberIgnoredConvention,
Expand Down Expand Up @@ -770,15 +771,24 @@ public virtual void ProcessEntityTypeBaseTypeChanged(
DiscoverRelationships(oldBaseTypeBuilder, context);
}

if (entityTypeBuilder.Metadata.BaseType != newBaseType)
var entityType = entityTypeBuilder.Metadata;
if (entityType.BaseType != newBaseType)
{
return;
}

ApplyOnRelatedEntityTypes(entityTypeBuilder.Metadata, context);
foreach (var entityType in entityTypeBuilder.Metadata.GetDerivedTypesInclusive())
if (newBaseType != null)
{
DiscoverRelationships(entityType.Builder, context);
foreach (var ignoredMember in newBaseType.GetAllBaseTypesInclusive().SelectMany(et => et.GetIgnoredMembers()))
{
ProcessEntityTypeMemberIgnoredOnBase(entityType, ignoredMember);
}
}

ApplyOnRelatedEntityTypes(entityType, context);
foreach (var derivedEntityType in entityType.GetDerivedTypesInclusive())
{
DiscoverRelationships(derivedEntityType.Builder, context);
}
}

Expand Down Expand Up @@ -849,6 +859,28 @@ 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>
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>
Expand All @@ -861,43 +893,48 @@ public virtual void ProcessEntityTypeMemberIgnored(
var anyAmbiguityRemoved = false;
foreach (var derivedEntityType in entityTypeBuilder.Metadata.GetDerivedTypesInclusive())
{
var ambiguousNavigations = GetAmbiguousNavigations(derivedEntityType);
if (ambiguousNavigations == null)
{
continue;
}

KeyValuePair<MemberInfo, Type>? ambiguousNavigation = null;
foreach (var navigation in ambiguousNavigations)
{
if (navigation.Key.GetSimpleMemberName() == name)
{
ambiguousNavigation = navigation;
}
}

if (ambiguousNavigation == null)
{
continue;
}
anyAmbiguityRemoved |= ProcessEntityTypeMemberIgnoredOnBase(derivedEntityType, name);
}

anyAmbiguityRemoved = true;
if (anyAmbiguityRemoved)
{
DiscoverRelationships(entityTypeBuilder, context);
}
}

var targetClrType = ambiguousNavigation.Value.Value;
RemoveAmbiguous(derivedEntityType, targetClrType);
private bool ProcessEntityTypeMemberIgnoredOnBase(IConventionEntityType entityType, string name)
{
var ambiguousNavigations = GetAmbiguousNavigations(entityType);
if (ambiguousNavigations == null)
{
return false;
}

var targetType = ((InternalEntityTypeBuilder)entityTypeBuilder)
.GetTargetEntityTypeBuilder(targetClrType, ambiguousNavigation.Value.Key, null)?.Metadata;
if (targetType != null)
KeyValuePair<MemberInfo, Type>? ambiguousNavigation = null;
foreach (var navigation in ambiguousNavigations)
{
if (navigation.Key.GetSimpleMemberName() == name)
{
RemoveAmbiguous(targetType, derivedEntityType.ClrType);
ambiguousNavigation = navigation;
}
}

if (anyAmbiguityRemoved)
if (ambiguousNavigation == null)
{
DiscoverRelationships(entityTypeBuilder, context);
return false;
}

var targetClrType = ambiguousNavigation.Value.Value;
RemoveAmbiguous(entityType, targetClrType);

var targetType = ((InternalEntityTypeBuilder)entityType.Builder)
.GetTargetEntityTypeBuilder(targetClrType, ambiguousNavigation.Value.Key, null)?.Metadata;
if (targetType != null)
{
RemoveAmbiguous(targetType, entityType.ClrType);
}

return true;
}

/// <summary>
Expand Down
18 changes: 15 additions & 3 deletions src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ private InternalPropertyBuilder Property(
ConfigurationSource? typeConfigurationSource,
ConfigurationSource? configurationSource)
{
IEnumerable<Property> propertiesToDetach = null;
List<Property> propertiesToDetach = null;
var existingProperty = Metadata.FindProperty(propertyName);
if (existingProperty != null)
{
Expand All @@ -456,7 +456,7 @@ private InternalPropertyBuilder Property(
{
if (configurationSource.Overrides(existingProperty.GetConfigurationSource()))
{
propertiesToDetach = new[]
propertiesToDetach = new List<Property>
{
existingProperty
};
Expand Down Expand Up @@ -527,7 +527,19 @@ private InternalPropertyBuilder Property(

Metadata.RemoveIgnored(propertyName);

propertiesToDetach = Metadata.FindDerivedProperties(propertyName);
foreach (var derivedType in Metadata.GetDerivedTypes())
{
var derivedProperty = derivedType.FindDeclaredProperty(propertyName);
if (derivedProperty != null)
{
if (propertiesToDetach == null)
{
propertiesToDetach = new List<Property>();
}

propertiesToDetach.Add(derivedProperty);
}
}
}

InternalPropertyBuilder builder;
Expand Down
49 changes: 43 additions & 6 deletions src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,8 @@ private InternalRelationshipBuilder HasNavigations(
out var shouldInvert,
out var shouldBeUnique,
out var removeOppositeNavigation,
out var removeConflictingNavigations))
out var removeConflictingNavigations,
out var changeRelatedTypes))
{
return null;
}
Expand Down Expand Up @@ -280,7 +281,8 @@ private InternalRelationshipBuilder HasNavigations(

InternalRelationshipBuilder builder;
if (shouldInvert == true
|| removeConflictingNavigations)
|| removeConflictingNavigations
|| changeRelatedTypes)
{
builder = ReplaceForeignKey(
configurationSource,
Expand All @@ -291,7 +293,7 @@ private InternalRelationshipBuilder HasNavigations(
dependentProperties,
principalProperties: principalProperties,
isUnique: shouldBeUnique,
removeCurrent: shouldInvert ?? false,
removeCurrent: shouldInvert == true || changeRelatedTypes,
principalEndConfigurationSource: shouldInvert != null ? configurationSource : (ConfigurationSource?)null,
oldRelationshipInverted: shouldInvert == true);

Expand Down Expand Up @@ -546,7 +548,8 @@ private bool CanSetNavigations(
out shouldInvert,
out shouldBeUnique,
out removeOppositeNavigation,
out removeConflictingNavigations);
out removeConflictingNavigations,
out _);

private bool CanSetNavigations(
MemberIdentity? navigationToPrincipal,
Expand All @@ -559,12 +562,14 @@ private bool CanSetNavigations(
out bool? shouldInvert,
out bool? shouldBeUnique,
out bool removeOppositeNavigation,
out bool removeConflictingNavigations)
out bool removeConflictingNavigations,
out bool changeRelatedTypes)
{
shouldInvert = null;
shouldBeUnique = null;
removeOppositeNavigation = false;
removeConflictingNavigations = false;
changeRelatedTypes = false;

if ((navigationToPrincipal == null
|| navigationToPrincipal.Value.Name == Metadata.DependentToPrincipal?.Name)
Expand Down Expand Up @@ -736,6 +741,38 @@ private bool CanSetNavigations(
dependentProperties: null,
principalProperties: null).Where(r => r.Metadata != Metadata).Distinct().Any();

if (shouldInvert == false
&& !removeConflictingNavigations
&& (principalEntityType != Metadata.PrincipalEntityType
|| dependentEntityType != Metadata.DeclaringEntityType))
{
if (navigationToPrincipalProperty != null
&& !IsCompatible(
navigationToPrincipalProperty,
pointsToPrincipal: true,
Metadata.DeclaringEntityType.ClrType,
Metadata.PrincipalEntityType.ClrType,
shouldThrow: false,
out _))
{
changeRelatedTypes = true;
return true;
}

if (navigationToDependentProperty != null
&& !IsCompatible(
navigationToDependentProperty,
pointsToPrincipal: false,
Metadata.DeclaringEntityType.ClrType,
Metadata.PrincipalEntityType.ClrType,
shouldThrow: false,
out _))
{
changeRelatedTypes = true;
return true;
}
}

return true;
}

Expand Down Expand Up @@ -775,7 +812,7 @@ private static bool IsCompatible(
navigationProperty,
principalType,
dependentType,
shouldBeCollection: false,
shouldBeCollection: null,
shouldThrow: true);
}

Expand Down
Loading

0 comments on commit 8ecbf2f

Please sign in to comment.