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,