From daf8072bdf17ed9e7fe5fc380a740d33e2df50fe Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Fri, 13 Dec 2019 12:42:33 -0800 Subject: [PATCH] Query: Cast result of IndexOf appropriately for SqlServer Resolves #18772 --- .../Query/Internal/SqlExpressionFactory.cs | 13 ++-- .../SqlServerStringMethodTranslator.cs | 26 ++++++-- .../Query/NullSemanticsQueryTestBase.cs | 4 +- .../BuiltInDataTypesSqlServerTest.cs | 31 ++++++++++ .../NorthwindFunctionsQuerySqlServerTest.cs | 9 ++- .../NorthwindSelectQuerySqlServerTest.cs | 2 +- .../Query/NorthwindWhereQuerySqlServerTest.cs | 4 +- .../Query/NullSemanticsQuerySqlServerTest.cs | 59 +++++++++++-------- 8 files changed, 106 insertions(+), 42 deletions(-) diff --git a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs index 9024813d57e..5b122e47332 100644 --- a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs +++ b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs @@ -46,13 +46,10 @@ public SqlExpressionFactory([NotNull] ITypeMappingSource typeMappingSource) /// public virtual SqlExpression ApplyDefaultTypeMapping(SqlExpression sqlExpression) { - if (sqlExpression == null - || sqlExpression.TypeMapping != null) - { - return sqlExpression; - } - - return ApplyTypeMapping(sqlExpression, _typeMappingSource.FindMapping(sqlExpression.Type)); + return sqlExpression == null + || sqlExpression.TypeMapping != null + ? sqlExpression + : ApplyTypeMapping(sqlExpression, _typeMappingSource.FindMapping(sqlExpression.Type)); } /// @@ -69,7 +66,9 @@ public virtual SqlExpression ApplyTypeMapping(SqlExpression sqlExpression, CoreT return sqlExpression; } +#pragma warning disable IDE0066 // Convert switch statement to expression switch (sqlExpression) +#pragma warning restore IDE0066 // Convert switch statement to expression { case SqlConditionalExpression sqlConditionalExpression: return ApplyTypeMappingOnSqlConditional(sqlConditionalExpression, typeMapping); diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs index 1142f4d3677..584fe0e92c9 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs @@ -82,12 +82,27 @@ public virtual SqlExpression Translate(SqlExpression instance, MethodInfo method var stringTypeMapping = ExpressionExtensions.InferTypeMapping(instance, argument); argument = _sqlExpressionFactory.ApplyTypeMapping(argument, stringTypeMapping); - var charIndexExpression = _sqlExpressionFactory.Subtract( - _sqlExpressionFactory.Function( + SqlExpression charIndexExpression; + var storeType = stringTypeMapping.StoreType; + if (string.Equals(storeType, "nvarchar(max)", StringComparison.OrdinalIgnoreCase) + || string.Equals(storeType, "varchar(max)", StringComparison.OrdinalIgnoreCase)) + { + charIndexExpression = _sqlExpressionFactory.Function( "CHARINDEX", new[] { argument, _sqlExpressionFactory.ApplyTypeMapping(instance, stringTypeMapping) }, - method.ReturnType), - _sqlExpressionFactory.Constant(1)); + typeof(long)); + + charIndexExpression = _sqlExpressionFactory.Convert(charIndexExpression, typeof(int)); + } + else + { + charIndexExpression = _sqlExpressionFactory.Function( + "CHARINDEX", + new[] { argument, _sqlExpressionFactory.ApplyTypeMapping(instance, stringTypeMapping) }, + method.ReturnType); + } + + charIndexExpression = _sqlExpressionFactory.Subtract(charIndexExpression, _sqlExpressionFactory.Constant(1)); return _sqlExpressionFactory.Case( new[] @@ -218,7 +233,10 @@ public virtual SqlExpression Translate(SqlExpression instance, MethodInfo method if (pattern is SqlConstantExpression constantPattern) { + // Intentionally string.Empty since we don't want to match nulls here. +#pragma warning disable CA1820 // Test for empty strings using string length if ((string)constantPattern.Value == string.Empty) +#pragma warning restore CA1820 // Test for empty strings using string length { return _sqlExpressionFactory.Constant(true); } diff --git a/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs index 830dd8f0f51..57f89a74ffa 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs @@ -801,12 +801,12 @@ public virtual void Where_IndexOf_empty() useRelationalNulls: false); } - [ConditionalFact(Skip = "issue #18772")] + [ConditionalFact] public virtual void Select_IndexOf() { using var ctx = CreateContext(); var query = ctx.Entities1.OrderBy(e => e.Id).Select(e => e.NullableStringA.IndexOf("oo")).ToList(); - var expected = _clientData._entities1.OrderBy(e => e.Id).Select(e => MaybeScalar(e.NullableStringA, () => e.NullableStringA.IndexOf("oo"))).ToList(); + var expected = _clientData._entities1.OrderBy(e => e.Id).Select(e => MaybeScalar(e.NullableStringA, () => e.NullableStringA.IndexOf("oo")) ?? 0).ToList(); for (var i = 0; i < query.Count; i++) { diff --git a/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs index 1f0ac1b936e..69db9a4dcb3 100644 --- a/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs @@ -87,6 +87,37 @@ FROM [MappedNullableDataTypes] AS [m] WHERE [m].[TimeSpanAsTime] = @__timeSpan_0"); } + [ConditionalFact] + public void String_indexOf_over_varchar_max() + { + using (var context = CreateContext()) + { + context.Set().Add( + new MappedNullableDataTypes { Int = 81, StringAsVarcharMax = string.Concat(Enumerable.Repeat("C", 8001)) }); + + Assert.Equal(1, context.SaveChanges()); + } + + Fixture.TestSqlLoggerFactory.Clear(); + + using (var context = CreateContext()) + { + var results = context.Set() + .Where(e => e.Int == 81) + .Select(m => m.StringAsVarcharMax.IndexOf("a")) + .ToList(); + + Assert.Equal(-1, Assert.Single(results)); + AssertSql( + @"SELECT CASE + WHEN 'a' = '' THEN 0 + ELSE CAST(CHARINDEX('a', [m].[StringAsVarcharMax]) AS int) - 1 +END +FROM [MappedNullableDataTypes] AS [m] +WHERE [m].[Int] = 81"); + } + } + [ConditionalFact] public virtual void Can_query_using_DateDiffHour_using_TimeSpan() { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs index 46c50276930..7fd2e5eddc1 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs @@ -12,7 +12,10 @@ namespace Microsoft.EntityFrameworkCore.Query { public class NorthwindFunctionsQuerySqlServerTest : NorthwindFunctionsQueryTestBase> { - public NorthwindFunctionsQuerySqlServerTest(NorthwindQuerySqlServerFixture fixture, ITestOutputHelper testOutputHelper) + public NorthwindFunctionsQuerySqlServerTest( +#pragma warning disable IDE0060 // Remove unused parameter + NorthwindQuerySqlServerFixture fixture, ITestOutputHelper testOutputHelper) +#pragma warning restore IDE0060 // Remove unused parameter : base(fixture) { ClearLog(); @@ -1180,7 +1183,7 @@ public override async Task Indexof_with_emptystring(bool async) AssertSql( @"SELECT CASE WHEN N'' = N'' THEN 0 - ELSE CHARINDEX(N'', [c].[ContactName]) - 1 + ELSE CAST(CHARINDEX(N'', [c].[ContactName]) AS int) - 1 END FROM [Customers] AS [c] WHERE [c].[CustomerID] = N'ALFKI'"); @@ -1245,7 +1248,7 @@ public override async Task Substring_with_Index_of(bool async) AssertSql( @"SELECT SUBSTRING([c].[ContactName], CASE WHEN N'a' = N'' THEN 0 - ELSE CHARINDEX(N'a', [c].[ContactName]) - 1 + ELSE CAST(CHARINDEX(N'a', [c].[ContactName]) AS int) - 1 END + 1, 3) FROM [Customers] AS [c] WHERE [c].[CustomerID] = N'ALFKI'"); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs index 535109191e3..bf03dac44c0 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs @@ -1169,7 +1169,7 @@ public override async Task Select_with_complex_expression_that_can_be_funcletize AssertSql( @"SELECT CASE WHEN N'' = N'' THEN 0 - ELSE CHARINDEX(N'', [c].[ContactName]) - 1 + ELSE CAST(CHARINDEX(N'', [c].[ContactName]) AS int) - 1 END FROM [Customers] AS [c] WHERE [c].[CustomerID] = N'ALFKI'"); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs index 11da2e0a041..f4f88a3d620 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs @@ -669,10 +669,10 @@ public override async Task Where_string_indexof(bool async) FROM [Customers] AS [c] WHERE (CASE WHEN N'Sea' = N'' THEN 0 - ELSE CHARINDEX(N'Sea', [c].[City]) - 1 + ELSE CAST(CHARINDEX(N'Sea', [c].[City]) AS int) - 1 END <> -1) OR CASE WHEN N'Sea' = N'' THEN 0 - ELSE CHARINDEX(N'Sea', [c].[City]) - 1 + ELSE CAST(CHARINDEX(N'Sea', [c].[City]) AS int) - 1 END IS NULL"); } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs index 4180492bea1..6b4f413188f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs @@ -1217,33 +1217,33 @@ public override void Null_semantics_applied_when_comparing_function_with_nullabl FROM [Entities1] AS [e] WHERE (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END = [e].[NullableIntA]) OR (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NULL AND [e].[NullableIntA] IS NULL)", // @"SELECT [e].[Id] FROM [Entities1] AS [e] WHERE (CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringA]) AS int) - 1 END = [e].[NullableIntA]) OR (CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringA]) AS int) - 1 END IS NULL AND [e].[NullableIntA] IS NULL)", // @"SELECT [e].[Id] FROM [Entities1] AS [e] WHERE ((CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END <> [e].[NullableIntB]) OR (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NULL OR [e].[NullableIntB] IS NULL)) AND (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NOT NULL OR [e].[NullableIntB] IS NOT NULL)"); // issue #18773 // AssertSql( @@ -1268,6 +1268,19 @@ public override void Where_IndexOf_empty() @""); } + public override void Select_IndexOf() + { + base.Select_IndexOf(); + + AssertSql( + @"SELECT CASE + WHEN N'oo' = N'' THEN 0 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 +END +FROM [Entities1] AS [e] +ORDER BY [e].[Id]"); + } + public override void Null_semantics_applied_when_comparing_two_functions_with_nullable_arguments() { base.Null_semantics_applied_when_comparing_two_functions_with_nullable_arguments(); @@ -1277,60 +1290,60 @@ public override void Null_semantics_applied_when_comparing_two_functions_with_nu FROM [Entities1] AS [e] WHERE (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END = CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringB]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringB]) AS int) - 1 END) OR (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NULL AND CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringB]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringB]) AS int) - 1 END IS NULL)", // @"SELECT [e].[Id] FROM [Entities1] AS [e] WHERE ((CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END <> CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringB]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringB]) AS int) - 1 END) OR (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NULL OR CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringB]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringB]) AS int) - 1 END IS NULL)) AND (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NOT NULL OR CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringB]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringB]) AS int) - 1 END IS NOT NULL)", // @"SELECT [e].[Id] FROM [Entities1] AS [e] WHERE ((CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END <> CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringA]) AS int) - 1 END) OR (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NULL OR CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringA]) AS int) - 1 END IS NULL)) AND (CASE WHEN N'oo' = N'' THEN 0 - ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 END IS NOT NULL OR CASE WHEN N'ar' = N'' THEN 0 - ELSE CHARINDEX(N'ar', [e].[NullableStringA]) - 1 + ELSE CAST(CHARINDEX(N'ar', [e].[NullableStringA]) AS int) - 1 END IS NOT NULL)"); // issue #18773 // AssertSql(