From 6e45ea2e967e0e7fe575f61e91fe83d1dbef30ba Mon Sep 17 00:00:00 2001 From: Joakim Riedel Date: Tue, 23 Feb 2021 22:16:31 +0100 Subject: [PATCH] Keep parameter names for inlined queries (#23437) Resolves #23271 --- .../ParameterExtractingExpressionVisitor.cs | 47 +++++++++++++---- .../Query/GearsOfWarQueryTestBase.cs | 29 +++++++++++ .../Query/GearsOfWarQuerySqlServerTest.cs | 45 +++++++++++++++- .../Query/TPTGearsOfWarQuerySqlServerTest.cs | 52 ++++++++++++++++++- 4 files changed, 162 insertions(+), 11 deletions(-) diff --git a/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs index 6ff016f902e..9b92757f082 100644 --- 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 _evaluatedValues = new(ExpressionEqualityComparer.Instance); + private readonly Dictionary _evaluatedValues = new(ExpressionEqualityComparer.Instance); private IDictionary _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. /// 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 index 11f571e6290..89d1c2ba1c9 100644 --- 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().Where(s => s.Id == squadId); + var outerQuery = ss.Set().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> predicate = s => s.SquadId == gearId; + + return AssertQuery( + async, + ss => ss.Set().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 index 59137096c07..eb53e67757b 100644 --- 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 index 5917ebc7d5b..b4874da642e 100644 --- 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]"); }