Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for query filter factories to "lazy load" query filter expressions #19522

Closed
wants to merge 1 commit into from

Conversation

joshcomley
Copy link

Query filters are a powerful tool in Entity Framework, but limited as they are restricted to being defined ahead of being used.

Suppose the following scenario

A business application with 200 tables. Extreme performance is not imperative as this does not need to scale to millions of users, but correct business security, per user, is paramount.

Currently, as the data context is generated, the logic behind query filters for each of those tables must be created before any of those tables are even queried. Additionally, the logic is static, sealed in a single LambdaExpression.

If we needed different logic per tenants we can either use a new derivative of our DbContext or we can use a new IModelCacheKeyFactory implementation (or perhaps some usage of ICompiledQueryCacheKeyGenerator), and configure the query filters depending on the current user/request.

But again, the current set-up forces us to determine the query filters for all 200 tables ahead of time.

A better set-up would be to only construct the query filters if a request requires it, essentially lazy load them.

EF Core has a strong opinion about sealing the model once it's built - it becomes read-only. I have attempted to adhere to this opinion in this PR, albeit with the singular exception of internally swapping a query filter factory in the entity type annotations for the actual LambdaExpression once the factory has been invoked for the first time. This maintains the idea of defining what tables have query filters ahead of time, and keeps the factory invoked only once per model-cache.

The result is the ability to do this (when combined with a per-request IModelCacheKeyFactory):

modelBuilder.Entity<ApplicationUser>().HasQueryFilter(() =>
{
    if (this.CheckSomethingAboutCurrentUser())
    {
        return user => user.FullName.Contains("hello");
    }
    return user => user.FullName.Contains("goodbye");
});

Given we can inject anything we like into the DbContext, we can check details about the request, perhaps some user information stored elsewhere, anything we like to construct our query filter, but at no expense if the filter is not used in that request.

@ajcvickers
Copy link
Contributor

@joshcomley I think you're attempting to address the situation outlined here: #18417 (comment)

Query filters are currently dynamic because they can use state from the DbContext instance. Another way of saying this is that it was never intended that different filters be used by building different models. Instead the model would always have the filters and they would then behave in different ways depending on the state of the context.

That being said, I think we agree that the current experience is lacking in several ways:

It is important that the underlying model remain read-only after it is built, and also that multiple models do not need to be built to support these things.

It is not immediately clear to me that this PR solves these problems in an appropriate way. This is probably a case where discussing a design before sending a PR would have been more productive.

@joshcomley
Copy link
Author

@ajcvickers Thank you for the reply. I understand that the query filters are dynamic already and take advantage of this, but the problem is that when I am setting up my query filters on all of my entity types, I don't know which of those entity types will actually be used in this request, and as such may waste time determining details for a query filter that I may not end up using.

For example, say I have:

/Shops
/Managers
/Products

I might need to do some obscure business lookup to external systems to determine the kind of query filter I need to use for a given user at a given time. This may have to happen upon each request, as real-time changes to the user's state in the external system need to be reflected. So depending on the user state I will do:

.HasQueryFilter(_ => [logic 1])

or

.HasQueryFilter(_ => [logic 2])

And so on. But if I have hundreds of tables, and a tonne of business logic that needs to be checked, I don't want to perform all that checking if only /Products is going to be requested. Figuring out what query filter logic I need through some external system look-ups for /Managers is totally unnecessary if no Manager entities will be requested at all.

There are also situations in which completely dynamic filters themselves are required (perhaps combining a bunch of ORs and/or ANDs as needed), and the current approach makes this a difficult task without sacrificing performance by way of eliminating query compilation caching.

I agree a conversation around design would be great, but where can I go to do this ahead of submitting a PR? Create a new issue in here?

@smitpatel
Copy link
Contributor

@joshkomley - It is not clear to me that why query filters are required for the scenario you describe rather than dynamic queries which would construct based on the business logic required in individual request.

@joshcomley
Copy link
Author

@smitpatel I am using OData for the API, query filters ensure that no matter what expand etc. is done, if a certain entity type is returned it has the filter applied. OData enables clients to use the API with their own custom queries, transformed into EF Core, and then secured with query filters.

@ajcvickers
Copy link
Contributor

@joshcomley Creating a new issue describing the feature you want is the place to start on this. I can then discuss with the team whether or not this seems like something we would want to add to the core code base. If it is, then we can discuss design/implementation on that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants