-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Named query filters #8576
Comments
@davidroth Thanks for the suggestion, This is an interesting idea, but not something we are planning to implement for 2.0. We may work on it in the future, but that would likely depend on additional feedback. We would also consider a well-written community PR for this. (If anyone does plan to send a PR, please check with us first so we can do some thinking and give some guidance/feedback on the API.) @davidroth I suspect you could figure this out anyway, but it is possible to have a flag on the context that can be used to switch on and off various parts of the filter. Something like" modelBuilder.Entity<Post>()
.HasQueryFilter(p => (!this.DeleteFilterEnabled || !p.IsDeleted)
&& p.TenantId == this.TenantId ); |
@ajcvickers @davidroth public class BloggingContext : DbContext
{
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Post>()
.HasQueryFilterFactory(ctx =>
{
if (ctx.ApplyDeleteFilter)
return p => !p.IsDeleted && p.TenantId == ctx.TenantId
else
return p => p.TenantId == ctx.TenantId;
});
}
} This way client code can maintain multiple situational filters without generating overly complex where clauses all the time. |
I suggest to check https://github.com/zzzprojects/EntityFramework.DynamicFilters to get idea. It's widely used and makes same thing for EF6. |
I've had a go at this. I've managed to get it working with relatively minor changes. It involves replacing the QueryFilter lambda on the Entity with a custom dictionary of [QueryFilters]. It works fine, apart from the internal expression caching - if I call the same query twice with the filter enabled and disabled, because QueryFilter is added 'after' the query is generated, it picks the cached one instead. Does anyone have any ideas how I can clear the query cache when I switch the filter on and off? I've put together a really simple console app to test it, and monitored the sql generated in the SQL Profiler:
Notice the second query uses [p=> p.Name] for the order by expression. If I set this to the same as the first query, the filter is bypassed. I have access to Enable and Disable methods via an extension method on the DbContext:
|
If anyone is interested in helping me with this; its this method in particular that is causing my issue.
It lives in the CompiledQueryCache.cs file: Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.cs |
@f135ta - And why do we want to skip cache? Why not compile just one expression which would apply all filters with disable/enable thing appropriately. |
Correct, its part of my changes to code base: In the EntityType.cs i've added the following code:
Actually, you're correct - "And why do we want to skip cache? Why not compile just one expression which would apply all filters with disable/enable thing appropriately." I'd rather the query was cached, but it appears that its added to and retrieved from the cache 'before' the queryfilters are applied - and i cant quite work out how to switch it around |
@f135ta - We cannot mutate the IModel when running the query. |
Ok, so the filters would be applied during model generation in the DbContext as normal.
Like they are currently. Except im passing in another parameter "FilterName" which is used to identify it in the QueryFilterCollection stored on the EntityType. (In place of the QueryFilter Lambda Expression that is currently there) .. The extension methods merely change a flag on the filter item in the QueryFilterCollection to enable/disable it. Would that not be correct? |
That particular bit becomes incorrect. QueryFilterCollection lives on EntityType inside the model. You cannot change the value of that flag. You would need a better way to define how to enable/disable filters. You would need a method on each query to control the behavior as suggested in first post. Similar to how include works on query roots. |
Ok, so the issue is that it lives on the EntityType? It makes sense now as to why its not picked up the way I was expecting; since the EntityType is built during the model generation and sealed off. I'll take a look at the Includes functionality and have a rethink. I'd still like to keep the HasQueryFilter functionality on the onModelBuilding because it makes sense for it to be there (if possible). Thanks for your help! I'll have another stab at it because its a feature that i'd really like to see implemented; I'll keep you updated on the progress ;-) |
@f135ta - HasQueryFilter would certainly live on OnModelCreating method. Basically, it is like tell us all the filters you would like your model to have. And then a different mechanism which has readonly access to model to enable/disable parts of it. A result operator would be easy choice to annotate to pass the info to query compiler and at the same time, differentiate the ExpressionTree in cache. |
Not sure if I should create a new issue for this, so I'll just dump it here for now. modelBuilder.Entity<Thing>.HasQueryFilter<SoftDeleteFilter>(); Where
modelBuilder.Entity<Thing>.RemoveQueryFilter<SoftDeleteFilter>();
var list = db.Things.IgnoreQueryFilter<SoftDeleteFilter>().WithQueryFilter<MyCustomQueryFilter>().ToList(); Note: var list = db.Things.IncludeDeleted().WithSomeFeature().ToList();
var list = db.Things.AsOf(new DateTime(DateTime.Now.Year, 1, 1)).ToList(); ...which adds a filter provider under the hood: public static IQueryable<T> AsOf<T>(this DbSet<T> source, DateTime timestamp) {
return source.WithQueryFilter(new TemporalQueryFilterProvider<T>(timestamp));
} Proposal 2: Thinking a little further, simple predicate-based query filters could be abstracted out into more general query rewriters: modelBuilder.Entity<Thing>.HasQueryRewriter<MultiTenancyQueryRewriter>();
modelBuilder.Entity<Thing>.HasQueryRewriter<TemporalQueryRewriter>(); I think this last bit is already tracked in #626 although it doesn't seem like anyone was working on enabling it. |
This question have a solution? Now, how is a better way for dinamically enable/disable one Global Filter? |
@f135ta are you still working on this one? I woul love to have this into effect so I'd like to help. |
@ilmax - Hi! Yes, its still on my list of things to work on. I've been mega busy with "real-life" so I havent had time to complete the work yet. Hopefully i'll be able to dedicate some time to it over the weekend. |
Quick update on my attempts with this. I've rethought my approach and instead started to look at the QueryCompiler and going down the route of intercepting the calls within there to fetch custom queryfilters and applying them before they're executed. I believe this will mean that they are caught and processed by the CompiledQueryCache and therefore fix my original problem (noted above). Queries can still be added in the same way using the EntityTypeConfiguration. I'll then look at how we make extension methods so that queries can have an inline enable/disable function for each query. I'm probably about 50% the way to getting a working prototype for this approach and will share my ideas in more detail when I have a proof of concept available.. |
Is any chance that this feature will be implemented in future release ? |
https://blogs.msdn.microsoft.com/dotnet/2018/12/04/announcing-entity-framework-core-2-2/ It's not on roadmap, so don't count on it for the next version (but there's still may be a change if EF team decides to do it) |
Thank you for the replay. I am full of hope that EF team will see a value of this feature |
I currently have to go with RLS in MSSQL, as it's not possible to implement proper filtration on multiple conditions with EfCore at the moment. Personally, I hate to store any kind of logic inside of a database, but this is the only option we have. So, "more flexibility" is definitely a thing I desire to be supported. Current solution is not usable, when in 1 app you have:
|
@vitalybibikov - I'm confused at how this function prevents you from using EF? Query Filters provide "additional" functionality to EF, in that they allow you to globally set a filter for an entity. There is absolutely nothing stopping you from coding the 4 things you have already mentioned yourself? Linq is provided for exactly that purpose; so you can shape your queries? |
I've never said it prevents us from using Ef Core. We are using it. It is possible to create a set of Extensions, though it won't be as performant as on OnModelCreating. While it is possible to implement and test a proper solution, it consumes time and requires support. |
I guess to find the ultimate approach of really implementing individual query filters, it would already help a lot to have a context with query specific properties available. With the feature set, you can add 1 or more properties to a Here such an example context that would not allow this flexibility: public class BloggingContext : DbContext
{
public bool IsPostDeleteFilterActive {get;set;}
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Post>()
.HasQueryFilter(p => !IsPostDeleteFilterActive || !p.IsDeleted)
}
}
var ctx = new BloggingContext();
var withDeleted = ctx.Posts;
var withoutDeleted = ctx.Posts;
// later in the code:
ctx.IsPostDeleteFilterActive = true;
var resultsWithDeleted = withDeleted.ToArray();
ctx.IsPostDeleteFilterActive = false;
var resultWithoutDeleted = withoutDeleted.ToArray(); It is not nice to need setting the properties correctly before you actually execute the queries. Sometimes you even cannot do this as you pass on the queryable to another library (like OData). It would be great if we can have something like this as a first implementation on EF: public class BloggingContext : DbContext
{
public DbSet<Post> Posts{get;set;}
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Post>()
.HasQueryFilter((p, context) => !context.Properties.Get<bool>("IsPostDeleteFilterActive", defaultValue: true) || !p.IsDeleted)
}
}
var ctx = new BloggingContext();
var withDeleted = ctx.Posts.WithProperty("IsPostDeleteFilterActive", false);
var withoutDeleted = ctx.Posts;
// later in the code:
var resultsWithDeleted = withDeleted.ToArray();
var resultWithoutDeleted = withoutDeleted.ToArray(); Here the properties/flags are attached to the query and can be used within the filters. The |
Any news in this one? Thanks! |
- Configure multiple query filters on a entity referenced by name. - Ignore individual filters by name. The current implmentation of query filter is kept but when ignoring the filters using the current extension method will ignore all filters (so also the named). Fixes dotnet#8576 dotnet#10275 dotnet#21459
I tried to implement a solution that allows for multiple query filters to be defined on a single entity and allow them to be ignored individually of each other. When setting up a query filter you could provide a name for the query filter. This allows you to define multiple query filters on the same entity. When you call modelBuilder.Entity<Order>()
.HasQueryFilter(o => o.Customer != null && o.Customer.CompanyName != null);
modelBuilder.Entity<OrderDetail>()
.HasQueryFilter("HasOrder", od => od.Order != null);
modelBuilder.Entity<OrderDetail>()
.HasQueryFilter("Quantity", od => EF.Property<short>(od, "Quantity") > _quantity); When it comes to ignoring query filters you can still use the current solution. This will ignore all the configured query filters (so named an not named) on all entities just like the current implementation. context.Set<OrderDetail>()
.Include(c => c.Orders)
.IgnoreQueryFilters(); But if you only want to ignore a specific filter you can do so by calling context.Set<OrderDetail>()
.Include(c => c.Orders)
.IgnoreQueryFilter("HasOrder")
.IgnoreQueryFilter("Quantity"); This will allow you to implement solutions like soft delete and global user filtering and disable them individually context wide. But it also gives developers the freedom to use the functionality to create their own solutions. I kept the current solution in place so calling Any feedback on the current solution is welcome also any guidance to improve the solution or using a different one. I really would love this functionality to be part of efcore so I'm willing to put the time in it. |
We discussed this issue in team meeting again. Currently Also related issue - #10275 is about adding multiple query filters perhaps for ease of creation but doesn't explicitly defines goal as it to be named filters. (Named filters can be ignored individually, multiple filters is just about creating and EF Core side composing it in single lambda for users). So above proposed design is incomplete and there are various questions which needs to be answered/decided. Further the implementation is not improving anything specific apart from just adding enumerable rather than single instance and enumerating it where instance was used. (ignoring the context where Ignore was specified. Issue with conflicting Ignore etc). We as a team don't have time to arrive at a full design for this feature or iterate over design proposals in 6.0 release timeframe. We can continue discussing design here if desired though response from the team would be pretty much delayed till most of 6.0 work is finished. |
@smitpatel The concerns about unclear scope / api is totally valid. However, the flipside of not having this functionality is that developers have to disable the query filters globally. Which leads to manually crafting often VERY complex and error prone statements. So there is an argument to be made that not having this functionality is worst than the potential ambiguity in the api. That said I feel there should be a way to get the best of both worlds. Some thoughts on removing ambiguity and an api proposal:
The above 3 scenarios could be expressed as follows: // query-level flag which ignores ALL query filters
context.Orders
.Include(c => c.Positions)
.IgnoreQueryFilters();
// query-level flag which ignores A SPECIFIC query filters
context.Orders
.Include(c => c.Positions)
.IgnoreQueryFilters("IsDeleted");
// ignore a SPECIFIC query filter for a SPECIFIC entity
// returns all orders with isDeleted query filter active + all positions without IsDeleted query filter active
context.Orders
.Include(c => c.Positions)
.IgnoreQueryFilters<OrderPosition>("IsDeleted"); Scenario 1 is cannot be combined with the other scenarios as by design it disables all query filters. However scenarios 2 + 3 can both be stacked as well as combined: context.Customers
.Include(c => c.Orders).ThenInclude(i => i.Positions)
.IgnoreQueryFilters("Tenant") // disable tenant filter on query level
.IgnoreQueryFilters<Customer>("IsDeleted") // disable is deleted filter for Customer entities
.IgnoreQueryFilters<OrderPosition>("IsDeleted"); // disable is deleted for OrderPosition entities In plain text the above would query:
This syntax removes any ambiguity I could think while keeping in line with the existing syntax as well. Thoughts? Did I miss any use case? |
Any update on this? |
@ldeluigi This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 8.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you. |
This is open since 2017 and doesn't seem very complicated. There is even a PR for this already (even though it doesn't meet the requirements?). Anyway what are the cons or difficult design choices with this one? |
@TheDusty01 I think all the discussion above outlines some of the design choices that need to be made, and shows that this is, in fact, quite complicated. |
Currently the alternative is to add a few complex statements to each call which is pretty error prone. I do understand the thoughts and risks but there are way more complex issues which are resolved faster than this one. So it may be better to provide any decent/slightly extensible solution instead of none at all I guess. |
A minimal version of this IMO is (quoting above): // query-level flag which ignores A SPECIFIC query filters
context.Orders
.Include(c => c.Positions)
.IgnoreQueryFilters("IsDeleted"); It also enables entity-level disabling, as EF users can maintain the keys they used for a specific entity in a variable or something and disable them all. Maybe even add an extension method that does it. |
so i had a slightly different requirement. i use snapshot testing when running tests against EF. so as part of that, when some code runs, any sql queries that are executed are capture into a file that is convention named based on the current test. we use QueryFilters for soft deletes, and this functionality is not relevant to the majority of tests. but our QueryFilters add significant noise to the sql snapshots. so i wanted a way of toggling QueryFilters on/off from within a test. so even though the requirements, i suspect the approach used may help other people when customising query filters note that none of this code is in the production app. it all exists in the test project this is the code i came up with: QueryFiltera wrapper around an AsyncLocal flag
QueryContextFactoryin effect calls
KeyGeneratorTo ensure that queries that queries are cached separately based on if QueryFilters are enabled or disabled
Replace services
Usage
|
While the new query filters feature is really cool, I think it is lacking additional flexibility for configuration + usage (enable / disable).
Let me use the example posted in your announcement blog: https://blogs.msdn.microsoft.com/dotnet/2017/05/12/announcing-ef-core-2-0-preview-1/
This example uses one filter for tenant id + isDeleted.
The only way to disable both filters is by using the following API:
Unfortunately there is no way to disable a specific part of the. It would make sense for example to disable only the
IsDeleted filter
, while keeping thetenant filter
enabled.This could be enabled by specifying multiple filters by key:
Now one would have full flexibility to either disable all filters, or disable only specific filters:
Have you considered this scenario?
The text was updated successfully, but these errors were encountered: