Onion Architecture, Unit of Work and a generic Repository pattern

Question

This is the first time I am implementing a more domain-driven design approach. I have decided to try the Onion Architecture as it focuses on the domain rather than on infrastructure/platforms/etc.

In order to abstract away from Entity Framework, I have created a generic repository with a Unit of Work implementation.

The IRepository<T> and IUnitOfWork interfaces:

public interface IRepository<T>
{
    void Add(T item);

    void Remove(T item);

    IQueryable<T> Query();
}

public interface IUnitOfWork : IDisposable
{
    void SaveChanges();
}

Entity Framework implementations of IRepository<T> and IUnitOfWork:

public class EntityFrameworkRepository<T> : IRepository<T> where T : class
{
    private readonly DbSet<T> dbSet;

    public EntityFrameworkRepository(IUnitOfWork unitOfWork)
    {
        var entityFrameworkUnitOfWork = unitOfWork as EntityFrameworkUnitOfWork;

        if (entityFrameworkUnitOfWork == null)
        {
            throw new ArgumentOutOfRangeException("Must be of type EntityFrameworkUnitOfWork");
        }

        dbSet = entityFrameworkUnitOfWork.GetDbSet<T>();
    }

    public void Add(T item)
    {
        dbSet.Add(item);
    }

    public void Remove(T item)
    {
        dbSet.Remove(item);
    }

    public IQueryable<T> Query()
    {
        return dbSet;
    }
}

public class EntityFrameworkUnitOfWork : IUnitOfWork
{
    private readonly DbContext context;

    public EntityFrameworkUnitOfWork()
    {
        this.context = new CustomerContext();;
    }

    internal DbSet<T> GetDbSet<T>()
        where T : class
    {
        return context.Set<T>();
    }

    public void SaveChanges()
    {
        context.SaveChanges();
    }

    public void Dispose()
    {
        context.Dispose();
    }
}

The Customer repository:

public interface ICustomerRepository : IRepository<Customer>
{

}

public class CustomerRepository : EntityFrameworkRepository<Customer>, ICustomerRepository 
{
    public CustomerRepository(IUnitOfWork unitOfWork): base(unitOfWork)
    {
    }
}

ASP.NET MVC controller using the repository:

public class CustomerController : Controller
{
    UnityContainer container = new UnityContainer();

    public ActionResult List()
    {
        var unitOfWork = container.Resolve<IUnitOfWork>();
        var customerRepository = container.Resolve<ICustomerRepository>();

        return View(customerRepository.Query());
    }

    [HttpPost]
    public ActionResult Create(Customer customer)
    {
        var unitOfWork = container.Resolve<IUnitOfWork>();
        var customerRepository = container.Resolve<ICustomerRepository>();; 

        customerRepository.Add(customer);

        unitOfWork.SaveChanges();

        return RedirectToAction("List");
    }
}

Dependency injection with unity:

container.RegisterType<IUnitOfWork, EntityFrameworkUnitOfWork>();
container.RegisterType<ICustomerRepository, CustomerRepository>();

Solution:

PROBLEMS?

• Repository implementation (EF code) is very generic. It all sits in side the EntityFrameworkRepository<T> class. Concrete model repositories do not contain any of this logic. This saves me from writing tons of redundant code, but possibly sacrifices flexibility?

• The ICustomerRepository and CustomerRepository classes are basically empty. They are purely there to provide abstraction. As far as I understand, this fits with the vision of the Onion Architecture, where infrastructure and platform-dependent code sits on the outside of your system, but having empty classes and empty interfaces feels wrong?

• To use a different persistence implementation (say Azure Table Storage), then a new CustomerRepository class would need to be created and would inherit a AzureTableStorageRepository<T>. But this could lead to redundant code (multiple CustomerRepositories)? How would this effect mocking?

• Another implementation (say Azure Table Storage) has limitations on transnational support so the a AzureTableStorageUnitOfWork class wouldn't work in this context.

Are there any other issues with the way I have done this?

(I have taken most of my inspiration from this post)

Underglaze

You are using the service locator anti-pattern by taking a dependency on the IoC container. Instead, you should register a factory class to inject into your controller. Some IoC containers can produce one of these for you in the form of a Func<T> dependency – Bittner

Answer 1

I can say that this code is good enough for the first time try but it does have some places to improve.

Let's go through some of them.

1. Dependency injection (DI) and usage of IoC.

You use the simplest version of Service Locator pattern - container instance itself.

I suggest you use 'constructor injection'. You can find more information here (ASP.NET MVC 4 Dependency Injection).

public class CustomerController : Controller
{
    private readonly IUnitOfWork unitOfWork;
    private readonly ICustomerRepository customerRepository;

    public CustomerController(
        IUnitOfWork unitOfWork, 
        ICustomerRepository customerRepository)
    {
        this.unitOfWork = unitOfWork;
        this.customerRepository = customerRepository;
    }

    public ActionResult List()
    {
        return View(customerRepository.Query());
    }

    [HttpPost]
    public ActionResult Create(Customer customer)
    {
        customerRepository.Add(customer);
        unitOfWork.SaveChanges();
        return RedirectToAction("List");
    }
}

2. Unit of Work (UoW) scope.

I can't find lifestyle of IUnitOfWork and ICustomerRepository. I am not familiar with Unity but msdn says that TransientLifetimeManager is used by default. It means that you'll get a new instance every time when you resolve type.

So, the following test fails:

[Test]
public void MyTest()
{
    var target = new UnityContainer();
    target.RegisterType<IUnitOfWork, EntityFrameworkUnitOfWork>();
    target.RegisterType<ICustomerRepository, CustomerRepository>();

    //act
    var unitOfWork1 = target.Resolve<IUnitOfWork>();
    var unitOfWork2 = target.Resolve<IUnitOfWork>();

    // assert
    // This Assert fails!
    unitOfWork1.Should().Be(unitOfWork2);
} 

And I expect that instance of UnitOfWork in your controller differs from the instance of UnitOfWork in your repository. Sometimes it may be resulted in bugs. But it is not highlighted in the ASP.NET MVC 4 Dependency Injection as an issue for Unity.

In Castle Windsor PerWebRequest lifestyle is used to share the same instance of type within single http request.

It is common approach when UnitOfWork is a PerWebRequest component. Custom ActionFilter can be used in order to invoke Commit() during invocation of OnActionExecuted() method.

I would also rename the SaveChanges() method and call it simply Commit as it is called in the example and in the PoEAA.

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

3.1. Dependencies on repositories.

If your repositories are going to be 'empty' it is not needed to create specific interfaces for them. It is possible to resolve IRepository<Customer> and have the following code in your controller

public CustomerController(
    IUnitOfWork unitOfWork, 
    IRepository<Customer> customerRepository)
{
    this.unitOfWork = unitOfWork;
    this.customerRepository = customerRepository;
}

There is a test that tests it.

[Test]
public void MyTest()
{
    var target = new UnityContainer();
    target.RegisterType<IRepository<Customer>, CustomerRepository>();

    //act
    var repository = target.Resolve<IRepository<Customer>>();

    // assert
    repository.Should().NotBeNull();
    repository.Should().BeOfType<CustomerRepository>();
}

But if you would like to have repositories that are 'layer of abstraction over the mapping layer where query construction code is concentrated.' (PoEAA, Repository)

A Repository mediates between the domain and data mapping layers, acting like an in-memory domain object collection. Client objects construct query specifications declaratively and submit them to Repository for satisfaction.

3.2. Inheritance on EntityFrameworkRepository.

In this case I would create a simple IRepository

public interface IRepository
{
    void Add(object item);

    void Remove(object item);

    IQueryable<T> Query<T>() where T : class;
}

and its implementation that knows how to work with EntityFramework infrastructure and can be easily replaced by another one (e.g. AzureTableStorageRepository).

public class EntityFrameworkRepository : IRepository
{
    public readonly EntityFrameworkUnitOfWork unitOfWork;

    public EntityFrameworkRepository(IUnitOfWork unitOfWork)
    {
        var entityFrameworkUnitOfWork = unitOfWork as EntityFrameworkUnitOfWork;

        if (entityFrameworkUnitOfWork == null)
        {
            throw new ArgumentOutOfRangeException("Must be of type EntityFrameworkUnitOfWork");
        }

        this.unitOfWork = entityFrameworkUnitOfWork;
    }

    public void Add(object item)
    {
        unitOfWork.GetDbSet(item.GetType()).Add(item);
    }

    public void Remove(object item)
    {
        unitOfWork.GetDbSet(item.GetType()).Remove(item);
    }

    public IQueryable<T> Query<T>() where T : class
    {
        return unitOfWork.GetDbSet<T>();
    }
}

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

public class EntityFrameworkUnitOfWork : IUnitOfWork
{
    private readonly DbContext context;

    public EntityFrameworkUnitOfWork()
    {
        this.context = new CustomerContext();
    }

    internal DbSet<T> GetDbSet<T>()
        where T : class
    {
        return context.Set<T>();
    }

    internal DbSet GetDbSet(Type type)
    {
        return context.Set(type);
    }

    public void Commit()
    {
        context.SaveChanges();
    }

    public void Dispose()
    {
        context.Dispose();
    }
}

And now CustomerRepository can be a proxy and refer to it.

public interface IRepository<T> where T : class
{
    void Add(T item);

    void Remove(T item);
}

public abstract class RepositoryBase<T> : IRepository<T> where T : class
{
    protected readonly IRepository Repository;

    protected RepositoryBase(IRepository repository)
    {
        Repository = repository;
    }

    public void Add(T item)
    {
        Repository.Add(item);
    }

    public void Remove(T item)
    {
        Repository.Remove(item);
    }
}

public interface ICustomerRepository : IRepository<Customer>
{
    IList<Customer> All();

    IList<Customer> FindByCriteria(Func<Customer, bool> criteria);
}

public class CustomerRepository : RepositoryBase<Customer>, ICustomerRepository
{
    public CustomerRepository(IRepository repository)
        : base(repository)
    { }

    public IList<Customer> All()
    {
        return Repository.Query<Customer>().ToList();
    }

    public IList<Customer> FindByCriteria(Func<Customer, bool> criteria)
    {
        return Repository.Query<Customer>().Where(criteria).ToList();
    }
}

Melpomene

Thank you for the great answer. (1) Done (2) I was struggling with this problem. Now using the PerResolveLifetimeManager (3.1) I am now using the specific interfaces for specific fetch methods (like GetCustomerByName). I have decided to remove the IQueryable<T> Query() method from the repositories as it was leaking EF details into the client code. (3.2) I will look into this closer. – Underglaze

Answer 2

The only con I see is that you are highly dependent on your IOC tool, so make sure your implementation is solid. However, this is not unique to Onion designs. I have used Onion on a number of projects and have not run into any real 'gotchas".

Lindsy

Isn't IoC pivotal to how Onion is meant to work? I could declare concrete classes in my calling code, but then I will increase coupling and decrease testability quite substantially? Thanks for the reply :) – Underglaze

Yes which is why I say its implementation needs to be solid. You need to use DI and IOC to make Onion work. – Lindsy

Answer 3

I see couple of serious problems in code.

The 1st problem is relashionship between repositories and UoW.

    var unitOfWork = container.Resolve<IUnitOfWork>();
    var customerRepository = container.Resolve<ICustomerRepository>();

Here is implicit dependency. Repository will not work itself without UoW! Not all repositories needs to be connected with UoW. For example what about stored procedures? You have stored procedure and you hide it behind repository. Stored procedure invokation uses separate transaction! At least not in all cases. So if I resolve the only repository and add item then it will not work. Moreover this code will not work if I set Transient life license because repository will have another UoW instance. So we have tight implicit coupling.

The 2nd problem you create tight coupling between DI container engine and use it as service locator! Service locator is not good approach to implement IoC and aggregation. In some case it is anti pattern. DI container should be used

Aldora

Sorry.. typed quickly and published with errors. DI containers should be used on top level. You have to implemeny controller factory and implement dependency injection via contructor. Using this way you will remove redundant dependency on container and will get explicit dependencies which are set in ctor. – Aldora