6
\$\begingroup\$

Before posting this, I have spent the past 4 days tirelessly investigating the Internet and the different Stack Exchange websites on how to correctly implement the Unit of Work pattern and Unit Test it using MVC 5 + EF 6.1. So far, the implementation I manage to make it work was this one.

Here's the actual problem:

I am lifting all the lookups of the Database to the Unit of Work (as I've understood). I have a repository called "TournamentRepository" which derives from a GenericRepository with the generic methods. In the example above, it uses a neat trick on implementing statics as extensions so we don't need to create new Interfaces for each new Repository.

  public static async Task<bool> IsUserRegisteredInTournament(this IGenericRepository<PGTournament> tournamentRepository, string tournamentUrl, string userId)
        {
            return await tournamentRepository.Get().Select(x => x.Participants.Where(y => y.UserId == userId))
                .AnyAsync();
        }

What I want to emphasize is the Get() method. I want to know if it is a leaky abstraction.

It is actually from the following:

public class GenericRepository<TEntity>: IGenericRepository<TEntity> where TEntity:class
    {
        private readonly IDbSet<TEntity> _iDbSet;
        private readonly DbSet<TEntity> _dbSet;
        //protected IDbSet<TEntity> Entities { get; set; }


        public GenericRepository(IDbRepositories dbRepositories)
        {
            _iDbSet = dbRepositories.Set<TEntity>();
            _dbSet = dbRepositories.SetDb<TEntity>();
        }
                public IDbSet<TEntity> Get()
    {
        return _iDbSet;
    }

    public async Task<TEntity> FindAsync(int id)
    {
        return await _iDbSet.FindAsync(id);
    }

    public async Task<IEnumerable<TEntity>> GetAllAsync()
    {
        return await _iDbSet.ToListAsync();
    }

    public TEntity SingleOrDefault(Expression<Func<TEntity, bool>> predicate)
    {
        return _iDbSet.SingleOrDefault(predicate);
    }

    public void Add(TEntity entity)
    {
        _iDbSet.Add(entity);
    }

    public void AddRange(IEnumerable<TEntity> entities)
    {
        _dbSet.AddRange(entities);
    }

    public void Remove(TEntity entity)
    {
        _iDbSet.Remove(entity);
    }

    public void RemoveRange(IEnumerable<TEntity> entities)
    {
        _dbSet.RemoveRange(entities);
    }

    /*
     * 
     * Creates a wrapper for Entity Framework's Select Method
     */
    public IQueryable<T> Select<T>(Func<TEntity, T> func ) where T: class 
    {
       return _iDbSet.Select(func).AsQueryable();
    }


    }

Whenever I've tried to do a Select at the repository, I would get no IntelliSense because the object is layered through the interface.

public interface IGenericRepository<TEntity> where TEntity : class
    {
        IDbSet<TEntity> Get();
        Task<IEnumerable<TEntity>> GetAllAsync();
        Task<TEntity> FindAsync(int id);

        // This method was not in the videos, but I thought it would be useful to add.
        TEntity SingleOrDefault(Expression<Func<TEntity, bool>> predicate);

        void Add(TEntity entity);
        void AddRange(IEnumerable<TEntity> entities);

        void Remove(TEntity entity);
        void RemoveRange(IEnumerable<TEntity> entities);

    }

I tried implementing my own Select. The only problem was when I tried to unit test and I ran an Async() method. For unit testing an Async() method, we should follow this.

Unfortunately, I was returning an IQueryable<T> and it did not go through all the conversions that MS does in their TestDbAsyncQueryProvider.

After hours of banging my head, I figured out that I could return the IDbSet<> and do the Select manually.

I rebranded the Get() method for it to return the current IDbSet<>.

Another note

You may have seen that I have a DbSet<> and an IDbSet<>. The reason is (hope you can correct me if I'm wrong), is that RemoveRange and AddRange lack this implementation in IDbSet<> (which I'm using for unit testing, if not, I would receive the un-overridable error for the property not being virtual). So for me, the only way was to actually get the Entity and pass it as both IDbSet and DbSet.

\$\endgroup\$
  • 1
    \$\begingroup\$ By the looks of it, it seems you use Entity Framework and wrap it inside your own repository. Since EF itself implements both the repository pattern and UnitOfWork this just adds code. I'd create a basic DbContext that implements IMyDbContext, which exposes the tables as IDbSet<> and then put ´IsUserRegisteredInTournament()´ in a business layer that can be properly tested \$\endgroup\$ – Mattias Åslund Apr 20 '16 at 14:46
  • \$\begingroup\$ @MattiasÅslund: I had designed this approach before, and I also read that UnitOfWork is implemented by Entity Framework itself so there is no need to use it. I would have the IDbSet<> called each DbSet<> of the database and then Inject them as repositories. Those worked like wonder, and they were much easier to Unit Test. Seeing this video, youtube.com/watch?v=rtXpYpZdOzM I understood that UnitOfWork could allow me to decouple Entity Framework from my Application, and implement a Clean Architecture approach. \$\endgroup\$ – Jose A Apr 20 '16 at 14:56
  • \$\begingroup\$ @MattiasÅslund: I have started to refactor the part of the application and I wanted to go with the best approach possible. One of the advantages of the unit of work (combined with Repository) that I didn't see over the Repository alone was that I could pass a single interface and then gather all the repositories from there, meaning that I would have a reduced set of constructors and could easily swap any special method I would create. Please, correct my reasoning if I'm wrong. \$\endgroup\$ – Jose A Apr 20 '16 at 14:59
  • \$\begingroup\$ It's hard to tell what belongs to the IGenericRepository because you didn't implement all methods in GenericRepository and also added methods to the latter. Please show the code as complete as possible and in the state you want it reviewed, so without tentative methods. Of course you can propose alternatives in separate snippets. \$\endgroup\$ – Gert Arnold Apr 20 '16 at 18:29
  • \$\begingroup\$ Two answers accepted: Get Arnold for the reason why this is leaky. Mattias Aslund for showing the correct implementation! \$\endgroup\$ – Jose A Apr 22 '16 at 12:19
5
\$\begingroup\$

Leaky as an old roof :)

An abstraction is leaky if you need to know details about the concrete implementation in order to work with it properly. Leakiness of an interface may reveal itself in various ways, for example: -

  1. if it's very hard (or impossible) to implement the interface with another technology stack than the one for which it was designed. Or

  2. if it perfectly exposes major features of one implementation, but would make powerful features of other implementations inaccessible. Or

  3. if it enforces a work flow that perfectly fits one implementation but forces other implementations to bend their own preferred work flow. Or

  4. If it doesn't tell the whole story, if an implementation has added core features.

IDbSet is so leaky, I wouldn't even call it an abstraction.

Let's look at what you encounter if you want to implement IDbSet for, say, NHibernate.

  1. You may succeed in implementing the interface methods in a way that they display the same behavior as DbSet. This is hard enough. Find is far from trivial. Add affects an entire object graph. In fact, all methods affecting an entity's tracking state have nitty-gritty details when it comes to the adhered object graph. If you don't respect these details, saving will behave differently.

  2. If forces you to use LINQ-to-NHibernate. This in itself inevitably introduces differences. Both types of LINQ have their own set of supported methods. Both have their own bugs, or run-time issues (like generated queries that perform poorly).
    NHibernate has other powerful query APIs that you can't benefit from, at least not fully.

  3. NHibernate's workflow resembles that of EF. But there are important differences. For example NHibernate's auto flush (auto commit) feature.

  4. The standard implementation if IDbSet, DbSet, has other important methods that are not part of the interface. For example, AsNoTracking. An NHibernate implementation should also implement these to be even remotely interchangeable (which it never will be).

Extension methods

So far, the challenges only just applied to implementing the interface specification itself. The real bummer are extension methods.

If you've got an IDbSet, it seems reasonable to expect that you can apply extension methods from DbSetMigrationsExtensions or the vast QueryableExtensions class. After all, DbSetMigrationsExtensions extend IDbSet and QueryableExtensions is in the same namespace.

But let's look at the source code of just an arbitrary method, QueryableExtensions.AnyAsync

public static Task<bool> AnyAsync<TSource>(this IQueryable<TSource> source, CancellationToken cancellationToken)
{
    Check.NotNull(source, "source");

    var provider = source.Provider as IDbAsyncQueryProvider;
    if (provider != null)
    {
        return provider.ExecuteAsync<bool>(
            Expression.Call(
                null,
                _any.MakeGenericMethod(typeof(TSource)),
                new[] { source.Expression }
                ),
            cancellationToken);
    }
    else
    {
        throw Error.IQueryable_Provider_Not_Async();
    }
}

As you see, it expects a Provider that implements IDbAsyncQueryProvider;. This is an interface in the System.Data.Entity.Infrastructure namespace. So you run from one implementation challenge into the other. I don't think there will be any provider implementing that interface, other than those created for Entity Framework. And it can be mocked for the purpose of unit tests in an EF environment, as in the link you shared.

Now what?

Does this mean you should revert to the original plan and jump through hoops to return IQueryable correctly? I don't think so. For one, IQueryable is leaky as well. But I may have answered your question in a stricter sense than you expected. Maybe you never intended to craft a completely different implementation. Maybe you only contemplated the implications of exposing IDbSet in a unit test setting. If so, IDbSet, or even DbSet is good enough.

\$\endgroup\$
  • \$\begingroup\$ Holy cow, this is like a super answer to all the blasphemy spread over the web. So, should I implement something like Mattias is suggesting me? I had that approach before, but a little bit complicated. I would call a class that would instantiate the DbContext and expose its property through there. \$\endgroup\$ – Jose A Apr 21 '16 at 23:25
  • \$\begingroup\$ I've added the complete GenericRepository list of methods. Just in case. \$\endgroup\$ – Jose A Apr 21 '16 at 23:31
  • 2
    \$\begingroup\$ By all means. Using interfaces solely for the purpose of unit test implementations (and never intending to implement them for entire new production platforms) is very sensible and very common. I've yet to see the first seamless transition from one DAL implementation to another. Beware of the differences between LINQ-to entities and LINQ-to-objects though. They made me decide to use integration tests mainly, and unit test where EF doesn't come in play. \$\endgroup\$ – Gert Arnold Apr 22 '16 at 6:00
3
\$\begingroup\$

Maybe I misunderstand what you want to achieve, so I put a piece of code here to clarify my position.

I like unit testing. So I like to create an interface for the data layer. Something like this should do;

public interface IMyDbContext
{
    IDbSet<Participant> Participants { get; set; }
    IDbSet<PGTournament> Tournaments { get; set; }
}

I then create a vanilla EF context, like this;

public partial class EFDbContext : System.Data.Entity.DbContext, IMyDbContext
{
    public EFDbContext() : base("name=EFDbContext")
    {
    }

    public virtual IDbSet<Participant> Participants { get; set; }
    public virtual IDbSet<PGTournament> Tournaments { get; set; }
}

Then I create a business logic object (or service object if you prefer);

public class MyBusinessLogic
{
    readonly IMyDbContext _db;

    public MyBusinessLogic(IMyDBContext db)
    {
        _db = db;
    }

    public async Task<bool> IsUserRegisteredInTournament(string tournamentUrl, string userId)
    {
        return await _db.Tournaments
            .Where(t => t.Url == tournamentUrl)
            .SelectMany(t => t.Participants)
            .Where(p => p.UserId == userId)
            .AnyAsync();
    }
}

Calling the business logic from code could look like;

using (var db = new EFDbContext())
using (var bll = new MyBusinessLogic(db))
{
    var isRegistered = await bll.IsUserRegisteredInTournament("http://MyTournament.org", "USER_ID_75");
    Trace.TraceInformation("User is registered: {0}", isRegistered);        
}

There is no leaky abstraction here. The business logic is responsible for business logic. The data layer is responsible for loading and saving data and the UI can request services from the business logic. You can easily test the business logic by injecting a mock IMyDbContext and if you like DI containers you can automate how the classes are created.

\$\endgroup\$
  • \$\begingroup\$ This solution is just plain awesome. I was implementing something similar, but this is even simpler than what I had before implementing UnitOfWork. \$\endgroup\$ – Jose A Apr 21 '16 at 23:21
  • \$\begingroup\$ Shoot. Now I feel that I wasted an entire week trying to reinvent the wheel when I had it solved already -_-. \$\endgroup\$ – Jose A Apr 21 '16 at 23:27
  • 1
    \$\begingroup\$ :) been there, done that. \$\endgroup\$ – Mattias Åslund Apr 22 '16 at 4:15
  • \$\begingroup\$ Thanks a billion! Let me go back in time and refactor the code ;) \$\endgroup\$ – Jose A Apr 22 '16 at 12:22
  • \$\begingroup\$ I just realized something through your code. My old code was also leaking the Dbcontext, because I would expose it as a property, even though I used IDbSet. Now I've completely understood why I had a redirection method which abstracted the DbContext. Thanks a million. \$\endgroup\$ – Jose A Apr 22 '16 at 12:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.