Query: Cast result of IndexOf appropriately for SqlServer

Resolves #18772
dotnet · Dec 13, 2019 · daf8072 · daf8072
1 parent bdff0a1
commit daf8072
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 42 deletions.
diff --git 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)
         /// </summary>
         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));
         }
 
         /// <summary>
@@ -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
@@ -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
@@ -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<int>(e.NullableStringA, () => e.NullableStringA.IndexOf("oo"))).ToList();
+            var expected = _clientData._entities1.OrderBy(e => e.Id).Select(e => MaybeScalar<int>(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
@@ -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<MappedNullableDataTypes>().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<MappedNullableDataTypes>()
+                    .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
@@ -12,7 +12,10 @@ namespace Microsoft.EntityFrameworkCore.Query
 {
     public class NorthwindFunctionsQuerySqlServerTest : NorthwindFunctionsQueryTestBase<NorthwindQuerySqlServerFixture<NoopModelCustomizer>>
     {
-        public NorthwindFunctionsQuerySqlServerTest(NorthwindQuerySqlServerFixture<NoopModelCustomizer> fixture, ITestOutputHelper testOutputHelper)
+        public NorthwindFunctionsQuerySqlServerTest(
+#pragma warning disable IDE0060 // Remove unused parameter
+            NorthwindQuerySqlServerFixture<NoopModelCustomizer> 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
@@ -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
@@ -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
@@ -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(