Correctly break double cycles in FK constraints

Fixes #22201
dotnet · Sep 4, 2020 · 58cc57a · 58cc57a
1 parent b276a55
commit 58cc57a
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 11 deletions.
diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
@@ -309,8 +309,15 @@ protected virtual IReadOnlyList<MigrationOperation> Sort(
                 {
                     foreach (var cyclicAddForeignKeyOperation in cyclicAddForeignKeyOperations)
                     {
-                        createTableOperation.ForeignKeys.Remove(cyclicAddForeignKeyOperation);
-                        constraintOperations.Add(cyclicAddForeignKeyOperation);
+                        var removed = createTableOperation.ForeignKeys.Remove(cyclicAddForeignKeyOperation);
+                        if (removed)
+                        {
+                            constraintOperations.Add(cyclicAddForeignKeyOperation);
+                        }
+                        else
+                        {
+                            Check.DebugAssert(false, "Operation removed twice: " + cyclicAddForeignKeyOperation.ToString());
+                        }
                     }
 
                     return true;

diff --git a/src/Shared/Multigraph.cs b/src/Shared/Multigraph.cs
@@ -16,8 +16,8 @@ internal class Multigraph<TVertex, TEdge> : Graph<TVertex>
         private readonly Dictionary<TVertex, Dictionary<TVertex, List<TEdge>>> _successorMap =
             new Dictionary<TVertex, Dictionary<TVertex, List<TEdge>>>();
 
-        private readonly Dictionary<TVertex, List<TVertex>> _predecessorMap =
-            new Dictionary<TVertex, List<TVertex>>();
+        private readonly Dictionary<TVertex, HashSet<TVertex>> _predecessorMap =
+            new Dictionary<TVertex, HashSet<TVertex>>();
 
         public IEnumerable<TEdge> Edges
             => _successorMap.Values.SelectMany(s => s.Values).SelectMany(e => e).Distinct();
@@ -71,7 +71,7 @@ public void AddEdge([NotNull] TVertex from, [NotNull] TVertex to, [CanBeNull] TE
 
             if (!_predecessorMap.TryGetValue(to, out var predecessors))
             {
-                predecessors = new List<TVertex>();
+                predecessors = new HashSet<TVertex>();
                 _predecessorMap.Add(to, predecessors);
             }
 
@@ -108,7 +108,7 @@ public void AddEdges([NotNull] TVertex from, [NotNull] TVertex to, [NotNull] IEn
 
             if (!_predecessorMap.TryGetValue(to, out var predecessors))
             {
-                predecessors = new List<TVertex>();
+                predecessors = new HashSet<TVertex>();
                 _predecessorMap.Add(to, predecessors);
             }
 
@@ -126,15 +126,15 @@ public IReadOnlyList<TVertex> TopologicalSort()
             => TopologicalSort(null, null);
 
         public IReadOnlyList<TVertex> TopologicalSort(
-            [CanBeNull] Func<TVertex, TVertex, IEnumerable<TEdge>, bool> canBreakEdge)
-            => TopologicalSort(canBreakEdge, null);
+            [CanBeNull] Func<TVertex, TVertex, IEnumerable<TEdge>, bool> tryBreakEdge)
+            => TopologicalSort(tryBreakEdge, null);
 
         public IReadOnlyList<TVertex> TopologicalSort(
             [CanBeNull] Func<IEnumerable<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle)
             => TopologicalSort(null, formatCycle);
 
         public IReadOnlyList<TVertex> TopologicalSort(
-            [CanBeNull] Func<TVertex, TVertex, IEnumerable<TEdge>, bool> canBreakEdge,
+            [CanBeNull] Func<TVertex, TVertex, IEnumerable<TEdge>, bool> tryBreakEdge,
             [CanBeNull] Func<IReadOnlyList<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle)
         {
             var sortedQueue = new List<TVertex>();
@@ -195,7 +195,7 @@ public IReadOnlyList<TVertex> TopologicalSort(
                     // Iterate over the unsorted vertices
                     while ((candidateIndex < candidateVertices.Count)
                         && !broken
-                        && (canBreakEdge != null))
+                        && tryBreakEdge != null)
                     {
                         var candidateVertex = candidateVertices[candidateIndex];
 
@@ -206,7 +206,7 @@ public IReadOnlyList<TVertex> TopologicalSort(
                         foreach (var incomingNeighbor in incomingNeighbors)
                         {
                             // Check to see if the edge can be broken
-                            if (canBreakEdge(incomingNeighbor, candidateVertex, _successorMap[incomingNeighbor][candidateVertex]))
+                            if (tryBreakEdge(incomingNeighbor, candidateVertex, _successorMap[incomingNeighbor][candidateVertex]))
                             {
                                 predecessorCounts[candidateVertex]--;
                                 if (predecessorCounts[candidateVertex] == 0)

diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs
@@ -270,6 +270,79 @@ public void Model_differ_breaks_foreign_key_cycles_in_drop_table_operations()
                 });
         }
 
+        [ConditionalFact]
+        public void Model_differ_breaks_double_foreign_key_cycles_in_create_table_operations()
+        {
+            Execute(
+                _ => { },
+                modelBuilder =>
+                {
+                    modelBuilder
+                        .Entity("Banks")
+                        .Property<string>("Id");
+
+                    modelBuilder
+                        .Entity("BankRegistrations")
+                        .Property<string>("Id");
+
+                    modelBuilder
+                        .Entity("BankProfiles")
+                        .Property<string>("Id");
+
+                    modelBuilder
+                        .Entity("Banks")
+                        .HasOne("BankRegistrations")
+                        .WithOne()
+                        .HasForeignKey("Banks", "DefaultBankRegistrationId");
+
+                    modelBuilder
+                        .Entity("Banks")
+                        .HasOne("BankRegistrations")
+                        .WithOne()
+                        .HasForeignKey("Banks", "StagingBankRegistrationId");
+
+                    modelBuilder
+                        .Entity("Banks")
+                        .HasOne("BankProfiles")
+                        .WithOne()
+                        .HasForeignKey("Banks", "DefaultBankProfileId");
+
+                    modelBuilder
+                        .Entity("Banks")
+                        .HasOne("BankProfiles")
+                        .WithOne()
+                        .HasForeignKey("Banks", "StagingBankProfileId");
+
+                    modelBuilder
+                        .Entity("BankRegistrations")
+                        .HasOne("Banks")
+                        .WithMany()
+                        .HasForeignKey("BankId");
+
+                    modelBuilder
+                        .Entity("BankProfiles")
+                        .HasOne("Banks")
+                        .WithMany()
+                        .HasForeignKey("BankId");
+
+                    modelBuilder
+                        .Entity("BankProfiles")
+                        .HasOne("BankRegistrations")
+                        .WithMany()
+                        .HasForeignKey("BankRegistrationId");
+                },
+                result =>
+                {
+                    Assert.Equal(14, result.Count);
+
+                    var createBankTableOperation = Assert.IsType<CreateTableOperation>(result[0]);
+                    Assert.Equal("Banks", createBankTableOperation.Name);
+                    Assert.Equal(0, createBankTableOperation.ForeignKeys.Count);
+
+                    Assert.Equal(4, result.OfType<AddForeignKeyOperation>().Count());
+                });
+        }
+
         [ConditionalFact]
         public void Create_table()
         {