From 58cc57ab0dd3665dad6011db220cafe73e93b6ca Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Fri, 4 Sep 2020 10:10:49 -0700 Subject: [PATCH] Correctly break double cycles in FK constraints Fixes #22201 --- .../Internal/MigrationsModelDiffer.cs | 11 ++- src/Shared/Multigraph.cs | 18 ++--- .../Internal/MigrationsModelDifferTest.cs | 73 +++++++++++++++++++ 3 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs index 416860dae4a..73291ab01b6 100644 --- a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs +++ b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs @@ -309,8 +309,15 @@ protected virtual IReadOnlyList 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 index 5723a8ca0c9..1119bc7648a 100644 --- a/src/Shared/Multigraph.cs +++ b/src/Shared/Multigraph.cs @@ -16,8 +16,8 @@ internal class Multigraph : Graph private readonly Dictionary>> _successorMap = new Dictionary>>(); - private readonly Dictionary> _predecessorMap = - new Dictionary>(); + private readonly Dictionary> _predecessorMap = + new Dictionary>(); public IEnumerable 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(); + predecessors = new HashSet(); _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(); + predecessors = new HashSet(); _predecessorMap.Add(to, predecessors); } @@ -126,15 +126,15 @@ public IReadOnlyList TopologicalSort() => TopologicalSort(null, null); public IReadOnlyList TopologicalSort( - [CanBeNull] Func, bool> canBreakEdge) - => TopologicalSort(canBreakEdge, null); + [CanBeNull] Func, bool> tryBreakEdge) + => TopologicalSort(tryBreakEdge, null); public IReadOnlyList TopologicalSort( [CanBeNull] Func>>, string> formatCycle) => TopologicalSort(null, formatCycle); public IReadOnlyList TopologicalSort( - [CanBeNull] Func, bool> canBreakEdge, + [CanBeNull] Func, bool> tryBreakEdge, [CanBeNull] Func>>, string> formatCycle) { var sortedQueue = new List(); @@ -195,7 +195,7 @@ public IReadOnlyList 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 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 index a55ce99ba73..8cae27110a7 100644 --- 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("Id"); + + modelBuilder + .Entity("BankRegistrations") + .Property("Id"); + + modelBuilder + .Entity("BankProfiles") + .Property("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(result[0]); + Assert.Equal("Banks", createBankTableOperation.Name); + Assert.Equal(0, createBankTableOperation.ForeignKeys.Count); + + Assert.Equal(4, result.OfType().Count()); + }); + } + [ConditionalFact] public void Create_table() {