Revert behavior to throw when attempting to modify an unconstrained alternate key property (#32492)

Fixes #28961
Reverts #30213 for #32385

In #32385, an unconstrained alternate key was added to the model purely to make a non-identifying relationship artificially identifying. #30213 attempted to fix this by throwing that the key was being modified. However, this scenario is very similar to the case for a many-to-many join type, where the composite primary key is also not the end of any relationship, but forces the two many-to-one relationships to be identifying.

I prepared a PR that would only throw if the key involved is alternate, but on reflection that doesn't seem like an appropriate distinction to make. So overall, I think we should just revert this change, which is what this PR does.

Author: ajcvickers

src/EFCore/ChangeTracking/Internal/NavigationFixer.cs

@@ -1414,39 +1414,28 @@ private void ConditionallyNullForeignKeyProperties(
 
         if (foreignKey.IsRequired
             && hasOnlyKeyProperties
-            && dependentEntry.EntityState != EntityState.Detached
-            && dependentEntry.EntityState != EntityState.Deleted)
+            && dependentEntry.EntityState != EntityState.Detached)
         {
-            if (foreignKey.DeleteBehavior == DeleteBehavior.Cascade
-                || foreignKey.DeleteBehavior == DeleteBehavior.ClientCascade
-                || foreignKey.IsOwnership)
+            try
             {
-                try
-                {
-                    _inFixup = true;
-                    switch (dependentEntry.EntityState)
-                    {
-                        case EntityState.Added:
-                            dependentEntry.SetEntityState(EntityState.Detached);
-                            DeleteFixup(dependentEntry);
-                            break;
-                        case EntityState.Unchanged:
-                        case EntityState.Modified:
-                            dependentEntry.SetEntityState(
-                                dependentEntry.SharedIdentityEntry != null ? EntityState.Detached : EntityState.Deleted);
-                            DeleteFixup(dependentEntry);
-                            break;
-                    }
-                }
-                finally
+                _inFixup = true;
+                switch (dependentEntry.EntityState)
                 {
-                    _inFixup = false;
+                    case EntityState.Added:
+                        dependentEntry.SetEntityState(EntityState.Detached);
+                        DeleteFixup(dependentEntry);
+                        break;
+                    case EntityState.Unchanged:
+                    case EntityState.Modified:
+                        dependentEntry.SetEntityState(
+                            dependentEntry.SharedIdentityEntry != null ? EntityState.Detached : EntityState.Deleted);
+                        DeleteFixup(dependentEntry);
+                        break;
                 }
             }
-            else
+            finally
             {
-                throw new InvalidOperationException(
-                    CoreStrings.KeyReadOnly(dependentProperties.First().Name, dependentEntry.EntityType.DisplayName()));
+                _inFixup = false;
             }
         }
     }

test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs

@@ -2082,7 +2082,7 @@ private static SecondLaw AddSecondLevel(bool thirdLevel1, bool thirdLevel2)
         return secondLevel;
     }
 
-    [ConditionalTheory] // Issue #28961
+    [ConditionalTheory] // Issue #28961 and Issue #32385
     [InlineData(false)]
     [InlineData(true)]
     public virtual Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior(bool async)
@@ -2096,16 +2096,14 @@ public virtual Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior
                     ? await context.SaveChangesAsync()
                     : context.SaveChanges();
 
-                Assert.Equal(
-                    CoreStrings.KeyReadOnly(nameof(SneakyChild.ParentId), nameof(SneakyChild)),
-                    (await Assert.ThrowsAsync<InvalidOperationException>(
-                        async () =>
-                        {
-                            parent.Children.Remove(parent.Children.First());
-                            _ = async
-                                ? await context.SaveChangesAsync()
-                                : context.SaveChanges();
-                        })).Message);
+                Assert.Equal(2, context.ChangeTracker.Entries().Count());
+
+                parent.Children.Remove(parent.Children.First());
+                _ = async
+                    ? await context.SaveChangesAsync()
+                    : context.SaveChanges();
+
+                Assert.Equal(1, context.ChangeTracker.Entries().Count());
             });
 
     [ConditionalTheory] // Issue #30764

test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs

@@ -856,9 +856,7 @@ public virtual void Save_required_one_to_one_changed_by_reference(
 
                 if (Fixture.ForceClientNoAction)
                 {
-                    Assert.Equal(
-                        CoreStrings.KeyReadOnly(nameof(RequiredSingle1.Id), nameof(RequiredSingle1)),
-                        Assert.Throws<InvalidOperationException>(() => context.SaveChanges()).Message);
+                    Assert.Throws<DbUpdateException>(() => context.SaveChanges());
                 }
                 else
                 {
@@ -1202,11 +1200,13 @@ public virtual void Sever_required_one_to_one(
                     throw new ArgumentOutOfRangeException(nameof(changeMechanism));
                 }
 
+                Assert.False(context.Entry(root).Reference(e => e.RequiredSingle).IsLoaded);
+                Assert.False(context.Entry(old1).Reference(e => e.Root).IsLoaded);
+                Assert.True(context.ChangeTracker.HasChanges());
+
                 if (Fixture.ForceClientNoAction)
                 {
-                    Assert.Equal(
-                        CoreStrings.KeyReadOnly(nameof(RequiredSingle1.Id), nameof(RequiredSingle1)),
-                        Assert.Throws<InvalidOperationException>(() => context.SaveChanges()).Message);
+                    Assert.Throws<DbUpdateException>(() => context.SaveChanges());
                 }
                 else
                 {