Make the SaveChangesAsync call optional in RepositoryBaseOfT

[enrij]

Recently i start liking (and using) the Unit Of Work pattern and I noticed that almost all methods to perform database changes in RepositoryBase<T> perform _dbContext.SaveChangesAsync() internally. IMHO this operation should, at least, be optional.

If you have to deal with domain events you want to fire before a change is persisted, you are mandated to rely on EF Core interceptors only, leaving your UnitOfWork almost useless.

Probably I'm wrong and I'm missing something, but as far as I understand, this is a pretty hard limitation.

[eldamir]

This exact case is what we're dealing with. Paraphrasing from @ardalis 's eShopOnWeb tutorial:

The reason why this pattern is OK, in my opinion, [...] is because this is a web application, and a web application should basically - on every request - be doing one operation at a time [...] otherwise use a different repository or service.

His point being that 99% of requests will do some reads and then one logical write for an aggregate. To me, that makes a lot of sense... Don't know about 99%, but certainly most of them.

My problem here is that for the cases that do more than one write, we will 100% certainly forget to do custom handling for the transactional part. We won't hear about it until there is an error in prod and users are stuck with corrupted data from a request failing half-way through.

So, for my concerns, this is not safe enough. I've implemented a UnitOfWork pattern in my code, and I've set it up in "filters" for my Razor Pages application, making sure that every request is wrapped in a UnitOfWork, so that either the request completes all of its writes completely or it rolls back the DB transaction and yields a 500 - INTERNAL SERVER ERROR.

This, however, does not work with Ardalis' repositories here, because they are hard-coded to save at each step.

I'm currently looking into inheriting and overriding his implementation to see if that would work. I still want all the other great parts - particularly the specification pattern - that comes with this library 😉

Ideal scenario is that this is a setting we can set to control the behaviour of the repositories

[eldamir]

Ooh, or at the very least make the fields on the RepositoryBase protected instead of private, so I can more easily subclass it 😉

[eldamir]

Right, I did this, which seems to do the trick:

public class RepositoryBase<T> : Ardalis.Specification.EntityFrameworkCore.RepositoryBase<T> where T : class
{
    private readonly DbContext _dbContext;
    private readonly ISpecificationEvaluator _specificationEvaluator;

    public RepositoryBase(DbContext dbContext) : base(dbContext)
    {
        _dbContext = dbContext;
    }

    public RepositoryBase(DbContext dbContext, ISpecificationEvaluator specificationEvaluator) : base(dbContext,
        specificationEvaluator)
    {
        _dbContext = dbContext;
        _specificationEvaluator = specificationEvaluator;
    }

    /// <inheritdoc/>
    public override async Task<T> AddAsync(T entity, CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().Add(entity);

        return entity;
    }

    /// <inheritdoc/>
    public override async Task<IEnumerable<T>> AddRangeAsync(IEnumerable<T> entities,
        CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().AddRange(entities);

        return entities;
    }

    /// <inheritdoc/>
    public override async Task UpdateAsync(T entity, CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().Update(entity);
    }

    /// <inheritdoc/>
    public override async Task UpdateRangeAsync(IEnumerable<T> entities, CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().UpdateRange(entities);
    }

    /// <inheritdoc/>
    public override async Task DeleteAsync(T entity, CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().Remove(entity);
    }

    /// <inheritdoc/>
    public override async Task DeleteRangeAsync(IEnumerable<T> entities, CancellationToken cancellationToken = default)
    {
        _dbContext.Set<T>().RemoveRange(entities);
    }

    /// <inheritdoc/>
    public override async Task DeleteRangeAsync(ISpecification<T> specification,
        CancellationToken cancellationToken = default)
    {
        var query = ApplySpecification(specification);
        _dbContext.Set<T>().RemoveRange(query);
    }
}

All I did was copy the implementation and remove the saves.

It isn't super clean, since there is now a _dbContext in my subclass and a _dbContext in the super class shadowing eachother, and the overridden methods will use one while the base class uses the other...

Shouldn't be any problem, since it is the same instance, but feels a little dumb to have two references in play... If the fields of the base class were protected instead, that would be helpful...

I suppose this could be done more elegantly in a PR to this repository instead, so that there would be a RepositoryBase, which I don't touch (except for protected fields instead of private), and then I'll add a subclass to it called RepositoryBaseWithoutAutoCommit or something....

[enrij]

I gave some thoughts on this topic since I wrote the initial post and i can see at least 3 possibile ways to achieve the goal:

1. You can create a derived class, like you did, with the opposite behavior (to save or not to save, this is the dilemma 💀)
2. You can implement an optional parameter on each method (probably the most flexible but for sure the worst looking IMHO 🤢)

public async Task<T> AddAsync(T entity, bool autoSave = true, CancellationToken cancellationToken = default)

3. You can inject some IOptions<SpecificationOptions> in the constructor (in which the behavior is defined) but pay the price to the need of some AddSpecifications(options => {...}) while registering services on application startup

I don't really like 100% any of the three, but I can't figure out something "cleaner"

[eldamir]

Given the freedom to edit the library, I would probably make it a setting on the repository, which would be provided in the constructor or library netted through configuration, so that any repository would have a ’shouldIAutoSave‘ property.

But I don't know if any of it is in the spirit of what the maintainers of this library is going for.

For the time being, I have a workaround that does the trick, and I can wait for this issue to get some more attention 😊

[ardalis]

I'm open to making the type use protected by default for all of the methods. That makes sense and would make this easier to implement. I also agree that the current implementation is probably too opinionated about its (lack of) UoW support, which is made even more confusing by the existence of a separate SaveChanges method. My medium-term plan is to stop supporting the repository implementation in this package and instead produce a separate Ardalis.Repository package that would be much slimmer and in line with what I actually use, personally. And if folks want to have variants of that, I would perhaps put them into sub-packages.

For example:

• Ardalis.Repository.UnitOfWork
• Ardalis.Repository.AutoSave
• Ardalis.Repository.Configurable (with parameterized methods indicating whether to save, perhaps)
• Ardalis.Repository.KitchenSink (the current version that has every method possible on it...)

Then you could explicitly choose a particular flavor worked best for your needs.

I'm not sure I'll get around to the sub variants any time soon but the top level Ardalis.Repository that would have my personal version is likely to happen in the next few months.

[ardalis]

If you like could someone open a separate issue for marking all the repo methods protected, and then do a quick PR if you want?

[eldamir]

Sure. Issue here #397.

I'll see if I can find some time to implement it. Shouldn't take long ✌️