Where to put SaveChanges in unit of work?

Question

For the sake of discussion, you can assume I'm using Entity Framework and a mediator pattern implementation in .NET.

I'm going back and forth trying to decide how to structure some of my data access in .NET. This confusion stems from the fact that I'm trying to follow the general advice of keeping my domain models and logic clear of any infrastructure concerns (any form of DbContext).

What this plays out as is that I fetch aggregates at my control layer and then pass them into my mediator dispatches which operate on the graph(s) and then towards the end of the controller, I call SaveChangesAsync to persist whatever changes have happened.

One example where I go back and forth is when doing a multi-delete. Imagine a controller that accepts a list of Guid IDs. Do I:

• Resolve all instances in the controller and then dispatch to my mediator a delete command and have the delete command depend on my DbContext and call .Remove itself?
• Resolve all instances in the controller and then dispatch to my mediator an arbitrary delete command, but perform the dbContext.Remove in the controller immediately after?
• Pass all the IDs via the mediator to be fetched by the handler? (this seems outright wrong)

I'm leaning towards the second option, but I hesitate because there are times when I might need to perform database access during a complex business logic operation. Particularly when dependent records are involved.

Is there any hard-and-fast rule for these kinds of ambiguous scenarios?

Answer 1

Committing and transactions

Here's my take on db contexts and when to commit them in a web context. That last part (web context) is important as it changes scope per request, which is not true for persistent services). Since you mentioned controllers, I assume you're working in a web context.

It all hinges one a clever little trick: when in a db transaction, SaveChanges gets ignored and changes can only be saved by committing the transaction.

This enables you to have a "default" way to save your data, but consumers can effectively override it when they choose to.

A simple example:

// DAL

public class PersonRepository
{
    private void SetPersonName(int personId, string name)
    {
        var person = db.People.Single(x => x.Id == personId);
        person.Name = name;

        db.SaveChanges();
    }
}

By default, calling this method will automatically save changes to the database. So in this case:

// BLL

public void ChangeNames(int[] peopleIds, string newName)
{
    foreach(var personId in peopleIds)
    {
        _personRepository.SetPersonName(personId, newName);
    }
}

There will be a db commit for ever person individually. But this might be a problem, e.g. if the nth ID in that list doesn't exist, it will break the code and leave you with a half-implemented list of name changes.

So here's an example of how we can override this commit behavior without changing the DAL.

// BLL

public void ChangeNames(int[] peopleIds, string newName)
{
    db.BeginTransaction();

    try
    {
        foreach(var personId in peopleIds)
        {
            _personRepository.SetPersonName(personId, newName);
        }

        db.CommitTransaction();
    }
    catch(Exception ex)
    {
        db.RollbackTransaction();
    }
}

Even though the DAL code still calls SaveChanges, the context ignores these calls because it's aware that a transaction has been registered.

So why do this?

It makes a lot of sense to commit your data in the DAL operation by default. DAL operations are expected to be self-contained.

However, for bulk operations, you tend to want to scope your commit on a higher level (i.e. the complete bulk operation), which means you have to develop a second DAL operation which either (a) iteratively does all DAL operations at once or (b) doesn't commit during the DAL operation.

This gets hairy quickly. Massive bulk import methods, duplicated code between the committing and non-commiting variants of your DAL operations, and further compounded issues if you want several distinct DAL operations to be processed in a single bulk.

BeginTransaction makes it possible for your committing code (which is easy to write and correct for all non-bulk operations) to act as if it's non-committing code, which means that performing a bulk operation no longer requires updating the DAL.

I believe this is an appropriate answer for the question you were asking.

Remarks

• I referenced db directly in the example BLL method. That's oversimplified for the sake of example. To not expose your DbContext to your BLL, wrap it in a UOW, and implement a BeginTransaction method in your UOW to allow the same behavior without the BLL having direct access to the DbContext.
• I mentioned a web context, because it's very easy to declare a scoped context that is specific to the current web request. In more persistent scenarios, e.g. a Windows service or local app, you need to use a bit more management to make sure you don't keep using the same context object indefinitely.

Your architecture

I believe this is an appropriate answer for the question you were asking.

Based on the options you presented, it seems like you're not really separating your layers using the usual Web > BLL > DAL architecture.

Resolve all instances in the controller and then dispatch to my mediator a delete command and have the delete command depend on my DbContext and call .Remove itself?

This sounds like Web > DAL, where your controller sends a mediator request directly to a DAL command object, without a business layer inbetween.

Resolve all instances in the controller and then dispatch to my mediator an arbitrary delete command, but perform the dbContext.Remove in the controller immediately after?

This sounds like you don't even have a DAL and your Web is handling EF directly, which is a really dangerous game to play and will pretty much unanimously be labeled bad practice by any developer worth their salt.

Pass all the IDs via the mediator to be fetched by the handler? (this seems outright wrong)

This sounds like Web > DAL, where your controller sends a mediator request directly to a DAL command object, without a business layer inbetween.

The main issue here is not about the bulk delete, but rather that the architecture that you're revealing while discussing the bulk delete is a much bigger issue that you're currently not seeing/tackling.
This shoots off on a tangent about good practice, so I suggest you really read up on clean coding, clean architecture, and how to properly structure a codebase. There is a lot to be said here that I can't cover in this answer.

To finish this answer, I'm going to assume that you use Web > DAL and you do it for good reason. That is to say that your codebase is intended as a "dumb" data storage with no real business logic between its web and database components. It's exceedingly rare, but these use cases do exist.

Looking back at my transaction example, it could be implemented as such:

• The DAL command handler deletes a person and calls SaveChanges
• The mediator connects WEB controllers and the DAL query/command objects
• For simple actions, the controller just fires the appropriate mediator request, which will automatically commit the changes.
• For compound (bulk) actions, the controller manually opens a transaction, fires all the mediator requests it wants, and then manually commits (or rolls back) the transaction.

This gives you most bang for your buck without requiring extra DAL work, in my opinion.