From da3d0a847817d6245d5947ad5ff267a3db2de996 Mon Sep 17 00:00:00 2001 From: Steve Date: Sat, 7 Dec 2019 12:13:14 -0500 Subject: [PATCH 1/2] Now replaces empty-string SqlConstant expressions with string.Empty and coalesces all nullable columns/parameters during a string concatenation --- ...NullSemanticsRewritingExpressionVisitor.cs | 24 +++++++++++++++++++ .../Query/GearsOfWarQueryTestBase.cs | 12 ++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs index 8d9c1f200c8..6097fe37784 100644 --- a/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Internal/NullSemanticsRewritingExpressionVisitor.cs @@ -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 + ? _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) { diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs index 3020bd67816..a7cd1103675 100644 --- a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs @@ -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().Select(w => w.FullName + null + w.LeaderNickname + nullableParam), + ss => ss.Set().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) From 7e510b81d419b717489fa78aa2752f1f51c5dad2 Mon Sep 17 00:00:00 2001 From: Steve Date: Mon, 9 Dec 2019 00:19:31 -0500 Subject: [PATCH 2/2] Fixes all tests broken by introducing string coalescing on nullable concat expressions --- .../ComplexNavigationsQuerySqlServerTest.cs | 2 +- .../Query/GearsOfWarQuerySqlServerTest.cs | 6 ++--- .../NorthwindDbFunctionsQuerySqlServerTest.cs | 2 +- ...orthwindMiscellaneousQuerySqlServerTest.cs | 26 +++++++++---------- .../Query/NorthwindWhereQuerySqlServerTest.cs | 4 +-- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs index cef34b937ca..68b6ddcf29f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs @@ -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 diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs index 1a704e273f4..b94f6bb2579 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs @@ -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() @@ -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) @@ -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) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs index c5bd122c50a..2ac33bca2ce 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs @@ -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] diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs index 6d84f6276b4..07f8c08ab9e 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs @@ -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] @@ -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]"); } @@ -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]"); } @@ -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) @@ -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]"); @@ -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%')"); @@ -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) @@ -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) @@ -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]"); @@ -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%')"); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs index a9c72b33b48..11da2e0a041 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs @@ -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) @@ -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)