Generic Entity Framework Query in CQRS

Question

I've been working on our application (CQRS and DDD) for awhile now. The application architecture is well layered and thought through.

However we are having difficulties in decide where to put the below code, in such a way that it makes sense from an architectural point of view.

What the code does: It's a generic method which executes a query against the dataset, specified by the generic parameter which inherits a particular interface. Note: we are not using repositories, since the query is, as the name indicates, for our read layer.

The interface, used by the generic method:

public interface IOrganisationalDbEntry
{
    string OrganisationLevelId { get; set; }
    OrganisationLevelDbEntry OrganisationLevel { get; set; }
}

The generic method, which lives inside our Entity Framework DbContext:

public IList<TDbEntry> GetByOrganisationLevel<TDbEntry>(Guid? organisationLevelId,
    Func<TDbEntry, bool> filter = null)
    where TDbEntry : class, IOrganisationalDbEntry
{
    var organisationLevel = organisationLevelId.HasValue ? organisationLevelId.Value.ToString() : null;

    var query = filter == null ? Set<TDbEntry>() : Set<TDbEntry>().Where(filter);

    IList<TDbEntry> result = query.Where(x => x.OrganisationLevelId == organisationLevel).ToList();

    // If there are no results for the current organisation level
    // => recursive check for entries for parent organisation level
    if (!result.Any() && !string.IsNullOrWhiteSpace(organisationLevel))
    {
        // Get parentOrganisationLevel
        var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
            .Where(x => x.Id == organisationLevel)
            .Select(x => x.ParentOrganisationLevelId)
            .SingleOrDefault();

        return
            GetByOrganisationLevel<TDbEntry>(
                string.IsNullOrWhiteSpace(parentOrganisationLevelId)
                    ? (Guid?)null
                    : new Guid(parentOrganisationLevelId), filter);
    }

    return result;
}

As you may be thinking, we have several EF Entities inheriting the IOrganisationalDbEntry. What comes to mind first, is a base Repository. But We do not want to use repositories in our query side, since we want this layer to be as flat as possible.

The next thing that comes to mind is a specific query and queryhandler for this query. Tho this is not acceptable since this method accepts a Func parameter to be able to add extra filter criteria while calling this method, to make it more flexible.

At this moment, this method is living as method on our IDbContext and DbContext implementation. However we are not satisfied with this. We are wondering whether there is a specific pattern or approach for our case. Imagine we have more of these methods, I do not want to keep expanding our context. Since this is not what the DbContext should be used for.

EDIT: Everywhere you read CQRS in the above question , replace it by CQS. I didn't want to modify the question since it would make some comments irrelavant.

Answer 1

I'll give it a shot since CQRS is something I'm still learning.

In CQRS we are supposed to separate our read model from our write model. Is this what you have done?

Then the read model should be designed to be a perfect fit for the thing that needs to read it:

• Your query looks very complex. Is it derived from the write model, or do you indeed have separate tables for the queries?
• Why a Func as a parameter? It seems strange to me that the criteria should be a negotiation between client and query.
• Why do you put the query method onto the DbContext? That class will soon enough be cluttered with too many queries.

Here are some suggestions to simplify things:

• Let the DbContext mimic the database tables only and nothing else. That means your DbContext will only have public properties for the tables you are going to query.
• Don't call it a Repository, but a Query. Split the Query-class when it grows too big.
• Let the Query class take the DbContext as a dependency. Query the DbContext-properties, don't use Set<T>.
• Let each query-method accept one criteria object as parameter, then build the query expression from that inside the method.
• Linq is tricky. While not entirely true, a rule of thumb is that a Func runs client side, while an Expression runs server side. That's also why I don't like the Func-parameter.
• Consider using only non-generic query-methods. Yes, there will be some duplication, but you want your peers and the future you to understand the queries two months from now.

I meant to post some code examples, but Chrome crashed again and edits got lost :-(

Answer 2

var organisationLevel = organisationLevelId.HasValue ? organisationLevelId.Value.ToString() : null;

var query = filter == null ? Set<TDbEntry>() : Set<TDbEntry>().Where(filter);

IList<TDbEntry> result = query.Where(x => x.OrganisationLevelId == organisationLevel).ToList();

Using C#6, you could change the first line to :

var organisationLevel = organisationLevelId?.Value.ToString();

The ?. operator will return null instead of throwing a NullReferenceException if !organisationLevelId.HasValue.

The two following lines could be changed to this :

var query = Set<TDbEntry>().Where(x => x.OrganisationLevelId == organisationLevel);

if(filter != null)
    query = query.Where(filter);

You could argue it's a useless change, but I think it's much clearer without the ? operator.

Since your result is casted to a IList<>, you should use .Count > 0 instead of !.Any() here :

if (!result.Any() && !string.IsNullOrWhiteSpace(organisationLevel))

It's more performant because Count is a pre-computed value, whereas Any() needs to call the iterator and check if there's a value.

This :

    var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
        .Where(x => x.Id == organisationLevel)
        .Select(x => x.ParentOrganisationLevelId)
        .SingleOrDefault();

Could be changed to :

    var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
        .Select(x => x.ParentOrganisationLevelId)
        .SingleOrDefault(x => x.Id == organisationLevel);

I'm not sure there's an impact on the SQL generated query (you should check with the profiler), but it removes a line of code and is in my opinion better!

Btw, you shouldn't use var here. I can't figure what's the type of parentOrganisationLevelId because of it. The guideline is to use var only when you can figure the type by reading the code, which isn't the case here.

return
    GetByOrganisationLevel<TDbEntry>(
        string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId), filter);

That is kinda disgusting. First, the indentation (I'm guessing Resharper placed it like that? It always does this to me) isn't cool. I understand that the ternary operator (?) is good to be split on multiple line, it's clearer. But I really think the Guid? definition should be moved to a variable declaration for readability.

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId)

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

Btw, think about using default(Guid?) instead of (Guid?)null. It's just weird!

Last comment, to reduce nesting you might want to revert your condition :

if (result.Any() || string.IsNullOrWhiteSpace(organisationLevel))
{
    return result;
}

// Get parentOrganisationLevel
var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
    .Select(x => x.ParentOrganisationLevelId)
    .SingleOrDefault(x => x.Id == organisationLevel);

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId);

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

You're using String.IsNullOrEmpty for nothing, since organisationLevel is defined by the Guid?.Value. The value will always either be a Guid.ToString() or null. So just checking for null would be good.

These are all readability issues. I really think your code is more readable this way :

var organisationLevel = organisationLevelId?.Value.ToString();
var query = Set<TDbEntry>().Where(x => x.OrganisationLevelId == organisationLevel);

if (filter != null)
    query = query.Where(filter);

var result = query.ToList();

if(result.Count > 0 || organisationLevel == null)
{
    return result;
}

var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
    .Select(x => x.ParentOrganisationLevelId)
    .SingleOrDefault(x => x.Id == organisationLevel);

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? default(Guid?)
            : new Guid(parentOrganisationLevelId);

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

I stripped the comments because they aren't clear. You should explain why you do the recursive check with the parent...Id instead or stating you do a recursive call. We can see that by ourselves! :)

Finally, does string.IsNullOrWhiteSpace(parentOrganisationLevelId) implies you could have whitespaced IDs in your database??!?!? If so you have so many problems bigger than.. every other problems you might have. That'd be the biggest problem ever.

PS : I already wrote a comment on your question, but you should return IEnumerable<> instead of IList<>!

Warning!! (lol)

The following is rated M for mature isn't about code review but about OP's question which is where this code should belong.

I realize there was a question hidden under all that text :p You're wondering where to place this method. Let's see our options :

• The repository/DataContext : Bad plan, it's too specific to be in a repository in my opinion.
• Your application services/Controller : Bad plan, it's too data-oriented.
• Some random class : That sucks, period.

What you need, I believe, is a QueryService (the name could be changed, that's what I'll use to talk). The query service is somewhere between your application services (or controller, depending on the complexity of your application) and your data access layer. This service will hold all specific queries that are used by the application services (or to show data to a GUI).

I can't draw here, but in CQRS (or CQS, doesn't matter in that case), you have a separation between the read and write.

When you write in your application, the process goes something like this :

GUI -> Application Services -> Domain -> DAL

Now, a common mistake is to mix the Read path with the Write path. What is just above is made for multiple purposes : Catching user intent, validating it through the domain and saving it in a DB. This process is captured in the 4 layers I wrote above.

When you read, none of the purposes I wrote above apply. You usually want to do A : Validation, which requires a database read or B : Expose data to your user via a GUI.

What does that mean? I means a different path of course!

GUI -> Query(?) Services -> DAL

The query service would usually return Read Models, or primitive types.

Hence, I believe you should have read services.

Application service definition : If your application has little complexity, this is probably mixed in your controller, otherwise you probably have classes that take care of validating/parsing/saving/etc. input from the GUI, that's my definition of an application service!

Answer 3

I agree with your statement that creating a new query and query handler for this is not acceptable. But it's not because of the Func parameter. It's because you've already created a query, or, to be more specific, you've used generics to create a--wait for it--generic query.

Wild guess here, but I'm thinking your architecture probably has interfaces like IQuery and IQueryHandler in it, so that all of your concepts like "query" and "query handler" follow a specific type structure. Also, all queries must be handled by a corresponding query handler. If this is the case, it's understandable why this object is incompatible with your existing "queries." It is okay for something to be conceptually a query while not being constrained to type TQuery.

If that is not the case, I still get the impression that there is some general (possibly dogma-inspired) sticking point where it "doesn't fit."

My recommendations:

1. Turn your method into a generic class where the type parameter is defined on the object, not a method. Your naming can stay the same; it is totally acceptable for a query object to be named that way.

2. Like Thomas Eyde suggests, pass the context as a dependency. It's going to be all right.

Either:

3a. Pass the Func into the constructor, enforcing the idea that every query has its own filter.

3b. (I recommend this) Completely remove the Func filter but defer the execution of the query by returning an IQueryable instead of an IList<T>. You can then chain the filter on instead of having to pass an expression or delegate to a method (usually a code smell if you aren't writing functional code or dealing with events).

I think the IQueryable is the way to go because it allows you to keep the filtering out at the consumer level and doesn't eagerly pull all the records back.

Notes on the recursion:

• You're essentially performing a recursive self join on a table. Why not simplify everything by using a stored procedure with a CTE? Obviously there are some EF considerations here, but it could be a worthwhile endeavor.
• If you take the IQueryable approach obviously you will have to refactor the code to remove the subsequent lookups. Naturally if you do this you will have considerations around how to create an object that can be called generically for everything that derives from your interface. However, if that becomes a grand undertaking, it's not a limitation of EF, or the framework, it's a complication of a design issue. Consider what you have going on currently: a recursive call that makes a round trip every iteration. Usually this is a sign that things can be done better.

Things to avoid getting hung up on:

• You can directly instantiate this class. It and all its dependencies don't have to be injected.
• Don't require a handler for this query. You want to keep the layer as flat as possible, right?
• Unit testing can be achieved via the LINQ to Objects provider which doesn't necessarily always mirror the EF provider. Okay, that's something to watch out for, but how many tests do you have around this already?

In scenarios where code has been written and there's a question of "now where does it go," it's usually a good idea to question if the overall mindset is "don't break rules" or "try to align with the spirit of the pattern." It's only when you fully invest in the latter that you find the freedom to try the techniques that will eventually lead to a good solution.