Different data sources for UnitOfWork and Repository patterns

Question

I was involved in some code-review discussions about UnitOfWork and Repository pattern implementation with EF, which put me in some sort of confusion.

Yes, I know that EF already implements UnitOfWork and Repository pattern, but the question is not about that. I am just asking for your opinion regarding existing code base.

Let's start with IUnitOfWork interface declaration:

public interface IUnitOfWork : IDisposable
{

    void CommitTransaction();
    IEnumerable<DbEntityValidationResult> Errors();
    dynamic Repository<TEntityType>() where TEntityType : BaseEntity;

}

1. I do not really like that IUnitOfWork declared as IDisposable. For me it looks like leaky abstraction, why should my UnitOfWork be Disposable if it isn't by his nature?

2. I also do not like the Errors() method. First of all it introduces dependency on DbEntityValidationResult class, which I do not want in my generic IUnitOfWork interface. I would prefer my newly created Commit() method to return my custom ValidationResult class, which will act as mapping for DbEntityValidationResult.

   Something like this:

    IEnumerable<ValidationResult> Commit();
   

3. The last thing is return value of Repository<T>() method. I don't like dynamic object, which is returned. I want some interface to be returned from such method, like IRepository<T> (I will ask about that a bit later).

Now let's move to the implementation. I will highlight some thing, that looks strange to me.

As soon as IUnitOfWork is declared as IDisposable the concrete implementation of IUnitOfWork implements some sort of Disposable pattern:

    private bool _disposed;

    ~UnitOfWork()
    {
        SharedDispose(false);
    }

    public void Dispose()
    {
        SharedDispose(true);
        GC.SuppressFinalize(this);
    }

    private void SharedDispose(bool disposing)
    {
        if (_disposed || !disposing)
        {
            return;
        }
        _context.Dispose();
        _disposed = true;
    }

I do not see any reasons to have all this stuff. As soon as we do not own any unmanaged resources, we can skip all these destructors and all these instructions for GC to skip finalization queue for our unit of work object. Yes, we still need to implement IDisposable as soon as EF DbContext (which we own internally) implements it, but the implementation can be so much simpler:

    public void Dispose()
    {
        _context.Dispose();
        _context = null;
    }

Now let's see dynamic Repository<T>() implementation:

    public dynamic Repository<TEntityType>() where TEntityType : BaseEntity
    {
        if (_repositories == null)
        {
            _repositories = new Dictionary<string, object>();
        }

        var type = typeof(TEntityType).Name;
        var repositoryTypeName = typeof(UnitOfWork).Namespace + ".Repositories." + type + "Repository";
        var repoType = Type.GetType(repositoryTypeName);

        if (!_repositories.ContainsKey(type))
        {
            object repositoryInstance;
            if (repoType != null)
            {
                repositoryInstance = Activator.CreateInstance(repoType, _context);
            }
            else
            {
                var repositoryType = typeof(GenericRepository<>);
                repositoryInstance = Activator.CreateInstance(repositoryType.MakeGenericType(typeof(TEntityType)), _context);
            }
            _repositories.Add(type, repositoryInstance);
        }

        return repoType == null ? (GenericRepository<TEntityType>)_repositories[type] : Convert.ChangeType(_repositories[type], repoType);
    }

1. The method uses reflection to create specific repository for T-entity or returns GenericRepository<T> if there is no specific implementation defined. So the question is that a good practice to use reflection for that?

2. Another thing, why do we need this cache dictionary if unit of work object is injected in the scope of HTTP request?

3. How we can unify these repositories to get rid of dynamic object, which is returned right now?

My first idea was not to use specific repositories at all, instead create more sophisticated API for generic repository (which supports filtering, query object, paging and etc) and just return the interface like IRepository<T>. But not really sure if it is a good solution? Beside this, the solution is using two data sources - the one is EF/Database and another one is web services. Web service repositories cannot so easily mapped to the IRepository<T> and most probably required some specific interface to be implemented.

What is the best practice to create repositories for two different data sources? How can we get rid of dynamic object? Do we need to create repositories for database and web services in the scope of one unit of work? Or should it be a separate one?

Answer 1

The interface

I don't mind the IDisposable. If one implementation is designed that way and you retrieve it from the same factory as the other implementations, you really don't have a choice:

using(IUnitOfWork uow = UoW.GetDefault())
{
  // work...
}

Errors-property is ugly. A return value from Commit is better, an exception is best. The code should not be allowed to continue running when something fails.

The dependency on DbEntityValidationResult is not good, but allowing a custom type isn't any better for the same reasons. I think a ValidationResult should be defined in the same package and namespace as the IUnitOfWork interface.

Then the dynamic return type. Looks like someone declared a defeat right there. Why bother with the interface at all when you decide to go typeless?

The implementation

The destructor calls SharedDispose(false), which means !disposing is true, which then causes the method to return immediately. You can delete the destructor, it's a nop. Or a bug. Someone obviously tried to follow best practice and failed.

The reflection code used to create the repository looks like a poor man's DI container. Nothing wrong in that, if this is the only place you need this kind of functionality. But if there is a 'real' DI container in there somewhere, I suggest a refactoring.

The caching is probably introduced because reflection is 'slow'. But slow according to what? My guess is it's negligible compared to the actual repository usage. I would remove the cache unless someone actually profiled the code and can prove a significant improvement.

If all repositories have the same shape, then dynamic may be replaced by a generic interface. But if they don't, I can't see any way to unify them. If that's the case, I suggest replacing the generic Repository() with typed versions, like:

public FooRepository GetFooRepository() {}
public BarRepository GetBarRepository() {}

Then there's no longer need for the reflection stuff, as each method knows which one to create.

A suggestion: Change abstraction level

It looks like the entity type used by DbContext is the domain class. That gives the following dependency:

Domain object -> DbContext

If that's true, then all the work done so far will not break the dependency, only hide it. I think all the problems with leaky abstractions also comes from that fact.

From the client point of view, we should see this:

Domain object -> Repository

From within the Repository we should see:

Domain object -> State DTO -> Entity -> DbContext

Depending on your needs, State DTO can be the Entity, or use the Entity, or be completely isolated from it.

The change here is that instead of persisting the domain object, the domain object will store its state to a new object, which is then persisted. This decouples the database schema from the domain object.

Two different data sources

This suggests you have two bounded contexts where the domain objects may have the same name but address different aspects. I would accept the difference and let each boundary have their own set of classes pretending the other context does not exist:

A Customer in the Shopping-module is very different from the Customer in the Invoicing-module.

And by that, I think I have given my opinion on your last question: If some data lives in the database and some other in a web service, then you have two bounded context and you need two units of work. How to coordinate those two, I choose to be out of scope of this question...