Does Repository Pattern follow SOLID principles?

Question

I am doing some research on SOLID principal, and found some issues in implementations of Repository pattern. I am going to explain each and every problem, Please correct me if I am wrong.

Problem 1

Repository Pattern Breaks Single responsibility principle (S)

Let say we have a interface which define as

public interface IRepository<T> where T: IEntity
{ 
    IEnumerable<T> List { get; }
    void Add(T entity);
    void Delete(T entity);
    void Update(T entity);
    T FindById(int Id);
}

Clearly it violates the single responsibility principle because when we implement this interface, In a single class we are putting Command and Query both. and this not expected.

Problem 2

Repository Pattern Breaks Interface segregation principle (I)

Say We have 2 Implementation of the above Interface.

First Implementation

CustomerRepository : IRepository<Customer>
{
   //All Implementation
}

Second Implementation

ProductRepository : IRepository<Product>
{
   //All Implementation except Delete Method. So Delete Method Will be
   void Delete (Product product){
       throw Not Implement Exception!
   }
}

And as per ISP "No client should be forced to depend on methods it does not use." So we saw that clearly it also violates the ISP.

So, My understanding is Repository pattern does not follow SOLID principal. What do you think? Why should we choice this type of pattern which violates the Principal? Need your opinion.

Answer 1

Clearly it violates the single responsibility principle because when we implement this interface, In a single class we are putting Command and Query both. and this not expected.

That's not what Single Responsibility Principle means. SRP means that the class should have one primary concern. The primary concern of a repository is to "mediate between the domain and data mapping layers using a collection-like interface for accessing domain objects" (Fowler). That's what this class does.

Repository Pattern Breaks Interface segregation principle

If that bothers you, then simply provide another interface that doesn't include the method you're not going to implement. I personally wouldn't do that, though; it's a lot of extra interfaces for marginal benefit, and it clutters the API unnecessarily. A NotImplementedException is very self-explanatory.

You're going to find that there are a lot of rules, laws or principles in computing that have exceptions, and some that are outright wrong. Embrace the ambiguity, learn to write software from a more practical perspective, and stop thinking about software design in such absolute terms.

Answer 2

I use the Repository pattern myself and I used the pattern to make sure all required interfaces are implemented. For this I created separate interfaces for all actions (IEntityCreator, IEntityReader, IEntityUpdater, IEntityRemover) and made the repostiory inherit all these interfaces. This way I can implement all methods in a concrete class and still use all interfaces separately. I don't see a reason to state that Repository pattern violates the SOLID principles. You just need to defined the 'responsibility' of the repository correctly: the responsibility of the Repository is to facilitate all access to the data of type T. That's all there is to say. As stated above I do also have a read-only repository interface named ReferenceRepository<T> which only includes the IEntityReader<T> interface. All interfaces are defined below for fast copying :) On top of that I also created a few concrete classes including the caching and/or logging. This is to incorporate any further actions required as stated by the I in SOLID. The type IEntity is used as a marker interface to allow only entities and not some other kind of object (you have to start somewhere).

/// <summary>
/// This interface defines all properties and methods common to all Entity Creators.
/// </summary>
/// <typeparam name="TEntity">The type of the entity.</typeparam>
public interface IEntityCreator<TEntity>
    where TEntity : IEntity
{
    #region Methods
    /// <summary>
    /// Create a new instance of <see cref="TEntity"/>
    /// </summary>
    /// <returns></returns>
    TEntity Create();
    #endregion
}

/// <summary>
/// This interface defines all properties and methods common to all Entity Readers.
/// </summary>
/// <typeparam name="TEntity">The type of the entity.</typeparam>
public interface IEntityReader<TEntity>
   where TEntity : IEntity
{
    #region Methods
    /// <summary>
    /// Get all entities in the data store.
    /// </summary>
    /// <returns></returns>
    IEnumerable<TEntity> GetAll();

    /// <summary>
    /// Find all entities that match the expression
    /// </summary>
    /// <param name="whereExpression">exprssion used to filter the results.</param>
    /// <returns></returns>
    IEnumerable<TEntity> Find(Expression<Func<TEntity, bool>> whereExpression);
    #endregion
}

/// <summary>
/// This interface defines all properties and methods common to all Entity Updaters. 
/// </summary>
/// <typeparam name="TEntity">The type of the entity.</typeparam>
public interface IEntityUpdater<TEntity>
    where TEntity : IEntity
{
    #region Methods
    /// <summary>
    /// Save an entity in the data store
    /// </summary>
    /// <param name="entity">The entity to save</param>
    void Save(TEntity entity);
    #endregion
}

/// <summary>
/// This interface defines all properties and methods common to all Entity removers.
/// </summary>
/// <typeparam name="TEntity">The type of the entity.</typeparam>
public interface IEntityRemover<TEntity>
    where TEntity : IEntity
{
    /// <summary>
    /// Delete an entity from the data store.
    /// </summary>
    /// <param name="entity">The entity to delete</param>
    void Delete(TEntity entity);

    /// <summary>
    /// Deletes all entities that match the specified where expression.
    /// </summary>
    /// <param name="whereExpression">The where expression.</param>
    void Delete(Expression<Func<TEntity, bool>> whereExpression);
}

/// <summary>
/// This interface defines all properties and methods common to all Repositories.
/// </summary>
public interface IRepository { }

/// <summary>
/// This interface defines all properties and methods common to all Read-Only repositories.
/// </summary>
/// <typeparam name="TEntity">The type of the entity.</typeparam>
public interface IReferenceRepository<TEntity> : IRepository, IEntityReader<TEntity>
   where TEntity : IEntity
{

}

/// <summary>
/// This interface defines all properties and methods common to all Read-Write Repositories.
/// </summary>
public interface IRepository<TEntity> : IReferenceRepository<TEntity>, IEntityCreator<TEntity>,
    IEntityUpdater<TEntity>, IEntityRemover<TEntity>
    where TEntity : IEntity
{

}

Answer 3

Clearly it violates the single responsibility principle

It's only clear if you have a very narrow definition of what the SRP is. The fact is SOLID violates SOLID. The principles themselves contradict themselves. SRP is at odds with DRY, since you often have to repeat yourself to properly separate concerns. LSP is at odds with ISP in some situations. OCP often conflicts with DRY and SRP. These principles are here not as hard and fast rules, but to guide you... try to adhere to them, but don't treat them as laws that cannot be broken.

On top of that, you are confusing the Repository architecture pattern, with a very specific Generic Repository implementation pattern. Note that a generic repository is different from a concrete repository. Nor is there any requirement that a Repository implement the methods you mention.

Yes, you can separate command and query as two separate concerns, but there is no requirement that you do so to make each a single responsibility. Command Query Seperation is a nice principle but not something that is covered by SOLID, and certainly there is no consensus on whether or not separating the concerns falls under the prevue of different responsibilities. They're more like different aspects of the same responsibility. You could take this to a ridiculous level if you wanted to and claim that Updating is a different responsibility from Deleting or that Querying by id is a different responsibility from querying by type or whatever. At some point you have to draw lines and box things in, and for most people "reading and writing an entity" is a single responsibility.

Repository Pattern Breaks Interface segregation principle

First, you are confusing Liskov Substitution Principal with Interface Segregation Principle. LSP is what is violated by your example.

As I said earlier, there is no requirement that Repository implement any specifc set of methods, other than a "collection-like interface". In fact, it would be perfectly acceptable to implement it like this:

public interface IRepository<T> where...[...] {IEnumerable<T> List { get; }}
public interface CustRepository : IRepository<Customer>, IRepoAdd, IRepoUpdate, IRepoDelete, IRepoFind {}

Now it can optionally implement any of the other members without breaking LSP, although it's a rather silly implementation and one I certainly wouldn't implement just to avoid breaking LSP.

Fact is, there is probably no good reason why you would want a repository without delete. The only possible reason I can think of would be a Read-Only Repository, which I would define a separate interface for using a read-only collection interface.

Answer 4

I think it does break ISP. It just does.

Perhaps it's such an established pattern that people find it hard to come to terms with.

https://www.newyorker.com/magazine/2017/02/27/why-facts-dont-change-our-minds

The interface-segregation principle (ISP) states that no client should be forced to depend on methods it does not use.[1] ISP splits interfaces that are very large into smaller and more specific ones so that clients will only have to know about the methods that are of interest to them.

I am implementing an API resource to get an order. With a typical repository I am forced to depend on a giant repository that can update and delete stuff.

I'd rather just depend on, and mock or fake, a type that just gets orders.

Answer 5

I know this is an old post just wanted to offer my 2 cents. If you want to follow solid better you'd need to seperate the interface into separate versions. One for read, one for edit, delete etc for interface segregation. But ideally I wouldnt use the repository pattern and I'd rather create a repository for each entity or purpose, with it's own interface.

Answer 6

Yes this is old, but I think there is one part that is still unclear. I agree that the proposed Repository Pattern breaks SRP clearly, but it depends on the definition of SRP.

The definition that something should only have a "single responsibility" is vague, because in that case you can always argue that something has only one responsibility. Yeah sure a repository only mediates between your application and the database. But one level deeper it is responsible for read, write, update, delete...

The last CLI that I implemented also had just one responsibility... to handle communication with some API... but one level deeper... you get the point

I prefer the definition of Robert C. Martin, which states that SRP means: "There is only one reason to change." This is in my opinion more precise. The repository can change if the write/update changes (auditing), if the read changes (caching) or if the delete changes (challenge before actual delete) and so on.

Following that, the proposed answers with single interfaces for every CRUD operation, would follow SRP and also follow ISP, because the two are basically related to one another.