Don't duplicate query filter parameters (#29422)

* Don't duplicate query filter parameters
Fixes #24476

---------

Co-authored-by: Steven.Darby <Steven.Darby@tribalgroup.com>

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

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)
+    /// <summary>
+    ///     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.
+    /// </summary>
+    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;

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<MultiContextFilter>().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<DeDupeFilter1>()
+            .Include(x => x.DeDupeFilter2s)
+            .Include(x => x.DeDupeFilter3s)
+            .ToList();
+
+        Assert.Single(query);
+        Assert.Single(query[0].DeDupeFilter2s);
+        Assert.Single(query[0].DeDupeFilter3s);
+    }
 }

test/EFCore.Specification.Tests/TestModels/QueryFilterFuncletizationContext.cs

@@ -74,6 +74,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
         // Multiple context used in filter
         modelBuilder.Entity<MultiContextFilter>()
             .HasQueryFilter(e => e.IsEnabled == Property && e.BossId == new IncorrectDbContext().BossId);
+
+        modelBuilder.Entity<DeDupeFilter1>().HasQueryFilter(x => x.Tenant == Tenant);
+        modelBuilder.Entity<DeDupeFilter2>().HasQueryFilter(x => x.TenantX == Tenant);
+        modelBuilder.Entity<DeDupeFilter3>().HasQueryFilter(x => x.Tenant == Tenant);
     }
 
     private void ConfigureFilter(EntityTypeBuilder<LocalMethodFilter> 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<DeDupeFilter2> { new() { TenantX = 1 }, new() { TenantX = 2 } },
+                DeDupeFilter3s = new List<DeDupeFilter3> { 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<DeDupeFilter2> DeDupeFilter2s { get; set; }
+    public ICollection<DeDupeFilter3> 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

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);
 

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