Query: Reuse parameters for inlined queries coming from same closure …

…variable Resolves #23271
dotnet · Feb 22, 2021 · ea829ed · ea829ed
1 parent 15115d8
commit ea829ed
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 10 deletions.
diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
@@ -34,7 +34,7 @@ public class ParameterExtractingExpressionVisitor : ExpressionVisitor
         private readonly EvaluatableExpressionFindingExpressionVisitor _evaluatableExpressionFindingExpressionVisitor;
         private readonly ContextParameterReplacingExpressionVisitor _contextParameterReplacingExpressionVisitor;
 
-        private readonly Dictionary<Expression, Expression> _evaluatedValues = new(ExpressionEqualityComparer.Instance);
+        private readonly Dictionary<Expression, EvaluatedValues> _evaluatedValues = new(ExpressionEqualityComparer.Instance);
 
         private IDictionary<Expression, bool> _evaluatableExpressions;
         private IQueryProvider? _currentQueryProvider;
@@ -76,6 +76,11 @@ public ParameterExtractingExpressionVisitor(
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         public virtual Expression ExtractParameters([NotNull] Expression expression)
+        {
+            return ExtractParameters(expression, clearEvaluatedValues: true);
+        }
+
+        private Expression ExtractParameters([NotNull] Expression expression, bool clearEvaluatedValues = false)
         {
             var oldEvaluatableExpressions = _evaluatableExpressions;
             _evaluatableExpressions = _evaluatableExpressionFindingExpressionVisitor.Find(expression);
@@ -87,7 +92,10 @@ public virtual Expression ExtractParameters([NotNull] Expression expression)
             finally
             {
                 _evaluatableExpressions = oldEvaluatableExpressions;
-                _evaluatedValues.Clear();
+                if (clearEvaluatedValues)
+                {
+                    _evaluatedValues.Clear();
+                }
             }
         }
 
@@ -280,16 +288,29 @@ private static Expression GenerateConstantExpression(object? value, Type returnT
 
         private Expression Evaluate(Expression expression, bool generateParameter)
         {
+            object? parameterValue;
+            string? parameterName;
             if (_evaluatedValues.TryGetValue(expression, out var cachedValue))
             {
-                return cachedValue;
-            }
+                var existingExpression = generateParameter ? cachedValue.Parameter : cachedValue.Constant;
+                if (existingExpression != null)
+                {
+                    return existingExpression;
+                }
 
-            var parameterValue = GetValue(expression, out var parameterName);
+                parameterValue = cachedValue.Value;
+                parameterName = cachedValue.CandidateParameterName;
+            }
+            else
+            {
+                parameterValue = GetValue(expression, out parameterName);
+                cachedValue = new EvaluatedValues { CandidateParameterName = parameterName, Value = parameterValue };
+                _evaluatedValues[expression] = cachedValue;
+            }
 
             if (parameterValue is IQueryable innerQueryable)
             {
-                return ExtractParameters(innerQueryable.Expression);
+                return ExtractParameters(innerQueryable.Expression, clearEvaluatedValues: false);
             }
 
             if (parameterName?.StartsWith(QueryFilterPrefix, StringComparison.Ordinal) != true)
@@ -303,7 +324,7 @@ private Expression Evaluate(Expression expression, bool generateParameter)
                 {
                     var constantValue = GenerateConstantExpression(parameterValue, expression.Type);
 
-                    _evaluatedValues.Add(expression, constantValue);
+                    cachedValue.Constant = constantValue;
 
                     return constantValue;
                 }
@@ -337,7 +358,7 @@ var compilerPrefixIndex
 
             var parameter = Expression.Parameter(expression.Type, parameterName);
 
-            _evaluatedValues.Add(expression, parameter);
+            cachedValue.Parameter = parameter;
 
             return parameter;
         }
@@ -645,5 +666,13 @@ private static bool IsQueryableMethod(Expression expression)
                 => expression is MethodCallExpression methodCallExpression
                     && methodCallExpression.Method.DeclaringType == typeof(Queryable);
         }
+
+        private class EvaluatedValues
+        {
+            public string? CandidateParameterName { get; set; }
+            public object? Value { get; set; }
+            public Expression? Constant { get; set; }
+            public Expression? Parameter { get; set; }
+        }
     }
 }
diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
@@ -6684,6 +6684,23 @@ public virtual Task Query_reusing_parameter_doesnt_declare_duplicate_parameter(b
                 assertOrder: true);
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(bool async)
+        {
+            var squadId = 1;
+
+            return AssertQuery(
+                async,
+                ss =>
+                {
+                    var innerQuery = ss.Set<Squad>().Where(s => s.Id == squadId);
+                    var outerQuery = ss.Set<Gear>().Where(g => innerQuery.Contains(g.Squad));
+                    return outerQuery.Concat(outerQuery).OrderBy(g => g.FullName);
+                },
+                assertOrder: true);
+        }
+
         [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(bool async)

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs
@@ -6281,6 +6281,34 @@ FROM [Gears] AS [g]
 ORDER BY [t].[FullName]");
         }
 
+        public override async Task Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(bool async)
+        {
+            await base.Query_reusing_parameter_with_inner_query_doesnt_declare_duplicate_parameter(async);
+
+            AssertSql(
+                @"@__squadId_0='1'
+
+SELECT [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[Discriminator], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank]
+FROM (
+    SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
+    FROM [Gears] AS [g]
+    INNER JOIN [Squads] AS [s] ON [g].[SquadId] = [s].[Id]
+    WHERE EXISTS (
+        SELECT 1
+        FROM [Squads] AS [s0]
+        WHERE ([s0].[Id] = @__squadId_0) AND ([s0].[Id] = [s].[Id]))
+    UNION ALL
+    SELECT [g0].[Nickname], [g0].[SquadId], [g0].[AssignedCityName], [g0].[CityOfBirthName], [g0].[Discriminator], [g0].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank]
+    FROM [Gears] AS [g0]
+    INNER JOIN [Squads] AS [s1] ON [g0].[SquadId] = [s1].[Id]
+    WHERE EXISTS (
+        SELECT 1
+        FROM [Squads] AS [s2]
+        WHERE ([s2].[Id] = @__squadId_0) AND ([s2].[Id] = [s1].[Id]))
+) AS [t]
+ORDER BY [t].[FullName]");
+        }
+
         public override async Task Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(bool async)
         {
             await base.Query_reusing_parameter_doesnt_declare_duplicate_parameter_complex(async);
@@ -7094,7 +7122,7 @@ public override async Task Constant_enum_with_same_underlying_value_as_previousl
             AssertSql(
                 @"@__p_0='1'
 
-SELECT TOP(@__p_0) [g].[Rank] & @__p_0
+SELECT TOP(@__p_0) [g].[Rank] & 1
 FROM [Gears] AS [g]
 ORDER BY [g].[Nickname]");
         }

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs
@@ -8242,7 +8242,7 @@ public override async Task Constant_enum_with_same_underlying_value_as_previousl
             AssertSql(
                 @"@__p_0='1'
 
-SELECT TOP(@__p_0) [g].[Rank] & @__p_0
+SELECT TOP(@__p_0) [g].[Rank] & 1
 FROM [Gears] AS [g]
 ORDER BY [g].[Nickname]");
         }