Skip and warn of duplicated SQL Server foreign keys (#25378)

Fixes #25197

Co-authored-by: Erik Ejlskov Jensen <eej@venzo.com>

src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs

@@ -149,6 +149,14 @@ public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions
         /// </summary>
         public EventDefinitionBase? LogReflexiveConstraintIgnored;
 
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public EventDefinitionBase? LogDuplicateForeignKeyConstraintIgnored;
+
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs

@@ -62,7 +62,8 @@ private enum Id
             IndexFound,
             ForeignKeyFound,
             ForeignKeyPrincipalColumnMissingWarning,
-            ReflexiveConstraintIgnored
+            ReflexiveConstraintIgnored,
+            DuplicateForeignKeyConstraintIgnored,
         }
 
         private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + ".";
@@ -230,5 +231,11 @@ private static EventId MakeScaffoldingId(Id id)
         ///     This event is in the <see cref="DbLoggerCategory.Scaffolding" /> category.
         /// </summary>
         public static readonly EventId ReflexiveConstraintIgnored = MakeScaffoldingId(Id.ReflexiveConstraintIgnored);
+
+        /// <summary>
+        ///     A duplicate foreign key constraint was skipped.
+        ///     This event is in the <see cref="DbLoggerCategory.Scaffolding" /> category.
+        /// </summary>
+        public static readonly EventId DuplicateForeignKeyConstraintIgnored = MakeScaffoldingId(Id.DuplicateForeignKeyConstraintIgnored);
     }
 }

src/EFCore.SqlServer/Extensions/Internal/SqlServerLoggerExtensions.cs

@@ -511,6 +511,28 @@ public static void ReflexiveConstraintIgnored(
             // No DiagnosticsSource events because these are purely design-time messages
         }
 
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public static void DuplicateForeignKeyConstraintIgnored(
+            this IDiagnosticsLogger<DbLoggerCategory.Scaffolding> diagnostics,
+            string foreignKeyName,
+            string tableName,
+            string duplicateForeignKeyName)
+        {
+            var definition = SqlServerResources.DuplicateForeignKeyConstraintIgnored(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, foreignKeyName, tableName, duplicateForeignKeyName);
+            }
+
+            // No DiagnosticsSource events because these are purely design-time messages
+        }
+
         /// <summary>
         ///     Logs for the <see cref="RelationalEventId.MultipleCollectionIncludeWarning" /> event.
         /// </summary>

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -252,6 +252,10 @@
     <value>Skipping foreign key '{foreignKeyName}' on table '{tableName}' since all of its columns reference themselves.</value>
     <comment>Debug SqlServerEventId.ReflexiveConstraintIgnored string string</comment>
   </data>
+  <data name="DuplicateForeignKeyConstraintIgnored" xml:space="preserve">
+    <value>Skipping foreign key '{foreignKeyName}' on table '{tableName}' since it is a duplicate of '{duplicateForeignKeyName}'.</value>
+    <comment>Warning SqlServerEventId.DuplicateForeignKeyConstraintIgnored string string string</comment>
+  </data>
   <data name="LogSavepointsDisabledBecauseOfMARS" xml:space="preserve">
     <value>Savepoints are disabled because Multiple Active Result Sets (MARS) is enabled. If 'SaveChanges' fails, then the transaction cannot be automatically rolled back to a known clean state. Instead, the transaction should be rolled back by the application before retrying 'SaveChanges'. See https://go.microsoft.com/fwlink/?linkid=2149338 for more information. To identify the code which triggers this warning, call 'ConfigureWarnings(w =&gt; w.Throw(SqlServerEventId.SavepointsDisabledBecauseOfMARS))'.</value>
     <comment>Warning SqlServerEventId.SavepointsDisabledBecauseOfMARS</comment>

src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs

@@ -1231,6 +1231,18 @@ FROM [sys].[foreign_keys] AS [f]
                         }
                         else
                         {
+                            var duplicated = table.ForeignKeys
+                                .FirstOrDefault(k => k.Columns.SequenceEqual(foreignKey.Columns)
+                                    && k.PrincipalTable.Equals(foreignKey.PrincipalTable));
+                            if (duplicated != null)
+                            {
+                                _logger.DuplicateForeignKeyConstraintIgnored(
+                                    foreignKey.Name!,
+                                    DisplayName(table.Schema, table.Name!),
+                                    duplicated.Name!);
+                                continue;
+                            }
+
                             table.ForeignKeys.Add(foreignKey);
                         }
                     }

test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs

@@ -2370,6 +2370,45 @@ CONSTRAINT MYFK FOREIGN KEY (Id) REFERENCES PrincipalTable(Id)
 DROP TABLE PrincipalTable;");
         }
 
+        [ConditionalFact]
+        public void Skip_duplicate_foreign_key()
+        {
+            Test(
+                @"CREATE TABLE PrincipalTable (
+    Id int PRIMARY KEY,
+);
+
+CREATE TABLE OtherPrincipalTable (
+    Id int PRIMARY KEY,
+);
+
+CREATE TABLE DependentTable (
+    Id int PRIMARY KEY,
+    ForeignKeyId int,
+    CONSTRAINT MYFK1 FOREIGN KEY (ForeignKeyId) REFERENCES PrincipalTable(Id),
+    CONSTRAINT MYFK2 FOREIGN KEY (ForeignKeyId) REFERENCES PrincipalTable(Id),
+    CONSTRAINT MYFK3 FOREIGN KEY (ForeignKeyId) REFERENCES OtherPrincipalTable(Id),
+);",
+                Enumerable.Empty<string>(),
+                Enumerable.Empty<string>(),
+                dbModel =>
+                {
+                    var (level, _, message, _, _) = Assert.Single(
+                        Fixture.ListLoggerFactory.Log, t => t.Id == SqlServerEventId.DuplicateForeignKeyConstraintIgnored);
+                    Assert.Equal(LogLevel.Warning, level);
+                    Assert.Equal(
+                        SqlServerResources.DuplicateForeignKeyConstraintIgnored(new TestLogger<SqlServerLoggingDefinitions>())
+                            .GenerateMessage("MYFK2", "dbo.DependentTable", "MYFK1"), message);
+
+                    var table = dbModel.Tables.Single(t => t.Name == "DependentTable");
+                    Assert.Equal(2, table.ForeignKeys.Count);
+                },
+                @"
+DROP TABLE DependentTable;
+DROP TABLE PrincipalTable;
+DROP TABLE OtherPrincipalTable;");
+        }
+
         #endregion
 
         private void Test(