Skip to content

Commit

Permalink
Fix to #18916 - Query: regression in EFCore 3.1 Preview 2 and Preview…
Browse files Browse the repository at this point in the history
… 3 for query with conditional expression whose element is a comparison containing null-value parameter

Problem was that now we are peeking into parameter values and simplifying the queries. As a result comparisons of non-nullable expressions with null-value parameter are optimized out to a simple constant false. Sql server expects search condition in some places (e.g. WHERE, HAVING or WHEN parts of CASE statements).
Before we look into parameter values we run SearchConditionConvertingExpressionVisitor which fixes those cases. However, this visitor is currently sql server specific, and the part of the pipeline that looks into parameter values is not provider specific, so we can't re-run it.

Fix is to manually convert constants encountered in those areas into conditions. We used to do it already for WHERE and HAVING but we missed WHEN.

Resolves #18916
  • Loading branch information
maumar committed Nov 19, 2019
1 parent 988a8a1 commit 5895118
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,21 @@ protected override Expression VisitExtension(Expression extensionExpression)
var newPredicate = newSelectExpression.Predicate;
var newHaving = newSelectExpression.Having;
if (newSelectExpression.Predicate is SqlConstantExpression predicateConstantExpression
&& predicateConstantExpression.Value is bool predicateBoolValue
&& !predicateBoolValue)
&& predicateConstantExpression.Value is bool predicateBoolValue)
{
changed = true;
newPredicate = SqlExpressionFactory.Equal(
predicateConstantExpression,
SqlExpressionFactory.Constant(true, predicateConstantExpression.TypeMapping));
newPredicate = predicateBoolValue
? null
: GenerateEqualTrueExpression(predicateConstantExpression);
}

if (newSelectExpression.Having is SqlConstantExpression havingConstantExpression
&& havingConstantExpression.Value is bool havingBoolValue
&& !havingBoolValue)
&& havingConstantExpression.Value is bool havingBoolValue)
{
changed = true;
newHaving = SqlExpressionFactory.Equal(
havingConstantExpression,
SqlExpressionFactory.Constant(true, havingConstantExpression.TypeMapping));
newHaving = havingBoolValue
? null
: GenerateEqualTrueExpression(havingConstantExpression);
}

return changed
Expand All @@ -122,7 +120,42 @@ protected override Expression VisitExtension(Expression extensionExpression)
: newSelectExpression;
}

if (newExpression is CaseExpression newCaseExpression
&& newCaseExpression.Operand == null)
{
var changed = false;
var newWhenClauses = new List<CaseWhenClause>();
for (var i = 0; i < newCaseExpression.WhenClauses.Count; i++)
{
if (newCaseExpression.WhenClauses[i].Test is SqlConstantExpression whenConstantTestExpression
&& whenConstantTestExpression.Value is bool)
{
changed = true;
newWhenClauses.Add(
new CaseWhenClause(
GenerateEqualTrueExpression(whenConstantTestExpression),
newCaseExpression.WhenClauses[i].Result));
}
else
{
newWhenClauses.Add(newCaseExpression.WhenClauses[i]);
}
}

return changed
? newCaseExpression.Update(
newCaseExpression.Operand,
newWhenClauses,
newCaseExpression.ElseResult)
: newCaseExpression;
}

return newExpression;

SqlExpression GenerateEqualTrueExpression(SqlExpression argument)
=> SqlExpressionFactory.Equal(
argument,
SqlExpressionFactory.Constant(true, argument.TypeMapping));
}

protected override Expression VisitSqlUnaryExpression(SqlUnaryExpression sqlUnaryExpression)
Expand Down
30 changes: 30 additions & 0 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7547,6 +7547,36 @@ public virtual Task FirstOrDefault_navigation_access_entity_equality_in_where_pr
.Where(f => f.Capital == ss.Set<Gear>().OrderBy(s => s.Nickname).FirstOrDefault().CityOfBirth));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Conditional_expression_with_test_being_simplified_to_constant_simple(bool isAsync)
{
var prm = true;
var prm2 = (string)null;

return AssertQuery(
isAsync,
ss => ss.Set<Gear>().Where(g => g.HasSoulPatch == prm
? true
: g.CityOfBirthName == prm2));
}


[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Conditional_expression_with_test_being_simplified_to_constant_complex(bool isAsync)
{
var prm = true;
var prm2 = "Dom's Lancer";
var prm3 = (string)null;

return AssertQuery(
isAsync,
ss => ss.Set<Gear>().Where(g => g.HasSoulPatch == prm
? ss.Set<Weapon>().Where(w => w.Id == g.SquadId).Single().Name == prm2
: g.CityOfBirthName == prm3));
}

protected async Task AssertTranslationFailed(Func<Task> testCode)
{
Assert.Contains(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7504,6 +7504,52 @@ WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
ORDER BY [g].[Nickname]) IS NULL))");
}

public override async Task Conditional_expression_with_test_being_simplified_to_constant_simple(bool isAsync)
{
await base.Conditional_expression_with_test_being_simplified_to_constant_simple(isAsync);

AssertSql(
@"@__prm_0='True'
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]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND (CASE
WHEN [g].[HasSoulPatch] = @__prm_0 THEN CAST(1 AS bit)
ELSE CASE
WHEN CAST(0 AS bit) = CAST(1 AS bit) THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
END = CAST(1 AS bit))");
}

public override async Task Conditional_expression_with_test_being_simplified_to_constant_complex(bool isAsync)
{
await base.Conditional_expression_with_test_being_simplified_to_constant_complex(isAsync);

AssertSql(
@"@__prm_0='True'
@__prm2_1='Dom's Lancer' (Size = 4000)
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]
WHERE [g].[Discriminator] IN (N'Gear', N'Officer') AND (CASE
WHEN [g].[HasSoulPatch] = @__prm_0 THEN CASE
WHEN ((
SELECT TOP(1) [w].[Name]
FROM [Weapons] AS [w]
WHERE [w].[Id] = [g].[SquadId]) = @__prm2_1) AND (
SELECT TOP(1) [w].[Name]
FROM [Weapons] AS [w]
WHERE [w].[Id] = [g].[SquadId]) IS NOT NULL THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
ELSE CASE
WHEN CAST(0 AS bit) = CAST(1 AS bit) THEN CAST(1 AS bit)
ELSE CAST(0 AS bit)
END
END = CAST(1 AS bit))");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
}
Expand Down

0 comments on commit 5895118

Please sign in to comment.