From ec4cdb592bfe50cb062392355e6e8fc444cd72e0 Mon Sep 17 00:00:00 2001
From: Arthur Vickers <ajcvickers@hotmail.com>
Date: Fri, 16 Dec 2022 21:55:46 +0000
Subject: [PATCH 1/2] Update discriminator columns when PK-to-PK dependent type
 is changed

Fixes #29789

Also tests for #29874 and #29875
---
 .../Update/ModificationCommand.cs             |  5 +-
 .../UpdatesInMemoryTestBase.cs                | 27 ++++++--
 .../TestModels/UpdatesModel/Gift.cs           | 30 +++++++++
 .../TestModels/UpdatesModel/Lift.cs           | 32 ++++++++++
 .../UpdatesTestBase.cs                        | 64 +++++++++++++++++++
 .../UpdatesSqlServerTPCTest.cs                |  5 +-
 .../UpdatesSqlServerTPTTest.cs                |  9 ++-
 7 files changed, 163 insertions(+), 9 deletions(-)
 create mode 100644 test/EFCore.Specification.Tests/TestModels/UpdatesModel/Gift.cs
 create mode 100644 test/EFCore.Specification.Tests/TestModels/UpdatesModel/Lift.cs

diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs
index 8564e50d916..d1df0541c59 100644
--- a/src/EFCore.Relational/Update/ModificationCommand.cs
+++ b/src/EFCore.Relational/Update/ModificationCommand.cs
@@ -385,7 +385,10 @@ private List<IColumnModification> GenerateColumnModifications()
 
         foreach (var entry in _entries.Where(x => !x.EntityType.IsMappedToJson()))
         {
-            var nonMainEntry = !_mainEntryAdded || entry != _entries[0];
+            var nonMainEntry = (!_mainEntryAdded || entry != _entries[0])
+                || (updating
+                    && (entry.EntityState == EntityState.Deleted
+                        || entry.EntityState == EntityState.Added));
 
             var optionalDependentWithAllNull = false;
 
diff --git a/test/EFCore.InMemory.FunctionalTests/UpdatesInMemoryTestBase.cs b/test/EFCore.InMemory.FunctionalTests/UpdatesInMemoryTestBase.cs
index 273343150df..da4722ac932 100644
--- a/test/EFCore.InMemory.FunctionalTests/UpdatesInMemoryTestBase.cs
+++ b/test/EFCore.InMemory.FunctionalTests/UpdatesInMemoryTestBase.cs
@@ -22,8 +22,14 @@ protected override void ExecuteWithStrategyInTransaction(
         Action<UpdatesContext> nestedTestOperation1 = null,
         Action<UpdatesContext> nestedTestOperation2 = null)
     {
-        base.ExecuteWithStrategyInTransaction(testOperation, nestedTestOperation1, nestedTestOperation2);
-        Fixture.Reseed();
+        try
+        {
+            base.ExecuteWithStrategyInTransaction(testOperation, nestedTestOperation1, nestedTestOperation2);
+        }
+        finally
+        {
+            Fixture.Reseed();
+        }
     }
 
     protected override async Task ExecuteWithStrategyInTransactionAsync(
@@ -31,8 +37,21 @@ protected override async Task ExecuteWithStrategyInTransactionAsync(
         Func<UpdatesContext, Task> nestedTestOperation1 = null,
         Func<UpdatesContext, Task> nestedTestOperation2 = null)
     {
-        await base.ExecuteWithStrategyInTransactionAsync(testOperation, nestedTestOperation1, nestedTestOperation2);
-        Fixture.Reseed();
+        try
+        {
+            await base.ExecuteWithStrategyInTransactionAsync(testOperation, nestedTestOperation1, nestedTestOperation2);
+        }
+        finally
+        {
+            Fixture.Reseed();
+        }
+    }
+
+    // Issue #29875
+    public override Task Can_change_type_of_pk_to_pk_dependent_by_replacing_with_new_dependent(bool async)
+    {
+        return Assert.ThrowsAsync<DbUpdateConcurrencyException>(
+            () => base.Can_change_type_of_pk_to_pk_dependent_by_replacing_with_new_dependent(async));
     }
 
     public abstract class UpdatesInMemoryFixtureBase : UpdatesFixtureBase
diff --git a/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Gift.cs b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Gift.cs
new file mode 100644
index 00000000000..08d2cde5283
--- /dev/null
+++ b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Gift.cs
@@ -0,0 +1,30 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#nullable enable
+
+namespace Microsoft.EntityFrameworkCore.TestModels.UpdatesModel;
+
+public class Gift
+{
+    public int Id { get; set; }
+    public string? Recipient { get; set; }
+
+    public GiftObscurer? Obscurer { get; set; }
+}
+
+public abstract class GiftObscurer
+{
+    public int Id { get; set; }
+    public string? Pattern { get; set; }
+}
+
+public class GiftBag : GiftObscurer
+{
+    public int Size { get; set; }
+}
+
+public class GiftPaper : GiftObscurer
+{
+    public int Thickness { get; set; }
+}
diff --git a/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Lift.cs b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Lift.cs
new file mode 100644
index 00000000000..f2c3f9f2f62
--- /dev/null
+++ b/test/EFCore.Specification.Tests/TestModels/UpdatesModel/Lift.cs
@@ -0,0 +1,32 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#nullable enable
+
+namespace Microsoft.EntityFrameworkCore.TestModels.UpdatesModel;
+
+public class Lift
+{
+    public int Id { get; set; }
+    public string? Recipient { get; set; }
+
+    public LiftObscurer Obscurer { get; set; } = null!;
+}
+
+public abstract class LiftObscurer
+{
+    public int Id { get; set; }
+
+    public int LiftId { get; set; }
+    public string? Pattern { get; set; }
+}
+
+public class LiftBag : LiftObscurer
+{
+    public int Size { get; set; }
+}
+
+public class LiftPaper : LiftObscurer
+{
+    public int Thickness { get; set; }
+}
diff --git a/test/EFCore.Specification.Tests/UpdatesTestBase.cs b/test/EFCore.Specification.Tests/UpdatesTestBase.cs
index d5f764e176a..e5c91320252 100644
--- a/test/EFCore.Specification.Tests/UpdatesTestBase.cs
+++ b/test/EFCore.Specification.Tests/UpdatesTestBase.cs
@@ -59,6 +59,60 @@ public virtual async Task Can_delete_and_add_for_same_key(bool async)
                 Assert.Equal(EntityState.Detached, context.Entry(rodney1).State);
             });
 
+    [ConditionalTheory] // Issue #29789
+    [InlineData(false)]
+    [InlineData(true)]
+    public virtual async Task Can_change_type_of_pk_to_pk_dependent_by_replacing_with_new_dependent(bool async)
+        => await ExecuteWithStrategyInTransactionAsync(
+            async context =>
+            {
+                var gift = new Gift { Recipient = "Alice", Obscurer = new GiftPaper { Pattern = "Stripes" } };
+                await context.AddAsync(gift);
+                _ = async ? await context.SaveChangesAsync() : context.SaveChanges();
+            },
+            async context =>
+            {
+                var gift = await context.Set<Gift>().Include(e => e.Obscurer).SingleAsync();
+                var bag = new GiftBag { Pattern = "Gold stars" };
+                gift.Obscurer = bag;
+                _ = async ? await context.SaveChangesAsync() : context.SaveChanges();
+            },
+            async context =>
+            {
+                var gift = await context.Set<Gift>().Include(e => e.Obscurer).SingleAsync();
+
+                Assert.IsType<GiftBag>(gift.Obscurer);
+                Assert.Equal(gift.Id, gift.Obscurer.Id);
+                Assert.Single(context.Set<GiftObscurer>());
+            });
+
+    [ConditionalTheory]
+    [InlineData(false)]
+    [InlineData(true)]
+    public virtual async Task Can_change_type_of__dependent_by_replacing_with_new_dependent(bool async)
+        => await ExecuteWithStrategyInTransactionAsync(
+            async context =>
+            {
+                var lift = new Lift { Recipient = "Alice", Obscurer = new LiftPaper { Pattern = "Stripes" } };
+                await context.AddAsync(lift);
+                _ = async ? await context.SaveChangesAsync() : context.SaveChanges();
+            },
+            async context =>
+            {
+                var lift = await context.Set<Lift>().Include(e => e.Obscurer).SingleAsync();
+                var bag = new LiftBag { Pattern = "Gold stars" };
+                lift.Obscurer = bag;
+                _ = async ? await context.SaveChangesAsync() : context.SaveChanges();
+            },
+            async context =>
+            {
+                var lift = await context.Set<Lift>().Include(e => e.Obscurer).SingleAsync();
+
+                Assert.IsType<LiftBag>(lift.Obscurer);
+                Assert.Equal(lift.Id, lift.Obscurer.LiftId);
+                Assert.Single(context.Set<LiftObscurer>());
+            });
+
     [ConditionalFact]
     public virtual void Mutation_of_tracked_values_does_not_mutate_values_in_store()
     {
@@ -772,6 +826,16 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                         .WithOne(l => l.Profile)
                         .IsRequired();
                 });
+
+            modelBuilder.Entity<Gift>();
+            modelBuilder.Entity<GiftObscurer>().HasOne<Gift>().WithOne(x => x.Obscurer).HasForeignKey<GiftObscurer>(e => e.Id);
+            modelBuilder.Entity<GiftBag>();
+            modelBuilder.Entity<GiftPaper>();
+
+            modelBuilder.Entity<Lift>();
+            modelBuilder.Entity<LiftObscurer>().HasOne<Lift>().WithOne(x => x.Obscurer).HasForeignKey<LiftObscurer>(e => e.LiftId);
+            modelBuilder.Entity<LiftBag>();
+            modelBuilder.Entity<LiftPaper>();
         }
 
         protected override void Seed(UpdatesContext context)
diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs
index 423609c9e4f..7b37353e709 100644
--- a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs
+++ b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPCTest.cs
@@ -110,8 +110,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         {
             base.OnModelCreating(modelBuilder, context);
 
-            modelBuilder.Entity<Category>()
-                .UseTpcMappingStrategy();
+            modelBuilder.Entity<Category>().UseTpcMappingStrategy();
+            // modelBuilder.Entity<GiftObscurer>().UseTpcMappingStrategy(); Issue #29874
+            modelBuilder.Entity<LiftObscurer>().UseTpcMappingStrategy();
         }
     }
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs
index bbc49a487a7..b10d8353e86 100644
--- a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs
+++ b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTPTTest.cs
@@ -15,6 +15,10 @@ public UpdatesSqlServerTPTTest(UpdatesSqlServerTPTFixture fixture, ITestOutputHe
     {
     }
 
+    [ConditionalTheory(Skip = "Issue #29874. Skipped because the database is in a bad state, but the test may or may not fail.")]
+    public override Task Can_change_type_of_pk_to_pk_dependent_by_replacing_with_new_dependent(bool async)
+        => Task.CompletedTask;
+
     public override void Save_with_shared_foreign_key()
     {
         base.Save_with_shared_foreign_key();
@@ -97,8 +101,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         {
             base.OnModelCreating(modelBuilder, context);
 
-            modelBuilder.Entity<Category>()
-                .UseTptMappingStrategy();
+            modelBuilder.Entity<Category>().UseTptMappingStrategy();
+            modelBuilder.Entity<GiftObscurer>().UseTptMappingStrategy();
+            modelBuilder.Entity<LiftObscurer>().UseTptMappingStrategy();
         }
     }
 }

From a15c082a76cb963c97bbdad874c49f60d7463286 Mon Sep 17 00:00:00 2001
From: Arthur Vickers <ajcvickers@hotmail.com>
Date: Sat, 17 Dec 2022 10:46:05 +0000
Subject: [PATCH 2/2] Fix as suggested by Andriy.

---
 src/EFCore.Relational/Update/ModificationCommand.cs | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs
index d1df0541c59..5fd9ecccdd9 100644
--- a/src/EFCore.Relational/Update/ModificationCommand.cs
+++ b/src/EFCore.Relational/Update/ModificationCommand.cs
@@ -385,10 +385,7 @@ private List<IColumnModification> GenerateColumnModifications()
 
         foreach (var entry in _entries.Where(x => !x.EntityType.IsMappedToJson()))
         {
-            var nonMainEntry = (!_mainEntryAdded || entry != _entries[0])
-                || (updating
-                    && (entry.EntityState == EntityState.Deleted
-                        || entry.EntityState == EntityState.Added));
+            var nonMainEntry = !_mainEntryAdded || entry != _entries[0];
 
             var optionalDependentWithAllNull = false;
 
@@ -545,7 +542,8 @@ void HandleColumnModification(IColumnMappingBase columnMapping)
                         writeValue = property.GetBeforeSaveBehavior() == PropertySaveBehavior.Save;
                     }
                     else if (((updating && property.GetAfterSaveBehavior() == PropertySaveBehavior.Save)
-                                 || (!isKey && nonMainEntry))
+                                 || (!isKey && nonMainEntry)
+                                 || entry.SharedIdentityEntry != null)
                              && storedProcedureParameter is not { ForOriginalValue: true })
                     {
                         // Note that for stored procedures we always need to send all parameters, regardless of whether the property
