From 8e57ad42705f35d1cdbde810cdcbc5fdcd9b90e6 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Sun, 5 Feb 2023 11:54:53 +0000 Subject: [PATCH] Stop allowing identifying relationships from overriding cascade behavior Fixes #28961 In the issue, an alternate key was added over the the foreign key to make it effectively an identifying relationship. That is, changing the relationship changes the identity of the dependent. This resulted in the special handling for identifying relationships to kick in, which didn't respect the configured DeleteBehavior. This change fixes that. --- .../Internal/NavigationFixer.cs | 43 ++++++++++------- .../GraphUpdates/GraphUpdatesTestBase.cs | 47 +++++++++++++++++++ .../GraphUpdatesTestBaseMiscellaneous.cs | 26 ++++++++++ .../GraphUpdatesTestBaseOneToOne.cs | 16 ++++--- .../GraphUpdatesSqlServerOwnedTest.cs | 4 ++ 5 files changed, 114 insertions(+), 22 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index ebb59381963..f8bfe00babc 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs @@ -1416,28 +1416,39 @@ private void ConditionallyNullForeignKeyProperties( if (foreignKey.IsRequired && hasOnlyKeyProperties - && dependentEntry.EntityState != EntityState.Detached) + && dependentEntry.EntityState != EntityState.Detached + && dependentEntry.EntityState != EntityState.Deleted) { - try + if (foreignKey.DeleteBehavior == DeleteBehavior.Cascade + || foreignKey.DeleteBehavior == DeleteBehavior.ClientCascade + || foreignKey.IsOwnership) { - _inFixup = true; - switch (dependentEntry.EntityState) + 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 { - 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; + _inFixup = false; } } - finally + else { - _inFixup = false; + throw new InvalidOperationException( + CoreStrings.KeyReadOnly(dependentProperties.First().Name, dependentEntry.EntityType.DisplayName())); } } } diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs index 9fce941f4dd..a36c8d7627f 100644 --- a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs @@ -548,6 +548,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con modelBuilder.Entity(); modelBuilder.Entity(); modelBuilder.Entity(); + + modelBuilder.Entity( + b => + { + b.HasOne(x => x.Parent).WithMany(x => x.Children).OnDelete(DeleteBehavior.Restrict); + b.HasAlternateKey(x => new { x.Id, x.ParentId }); + }); } protected virtual object CreateFullGraph() @@ -3983,6 +3990,46 @@ public virtual SecondLaw SecondLaw } } + protected class NaiveParent : NotifyingEntity + { + private Guid _id; + private readonly ICollection _children = new ObservableHashSet(); + + public Guid Id + { + get => _id; + set => SetWithNotify(value, ref _id); + } + + public virtual ICollection Children + => _children; + } + + protected class SneakyChild : NotifyingEntity + { + private Guid _id; + private Guid _parentId; + private NaiveParent _parent = null!; + + public Guid Id + { + get => _id; + set => SetWithNotify(value, ref _id); + } + + public Guid ParentId + { + get => _parentId; + set => SetWithNotify(value, ref _parentId); + } + + public virtual NaiveParent Parent + { + get => _parent; + set => SetWithNotify(value, ref _parent); + } + } + protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged { protected void SetWithNotify(T value, ref T field, [CallerMemberName] string propertyName = "") diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs index 2959e19cbcb..fab0f7d85bd 100644 --- a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs +++ b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs @@ -1932,4 +1932,30 @@ private static SecondLaw AddSecondLevel(bool thirdLevel1, bool thirdLevel2) return secondLevel; } + + [ConditionalTheory] // Issue #28961 + [InlineData(false)] + [InlineData(true)] + public virtual Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior(bool async) + => ExecuteWithStrategyInTransactionAsync( + async context => + { + var parent = new NaiveParent { Children = { new() } }; + context.Add(parent); + + _ = async + ? 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); + }); } diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs index 1ec8f5bf991..8eb8f4cbcce 100644 --- a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs +++ b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseOneToOne.cs @@ -857,7 +857,9 @@ public virtual void Save_required_one_to_one_changed_by_reference( if (Fixture.ForceClientNoAction) { - Assert.Throws(() => context.SaveChanges()); + Assert.Equal( + CoreStrings.KeyReadOnly(nameof(RequiredSingle1.Id), nameof(RequiredSingle1)), + Assert.Throws(() => context.SaveChanges()).Message); } else { @@ -1199,16 +1201,18 @@ 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.Throws(() => context.SaveChanges()); + Assert.Equal( + CoreStrings.KeyReadOnly(nameof(RequiredSingle1.Id), nameof(RequiredSingle1)), + Assert.Throws(() => context.SaveChanges()).Message); } else { + 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()); + context.SaveChanges(); Assert.False(context.ChangeTracker.HasChanges()); diff --git a/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs b/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs index f357c6c4902..3fcebae65e7 100644 --- a/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs @@ -38,6 +38,10 @@ public override Task Update_root_by_collection_replacement_of_deleted_third_leve public override Task Sever_relationship_that_will_later_be_deleted(bool async) => Task.CompletedTask; + // No owned types + public override Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior(bool async) + => Task.CompletedTask; + // Owned dependents are always loaded public override void Required_one_to_one_are_cascade_deleted_in_store( CascadeTiming? cascadeDeleteTiming,