Don't duplicate query filter parameters

Fixes dotnet#24476
stevendarby · Mar 25, 2023 · 37cd3a1 · 37cd3a1
1 parent d4aa1ea
commit 37cd3a1
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 3 deletions.
diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
@@ -63,7 +63,7 @@ public ParameterExtractingExpressionVisitor(
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual Expression ExtractParameters(Expression expression)
-        => ExtractParameters(expression, clearEvaluatedValues: true);
+        => ExtractParameters(expression, clearEvaluatedValues: !_generateContextAccessors);
 
     private Expression ExtractParameters(Expression expression, bool clearEvaluatedValues)
     {
@@ -274,7 +274,10 @@ private Expression Evaluate(Expression expression, bool generateParameter)
         string? parameterName;
         if (_evaluatedValues.TryGetValue(expression, out var cachedValue))
         {
-            var existingExpression = generateParameter ? cachedValue.Parameter : cachedValue.Constant;
+            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
@@ -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);
+    }
 }
diff --git 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<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
diff --git 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
@@ -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