locked
Question on Repository and Unit of Work (with DbContext) RRS feed

  • Question

  • User1871598166 posted

    Hello everyone!

    I was looking at the thread "http://forums.asp.net/t/2024964.aspx" and found an interesting comment from mikesdotnetting. That thread is locked so I had to create a new one.

    Anyway, he said:

    More and more and more people are questioning the value of writing a "Repository wrapper" class around a repository pattern. You add a class called ProductRepository and then put a method in it called FindById which wraps the DbSet<T>.Find() method. How is that an implementation of the repository pattern? 

    To my mind, there is nothing wrong with having DbContext in your BLL layer. You can mock it in your tests

    I've been using Entity Framework most of the time with lazy loading disabled. For my queries with complex objects I need to add quite a few "Include" to eager load those entities. Sometimes with a "Select" inside to eager load child of child property and so on. My wrapper to DbContext ends up being a place to center that logic.

    I also ran into situations with disconnected scenarios where I needed some data layer logic and it seemed to me that this so called wrapper would be a good place for this code. I mean, just to keep data layer logic outside BLL.

    What do you guys think this about this approach? Is using a repository along with DbContext worth these days?

    Tuesday, January 27, 2015 10:08 PM

Answers

  • User-1611549905 posted

    @AidyF: They aren't actually the same. The wrapper is calling .Include().

    Calls to .Include() are a bit of an interesting one here. I've been thinking a bit about this over the weekend and I came to the conclusion that you are right in saying that they are a data access concern, the reason being that if you get them wrong it's not possible (or at least, unduly difficult) to write a failing test without hitting the database. This is due to the fact that calling .Include() on a mock object is either a no-op (for Mock<IDbSet<T>>) or throws an exception (for Mock<DbSet<T>>), which is different behaviour in both cases from calling it against an actual database.

    Strictly speaking, they're also a business concern (and therefore an inseparable concern) because the query would be returning incorrect results without them, and your failing test would be checking a business rule (child entities should not be null). In practice, whether this is an issue or not will depend on the complexity of your queries and how much logic is involved in determining what to .Include() and what not to .Include().

    The wrapper class here will allow the parts of the query which aren't dependent on the .Include() statement to be tested without hitting the database, so it does add some value. However, as I said, the same value can be added using extension methods on IDbSet<Product>.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, February 2, 2015 5:26 AM

All replies

  • User-760709272 posted

    In your instance where you have complex data and commonly used complex queries it is ok to use a repository wrapper to abstract that.  The argument against using a repo wrapper is when you're only using it to do basic one-line operations like find from a table, list all rows from a table, insert a record etc.  Other common misuses of a repo is when your repo retuens IQuerable and is then filtered and further manipulated by the calling code.

    Wednesday, January 28, 2015 4:00 AM
  • User-1611549905 posted

    Actually, for complex queries especially, the "repository" wrapper is a horrible option, for the simple reason that it results in either methods with large numbers of parameters (a code smell) or else multiple separate methods to perform very similar tasks (another code smell). In these cases, you're better served by Query Objects. These basically apply the Replace Method with Method Object refactoring to your "Repository" wrapper so you get one class per query.

    One thing to note here is that query construction belongs in your business layer, because it is a business concern, involving the implementation of business rules. I'd also argue that calling .Include() belongs there too because performance is a business concern and needs to be implemented differently in different use cases. (If you're worrying about testability here, calling .Include() on a Mock<IDbSet> instance is a no-op, so don't worry about it.)

    Also, when people create a "Repository" wrapper round DbContext, what they are creating isn't actually a Repository. The defining feature of the Repository pattern is that it provides you with a collection-like interface to your entities. DbSet<T> does that, therefore DbSet<T> is your actual Repository. Your data access layer is Entity Framework itself. What the "Repository" facade actually does is split your business layer into two sub-layers. The lower layer is best described as a querying layer. The upper layer is more often than not a completely superfluous anaemic layer that serves no purpose whatsoever.

    Wednesday, January 28, 2015 4:53 AM
  • User-760709272 posted

    it results in either methods with large numbers of parameters

    Not necessarily, I mean complex in that there are many tables that relate to each other, it might still take only a single param like an id.  In the event that you have a lot of wheres then it only results in a lot of params if you code it badly.

    or else multiple separate methods to perform very similar tasks

    Only if you code it badly.

    One thing to note here is that query construction belongs in your business layer, because it is a business concern

    If the query is directly related to your data store then it isn't a business concern, it is a data concern and should not be in your business code.

    I'd also argue that calling .Include() belongs there too

    No, that couples your business code to your data store.

     

    The defining feature of the Repository pattern is that it provides you with a collection-like interface to your entities.

    No,

    As usual your arguments against repositories are actually arguments against badly implemented ones, so just don't implement it badly.  At least you didn't mention "n+1" queries this time.

    Wednesday, January 28, 2015 5:47 AM
  • User-1611549905 posted

    AidyF

    jammycakes

    it results in either methods with large numbers of parameters

    Not necessarily, I mean complex in that there are many tables that relate to each other, it might still take only a single param like an id.  In the event that you have a lot of wheres then it only results in a lot of params if you code it badly.

    In that case, I'd opt for Ayende's approach of extension methods on IQueryable. But if there are more than a couple of parameters, a query object is the way to go.

    AidyF

    jammycakes

    One thing to note here is that query construction belongs in your business layer, because it is a business concern

    If the query is directly related to your data store then it isn't a business concern, it is a data concern and should not be in your business code.

    No. If the query changes in response to changing business requirements, then it is a business concern. If you need to write unit tests against it to validate that it is implementing business requirements correctly, then it is a business concern.

    It may also be a data access concern, if, for example, you need to bypass Entity Framework for performance reasons, or if you need to optimise things. But this is where the concept of separation of business layer and data access layer breaks down. Some queries simply can't be categorised neatly as one or the other, and trying to do so just complicates things unnecessarily.

    AidyF

    jammycakes

    I'd also argue that calling .Include() belongs there too

    No, that couples your business code to your data store.

    See my last comment above. Different business requirements will have different needs here. Therefore, it is a business concern.

    In what way specifically does it couple your business code to your data store anyway, and exactly what problem does it cause?

    AidyF

    jammycakes

    The defining feature of the Repository pattern is that it provides you with a collection-like interface to your entities.

    No,

    Could you elaborate please? I'm quoting almost verbatim from Patterns of Enterprise Application Architecture here. If I've got my understanding of the Repository pattern wrong, could you please explain why, citing your sources?

    AidyF

    As usual your arguments against repositories are actually arguments against badly implemented ones, so just don't implement it badly.  At least you didn't mention "n+1" queries this time.

    Do you have an example of a well-implemented "repository" that you can show me?

    Wednesday, January 28, 2015 6:28 AM
  • User1871598166 posted

    Thanks for the input guys.

    I wanted to show a little bit of what I meant initially and how I have been creating my project's structure. My Repository "wrapper" look something like this:

    public class ProductRepository
    {
      public IEnumerable<Product> Select()
      {
        return context.Products
          .Include(x => x.Category)
          .Include(x => x.SomeCollection.Select(p => p.SomeProperty));
      }
    
      public Product GetById(int id)
      {
        return Select().FirstOrDefault(x => x.Id == id);
      }
    
      // Other operations
    }

    Now on my service class I receive the IEnumerable (which is an IQueryable) and build the query there:

    public class ProductService
    {
      public IEnumerable<Product> Search()
      {
        return repository.Select()
          .Where(x => x.Category.Id == something);
          // and so on
      }
    }


    So, my service expect an IEnumerable that will be usable for its operations. And to implement a method that returns an IEnumerable that meets that criteria is the responsability of the Repository. Off course, I'm considering that all the time that child objects will be necessary. I believe I might need to refactor that implementation at some point if I had performance issues. But I never build anything so heavy that needed that refactor.

    Do you think it is worth to start up from a refactored version? Maybe something more flexible?

    I never really studied NHibernate, but I've worked with a couple of projects using it and somehow they configured it to eager load child objects automatically. On these projects, we used NHibernate's context without the wrapper.

    I'm going to take a look at the query objects way of doing it.

    What you think of the aforementioned sample code? Do you think justifiable to use the "wrapper" in this situation?

    Ps.: I should point out that I also use the repository "wrapper" for some specific actions related to the disconnected scenarios, like when inserting a new product and I need to mark that category as "unchanged" so that entity framework doesn't try to insert a new category along with the product.

    Thanks for helping!

    Friday, January 30, 2015 8:01 PM
  • User-1611549905 posted

    The main problem I can see here that you'll have to look out for is that different use cases will have different requirements as far as calls to .Include() are concerned. For example, you may not need to display categories, in which case, calling .Include() here would result in superfluous data being returned.

    You can do this using a Repository Facade here, though as an alternative you may want to consider implementing it as a set of extension methods on DbSet<Product> instead. This should be a very simple refactoring as the Repository Facade normally maintains no state other than your DbContext itself.

    The big problem with the Repository Facade is that it is almost always used inappropriately. In particular, people try to use it to introduce a clean separation of concerns between the business layer and the DAL where no such separation of concerns is possible. This results in either data access concerns being miscategorised as exclusively business concerns, resulting in poor performance, or business concerns being miscategorised as exclusively data access concerns, resulting in unnecessary complexity, poor testability, and an anaemic business layer.

    As a general rule, the best way to tell whether code is a business concern or a data access concern is by looking at it through the lens of test-driven development. If you were to find a bug in a particular line of code, the first thing you would do under TDD would be to write a failing test. If such a failing test would hit the database, then you are dealing with a data access concern; if it would verify the correctness of a business rule, you are dealing with a business concern. If it would do both, you are dealing with an inseparable concern. Most if not all performance optimisations will introduce inseparable concerns.

    Monday, February 2, 2015 3:13 AM
  • User-760709272 posted

    What you think of the aforementioned sample code? Do you think justifiable to use the "wrapper" in this situation?

    If your code was all of that type then you're not getting much value from your wrapper.  What is the difference between

    public class ProductService
    {
      public IEnumerable<Product> Search()
      {
        return repository.Select()
          .Where(x => x.Category.Id == something);
          // and so on
      }
    }
    
    // and
    
    public class ProductService
    {
      public IEnumerable<Product> Search()
      {
        return DbContext.TableName
          .Where(x => x.Category.Id == something);
          // and so on
      }
    }

    Look kinda the same, so your wrapper isn't actually doing anything, just passing through to EF.

    Monday, February 2, 2015 4:14 AM
  • User-1611549905 posted

    @AidyF: They aren't actually the same. The wrapper is calling .Include().

    Calls to .Include() are a bit of an interesting one here. I've been thinking a bit about this over the weekend and I came to the conclusion that you are right in saying that they are a data access concern, the reason being that if you get them wrong it's not possible (or at least, unduly difficult) to write a failing test without hitting the database. This is due to the fact that calling .Include() on a mock object is either a no-op (for Mock<IDbSet<T>>) or throws an exception (for Mock<DbSet<T>>), which is different behaviour in both cases from calling it against an actual database.

    Strictly speaking, they're also a business concern (and therefore an inseparable concern) because the query would be returning incorrect results without them, and your failing test would be checking a business rule (child entities should not be null). In practice, whether this is an issue or not will depend on the complexity of your queries and how much logic is involved in determining what to .Include() and what not to .Include().

    The wrapper class here will allow the parts of the query which aren't dependent on the .Include() statement to be tested without hitting the database, so it does add some value. However, as I said, the same value can be added using extension methods on IDbSet<Product>.

    • Marked as answer by Anonymous Thursday, October 7, 2021 12:00 AM
    Monday, February 2, 2015 5:26 AM
  • User-760709272 posted

    Sorry, I missed the Include lines, so I retract my post above :)

    Monday, February 2, 2015 6:24 AM
  • User1871598166 posted

    The wrapper class here will allow the parts of the query which aren't dependent on the .Include() statement to be tested without hitting the database, so it does add some value. However, as I said, the same value can be added using extension methods on IDbSet<Product>.

    That's exactly why I do it this way.

    When using TDD it makes possible for me to test without an actual database. The ".Include()" is something very specific to Entity Framework. As I said I've worked in projects with NHibernate configured to load child entities automatically (not lazy loading). So with NHibernate working this way we didn't use a wrapper. Although in this case we needed to be careful to allow tests that didn't depend on NHibernate.

    I also want to point out that this structure has been like a starting point for my projects. As they grow I imagine I'm gonna need to refactor this. Because obviously it won't be necessary to load child entities for every operation. But I think the wrapper suits a starting point for small and maybe medium applications very well.

    And then, as jammycakes pointed out:

    Most if not all performance optimisations will introduce inseparable concerns.

    Even though I never got to this point before, I can see that happening.

    Monday, February 2, 2015 8:16 AM