Fixes and cleanup around JSON collection SQL value conversions

Related to dotnet#30727

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -2772,7 +2772,7 @@ public RelationalInferredTypeMappingApplier(
         /// <returns>Whether an inferred type mapping could be found.</returns>
         protected virtual bool TryGetInferredTypeMapping(
             TableExpressionBase table,
-             string columnName,
+            string columnName,
             [NotNullWhen(true)] out RelationalTypeMapping? inferredTypeMapping)
         {
             if (_inferredTypeMappings.TryGetValue((table, columnName), out inferredTypeMapping))

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -290,6 +290,9 @@
   <data name="NoSavepointRelease" xml:space="preserve">
     <value>SQL Server does not support releasing a savepoint.</value>
   </data>
+  <data name="QueryingOrderedBinaryJsonCollectionsNotSupported" xml:space="preserve">
+    <value>The query is attempting to query a JSON collection of binary data in a context that requires preserving the ordering of the collection; this isn't supported by SQL Server.</value>
+  </data>
   <data name="SaveChangesFailedBecauseOfComputedColumnWithFunction" xml:space="preserve">
     <value>Could not save changes because the target table has computed column with a function that performs data access. Please configure your table accordingly, see https://aka.ms/efcore-docs-sqlserver-save-changes-and-output-clause for more information.</value>
   </data>

src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs

@@ -4,6 +4,7 @@
 using System.Diagnostics.CodeAnalysis;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.SqlServer.Internal;
+using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
 
 namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;
 
@@ -148,15 +149,21 @@ public Expression Process(Expression expression)
 
                     // Record the OPENJSON expression and its projected column(s), along with the store type we just removed from the WITH
                     // clause. Then visit the select expression, adding a cast around the matching ColumnExpressions.
-                    // TODO: Need to pass through the type mapping API for converting the JSON value (nvarchar) to the relational store type
-                    // (e.g. datetime2), see #30677
                     foreach (var column in openJsonExpression.ColumnInfos)
                     {
                         var typeMapping = _typeMappingSource.FindMapping(column.StoreType);
                         Check.DebugAssert(
                             typeMapping is not null,
                             $"Could not find mapping for store type {column.StoreType} when converting OPENJSON/WITH");
 
+                        // Binary data (varbinary) is stored in JSON as base64, which OPENJSON knows how to decode as long the type is
+                        // specified in the WITH clause. We're now removing the WITH and applying a relational CAST, but that doesn't work
+                        // for base64 data.
+                        if (typeMapping is SqlServerByteArrayTypeMapping)
+                        {
+                            throw new InvalidOperationException(SqlServerStrings.QueryingOrderedBinaryJsonCollectionsNotSupported);
+                        }
+
                         _castsToApply.Add((newOpenJsonExpression, column.Name), typeMapping);
                     }
 

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs

@@ -206,7 +206,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
                 IsDistinct: false,
                 Limit: null,
                 Offset: null,
-                // We can only applying the indexing if the JSON array is ordered by its natural ordered, i.e. by the "key" column that
+                // We can only apply the indexing if the JSON array is ordered by its natural ordered, i.e. by the "key" column that
                 // we created in TranslateCollection. For example, if another ordering has been applied (e.g. by the JSON elements
                 // themselves), we can no longer simply index into the original array.
                 Orderings:
@@ -396,7 +396,6 @@ protected override Expression ApplyInferredTypeMappings(
     protected class SqlServerInferredTypeMappingApplier : RelationalInferredTypeMappingApplier
     {
         private readonly IRelationalTypeMappingSource _typeMappingSource;
-        private readonly ISqlExpressionFactory _sqlExpressionFactory;
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -409,7 +408,7 @@ public SqlServerInferredTypeMappingApplier(
             ISqlExpressionFactory sqlExpressionFactory,
             IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping?> inferredTypeMappings)
             : base(sqlExpressionFactory, inferredTypeMappings)
-            => (_typeMappingSource, _sqlExpressionFactory) = (typeMappingSource, sqlExpressionFactory);
+            => _typeMappingSource = typeMappingSource;
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -480,18 +479,5 @@ protected virtual SqlServerOpenJsonExpression ApplyTypeMappingsOnOpenJsonExpress
                 path: null,
                 new[] { new SqlServerOpenJsonExpression.ColumnInfo("value", elementTypeMapping.StoreType, "$") });
         }
-
-        /// <summary>
-        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
-        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
-        ///     any release. You should only use it directly in your code with extreme caution and knowing that
-        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-        /// </summary>
-        public virtual SqlExpression ApplyTypeMappingOnColumn(ColumnExpression columnExpression, RelationalTypeMapping typeMapping)
-            // TODO: this should be part of #30677
-            // OPENJSON's value column has type nvarchar(max); apply a CAST() unless that's the inferred element type mapping
-            => typeMapping.StoreType is "nvarchar(max)"
-                ? columnExpression
-                : _sqlExpressionFactory.Convert(columnExpression, typeMapping.ClrType, typeMapping);
     }
 }

src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs

@@ -493,11 +493,7 @@ public SqlServerTypeMappingSource(
         stringTypeMapping = (SqlServerStringTypeMapping)stringTypeMapping
             .Clone(new CollectionToJsonStringConverter(mappingInfo.ClrType, elementTypeMapping));
 
-        // The JSON representation for new[] { 1, 2 } is AQI= (base64), this cannot simply be cast to varbinary(max) (0x0102)
-        if (elementTypeMapping is not SqlServerByteArrayTypeMapping)
-        {
-            stringTypeMapping = (SqlServerStringTypeMapping)stringTypeMapping.CloneWithElementTypeMapping(elementTypeMapping);
-        }
+        stringTypeMapping = (SqlServerStringTypeMapping)stringTypeMapping.CloneWithElementTypeMapping(elementTypeMapping);
 
         return stringTypeMapping;
     }

src/EFCore.Sqlite.Core/Properties/SqliteStrings.resx

@@ -216,6 +216,9 @@
   <data name="OrderByNotSupported" xml:space="preserve">
     <value>SQLite does not support expressions of type '{type}' in ORDER BY clauses. Convert the values to a supported type, or use LINQ to Objects to order the results on the client side.</value>
   </data>
+  <data name="QueryingJsonCollectionOfGivenTypeNotSupported" xml:space="preserve">
+    <value>Querying JSON collections with element provider type '{type}' isn't supported because of SQLite limitations.</value>
+  </data>
   <data name="SequencesNotSupported" xml:space="preserve">
     <value>SQLite does not support sequences. See https://go.microsoft.com/fwlink/?LinkId=723262 for more information and examples.</value>
   </data>

src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs

@@ -222,12 +222,15 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
         var selectExpression = new SelectExpression(
             jsonEachExpression, columnName: "value", columnType: elementClrType, columnTypeMapping: elementTypeMapping, isColumnNullable);
 
-        // TODO: SQLite does have REAL and BLOB types, which JSON does not. Need to possibly cast to that.
-        if (elementTypeMapping is not null)
-        {
-            // TODO: In any case, we still ned to pass through the type mapping API for doing any conversions (e.g. for datetime, from JSON
-            // ISO8601 to SQLite's format without the T), see #30677. Do this here.
-        }
+        // If we have a collection column, we know the type mapping at this point (as opposed to parameters, whose type mapping will get
+        // inferred later based on usage in SqliteInferredTypeMappingApplier); we should be able to apply any SQL logic needed to convert
+        // the JSON value out to its relational counterpart (e.g. datetime() for timestamps, see ApplyJsonSqlConversion).
+        //
+        // However, doing it here would interfere with pattern matching in e.g. TranslateElementAtOrDefault, where we specifically check
+        // for a bare column being projected out out of the table - if the user composed any operators over the collection, it's no longer
+        // possible to apply a specialized translation via the -> operator. We could add a way to recognize the special conversions we
+        // compose on top, but instead of going into that complexity, we'll just apply the SQL conversion later, in
+        // SqliteInferredTypeMappingApplier, as if we had a parameter collection.
 
         // Append an ordering for the json_each 'key' column.
         selectExpression.AppendOrdering(
@@ -311,7 +314,7 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
                 // conversions.
                 if (projectionColumn.TypeMapping is not null)
                 {
-                    translation = ApplyTypeMappingOnColumn(translation, projectionColumn.TypeMapping, projectionColumn.IsNullable);
+                    translation = ApplyJsonSqlConversion(translation, projectionColumn.TypeMapping, projectionColumn.IsNullable);
                 }
 
                 return source.UpdateQueryExpression(_sqlExpressionFactory.Select(translation));
@@ -405,10 +408,15 @@ when TryGetInferredTypeMapping(jsonEachExpression, "value", out var typeMapping)
                     return visited;
                 }
 
+                // Note that we match also ColumnExpressions which already have a type mapping, i.e. coming out of column collections (as
+                // opposed to parameter collections, where the type mapping needs to be inferred). This is in order to apply SQL conversion
+                // logic later in the process, see note in TranslateCollection.
                 case ColumnExpression { Name: "value" } columnExpression
-                    when _currentSelectInferredTypeMappings is not null
-                    && _currentSelectInferredTypeMappings.TryGetValue(columnExpression.Table, out var inferredTypeMapping):
-                    return ApplyTypeMappingOnColumn(columnExpression, inferredTypeMapping, columnExpression.IsNullable);
+                    when _currentSelectInferredTypeMappings?.TryGetValue(columnExpression.Table, out var inferredTypeMapping) is true:
+                    return ApplyJsonSqlConversion(
+                        columnExpression.ApplyTypeMapping(inferredTypeMapping),
+                        inferredTypeMapping,
+                        columnExpression.IsNullable);
 
                 default:
                     return base.VisitExtension(expression);
@@ -448,18 +456,36 @@ protected virtual TableValuedFunctionExpression ApplyTypeMappingsOnJsonEachExpre
         }
     }
 
-    private static SqlExpression ApplyTypeMappingOnColumn(SqlExpression expression, RelationalTypeMapping typeMapping, bool isNullable)
+    /// <summary>
+    /// Wraps the given expression with any SQL logic necessary to convert a value coming out of a JSON document into the relational value
+    /// represented by the given type mapping.
+    /// </summary>
+    private static SqlExpression ApplyJsonSqlConversion(SqlExpression expression, RelationalTypeMapping typeMapping, bool isNullable)
         => typeMapping switch
         {
-            // TODO: These server-side conversions need to be managed on the type mapping, #30677
-
             // The "standard" JSON timestamp representation is ISO8601, with a T between date and time; but SQLite's representation has
             // no T. Apply a conversion on the value coming out of json_each.
-            SqliteDateTimeTypeMapping => new SqlFunctionExpression(
-                "datetime", new[] { expression }, isNullable, new[] { true }, typeof(DateTime), typeMapping),
-
-            SqliteGuidTypeMapping => new SqlFunctionExpression(
-                "upper", new[] { expression }, isNullable, new[] { true }, typeof(Guid), typeMapping),
+            SqliteDateTimeTypeMapping
+                => new SqlFunctionExpression("datetime", new[] { expression }, isNullable, new[] { true }, typeof(DateTime), typeMapping),
+
+            SqliteGuidTypeMapping
+                => new SqlFunctionExpression("upper", new[] { expression }, isNullable, new[] { true }, typeof(Guid), typeMapping),
+
+            // The JSON representation for decimal is e.g. 1 (JSON int), whereas our literal representation is "1.0" (string).
+            // We can cast the 1 to TEXT, but we'd still get "1" not "1.0".
+            SqliteDecimalTypeMapping
+                => throw new InvalidOperationException(SqliteStrings.QueryingJsonCollectionOfGivenTypeNotSupported("decimal")),
+
+            // The JSON representation for new[] { 1, 2 } is AQI= (base64), and SQLite has no built-in base64 conversion function.
+            ByteArrayTypeMapping
+                => throw new InvalidOperationException(SqliteStrings.QueryingJsonCollectionOfGivenTypeNotSupported("byte[]")),
+
+            // The JSON representation for DateTimeOffset is ISO8601 (2023-01-01T12:30:00+02:00), but our SQL literal representation
+            // is 2023-01-01 12:30:00+02:00 (no T).
+            // Note that datetime('2023-01-01T12:30:00+02:00') yields '2023-01-01 10:30:00', converting to UTC (removing the timezone), so
+            // we can't use that.
+            SqliteDateTimeOffsetTypeMapping
+                => throw new InvalidOperationException(SqliteStrings.QueryingJsonCollectionOfGivenTypeNotSupported("DateTimeOffset")),
 
             _ => expression
         };

src/EFCore.Sqlite.Core/Storage/Internal/SqliteTypeMappingSource.cs

@@ -205,23 +205,7 @@ public static bool IsSpatialiteType(string columnType)
             stringTypeMapping = (SqliteStringTypeMapping)stringTypeMapping
                 .Clone(new CollectionToJsonStringConverter(mappingInfo.ClrType, elementTypeMapping));
 
-            switch (elementTypeMapping)
-            {
-                // The JSON representation for DateTimeOffset is ISO8601 (2023-01-01T12:30:00+02:00), but our SQL literal representation
-                // is 2023-01-01 12:30:00+02:00 (no T).
-                // datetime('2023-01-01T12:30:00+02:00') yields '2023-01-01 10:30:00' - converted to UTC, no timezone.
-                case SqliteDateTimeOffsetTypeMapping:
-                // The JSON representation for decimal is e.g. 1 (JSON int), whereas our literal representation is "1.0" (string)
-                case SqliteDecimalTypeMapping:
-                // The JSON representation for new[] { 1, 2 } is AQI= (base64?), our SQL literal representation is X'0102'
-                case ByteArrayTypeMapping:
-                    break;
-
-
-                default:
-                    stringTypeMapping = (SqliteStringTypeMapping)stringTypeMapping.CloneWithElementTypeMapping(elementTypeMapping);
-                    break;
-            }
+            stringTypeMapping = (SqliteStringTypeMapping)stringTypeMapping.CloneWithElementTypeMapping(elementTypeMapping);
 
             return stringTypeMapping;
         }

test/EFCore.Specification.Tests/Query/NonSharedPrimitiveCollectionsQueryTestBase.cs

@@ -3,6 +3,8 @@
 
 namespace Microsoft.EntityFrameworkCore.Query;
 
+using static System.Linq.Expressions.Expression;
+
 public abstract class NonSharedPrimitiveCollectionsQueryTestBase : NonSharedModelTestBase
 {
     #region Support for specific element types
@@ -75,7 +77,7 @@ public virtual Task Array_of_byte_array()
     public virtual Task Array_of_enum()
         => TestArray(MyEnum.Label1, MyEnum.Label2);
 
-    enum MyEnum { Label1, Label2 }
+    private enum MyEnum { Label1, Label2 }
 
     // This ensures that collections of Geometry (e.g. Geometry[]) aren't mapped; NTS has GeometryCollection for that.
     // See SQL Server/SQLite for a sample implementation.
@@ -238,24 +240,23 @@ protected async Task TestArray<TElement>(
 
         await using var context = contextFactory.CreateContext();
 
-        var entityParam = Expression.Parameter(typeof(TestEntity), "m");
-        var efPropertyCall = Expression.Call(
+        var entityParam = Parameter(typeof(TestEntity), "m");
+        var efPropertyCall = Call(
             typeof(EF).GetMethod(nameof(EF.Property), BindingFlags.Public | BindingFlags.Static)!.MakeGenericMethod(arrayClrType),
             entityParam,
-            Expression.Constant("SomeArray"));
+            Constant("SomeArray"));
 
-        var elementParam = Expression.Parameter(typeof(TElement), "a");
-        var predicate = Expression.Lambda<Func<TestEntity, bool>>(
-            Expression.Equal(
-                Expression.Call(
+        var elementParam = Parameter(typeof(TElement), "a");
+        var predicate = Lambda<Func<TestEntity, bool>>(
+            Equal(
+                Call(
                     EnumerableMethods.CountWithPredicate.MakeGenericMethod(typeof(TElement)),
                     efPropertyCall,
-                    Expression.Lambda(
-                        Expression.Equal(elementParam, Expression.Constant(value1)),
-                        elementParam)),
-            Expression.Constant(2)),
+                    Lambda(Equal(elementParam, Constant(value1)), elementParam)),
+            Constant(2)),
             entityParam);
 
+        // context.Set<TestEntity>().SingleAsync(m => EF.Property<int[]>(m, "SomeArray").Count(a => a == <value1>) == 2)
         var result = await context.Set<TestEntity>().SingleAsync(predicate);
         Assert.Equal(1, result.Id);
     }