Skip to content

Commit

Permalink
[release/3.1] Cherry-picking commits for for closely related detach/d…
Browse files Browse the repository at this point in the history 
…elete 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
  • Loading branch information
@ajcvickers
ajcvickers committed Feb 15, 2020
1 parent 3e49bdd commit 859588d
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 308 deletions.
26 changes: 21 additions & 5 deletions src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 10 additions & 4 deletions src/EFCore/ChangeTracking/Internal/StateManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand Down
74 changes: 37 additions & 37 deletions test/EFCore.Specification.Tests/PropertyValuesTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
203 changes: 0 additions & 203 deletions test/EFCore.Tests/ChangeTracking/Internal/FixupTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ContainerRoomX> Rooms { get; set; } = new List<ContainerRoomX>();
}

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<ContainerRoomX> Rooms { get; set; } = new List<ContainerRoomX>();
}

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<ContainerRoomX>()
.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<ParentX>().Single(x => x.Id == 101);
var join = context.Set<ParentChildX>().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<ParentChildX> { 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<ParentChildX> ParentChildren { get; set; } = new List<ParentChildX>();
}

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<ParentChildX> ParentChildren { get; set; } = new List<ParentChildX>();
}

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<ParentX>()
.HasMany(x => x.ParentChildren)
.WithOne(op => op.Parent)
.IsRequired();

modelBuilder.Entity<ChildX>()
.HasMany(x => x.ParentChildren)
.WithOne(op => op.Child)
.IsRequired();

modelBuilder.Entity<ParentChildX>().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<InvalidOperationException>(() => context.Add(a)).Message);
}
}

private class EntityB
{
public int EntityBId { get; set; }
Expand Down
Loading

0 comments on commit 859588d

Please sign in to comment.