Skip to content

Commit

Permalink
Don't discover collection of string as navigation
Browse files Browse the repository at this point in the history 
Handle OwnedAttribute correctly
Convert types to owned in a more robust way

Fixes #11074
Fixes #11152
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Mar 14, 2018
1 parent 5b6e178 commit 91eeff8
Show file tree
Hide file tree
Showing 15 changed files with 208 additions and 77 deletions.
10 changes: 8 additions & 2 deletions src/EFCore.Specification.Tests/DataAnnotationTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2089,10 +2089,16 @@ public virtual void OwnedEntityTypeAttribute_configures_all_references_as_owned(
modelBuilder.Entity<Book>();
modelBuilder.Entity<One>();

Validate(modelBuilder);

Assert.True(model.FindEntityType(typeof(Book)).FindNavigation(nameof(Book.AdditionalDetails)).ForeignKey.IsOwnership);
var one = model.FindEntityType(typeof(One));
Assert.True(one.FindNavigation(nameof(One.Details)).ForeignKey.IsOwnership);
Assert.True(one.FindNavigation(nameof(One.AdditionalDetails)).ForeignKey.IsOwnership);
var ownership1 = one.FindNavigation(nameof(One.Details)).ForeignKey;
Assert.True(ownership1.IsOwnership);
Assert.NotNull(ownership1.DeclaringEntityType.FindProperty(nameof(Details.Name)));
var ownership2 = one.FindNavigation(nameof(One.AdditionalDetails)).ForeignKey;
Assert.True(ownership2.IsOwnership);
Assert.NotNull(ownership2.DeclaringEntityType.FindProperty(nameof(Details.Name)));
}

public abstract class DataAnnotationFixtureBase : SharedStoreFixtureBase<DbContext>
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/Metadata/Conventions/Internal/BaseTypeDiscoveryConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public virtual InternalEntityTypeBuilder Apply(InternalEntityTypeBuilder entityT

var baseEntityType = FindClosestBaseType(entityType);
if (baseEntityType == null
|| baseEntityType.FindOwnership() != null)
|| baseEntityType.DefiningNavigationName != null)
{
return entityTypeBuilder;
}
Expand Down
9 changes: 6 additions & 3 deletions src/EFCore/Metadata/Conventions/Internal/ForeignKeyPropertyDiscoveryConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public virtual InternalRelationshipBuilder Apply(InternalRelationshipBuilder rel
if (foreignKey.DeclaringEntityType.DefiningEntityType == foreignKey.PrincipalEntityType
|| foreignKey.IsOwnership
|| foreignKey.DeclaringEntityType.IsQueryType
|| foreignKey.IsSelfReferencing()
|| foreignKey.PrincipalToDependent?.IsCollection() == true
|| foreignKey.DeclaringEntityType.FindOwnership() != null)
{
relationshipBuilder = relationshipBuilder.RelatedEntityTypes(
Expand Down Expand Up @@ -180,16 +182,17 @@ private InternalRelationshipBuilder SetForeignKeyProperties(
var foreignKey = relationshipBuilder.Metadata;
if (ConfigurationSource.Convention.Overrides(foreignKey.GetPrincipalEndConfigurationSource())
&& foreignKey.DeclaringEntityType.DefiningEntityType != foreignKey.PrincipalEntityType
&& !foreignKey.DeclaringEntityType.IsQueryType
&& !foreignKey.IsOwnership
&& !foreignKey.DeclaringEntityType.IsQueryType
&& !foreignKey.IsSelfReferencing()
&& (foreignKey.PrincipalToDependent?.IsCollection() != true))
&& foreignKey.PrincipalToDependent?.IsCollection() != true
&& foreignKey.DeclaringEntityType.FindOwnership() == null)
{
var candidatePropertiesOnPrincipal = FindCandidateForeignKeyProperties(foreignKey, onDependent: false);
if (candidatePropertiesOnPrincipal != null
&& !foreignKey.PrincipalEntityType.FindForeignKeysInHierarchy(candidatePropertiesOnPrincipal).Any())
{
// Ambiguous principal end
// Principal end became ambiguous
if (relationshipBuilder.Metadata.GetPrincipalEndConfigurationSource() == ConfigurationSource.Convention)
{
relationshipBuilder.Metadata.SetPrincipalEndConfigurationSource(null);
Expand Down
17 changes: 11 additions & 6 deletions src/EFCore/Metadata/Conventions/Internal/OwnedEntityTypeAttributeConvention.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,16 @@ public class OwnedEntityTypeAttributeConvention : EntityTypeAttributeConvention<
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public override InternalEntityTypeBuilder Apply(InternalEntityTypeBuilder entityTypeBuilder, OwnedAttribute attribute)
=> (entityTypeBuilder.Metadata.HasClrType()
? entityTypeBuilder.ModelBuilder.Owned(entityTypeBuilder.Metadata.ClrType, ConfigurationSource.DataAnnotation)
: entityTypeBuilder.ModelBuilder.Owned(entityTypeBuilder.Metadata.Name, ConfigurationSource.DataAnnotation))
&& !entityTypeBuilder.Metadata.HasDefiningNavigation()
? null
: entityTypeBuilder;
{
if (entityTypeBuilder.Metadata.HasClrType())
{
entityTypeBuilder.ModelBuilder.Owned(entityTypeBuilder.Metadata.ClrType, ConfigurationSource.DataAnnotation);
}
else
{
entityTypeBuilder.ModelBuilder.Owned(entityTypeBuilder.Metadata.Name, ConfigurationSource.DataAnnotation);
}
return entityTypeBuilder.Metadata.Builder;
}
}
}
11 changes: 9 additions & 2 deletions src/EFCore/Metadata/Internal/EntityType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ public virtual IEnumerable<ForeignKey> GetDerivedForeignKeys()
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public virtual IEnumerable<ForeignKey> GetDerivedForeignKeysInclusive()
=> GetDeclaredForeignKeys().Concat(GetDerivedTypes().SelectMany(et => et.GetDeclaredForeignKeys()));
=> GetDeclaredForeignKeys().Concat(GetDerivedForeignKeys());

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
Expand Down Expand Up @@ -1152,12 +1152,19 @@ public virtual IEnumerable<ForeignKey> GetReferencingForeignKeys()
public virtual IEnumerable<ForeignKey> GetDeclaredReferencingForeignKeys()
=> DeclaredReferencingForeignKeys ?? Enumerable.Empty<ForeignKey>();

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public virtual IEnumerable<ForeignKey> GetDerivedReferencingForeignKeys()
=> GetDerivedTypes().SelectMany(et => et.GetDeclaredReferencingForeignKeys());

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public virtual IEnumerable<ForeignKey> GetDerivedReferencingForeignKeysInclusive()
=> GetDeclaredReferencingForeignKeys().Concat(GetDerivedTypes().SelectMany(et => et.GetDeclaredReferencingForeignKeys()));
=> GetDeclaredReferencingForeignKeys().Concat(GetDerivedReferencingForeignKeys());

private SortedSet<ForeignKey> DeclaredReferencingForeignKeys { get; set; }

Expand Down
4 changes: 2 additions & 2 deletions src/EFCore/Metadata/Internal/EntityTypeExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ public static bool IsInDefinitionPath([NotNull] this IEntityType entityType, [No
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public static bool IsInOwnershipPath([NotNull] this EntityType entityType, [NotNull] EntityType entityTypeToOwn)
public static bool IsInOwnershipPath([NotNull] this EntityType entityType, [NotNull] EntityType targetType)
{
var owner = entityType;
while (true)
Expand All @@ -267,7 +267,7 @@ public static bool IsInOwnershipPath([NotNull] this EntityType entityType, [NotN
}

owner = ownOwnership.PrincipalEntityType;
if (owner == entityTypeToOwn)
if (owner == targetType)
{
return true;
}
Expand Down
27 changes: 27 additions & 0 deletions src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,7 @@ public virtual InternalEntityTypeBuilder HasBaseType([CanBeNull] EntityType base

List<InternalIndexBuilder> detachedIndexes = null;
HashSet<Property> removedInheritedPropertiesToDuplicate = null;
List< (string, ConfigurationSource)> membersToIgnore = null;
if (Metadata.BaseType != null)
{
var removedInheritedProperties = new HashSet<Property>(
Expand Down Expand Up @@ -843,10 +844,36 @@ public virtual InternalEntityTypeBuilder HasBaseType([CanBeNull] EntityType base
}
}
}

foreach (var ignoredMember in Metadata.BaseType.GetIgnoredMembers())
{
if (baseEntityType != null
&& (baseEntityType.FindProperty(ignoredMember) != null
|| baseEntityType.FindNavigation(ignoredMember) != null))
{
continue;
}

if (membersToIgnore == null)
{
membersToIgnore = new List<(string, ConfigurationSource)>();
}

membersToIgnore.Add(
(ignoredMember, Metadata.BaseType.FindDeclaredIgnoredMemberConfigurationSource(ignoredMember).Value));
}
}

Metadata.HasBaseType(baseEntityType, configurationSource);

if (membersToIgnore != null)
{
foreach (var ignoreTuple in membersToIgnore)
{
Ignore(ignoreTuple.Item1, ignoreTuple.Item2);
}
}

if (removedInheritedPropertiesToDuplicate != null)
{
foreach (var property in removedInheritedPropertiesToDuplicate)
Expand Down
14 changes: 6 additions & 8 deletions src/EFCore/Metadata/Internal/InternalModelBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,7 @@ public virtual bool Owned(
[NotNull] Type type, ConfigurationSource configurationSource)
=> Owned(new TypeIdentity(type), configurationSource);

private bool Owned(
TypeIdentity type, ConfigurationSource configurationSource)
private bool Owned(TypeIdentity type, ConfigurationSource configurationSource)
{
if (IsIgnored(type, configurationSource))
{
Expand All @@ -292,12 +291,11 @@ private bool Owned(
throw new InvalidOperationException(CoreStrings.ClashingNonOwnedEntityType(entityType.DisplayName()));
}

var potentialOwnerships = entityType.GetForeignKeys().Where(fk => fk.PrincipalToDependent != null).ToList();
foreach (var foreignKey in potentialOwnerships)
{
foreignKey.PrincipalEntityType.FindNavigation(foreignKey.PrincipalToDependent.Name).ForeignKey.Builder
.IsOwnership(true, configurationSource);
}
var ownership = entityType.GetForeignKeys().FirstOrDefault(fk =>
fk.PrincipalToDependent != null
&& !fk.PrincipalEntityType.IsInOwnershipPath(entityType)
&& !fk.PrincipalEntityType.IsInDefinitionPath(clrType));
ownership?.Builder.IsOwnership(true, configurationSource);
}

if (clrType == null)
Expand Down
77 changes: 38 additions & 39 deletions src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,8 +828,8 @@ private static bool CanSetRequiredOnProperties(
ConfigurationSource? configurationSource,
bool shouldThrow)
{
if ((isRequired == null)
|| (properties == null))
if (isRequired == null
|| properties == null)
{
return true;
}
Expand Down Expand Up @@ -883,6 +883,8 @@ public virtual InternalRelationshipBuilder IsOwnership(bool ownership, Configura
{
otherOwnership.Builder.IsOwnership(false, configurationSource);
}

Metadata.SetIsOwnership(true, configurationSource);
}
else if (otherOwnerships.Count > 0)
{
Expand All @@ -893,6 +895,7 @@ public virtual InternalRelationshipBuilder IsOwnership(bool ownership, Configura
}

var otherOwnership = otherOwnerships.Single();
Metadata.SetIsOwnership(true, configurationSource);
Metadata.DeclaringEntityType.Builder.RemoveForeignKey(Metadata, Metadata.GetConfigurationSource());

if (otherOwnership.Builder.IsWeakTypeDefinition(configurationSource) == null)
Expand All @@ -919,6 +922,7 @@ public virtual InternalRelationshipBuilder IsOwnership(bool ownership, Configura
}
else
{
Metadata.SetIsOwnership(true, configurationSource);
newRelationshipBuilder.Metadata.DeclaringEntityType.Builder.HasBaseType((Type)null, configurationSource);
}

Expand All @@ -928,12 +932,18 @@ public virtual InternalRelationshipBuilder IsOwnership(bool ownership, Configura
newRelationshipBuilder.Metadata.Properties.Select(p => p.Name).ToList(), ConfigurationSource.Convention);
}
}
else
{
newRelationshipBuilder.Metadata.SetIsOwnership(false, configurationSource);
}

newRelationshipBuilder.Metadata.SetIsOwnership(ownership, configurationSource);

foreach (var directlyDerivedType in newRelationshipBuilder.Metadata.DeclaringEntityType.GetDirectlyDerivedTypes().ToList())
foreach (var derivedForeignKey in newRelationshipBuilder.Metadata.DeclaringEntityType.GetDerivedForeignKeys()
.Where(fk => fk.PrincipalToDependent != null)
.Concat(newRelationshipBuilder.Metadata.DeclaringEntityType.GetDerivedReferencingForeignKeys()
.Where(fk => fk.DependentToPrincipal != null)).ToList())
{
directlyDerivedType.Builder.HasBaseType((string)null, ConfigurationSource.Convention);
derivedForeignKey.DeclaringEntityType.Builder
.RemoveForeignKey(derivedForeignKey, configurationSource);
}

return batch.Run(newRelationshipBuilder);
Expand All @@ -951,45 +961,34 @@ public virtual InternalRelationshipBuilder IsWeakTypeDefinition(ConfigurationSou
return this;
}

if (Metadata.GetConfigurationSource().Overrides(ConfigurationSource.Explicit)
|| !Metadata.PrincipalEntityType.IsInDefinitionPath(Metadata.DeclaringEntityType.ClrType))
EntityType newEntityType;
if (Metadata.DeclaringEntityType.ClrType == null)
{
EntityType newEntityType;
if (Metadata.DeclaringEntityType.ClrType == null)
{
newEntityType = ModelBuilder.Entity(
Metadata.DeclaringEntityType.Name,
Metadata.PrincipalToDependent.Name,
Metadata.PrincipalEntityType,
Metadata.DeclaringEntityType.GetConfigurationSource()).Metadata;
}
else
{
newEntityType = ModelBuilder.Entity(
Metadata.DeclaringEntityType.ClrType,
Metadata.PrincipalToDependent.Name,
Metadata.PrincipalEntityType,
Metadata.DeclaringEntityType.GetConfigurationSource()).Metadata;
}

var newOwnership = newEntityType.GetForeignKeys().SingleOrDefault(fk => fk.IsOwnership);
if (newOwnership == null)
{
Debug.Assert(Metadata.Builder != null);
return Metadata.Builder;
}

Debug.Assert(Metadata.Builder == null);
ModelBuilder.Metadata.ConventionDispatcher.Tracker.Update(Metadata, newOwnership);
return newOwnership.Builder;
newEntityType = ModelBuilder.Entity(
Metadata.DeclaringEntityType.Name,
Metadata.PrincipalToDependent.Name,
Metadata.PrincipalEntityType,
Metadata.DeclaringEntityType.GetConfigurationSource()).Metadata;
}
else
{
newEntityType = ModelBuilder.Entity(
Metadata.DeclaringEntityType.ClrType,
Metadata.PrincipalToDependent.Name,
Metadata.PrincipalEntityType,
Metadata.DeclaringEntityType.GetConfigurationSource()).Metadata;
}

if (Metadata.Builder.IsOwnership(false, configurationSource) == null)
var newOwnership = newEntityType.GetForeignKeys().SingleOrDefault(fk => fk.IsOwnership);
if (newOwnership == null)
{
return null;
Debug.Assert(Metadata.Builder != null);
return Metadata.Builder;
}

return this;
Debug.Assert(Metadata.Builder == null);
ModelBuilder.Metadata.ConventionDispatcher.Tracker.Update(Metadata, newOwnership);
return newOwnership.Builder;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/PropertyInfoExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public static Type FindCandidateNavigationPropertyType(
targetType = targetSequenceType ?? targetType;
targetType = targetType.UnwrapNullableType();

if (typeMappingSource.FindMapping(propertyInfo) != null
if (typeMappingSource.FindMapping(targetType) != null
|| targetType.GetTypeInfo().IsInterface
|| targetType.GetTypeInfo().IsValueType
|| targetType == typeof(object)
Expand Down
10 changes: 9 additions & 1 deletion test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6692,12 +6692,18 @@ public void Move_properties_to_owned_type_with_existing_ownership()
public void Rename_property_on_owned_type_and_add_similar_to_owner()
{
Execute(
source => source.Entity<Order>().OwnsOne(o => o.Billing).Property<int>("OldZip"),
source => source.Entity<Order>(
x =>
{
x.OwnsOne(o => o.Billing).Property<int>("OldZip");
x.Ignore(o => o.Shipping);
}),
target => target.Entity<Order>(
x =>
{
x.Property<int>("NotZip");
x.OwnsOne(o => o.Billing).Property<int>("NewZip");
x.Ignore(o => o.Shipping);
}),
operations =>
{
Expand All @@ -6723,12 +6729,14 @@ public void Rename_property_on_owning_type_and_add_similar_to_owned()
{
x.Property<DateTime>("OldDate");
x.OwnsOne(o => o.Billing);
x.Ignore(o => o.Shipping);
}),
target => target.Entity<Order>(
x =>
{
x.Property<DateTime>("NewDate");
x.OwnsOne(o => o.Billing).Property<DateTime>("AnotherDate");
x.Ignore(o => o.Shipping);
}),
operations =>
{
Expand Down
Loading

0 comments on commit 91eeff8

Please sign in to comment.