-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Orphans are deleted when overlapping composite alternate key is mutated #28961
Comments
@mrpmorris I would set the delete behavior to Here's an example: public static class Your
{
public static string ConnectionString = @"Data Source=(LocalDb)\MSSQLLocalDB;Database=Test";
}
public class Parent
{
public Guid Id { get; private set; } = Guid.NewGuid();
public virtual List<Child> Children { get; private set; } = new();
}
public class Child
{
public Guid Id { get; private set; } = Guid.NewGuid();
public Guid ParentId { get; set; }
public virtual Parent Parent { get; set; } = null!;
}
public class SomeDbContext : DbContext
{
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder.UseSqlServer(Your.ConnectionString);
public SomeDbContext()
{
ChangeTracker.CascadeDeleteTiming = CascadeTiming.Never;
}
public DbSet<Parent> Parents { get; set; } = null!;
private DbSet<Child> Children { get; set; } = null!;
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Child>()
.HasOne(x => x.Parent)
.WithMany(x => x.Children)
.OnDelete(DeleteBehavior.ClientCascade);
}
}
public class Program
{
public static void Main()
{
using (var context = new SomeDbContext())
{
context.Database.EnsureDeleted();
context.Database.EnsureCreated();
var parent = new Parent();
parent.Children.Add(new Child());
context.Parents.Add(parent);
context.SaveChanges();
}
using (var context = new SomeDbContext())
{
context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Never;
var parent = context.Parents.First();
context.Remove(parent);
try
{
context.SaveChanges();
}
catch (Exception e)
{
Console.WriteLine(e.Message);
}
}
using (var context = new SomeDbContext())
{
context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Never;
var parent = context.Parents.Include(x => x.Children).First();
context.Remove(parent);
try
{
context.SaveChanges();
}
catch (Exception e)
{
Console.WriteLine(e.Message);
}
}
using (var context = new SomeDbContext())
{
context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Never;
var parent = context.Parents.Include(x => x.Children).First();
parent.Children.RemoveAt(0);
context.SaveChanges();
}
}
} |
I have a UnitOfWork to abstract away the DbContext, so that setting would have to go into the constructor, and would affect everything. But obviously I want to control it per aggregate. It seems hacky, telling EF you want to ClientCascade deletes in order to tell it not to delete. Can you tell me why this is better than HasAlternateKey? |
I need to investigate this some more to understand why the alternate key is causing the dependent to be deleted, but it seems to be a bug. |
Damn, it makes so much sense to me. |
@mrpmorris Can you explain your logic? |
The way I saw it was that the key is the real identity, so used for referencing. Typically I have a single column for a key, because I don't like multi-part keys getting longer as I add more child tables.... So, saying I have an "Alternate Key" to me seems like I am saying "This isn't what you'd use for referential integrity, but it is another way of specifying the object's identity" - and it makes sense to me that if you destroy its identity then you want to destroy the object.
PurchaseOrderLine[Id] = for other tables to reference and have a FK in the DB. I just want to be able to do
But, at the same time, prevent this in the DB with a prohibited delete if the order has lines
And I'd like to be able to do it by convention when defining my model with ModelBuilder rather than having to use specific code at the point of consuming the model. |
Changing an alternate key value is supposed to throw. Once #4073 is implemented, it should cause the entity to be updated with the new key values. |
I think there needs to be a simple way of doing this. As I am doing DDD I only have a repo for my aggregate root, so the only way of indicating a child part should be deleted is removing it from a Maybe
Not my final suggestion, just a seedling of an idea :) |
Tracked by #10066. Make sure to vote for that issue. |
Thanks. To vote, do I just thumb it up? |
@ajcvickers That other report is nearly 5 years old. That suggests it's not going to happen, at least in the near future :( So my current implementation relies on a bug that will soon be fixed, and my alternatives are either to not update my version or implement something that will effect all client-deletes? |
@mrpmorris You can always override SaveChanges (or use and event/interceptor) and process the deletion of orphans there. This was the way it was done before any form of orphan deletion was available. |
I really hope the priority of that report goes up. Otherwise it's a lot of additional work to use EF for a DDD approach. |
Fixes #28961 In the issue, an alternate key was added over the the foreign key to make it effectively an identifying relationship. That is, changing the relationship changes the identity of the dependent. This resulted in the special handling for identifying relationships to kick in, which didn't respect the configured DeleteBehavior. This change fixes that.
…lternate key property Fixes #28961 Reverts #30213 for #32385 In #32385, an unconstrained alternate key was added to the model purely to make a non-identifying relationship artificially identifying. #30213 attempted to fix this by throwing that the key was being modified. However, this scenario is very similar to the case for a many-to-many join type, where the composite primary key is also not the end of any relationship, but forces the two many-to-one relationships to be identifying. I prepared a PR that would only throw if the key involved is alternate, but on reflection that doesn't seem like an appropriate distinction to make. So overall, I think we should just revert this change, which is what this PR does.
…lternate key property (#32492) Fixes #28961 Reverts #30213 for #32385 In #32385, an unconstrained alternate key was added to the model purely to make a non-identifying relationship artificially identifying. #30213 attempted to fix this by throwing that the key was being modified. However, this scenario is very similar to the case for a many-to-many join type, where the composite primary key is also not the end of any relationship, but forces the two many-to-one relationships to be identifying. I prepared a PR that would only throw if the key involved is alternate, but on reflection that doesn't seem like an appropriate distinction to make. So overall, I think we should just revert this change, which is what this PR does.
…lternate key property (#32492) Fixes #28961 Reverts #30213 for #32385 In #32385, an unconstrained alternate key was added to the model purely to make a non-identifying relationship artificially identifying. #30213 attempted to fix this by throwing that the key was being modified. However, this scenario is very similar to the case for a many-to-many join type, where the composite primary key is also not the end of any relationship, but forces the two many-to-one relationships to be identifying. I prepared a PR that would only throw if the key involved is alternate, but on reflection that doesn't seem like an appropriate distinction to make. So overall, I think we should just revert this change, which is what this PR does.
…nconstrained alternate key property (#32523) * Revert behavior to throw when attempting to modify an unconstrained alternate key property (#32492) Fixes #28961 Reverts #30213 for #32385 In #32385, an unconstrained alternate key was added to the model purely to make a non-identifying relationship artificially identifying. #30213 attempted to fix this by throwing that the key was being modified. However, this scenario is very similar to the case for a many-to-many join type, where the composite primary key is also not the end of any relationship, but forces the two many-to-one relationships to be identifying. I prepared a PR that would only throw if the key involved is alternate, but on reflection that doesn't seem like an appropriate distinction to make. So overall, I think we should just revert this change, which is what this PR does. * Quirk
Given the following simple
Parent
/Child
model, whereChild
is an aggregate part ofParent
(and cannot exist without it).I want to prohibit a
Parent
from being deleted if it hasChildren, and I also want any
Childthat is removed from
Parent.Children` to be automatically deleted on save.As per the documentation, this code throws an exception because
Child.Parent
cannot benull
.It seems that
modelBuilder.Entity<Child>().HasAlternateKey(x => new { x.Id, x.ParentId })
has the behaviour I expected, but @ajcvickers said "I certainly wouldn't do it that way" - https://twitter.com/ajcvickers/status/1565633141880102914What is the recommended practice to ensure the child is deleted instead?
The text was updated successfully, but these errors were encountered: