Add support for Global Query Filters on derived types.

[zbarnett]

Global Query Filters can now be added to entities at any level of an inheritance hierarchy, not just the root level. Removed the limitation mentioned here: https://docs.microsoft.com/en-us/ef/core/querying/filters#limitations

• Filters attached to derived types will have a wrapper automatically added so that the filter only applies to rows of that type and its subtypes.
  • (!(x is {DerivedType}) || (x is {DerivedType} && {filter}))
• If multiple filters are applied at the same time (filters at multiple levels of the hierarchy) the filters are ANDed together so that all of them must be satisfied.
• Note: it is still not possible to assign multiple filters to the same entity.

Fixes #10259

I implemented this for my employer and am awaiting official permission before I sign the CLA (I have verbal permission but we're waiting on the legal dept.), but wanted to get this PR out there in the meantime to get feedback.

I modified the InheritanceQueryFixtureBase context for the specification tests but only noticed test failures for SQL Server. I'm not sure how to test the other providers. Any guidance would be appreciated.

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format:
• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo

[dnfadmin]

All CLA requirements met.

[AndriySvyryd]

@zbarnett Please sign the CLA so we can review the changes

[zbarnett]

@AndriySvyryd unfortunately I'm still waiting on our legal dept to approve it. I'll sign as soon as I can.

[anton-kirschhock]

@zbarnett The Limitation of the Global Queries that it cannot go into the hierarchy is blocking us from delivering a major Feature to our customers and this PR could might be a solution to it.
Any idea when it will be available?

[SanderLesage]

Very cool @zbarnett! This is a very nice feature we have been looking for already for a long time. Hope you can sign the CLA soon 🥇

[pinkfloydx33]

Will this make it to EF7?

[ajcvickers]

CLA is not signed. We cannot even look at the code unless the CLA is signed.

[zbarnett]

Sorry about this taking so long. I just checked with management and we're still waiting on legal to approve it. This is the first time I've made any open source contributions at my current job and wasn't aware the process was so lengthy. Will sign as soon as I can.

[zbarnett]

Finally got this approved with my employer and I just signed the CLA. I'll be updating this fork and fixing tests over the next few days. Thank y'all for being patient.

[christianacca]

I'll be updating this fork and fixing tests over the next few days. Thank y'all for being patient.

@zbarnett any ETA?

[zbarnett]

I'll be updating this fork and fixing tests over the next few days. Thank y'all for being patient.

@zbarnett any ETA?

Unfortunately had some unexpected things come up which caused this to be put on the back burner for a bit. Still definitely planning to circle back to it, though.

[christianacca]

Now that a CLA has been signed, @ajcvickers can anyone on the EF Core team help @zbarnett with the outstanding fixes to unit tests?

[maumar]

We decided that we will not accept this PR, sorry.

The problem is that this approach doesn't work great for TPT/TPC. Entities are spread around several tables that we would join multiple times. The resulting query is nasty because we are unable to prune unnecessary joins efficiently.
We need to put some more thought as to how do we approach the query filter problem for these. Both from the model side (what's the correct "granularity" of filter we should allow to specify - entity? table?) and the query side (any optimizations we can apply to make the filtering queries more efficient).

Also, this functionality is mostly available in current EF by declaring a filter based on individual subtypes on the root. The PR is a convenient/useful sugar over the base functionality and moreover allows for better encapsulation, but we don't think it's enough value to lock ourselves into a particular design before we actually figure out what to do in case of TPT/TPC.

[zbarnett]

@maumar I'm curious how y'all arrived at that conclusion. I've been using these changes in production for about a year in a schema of a little over 500 tables that uses solely TPT inheritance with a max depth of 6 without any problems. It was my understanding that the joins for the the base types were necessary regardless in the case of TPT/TPC inheritance in order to construct a fully populated entity so I didn't think my changes should cause any additional joins. Though, it's very possible I'm misunderstanding something. I will say that I haven't used TPC inheritance at all so I'm not familiar with the implications there.

Thanks for taking a look at this.

[maumar]

@zbarnett

Here is the example that illustrates the problem:

    [ConditionalFact]
    public void Query_filters_test()
    {
        using var ctx = new MyContext();
        ctx.Database.EnsureDeleted();
        ctx.Database.EnsureCreated();
        var query = ctx.Entities.OfType<BranchEntity2>().ToList();
    }

    public class RootEntity
    {
        public int Id { get; set; }
        public string RootName { get; set; }
    }

    public class BranchEntity1 : RootEntity
    {
        public string BranchName1 { get; set; }
    }

    public class BranchEntity2 : RootEntity
    {
        public string BranchName2 { get; set; }
    }

    public class LeafEntity11 : BranchEntity1
    {
        public string LeafName11 { get; set; }
    }

    public class LeafEntity12 : BranchEntity1
    {
        public string LeafName12 { get; set; }
    }

    public class LeafEntity21 : BranchEntity2
    {
        public string LeafName21 { get; set; }
    }

    public class LeafEntity22 : BranchEntity2
    {
        public string LeafName22 { get; set; }
    }

    public class MyContext : DbContext
    {
        public DbSet<RootEntity> Entities { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<RootEntity>().UseTptMappingStrategy().HasQueryFilter(x => x.RootName != "bad root");
            modelBuilder.Entity<BranchEntity1>().HasQueryFilter(x => x.BranchName1 != "bad branch 1");
            modelBuilder.Entity<BranchEntity2>().HasQueryFilter(x => x.BranchName2 != "bad branch 2");
            modelBuilder.Entity<LeafEntity11>().HasQueryFilter(x => x.LeafName11 != "bad leaf 11");
            modelBuilder.Entity<LeafEntity12>().HasQueryFilter(x => x.LeafName12 != "bad leaf 12");
            modelBuilder.Entity<LeafEntity21>().HasQueryFilter(x => x.LeafName21 != "bad leaf 21");
            modelBuilder.Entity<LeafEntity22>().HasQueryFilter(x => x.LeafName22 != "bad leaf 22");
        }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=QueryFiltersSample;Trusted_Connection=True;MultipleActiveResultSets=true");
        }
    }

it's a binary tree-like structure, with root entity having two branches and each branch having two leaves. Each entity in hierarchy has it's own query filter defined.

Now, when we query using OfType<BranchEntity2> this is the sql that gets generated with this PR applied:

SELECT [e].[Id], [e].[RootName], [b0].[BranchName2], [l1].[LeafName21], [l2].[LeafName22], CASE
    WHEN [l2].[Id] IS NOT NULL THEN N'LeafEntity22'
    WHEN [l1].[Id] IS NOT NULL THEN N'LeafEntity21'
    WHEN [b0].[Id] IS NOT NULL THEN N'BranchEntity2'
END AS [Discriminator]
FROM [Entities] AS [e]
LEFT JOIN [BranchEntity1] AS [b] ON [e].[Id] = [b].[Id]
LEFT JOIN [BranchEntity2] AS [b0] ON [e].[Id] = [b0].[Id]
LEFT JOIN [LeafEntity11] AS [l] ON [e].[Id] = [l].[Id]
LEFT JOIN [LeafEntity12] AS [l0] ON [e].[Id] = [l0].[Id]
LEFT JOIN [LeafEntity21] AS [l1] ON [e].[Id] = [l1].[Id]
LEFT JOIN [LeafEntity22] AS [l2] ON [e].[Id] = [l2].[Id]
WHERE ([e].[RootName] <> N'bad root' OR ([e].[RootName] IS NULL)) AND ((([l0].[Id] IS NULL) AND ([l].[Id] IS NULL) AND ([b].[Id] IS NULL)) OR ((([l0].[Id] IS NOT NULL) OR ([l].[Id] IS NOT NULL) OR ([b].[Id] IS NOT NULL)) AND ([b].[BranchName1] <> N'bad branch 1' OR ([b].[BranchName1] IS NULL)))) AND ((([l2].[Id] IS NULL) AND ([l1].[Id] IS NULL) AND ([b0].[Id] IS NULL)) OR ((([l2].[Id] IS NOT NULL) OR ([l1].[Id] IS NOT NULL) OR ([b0].[Id] IS NOT NULL)) AND ([b0].[BranchName2] <> N'bad branch 2' OR ([b0].[BranchName2] IS NULL)))) AND (([l].[Id] IS NULL) OR (([l].[Id] IS NOT NULL) AND ([l].[LeafName11] <> N'bad leaf 11' OR ([l].[LeafName11] IS NULL)))) AND (([l0].[Id] IS NULL) OR (([l0].[Id] IS NOT NULL) AND ([l0].[LeafName12] <> N'bad leaf 12' OR ([l0].[LeafName12] IS NULL)))) AND (([l1].[Id] IS NULL) OR (([l1].[Id] IS NOT NULL) AND ([l1].[LeafName21] <> N'bad leaf 21' OR ([l1].[LeafName21] IS NULL)))) AND (([l2].[Id] IS NULL) OR (([l2].[Id] IS NOT NULL) AND ([l2].[LeafName22] <> N'bad leaf 22' OR ([l2].[LeafName22] IS NULL)))) AND (([l2].[Id] IS NOT NULL) OR ([l1].[Id] IS NOT NULL) OR ([b0].[Id] IS NOT NULL))

note that join for BranchEntity1 is added, as well as it's children LeafEntity1 and LeafEntity2. We don't want this to happen because we are guaranteed that results can't be coming from these tables. However, they are added because the entire filter predicate is being evaluated so can't effectively prune those references.

If we remove query filters on all the derived entities we get this sql:

SELECT [e].[Id], [e].[RootName], [b0].[BranchName2], [l1].[LeafName21], [l2].[LeafName22], CASE
    WHEN [l2].[Id] IS NOT NULL THEN N'LeafEntity22'
    WHEN [l1].[Id] IS NOT NULL THEN N'LeafEntity21'
    WHEN [b0].[Id] IS NOT NULL THEN N'BranchEntity2'
END AS [Discriminator]
FROM [Entities] AS [e]
LEFT JOIN [BranchEntity2] AS [b0] ON [e].[Id] = [b0].[Id]
LEFT JOIN [LeafEntity21] AS [l1] ON [e].[Id] = [l1].[Id]
LEFT JOIN [LeafEntity22] AS [l2] ON [e].[Id] = [l2].[Id]
WHERE ([e].[RootName] <> N'bad root' OR ([e].[RootName] IS NULL)) AND (([l2].[Id] IS NOT NULL) OR ([l1].[Id] IS NOT NULL) OR ([b0].[Id] IS NOT NULL))

where joins for BranchEntity1, LeafEntity11 and LeafEntity12 are correctly pruned out.

Users can write the entire filter predicate on the root entity and will get the same sql as that first one, so it may seem like not a big issue. Our thinking however is that when users themselves write code/query that produces bad sql it's nowhere near as bad as when some EF feature generates bad code for them.

[zbarnett]

@maumar thanks for that great example! I see exactly what you mean now and I definitely agree that we don't want EF to generate less than optimal queries.

If I reworked this a bit to play nicely with the type pruning could the decision be reconsidered?

[maumar]

@zbarnett yes, if pruning works we would reconsider - this was the major problem we had with the change and we are not fundamentally opposed to it. Fair warning though, this might be a significant amount of work.

One other thing to consider from design stand point - my understanding is that this change expects that each level of the hierarchy filters only itself. This is somewhat different than before, where root was responsible for providing filter for all it's derived types. What should happen when we encounter mix of the two approaches? (i.e root has some filter based on a property in branch, and branch has it's own separate filter). We shouldn't block this (breaking change), but perhaps perform some de-duplication? Or maybe leave it be, because it's the user who wrote bad code, not us. This shouldn't be a big deal, but we will discuss it in the EF team and get back to you with what/if we have a strong opinion here.

edit: EF Team decision: in case of mixing - leave as is

[ajcvickers]

Closing as there are significant changes needed and the code is diverging.