Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#3836 - Coalescing string concatenation on nullable Expressions #19239

Merged
merged 3 commits into from
Dec 11, 2019
Merged

#3836 - Coalescing string concatenation on nullable Expressions #19239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f90018
Merge pull request #1 from aspnet/master
svengeance Dec 7, 2019
da3d0a8
Now replaces empty-string SqlConstant expressions with string.Empty a…
svengeance Dec 7, 2019
7e510b8
Fixes all tests broken by introducing string coalescing on nullable c…
svengeance Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,30 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres
return sqlBinaryExpression.Update(newLeft, newRight);
}

if (sqlBinaryExpression.OperatorType == ExpressionType.Add
&& sqlBinaryExpression.Type == typeof(string))
{
if (leftNullable)
{
newLeft = newLeft is SqlConstantExpression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maumar - Introduce local function for this.

? _sqlExpressionFactory.Constant(string.Empty)
: newLeft is ColumnExpression || newLeft is SqlParameterExpression
? _sqlExpressionFactory.Coalesce(newLeft, _sqlExpressionFactory.Constant(string.Empty))
: newLeft;
}

if (rightNullable)
{
newRight = newRight is SqlConstantExpression
? _sqlExpressionFactory.Constant(string.Empty)
: newRight is ColumnExpression || newRight is SqlParameterExpression
? _sqlExpressionFactory.Coalesce(newRight, _sqlExpressionFactory.Constant(string.Empty))
: newRight;
}

return sqlBinaryExpression.Update(newLeft, newRight);
}

if (sqlBinaryExpression.OperatorType == ExpressionType.Equal
|| sqlBinaryExpression.OperatorType == ExpressionType.NotEqual)
{
Expand Down
12 changes: 12 additions & 0 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5886,6 +5886,18 @@ public virtual Task String_concat_with_null_conditional_argument2(bool async)
assertOrder: true);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task String_concat_nullable_expressions_are_coalesced(bool async)
{
object nullableParam = null;

return AssertQuery(
async,
ss => ss.Set<Gear>().Select(w => w.FullName + null + w.LeaderNickname + nullableParam),
ss => ss.Set<Gear>().Select(w => w.FullName + string.Empty + w.LeaderNickname ?? string.Empty + nullableParam ?? string.Empty));
}

[ConditionalTheory(Skip = "issue #14205")]
[MemberData(nameof(IsAsyncData))]
public virtual Task String_concat_on_various_types(bool async)
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3097,7 +3097,7 @@ public override async Task Select_optional_navigation_property_string_concat(boo
await base.Select_optional_navigation_property_string_concat(async);

AssertSql(
@"SELECT ([l].[Name] + N' ') + CASE
@"SELECT (COALESCE([l].[Name], N'') + N' ') + CASE
WHEN [t].[Id] IS NOT NULL THEN [t].[Name]
ELSE N'NULL'
END
Expand Down
6 changes: 3 additions & 3 deletions test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1871,7 +1871,7 @@ public override async Task Non_unicode_string_literals_is_used_for_non_unicode_c
AssertSql(
@"SELECT [c].[Name], [c].[Location], [c].[Nation]
FROM [Cities] AS [c]
WHERE CHARINDEX('Add', [c].[Location] + 'Added') > 0");
WHERE CHARINDEX('Add', COALESCE([c].[Location], '') + 'Added') > 0");
}

public override void Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result1()
Expand Down Expand Up @@ -6014,7 +6014,7 @@ public override async Task String_concat_with_null_conditional_argument(bool asy
@"SELECT [w0].[Id], [w0].[AmmunitionType], [w0].[IsAutomatic], [w0].[Name], [w0].[OwnerFullName], [w0].[SynergyWithId]
FROM [Weapons] AS [w]
LEFT JOIN [Weapons] AS [w0] ON [w].[SynergyWithId] = [w0].[Id]
ORDER BY [w0].[Name] + CAST(5 AS nvarchar(max))");
ORDER BY COALESCE([w0].[Name], N'') + CAST(5 AS nvarchar(max))");
}

public override async Task String_concat_with_null_conditional_argument2(bool async)
Expand All @@ -6025,7 +6025,7 @@ public override async Task String_concat_with_null_conditional_argument2(bool as
@"SELECT [w0].[Id], [w0].[AmmunitionType], [w0].[IsAutomatic], [w0].[Name], [w0].[OwnerFullName], [w0].[SynergyWithId]
FROM [Weapons] AS [w]
LEFT JOIN [Weapons] AS [w0] ON [w].[SynergyWithId] = [w0].[Id]
ORDER BY [w0].[Name] + N'Marcus'' Lancer'");
ORDER BY COALESCE([w0].[Name], N'') + N'Marcus'' Lancer'");
}

public override async Task String_concat_on_various_types(bool async)
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ public virtual void IsDate_join_fields()
AssertSql(
@"SELECT COUNT(*)
FROM [Orders] AS [o]
WHERE CAST(ISDATE([o].[CustomerID] + CAST([o].[OrderID] AS nchar(5))) AS bit) = CAST(1 AS bit)");
WHERE CAST(ISDATE(COALESCE([o].[CustomerID], N'') + CAST([o].[OrderID] AS nchar(5))) AS bit) = CAST(1 AS bit)");
}

[ConditionalFact]
Expand Down
26 changes: 13 additions & 13 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ public override async Task Join_Customers_Orders_Projection_With_String_Concat_S
@"@__p_0='10'
@__p_1='5'

SELECT ([c].[ContactName] + N' ') + [c].[ContactTitle] AS [Contact], [o].[OrderID]
SELECT (COALESCE([c].[ContactName], N'') + N' ') + COALESCE([c].[ContactTitle], N'') AS [Contact], [o].[OrderID]
FROM [Customers] AS [c]
INNER JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
ORDER BY [o].[OrderID]
Expand Down Expand Up @@ -2756,7 +2756,7 @@ public override async Task String_concat_with_navigation1(bool async)
await base.String_concat_with_navigation1(async);

AssertSql(
@"SELECT ([o].[CustomerID] + N' ') + [c].[City]
@"SELECT (COALESCE([o].[CustomerID], N'') + N' ') + COALESCE([c].[City], N'')
FROM [Orders] AS [o]
LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID]");
}
Expand All @@ -2766,7 +2766,7 @@ public override async Task String_concat_with_navigation2(bool async)
await base.String_concat_with_navigation2(async);

AssertSql(
@"SELECT ([c].[City] + N' ') + [c].[City]
@"SELECT (COALESCE([c].[City], N'') + N' ') + COALESCE([c].[City], N'')
FROM [Orders] AS [o]
LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID]");
}
Expand Down Expand Up @@ -3665,9 +3665,9 @@ public override async Task Anonymous_complex_distinct_where(bool async)
await base.Anonymous_complex_distinct_where(async);

AssertSql(
@"SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [A]
@"SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [A]
FROM [Customers] AS [c]
WHERE ([c].[CustomerID] + [c].[City]) = N'ALFKIBerlin'");
WHERE ([c].[CustomerID] + COALESCE([c].[City], N'')) = N'ALFKIBerlin'");
}

public override async Task Anonymous_complex_distinct_orderby(bool async)
Expand All @@ -3677,7 +3677,7 @@ public override async Task Anonymous_complex_distinct_orderby(bool async)
AssertSql(
@"SELECT [t].[c] AS [A]
FROM (
SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [c]
SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [c]
FROM [Customers] AS [c]
) AS [t]
ORDER BY [t].[c]");
Expand All @@ -3690,7 +3690,7 @@ public override async Task Anonymous_complex_distinct_result(bool async)
AssertSql(
@"SELECT COUNT(*)
FROM (
SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [c]
SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [c]
FROM [Customers] AS [c]
) AS [t]
WHERE [t].[c] IS NOT NULL AND ([t].[c] LIKE N'A%')");
Expand All @@ -3701,9 +3701,9 @@ public override async Task Anonymous_complex_orderby(bool async)
await base.Anonymous_complex_orderby(async);

AssertSql(
@"SELECT [c].[CustomerID] + [c].[City] AS [A]
@"SELECT [c].[CustomerID] + COALESCE([c].[City], N'') AS [A]
FROM [Customers] AS [c]
ORDER BY [c].[CustomerID] + [c].[City]");
ORDER BY [c].[CustomerID] + COALESCE([c].[City], N'')");
}

public override async Task Anonymous_subquery_orderby(bool async)
Expand Down Expand Up @@ -3769,9 +3769,9 @@ public override async Task DTO_complex_distinct_where(bool async)
await base.DTO_complex_distinct_where(async);

AssertSql(
@"SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [Property]
@"SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [Property]
FROM [Customers] AS [c]
WHERE ([c].[CustomerID] + [c].[City]) = N'ALFKIBerlin'");
WHERE ([c].[CustomerID] + COALESCE([c].[City], N'')) = N'ALFKIBerlin'");
}

public override async Task DTO_complex_distinct_orderby(bool async)
Expand All @@ -3781,7 +3781,7 @@ public override async Task DTO_complex_distinct_orderby(bool async)
AssertSql(
@"SELECT [t].[c] AS [Property]
FROM (
SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [c]
SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [c]
FROM [Customers] AS [c]
) AS [t]
ORDER BY [t].[c]");
Expand All @@ -3794,7 +3794,7 @@ public override async Task DTO_complex_distinct_result(bool async)
AssertSql(
@"SELECT COUNT(*)
FROM (
SELECT DISTINCT [c].[CustomerID] + [c].[City] AS [c]
SELECT DISTINCT [c].[CustomerID] + COALESCE([c].[City], N'') AS [c]
FROM [Customers] AS [c]
) AS [t]
WHERE [t].[c] IS NOT NULL AND ([t].[c] LIKE N'A%')");
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,7 @@ public override async Task Where_concat_string_string_comparison(bool async)

SELECT [c].[CustomerID]
FROM [Customers] AS [c]
WHERE (@__i_0 + [c].[CustomerID]) = [c].[CompanyName]");
WHERE (COALESCE(@__i_0, N'') + [c].[CustomerID]) = [c].[CompanyName]");
}

public override async Task Where_string_concat_method_comparison(bool async)
Expand All @@ -1345,7 +1345,7 @@ public override async Task Where_string_concat_method_comparison(bool async)

SELECT [c].[CustomerID]
FROM [Customers] AS [c]
WHERE (@__i_0 + [c].[CustomerID]) = [c].[CompanyName]");
WHERE (COALESCE(@__i_0, N'') + [c].[CustomerID]) = [c].[CompanyName]");
}

public override async Task Where_ternary_boolean_condition_true(bool async)
Expand Down