Relax restriction on derived FKs using value-generation properties fr…

…om composite keys. Fixes dotnet#11939
joaopgrassi · Sep 2, 2020 · 474ef51 · 474ef51
1 parent 3f0e31f
commit 474ef51
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 83 deletions.
diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs
@@ -798,29 +798,36 @@ protected virtual void ValidateForeignKeys(
                         continue;
                     }
 
-                    var inheritedKey = declaredForeignKey.Properties.Where(p => p.ValueGenerated != ValueGenerated.Never)
-                        .SelectMany(p => p.GetContainingKeys().Where(k => k.DeclaringEntityType != entityType)).FirstOrDefault();
-                    if (inheritedKey != null)
+                    foreach (var generatedProperty in declaredForeignKey.Properties)
                     {
-                        var generatedProperty = declaredForeignKey.Properties.First(
-                            p => p.ValueGenerated != ValueGenerated.Never && inheritedKey.Properties.Contains(p));
-
-                        if (entityType.BaseType.ClrType.IsAbstract
-                            && entityType.BaseType.GetDerivedTypes().All(
-                                d => d.GetDeclaredForeignKeys().Any(fk => fk.Properties.Contains(generatedProperty))))
+                        if (!generatedProperty.ValueGenerated.ForAdd())
                         {
                             continue;
                         }
 
-                        throw new InvalidOperationException(
-                            CoreStrings.ForeignKeyPropertyInKey(
-                                generatedProperty.Name,
-                                entityType.DisplayName(),
-                                inheritedKey.Properties.Format(),
-                                inheritedKey.DeclaringEntityType.DisplayName()));
+                        foreach (var inheritedKey in generatedProperty.GetContainingKeys())
+                        {
+                            if (inheritedKey.DeclaringEntityType != entityType
+                                && inheritedKey.Properties.All(p => declaredForeignKey.Properties.Contains(p))
+                                && !ContainedInForeignKeyForAllConcreteTypes(inheritedKey.DeclaringEntityType, generatedProperty))
+                            {
+                                throw new InvalidOperationException(
+                                    CoreStrings.ForeignKeyPropertyInKey(
+                                        generatedProperty.Name,
+                                        entityType.DisplayName(),
+                                        inheritedKey.Properties.Format(),
+                                        inheritedKey.DeclaringEntityType.DisplayName()));
+                            }
+                        }
                     }
                 }
             }
+
+            static bool ContainedInForeignKeyForAllConcreteTypes(IEntityType entityType, IProperty property)
+                => entityType.ClrType?.IsAbstract == true
+                    && entityType.GetDerivedTypes().Where(t => t.ClrType?.IsAbstract != true)
+                        .All(d => d.GetForeignKeys()
+                            .Any(fk => fk.Properties.Contains(property)));
         }
 
         /// <summary>

diff --git a/src/EFCore/Metadata/Internal/EntityType.cs b/src/EFCore/Metadata/Internal/EntityType.cs
@@ -791,12 +791,6 @@ public virtual Key AddKey(
                     throw new InvalidOperationException(CoreStrings.KeyPropertiesWrongEntity(properties.Format(), this.DisplayName()));
                 }
 
-                if (property.ValueGenerated != ValueGenerated.Never
-                    && property.GetContainingForeignKeys().Any(k => k.DeclaringEntityType != this))
-                {
-                    throw new InvalidOperationException(CoreStrings.KeyPropertyInForeignKey(property.Name, this.DisplayName()));
-                }
-
                 if (property.IsNullable)
                 {
                     throw new InvalidOperationException(CoreStrings.NullableKey(this.DisplayName(), property.Name));

diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs
diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx
@@ -469,7 +469,7 @@
     <value>The specified foreign key properties {foreignKeyProperties} are not declared on the entity type '{entityType}'. Ensure foreign key properties are declared on the target entity type.</value>
   </data>
   <data name="ForeignKeyPropertyInKey" xml:space="preserve">
-    <value>The property '{property}' cannot be part of a foreign key on '{entityType}' because it has value generation enabled and is contained in the key {keyProperties} defined on a base entity type '{baseEntityType}'.</value>
+    <value>The property '{property}' cannot be part of a foreign key on '{entityType}' because it has a store-generated value and is contained in the key {keyProperties} defined on a base entity type '{baseEntityType}'.</value>
   </data>
   <data name="ForeignKeyReferencedEntityKeyMismatch" xml:space="preserve">
     <value>The provided principal entity key '{principalKey}' is not a key on the entity type '{principalEntityType}'.</value>
@@ -660,9 +660,6 @@
   <data name="KeyPropertyCannotBeNullable" xml:space="preserve">
     <value>The property '{1_entityType}.{0_property}' cannot be marked as nullable/optional because it has been included in a key {keyProperties}.</value>
   </data>
-  <data name="KeyPropertyInForeignKey" xml:space="preserve">
-    <value>The property '{1_entityType}.{0_property}' cannot be part of a key because it has value generation enabled and is contained in a foreign key defined on a derived entity type.</value>
-  </data>
   <data name="KeyPropertyMustBeReadOnly" xml:space="preserve">
     <value>The property '{1_entityType}.{0_property}' must be marked as read-only after it has been saved because it is part of a key. Key properties are always read-only once an entity has been saved for the first time.</value>
   </data>

diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs
@@ -913,7 +913,7 @@ public virtual void Detects_ForeignKey_on_inherited_generated_key_property()
         }
 
         [ConditionalFact]
-        public virtual void Passes_ForeignKey_on_inherited_generated_key_property_abstract_base()
+        public virtual void Passes_for_ForeignKey_on_inherited_generated_key_property_abstract_base()
         {
             var modelBuilder = CreateConventionalModelBuilder();
             modelBuilder.Entity<Abstract>().Property(e => e.Id).ValueGeneratedOnAdd();
@@ -922,6 +922,19 @@ public virtual void Passes_ForeignKey_on_inherited_generated_key_property_abstra
             Validate(modelBuilder.Model);
         }
 
+        [ConditionalFact]
+        public virtual void Passes_for_ForeignKey_on_inherited_generated_composite_key_property()
+        {
+            var modelBuilder = CreateConventionalModelBuilder();
+            modelBuilder.Entity<Abstract>().Property<int>("SomeId").ValueGeneratedOnAdd();
+            modelBuilder.Entity<Abstract>().Property<int>("SomeOtherId").ValueGeneratedOnAdd();
+            modelBuilder.Entity<Abstract>().HasAlternateKey("SomeId", "SomeOtherId");
+            modelBuilder.Entity<Generic<int>>().HasOne<Abstract>().WithOne().HasForeignKey<Generic<int>>("SomeId");
+            modelBuilder.Entity<Generic<string>>();
+
+            Validate(modelBuilder.Model);
+        }
+
         [ConditionalTheory]
         [InlineData(ChangeTrackingStrategy.ChangedNotifications)]
         [InlineData(ChangeTrackingStrategy.ChangingAndChangedNotifications)]

diff --git a/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs b/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs
@@ -394,24 +394,6 @@ public void Can_add_a_key_if_any_properties_are_part_of_derived_foreign_key()
             Assert.NotNull(baseType.AddKey(new[] { fkProperty }));
         }
 
-        [ConditionalFact]
-        public void Adding_a_key_with_value_generation_throws_if_any_properties_are_part_of_derived_foreign_key()
-        {
-            var model = CreateModel();
-            var baseType = model.AddEntityType(typeof(BaseType));
-            var idProperty = baseType.AddProperty(Customer.IdProperty);
-            var fkProperty = baseType.AddProperty("fk", typeof(int));
-            fkProperty.ValueGenerated = ValueGenerated.OnAdd;
-            var key = baseType.AddKey(new[] { idProperty });
-            var entityType = model.AddEntityType(typeof(Customer));
-            entityType.BaseType = baseType;
-            entityType.AddForeignKey(new[] { fkProperty }, key, entityType);
-
-            Assert.Equal(
-                CoreStrings.KeyPropertyInForeignKey("fk", typeof(BaseType).Name),
-                Assert.Throws<InvalidOperationException>(() => baseType.AddKey(new[] { fkProperty })).Message);
-        }
-
         [ConditionalFact]
         public void Can_remove_keys()
         {

diff --git a/test/EFCore.Tests/Metadata/Internal/InternalEntityTypeBuilderTest.cs b/test/EFCore.Tests/Metadata/Internal/InternalEntityTypeBuilderTest.cs
@@ -528,7 +528,8 @@ public void Removing_relationship_removes_unused_conventional_index()
             Assert.Empty(dependentEntityBuilder.Metadata.GetForeignKeys());
         }
 
-        [ConditionalFact] // TODO: Add test if the index is being used by another FK when support for multiple FK on same set of properties is added
+        [ConditionalFact]
+        // TODO: Add test if the index is being used by another FK when support for multiple FK on same set of properties is added
         public void Removing_relationship_does_not_remove_conventional_index_if_in_use()
         {
             var modelBuilder = CreateModelBuilder();
@@ -1053,33 +1054,6 @@ public void Key_throws_for_derived_type()
                             new[] { Order.IdProperty.Name, Order.CustomerIdProperty.Name }, ConfigurationSource.DataAnnotation)).Message);
         }
 
-        [ConditionalFact]
-        public void Key_throws_if_conflicting_with_derived_foreign_key()
-        {
-            var modelBuilder = CreateModelBuilder();
-            var principalEntityBuilder = modelBuilder.Entity(typeof(Customer), ConfigurationSource.Explicit);
-            principalEntityBuilder.PrimaryKey(new[] { Customer.IdProperty }, ConfigurationSource.Explicit);
-            var dependentEntityBuilder = modelBuilder.Entity(typeof(Order), ConfigurationSource.Explicit);
-            var derivedDependentEntityBuilder = modelBuilder.Entity(typeof(SpecialOrder), ConfigurationSource.Convention);
-            derivedDependentEntityBuilder.HasBaseType(dependentEntityBuilder.Metadata, ConfigurationSource.Explicit);
-            var idProperty = dependentEntityBuilder.Property(Order.IdProperty, ConfigurationSource.Convention).Metadata;
-            idProperty.ValueGenerated = ValueGenerated.OnAdd;
-
-            derivedDependentEntityBuilder.HasRelationship(
-                    principalEntityBuilder.Metadata,
-                    Order.CustomerProperty.Name,
-                    nameof(Customer.SpecialOrders),
-                    ConfigurationSource.Explicit)
-                .HasForeignKey(new[] { idProperty }, ConfigurationSource.Explicit);
-
-            Assert.Null(dependentEntityBuilder.HasKey(new[] { Order.IdProperty }, ConfigurationSource.DataAnnotation));
-
-            Assert.Equal(
-                CoreStrings.KeyPropertyInForeignKey(Order.IdProperty.Name, nameof(Order)),
-                Assert.Throws<InvalidOperationException>(
-                    () => dependentEntityBuilder.HasKey(new[] { Order.IdProperty }, ConfigurationSource.Explicit)).Message);
-        }
-
         [ConditionalFact]
         public void Key_throws_for_property_names_for_shadow_entity_type_if_they_do_not_exist()
         {
@@ -1280,8 +1254,7 @@ public void HasKey_can_override_lower_or_equal_source_HasNoKey()
             Assert.Equal(
                 CoreStrings.KeylessTypeWithKey("{'CustomerId'}", nameof(Order)),
                 Assert.Throws<InvalidOperationException>(
-                    () =>
-                        entityBuilder.HasKey(new[] { Order.CustomerIdProperty.Name }, ConfigurationSource.Explicit)).Message);
+                    () => entityBuilder.HasKey(new[] { Order.CustomerIdProperty.Name }, ConfigurationSource.Explicit)).Message);
         }
 
         [ConditionalFact]