Allow ForeignKeyAttribute to be used on skip navigations.

Fixes #22530
dotnet · Aug 9, 2021 · 685050b · 685050b
1 parent 59ad39a
commit 685050b
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 27 deletions.
diff --git a/src/EFCore/Metadata/Conventions/ForeignKeyAttributeConvention.cs b/src/EFCore/Metadata/Conventions/ForeignKeyAttributeConvention.cs
@@ -1,10 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations.Schema;
-using System.Linq;
 using System.Reflection;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Diagnostics;
@@ -24,7 +21,11 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
     ///         For one-to-one relationships the attribute has to be specified on the navigation property pointing to the principal.
     ///     </para>
     /// </summary>
-    public class ForeignKeyAttributeConvention : IForeignKeyAddedConvention, INavigationAddedConvention, IModelFinalizingConvention
+    public class ForeignKeyAttributeConvention :
+        IForeignKeyAddedConvention,
+        INavigationAddedConvention,
+        ISkipNavigationForeignKeyChangedConvention,
+        IModelFinalizingConvention
     {
         /// <summary>
         ///     Creates a new instance of <see cref="ForeignKeyAttributeConvention" />.
@@ -70,8 +71,7 @@ public virtual void ProcessNavigationAdded(
             Check.NotNull(navigationBuilder, nameof(navigationBuilder));
 
             var onDependent = navigationBuilder.Metadata.IsOnDependent;
-            var newRelationshipBuilder =
-                UpdateRelationshipBuilder(navigationBuilder.Metadata.ForeignKey.Builder, context);
+            var newRelationshipBuilder = UpdateRelationshipBuilder(navigationBuilder.Metadata.ForeignKey.Builder, context);
             if (newRelationshipBuilder != null)
             {
                 var newNavigationBuilder = onDependent
@@ -422,6 +422,41 @@ private bool IsNavigationCandidate(PropertyInfo propertyInfo, IConventionEntityT
             return null;
         }
 
+        /// <inheritdoc />
+        public virtual void ProcessSkipNavigationForeignKeyChanged(
+            IConventionSkipNavigationBuilder skipNavigationBuilder,
+            IConventionForeignKey? foreignKey,
+            IConventionForeignKey? oldForeignKey,
+            IConventionContext<IConventionForeignKey> context)
+        {
+            if (foreignKey != null
+                && foreignKey.IsInModel)
+            {
+                var fkPropertiesToSet = FindCandidateDependentPropertiesThroughNavigation(skipNavigationBuilder.Metadata);
+
+                foreignKey.Builder.HasForeignKey(fkPropertiesToSet, fromDataAnnotation: true);
+            }
+        }
+
+        private static IReadOnlyList<string>? FindCandidateDependentPropertiesThroughNavigation(
+            IConventionSkipNavigation skipNavigation)
+        {
+            var navigationFkAttribute = GetForeignKeyAttribute(skipNavigation);
+            if (navigationFkAttribute == null)
+            {
+                return null;
+            }
+
+            var properties = navigationFkAttribute.Name.Split(',').Select(p => p.Trim()).ToList();
+            if (properties.Any(string.IsNullOrWhiteSpace))
+            {
+                throw new InvalidOperationException(
+                    CoreStrings.InvalidPropertyListOnNavigation(skipNavigation!.Name, skipNavigation.DeclaringEntityType.DisplayName()));
+            }
+
+            return properties;
+        }
+
         /// <inheritdoc />
         public virtual void ProcessModelFinalizing(
             IConventionModelBuilder modelBuilder,
@@ -446,17 +481,6 @@ var fkPropertyOnPrincipal
                         }
                     }
                 }
-
-                foreach (var declaredSkipNavigation in entityType.GetDeclaredSkipNavigations())
-                {
-                    var fkAttribute = GetForeignKeyAttribute(declaredSkipNavigation);
-                    if (fkAttribute != null
-                        && declaredSkipNavigation.ForeignKey?.GetPropertiesConfigurationSource() != ConfigurationSource.Explicit)
-                    {
-                        throw new InvalidOperationException(
-                            CoreStrings.FkAttributeOnSkipNavigation(entityType.DisplayName(), declaredSkipNavigation.Name));
-                    }
-                }
             }
         }
     }

diff --git a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs
@@ -196,6 +196,7 @@ public virtual ConventionSet CreateConventionSet()
             conventionSet.SkipNavigationInverseChangedConventions.Add(foreignKeyPropertyDiscoveryConvention);
 
             conventionSet.SkipNavigationForeignKeyChangedConventions.Add(manyToManyJoinEntityTypeConvention);
+            conventionSet.SkipNavigationForeignKeyChangedConventions.Add(foreignKeyAttributeConvention);
             conventionSet.SkipNavigationForeignKeyChangedConventions.Add(keyDiscoveryConvention);
             conventionSet.SkipNavigationForeignKeyChangedConventions.Add(foreignKeyPropertyDiscoveryConvention);
 

diff --git a/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs b/test/EFCore.Cosmos.Tests/ModelBuilding/CosmosModelBuilderGenericTest.cs
@@ -375,7 +375,7 @@ public override void Join_type_is_automatically_configured_by_convention()
                 // Many-to-many not configured by convention on Cosmos
             }
 
-            public override void Throws_for_ForeignKeyAttribute_on_navigation()
+            public override void ForeignKeyAttribute_configures_the_properties()
             {
                 // Many-to-many not configured by convention on Cosmos
             }

diff --git a/test/EFCore.Tests/ModelBuilding/ManyToManyTestBase.cs b/test/EFCore.Tests/ModelBuilding/ManyToManyTestBase.cs
@@ -1,10 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations.Schema;
-using System.Linq;
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata;
@@ -497,17 +494,25 @@ public virtual void Throws_for_self_ref_using_self()
             }
 
             [ConditionalFact]
-            public virtual void Throws_for_ForeignKeyAttribute_on_navigation()
+            public virtual void ForeignKeyAttribute_configures_the_properties()
             {
                 var modelBuilder = CreateModelBuilder();
 
                 modelBuilder.Entity<CategoryWithAttribute>();
 
-                Assert.Equal(
-                    CoreStrings.FkAttributeOnSkipNavigation(
-                        nameof(ProductWithAttribute), nameof(Product.Categories)),
-                    Assert.Throws<InvalidOperationException>(
-                        () => modelBuilder.FinalizeModel()).Message);
+                var model = modelBuilder.FinalizeModel();
+
+                var category = model.FindEntityType(typeof(CategoryWithAttribute))!;
+                var productsNavigation = category.GetSkipNavigations().Single();
+                var categoryFk = productsNavigation.ForeignKey;
+                Assert.Equal("CategoriesID", categoryFk.Properties.Single().Name);
+
+                var categoryNavigation = productsNavigation.TargetEntityType.GetSkipNavigations().Single();
+                var productFk = categoryNavigation.ForeignKey;
+                Assert.Equal("ProductKey", productFk.Properties.Single().Name);
+
+                var joinEntityType = categoryFk.DeclaringEntityType;
+                Assert.Equal(2, joinEntityType.GetForeignKeys().Count());
             }
 
             [ConditionalFact]