From 58440920b37dc463a5dd8f993f97c393073d944b Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 21 Oct 2019 08:02:44 -0700 Subject: [PATCH] Revert 3.0 fix for consistent reload and replace with fix that doesn't dramatically change detach behavior Fixes #17784 In 3.0 we made a [simple change](https://github.com/aspnet/EntityFrameworkCore/commit/7e65b82821ce2540d12d35c6449712f07c56a527#diff-7767de8ee880eb46efbfc9d2153dd33a) for #15645 that started clearing navigation properties when they referenced detached entities. In retrospect this was the wrong thing to do as people are relying on this to be able to detach all entities from the context. The new fix restores the pre-3.0 detach behavior, and sends the reload of a deleted entity through a simulated to delete instead. --- src/EFCore/ChangeTracking/EntityEntry.cs | 1 + .../Internal/NavigationFixer.cs | 3 +- .../GraphUpdatesTestBase.cs | 42 +++++++++---------- .../PropertyValuesTestBase.cs | 4 +- .../ProxyGraphUpdatesTestBase.cs | 2 +- 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/EFCore/ChangeTracking/EntityEntry.cs b/src/EFCore/ChangeTracking/EntityEntry.cs index 7c53262a47b..9007c747703 100644 --- a/src/EFCore/ChangeTracking/EntityEntry.cs +++ b/src/EFCore/ChangeTracking/EntityEntry.cs @@ -388,6 +388,7 @@ private void Reload(PropertyValues storeValues) { if (State != EntityState.Added) { + State = EntityState.Deleted; State = EntityState.Detached; } } diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index ca4efbffb8a..bc2f0a34caa 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs @@ -511,7 +511,8 @@ public virtual void StateChanged( { InitialFixup(entry, fromQuery: false); } - else if (entry.EntityState == EntityState.Detached) + else if (oldState == EntityState.Deleted + && entry.EntityState == EntityState.Detached) { DeleteFixup(entry); } diff --git a/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs index 80504aeb9bc..e63a1667d15 100644 --- a/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs @@ -836,7 +836,7 @@ public virtual void Detaching_principal_entity_will_remove_references_to_it() } [ConditionalFact] - public virtual void Detaching_dependent_entity_will_remove_references_to_it() + public virtual void Detaching_dependent_entity_will_not_remove_references_to_it() { ExecuteWithStrategyInTransaction( context => @@ -944,26 +944,26 @@ public virtual void Detaching_dependent_entity_will_remove_references_to_it() Assert.True(requiredChildrenAk.All(e => e.Parent == root)); Assert.True(requiredCompositeChildren.All(e => e.Parent == root)); - Assert.Null(root.OptionalSingle); - Assert.Null(root.RequiredSingle); - Assert.Null(root.OptionalSingleAk); - Assert.Null(root.OptionalSingleDerived); - Assert.Null(root.RequiredSingleAk); - Assert.Null(root.OptionalSingleAkDerived); - Assert.Null(root.OptionalSingleMoreDerived); - Assert.Null(root.RequiredNonPkSingle); - Assert.Null(root.OptionalSingleAkMoreDerived); - Assert.Null(root.RequiredNonPkSingleAk); - Assert.Null(root.RequiredNonPkSingleDerived); - Assert.Null(root.RequiredNonPkSingleAkDerived); - Assert.Null(root.RequiredNonPkSingleMoreDerived); - Assert.Null(root.RequiredNonPkSingleAkMoreDerived); - - Assert.DoesNotContain(optionalChild, root.OptionalChildren); - Assert.DoesNotContain(requiredChild, root.RequiredChildren); - Assert.DoesNotContain(optionalChildAk, root.OptionalChildrenAk); - Assert.DoesNotContain(requieredChildAk, root.RequiredChildrenAk); - Assert.DoesNotContain(requiredCompositeChild, root.RequiredCompositeChildren); + Assert.NotNull(root.OptionalSingle); + Assert.NotNull(root.RequiredSingle); + Assert.NotNull(root.OptionalSingleAk); + Assert.NotNull(root.OptionalSingleDerived); + Assert.NotNull(root.RequiredSingleAk); + Assert.NotNull(root.OptionalSingleAkDerived); + Assert.NotNull(root.OptionalSingleMoreDerived); + Assert.NotNull(root.RequiredNonPkSingle); + Assert.NotNull(root.OptionalSingleAkMoreDerived); + Assert.NotNull(root.RequiredNonPkSingleAk); + Assert.NotNull(root.RequiredNonPkSingleDerived); + Assert.NotNull(root.RequiredNonPkSingleAkDerived); + Assert.NotNull(root.RequiredNonPkSingleMoreDerived); + Assert.NotNull(root.RequiredNonPkSingleAkMoreDerived); + + Assert.Contains(optionalChild, root.OptionalChildren); + Assert.Contains(requiredChild, root.RequiredChildren); + Assert.Contains(optionalChildAk, root.OptionalChildrenAk); + Assert.Contains(requieredChildAk, root.RequiredChildrenAk); + Assert.Contains(requiredCompositeChild, root.RequiredCompositeChildren); }); } diff --git a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs index 87953299c03..5f679c0826a 100644 --- a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs +++ b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs @@ -1301,7 +1301,9 @@ public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(E { Assert.Equal(EntityState.Detached, entry.State); Assert.Null(mailRoom.Building); - Assert.Same(state == EntityState.Deleted ? building : null, office.Building); + + Assert.Equal(EntityState.Detached, context.Entry(office.Building).State); + Assert.Same(building, office.Building); } Assert.Same(mailRoom, building.PrincipalMailRoom); diff --git a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs index 7239f3be749..9eb0b5fcb37 100644 --- a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs @@ -682,7 +682,7 @@ public virtual void Save_required_one_to_one_changed_by_reference(ChangeMechanis Assert.Same(root, new1.Root); Assert.Same(new1, new2.Back); - Assert.Null(old1.Root); + Assert.NotNull(old1.Root); Assert.Null(old2.Back); Assert.Equal(old1.Id, old2.Id); });