Comments

4 Common Mistakes with the Repository Pattern

As part of my new ASP.NET Core course, I’ve been encouraging my students to post their code to GitHub. While reviewing various solutions, I’ve encountered a few common mistakes in the implementation of the repository pattern.

One repository per domain

You should think of a repository as a collection of domain objects in memory. If you’re building an application called Vega, you shouldn’t have a repository like the following:

public class VegaRepository 
{
}

Instead, you should have a separate repository per domain class, like OrderRepository, ShippingRepository and ProductRepository.

Repositories that return view models/DTOs

Once again, a repository is like a collection of domain objects. So it should not return view models/DTOs or anything that is not a domain object. I’ve seen many students using AutoMapper inside their repository methods:

public IEnumerable<OrderViewModel> GetOrders() 
{
     var orders = context.Orders.ToList();

     return mapper.Map<List<Order>, List<OrderViewModel>(orders);
}

Mapping is not the responsibility of the repository. It’s the responsibility of your controllers. Your repositories should return domain objects and the client of the repository can decide if it needs to do the mapping. By mapping the domain objects to view models (or something else) inside a repository, you prevent the client of your repositories from getting access to the underlying domain object. What if you return OrderViewModel but somewhere else you need OrderDetailsViewModel or OrderSnapshotViewModel? So, the client of the repository should decide what it wants to map the Order object to.

Save/Update method in repositories

Yet another very common mistake! As I’ve explained in my YouTube video before, your repositories should not have a Save() or Update() method. I repeat: think of a repository as a collection of domain objects in memory. Do collections have a Save() or Update() method? No! Here’s an example:

var list = new List<int>();
list.Add(1);
list.Remove(1);
list.Find(1);

list.Save();    // doesn't exist!
list.Update();  // doesn't exist!

Another reason your repositories should not have a Save() method is because sometimes as part of a transaction you may work with multiple repositories. And then you want to persist the changes across multiple repositories in one transaction. Here’s an example:

orderRepository.Add(order);
orderRepository.Save();

shippingRepository.Add(shipping);
shippingRepository.Save();

Can you see the problem in this code? For each change, we need a separate call to the Save() method on the corresponding repository. What if one of these calls to the Save() method fails? You’ll end up with a database in an inconsistent state. Yes, we can wrap that whole thing inside a transaction to make it even more ugly!

A pattern that goes hand in hand with the repository pattern is the unit of work. With the unit of work, we can re-write that ugly code like this:

orderRepository.Add(order);
shippingRepository.Add(shipping);
unitOfWork.Complete();

Now, either both objects are saved together or none are saved. The database will always be in a consistent state. No need to wrap this block inside a transaction. No need for two separate calls to the Save() method!

If you want to learn how to implement the repository and unit of work pattern together, watch my YouTube video here.

Repositories that return IQueryable

One of the reasons we use the repository pattern is to encapsulate fat queries. These queries make it hard to read, understand and test actions in ASP.NET MVC controllers. Also, as your application grows, the chances of you repeating a fat query in multiple places increases. With the repository pattern, we encapsulate these queries inside repository classes. The result is slimmer, cleaner, more maintainable and easier-to-test actions. Consider this example:

var orders = context.Orders
    .Include(o => o.Details)
        .ThenInclude(d => d.Product)
    .Where(o => o.CustomerId == 1234);

Here we are directly using a DbContext without the repository pattern. When your repository methods return IQueryable, someone else is going to get that IQueryable and compose a query on top of it. Here’s the result:

var orders = repository.GetOrders()
    .Include(o => o.Details)
        .ThenInclude(d => d.Product)
    .Where(o => o.CustomerId == 1234);

Can you see the difference between these two code snippets? The only difference is in the first line. In the first example, we use context.Orders, in the second we use repository.GetOrders(). So, what problem is this repository solving? Nothing!

Your repositories should return domain objects. So, the GetOrders() method should return an IEnumerable. With this, the second example can be re-written as:

var orders = repository.GetOrders(1234);

See the difference?

What are the other issues you’ve seen in the implementation of the repository pattern? Share your thoughts!

If you enjoyed this article, please share it.

Related Posts

Tags: ,

11 responses to “4 Common Mistakes with the Repository Pattern”

  1. Dayshine says:

    “Mapping is not the responsibility of the repository. It’s the responsibility of your controllers”

    What about mapping from your repository object to your domain object?

    This is all good advice for the interface. But aren’t you glossing over the implementation part of your repository pattern? None of this is actual *implementation*.

  2. Saeid says:

    About “Repositories that return view models/DTOs”
    Suppose your domain model has 20 fields with large amount of data and you want to use only 3 fields here, you have to fetch the whole row first and then map it, which is very inefficient.

    • admin says:

      Very good point!

      What you’re referring to here is a simple data structure and not a domain object. You want this structure with 3 properties only for a specific view (or multiple views) in the application. This data structure is nothing but a data container. It’s not a domain object. It doesn’t have any methods, neither does it encapsulate any business rules.

      To expand more on your example, those 3 properties may be mapped to a subset of columns in 1 table, or they can be the result of joining N tables and picking 3 columns. In either case, it is not a domain object, and by definition, it doesn’t fit the definition of the repository pattern.

      These objects are only used for the read operations and for these I use a separate group of classes, which I call providers (eg CustomerProvider). It has a bunch of methods for querying customer data mainly for reporting. In the implementation of this provider, you can also use AsNoTracking() to improve the performance. You wouldn’t do that in your repository implementations because you need to keep track of changes to your domain objects and persist them in the database. You may even use a stored procedure for performance optimization.

      The more complex your application gets, the more you may want to consider separating the read and write models. And then eventually you’ll find in the CQRS realm.

  3. Rahul says:

    Hi Mosh,

    @’Save/Update method in repositories’

    Lets assume that we need convert existing application to its latest tech with pattern, where the request or update are done with existing SQL or Stored procedures.

    Please let us know how do i implement the Unit of work complete method (needs to be utilized existing stored procedures for update function) and if not please prove as stated ‘Repositories should not have a Save() or Update() method’

    • admin says:

      Hi Rahul,

      I recommend you to watch my YouTube video first and then read my comment below:

      https://youtu.be/rtXpYpZdOzM

      Regarding your question about implementing the unit of work for an application with existing stored procedures: you don’t really need to implement a unit of work from scratch. One complex part of this implementation is about keeping track of object changes in memory. DbContext already does that for you and you don’t need to re-implement it. You can map the CRUD operations to stored procedures in the database:

      https://msdn.microsoft.com/en-us/library/dn468673(v=vs.113).aspx

      When EF tries to generate queries for inserting, updating, deleting and getting data, it’ll call your stored procedures.

      Finally, you wrap your DbContext with an IUnitOfWork interface do decouple it from your application and simplify testing. Again, I’ve explained that in my YouTube video.

  4. Clayton says:

    Hi Mosh,

    I agree with you that the repository must return only domain entities. If you want to return ViewModel objects, it’s possible to create a Service layer that would be responsible for get domain entities using a repository and convert to ViewModels. Finally, the controller(mvc or webApi) could use these services and know nothing about the entities.

    Leave your comments.

    Have a nice day!

    • admin says:

      Hi Clayton,

      Services should not return ViewModels either! A ViewModel is part of the presentation layer which sits above the service layer. Mapping of domain objects to ViewModels is the responsibility of whoever wants to display those objects. In an ASP.NET MVC application, it’s the controller, in a WPF application it’s the Form (or Window).

      Services are often misunderstood. Technically, your services should be used only for the write operations and not for reading data. It took me a little while to really grasp this. The reason is that if a controller needs some data from the database, it can simply use the repository interfaces to get the data. There is no need to go through a service layer for getting data. With this implementation, you’ll end up with services methods that have 1 line of code, which is simply delegating to the repository. What’s the point of these methods? Nothing but increasing the development and maintenance cost. I had a lot of services like these several years ago but eventually, I came to this realization that these methods are redundant and add absolutely no value.

      Just to clarify, controller uses the repository interface (not the implementation). So it doesn’t go straight to the data access layer. Repository interfaces are part of the business layer or the core domain.

      Hope that helps!

  5. Shehab says:

    Mosh,

    If we return IEnumerable from the repository and the entity has a large dataset, would that be slower than returning IQueryable with a ‘fatter’ query inside the repository?
    I think not because the service call to the repository is filtering the records before it get called but I wanted to see what you think.

    Thanks in advance.

    • admin says:

      I’m not quite sure what you mean. A fat query can get slow when dealing with large datasets. Whether you return IQueryable from your repositories (which you shouldn’t) and then do a ToList() somewhere else, or do the ToList() inside the repository (which you should), the performance is the same.

  6. chong says:

    “Instead, you should have a separate repository per domain class, like OrderRepository, ShippingRepository and ProductRepository.”

    Hi Mosh,
    Do you mind to share your thought about Generic Repository pattern? What do you think about it?
    Thanks.

    • admin says:

      Generic repository is good as long as your methods don’t method IQueryable. Otherwise, it’s like you’re wrapping DbSet with another class with the same interface. No point in doing that!

      It’s useful when you wanna get simple objects (like reference data) without having to create a separate repository for each entity.

Leave a Reply