From f50cc97075997b371b4ca871cab280eb08062af7 Mon Sep 17 00:00:00 2001 From: Steven Darby <41150696+stevendarby@users.noreply.github.com> Date: Mon, 27 Mar 2023 19:34:13 +0100 Subject: [PATCH] Don't duplicate query filter parameters (#29422) * Don't duplicate query filter parameters Fixes #24476 --------- Co-authored-by: Steven.Darby --- .../NavigationExpandingExpressionVisitor.cs | 6 ++- .../ParameterExtractingExpressionVisitor.cs | 16 +++++++- .../QueryFilterFuncletizationTestBase.cs | 17 +++++++++ .../QueryFilterFuncletizationContext.cs | 37 ++++++++++++++++++- .../QueryFilterFuncletizationSqlServerTest.cs | 26 +++++++++++++ .../QueryFilterFuncletizationSqliteTest.cs | 28 ++++++++++++++ 6 files changed, 125 insertions(+), 5 deletions(-) diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs index 22d87b508ee..f6fedc5ac46 100644 --- a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs @@ -212,7 +212,8 @@ protected override Expression VisitExtension(Expression extensionExpression) // Apply defining query only when it is not custom query root && entityQueryRootExpression.GetType() == typeof(EntityQueryRootExpression)) { - var processedDefiningQueryBody = _parameterExtractingExpressionVisitor.ExtractParameters(definingQuery.Body); + var processedDefiningQueryBody = + _parameterExtractingExpressionVisitor.ExtractParameters(definingQuery.Body, clearEvaluatedValues: false); processedDefiningQueryBody = _queryTranslationPreprocessor.NormalizeQueryableMethod(processedDefiningQueryBody); processedDefiningQueryBody = _nullCheckRemovingExpressionVisitor.Visit(processedDefiningQueryBody); processedDefiningQueryBody = @@ -1737,7 +1738,8 @@ private Expression ApplyQueryFilter(IEntityType entityType, NavigationExpansionE if (!_parameterizedQueryFilterPredicateCache.TryGetValue(rootEntityType, out var filterPredicate)) { filterPredicate = queryFilter; - filterPredicate = (LambdaExpression)_parameterExtractingExpressionVisitor.ExtractParameters(filterPredicate); + filterPredicate = (LambdaExpression)_parameterExtractingExpressionVisitor.ExtractParameters( + filterPredicate, clearEvaluatedValues: false); filterPredicate = (LambdaExpression)_queryTranslationPreprocessor.NormalizeQueryableMethod(filterPredicate); // We need to do entity equality, but that requires a full method call on a query root to properly flow the diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs index 2870d6aaece..43ce2d91a3a 100644 --- a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs @@ -65,7 +65,13 @@ public ParameterExtractingExpressionVisitor( public virtual Expression ExtractParameters(Expression expression) => ExtractParameters(expression, clearEvaluatedValues: true); - private Expression ExtractParameters(Expression expression, bool clearEvaluatedValues) + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual Expression ExtractParameters(Expression expression, bool clearEvaluatedValues) { var oldEvaluatableExpressions = _evaluatableExpressions; _evaluatableExpressions = _evaluatableExpressionFindingExpressionVisitor.Find(expression); @@ -272,7 +278,13 @@ private Expression Evaluate(Expression expression, bool generateParameter) string? parameterName; if (_evaluatedValues.TryGetValue(expression, out var cachedValue)) { - var existingExpression = generateParameter ? cachedValue.Parameter : cachedValue.Constant; + // The _generateContextAccessors condition allows us to reuse parameter expressions evaluated in query filters. + // In principle, _generateContextAccessors is orthogonal to query filters, but in practice it is only used in the + // nav expansion query filters (and defining query). If this changes in future, they would need to be decoupled. + var existingExpression = generateParameter || _generateContextAccessors + ? cachedValue.Parameter + : cachedValue.Constant; + if (existingExpression != null) { return existingExpression; diff --git a/test/EFCore.Specification.Tests/Query/QueryFilterFuncletizationTestBase.cs b/test/EFCore.Specification.Tests/Query/QueryFilterFuncletizationTestBase.cs index 97ce2401101..d9fd28139ad 100644 --- a/test/EFCore.Specification.Tests/Query/QueryFilterFuncletizationTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/QueryFilterFuncletizationTestBase.cs @@ -340,4 +340,21 @@ public virtual void Using_multiple_context_in_filter_parametrize_only_current_co query = context.Set().ToList(); Assert.Equal(2, query.Count); } + + [ConditionalFact] + public virtual void Using_multiple_entities_with_filters_reuses_parameters() + { + using var context = CreateContext(); + context.Tenant = 1; + context.Property = false; + + var query = context.Set() + .Include(x => x.DeDupeFilter2s) + .Include(x => x.DeDupeFilter3s) + .ToList(); + + Assert.Single(query); + Assert.Single(query[0].DeDupeFilter2s); + Assert.Single(query[0].DeDupeFilter3s); + } } diff --git a/test/EFCore.Specification.Tests/TestModels/QueryFilterFuncletizationContext.cs b/test/EFCore.Specification.Tests/TestModels/QueryFilterFuncletizationContext.cs index ab34f7c4349..29980bb11d9 100644 --- a/test/EFCore.Specification.Tests/TestModels/QueryFilterFuncletizationContext.cs +++ b/test/EFCore.Specification.Tests/TestModels/QueryFilterFuncletizationContext.cs @@ -74,6 +74,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) // Multiple context used in filter modelBuilder.Entity() .HasQueryFilter(e => e.IsEnabled == Property && e.BossId == new IncorrectDbContext().BossId); + + modelBuilder.Entity().HasQueryFilter(x => x.Tenant == Tenant); + modelBuilder.Entity().HasQueryFilter(x => x.TenantX == Tenant); + modelBuilder.Entity().HasQueryFilter(x => x.Tenant == Tenant); } private void ConfigureFilter(EntityTypeBuilder builder) @@ -205,7 +209,14 @@ public static void SeedData(QueryFilterFuncletizationContext context) new MultiContextFilter { BossId = 1, IsEnabled = false }, new MultiContextFilter { BossId = 1, IsEnabled = true }, new MultiContextFilter { BossId = 2, IsEnabled = true }, - new MultiContextFilter { BossId = 2, IsEnabled = false } + new MultiContextFilter { BossId = 2, IsEnabled = false }, + new DeDupeFilter1 + { + Tenant = 1, + DeDupeFilter2s = new List { new() { TenantX = 1 }, new() { TenantX = 2 } }, + DeDupeFilter3s = new List { new() { Tenant = 1 }, new() { Tenant = 2 } } + }, + new DeDupeFilter1 { Tenant = 2 } ); context.SaveChanges(); @@ -418,4 +429,28 @@ public class MultiContextFilter public bool IsEnabled { get; set; } } +public class DeDupeFilter1 +{ + public int Id { get; set; } + public int Tenant { get; set; } + public ICollection DeDupeFilter2s { get; set; } + public ICollection DeDupeFilter3s { get; set; } +} + +public class DeDupeFilter2 +{ + public int Id { get; set; } + public int TenantX { get; set; } + public int? DeDupeFilter1Id { get; set; } + public DeDupeFilter1 DeDupeFilter1 { get; set; } +} + +public class DeDupeFilter3 +{ + public int Id { get; set; } + public short Tenant { get; set; } + public int? DeDupeFilter1Id { get; set; } + public DeDupeFilter1 DeDupeFilter1 { get; set; } +} + #endregion diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryFilterFuncletizationSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryFilterFuncletizationSqlServerTest.cs index 88ced32a671..4bd1c669950 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QueryFilterFuncletizationSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QueryFilterFuncletizationSqlServerTest.cs @@ -530,6 +530,32 @@ FROM [MultiContextFilter] AS [m] """); } + public override void Using_multiple_entities_with_filters_reuses_parameters() + { + base.Using_multiple_entities_with_filters_reuses_parameters(); + + AssertSql( +""" +@__ef_filter__Tenant_0='1' +@__ef_filter__Tenant_0_1='1' (DbType = Int16) + +SELECT [d].[Id], [d].[Tenant], [t].[Id], [t].[DeDupeFilter1Id], [t].[TenantX], [t0].[Id], [t0].[DeDupeFilter1Id], [t0].[Tenant] +FROM [DeDupeFilter1] AS [d] +LEFT JOIN ( + SELECT [d0].[Id], [d0].[DeDupeFilter1Id], [d0].[TenantX] + FROM [DeDupeFilter2] AS [d0] + WHERE [d0].[TenantX] = @__ef_filter__Tenant_0 +) AS [t] ON [d].[Id] = [t].[DeDupeFilter1Id] +LEFT JOIN ( + SELECT [d1].[Id], [d1].[DeDupeFilter1Id], [d1].[Tenant] + FROM [DeDupeFilter3] AS [d1] + WHERE [d1].[Tenant] = @__ef_filter__Tenant_0_1 +) AS [t0] ON [d].[Id] = [t0].[DeDupeFilter1Id] +WHERE [d].[Tenant] = @__ef_filter__Tenant_0 +ORDER BY [d].[Id], [t].[Id] +"""); + } + private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/QueryFilterFuncletizationSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/QueryFilterFuncletizationSqliteTest.cs index 647f857627d..7495795f99a 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/QueryFilterFuncletizationSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/QueryFilterFuncletizationSqliteTest.cs @@ -34,6 +34,34 @@ public override void DbContext_list_is_parameterized() Assert.Equal(2, query.Count); } + public override void Using_multiple_entities_with_filters_reuses_parameters() + { + base.Using_multiple_entities_with_filters_reuses_parameters(); + + AssertSql( +""" +@__ef_filter__Tenant_0='1' + +SELECT "d"."Id", "d"."Tenant", "t"."Id", "t"."DeDupeFilter1Id", "t"."TenantX", "t0"."Id", "t0"."DeDupeFilter1Id", "t0"."Tenant" +FROM "DeDupeFilter1" AS "d" +LEFT JOIN ( + SELECT "d0"."Id", "d0"."DeDupeFilter1Id", "d0"."TenantX" + FROM "DeDupeFilter2" AS "d0" + WHERE "d0"."TenantX" = @__ef_filter__Tenant_0 +) AS "t" ON "d"."Id" = "t"."DeDupeFilter1Id" +LEFT JOIN ( + SELECT "d1"."Id", "d1"."DeDupeFilter1Id", "d1"."Tenant" + FROM "DeDupeFilter3" AS "d1" + WHERE "d1"."Tenant" = @__ef_filter__Tenant_0 +) AS "t0" ON "d"."Id" = "t0"."DeDupeFilter1Id" +WHERE "d"."Tenant" = @__ef_filter__Tenant_0 +ORDER BY "d"."Id", "t"."Id" +"""); + } + + private void AssertSql(params string[] expected) + => Fixture.TestSqlLoggerFactory.AssertBaseline(expected); + public class QueryFilterFuncletizationSqliteFixture : QueryFilterFuncletizationRelationalFixture { protected override ITestStoreFactory TestStoreFactory