Skip to content

Commit

Permalink
Dissalow defining FKs on properties that are part of an inherited key
Browse files Browse the repository at this point in the history 
Fixes #2837
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Dec 11, 2015
1 parent 4e23c4e commit 5d75dfc
Show file tree
Hide file tree
Showing 9 changed files with 249 additions and 35 deletions.
12 changes: 11 additions & 1 deletion src/EntityFramework.Core/Metadata/Internal/EntityType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,11 @@ public virtual Key AddKey([NotNull] IReadOnlyList<Property> properties,
{
throw new ArgumentException(CoreStrings.KeyPropertiesWrongEntity(Property.Format(properties), this.DisplayName()));
}

if (property.FindContainingForeignKeys().Any(k => k.DeclaringEntityType != this))
{
throw new InvalidOperationException(CoreStrings.KeyPropertyInForeignKey(property.Name, this.DisplayName()));
}
}

var key = FindKey(properties);
Expand Down Expand Up @@ -515,7 +520,12 @@ public virtual ForeignKey AddForeignKey(
if ((actualProperty == null)
|| (actualProperty.DeclaringEntityType != property.DeclaringEntityType))
{
throw new ArgumentException(CoreStrings.ForeignKeyPropertiesWrongEntity(Property.Format(properties), Name));
throw new ArgumentException(CoreStrings.ForeignKeyPropertiesWrongEntity(Property.Format(properties), this.DisplayName()));
}

if (actualProperty.FindContainingKeys().Any(k => k.DeclaringEntityType != this))
{
throw new InvalidOperationException(CoreStrings.ForeignKeyPropertyInKey(actualProperty.Name, this.DisplayName()));
}
}

Expand Down
37 changes: 31 additions & 6 deletions src/EntityFramework.Core/Metadata/Internal/InternalEntityTypeBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,17 @@ private InternalKeyBuilder HasKey(IReadOnlyList<Property> properties, Configurat
return null;
}

var id = GetActualProperties(properties, configurationSource);
var key = Metadata.FindDeclaredKey(id);
var actualProperties = GetActualProperties(properties, configurationSource);
var key = Metadata.FindDeclaredKey(actualProperties);
if (key == null)
{
key = Metadata.AddKey(id, configurationSource);
if ((configurationSource != ConfigurationSource.Explicit) // let it throw for explicit
&& actualProperties.Any(p => p.FindContainingForeignKeys().Any(k => k.DeclaringEntityType != Metadata)))
{
return null;
}

key = Metadata.AddKey(actualProperties, configurationSource);
}
else
{
Expand Down Expand Up @@ -375,7 +381,7 @@ public virtual InternalEntityTypeBuilder HasBaseType([CanBeNull] EntityType base

var detachedRelationships = new HashSet<RelationshipBuilderSnapshot>();
PropertyBuildersSnapshot detachedProperties = null;

var changedRelationships = new List<InternalRelationshipBuilder>();
IReadOnlyList<RelationshipSnapshot> relationshipsToBeRemoved = new List<RelationshipSnapshot>();
if (baseEntityType != null)
{
Expand All @@ -390,6 +396,20 @@ public virtual InternalEntityTypeBuilder HasBaseType([CanBeNull] EntityType base
return null;
}

var foreignKeysUsingKeyProperties = Metadata.GetDeclaredForeignKeys()
.Where(fk => relationshipsToBeRemoved.All(r => r.ForeignKey != fk)
&& fk.Properties.Any(p => baseEntityType.FindProperty(p.Name)?.IsKey() == true)).ToList();

if (foreignKeysUsingKeyProperties.Any(fk =>
!configurationSource.Overrides(fk.GetForeignKeyPropertiesConfigurationSource())))
{
return null;
}

changedRelationships.AddRange(
foreignKeysUsingKeyProperties.Select(foreignKeyUsingKeyProperties =>
foreignKeyUsingKeyProperties.Builder.HasForeignKey(null, configurationSource, runConventions: false)));

foreach (var relationshipToBeRemoved in relationshipsToBeRemoved)
{
var removedConfigurationSource = relationshipToBeRemoved.ForeignKey.DeclaringEntityType.Builder
Expand Down Expand Up @@ -433,6 +453,11 @@ public virtual InternalEntityTypeBuilder HasBaseType([CanBeNull] EntityType base
detachedRelationship.Attach();
}

foreach (var changedRelationship in changedRelationships)
{
ModelBuilder.Metadata.ConventionDispatcher.OnForeignKeyAdded(changedRelationship);
}

foreach (var relationshipToBeRemoved in relationshipsToBeRemoved)
{
var dependentEntityType = relationshipToBeRemoved.ForeignKey.DeclaringEntityType.Builder;
Expand Down Expand Up @@ -669,7 +694,7 @@ private RelationshipBuilderSnapshot DetachRelationship([NotNull] ForeignKey fore
{
return null;
}

var removedForeignKey = Metadata.RemoveForeignKey(
foreignKey.Properties, foreignKey.PrincipalKey, foreignKey.PrincipalEntityType, runConventions);

Expand All @@ -689,7 +714,7 @@ private RelationshipBuilderSnapshot DetachRelationship([NotNull] ForeignKey fore

RemoveShadowPropertiesIfUnused(foreignKey.Properties.Where(p => p.DeclaringEntityType.FindDeclaredProperty(p.Name) != null).ToList());
foreignKey.PrincipalKey.DeclaringEntityType.Builder?.RemoveKeyIfUnused(foreignKey.PrincipalKey);

return currentConfigurationSource;
}

Expand Down
31 changes: 19 additions & 12 deletions src/EntityFramework.Core/Metadata/Internal/InternalRelationshipBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Linq;
using System.Reflection;
using JetBrains.Annotations;
using Microsoft.Data.Entity.Internal;
using Microsoft.Data.Entity.Utilities;

namespace Microsoft.Data.Entity.Metadata.Internal
Expand Down Expand Up @@ -62,8 +61,8 @@ public virtual InternalRelationshipBuilder Navigations(
}

var newRelationshipBuilder = ReplaceForeignKey(configurationSource,
navigationToPrincipalName: navigationToPrincipalName ?? "",
navigationToDependentName: navigationToDependentName ?? "");
navigationToPrincipalName: navigationToPrincipalName ?? "",
navigationToDependentName: navigationToDependentName ?? "");

if (newRelationshipBuilder != null
&& newRelationshipBuilder.Metadata.Builder == null)
Expand Down Expand Up @@ -828,16 +827,16 @@ public virtual InternalRelationshipBuilder HasForeignKey(
=> HasForeignKey(
GetExistingProperties(properties, Metadata.DeclaringEntityType), configurationSource, runConventions: true);

private InternalRelationshipBuilder HasForeignKey(
IReadOnlyList<Property> properties, ConfigurationSource configurationSource, bool runConventions)
public virtual InternalRelationshipBuilder HasForeignKey(
[CanBeNull] IReadOnlyList<Property> properties, ConfigurationSource configurationSource, bool runConventions)
{
if (properties == null)
{
return !configurationSource.Overrides(Metadata.GetForeignKeyPropertiesConfigurationSource())
? null
: ReplaceForeignKey(configurationSource,
dependentProperties: new Property[0],
runConventions: runConventions);
dependentProperties: new Property[0],
runConventions: runConventions);
}

if (Metadata.Properties.SequenceEqual(properties))
Expand All @@ -861,6 +860,13 @@ private InternalRelationshipBuilder HasForeignKey(
return null;
}

if ((Metadata.DeclaringEntityType.BaseType != null)
&& (configurationSource != ConfigurationSource.Explicit) // let it throw for explicit
&& properties.Any(p => p.FindContainingKeys().Any(k => k.DeclaringEntityType != Metadata.DeclaringEntityType)))
{
return null;
}

var resetIsRequired = false;
if (!CanSetRequiredOnProperties(properties, Metadata.IsRequired, configurationSource, shouldThrow: false))
{
Expand Down Expand Up @@ -905,7 +911,8 @@ private InternalRelationshipBuilder HasForeignKey(
private bool CanSetForeignKey(
IReadOnlyList<Property> properties, EntityType dependentEntityType, ConfigurationSource configurationSource)
{
if (properties != null && Metadata.Properties.SequenceEqual(properties))
if (properties != null
&& Metadata.Properties.SequenceEqual(properties))
{
return true;
}
Expand Down Expand Up @@ -1609,10 +1616,10 @@ private InternalRelationshipBuilder FindForeignKey(
IReadOnlyList<Property> dependentProperties = null)
{
var matchingRelationships = Metadata.DeclaringEntityType.Builder.GetRelationshipBuilders(
Metadata.PrincipalEntityType,
navigationToPrincipalName == "" ? null : navigationToPrincipalName,
navigationToDependentName == "" ? null : navigationToDependentName,
dependentProperties);
Metadata.PrincipalEntityType,
navigationToPrincipalName == "" ? null : navigationToPrincipalName,
navigationToDependentName == "" ? null : navigationToDependentName,
dependentProperties);

matchingRelationships = matchingRelationships.Distinct().ToList();

Expand Down
16 changes: 16 additions & 0 deletions src/EntityFramework.Core/Properties/CoreStrings.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/EntityFramework.Core/Properties/CoreStrings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,4 +516,10 @@
<data name="PrincipalEntityTypeNotInRelationship" xml:space="preserve">
<value>You are configuring a relationship between '{dependentEntityType}' and '{principalEntityType}' but have specified a foreign key targetting '{entityType}'. The foreign key must be targetting a type that is part of the relationship.</value>
</data>
<data name="ForeignKeyPropertyInKey" xml:space="preserve">
<value>The property '{property}' cannot be part of a foreign key on '{entityType}' because it is contained in a key defined on a base entity type.</value>
</data>
<data name="KeyPropertyInForeignKey" xml:space="preserve">
<value>The property '{property}' cannot be part of a key on '{entityType}' because it is contained in a foreign key defined on a derived entity type.</value>
</data>
</root>
61 changes: 47 additions & 14 deletions ...ork.Core.Tests/Metadata/Conventions/Internal/ForeignKeyPropertyDiscoveryConventionTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ public void Does_not_match_dependent_PK_for_self_ref()
Assert.True(fk.IsUnique);
Assert.False(fk.IsRequired);
}

[Fact]
public void Matches_composite_dependent_PK_for_unique_FK()
{
Expand All @@ -404,6 +404,40 @@ public void Matches_composite_dependent_PK_for_unique_FK()
Assert.True(fk.IsRequired);
}

[Fact]
public void Does_not_match_composite_dependent_PK_for_unique_FK_on_derived_type()
{
var modelBuilder = new InternalModelBuilder(new Model());

var principalTypeWithCompositeKey = modelBuilder.Entity(typeof(PrincipalEntityWithCompositeKey), ConfigurationSource.Explicit);
principalTypeWithCompositeKey.PrimaryKey(new[] { PrincipalEntityWithCompositeKey.IdProperty, PrincipalEntityWithCompositeKey.NameProperty }, ConfigurationSource.Explicit);
principalTypeWithCompositeKey.Property(PrincipalEntityWithCompositeKey.NameProperty, ConfigurationSource.Explicit).IsRequired(true, ConfigurationSource.Explicit);

var dependentTypeWithCompositeKeyBase = modelBuilder.Entity(typeof(DependentCompositeBase), ConfigurationSource.Explicit);
var dependentTypeWithCompositeKey = modelBuilder.Entity(typeof(DependentEntityWithCompositeKey), ConfigurationSource.Explicit);
dependentTypeWithCompositeKey.HasBaseType(dependentTypeWithCompositeKeyBase.Metadata, ConfigurationSource.Explicit);
dependentTypeWithCompositeKeyBase.PrimaryKey(new[] { nameof(DependentEntityWithCompositeKey.NotId), nameof(DependentEntityWithCompositeKey.NotName) }, ConfigurationSource.Explicit);

var relationshipBuilder = dependentTypeWithCompositeKey.Relationship(
principalTypeWithCompositeKey,
"NavProp",
"InverseReferenceNav", ConfigurationSource.Convention)
.IsUnique(true, ConfigurationSource.DataAnnotation)
.DependentEntityType(dependentTypeWithCompositeKey, ConfigurationSource.DataAnnotation);

var newRelationshipBuilder = new ForeignKeyPropertyDiscoveryConvention().Apply(relationshipBuilder);
Assert.Same(relationshipBuilder, newRelationshipBuilder);

var fk = (IForeignKey)dependentTypeWithCompositeKey.Metadata.GetForeignKeys().Single();
Assert.Same(fk, newRelationshipBuilder.Metadata);
Assert.NotEqual(dependentTypeWithCompositeKey.Metadata.FindPrimaryKey().Properties[0], fk.Properties[0]);
Assert.NotEqual(dependentTypeWithCompositeKey.Metadata.FindPrimaryKey().Properties[1], fk.Properties[1]);
Assert.Equal("NavProp" + CompositePrimaryKey[0].Name + "1", fk.Properties[0].Name);
Assert.Equal("NavProp" + CompositePrimaryKey[1].Name + "1", fk.Properties[1].Name);
Assert.Same(principalTypeWithCompositeKey.Metadata.FindPrimaryKey(), fk.PrincipalKey);
Assert.True(fk.IsUnique);
}

[Fact]
public void Does_not_match_composite_dependent_PK_for_non_unique_FK()
{
Expand Down Expand Up @@ -795,18 +829,17 @@ private static InternalModelBuilder BuildModel()
var modelBuilder = new InternalModelBuilder(new Model());

var principalType = modelBuilder.Entity(typeof(PrincipalEntity), ConfigurationSource.Explicit);
principalType.PrimaryKey(new[] { "PeeKay" }, ConfigurationSource.Explicit);
principalType.PrimaryKey(new[] { nameof(PrincipalEntity.PeeKay) }, ConfigurationSource.Explicit);

var dependentType = modelBuilder.Entity(typeof(DependentEntity), ConfigurationSource.Explicit);
dependentType.PrimaryKey(new[] { "KayPee" }, ConfigurationSource.Explicit);
dependentType.PrimaryKey(new[] { nameof(DependentEntity.KayPee)}, ConfigurationSource.Explicit);

var principalTypeWithCompositeKey = modelBuilder.Entity(typeof(PrincipalEntityWithCompositeKey), ConfigurationSource.Explicit);
principalTypeWithCompositeKey.PrimaryKey(new[] { PrincipalEntityWithCompositeKey.IdProperty, PrincipalEntityWithCompositeKey.NameProperty }, ConfigurationSource.Explicit);
principalTypeWithCompositeKey.Property(PrincipalEntityWithCompositeKey.NameProperty, ConfigurationSource.Explicit).IsRequired(true, ConfigurationSource.Explicit);

var dependentTypeWithCompositeKey = modelBuilder.Entity(typeof(DependentEntityWithCompositeKey), ConfigurationSource.Explicit);
dependentTypeWithCompositeKey.PrimaryKey(new[] { "NotId", "NotName" }, ConfigurationSource.Explicit);
dependentTypeWithCompositeKey.Property("NotName", typeof(string), ConfigurationSource.Explicit).IsRequired(true, ConfigurationSource.Explicit);
dependentTypeWithCompositeKey.PrimaryKey(new[] { nameof(DependentEntityWithCompositeKey.NotId), nameof(DependentEntityWithCompositeKey.NotName) }, ConfigurationSource.Explicit);

return modelBuilder;
}
Expand All @@ -817,10 +850,7 @@ private static InternalModelBuilder BuildModel()

private InternalEntityTypeBuilder DependentType => _model.Entity(typeof(DependentEntity), ConfigurationSource.Convention);

private IReadOnlyList<Property> CompositePrimaryKey
{
get { return PrincipalTypeWithCompositeKey.Metadata.FindPrimaryKey().Properties; }
}
private IReadOnlyList<Property> CompositePrimaryKey => PrincipalTypeWithCompositeKey.Metadata.FindPrimaryKey().Properties;

private InternalEntityTypeBuilder PrincipalTypeWithCompositeKey => _model.Entity(typeof(PrincipalEntityWithCompositeKey), ConfigurationSource.Convention);

Expand All @@ -845,7 +875,7 @@ private class DependentEntity
public static readonly PropertyInfo PrincipalEntityPeEKaYProperty = typeof(DependentEntity).GetProperty("PrincipalEntityPeEKaY");
public static readonly PropertyInfo IDProperty = typeof(DependentEntity).GetProperty("ID");
public static readonly PropertyInfo PeEKaYProperty = typeof(DependentEntity).GetProperty("PeEKaY");

public int KayPee { get; set; }
public int SomeNavID { get; set; }
public int SomeNavPeEKaY { get; set; }
Expand All @@ -868,7 +898,13 @@ private class PrincipalEntityWithCompositeKey
public DependentEntityWithCompositeKey InverseReferenceNav { get; set; }
}

private class DependentEntityWithCompositeKey
private class DependentCompositeBase
{
public int NotId { get; set; }
public string NotName { get; set; }
}

private class DependentEntityWithCompositeKey : DependentCompositeBase
{
public static readonly PropertyInfo NavPropIdProperty = typeof(DependentEntityWithCompositeKey).GetProperty("NavPropId");
public static readonly PropertyInfo NavPropNameProperty = typeof(DependentEntityWithCompositeKey).GetProperty("NavPropName");
Expand All @@ -877,9 +913,6 @@ private class DependentEntityWithCompositeKey
public static readonly PropertyInfo IdProperty = typeof(DependentEntityWithCompositeKey).GetProperty("Id");
public static readonly PropertyInfo NameProperty = typeof(DependentEntityWithCompositeKey).GetProperty("Name");

public int NotId { get; set; }
public string NotName { get; set; }

public int NavPropId { get; set; }
public string NavPropName { get; set; }
public int PrincipalEntityWithCompositeKeyId { get; set; }
Expand Down
Loading

0 comments on commit 5d75dfc

Please sign in to comment.