Allowing query handlers to use other query handlers

Question

I recently started looking into CQRS when using Entity Framework and the impact it has had on my systems has been overwhelming.

I implemented the following patterns described in these blog posts:

• Query implementation
• Command implementation

I have implemented these with Autofac as dependency injection, and Entity Framework with code first.

Now for the question. I have decided to let my query handlers use each other, and build upon other query handlers results. Is that considered okay? What issues may I run into in the future (if any)? Is the code SOLID?

Here's an example of my GetEmployeesQueryHandler:

class GetEmployeesQueryHandler : IQueryHandler<GetEmployeesQuery, IQueryable<Employee>>
{
    private readonly DatabaseContext database;

    public GetEmployeesQueryHandler(DatabaseContext database)
    {
        this.database = database;
    }

    public IQueryable<Employee> Handle(GetEmployeesQuery query)
    {
        return database.Employees;
    }
}

My Entity Framework DatabaseContext gets injected into the constructor, and I simply return all employees.

Then I have a more specific query, which bases itself on this query. It does that by first fetching all employees, and then filtering them based on the Id of the employee.

class GetEmployeeByIdQueryHandler : IQueryHandler<GetEmployeeByIdQuery, Employee>
{
    private readonly IQueryProcessor processor;

    public GetEmployeeByIdQueryHandler(IQueryProcessor container)
    {
        this.processor = container;
    }

    public Employee Handle(GetEmployeeByIdQuery query)
    {
        var employees = processor.Process(new GetEmployeesQuery());
        return employees.SingleOrDefault(e => e.Id == query.EmployeeId);
    }
}

And that's it. What do you think? Is it okay for my GetEmployeesByIdQueryHandler to use the GetEmployeesQueryHandler to get all employees first, for better code reuse?

Note that I return an IQueryable<Employee> when I fetch all employees, so the SQL query will still be lazily generated, and evaluated in an optimized way.

For those who want a TL;DR and don't want to read the blog posts, below are the interfaces and the rest of the classes backing up my code.

---

Implementations

GetEmployeesQuery

public class GetEmployeesQuery : IQuery<IQueryable<Employee>>
{
}

---

GetEmployeeByIdQuery

public class GetEmployeeByIdQuery : IQuery<Employee>
{
    public Guid EmployeeId { get; set; }
}

---

QueryProcessor

/// <summary>
/// Represents a class which automatically instantiates a QueryHandler that corresponds to a given Query, and processes the result of running the Query on that QueryHandler.
/// </summary>
sealed class QueryProcessor : IQueryProcessor
{
    private readonly IComponentContext context;

    public QueryProcessor(IComponentContext context)
    {
        this.context = context;
    }
    
    /// <summary>
    /// Automatically figures out which QueryHandler belongs to the given Query, instantiates it, and returns the result of running the Query on that QueryHandler.
    /// </summary>
    /// <typeparam name="TResult">The type of result to return. This can be infered from Query given.</typeparam>
    /// <param name="query">The query to return to use as basis for finding a suitable QueryHandler and returning the result.</param>
    /// <returns></returns>
    public TResult Process<TResult>(IQuery<TResult> query)
    {
        var handlerType = typeof(IQueryHandler<,>).MakeGenericType(query.GetType(), typeof(TResult));
        dynamic handler = context.Resolve(handlerType);

        return handler.Handle((dynamic)query);
    }
}

---

Interfaces

IQueryHandler

/// <summary>
/// Defines a test-friendly query handler that can be automatically instantiated and populated with dependency injection.
/// </summary>
/// <typeparam name="TQuery">The type of query that this query handler should handle. For example, GetEmployeeByIdQuery.</typeparam>
/// <typeparam name="TResult">The type of result that the query returns. Must be the same as defined in the query itself. For example, Employee.</typeparam>
public interface IQueryHandler<TQuery, TResult> where TQuery : IQuery<TResult>
{
    TResult Handle(TQuery query);
}

---

IQueryProcessor

public interface IQueryProcessor
{
    TResult Process<TResult>(IQuery<TResult> query);
}

---

IQuery

public interface IQuery<TResult>
{
}

Answer 1

I have decided to let my query handlers use each other, and build upon other query handlers results. Is that considered okay? What issues may I run into in the future (if any)? Is the code SOLID?

You are letting components be built up by other components. This is completely SOLID and is a good approach.

What I would like to warn about is returning IQueryable<T>. This is fine as long as the query handler is used by other query handlers (since this enables composible queries with good performance), don't return an IQueryable<T> to the presentation layer. This makes the system unreliable and hard to test. This means that you will have to implement sorting and paging inside your business layer, but this is actually quite easy.

Note that your components should not dispose any dependencies that are injected into him. The reason is that such component doesn't own the dependency and has no idea what the lifetime of the dependency is. Disposing that dependency can make the application break, and this is something you already noted in the comments.

As a matter of fact, you are violating SOLID by injecting a dependency that implements IDisposable; you are violating the Dependency Inversion Principle. This principle states that "abstractions should not depend on details", but IDisposable is an implementation detail. If you prevent having IDisposable on the abstraction, but instead place it on the implementation, you'll see that it becomes impossible for the consumer to call Dispose() (which is good, because it has no idea whether or not it should call it), and now only the one who created that dependency can dispose that dependency (which is your composition root). This makes your application code much simpler, because you will hardly ever need to implement IDisposable at all.

In your case however, you can't remove IDisposable from the DatabaseContext, because you inherit from DbContext. But injecting a DbContext is itself a DIP violation. Although not all DIP violations are bad (and you will always have DIP violations somewhere in your application), I rather hide the DbContext from my code, for instance by using an IUnitOfWork abstraction with a single IQueryable<T> Set<T>() method. Advantage of this is that the DbContext can be resolved at runtime, instead of injected into consumers, and it allows to easily wrap the IUnitOfWork with some sort of security decorator that filters the results based on the user's rights.

Do note that the part of the system that created a disposable component typically holds the ownership, and is therefore responsible of disposing that. Although this ownership can be transferred, you should typically not do this, because this complicates your application (I think you already noticed this). So you composition root creates this dependency and should dispose it. In case you use a DI library, the library will create that insance for you. In that case, the library is also responsible of disposing it for you. Although you can view this as 'something magical', IMO it's simply a basic feature of the library you are using. You should understand the libraries you are use. In the case of Autofac, you can be pretty sure that Autofac handles this correctly for you. In case you would switch back to Pure DI (formally known as poor man's DI), your code will obviously again be in control over that dependency, and you will have to implement this disposal again manually. There's no design smell here. But that said, although disposing is not the problem, scoping might actually be. Please read this answer of mine.

Although the query handler pattern might seem over-engineered at first, if you read the article closely, you'll see that it is simply an implementation of the SOLID principles. IMO, you should always strive to adhere to the SOLID principles in the core parts of your application. Querying is obviously a core part of every application. My experience is that query handlers even work well on small projects. They allow adding cross-cutting concerns with such easy, that it can really boost the productivity and flexibility in smaller applications.

Using IQueryProcessor instead of injecting IQueryHandlers directly has some downsides, such as the fact that handlers get resolved lazily at runtime. This makes it harder to verify the complete object graph and it makes it hard to see what queries a component executes. On the other hand, it makes your code cleaner, because the generic types and often long names for query classes can give some noise in your code (that's more a limitation in C# than a limitation of the pattern). In that case the IQueryProcessor can help.

Since your IQueryProcessor implementation will be part of your composition root, it is completely fine to have a dependency on the container, since the rest of your composition will have a dependency on the container as well (although it would be good to call it AutofacQueryProcessor). It is incorrect to assume that this is a implementation the Service Locator anti-pattern. This is clearly explained by Mark Seemann here. In case you are swapping your DI library, you will have to change your complete composition root, including the IQueryProcessor implementation (because it is part of the composition root). There's nothing wrong with that.

It is no problem that your container is stored inside your graph. Although you could try making the process generic by injecting a Func, that would still mean that the injected Func would depend on the container, making the container still part of the object graph. This is actually what dependency inversion is all about. Components can use other components and code at runtime that they don't have a compile time dependency on.

Answer 2

I have the complete opposite view. Every query is specific to its use case. Today it may be true that you can call the other query but if someone changes its output then your query could be impacted. Maybe the other query gets a permission added to it that yours doesn't have. It is far better to simply build and return exactly what you need and no more. You may be thinking that you aren't reusing any code this way and you would be right. If two UI had the same data on it why would you have two?