Is there a better way to soft delete navigation properties in Entity Framework 6 when using Repository pattern?

Question

public void ReassignLineItems(InvoiceUpdateDto invoiceUpdate)
{
 Invoice invoiceInRepository = _unitOfWork.InvoiceRepository
 .FirstOrDefault(invoice => invoice.Id == invoiceUpdate.Id);
 invoiceInRepository.LineItems = invoiceUpdate.LineItems;
 _unitOfWork.SaveChanges();
}

After retrieving Invoice from the repository, and then detaching LineItem from a navigation collection property by either removing it from the collection, or by assigning a new collection to Invoice, Entity Framework tries to set the foreign key value to NULL. Instead, I want the foreign key to be intact, but the IsDeleted property of LineItem to change. I don't want to manually set the IsDeleted property to false, as that does not seem like a business logic responsibility.

In order to achieve this, I had to do the following.

public new void SaveChanges()
{
 var entries = ChangeTracker.Entries()
 .Where(e => e.State == EntityState.Added ||
 e.State == EntityState.Modified ||
 e.State == EntityState.Deleted);
 foreach (var entry in entries)
 {
 if (entry.Entity is LineItem lineItem)
 {
 if (entry.State == EntityState.Modified
 && lineItem.Order == null)
 {
 lineItem.IsDeleted = true;
 entry.Property(nameof(LineItem.Order)).IsModified = false;
 }
 }
 }
 base.SaveChanges()
}

Make a custom SaveChanges method in the DbContext class and have it find the detached LineItem from the ChangeTracker, and set the IsDeleted to false IF the foreign key property was null, and have the change to the foreign key property be ignored. Finally, call base.SaveChanges.

This works but it means that for each type that is removed from a navigation collection property, I have to write code inside the custom SaveChanges method for that type. This could make the SaveChanges method grow too large and cause performance issues because there would be a lot of if statements and type checks.

Is there a better way to achieve this?

• Ewan: Urg EF is a pain in the arse with this kind of thing. Rather than override the change logic, I would retrieve the invoice, remove matching line items, set all the deleted flags, add the updated line items.. save. (Also loose the DTO) I'm not writing this as answer because i would have to test it and EF is not the best when it comes to updating collections in a performant manner

Answer 1

I think there is a misconception here. The repository pattern is a persistence pattern. What you are describing is business logic, not persistence logic.

There is a business process you need to model, and the first step in doing this is to create a proper domain model, not an anemic domain model like you currently have (related search: anemic vs rich domain model).

The ReassignLineItems method belongs on one of the entities where they set the Order and IsDeleted properties instead of stuffing this logic in a repository. You shouldn't need to test for an entity's state in the ORM. If properly configured, your ORM should pick up the changes automatically and issue the proper UPDATE statements to the database.

This new method belongs on one of the existing entities, but this process might have a representation in both entities. A concrete recommendation relies on a myriad details which you don't have included in your question. Instead, take this as an opportunity to reconsider the design of this part of your system. You have found non-trivial business logic, and it needs a place to live. You've also discovered that mixing business logic with persistence logic can be troublesome. The solution is separation of concerns and encapsulation; two fundamental tenets of object-oriented design that an anemic domain model flippantly and maliciously throws out the window.

---

If I remember correctly, Entity Framework 6 is an older version, and I don't remember if it can map private properties. If it can't, I would still recommend adding proper methods to your entities so this logic has a place to live even if properties have public setters and getters. Encapsulation should be enforced by code review to ensure nobody is bypassing the new methods. Not ideal, but it's still better than what you've got now.

An alternative that I've seen many times (though it isn't my preference) is to put this logic in a service class.

This is a decision that forces you to make trade-offs due to a constraint imposed on you by a tool. A tool which may not be possible or practical to upgrade.

• Ewan: I'm not sure i agree with you here. It seems to me that having a soft delete is persistence logic and OK to implement in the repo.
• Greg Burghardt: @Ewan: that depends on what "soft delete" means from a business perspective. Often times there is a real business need being met, which us developers tend to call "delete" because we haven't asked the domain experts what it really means. I'm considering posting a second answer or amending this one to include that.
• EMN: @GregBurghardt The business has a policy that prohibits deleting database records; instead, they perform soft deletes. The policy exists to mitigate risk, but it isn’t something that domain experts (other departments) consider. Would this be considered business logic?
• Greg Burghardt: @EMB - "The business has a policy" and "isn't something that domain experts ... consider" --- How are "business" and "domain experts" different here? Are there legal requirements? Audit requirements? My point is, "soft delete" is usually technical jargon for a real business requirement. And "reassign line items" seems like a business process. That would be business logic in my opinion. (1/2)
• Greg Burghardt: (2/2) Are you sure "delete" doesn't mean "invalidate"? Of course, this might not be useful for an existing system where you cannot make big changes. I'm always wary of "soft deletes" because someone isn't telling me something I should know. I think my point is to not let the word "delete" fool you. This isn't persistence logic simply because you use the word "delete." It's OK to reconsider your design in this case.