Skip to content
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

Add Unit Of Work Above The Repository With Specifications #327

Open
eissa4444 opened this issue Feb 21, 2022 · 28 comments
Open

Add Unit Of Work Above The Repository With Specifications #327

eissa4444 opened this issue Feb 21, 2022 · 28 comments

Comments

@eissa4444
Copy link

eissa4444 commented Feb 21, 2022

I'd like to add the unit of work pattern above the repository and specifications. Does this make sense or it will be a useless feature and
no need for it on domain driven design philosophy ?

@eissa4444 eissa4444 changed the title Add Unit Of Work Up On Repository With Specifications Add Unit Of Work Above The Repository With Specifications Feb 21, 2022
@ardalis
Copy link
Owner

ardalis commented Feb 21, 2022

It is needed sometimes. I can show an example, but I also don't want people to think that it is always needed, so that is why it's not included thus far in the relatively simple template.

@eissa4444
Copy link
Author

Thanks Steve got it.

@chadboettcher
Copy link

I'd love to see an example, Steve!

@ThomasVague
Copy link

Any progress on an example? I'm currently tring to implement your specfication pattern, but I'm stuck on how to rollback if I add multiple entities to the database, without explicitly removing them when an exception is thrown.

@ardalis
Copy link
Owner

ardalis commented May 6, 2022

One option is to use a transaction, and then rollback the transaction if an exception occurs. This would be similar to but not identical to a typical UoW pattern. For the Unit of Work pattern usually it just controls the call to SaveChanges in the DbContext.

@ardalis
Copy link
Owner

ardalis commented May 6, 2022

@ThomasVague The specification pattern is mainly about fetching, not saving, aggregates. How is the rollback issue you describe impacting your ability to use specifications?

@ThomasVague
Copy link

Yeah, I probably shouldn't have said specification pattern. I was referring to your library as a whole, with using the IRepository interface as a way of saving multiple entities. Since the update and create funtions are doing a call to SaveChangesAsync, and there is no transaction exposed, I'm not sure how to do a rollback. You said that "one option is to use a transaction", but I don't see how when the dbContext is not exposed?

@ardalis
Copy link
Owner

ardalis commented May 9, 2022

You can get the dbContext anywhere you need it with DI. So if you need to do a few operations you could have an interface like IUnitOfWork or ITransactionManager. It could expose methods BeginTransaction and CommitTransaction (and probably Rollback...). You could have an implementation: EfUnitOfWork which would request AppDbContext in its constructor and inside of the Begin/End transaction methods it would use the requested dbContext to start/commit a transaction.

Then your higher level code would be:

try {
_unitOfWork.BeginTransaction();
_someRepositoryOne.Create(entity);
_someRepositoryTwo.Create(otherEntity);
_unitOfWork.CommitTransaction();
}
catch
{
_unitOfWork.RollbackTransaction();
}

Because dbContext is scoped, you'll have the same instance of the DbContext in each repository and in the unit of work implementation. I'll still try and write up some actual code for this (this is all off the top of my head so might not compile or work 100%) but should get you started in the right direction.

@ThomasVague
Copy link

Great, thanks! I'll definitely look into that.

@ThomasVague
Copy link

ThomasVague commented May 13, 2022

It seemd to work nice! Here's my code if anyone's interested

public class UnitOfWork : IUnitOfWork
{
	private readonly ApplicationDbContext _context;
	private IDbContextTransaction? _transaction;

	public UnitOfWork(ApplicationDbContext context)
	{
		_context = context;
	}

	public void BeginTransaction()
	{
		if (_transaction != null)
		{
			return;
		}

		_transaction = _context.Database.BeginTransaction();
	}

	public Task<int> SaveChangesAsync()
	{
		return _context.SaveChangesAsync();
	}

	public void Commit()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Commit();
		_transaction.Dispose();
		_transaction = null;
	}

	public async Task SaveAndCommitAsync()
	{
		await SaveChangesAsync();
		Commit();
	}

	public void Rollback()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Rollback();
		_transaction.Dispose();
		_transaction = null;
	}

	public void Dispose()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Dispose();
		_transaction = null;
	}
}

public interface IUnitOfWork : IDisposable, ITransientService
{
	void BeginTransaction();
	void Commit();
	void Rollback();
	Task<int> SaveChangesAsync();
	Task SaveAndCommitAsync();
}

Any thoughts on IUnitOfWork should be transient or scoped?

@ardalis
Copy link
Owner

ardalis commented May 13, 2022

It should be scoped if there's any chance it will be used between multiple services.

@ardalis
Copy link
Owner

ardalis commented May 19, 2022

@ThomasVague are you using a convention in your service registration such that ITransientService, IScopedService results in the registration working properly (Transient, Scoped, etc.)? Curious if that's something you made custom or if you're using a NuGet package for that.

@ThomasVague
Copy link

ThomasVague commented May 20, 2022

@ardalis I actually borrowed the idea from FSH's https://github.com/fullstackhero/dotnet-webapi-boilerplate. So yeah, the interfaces inherits from a ITransientService or IScopedService respectively and gets registered by a function "AddServices". Here's the code:

	internal static IServiceCollection AddServices(this IServiceCollection services) =>
		services
			.AddServices(typeof(ITransientService), ServiceLifetime.Transient)
			.AddServices(typeof(IScopedService), ServiceLifetime.Scoped);

	internal static IServiceCollection AddServices(this IServiceCollection services, Type interfaceType, ServiceLifetime lifetime)
	{
		var interfaceTypes =
			AppDomain.CurrentDomain.GetAssemblies()
				.SelectMany(s => s.GetTypes())
				.Where(t => interfaceType.IsAssignableFrom(t)
							&& t.IsClass && !t.IsAbstract)
				.Select(t => new
				{
					Service = t.GetInterfaces().FirstOrDefault(),
					Implementation = t
				})
				.Where(t => t.Service is not null
							&& interfaceType.IsAssignableFrom(t.Service));

		foreach (var type in interfaceTypes)
		{
			services.AddService(type.Service!, type.Implementation, lifetime);
		}

		return services;
	}

	internal static IServiceCollection AddService(this IServiceCollection services, Type serviceType, Type implementationType, ServiceLifetime lifetime) =>
		lifetime switch
		{
			ServiceLifetime.Transient => services.AddTransient(serviceType, implementationType),
			ServiceLifetime.Scoped => services.AddScoped(serviceType, implementationType),
			ServiceLifetime.Singleton => services.AddSingleton(serviceType, implementationType),
			_ => throw new ArgumentException("Invalid lifeTime", nameof(lifetime))
		};

@ardalis
Copy link
Owner

ardalis commented May 20, 2022

Great, thanks. That's where I'd seen it previously, I think.

@Sarmadjavediqbal
Copy link

Sarmadjavediqbal commented May 3, 2023

I tried to implement unitOfWork pattern mentioned above
#327 (comment)

but I am getting following error,

{
"messages": [],
"source": "Microsoft.Data.SqlClient.SqlCommand+<>c",
"exception": "BeginExecuteReader requires the command to have a transaction when the connection assigned to the command is in a pending local transaction. The Transaction property of the command has not been initialized.",
"errorId": "6d4329bc-c24f-4852-ba88-35caf4b9ad6d",
"supportMessage": "Provide the ErrorId 6d4329bc-c24f-4852-ba88-35caf4b9ad6d to the support team for further analysis.",
"statusCode": 500
}

Can someone please correct my code:

Following is my code;

using Mapster;
using Microsoft.Data.SqlClient;
using API.Application.Common.Custom.IUnitOfWork;
using API.Domain.MyAPI.Entities;
using System.Data;
using System.Transactions;

namespace API.Application.MyAPI.Challan.Commands.Update;
public class UpdateChallanRequest : IRequest<ChallanDto>
{
    public int Id { get; set; }
    public decimal Property1 { get; set; }
    public DateTime Property2 { get; set; }
}

public class UpdateChallanRequestValidator : CustomValidator<UpdateChallanRequest>
{
    public UpdateChallanRequestValidator()
    {
    }
}

internal class UpdateChallanRequestHandler : IRequestHandler<UpdateChallanRequest, ChallanDto>
{
    private readonly IRepositoryWithEvents<TrnsChallanGeneration> _repos;
    private readonly IDapperRepository _repository;
    private readonly IUnitOfWork _unitOfWork;
    private IDbTransaction _transaction;
    private IStringLocalizer<(someClass1, someClass2)> _localizer;

    public UpdateChallanRequestHandler(IDapperRepository repository, IUnitOfWork unitOfWork, IStringLocalizer<(someClass1, someClass2)> localizer, IRepositoryWithEvents<someClass1> repos, IDbTransaction transaction)
    {
        _repository = repository;
        _unitOfWork = unitOfWork;
        _localizer = localizer;
        _repos = repos;
        _transaction = transaction;
    }

    public async Task<ChallanDto> Handle(UpdateChallanRequest request, CancellationToken cancellationToken)
    {
        _unitOfWork.BeginTransaction();
        var challan = _repos.GetByIdAsync(request.Id, cancellationToken);

        string query;
        string query1;

        var parameters = new
        {
            ID = request.Id,
            param1 = request.Property1,
            param2 = request.Property2
        };

        query = "BEGIN" +
                            "UPDATE dbo.someTable1" +
                            "SET someColumn1=@param1, Active=0, someColumn2=@param2, flagPaid=true" +
                            "WHERE dbo.someTable1.id = @ID;" +
                        "END";
        var result = await _repository.QueryAsync<someClass1>(query, parameters, cancellationToken: cancellationToken);

        await _unitOfWork.SaveChangesAsync();


        try
        {
            if(challan.Id == null)
            {
                throw new ArgumentNullException();
            }
            else if (request.Id != null && challan.Id != null)
            {
                query1 = "BEGIN" +
                            "INSERT INTO dbo.someTable2" +
                            "(someColumn1, someColumn2, someColumn3)" +
                            "VALUES (" +
                                "SELECT someColumn1 from dbo.someTable1 WHERE dbo.someTable1.id = @ID;," +
                                "SELECT someColumn2 from dbo.someTable1  WHERE dbo.someTable1.id = @ID;," +
                                "SELECT someColumn3 from dbo.someTable1  WHERE dbo.someTable1.id = @ID;," +
                        "END";
                await _repository.QueryAsync<someClass1>(query1, parameters, cancellationToken: cancellationToken);

                await _unitOfWork.SaveChangesAsync();

                _unitOfWork.Commit();
            }
        }
        catch(Exception ex)
        {
            _unitOfWork.Rollback();
            throw new ArgumentNullException("Challan Not Found.");
        }

        return result.Adapt<ChallanDto>();
    }
}

Can someone please correct my code.

@Sarmadjavediqbal
Copy link

I have figured it out. I was opening 2 connections with database where as System.Transaction allows only 1 or if you want to check and validate some value from database you must close the connection first before opening another connection with System.Transaction. This issue can be closed now.

@Tcioch
Copy link

Tcioch commented May 17, 2023

I have a similar question.
I coordinate several different domain services, which have their own repositories, in a handler.
I'm using RepositoryBase and there is only AddAsync, which saves the changes immediately, so I can't use DbContext to save the changes higher up - in the handler when none of the services throw an exception.
How could I do it differently?
Do I not use RepositoryBase?
Is it ok to coordinate the domain services in the layer above - in handler? If so, is it also ok to use the context in this layer to validate changes?

@Sarmadjavediqbal
Copy link

@Tcioch I do not understand your question. Could you please share some details with example code?

@Tcioch
Copy link

Tcioch commented May 18, 2023

@Sarmadjavediqbal

I'm currently facing a challenge related to coordinating multiple domain services that each have their own repositories in a handler. I am using the Ardalis.Specification pattern and making use of the RepositoryBase, which has the AddAsync method. However, this immediately commits the changes to the database, and I need a way to delay this action.
In particular, I want to be able to save changes in my DbContext at a higher level - in the handler - but only when none of the services throw an exception. This way, I can take full advantage of the Unit of Work pattern that Entity Framework provides.
Is there a way I can achieve this using RepositoryBase or do I need to not use it? Also, is it a proper approach to coordinate the domain services in the layer above - in the handler? And if that is the case, would it be appropriate to use the DbContext in this layer to commit the changes? I appreciate any guidance you can provide on this.

//Handler
public class MyHandler : IRequestHandler<MyCommand>
{
    private readonly IService1 _service1;
    private readonly IService2 _service2;
    private readonly IService3 _service3;
    // I would like to add DbContext here;

    public MyHandler(IService1 service1, IService2 service2, IService3 service3)
    {
        _service1 = service1;
        _service2 = service2;
        _service3 = service3;
    }

    public async Task Handle(MyCommand request, CancellationToken cancellationToken)
    {
          await _service1.Action(request.Data);
          await _service2.Action2(request.Data);
          await _service3.Action3(request. Data);

          // Intend to save all changes here, but the AddAsync in services commits the changes immediately
          await _dbContext.SaveChangesAsync();
    }
}

And here is my service

public class Service1 : IService1
{
    private readonly ICarRepository _repository;

    public Service1(ICarRepository repository)
    {
        _repository = repository;
    }

    public async Task Action(SomeData data)
    {
        var car = new Car { /* init car */ };
        await _repository.AddAsync(car); // this immediately commits the changes to the database
    }
}

And ICarRepository just gets IBaseRepository.

@Sarmadjavediqbal
Copy link

Sarmadjavediqbal commented May 18, 2023

@Tcioch

What you are trying to do is commit changes all at once like:

public async Task Handle(MyCommand request, CancellationToken cancellationToken)
    {
          await _service1.Action(request.Data);
          await _service2.Action2(request.Data);
          await _service3.Action3(request. Data);

          // Intend to save all changes here, but the AddAsync in services commits the changes immediately
          await _dbContext.SaveChangesAsync();
    }

What you should be doing is commit changes after each service is executed like this:

public async Task Handle(MyCommand request, CancellationToken cancellationToken)
    {
          try 
         {
                 if (condition)
                 {
                         await _service1.Action(request.Data);
                         await _dbContext.SaveChangesAsync();
                         _dbContext.Commit();

                         await _service2.Action2(request.Data);
                         await _dbContext.SaveChangesAsync();
                         _dbContext.Commit();

                         await _service3.Action3(request. Data);
                         await _dbContext.SaveChangesAsync();
                        _dbContext.Commit();
                 }
                 else
                 {
                        throw new NotFoundException();
                 }
         }
         catch (Exception ex)
         {
                 throw new NotFoundException();
         }

         return;
    }

@Sarmadjavediqbal
Copy link

Sarmadjavediqbal commented May 18, 2023

@Tcioch Also don't call DbContext directly into your class. It is not the best practice to use it like this. Add some Unit of Work type repository into your API. Best practice is to follow the SOLID principles.

@Tcioch
Copy link

Tcioch commented May 18, 2023

@Sarmadjavediqbal
I completely do not understand.
DbContext does not have a commit() method. Secondly SaveChangesAsync calls save to database, so why would I use commit() if it even existed?
And that's what I mean, I don't want to save the changes after the services, because if any service raised an exception, I don't want to have data inconsistency. Either all services will go through, or none.
And what do you mean by not using DbContext directly on the class? I don't see it.

@Tcioch
Copy link

Tcioch commented May 18, 2023

@Sarmadjavediqbal

This is my repository (example)

public interface ICarRepository : IRepositoryBase<Car>
{
}

public class CarRepository : RepositoryBase<Car>, ICarRepository
{
    public CarRepository(DbContext dbContext) : base(dbContext)
    {
    }
}

Where IRepositoryBase is from Ardalis.Specification

@Sarmadjavediqbal
Copy link

@Tcioch it seems that you are trying to saveChanges without using System.Transaction.

@Sarmadjavediqbal
Copy link

@Tcioch Use Unit Of Work pattern in your API. as mentioned above in #327 (comment)

by using System.Transaction you can save changes and commit them without any hassle. Also if it throws an exception the API will RollBack all the Transactions committed and you will have a consistency in your data as per your expectations.
I hope this answers your question.

public async Task Handle(MyCommand request, CancellationToken cancellationToken)
    {
          try 
         {
                 if (condition == true)
                 {
                         await _service1.Action(request.Data);
                         await _unitOfWork.SaveChangesAsync();
                         _unitOfWork.Commit();

                         await _service2.Action2(request.Data);
                         await _unitOfWork.SaveChangesAsync();
                         _unitOfWork.Commit();

                         await _service3.Action3(request. Data);
                         await _unitOfWork.SaveChangesAsync();
                        _unitOfWork.Commit();
                 }
                 else
                 {
                        throw new NotFoundException();
                 }
         }
         catch (Exception ex)
         {
                 _unitOfWork.RollBack();
                 throw new NotFoundException();
         }

         return;
    }

@Tcioch
Copy link

Tcioch commented May 19, 2023

@Sarmadjavediqbal
Thank you very much for your answer.
Admittedly, I don't do a commit after every save, because I just mean the opposite. Also, I don't use SaveChangesAsync, because they are saved already a layer below, in the domain service (using AddAsync, for example).
I thought all along that EF opens and closes transactions on each request(and indeed it does, unless there is some other transaction open) and we can only juggle via SaveChangesAsync (call it or not).
Anyway, thanks for your help, now it works as I want :)

@norvegec
Copy link

norvegec commented Jun 3, 2023

It seemd to work nice! Here's my code if anyone's interested

public class UnitOfWork : IUnitOfWork
{
	private readonly ApplicationDbContext _context;
	private IDbContextTransaction? _transaction;

	public UnitOfWork(ApplicationDbContext context)
	{
		_context = context;
	}

	public void BeginTransaction()
	{
		if (_transaction != null)
		{
			return;
		}

		_transaction = _context.Database.BeginTransaction();
	}

	public Task<int> SaveChangesAsync()
	{
		return _context.SaveChangesAsync();
	}

	public void Commit()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Commit();
		_transaction.Dispose();
		_transaction = null;
	}

	public async Task SaveAndCommitAsync()
	{
		await SaveChangesAsync();
		Commit();
	}

	public void Rollback()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Rollback();
		_transaction.Dispose();
		_transaction = null;
	}

	public void Dispose()
	{
		if (_transaction == null)
		{
			return;
		}

		_transaction.Dispose();
		_transaction = null;
	}
}

public interface IUnitOfWork : IDisposable, ITransientService
{
	void BeginTransaction();
	void Commit();
	void Rollback();
	Task<int> SaveChangesAsync();
	Task SaveAndCommitAsync();
}

Any thoughts on IUnitOfWork should be transient or scoped?

DbContext already properly implements UoW and Repository patterns. If you re-implement it yourself, consider that UoW must control the creation of DbContext by itself, rather than assume someone else (e.g. DI container) would provide it with the "right" instance. Otherwise, it's guarantees could be easily broken, like so:

var sharedDbApplicationContext = dbContextFactory();
using var kindaUoW = new UnitOfWork(sharedDbApplicationContext);
var petsRepo = new PetsRepository(sharedDbApplicationContext);
var carsRepo = new CarsRepository(sharedDbApplicationContext);
// pets, cars, employees here are examples of different aggregation roots
petsRepo.Add(new Dog());
carsRepo.Add(new Car());
sharedDbApplicationContext.DbSet<Employees>().Add(new Employee());
kindaUoW.BeginTransaction();
await kindaUoW.SaveAndCommitAsync(); // violation: persisted dog, car and employee in one go

One could try to counter me that by ApplicationDbContext you might actually meant smth smaller, like PetsDbContext, CarsDbContext, etc. It's a step in right direction, but still won't prevent misuse:

var petsDbContext = dbContextFactory();
using var kindaUoW = new UnitOfWork(petsDbContext);
kindaUoW.BeginTransaction();
var petsRepo = new PetsRepository(sharedDbApplicationContext);
petsRepo.Add(myCat);
petsRepo.Add(myDog);
await kindaUoW.SaveAndCommitAsync(); // voila(-tion): persisted both dog and cat

In these examples I've simplified things - actually one service could be adding a cat, another a dog. But since both could be executed with same (scoped) PetsDbContext the execution flow remains as above.

@shad0w-jo4n
Copy link

I think the best and flexible solution for transactions in clean architecture is the System.Transactions.TransactionScope, for e.g., npsql supports this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants