Encapsulate common parts of Entity Framework queries

Question

I'm developing a system with some complex logic regarding user permissions to access different data from a SQL Server database using C# and Entity Framework. I have different places across the system where I need to query certain DB entities called 'documents' that are only accessible to current user. I came up with the following class that queries only the necessary documents:

internal class DocumentPermissionsHelper
{
    private readonly MyDbContext dc;
    public DocumentPermissionsHelper(MyDbContext dc) { this.dc = dc; }

    public IQueryable<Document> SelectDocumentsAccessibleToUser(IQueryable<Document> query, Guid userId)
    {
        var permissionsQuery = dc. ... // really big query with multiple JOIN's to determine which user has access to which documents 
        IQueriable<Guid> docIdsAccessibleToUserQuery = 
            from permissions in permissionsQuery.Where(_ => _.UserId == userId && _.AccessLevel == "Read")
            select permissions.DocumentId;

        return from accessibleDocId in docIdsAccessibleToUserQuery.Distinct()
                from accessibleDoc in query.Where(g => g.Id == accessibleDocId)
                select accessibleDoc;
    }
}

Then I use it in several different queries like this:

public void GetLatestDocsCount(Guid currentUserId)
{
    using (var dc = new MyDbContext(_environment))
    {
        var helper = new DocumentPermissionsHelper(dc);
        var docQuery = dc.Docs.Where(d => d.CreatedDate > DateTime.Now.AddDays(-1));
        docQuery = helper.SelectDocumentsAccessibleToUser(docQuery, currentUserId);
        int latestDocsCount = docQuery.Count();
        return latestDocsCount;
    }
}

As I said, I have many different queries that produce different kinds of data, but all have one common part: the building of subquery that selects only documents accessible to a certain UserID.

Is there a better way to 'inject' this common query part into multiple Entity Framework queries rather than instantiate DocumentPermissionsHelper every time? My own solution seems somewhat clumsy to me.

Answer 1

Probably more can be made to clean that code but from your snippet I'd first introduce an extension method. You're using an object just to hold some parameters.

static class MyContextExtensions
{
    public static IQueryable<Document> SelectDocumentsAccessibleToUser(this MyDbContext dc, IQueryable<Document> query, Guid userId)
    {
        var permissionsQuery = dc. ... // really big query with multiple JOIN's to determine which user has access to which documents 
        IQueriable<Guid> docIdsAccessibleToUserQuery = 
            from permissions in permissionsQuery.Where(_ => _.UserId == userId && _.AccessLevel == "Read")
            select permissions.DocumentId;

        return from accessibleDocId in docIdsAccessibleToUserQuery.Distinct()
                from accessibleDoc in query.Where(g => g.Id == accessibleDocId)
                select accessibleDoc;
    }
}

Simply used like this:

public void GetLatestDocsCount(Guid currentUserId)
{
    using (var dc = new MyDbContext(_environment))
    {
        var docQuery = dc.Docs.Where(d => d.CreatedDate > DateTime.Now.AddDays(-1));
        docQuery = dc.SelectDocumentsAccessibleToUser(docQuery, currentUserId);
        int latestDocsCount = docQuery.Count();
        return latestDocsCount;
    }
}

Few other things:

• You named a lambda parameter as _ and you're actually using it. It's not a rule but as widely accepted convention _ is reserved for an unused parameter. Rename it to something appropriate (or...x, at least).
• Do not hard-code "Read" string, you might want to introduce a private const string field for that. From your snippet it may be local to MyContextExtensions but I guess you use it also elsewhere. Is there a reason you're using a string for that? It's a good candidate to be an enum.
• I feel strange when I see mixing plain LINQ calls and query syntax. If there is no other reason you may want to stick to one of them (at least within same expression).

For example:

IQueriable<Guid> docIdsAccessibleToUserQuery = permissionsQuery
    .Where(_ => _.UserId == userId && _.AccessLevel == "Read")
    .Select(x => x.DocumentId);

• latestDocsCount local variable does not really help to understand better your code, you may drop it and simply return docQuery.Count();.
• It's opinable but IMO if you keep functions short (good!) then docQuery might simply be query.
• DateTime.Now.AddDays(-1) shouldn't be hard-coded, it's an important peace of information. I'd move that query into a separate method GetDocumentsCreatedYesterday().
• You're using DateTime.Now instead of DateTime.UtcNow. It may be or not an issue in your case but consider that time is adjusted (summer time) then twice per year you may get incorrect results. Also remember that if your users are spread all over the world then you'd better to store UTC time and format to local time only for presentation.
• After our first refactoring step you do not have DocumentPermissionsHelper class but in general don't forget to mark a class as sealed when it's not intended to be extended.