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

Support multiple HasQueryFilter calls on same entity type #10275

Open
nphmuller opened this issue Nov 13, 2017 · 17 comments

Comments

@nphmuller
Copy link

commented Nov 13, 2017

As of 2.0 multiple HasQueryFilter() calls on EntityTypeBuilder result in only the latest one being used. It would be nice if multiple query filters could be defined this way.

Example:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<MyEntity>()
        .HasQueryFilter(e => e.IsDeleted == isDeleted)
        .HasQueryFilter(e => e.TenantId == tenantId);
}

// In application code 
myDbContext.Set<MyEntity>().ToList() // executed query only has the tenantId filter.

Current workaround is to define all filters in single expression, or rewrite the expression to concat multiple filters.

@nphmuller

This comment has been minimized.

Copy link
Author

commented Nov 13, 2017

I've set up a Gist which shows my query filter use-case and the code I had to write to get it working.
https://gist.github.com/nphmuller/8891c315d79aaaf720f9164cd0f10400
https://gist.github.com/nphmuller/05ff66dfa67e1d02cdefcd785661a34d

@anpete

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

This is currently by-design as it is usually pretty easy to combine filters with '&&'. Filters can also be unset by passing null and so it is not just a matter of allowing HasQueryFilter to be called multiple times.

@nphmuller

This comment has been minimized.

Copy link
Author

commented Nov 14, 2017

@anpete - Sure, than consider this a feature request. ;)

When the filters can't be combined easily with &&, you have to use the Expression API to combine filters. Basically like the Gist I posted. Although the Gist can probably be simplified quite a bit, especially when ReplacingExpressionVisitor doesn't have to be used anymore.

However, say I want to combine multiple filters based on different base/interface types. Like a type that implements both ISoftDeletableEntity and ITenantEntity. I don't think it's possible to wire both of these up in a single HasQueryFilter call, since TEntity in Expression<Func<TEntity, bool>> is different.

@challamzinniagroup

This comment has been minimized.

Copy link

commented Oct 22, 2018

I would second a vote for this feature as I just hit it myself with the exact two scenarios mentioned; soft deletes and multi-tenancy. I was able to combine filters for my current use-case; but there can certainly be issues where the code to calc filters based on model inheritance could get awful "gummy" in a single filter declaration.

@nphmuller

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

Here's a gist of the workaround we have implemented:
https://gist.github.com/nphmuller/05ff66dfa67e1d02cdefcd785661a34d

Edit: Woops, looks like I've already posted a Gist a year ago. The code in this one is a bit more cleaned up though. :)

@Levitikon217

This comment has been minimized.

Copy link

commented Dec 17, 2018

Interestingly the "Global Query Filters" documentation written 9 days before this bug was opened suggests calling HasQueryFilter multiple times. However I am still seeing this issue today in 2.1, 1 year later. Is there any updates on this? Please remove this documentation as it clearly doesn't work.

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    modelBuilder.Entity<Blog>().Property<string>("TenantId").HasField("_tenantId");

    // Configure entity filters
    modelBuilder.Entity<Blog>().HasQueryFilter(b => EF.Property<string>(b, "TenantId") == _tenantId);
    modelBuilder.Entity<Post>().HasQueryFilter(p => !p.IsDeleted);
}

https://docs.microsoft.com/en-us/ef/core/querying/filters

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@FelixKing - Those HasQueryFilter calls are on different entity types. You can call HasQueryFilter once per entity type. This issue talks about calling it multiple times on same entity.

@nphmuller nphmuller changed the title Support multiple HasQueryFilter calls Support multiple HasQueryFilter calls on same entity type Dec 17, 2018

@YZahringer

This comment has been minimized.

Copy link

commented Jan 25, 2019

My workaround with extension methods:

internal static void AddQueryFilter<T>(this EntityTypeBuilder entityTypeBuilder, Expression<Func<T, bool>> expression)
{
    var parameterType = Expression.Parameter(entityTypeBuilder.Metadata.ClrType);
    var expressionFilter = ReplacingExpressionVisitor.Replace(
        expression.Parameters.Single(), parameterType, expression.Body);
    
    var internalEntityTypeBuilder = entityTypeBuilder.GetInternalEntityTypeBuilder();
    if (internalEntityTypeBuilder.Metadata.QueryFilter != null)
    {
        var currentQueryFilter = internalEntityTypeBuilder.Metadata.QueryFilter;
        var currentExpressionFilter = ReplacingExpressionVisitor.Replace(
            currentQueryFilter.Parameters.Single(), parameterType, currentQueryFilter.Body);
        expressionFilter = Expression.AndAlso(currentExpressionFilter, expressionFilter);
    }

    var lambdaExpression = Expression.Lambda(expressionFilter, parameterType);
    entityTypeBuilder.HasQueryFilter(lambdaExpression);
}

internal static InternalEntityTypeBuilder GetInternalEntityTypeBuilder(this EntityTypeBuilder entityTypeBuilder)
{
    var internalEntityTypeBuilder = typeof(EntityTypeBuilder)
        .GetProperty("Builder", BindingFlags.NonPublic | BindingFlags.Instance)?
        .GetValue(entityTypeBuilder) as InternalEntityTypeBuilder;

    return internalEntityTypeBuilder;
}

Usage:

if (typeof(ITrackSoftDelete).IsAssignableFrom(entityType.ClrType))
    modelBuilder.Entity(entityType.ClrType).AddQueryFilter<ITrackSoftDelete>(e => IsSoftDeleteFilterEnabled == false || e.IsDeleted == false);
if (typeof(ITrackTenant).IsAssignableFrom(entityType.ClrType))
    modelBuilder.Entity(entityType.ClrType).AddQueryFilter<ITrackTenant>(e => e.TenantId == MyTenantId);
@mguinness

This comment has been minimized.

Copy link

commented Mar 7, 2019

Managing global filters can certainly become unwieldy. I have filters based on user roles and using HasQueryFilter isn't really cutting it and something like PredicateBuilder would be really useful.

Combining filters seems to be a common use case including this SO question but the solution can be difficult for beginners to follow. Can extension methods like these be added into EF Core, or would a separate nuget package be more expedient?

@cgountanis

This comment has been minimized.

Copy link

commented Jun 7, 2019

When doing multiple includes and using HasQueryFilter on say, objects with child objects which all have a IsDeleted flag, the execute goes up to 14,000 MS. Is there a solution to this? I would rather not include deleted records in the context vs, the business or presentation layer.

In my case the soft delete is a DateTime? and I only include where IS NULL, could that be the issue? IS a bool faster than a IS NULL check?

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@cgountanis Please file a new issue and include a small, runnable project solution or complete code listing that demonstrates the behavior you are seeing.

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@cgountanis Just saw #15996. Thanks!

@haacked

This comment has been minimized.

Copy link

commented Aug 19, 2019

Here's a scenario where support for multiple HasQueryFilter calls on the same entity would be useful. https://haacked.com/archive/2019/07/29/query-filter-by-interface/

In short, I wrote a method that lets you do this:

modelBuilder.SetQueryFilterOnAllEntities<ITenantEntity>(e => e.TenantId == tenantId);
modelBuilder.SetQueryFilterOnAllEntities<ISoftDeletable>(e => !e.IsDeleted);

What that code does is is find all entities that implement the interface and adds the specified query filter to the entity (some expression tree rewriting is involved).

However, this doesn't work in the case where an entity implements both interfaces because the last query filter overwrites the previous one.

@haacked

This comment has been minimized.

Copy link

commented Aug 19, 2019

I want to note that I can update my own method now that I know about this behavior, but it was surprising. It lead to entities from one tenant bleeding into another until i figured out what was going on.

So in short, I think it's surprising that the last HasQueryFilter call wins. I'd rather it throw an exception if called twice on the same entity, or that it combine query filters per this issue.

@ajcvickers ajcvickers removed this from the Backlog milestone Aug 19, 2019

@haacked

This comment has been minimized.

Copy link

commented Aug 20, 2019

Actually, as I think about it, throwing an exception could be problematic if you were appending to an existing query filter by overwriting it. Perhaps adding an AppendQueryFilter method would be useful which would be explicit. Then you could safely throw on multiple calls to HasQueryFilter. Since the primary use case is soft deletes and multi-tenancy, there's security implications for users of the API getting it wrong.

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@haacked Thanks for the feedback. We agree that there is a usability issue here. We can't do anything here for 3.0, but for a future release we would like to make it possible to:

  • Add multiple filters independently of each other--tracked by this issue
  • Allow those filters to be named in some way (#8576) such that enabling/disabling (#17347) a query filter is also independent of other filters
  • We may also want to make it easier to use the same filter across multiple types in the model (#17434)

Also, we plan to implement filtered Include (#1833) for more localized ad-hoc filtering.

@haacked

This comment has been minimized.

Copy link

commented Aug 26, 2019

Those all sound great! Thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.