From 7e65b82821ce2540d12d35c6449712f07c56a527 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Wed, 3 Jul 2019 16:58:36 -0700 Subject: [PATCH] Remove references to an entity when it is detached Fixes #15645 Fixes #13110 (was already working in 3.0, but added a test) Fixes #15500 (was already working in 3.0, but added a test) See also #16454 --- .../Internal/NavigationFixer.cs | 3 +- .../GraphUpdatesFixtureBase.cs | 61 +++++ .../GraphUpdatesTestBase.cs | 257 ++++++++++++++++++ .../PropertyValuesTestBase.cs | 25 +- .../ProxyGraphUpdatesTestBase.cs | 4 +- .../Internal/InternalEntityEntryTestBase.cs | 40 ++- 6 files changed, 383 insertions(+), 7 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs index e346306f6e2..7e1ce3bb812 100644 --- a/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs +++ b/src/EFCore/ChangeTracking/Internal/NavigationFixer.cs @@ -513,8 +513,7 @@ public virtual void StateChanged( { InitialFixup(entry, null, fromQuery: false); } - else if (entry.EntityState == EntityState.Detached - && oldState == EntityState.Deleted) + else if (entry.EntityState == EntityState.Detached) { DeleteFixup(entry); } diff --git a/test/EFCore.Specification.Tests/GraphUpdatesFixtureBase.cs b/test/EFCore.Specification.Tests/GraphUpdatesFixtureBase.cs index f592885b87a..b93a1dfcaf1 100644 --- a/test/EFCore.Specification.Tests/GraphUpdatesFixtureBase.cs +++ b/test/EFCore.Specification.Tests/GraphUpdatesFixtureBase.cs @@ -413,6 +413,9 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con modelBuilder.Entity(); modelBuilder.Entity(); modelBuilder.Entity(); + + modelBuilder.Entity(); + modelBuilder.Entity(); } protected virtual object CreateFullGraph() @@ -690,6 +693,18 @@ protected override void Seed(PoolableDbContext context) ChildAsAParent = new ChildAsAParent() }); + var bloog = new Bloog { Id = 515 }; + + context.AddRange( + new Poost + { + Id = 516, Bloog = bloog + }, + new Poost + { + Id = 517, Bloog = bloog + }); + context.SaveChanges(); } @@ -2844,6 +2859,52 @@ public ICollection Choices } } + protected class Bloog : NotifyingEntity + { + private int _id; + private IEnumerable _poosts = new ObservableHashSet(ReferenceEqualityComparer.Instance); + + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id + { + get => _id; + set => SetWithNotify(value, ref _id); + } + + public IEnumerable Poosts + { + get => _poosts; + set => SetWithNotify(value, ref _poosts); + } + } + + protected class Poost : NotifyingEntity + { + private int _id; + private int? _bloogId; + private Bloog _bloog; + + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id + { + get => _id; + set => SetWithNotify(value, ref _id); + } + + + public int? BloogId + { + get => _bloogId; + set => SetWithNotify(value, ref _bloogId); + } + + public Bloog Bloog + { + get => _bloog; + set => SetWithNotify(value, ref _bloog); + } + } + protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged { protected void SetWithNotify(T value, ref T field, [CallerMemberName] string propertyName = "") diff --git a/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs index e1253cc2df6..26fea91ad23 100644 --- a/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/GraphUpdatesTestBase.cs @@ -695,6 +695,263 @@ public virtual void Save_removed_optional_many_to_one_dependents( }); } + [ConditionalFact] + public virtual void Resetting_a_deleted_reference_fixes_up_again() + { + ExecuteWithStrategyInTransaction( + context => + { + var bloog = context.Set().Include(e => e.Poosts).Single(); + var poost1 = bloog.Poosts.First(); + var poost2 = bloog.Poosts.Skip(1).First(); + + Assert.Equal(2, bloog.Poosts.Count()); + Assert.Same(bloog, poost1.Bloog); + Assert.Same(bloog, poost2.Bloog); + + context.Remove(bloog); + + Assert.Equal(2, bloog.Poosts.Count()); + + if (Fixture.ForceClientNoAction) + { + Assert.Same(bloog, poost1.Bloog); + Assert.Same(bloog, poost2.Bloog); + } + else + { + Assert.Null(poost1.Bloog); + Assert.Null(poost2.Bloog); + } + + poost1.Bloog = bloog; + + Assert.Equal(2, bloog.Poosts.Count()); + + if (Fixture.ForceClientNoAction) + { + Assert.Same(bloog, poost1.Bloog); + Assert.Same(bloog, poost2.Bloog); + } + else + { + Assert.Same(bloog, poost1.Bloog); + Assert.Null(poost2.Bloog); + } + + poost1.Bloog = null; + + Assert.Equal(2, bloog.Poosts.Count()); + + if (Fixture.ForceClientNoAction) + { + Assert.Null(poost1.Bloog); + Assert.Same(bloog, poost2.Bloog); + } + else + { + Assert.Null(poost1.Bloog); + Assert.Null(poost2.Bloog); + } + + if (!Fixture.ForceClientNoAction) + { + context.SaveChanges(); + + Assert.Equal(2, bloog.Poosts.Count()); + Assert.Null(poost1.Bloog); + Assert.Null(poost2.Bloog); + } + }); + } + + [ConditionalFact] + public virtual void Detaching_principal_entity_will_remove_references_to_it() + { + ExecuteWithStrategyInTransaction( + context => + { + var root = LoadOptionalGraph(context); + LoadRequiredGraph(context); + LoadOptionalAkGraph(context); + LoadRequiredAkGraph(context); + LoadRequiredCompositeGraph(context); + LoadRequiredNonPkGraph(context); + LoadOptionalOneToManyGraph(context); + LoadRequiredNonPkAkGraph(context); + + Assert.Same(root, root.OptionalSingle.Root); + Assert.Same(root, root.RequiredSingle.Root); + Assert.Same(root, root.OptionalSingleAk.Root); + Assert.Same(root, root.OptionalSingleDerived.DerivedRoot); + Assert.Same(root, root.RequiredSingleAk.Root); + Assert.Same(root, root.OptionalSingleAkDerived.DerivedRoot); + Assert.Same(root, root.OptionalSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, root.RequiredNonPkSingle.Root); + Assert.Same(root, root.OptionalSingleAkMoreDerived.MoreDerivedRoot); + Assert.Same(root, root.RequiredNonPkSingleAk.Root); + Assert.Same(root, root.RequiredNonPkSingleDerived.DerivedRoot); + Assert.Same(root, root.RequiredNonPkSingleAkDerived.DerivedRoot); + Assert.Same(root, root.RequiredNonPkSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, root.RequiredNonPkSingleAkMoreDerived.MoreDerivedRoot); + + Assert.True(root.OptionalChildren.All(e => e.Parent == root)); + Assert.True(root.RequiredChildren.All(e => e.Parent == root)); + Assert.True(root.OptionalChildrenAk.All(e => e.Parent == root)); + Assert.True(root.RequiredChildrenAk.All(e => e.Parent == root)); + Assert.True(root.RequiredCompositeChildren.All(e => e.Parent == root)); + + context.Entry(root).State = EntityState.Detached; + + Assert.Null(root.OptionalSingle.Root); + Assert.Null(root.RequiredSingle.Root); + Assert.Null(root.OptionalSingleAk.Root); + Assert.Null(root.OptionalSingleDerived.DerivedRoot); + Assert.Null(root.RequiredSingleAk.Root); + Assert.Null(root.OptionalSingleAkDerived.DerivedRoot); + Assert.Null(root.OptionalSingleMoreDerived.MoreDerivedRoot); + Assert.Null(root.RequiredNonPkSingle.Root); + Assert.Null(root.OptionalSingleAkMoreDerived.MoreDerivedRoot); + Assert.Null(root.RequiredNonPkSingleAk.Root); + Assert.Null(root.RequiredNonPkSingleDerived.DerivedRoot); + Assert.Null(root.RequiredNonPkSingleAkDerived.DerivedRoot); + Assert.Null(root.RequiredNonPkSingleMoreDerived.MoreDerivedRoot); + Assert.Null(root.RequiredNonPkSingleAkMoreDerived.MoreDerivedRoot); + + Assert.True(root.OptionalChildren.All(e => e.Parent == null)); + Assert.True(root.RequiredChildren.All(e => e.Parent == null)); + Assert.True(root.OptionalChildrenAk.All(e => e.Parent == null)); + Assert.True(root.RequiredChildrenAk.All(e => e.Parent == null)); + Assert.True(root.RequiredCompositeChildren.All(e => e.Parent == null)); + }); + } + + [ConditionalFact] + public virtual void Detaching_dependent_entity_will_remove_references_to_it() + { + ExecuteWithStrategyInTransaction( + context => + { + var root = LoadOptionalGraph(context); + LoadRequiredGraph(context); + LoadOptionalAkGraph(context); + LoadRequiredAkGraph(context); + LoadRequiredCompositeGraph(context); + LoadRequiredNonPkGraph(context); + LoadOptionalOneToManyGraph(context); + LoadRequiredNonPkAkGraph(context); + + var optionalSingle = root.OptionalSingle; + var requiredSingle = root.RequiredSingle; + var optionalSingleAk = root.OptionalSingleAk; + var optionalSingleDerived = root.OptionalSingleDerived; + var requiredSingleAk = root.RequiredSingleAk; + var optionalSingleAkDerived = root.OptionalSingleAkDerived; + var optionalSingleMoreDerived = root.OptionalSingleMoreDerived; + var requiredNonPkSingle = root.RequiredNonPkSingle; + var optionalSingleAkMoreDerived = root.OptionalSingleAkMoreDerived; + var requiredNonPkSingleAk = root.RequiredNonPkSingleAk; + var requiredNonPkSingleDerived = root.RequiredNonPkSingleDerived; + var requiredNonPkSingleAkDerived = root.RequiredNonPkSingleAkDerived; + var requiredNonPkSingleMoreDerived = root.RequiredNonPkSingleMoreDerived; + var requiredNonPkSingleAkMoreDerived = root.RequiredNonPkSingleAkMoreDerived; + + var optionalChildren = root.OptionalChildren; + var requiredChildren = root.RequiredChildren; + var optionalChildrenAk = root.OptionalChildrenAk; + var requiredChildrenAk = root.RequiredChildrenAk; + var requiredCompositeChildren = root.RequiredCompositeChildren; + var optionalChild = optionalChildren.First(); + var requiredChild = requiredChildren.First(); + var optionalChildAk = optionalChildrenAk.First(); + var requieredChildAk = requiredChildrenAk.First(); + var requiredCompositeChild = requiredCompositeChildren.First(); + + Assert.Same(root, optionalSingle.Root); + Assert.Same(root, requiredSingle.Root); + Assert.Same(root, optionalSingleAk.Root); + Assert.Same(root, optionalSingleDerived.DerivedRoot); + Assert.Same(root, requiredSingleAk.Root); + Assert.Same(root, optionalSingleAkDerived.DerivedRoot); + Assert.Same(root, optionalSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingle.Root); + Assert.Same(root, optionalSingleAkMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingleAk.Root); + Assert.Same(root, requiredNonPkSingleDerived.DerivedRoot); + Assert.Same(root, requiredNonPkSingleAkDerived.DerivedRoot); + Assert.Same(root, requiredNonPkSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingleAkMoreDerived.MoreDerivedRoot); + + Assert.True(optionalChildren.All(e => e.Parent == root)); + Assert.True(requiredChildren.All(e => e.Parent == root)); + Assert.True(optionalChildrenAk.All(e => e.Parent == root)); + Assert.True(requiredChildrenAk.All(e => e.Parent == root)); + Assert.True(requiredCompositeChildren.All(e => e.Parent == root)); + + context.Entry(optionalSingle).State = EntityState.Detached; + context.Entry(requiredSingle).State = EntityState.Detached; + context.Entry(optionalSingleAk).State = EntityState.Detached; + context.Entry(optionalSingleDerived).State = EntityState.Detached; + context.Entry(requiredSingleAk).State = EntityState.Detached; + context.Entry(optionalSingleAkDerived).State = EntityState.Detached; + context.Entry(optionalSingleMoreDerived).State = EntityState.Detached; + context.Entry(requiredNonPkSingle).State = EntityState.Detached; + context.Entry(optionalSingleAkMoreDerived).State = EntityState.Detached; + context.Entry(requiredNonPkSingleAk).State = EntityState.Detached; + context.Entry(requiredNonPkSingleDerived).State = EntityState.Detached; + context.Entry(requiredNonPkSingleAkDerived).State = EntityState.Detached; + context.Entry(requiredNonPkSingleMoreDerived).State = EntityState.Detached; + context.Entry(requiredNonPkSingleAkMoreDerived).State = EntityState.Detached; + context.Entry(optionalChild).State = EntityState.Detached; + context.Entry(requiredChild).State = EntityState.Detached; + context.Entry(optionalChildAk).State = EntityState.Detached; + context.Entry(requieredChildAk).State = EntityState.Detached; + context.Entry(requiredCompositeChild).State = EntityState.Detached; + + Assert.Same(root, optionalSingle.Root); + Assert.Same(root, requiredSingle.Root); + Assert.Same(root, optionalSingleAk.Root); + Assert.Same(root, optionalSingleDerived.DerivedRoot); + Assert.Same(root, requiredSingleAk.Root); + Assert.Same(root, optionalSingleAkDerived.DerivedRoot); + Assert.Same(root, optionalSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingle.Root); + Assert.Same(root, optionalSingleAkMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingleAk.Root); + Assert.Same(root, requiredNonPkSingleDerived.DerivedRoot); + Assert.Same(root, requiredNonPkSingleAkDerived.DerivedRoot); + Assert.Same(root, requiredNonPkSingleMoreDerived.MoreDerivedRoot); + Assert.Same(root, requiredNonPkSingleAkMoreDerived.MoreDerivedRoot); + + Assert.True(optionalChildren.All(e => e.Parent == root)); + Assert.True(requiredChildren.All(e => e.Parent == root)); + Assert.True(optionalChildrenAk.All(e => e.Parent == root)); + 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); + }); + } + [ConditionalTheory] [InlineData((int)ChangeMechanism.Principal, CascadeTiming.OnSaveChanges)] [InlineData((int)ChangeMechanism.Dependent, CascadeTiming.OnSaveChanges)] diff --git a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs index fb4f4b709b3..a2af8e593e3 100644 --- a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs +++ b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs @@ -1264,9 +1264,18 @@ public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(E { using (var context = CreateContext()) { + var office = new Office { Number = "35"}; + var mailRoom = new MailRoom { id = 36 }; var building = Building.Create(Guid.NewGuid(), "Bag End", 77); + + building.Offices.Add(office); + building.PrincipalMailRoom = mailRoom; + office.Building = building; + mailRoom.Building = building; + var entry = context.Entry(building); + context.Attach(building); entry.State = state; if (async) @@ -1282,7 +1291,21 @@ public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(E Assert.Equal("Bag End", entry.Property(e => e.Name).CurrentValue); Assert.Equal("Bag End", building.Name); - Assert.Equal(state == EntityState.Added ? EntityState.Added : EntityState.Detached, entry.State); + if (state == EntityState.Added) + { + Assert.Equal(EntityState.Added, entry.State); + Assert.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); + } + else + { + Assert.Equal(EntityState.Detached, entry.State); + Assert.Null(mailRoom.Building); + Assert.Same(state == EntityState.Deleted ? building : null, office.Building); + } + + Assert.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); } } diff --git a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs index 8809f128eb1..2fa53c47e89 100644 --- a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs @@ -680,8 +680,8 @@ public virtual void Save_required_one_to_one_changed_by_reference(ChangeMechanis Assert.Same(root, new1.Root); Assert.Same(new1, new2.Back); - Assert.Same(oldRoot, old1.Root); - Assert.Same(old1, old2.Back); + Assert.Null(old1.Root); + Assert.Null(old2.Back); Assert.Equal(old1.Id, old2.Id); }); } diff --git a/test/EFCore.Tests/ChangeTracking/Internal/InternalEntityEntryTestBase.cs b/test/EFCore.Tests/ChangeTracking/Internal/InternalEntityEntryTestBase.cs index d2d08463a97..1b612728c9c 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/InternalEntityEntryTestBase.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/InternalEntityEntryTestBase.cs @@ -79,15 +79,51 @@ public virtual void Changing_state_from_Unknown_causes_entity_to_start_tracking( } } + [ConditionalTheory] + [InlineData(false)] + [InlineData(true)] + public virtual void Changing_state_to_Unknown_causes_entity_to_stop_tracking(bool useTempValue) + { + using (var context = new TKContext()) + { + var entry1 = context.Entry(new TSomeEntity()).GetInfrastructure(); + var keyProperty = entry1.EntityType.FindProperty("Id"); + + if (useTempValue) + { + entry1.SetTemporaryValue(keyProperty, -1, setModified: false); + } + else + { + entry1[keyProperty] = -1; + } + + entry1.SetEntityState(EntityState.Added); + entry1.SetEntityState(EntityState.Detached); + + Assert.Equal(EntityState.Detached, entry1.EntityState); + Assert.DoesNotContain(entry1, context.GetService().Entries); + + var entry2 = context.Entry(new TSomeEntity()).GetInfrastructure(); + entry2[keyProperty] = -1; + + entry2.SetEntityState(EntityState.Added); + entry2.SetEntityState(EntityState.Detached); + + Assert.Equal(EntityState.Detached, entry2.EntityState); + Assert.DoesNotContain(entry2, context.GetService().Entries); + } + } + [ConditionalFact] - public virtual void Changing_state_to_Unknown_causes_entity_to_stop_tracking() + public virtual void Changing_state_to_Unknown_causes_entity_with_temporary_key_to_stop_tracking() { using (var context = new TKContext()) { var entry = context.Entry(new TSomeEntity()).GetInfrastructure(); var keyProperty = entry.EntityType.FindProperty("Id"); - entry[keyProperty] = 1; + entry.SetTemporaryValue(keyProperty, -1, setModified: false); entry.SetEntityState(EntityState.Added); entry.SetEntityState(EntityState.Detached);