Keep parameter names for inlined queries (#23437)

Resolves #23271
dotnet · Feb 23, 2021 · 6e45ea2 · 6e45ea2
1 parent 3278828
commit 6e45ea2
Show file tree

Hide file tree

Showing 4 changed files with 162 additions and 11 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)
         {
             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,30 +288,43 @@ 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)
             {
                 if (parameterValue is Expression innerExpression)
                 {
-                    return ExtractParameters(innerExpression);
+                    return ExtractParameters(innerExpression, clearEvaluatedValues: false);
                 }
 
                 if (!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 sealed 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,35 @@ 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_with_inner_query_expression_doesnt_declare_duplicate_parameter(bool async)
+        {
+            var gearId = 1;
+            Expression<Func<Gear, bool>> predicate = s => s.SquadId == gearId;
+
+            return AssertQuery(
+                async,
+                ss => ss.Set<Squad>().Where(s => s.Members.AsQueryable().Where(predicate).Where(predicate).Any()));
+        }
+
         [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,49 @@ 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_with_inner_query_expression_doesnt_declare_duplicate_parameter(bool async)
+        {
+            await base.Query_reusing_parameter_with_inner_query_expression_doesnt_declare_duplicate_parameter(async);
+
+            AssertSql(
+                @"@__gearId_0='1'
+
+SELECT [s].[Id], [s].[Banner], [s].[Banner5], [s].[InternalNumber], [s].[Name]
+FROM [Squads] AS [s]
+WHERE EXISTS (
+    SELECT 1
+    FROM [Gears] AS [g]
+    WHERE (([s].[Id] = [g].[SquadId]) AND ([g].[SquadId] = @__gearId_0)) AND ([g].[SquadId] = @__gearId_0))");
+        }
+
         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 +7137,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
@@ -7322,6 +7322,56 @@ LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[Squad
 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].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank], [t].[Discriminator]
+FROM (
+    SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank], CASE
+        WHEN [o].[Nickname] IS NOT NULL THEN N'Officer'
+    END AS [Discriminator]
+    FROM [Gears] AS [g]
+    LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[SquadId] = [o].[SquadId])
+    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].[FullName], [g0].[HasSoulPatch], [g0].[LeaderNickname], [g0].[LeaderSquadId], [g0].[Rank], CASE
+        WHEN [o0].[Nickname] IS NOT NULL THEN N'Officer'
+    END AS [Discriminator]
+    FROM [Gears] AS [g0]
+    LEFT JOIN [Officers] AS [o0] ON ([g0].[Nickname] = [o0].[Nickname]) AND ([g0].[SquadId] = [o0].[SquadId])
+    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_with_inner_query_expression_doesnt_declare_duplicate_parameter(bool async)
+        {
+            await base.Query_reusing_parameter_with_inner_query_expression_doesnt_declare_duplicate_parameter(async);
+
+            AssertSql(
+                @"@__gearId_0='1'
+
+SELECT [s].[Id], [s].[Banner], [s].[Banner5], [s].[InternalNumber], [s].[Name]
+FROM [Squads] AS [s]
+WHERE EXISTS (
+    SELECT 1
+    FROM [Gears] AS [g]
+    LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[SquadId] = [o].[SquadId])
+    WHERE (([s].[Id] = [g].[SquadId]) AND ([g].[SquadId] = @__gearId_0)) AND ([g].[SquadId] = @__gearId_0))");
+        }
+
         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);
@@ -8242,7 +8292,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]");
         }