From d4a490e5e94e2a164c5a7c9f6f725a796554ae5f Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Sat, 21 Dec 2019 14:58:27 -0800 Subject: [PATCH 1/5] Prevent dependents from being deleted when principal is detached Fixes #12590 Fixes #18982 Regression test for #16546 --- .../ChangeTracking/Internal/StateManager.cs | 8 +- .../PropertyValuesTestBase.cs | 12 + .../ProxyGraphUpdatesTestBase.cs | 2 +- .../ChangeTracking/ChangeTrackerTest.cs | 317 ++++++++++++++++++ .../ChangeTracking/Internal/OwnedFixupTest.cs | 72 ++++ 5 files changed, 407 insertions(+), 4 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/StateManager.cs b/src/EFCore/ChangeTracking/Internal/StateManager.cs index f957a3a74b9..6e849ee6a51 100644 --- a/src/EFCore/ChangeTracking/Internal/StateManager.cs +++ b/src/EFCore/ChangeTracking/Internal/StateManager.cs @@ -959,6 +959,7 @@ public virtual void CascadeChanges(bool force) public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumerable foreignKeys = null) { var doCascadeDelete = force || CascadeDeleteTiming != CascadeTiming.Never; + var principalIsDetached = entry.EntityState == EntityState.Detached; foreignKeys ??= entry.EntityType.GetReferencingForeignKeys(); foreach (var fk in foreignKeys) @@ -980,9 +981,10 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer || fk.DeleteBehavior == DeleteBehavior.ClientCascade) && doCascadeDelete) { - var cascadeState = dependent.EntityState == EntityState.Added - ? EntityState.Detached - : EntityState.Deleted; + var cascadeState = principalIsDetached + || dependent.EntityState == EntityState.Added + ? EntityState.Detached + : EntityState.Deleted; if (SensitiveLoggingEnabled) { diff --git a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs index 2c51c06bab8..146ca9c80c9 100644 --- a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs +++ b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs @@ -1309,6 +1309,18 @@ public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(E Assert.Same(mailRoom, building.PrincipalMailRoom); Assert.Contains(office, building.Offices); } + else + { + Assert.Equal(EntityState.Detached, entry.State); + Assert.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); + + Assert.Equal(EntityState.Detached, context.Entry(office.Building).State); + Assert.Same(building, office.Building); + } + + Assert.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); } [ConditionalFact] diff --git a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs index b2da38772a0..cfbeb9d7581 100644 --- a/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs +++ b/test/EFCore.Specification.Tests/ProxyGraphUpdatesTestBase.cs @@ -683,7 +683,7 @@ public virtual void Save_required_one_to_one_changed_by_reference(ChangeMechanis Assert.Same(new1, new2.Back); Assert.NotNull(old1.Root); - Assert.Null(old2.Back); + Assert.Same(old1, old2.Back); Assert.Equal(old1.Id, old2.Id); }); } diff --git a/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs b/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs index 61388349b2f..33cee375b6a 100644 --- a/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs +++ b/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs @@ -2165,6 +2165,323 @@ public void Can_add_owned_dependent_with_reference_to_parent(bool useAdd, bool s } } + [ConditionalTheory] // Issue #17828 + [InlineData(false)] + [InlineData(true)] + public void DetectChanges_reparents_even_when_immediate_cascade_enabled(bool delayCascade) + { + using var context = new EarlyLearningCenter(); + + // Construct initial state + var parent1 = new Category { Id = 1 }; + var parent2 = new Category { Id = 2 }; + var child = new Product { Id = 3, Category = parent1 }; + + context.AddRange(parent1, parent2, child); + context.ChangeTracker.AcceptAllChanges(); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(parent1).State); + Assert.Equal(EntityState.Unchanged, context.Entry(parent2).State); + Assert.Equal(EntityState.Unchanged, context.Entry(child).State); + + if (delayCascade) + { + context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.OnSaveChanges; + } + + child.Category = parent2; + + context.ChangeTracker.DetectChanges(); + + context.Remove(parent1); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Deleted, context.Entry(parent1).State); + Assert.Equal(EntityState.Unchanged, context.Entry(parent2).State); + Assert.Equal(EntityState.Modified, context.Entry(child).State); + } + + [ConditionalTheory] // Issue #12590 + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void Dependents_are_detached_not_deleted_when_principal_is_detached(bool delayCascade, bool trackNewDependents) + { + using var context = new EarlyLearningCenter(); + + var category = new Category + { + Id = 1, + Products = new List + { + new Product { Id = 1 }, + new Product { Id = 2 }, + new Product { Id = 3 } + } + }; + + context.Attach(category); + + var categoryEntry = context.Entry(category); + var product0Entry = context.Entry(category.Products[0]); + var product1Entry = context.Entry(category.Products[1]); + var product2Entry = context.Entry(category.Products[2]); + + Assert.Equal(EntityState.Unchanged, categoryEntry.State); + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + + if (delayCascade) + { + context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.OnSaveChanges; + } + + context.Entry(category).State = EntityState.Detached; + + Assert.Equal(EntityState.Detached, categoryEntry.State); + + if (delayCascade) + { + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + } + else + { + Assert.Equal(EntityState.Detached, product0Entry.State); + Assert.Equal(EntityState.Detached, product1Entry.State); + Assert.Equal(EntityState.Detached, product2Entry.State); + } + + var newCategory = new Category { Id = 1, }; + + if (trackNewDependents) + { + newCategory.Products = new List + { + new Product { Id = 1 }, + new Product { Id = 2 }, + new Product { Id = 3 } + }; + } + + var traversal = new List(); + + if (delayCascade && trackNewDependents) + { + Assert.Equal( + CoreStrings.IdentityConflict(nameof(Product), "{'Id'}"), + Assert.Throws(TrackGraph).Message); + } + else + { + TrackGraph(); + + Assert.Equal( + trackNewDependents + ? new List + { + " -----> Category:1", + "Category:1 ---Products--> Product:1", + "Category:1 ---Products--> Product:2", + "Category:1 ---Products--> Product:3" + } + : new List + { + " -----> Category:1" + }, + traversal); + + if (trackNewDependents || delayCascade) + { + Assert.Equal(4, context.ChangeTracker.Entries().Count()); + + categoryEntry = context.Entry(newCategory); + product0Entry = context.Entry(newCategory.Products[0]); + product1Entry = context.Entry(newCategory.Products[1]); + product2Entry = context.Entry(newCategory.Products[2]); + + Assert.Equal(EntityState.Modified, categoryEntry.State); + + if (trackNewDependents) + { + Assert.Equal(EntityState.Modified, product0Entry.State); + Assert.Equal(EntityState.Modified, product1Entry.State); + Assert.Equal(EntityState.Modified, product2Entry.State); + + Assert.NotSame(newCategory.Products[0], category.Products[0]); + Assert.NotSame(newCategory.Products[1], category.Products[1]); + Assert.NotSame(newCategory.Products[2], category.Products[2]); + } + else + { + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + + Assert.Same(newCategory.Products[0], category.Products[0]); + Assert.Same(newCategory.Products[1], category.Products[1]); + Assert.Same(newCategory.Products[2], category.Products[2]); + } + + Assert.Same(newCategory, newCategory.Products[0].Category); + Assert.Same(newCategory, newCategory.Products[1].Category); + Assert.Same(newCategory, newCategory.Products[2].Category); + + Assert.Equal(newCategory.Id, product0Entry.Property("CategoryId").CurrentValue); + Assert.Equal(newCategory.Id, product1Entry.Property("CategoryId").CurrentValue); + Assert.Equal(newCategory.Id, product2Entry.Property("CategoryId").CurrentValue); + } + else + { + Assert.Single(context.ChangeTracker.Entries()); + + categoryEntry = context.Entry(newCategory); + + Assert.Equal(EntityState.Modified, categoryEntry.State); + Assert.Null(newCategory.Products); + } + } + + void TrackGraph() + { + context.ChangeTracker.TrackGraph( + newCategory, n => + { + n.Entry.State = EntityState.Modified; + traversal.Add(NodeString(n)); + }); + } + } + + [ConditionalTheory] // Issue #16546 + [InlineData(false)] + [InlineData(true)] + public void Optional_relationship_with_cascade_still_cascades(bool delayCascade) + { + Kontainer detachedContainer; + var databaseName = "K" + delayCascade; + using (var context = new KontainerContext(databaseName)) + { + context.Add( + new Kontainer + { + Name = "C1", + Rooms = { new KontainerRoom { Number = 1, Troduct = new Troduct { Description = "Heavy Engine XT3" } } } + } + ); + + context.SaveChanges(); + + detachedContainer = context.Set() + .Include(container => container.Rooms) + .ThenInclude(room => room.Troduct) + .AsNoTracking() + .Single(); + } + + using (var context = new KontainerContext(databaseName)) + { + var attachedContainer = context.Set() + .Include(container => container.Rooms) + .ThenInclude(room => room.Troduct) + .Single(); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer).State); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer.Rooms.Single()).State); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer.Rooms.Single().Troduct).State); + + var detachedRoom = detachedContainer.Rooms.Single(); + detachedRoom.Troduct = null; + detachedRoom.TroductId = null; + + var attachedRoom = attachedContainer.Rooms.Single(); + + if (delayCascade) + { + context.ChangeTracker.DeleteOrphansTiming = CascadeTiming.OnSaveChanges; + } + + context.Entry(attachedRoom).CurrentValues.SetValues(detachedRoom); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer).State); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer.Rooms.Single().Troduct).State); + + if (delayCascade) + { + Assert.Equal(EntityState.Modified, context.Entry(attachedContainer.Rooms.Single()).State); + } + else + { + // Deleted because FK with cascade has been set to null + Assert.Equal(EntityState.Deleted, context.Entry(attachedContainer.Rooms.Single()).State); + } + + context.ChangeTracker.CascadeChanges(); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer).State); + Assert.Equal(EntityState.Unchanged, context.Entry(attachedContainer.Rooms.Single().Troduct).State); + Assert.Equal(EntityState.Deleted, context.Entry(attachedContainer.Rooms.Single()).State); + + context.SaveChanges(); + } + } + + private class Kontainer + { + public int Id { get; set; } + public string Name { get; set; } + public List Rooms { get; set; } = new List(); + } + + private class KontainerRoom + { + public int Id { get; set; } + public int Number { get; set; } + public int KontainerId { get; set; } + public Kontainer Kontainer { get; set; } + public int? TroductId { get; set; } + public Troduct Troduct { get; set; } + } + + private class Troduct + { + public int Id { get; set; } + public string Description { get; set; } + public List Rooms { get; set; } = new List(); + } + + private class KontainerContext : DbContext + { + private readonly string _databaseName; + + public KontainerContext(string databaseName) + { + _databaseName = databaseName; + } + + protected internal override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity() + .HasOne(room => room.Troduct) + .WithMany(product => product.Rooms) + .HasForeignKey(room => room.TroductId) + .IsRequired(false) + .OnDelete(DeleteBehavior.Cascade); + } + + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder + .UseInternalServiceProvider(InMemoryFixture.DefaultServiceProvider) + .UseInMemoryDatabase(_databaseName); + } + [ConditionalTheory] [InlineData(false)] [InlineData(true)] diff --git a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs index 11f1bf30eb4..e253b9cf49c 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs @@ -22,6 +22,72 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal { public class OwnedFixupTest { + private class Thing + { + public Guid ThingId { get; set; } + public List OwnedByThings { get; set; } = new List(); + } + + private class OwnedByThing + { + public Guid OwnedByThingId { get; set; } + public Guid ThingId { get; set; } + public Thing Thing { get; set; } + } + + [ConditionalTheory] // Issue #18982 + [InlineData(false)] + [InlineData(true)] + public void Detaching_owner_does_not_delete_owned_entities(bool delayCascade) + { + using var context = new FixupContext(); + + var thing = new Thing + { + ThingId = Guid.NewGuid(), + OwnedByThings = new List + { + new OwnedByThing + { + OwnedByThingId = Guid.NewGuid() + }, + new OwnedByThing + { + OwnedByThingId = Guid.NewGuid() + } + } + }; + + context.Attach(thing); + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(thing).State); + Assert.Equal(EntityState.Unchanged, context.Entry(thing.OwnedByThings[0]).State); + Assert.Equal(EntityState.Unchanged, context.Entry(thing.OwnedByThings[1]).State); + + if (delayCascade) + { + context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.OnSaveChanges; + } + + context.Entry(thing).State = EntityState.Detached; + + if (delayCascade) + { + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Detached, context.Entry(thing).State); + Assert.Equal(EntityState.Unchanged, context.Entry(thing.OwnedByThings[0]).State); + Assert.Equal(EntityState.Unchanged, context.Entry(thing.OwnedByThings[1]).State); + } + else + { + Assert.Empty(context.ChangeTracker.Entries()); + Assert.Equal(EntityState.Detached, context.Entry(thing).State); + Assert.Equal(EntityState.Detached, context.Entry(thing.OwnedByThings[0]).State); + Assert.Equal(EntityState.Detached, context.Entry(thing.OwnedByThings[1]).State); + } + } + [ConditionalFact] public void Can_detach_Added_owner_referencing_detached_weak_owned_entity() { @@ -4294,6 +4360,12 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder) }); }); }); + + modelBuilder.Entity().OwnsMany(p => p.OwnedByThings, a => + { + a.WithOwner().HasForeignKey(e => e.ThingId); + a.HasKey(e => e.OwnedByThingId); + }); } protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) From 40c30b74919075c3b6614e8e2828492ce6829f24 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 15 Jul 2019 15:09:28 -0700 Subject: [PATCH 2/5] Tests for two recently filed issues Issue #16483 which is already fixed in 3.0 Issue #16546 which I thought might be fixed, but isn't --- .../PropertyValuesTestBase.cs | 12 - .../ChangeTracking/Internal/FixupTest.cs | 265 ++++++++++++++++++ 2 files changed, 265 insertions(+), 12 deletions(-) diff --git a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs index 146ca9c80c9..2c51c06bab8 100644 --- a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs +++ b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs @@ -1309,18 +1309,6 @@ public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(E Assert.Same(mailRoom, building.PrincipalMailRoom); Assert.Contains(office, building.Offices); } - else - { - Assert.Equal(EntityState.Detached, entry.State); - Assert.Same(mailRoom, building.PrincipalMailRoom); - Assert.Contains(office, building.Offices); - - Assert.Equal(EntityState.Detached, context.Entry(office.Building).State); - Assert.Same(building, office.Building); - } - - Assert.Same(mailRoom, building.PrincipalMailRoom); - Assert.Contains(office, building.Offices); } [ConditionalFact] diff --git a/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs index da10152ce84..fe6407f136c 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs @@ -7,6 +7,8 @@ using System.Linq; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata.Builders; +using Microsoft.EntityFrameworkCore.Metadata.Internal; using Microsoft.EntityFrameworkCore.Storage; using Xunit; @@ -2576,6 +2578,269 @@ protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBu [ConditionalFact] public void Use_correct_entity_after_SetValues() + { + var detachedProduct = new ProductX + { + Description = "Heavy Engine XT3" + }; + + var detachedRoom = new ContainerRoomX + { + Number = 1, + Product = detachedProduct + }; + + var detachedContainer = new ContainerX + { + Name = "C1", + Rooms = + { + detachedRoom + } + }; + + using (var context = new EscapeRoom(nameof(EscapeRoom))) + { + context.Add(detachedContainer); + context.SaveChanges(); + } + + using (var context = new EscapeRoom(nameof(EscapeRoom))) + { + var attachedProduct = new ProductX + { + Id = detachedProduct.Id, + Description = "Heavy Engine XT3" + }; + + var attachedRoom = new ContainerRoomX + { + Id = detachedRoom.Id, + ContainerId = detachedRoom.ContainerId, + Number = 1, + ProductId = detachedRoom.ProductId, + Product = attachedProduct + }; + + var attached = new ContainerX + { + Id = detachedContainer.Id, + Name = "C1", + Rooms = + { + attachedRoom + } + }; + + context.Attach(attached); + + detachedRoom.Product = null; + detachedRoom.ProductId = null; + + context.Entry(attachedRoom).CurrentValues.SetValues(detachedRoom); + + context.SaveChanges(); + + // Fails - see #16546 + //Assert.Equal(EntityState.Unchanged, context.Entry(attachedRoom).State); + } + } + + public class ContainerX + { + public int Id { get; set; } + public string Name { get; set; } + public List Rooms { get; set; } = new List(); + } + + public class ContainerRoomX + { + public int Id { get; set; } + public int Number { get; set; } + public int ContainerId { get; set; } + public ContainerX Container { get; set; } + public int? ProductId { get; set; } + public ProductX Product { get; set; } + } + + public class ProductX + { + public int Id { get; set; } + public string Description { get; set; } + public List Rooms { get; set; } = new List(); + } + + protected class EscapeRoom : DbContext + { + private readonly string _databaseName; + + public EscapeRoom(string databaseName) + { + _databaseName = databaseName; + } + + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder.UseInMemoryDatabase(_databaseName); + + protected internal override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity() + .HasOne(room => room.Product) + .WithMany(product => product.Rooms) + .HasForeignKey(room => room.ProductId) + .IsRequired(false) + .OnDelete(DeleteBehavior.Cascade); + } + } + + [ConditionalFact] + public void Replaced_duplicate_entities_are_used_even_with_bad_hash() + { + using (var context = new BadHashDay("BadHashDay")) + { + context.AddRange( + new ParentX + { + Id = 101, Name = "Parent1" + }, + new ChildX + { + Id = 201, Name = "Child1" + }, + new ParentChildX + { + ParentId = 101, ChildId = 201, SortOrder = 1 + }); + + context.SaveChanges(); + } + + using (var context = new BadHashDay("BadHashDay")) + { + var parent = context.Set().Single(x => x.Id == 101); + var join = context.Set().Single(); + + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); + Assert.Equal(EntityState.Unchanged, context.Entry(join).State); + + parent.ParentChildren.Clear(); + + var newJoin = new ParentChildX + { + ParentId = 101, + ChildId = 201, + SortOrder = 1 + }; + + parent.ParentChildren = new List + { + newJoin + }; + + Assert.Equal(3, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); + Assert.Equal(EntityState.Deleted, context.Entry(join).State); + Assert.Equal(EntityState.Added, context.Entry(newJoin).State); + + context.SaveChanges(); + + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); + Assert.Equal(EntityState.Detached, context.Entry(join).State); + Assert.Equal(EntityState.Unchanged, context.Entry(newJoin).State); + } + } + + protected class ParentX + { + public int Id { get; set; } + public string Name { get; set; } + public virtual IList ParentChildren { get; set; } = new List(); + } + + protected class ParentChildX + { + public int ParentId { get; set; } + public int ChildId { get; set; } + public int SortOrder { get; set; } + public virtual ParentX Parent { get; set; } + public virtual ChildX Child { get; set; } + + // Bad implementation of Equals to test for regression + public override bool Equals(object obj) + { + if (obj == null) + { + return false; + } + + var other = (ParentChildX)obj; + + if (!Equals(ParentId, other.ParentId)) + { + return false; + } + + if (!Equals(ChildId, other.ChildId)) + { + return false; + } + + return true; + } + + // Bad implementation of GetHashCode to test for regression + public override int GetHashCode() + { + var hashCode = 13; + hashCode = (hashCode * 7) + ParentId.GetHashCode(); + hashCode = (hashCode * 7) + ChildId.GetHashCode(); + return hashCode; + } + } + + protected class ChildX + { + public int Id { get; set; } + public string Name { get; set; } + public virtual IList ParentChildren { get; set; } = new List(); + } + + protected class BadHashDay : DbContext + { + private readonly string _databaseName; + + public BadHashDay(string databaseName) + { + _databaseName = databaseName; + } + + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder.UseInMemoryDatabase(_databaseName); + + protected internal override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity() + .HasMany(x => x.ParentChildren) + .WithOne(op => op.Parent) + .IsRequired(); + + modelBuilder.Entity() + .HasMany(x => x.ParentChildren) + .WithOne(op => op.Child) + .IsRequired(); + + modelBuilder.Entity().HasKey( + x => new + { + x.ParentId, x.ChildId + }); + } + } + + [ConditionalFact] + public void Detached_entity_is_not_replaced_by_tracked_entity() { var detachedProduct = new ProductX { Description = "Heavy Engine XT3" }; From 9041838f8c6c1bd32bf00ae6f31a6220db8556f5 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 30 Dec 2019 16:01:03 -0800 Subject: [PATCH 3/5] Prevent temporary value being left over when FK is later set to null Fixes #19137 --- .../Internal/InternalEntityEntry.cs | 6 +- .../StoreGeneratedTestBase.cs | 125 ++++++++++++++++++ 2 files changed, 128 insertions(+), 3 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs index 9b581c6898d..2412ca0c769 100644 --- a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs +++ b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs @@ -1150,11 +1150,11 @@ private void SetProperty( WritePropertyValue(propertyBase, value, isMaterialization); if (currentValueType != CurrentValueType.Normal - && !_temporaryValues.IsEmpty - && equals(value, asProperty.ClrType.GetDefaultValue())) + && !_temporaryValues.IsEmpty) { + var defaultValue = asProperty.ClrType.GetDefaultValue(); var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex(); - _temporaryValues.SetValue(asProperty, value, storeGeneratedIndex); + _temporaryValues.SetValue(asProperty, defaultValue, storeGeneratedIndex); } } else diff --git a/test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs b/test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs index af3dc785b38..ed4686c0152 100644 --- a/test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs +++ b/test/EFCore.Specification.Tests/StoreGeneratedTestBase.cs @@ -510,6 +510,129 @@ public virtual void Identity_property_on_Added_entity_with_temporary_value_gets_ context => Assert.Equal("Banana Joe", context.Set().Single(e => e.Id == id).Identity)); } + [ConditionalFact] // Issue #19137 + public void Clearing_optional_FK_does_not_leave_temporary_value() + { + ExecuteWithStrategyInTransaction( + context => + { + var product = new OptionalProduct(); + context.Add(product); + + var productEntry = context.Entry(product); + Assert.Equal(EntityState.Added, productEntry.State); + + Assert.Equal(0, product.Id); + Assert.True(productEntry.Property(e => e.Id).CurrentValue < 0); + Assert.True(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Null(product.CategoryId); + Assert.Null(productEntry.Property(e => e.CategoryId).CurrentValue); + Assert.False(productEntry.Property(e => e.CategoryId).IsTemporary); + + context.SaveChanges(); + + productEntry = context.Entry(product); + Assert.Equal(EntityState.Unchanged, productEntry.State); + + Assert.Equal(1, product.Id); + Assert.Equal(1, productEntry.Property(e => e.Id).CurrentValue); + Assert.False(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Null(product.CategoryId); + Assert.Null(productEntry.Property(e => e.CategoryId).CurrentValue); + Assert.False(productEntry.Property(e => e.CategoryId).IsTemporary); + + var category = new OptionalCategory(); + product.Category = category; + + productEntry = context.Entry(product); + Assert.Equal(EntityState.Modified, productEntry.State); + + Assert.Equal(1, product.Id); + Assert.Equal(1, productEntry.Property(e => e.Id).CurrentValue); + Assert.False(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Null(product.CategoryId); + Assert.True(productEntry.Property(e => e.CategoryId).CurrentValue < 0); + Assert.True(productEntry.Property(e => e.CategoryId).IsTemporary); + + var categoryEntry = context.Entry(category); + Assert.Equal(EntityState.Added, categoryEntry.State); + Assert.Equal(0, category.Id); + Assert.True(categoryEntry.Property(e => e.Id).CurrentValue < 0); + Assert.True(categoryEntry.Property(e => e.Id).IsTemporary); + + context.SaveChanges(); + + productEntry = context.Entry(product); + Assert.Equal(EntityState.Unchanged, productEntry.State); + + Assert.Equal(1, product.Id); + Assert.Equal(1, productEntry.Property(e => e.Id).CurrentValue); + Assert.False(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Equal(1, product.CategoryId); + Assert.Equal(1, productEntry.Property(e => e.CategoryId).CurrentValue); + Assert.False(productEntry.Property(e => e.CategoryId).IsTemporary); + + categoryEntry = context.Entry(category); + Assert.Equal(EntityState.Unchanged, categoryEntry.State); + Assert.Equal(1, category.Id); + Assert.Equal(1, categoryEntry.Property(e => e.Id).CurrentValue); + Assert.False(categoryEntry.Property(e => e.Id).IsTemporary); + + product.Category = null; + + productEntry = context.Entry(product); + Assert.Equal(EntityState.Modified, productEntry.State); + + Assert.Equal(1, product.Id); + Assert.Equal(1, productEntry.Property(e => e.Id).CurrentValue); + Assert.False(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Null(product.CategoryId); + Assert.Null(productEntry.Property(e => e.CategoryId).CurrentValue); + Assert.False(productEntry.Property(e => e.CategoryId).IsTemporary); + + categoryEntry = context.Entry(category); + Assert.Equal(EntityState.Unchanged, categoryEntry.State); + Assert.Equal(1, category.Id); + Assert.Equal(1, categoryEntry.Property(e => e.Id).CurrentValue); + + context.SaveChanges(); + + productEntry = context.Entry(product); + Assert.Equal(EntityState.Unchanged, productEntry.State); + + Assert.Equal(1, product.Id); + Assert.Null(product.CategoryId); + Assert.False(productEntry.Property(e => e.Id).IsTemporary); + + Assert.Equal(1, productEntry.Property(e => e.Id).CurrentValue); + Assert.Null(productEntry.Property(e => e.CategoryId).CurrentValue); + Assert.False(productEntry.Property(e => e.CategoryId).IsTemporary); + + categoryEntry = context.Entry(category); + Assert.Equal(EntityState.Unchanged, categoryEntry.State); + Assert.Equal(1, category.Id); + Assert.Equal(1, categoryEntry.Property(e => e.Id).CurrentValue); + Assert.False(categoryEntry.Property(e => e.Id).IsTemporary); + }); + } + + protected class OptionalProduct + { + public int Id { get; set; } + public int? CategoryId { get; set; } + public OptionalCategory Category { get; set; } + } + + protected class OptionalCategory + { + public int Id { get; set; } + } + [ConditionalFact] public virtual void Identity_property_on_Added_entity_with_temporary_value_gets_value_from_store_even_if_same() { @@ -1535,6 +1658,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con b.Property(e => e.NullableAsNonNullable).HasField("_nullableAsNonNullable").ValueGeneratedOnAddOrUpdate(); b.Property(e => e.NonNullableAsNullable).HasField("_nonNullableAsNullable").ValueGeneratedOnAddOrUpdate(); }); + + modelBuilder.Entity(); } } } From 3e49bddbaf7b6549b94423dce4080bfe7c9a3366 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 30 Dec 2019 13:39:32 -0800 Subject: [PATCH 4/5] Don't set FK properties to null when detaching principal Fixes #19203 --- .../ChangeTracking/Internal/StateManager.cs | 2 +- .../ChangeTracking/ChangeTrackerTest.cs | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/EFCore/ChangeTracking/Internal/StateManager.cs b/src/EFCore/ChangeTracking/Internal/StateManager.cs index 6e849ee6a51..bd02465d7b8 100644 --- a/src/EFCore/ChangeTracking/Internal/StateManager.cs +++ b/src/EFCore/ChangeTracking/Internal/StateManager.cs @@ -999,7 +999,7 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer CascadeDelete(dependent, force); } - else + else if (!principalIsDetached) { foreach (var dependentProperty in fk.Properties) { diff --git a/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs b/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs index 33cee375b6a..f18cb3e9a19 100644 --- a/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs +++ b/test/EFCore.Tests/ChangeTracking/ChangeTrackerTest.cs @@ -2357,6 +2357,100 @@ void TrackGraph() } } + [ConditionalTheory] // Issue #19203 + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void Dependent_FKs_are_not_nulled_when_principal_is_detached(bool delayCascade, bool trackNewDependents) + { + using var context = new EarlyLearningCenter(); + + var category = new OptionalCategory + { + Id = 1, + Products = new List + { + new OptionalProduct { Id = 1 }, + new OptionalProduct { Id = 2 }, + new OptionalProduct { Id = 3 } + } + }; + + context.Attach(category); + + var categoryEntry = context.Entry(category); + var product0Entry = context.Entry(category.Products[0]); + var product1Entry = context.Entry(category.Products[1]); + var product2Entry = context.Entry(category.Products[2]); + + Assert.Equal(EntityState.Unchanged, categoryEntry.State); + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + + if (delayCascade) + { + context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.OnSaveChanges; + } + + context.Entry(category).State = EntityState.Detached; + + Assert.Equal(EntityState.Detached, categoryEntry.State); + + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + + var newCategory = new OptionalCategory { Id = 1, }; + + if (trackNewDependents) + { + newCategory.Products = new List + { + new OptionalProduct { Id = 1 }, + new OptionalProduct { Id = 2 }, + new OptionalProduct { Id = 3 } + }; + } + + if (trackNewDependents) + { + Assert.Equal( + CoreStrings.IdentityConflict(nameof(OptionalProduct), "{'Id'}"), + Assert.Throws(() => context.Attach(newCategory)).Message); + } + else + { + context.Update(newCategory); + + Assert.Equal(4, context.ChangeTracker.Entries().Count()); + + categoryEntry = context.Entry(newCategory); + product0Entry = context.Entry(newCategory.Products[0]); + product1Entry = context.Entry(newCategory.Products[1]); + product2Entry = context.Entry(newCategory.Products[2]); + + Assert.Equal(EntityState.Modified, categoryEntry.State); + + Assert.Equal(EntityState.Unchanged, product0Entry.State); + Assert.Equal(EntityState.Unchanged, product1Entry.State); + Assert.Equal(EntityState.Unchanged, product2Entry.State); + + Assert.Same(newCategory.Products[0], category.Products[0]); + Assert.Same(newCategory.Products[1], category.Products[1]); + Assert.Same(newCategory.Products[2], category.Products[2]); + + Assert.Same(newCategory, newCategory.Products[0].Category); + Assert.Same(newCategory, newCategory.Products[1].Category); + Assert.Same(newCategory, newCategory.Products[2].Category); + + Assert.Equal(newCategory.Id, product0Entry.Property("CategoryId").CurrentValue); + Assert.Equal(newCategory.Id, product1Entry.Property("CategoryId").CurrentValue); + Assert.Equal(newCategory.Id, product2Entry.Property("CategoryId").CurrentValue); + } + } + [ConditionalTheory] // Issue #16546 [InlineData(false)] [InlineData(true)] @@ -3305,6 +3399,21 @@ private class Product public List OrderDetails { get; set; } } + private class OptionalCategory + { + public int Id { get; set; } + + public List Products { get; set; } + } + + private class OptionalProduct + { + public int Id { get; set; } + + public int? CategoryId { get; set; } + public OptionalCategory Category { get; set; } + } + private class SpecialProduct : Product { } @@ -3475,6 +3584,8 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder) b.HasOne(e => e.Order).WithMany(e => e.OrderDetails).HasForeignKey(e => e.OrderId); b.HasOne(e => e.Product).WithMany(e => e.OrderDetails).HasForeignKey(e => e.ProductId); }); + + modelBuilder.Entity(); } protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) From 859588d46d8d5d6ed83def08c951be772d18371a Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Thu, 13 Feb 2020 09:30:58 -0800 Subject: [PATCH 5/5] [release/3.1] Cherry-picking commits for for closely related detach/delete issues We made tweeks to the cascade delete behavior in 3.0 and also changed the default timing for when cascades happen. This change introduced several bugs which all result in deletion or severing of relationships instead of detaching them. This was then hit by more people due to the timing change. Issues: #19203 #19137 #18982 #16546 --- .../Internal/InternalEntityEntry.cs | 26 ++- .../ChangeTracking/Internal/StateManager.cs | 14 +- .../PropertyValuesTestBase.cs | 74 +++---- .../ChangeTracking/Internal/FixupTest.cs | 203 ------------------ .../ChangeTracking/Internal/OwnedFixupTest.cs | 104 ++++----- 5 files changed, 113 insertions(+), 308 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs index 2412ca0c769..d12bfca101e 100644 --- a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs +++ b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs @@ -1149,12 +1149,28 @@ private void SetProperty( { WritePropertyValue(propertyBase, value, isMaterialization); - if (currentValueType != CurrentValueType.Normal - && !_temporaryValues.IsEmpty) + var useNewBehavior + = !AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue19137", out var isEnabled) || !isEnabled; + + if (useNewBehavior) { - var defaultValue = asProperty.ClrType.GetDefaultValue(); - var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex(); - _temporaryValues.SetValue(asProperty, defaultValue, storeGeneratedIndex); + if (currentValueType != CurrentValueType.Normal + && !_temporaryValues.IsEmpty) + { + var defaultValue = asProperty.ClrType.GetDefaultValue(); + var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex(); + _temporaryValues.SetValue(asProperty, defaultValue, storeGeneratedIndex); + } + } + else + { + if (currentValueType != CurrentValueType.Normal + && !_temporaryValues.IsEmpty + && equals(value, asProperty.ClrType.GetDefaultValue())) + { + var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex(); + _temporaryValues.SetValue(asProperty, value, storeGeneratedIndex); + } } } else diff --git a/src/EFCore/ChangeTracking/Internal/StateManager.cs b/src/EFCore/ChangeTracking/Internal/StateManager.cs index bd02465d7b8..f5e143446d9 100644 --- a/src/EFCore/ChangeTracking/Internal/StateManager.cs +++ b/src/EFCore/ChangeTracking/Internal/StateManager.cs @@ -960,6 +960,7 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer { var doCascadeDelete = force || CascadeDeleteTiming != CascadeTiming.Never; var principalIsDetached = entry.EntityState == EntityState.Detached; + var useNewBehavior = !AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue18982", out var isEnabled) || !isEnabled; foreignKeys ??= entry.EntityType.GetReferencingForeignKeys(); foreach (var fk in foreignKeys) @@ -981,10 +982,14 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer || fk.DeleteBehavior == DeleteBehavior.ClientCascade) && doCascadeDelete) { - var cascadeState = principalIsDetached - || dependent.EntityState == EntityState.Added + var cascadeState = useNewBehavior + ? (principalIsDetached + || dependent.EntityState == EntityState.Added + ? EntityState.Detached + : EntityState.Deleted) + : (dependent.EntityState == EntityState.Added ? EntityState.Detached - : EntityState.Deleted; + : EntityState.Deleted); if (SensitiveLoggingEnabled) { @@ -999,7 +1004,8 @@ public virtual void CascadeDelete(InternalEntityEntry entry, bool force, IEnumer CascadeDelete(dependent, force); } - else if (!principalIsDetached) + else if (!useNewBehavior + || !principalIsDetached) { foreach (var dependentProperty in fk.Properties) { diff --git a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs index 2c51c06bab8..c719d3e6af3 100644 --- a/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs +++ b/test/EFCore.Specification.Tests/PropertyValuesTestBase.cs @@ -1262,53 +1262,53 @@ public async Task Values_can_be_reloaded_from_database_for_entity_in_any_state(E [InlineData(EntityState.Detached, false)] public async Task Reload_when_entity_deleted_in_store_can_happen_for_any_state(EntityState state, bool async) { - 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); + using var context = CreateContext(); - building.Offices.Add(office); - building.PrincipalMailRoom = mailRoom; - office.Building = building; - mailRoom.Building = building; + var office = new Office { Number = "35" }; + var mailRoom = new MailRoom { id = 36 }; + var building = Building.Create(Guid.NewGuid(), "Bag End", 77); - var entry = context.Entry(building); + building.Offices.Add(office); + building.PrincipalMailRoom = mailRoom; + office.Building = building; + mailRoom.Building = building; - context.Attach(building); - entry.State = state; + var entry = context.Entry(building); - if (async) - { - await entry.ReloadAsync(); - } - else - { - entry.Reload(); - } + context.Attach(building); + entry.State = state; - Assert.Equal("Bag End", entry.Property(e => e.Name).OriginalValue); - Assert.Equal("Bag End", entry.Property(e => e.Name).CurrentValue); - Assert.Equal("Bag End", building.Name); - - 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); + if (async) + { + await entry.ReloadAsync(); + } + else + { + entry.Reload(); + } - Assert.Equal(EntityState.Detached, context.Entry(office.Building).State); - Assert.Same(building, office.Building); - } + Assert.Equal("Bag End", entry.Property(e => e.Name).OriginalValue); + Assert.Equal("Bag End", entry.Property(e => e.Name).CurrentValue); + Assert.Equal("Bag End", building.Name); + 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.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); + + Assert.Equal(EntityState.Detached, context.Entry(office.Building).State); + Assert.Same(building, office.Building); + } + + Assert.Same(mailRoom, building.PrincipalMailRoom); + Assert.Contains(office, building.Offices); } [ConditionalFact] diff --git a/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs index fe6407f136c..807e3738d86 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs @@ -2888,209 +2888,6 @@ public void Detached_entity_is_not_replaced_by_tracked_entity() } } - public class ContainerX - { - public int Id { get; set; } - public string Name { get; set; } - public List Rooms { get; set; } = new List(); - } - - public class ContainerRoomX - { - public int Id { get; set; } - public int Number { get; set; } - public int ContainerId { get; set; } - public ContainerX Container { get; set; } - public int? ProductId { get; set; } - public ProductX Product { get; set; } - } - - public class ProductX - { - public int Id { get; set; } - public string Description { get; set; } - public List Rooms { get; set; } = new List(); - } - - protected class EscapeRoom : DbContext - { - private readonly string _databaseName; - - public EscapeRoom(string databaseName) - { - _databaseName = databaseName; - } - - protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) - => optionsBuilder.UseInMemoryDatabase(_databaseName); - - protected internal override void OnModelCreating(ModelBuilder modelBuilder) - { - modelBuilder.Entity() - .HasOne(room => room.Product) - .WithMany(product => product.Rooms) - .HasForeignKey(room => room.ProductId) - .IsRequired(false) - .OnDelete(DeleteBehavior.Cascade); - } - } - - [ConditionalFact] - public void Replaced_duplicate_entities_are_used_even_with_bad_hash() - { - using (var context = new BadHashDay("BadHashDay")) - { - context.AddRange( - new ParentX { Id = 101, Name = "Parent1" }, - new ChildX { Id = 201, Name = "Child1" }, - new ParentChildX - { - ParentId = 101, - ChildId = 201, - SortOrder = 1 - }); - - context.SaveChanges(); - } - - using (var context = new BadHashDay("BadHashDay")) - { - var parent = context.Set().Single(x => x.Id == 101); - var join = context.Set().Single(); - - Assert.Equal(2, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); - Assert.Equal(EntityState.Unchanged, context.Entry(join).State); - - parent.ParentChildren.Clear(); - - var newJoin = new ParentChildX - { - ParentId = 101, - ChildId = 201, - SortOrder = 1 - }; - - parent.ParentChildren = new List { newJoin }; - - Assert.Equal(3, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); - Assert.Equal(EntityState.Deleted, context.Entry(join).State); - Assert.Equal(EntityState.Added, context.Entry(newJoin).State); - - context.SaveChanges(); - - Assert.Equal(2, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Unchanged, context.Entry(parent).State); - Assert.Equal(EntityState.Detached, context.Entry(join).State); - Assert.Equal(EntityState.Unchanged, context.Entry(newJoin).State); - } - } - - protected class ParentX - { - public int Id { get; set; } - public string Name { get; set; } - public virtual IList ParentChildren { get; set; } = new List(); - } - - protected class ParentChildX - { - public int ParentId { get; set; } - public int ChildId { get; set; } - public int SortOrder { get; set; } - public virtual ParentX Parent { get; set; } - public virtual ChildX Child { get; set; } - - // Bad implementation of Equals to test for regression - public override bool Equals(object obj) - { - if (obj == null) - { - return false; - } - - var other = (ParentChildX)obj; - - if (!Equals(ParentId, other.ParentId)) - { - return false; - } - - if (!Equals(ChildId, other.ChildId)) - { - return false; - } - - return true; - } - - // Bad implementation of GetHashCode to test for regression - public override int GetHashCode() - { - var hashCode = 13; - hashCode = (hashCode * 7) + ParentId.GetHashCode(); - hashCode = (hashCode * 7) + ChildId.GetHashCode(); - return hashCode; - } - } - - protected class ChildX - { - public int Id { get; set; } - public string Name { get; set; } - public virtual IList ParentChildren { get; set; } = new List(); - } - - protected class BadHashDay : DbContext - { - private readonly string _databaseName; - - public BadHashDay(string databaseName) - { - _databaseName = databaseName; - } - - protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) - => optionsBuilder.UseInMemoryDatabase(_databaseName); - - protected internal override void OnModelCreating(ModelBuilder modelBuilder) - { - modelBuilder.Entity() - .HasMany(x => x.ParentChildren) - .WithOne(op => op.Parent) - .IsRequired(); - - modelBuilder.Entity() - .HasMany(x => x.ParentChildren) - .WithOne(op => op.Child) - .IsRequired(); - - modelBuilder.Entity().HasKey( - x => new { x.ParentId, x.ChildId }); - } - } - - [ConditionalFact] - public void Detached_entity_is_not_replaced_by_tracked_entity() - { - using (var context = new BadBeeContext(nameof(BadBeeContext))) - { - var b1 = new EntityB { EntityBId = 1 }; - context.BEntities.Attach(b1); - - var b2 = new EntityB { EntityBId = 1 }; - - var a = new EntityA { EntityAId = 1, EntityB = b2 }; - - Assert.Equal( - CoreStrings.IdentityConflict( - nameof(EntityB), - $"{{'{nameof(EntityB.EntityBId)}'}}"), - Assert.Throws(() => context.Add(a)).Message); - } - } - private class EntityB { public int EntityBId { get; set; } diff --git a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs index e253b9cf49c..3da420405aa 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs @@ -3400,78 +3400,64 @@ public void Parent_and_identity_swapped_bidirectional_collection(EntityState ent [InlineData(true)] public void Fixup_works_when_changing_state_from_Detached_to_Modified(bool detachDependent) { - using (var context = new OwnedModifiedContext(Guid.NewGuid().ToString())) - { - var details = new ProductDetails { Color = "C1", Size = "S1" }; + using var context = new OwnedModifiedContext(Guid.NewGuid().ToString()); - var product = new Product { Name = "Product1", Details = details }; + var details = new ProductDetails { Color = "C1", Size = "S1" }; + var product = new Product { Name = "Product1", Details = details }; - context.Add(product); - context.SaveChanges(); + context.Add(product); - Assert.Equal(2, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Unchanged, context.Entry(product).State); - Assert.Equal(EntityState.Unchanged, context.Entry(details).State); + Assert.True(context.ChangeTracker.HasChanges()); - context.Entry(product).State = EntityState.Detached; - if (detachDependent) - { - context.Entry(details).State = EntityState.Detached; - } + context.SaveChanges(); - if (detachDependent) - { - Assert.Empty(context.ChangeTracker.Entries()); - } - else - { - Assert.Single(context.ChangeTracker.Entries()); - Assert.Equal(EntityState.Deleted, context.Entry(details).State); - } + Assert.False(context.ChangeTracker.HasChanges()); - var newDetails = new ProductDetails { Color = "C2", Size = "S2" }; + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(product).State); + Assert.Equal(EntityState.Unchanged, context.Entry(details).State); - var newProduct = new Product - { - Id = product.Id, - Name = "Product1NewName", - Details = newDetails - }; + context.Entry(product).State = EntityState.Detached; + if (detachDependent) + { + context.Entry(details).State = EntityState.Detached; + } - context.Update(newProduct); + Assert.False(context.ChangeTracker.HasChanges()); - if (detachDependent) - { - Assert.Equal(2, context.ChangeTracker.Entries().Count()); - } - else - { - Assert.Equal(3, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Deleted, context.Entry(details).State); - } + Assert.Empty(context.ChangeTracker.Entries()); + Assert.Equal(EntityState.Detached, context.Entry(details).State); - Assert.Equal(EntityState.Modified, context.Entry(newProduct).State); - Assert.Equal(EntityState.Modified, context.Entry(newDetails).State); + var newDetails = new ProductDetails { Color = "C2", Size = "S2" }; - Assert.Same(details, product.Details); - Assert.Equal("C1", product.Details.Color); - Assert.Same(newDetails, newProduct.Details); - Assert.Equal("C2", newProduct.Details.Color); + var newProduct = new Product + { + Id = product.Id, + Name = "Product1NewName", + Details = newDetails + }; - if (detachDependent) - { - context.SaveChanges(); + context.Update(newProduct); - Assert.Equal(2, context.ChangeTracker.Entries().Count()); - Assert.Equal(EntityState.Unchanged, context.Entry(newProduct).State); - Assert.Equal(EntityState.Unchanged, context.Entry(newDetails).State); - } - else - { - // Because attempting to update an entity after it has been deleted - Assert.Throws(() => context.SaveChanges()); - } - } + Assert.True(context.ChangeTracker.HasChanges()); + + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + + Assert.Equal(EntityState.Modified, context.Entry(newProduct).State); + Assert.Equal(EntityState.Modified, context.Entry(newDetails).State); + + Assert.Same(details, product.Details); + Assert.Equal("C1", product.Details.Color); + Assert.Same(newDetails, newProduct.Details); + Assert.Equal("C2", newProduct.Details.Color); + + context.SaveChanges(); + + Assert.False(context.ChangeTracker.HasChanges()); + + Assert.Equal(2, context.ChangeTracker.Entries().Count()); + Assert.Equal(EntityState.Unchanged, context.Entry(newProduct).State); + Assert.Equal(EntityState.Unchanged, context.Entry(newDetails).State); } private class Product