[release/5.0] Don't compare values from different keys for deleted en…

…tities (#24244) * Don't compare values from different keys in for deleted entities Fixes #24221 * Add quirk mode for #24221
dotnet · Mar 10, 2021 · d0f7671 · d0f7671
1 parent 2cec5f7
commit d0f7671
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 8 deletions.
diff --git a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs
@@ -625,14 +625,14 @@ private void AddForeignKeyEdges(
             }
         }
 
-        private static void AddMatchingPredecessorEdge(
-            Dictionary<IKeyValueIndex, List<ModificationCommand>> predecessorsMap,
-            IKeyValueIndex dependentKeyValue,
+        private static void AddMatchingPredecessorEdge<T>(
+            Dictionary<T, List<ModificationCommand>> predecessorsMap,
+            T keyValue,
             Multigraph<ModificationCommand, IAnnotatable> commandGraph,
             ModificationCommand command,
             IAnnotatable edge)
         {
-            if (predecessorsMap.TryGetValue(dependentKeyValue, out var predecessorCommands))
+            if (predecessorsMap.TryGetValue(keyValue, out var predecessorCommands))
             {
                 foreach (var predecessor in predecessorCommands)
                 {
@@ -647,7 +647,7 @@ private static void AddMatchingPredecessorEdge(
         private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> commandGraph)
         {
             Dictionary<IIndex, Dictionary<object[], ModificationCommand>> indexPredecessorsMap = null;
-            var keyPredecessorsMap = new Dictionary<IKeyValueIndex, List<ModificationCommand>>();
+            var keyPredecessorsMap = new Dictionary<(IKey, IKeyValueIndex), List<ModificationCommand>>();
             foreach (var command in commandGraph.Vertices)
             {
                 if (command.EntityState != EntityState.Modified
@@ -697,10 +697,10 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c
 
                         if (principalKeyValue != null)
                         {
-                            if (!keyPredecessorsMap.TryGetValue(principalKeyValue, out var predecessorCommands))
+                            if (!keyPredecessorsMap.TryGetValue((GetKey(key), principalKeyValue), out var predecessorCommands))
                             {
                                 predecessorCommands = new List<ModificationCommand>();
-                                keyPredecessorsMap.Add(principalKeyValue, predecessorCommands);
+                                keyPredecessorsMap.Add((GetKey(key), principalKeyValue), predecessorCommands);
                             }
 
                             predecessorCommands.Add(command);
@@ -761,12 +761,18 @@ private void AddUniqueValueEdges(Multigraph<ModificationCommand, IAnnotatable> c
                             if (principalKeyValue != null)
                             {
                                 AddMatchingPredecessorEdge(
-                                    keyPredecessorsMap, principalKeyValue, commandGraph, command, key);
+                                    keyPredecessorsMap, (GetKey(key), principalKeyValue), commandGraph, command, key);
                             }
                         }
                     }
                 }
             }
         }
+
+        private static readonly bool _useOldKeyComparison =
+            AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue24221", out var enabled) && enabled;
+
+        private IKey GetKey(IKey key)
+            => _useOldKeyComparison ? null : key;
     }
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs b/test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs
@@ -329,6 +329,70 @@ private class BNum
             public string TheWalrus { get; set; }
         }
 
+        [ConditionalFact]
+        public async Task Can_add_and_remove_entities_with_keys_of_different_type()
+        {
+            using var testDatabase = SqlServerTestStore.CreateInitialized(DatabaseName);
+
+            var options = Fixture.CreateOptions(testDatabase);
+            using (var context = new CompositeKeysDbContext(options))
+            {
+                context.Database.EnsureCreatedResiliently();
+                var first = new Int32CompositeKeys
+                {
+                    Id1 = 1, Id2 = 2
+                };
+
+                context.Add(first);
+
+                var second = new Int64CompositeKeys
+                {
+                    Id1 = 1,
+                    Id2 = 2
+                };
+
+                context.Add(second);
+                await context.SaveChangesAsync();
+            }
+
+            using (var context = new CompositeKeysDbContext(options))
+            {
+                var first = context.Set<Int32CompositeKeys>().Single();
+                context.Remove(first);
+
+                var second = context.Set<Int64CompositeKeys>().Single();
+                context.Remove(second);
+
+                await context.SaveChangesAsync();
+            }
+        }
+
+        private class CompositeKeysDbContext : DbContext
+        {
+            public CompositeKeysDbContext(DbContextOptions options)
+                : base(options)
+            {
+            }
+
+            protected override void OnModelCreating(ModelBuilder modelBuilder)
+            {
+                modelBuilder.Entity<Int32CompositeKeys>().HasKey(i => new { i.Id1, i.Id2 });
+                modelBuilder.Entity<Int64CompositeKeys>().HasKey(l => new { l.Id1, l.Id2 });
+            }
+        }
+
+        private class Int32CompositeKeys
+        {
+            public int Id1 { get; set; }
+            public int Id2 { get; set; }
+        }
+
+        private class Int64CompositeKeys
+        {
+            public long Id1 { get; set; }
+            public long Id2 { get; set; }
+        }
+
         [ConditionalFact]
         public void Can_insert_non_owner_principal_for_owned()
         {