Unit of work with EF 6 and Dependency injection Design problems

Question

I develop web application with entity framework 6, and have difficulties with designing the application structure. My main issue is how to deal with the dependency injection in my specific case.

The code below is how I would like the application to look like. I'm using Autofac but I guess it is basic enough for every DI user to understand:

public interface IUnitOfWork
{
    bool Commit();
}

public class UnitOfWork : IUnitOfWork, IDisposable
{
    private DbContext _context;

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

    public bool Commit()
    {
        // ..
    }

    public bool Dispose()
    { 
          _context.Dispose();
    }
}

public class ProductsController : ApiController 
{
     public ProductsController(IProductsManager managet)
}   


public class ProductsManager : IProductsManager
{
    private Func<Owned<IUnitOfWork>> _uowFactory;
    private IProductsDataService _dataService;

    public Manager(Func<Owned<IUnitOfWork>> uowFactory, IProductsDataService dataService)
    {
        _dataService = dataService;
        _uowFactory = uowFactory;
    }

    public bool AddProduct(ProductEntity product)
    {
        using (ownedUow = _uowFactory())
        {
            var uow = ownedUow.Value;

            var addedProduct = _dataService.AddProduct(product);

            if (addedProduct != null)
                uow.Commit();
        }
    }
}

public interface IProductsDataService
{
    ProductEntity AddProduct (Product product)
}

public class ProductsDataService : IProductsDataService 
{
    private IRepositoriesFactory _reposFactory;

    public DataService(IRepositoriesFactory reposFactory)
    {
        _reposFactory = reposFactory;
    }

    public ProductEntity AddProduct(ProductEntity product)
    {
        var repo = reposFactory.Get<IProductsRepository>();

        return repo.AddProduct(product);
    }
}


public interface IRepositoriesFactory
{
    T Get<T>() where T : IRepository
}

public class RepositoriesFactory : IRepositoriesFactory
{
    private ILifetimeScope _scope;

    public RepositoriesFactory(ILifetimeScope scope)
    {
        _scope = scope;
    }

    public T Get<T>() where T : IRepository
    {
        return _scope.Resolve<T>();
    }

}

public interface IProductsRepository
{
    ProductEntity AddProduct(ProductEntity);
}


public ProductsRepository : IProductsRepository
{
    private DbContext _context;

    public ProductsRepository(DbContext context)
    {
        _context = context;
    }

    public ProductEntity AddProduct(ProductEntity)
    {
        // Implementation..
    }
}

This is the implementation I find ideal, however I don't know how to accomplish this, Because my ProductsDataService is singleton, therefore it is not related to the Owned scope that is created by the unit of works factory. Is there a way I can associate the Repositories to be created and take in their ctor the same DbContext that was created for the unit of work? Change the code in the RepositoriesFactory somehow?

At the moment what I have is that the unit of work contains the repositories factory so that the context within the repositories will be the same as in the unit of work (I register the DbContext as per scope), The manager at the moment does the job of the DataService as well, which I dont like.

I know I can pass around the UnitOfWork - method injection to the DataService methods, but I'd rather use Ctor injection, as it looks better in my opinion.

What I want is to seperate this - A manager that it's job is to instantiate unit of works and commit them if needed, and another class (DataService) that actually executes the logic.

Regardless, I would like to hear your opinion about this implementation if you have any comments / ideas for improvement.

Thanks for your time!

EDIT: This is what I ended up with:

public interface IUnitOfWork
{
    bool Commit();
}

public class DatabaseUnitOfWork : IUnitOfWork
{
    private DbContext _context;

    public DatabaseUnitOfWork(DbContext context)
    {
        _context = context;
    }

    public bool Commit()
    {
        // ..
    }
}

// Singleton
public class ProductsManager : IProductsManager
{
    private Func<Owned<IProductsDataService>> _uowFactory;

    public ProductsManager(Func<Owned<IProductsDataService>> uowFactory)
    {
        _uowFactory = uowFactory;
    }

    public bool AddProduct(ProductEntity product)
    {
        using (ownedUow = _uowFactory())
        {
            var dataService = ownedUow.Value;

            var addedProduct = _dataService.AddProduct(product);

            if (addedProduct != null)
                uow.Commit();
        }
    }
}

public interface IProductsDataService : IUnitOfWork
{
    ProductEntity AddProduct (Product product)
}

public class ProductsDataService : DatabaseUnitOfWork, IDataService 
{
    private IRepositoriesFactory _reposFactory;

    public DataService(IRepositoriesFactory reposFactory)
    {
        _reposFactory = reposFactory;
    }

    public ProductEntity AddProduct(ProductEntity product)
    {
        var repo = _reposFactory .Get<IProductsRepository>();

        return repo.AddProduct(product);
    }
}


public interface IRepositoriesFactory
{
    Get<T>() where T : IRepository
}

public class RepositoriesFactory : IRepositoriesFactory
{
    private ILifetimeScope _scope;

    public RepositoriesFactory(ILifetimeScope scope)
    {
        _scope = scope;
    }

    public Get<T>() where T : IRepository
    {
        return _scope.Resolve<T>();
    }

}

public interface IProductsRepository
{
    ProductEntity AddProduct(ProductEntity);
}


public ProductsRepository : IProductsRepository
{
    private DbContext _context;

    public ProductsRepository(DbContext context)
    {
        _context = context;
    }

    public ProductEntity AddProduct(ProductEntity)
    {
        // Implementation..
    }
}

Answer 1

You don't want singleton DbContext in a singleton instance. This is ok, it can be done with factory. Additionally you want to share this DbContext. This is ok also, you can resolve and return DbContext with related life time on the factory. The problem is; you want share non-singleton DbContext in a single instance without managing lifetime (Tcp/Ip request).

What's the reason of ProductService and ProductManager are singleton ? I suggest yo to use ProductService and ProductManager per lifetimescope. When you have http request it's fine. When you have tcp/ip request you can begin new lifetime scope(as top level as you can) then resolve ProductManager there.

Update: Code for solution 1 which I mentioned on comments.

Managers have to be singleton (as you told).

Other than managers you should register dbcontext,services,repositories and Uow as per lifetime scope.

We could initilaize like this:

public class ProductsManager : IProductsManager
    {
        //This is kind of service locator. We hide Uow and ProductDataService dependencies.
        private readonly ILifetimeScope _lifetimeScope;

        public ProductsManager(ILifetimeScope lifetimeScope)
        {
            _lifetimeScope = lifetimeScope;
        }
    }

But This is kind of service locator. We hide Uow and ProductDataService dependencies.

So we should implement a provider:

public IProductsManagerProvider : IProductsManager
{

}
public class ProductsManagerProvider : IProductsManagerProvider
{
    private readonly IUnitOfWork _uow;
    private readonly IProductsDataService _dataService;

    public ProductsManagerProvider (IUnitOfWork uow, IProductsDataService dataService)
    {
        _dataService = dataService;
        _uow = uow;
    }

    public bool AddProduct(ProductEntity product)
    {
        var result=false;
        var addedProduct = _dataService.AddProduct(product);
        if (addedProduct != null)
            result=_uow.Commit()>0;
        return result;
    }
}

And we register it as per dependency (Because we will use it with factory).

container.RegisterType<ProductsManagerProvider>().As<IProductsManagerProvider>().InstancePerDependency();

Your ProductsManager class should be like this. (Now we don't hide any dependecies).

public class ProductsManager : IProductsManager
{
    private readonly Func<Owned<IProductsManagerProvider>> _managerProvider;
    //Now we don't hide any dependencies.
    public ProductsManager(Func<Owned<IProductsManagerProvider>> managerProvider)
    {
        _managerProvider = managerProvider;
    }

    public bool AddProduct(ProductEntity product)
    {
        using (var provider = _managerProvider())
        {
            return provider.Value.AddProduct(product);
        }
    }
}

I have tested with my own classes.

You have singleton manager instance which has a factory to create manager provider. Manager Providers are per dependency because every time we should get new instance in singleton. Everything in providers per lifetime so their lifetime are connected providers per dependency lifetime.

When you addproduct in manager Container creates 1 Provider,1 DbContext, 1 DataService and 1 Uow (DbContext is shared). Provider is disposed (per dependency) with all realeted instances (DbContex,Uow,DataService) after returns on method in Manager.

Answer 2

I agree with Bruno Garcia's advice about the problem with your code. However, I see a few other problems with it.

I will start by saying I haven't used the Unit Of Work pattern explicitly like you have, but I do understand what you're going for.

The problem that Bruno didn't get into is the fact that you have poor separation of concerns. He hinted at that a bit, and I'll explain more: Your controller has two separate competing objects in it, both trying to utilize the same resource (DbContext). As he said, what you're looking to do is to have just a single DbContext for each request. There's a problem with that, though: There is nothing stopping a Controller from trying to continue to utilize the ProductsRepository after the UnitOfWork is disposed of. If you do, the connection to the database has already been disposed of.

Because you have two objects that need to use the same resource, you should rearchitect it where one object encapsulates the other. This also gives the added benefit of hiding from the Controller any concerns at all about Data Propagation.

All the Controller should know about is your Service object, which should contain all of the business logic as well as gateways to the Repository and its Unit of Work, while keeping it invisible from the Service's consumer. This way, the Controller only has a single object to worry about handling and disposing of.

One of the other ways you could get around this would be to have the ProductsRepository derive from the UnitOfWork so that you don't have to worry about any duplicated code.

Then, inside your AddProduct method, you would call _context.SaveChanges() before returning that object back up along the pipeline to your Controller.

UPDATE (Braces are styled for compactness)

Here is the layout of what you'd want to do:

UnitOfWork is your bottom most layer that includes the connection to the database. However, make this abstract as you don't want to allow a concrete implementation of it. You no longer need the interface, as what you do within your Commit method should never be exposed, and the saving of the object should be done within the methods. I'll show how down the line.

public abstract class UnitOfWork : IDisposable {
    private DbContext _context;

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

    protected bool Commit() {
        // ... (Assuming this is calling _context.SaveChanges())
    }

    public bool Dispose() {
        _context.Dispose();
    }
}

Your repository is the next layer up. Derive that from UnitOfWork so that it inherits all of the behavior, and will be the same for each of the specific types.

public interface IProductsRepository {
    ProductEntity AddProduct(ProductEntity product);
}

public ProductsRepository: UnitOfWork, IProductsRepository {
    public ProductsRepository(DbContext context) : base(context) { }

    public ProductEntity AddProduct(ProductEntity product) {
        // Don't forget to check here. Only do that where you're using it.
        if (product == null) {
            throw new ArgumentNullException(nameof(product));
        }

        var newProduct = // Implementation...

        if (newProduct != null) {
            Commit();
        }

        return newProduct;
    }
}

With that in place, all you care about now is just having your ProductsRepository. In your DataService layer, utilize Dependency Injection and just pass the ProductsRepository itself. If you are really set on using the factory, then pass the factory, but have your member variable still be the IProductsRepository. Don't make each method have to figure that out.

Don't forget to have all of your interfaces derive from IDisposable

public interface IProductsDataService : IDisposable {
    ProductEntity AddProduct(ProductEntity product);
}

public class ProductsDataService : IProductsDataService {
    private IProductsRepository _repository;

    public ProductsDataService(IProductsRepository repository) {
        _repository = repository;
    }

    public ProductEntity AddProduct(ProductEntity product) {
        return _repository.AddProduct(product);
    }

    public bool Dispose() {
        _repository.Dispose();
    }
}

If you'd dead set on using the ProductsManager, you can, but it is just another layer that is no longer providing much benefit. Same deal would go with that class.

I'll finish with your Controller as I would have it.

public class ProductsController : Controller {
    private IProductsDataService _service;

    public ProductsController(IProductsDataService service) {
        _service = service;
    }

    protected override void Dispose(bool disposing) {
        _service.Dispose();

        base.Dispose(disposing);
    }

    // Or whatever you're using it as.
    [HttpPost]
    public ActionResult AddProduct(ProductEntity product) {
        var newProduct = _service.AddProduct(product);

        return View(newProduct);
    }
}

Answer 3

It seems the issue is not really making sure the DbContext instance injected into UnitOfWork and ProductsRepository is the same.

This could be achieved by registering DbContext as InstancePerLifetimeScope and creating a new LifetimeScope before resolving IUnitOfWork and ProductsRepository. Any dependency not owned by you would be disposed at the time of disposal of the LifetimeScope.

The problem seems to be that you don't have an explicit relationship between those two classes. Your UoW doesn't depend on 'any DbContext', it depends on whatever DbContext is involved in the current transaction. That specific one.

There's no direct relationship between your UoW and the Repositories either. That doesn't look like a UoW pattern.

I couldn't figure out who's going to Dispose the IRepository created by your IRepositoryFactory. You are using the container to resolve it (via the ILifetimeScope you got injected into RepositoriesFactory). Unless whoever gets that instance from the Factory disposes it, it would only be disposed by disposing the LifetimeScope injected into IRepositoryFactory.

Another problem that would arise is ownership of DbContext. You'd be able to Dispose it on that using block via Dispose on your IUnitOfWork. But your UnitOfWork doesn't own that instance either. The container does. Would the Repositories also try disposing the DbContext? They also received via constructor injection.

I'd suggest rethinking this solution.