From cbc094a4fc68ab866f2808d08d3f33415b2f9c1e Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Sat, 2 Dec 2023 10:49:44 +0000 Subject: [PATCH] Revert behavior to throw when attempting to modify an unconstrained alternate key property 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. --- .../Internal/NavigationFixer.cs | 43 +++++++------------ .../GraphUpdatesTestBaseMiscellaneous.cs | 20 ++++----- .../GraphUpdatesTestBaseOneToOne.cs | 12 +++--- 3 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index a88106c697a..86a1ae7a5ff 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/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; } } } diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs index 562fc561d44..1615ace95ef 100644 --- a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs +++ b/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( - 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 diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs index 12c2555559b..b746828a530 100644 --- a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs +++ b/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(() => context.SaveChanges()).Message); + Assert.Throws(() => 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(() => context.SaveChanges()).Message); + Assert.Throws(() => context.SaveChanges()); } else {