Use JsonScalarExpression for primitive collection indexing

Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.

src/EFCore.Relational/Query/JsonQueryExpression.cs

@@ -117,8 +117,9 @@ public virtual SqlExpression BindProperty(IProperty property)
 
         return new JsonScalarExpression(
             JsonColumn,
-            property,
             newPath,
+            property.ClrType.UnwrapNullableType(),
+            property.FindRelationalTypeMapping()!,
             IsNullable || property.IsNullable);
     }
 

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -2786,7 +2786,7 @@ when InferredTypeMappings.TryGetValue((columnExpression.Table, columnExpression.
                 case ValuesExpression valuesExpression:
                     // By default, the ValuesExpression also contains an ordering by a synthetic increasing _ord. If the containing
                     // SelectExpression doesn't project it out or require it (limit/offset), strip that out.
-                    // TODO: Strictly-speaking, this doesn't belong in this visitor which is about applying type mappings
+                    // TODO: Strictly-speaking, stripping the ordering doesn't belong in this visitor which is about applying type mappings
                     return ApplyTypeMappingsOnValuesExpression(
                         valuesExpression,
                         stripOrdering: _currentSelectExpression is { Limit: null, Offset: null }

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -10,6 +10,7 @@ namespace Microsoft.EntityFrameworkCore.Query;
 /// <inheritdoc />
 public class SqlExpressionFactory : ISqlExpressionFactory
 {
+    private readonly IRelationalTypeMappingSource _typeMappingSource;
     private readonly RelationalTypeMapping _boolTypeMapping;
 
     /// <summary>
@@ -19,7 +20,8 @@ public class SqlExpressionFactory : ISqlExpressionFactory
     public SqlExpressionFactory(SqlExpressionFactoryDependencies dependencies)
     {
         Dependencies = dependencies;
-        _boolTypeMapping = dependencies.TypeMappingSource.FindMapping(typeof(bool), dependencies.Model)!;
+        _typeMappingSource = dependencies.TypeMappingSource;
+        _boolTypeMapping = _typeMappingSource.FindMapping(typeof(bool), dependencies.Model)!;
     }
 
     /// <summary>
@@ -38,7 +40,7 @@ public SqlExpressionFactory(SqlExpressionFactoryDependencies dependencies)
                 && sqlUnaryExpression.Type == typeof(object)
                     ? sqlUnaryExpression.Operand
                     : ApplyTypeMapping(
-                        sqlExpression, Dependencies.TypeMappingSource.FindMapping(sqlExpression.Type, Dependencies.Model));
+                        sqlExpression, _typeMappingSource.FindMapping(sqlExpression.Type, Dependencies.Model));
 
     /// <inheritdoc />
     [return: NotNullIfNotNull("sqlExpression")]
@@ -60,6 +62,13 @@ public SqlExpressionFactory(SqlExpressionFactoryDependencies dependencies)
             ColumnExpression e => e.ApplyTypeMapping(typeMapping),
             DistinctExpression e => ApplyTypeMappingOnDistinct(e, typeMapping),
             InExpression e => ApplyTypeMappingOnIn(e),
+
+            // We only do type inference for JSON scalar expression which represent a single array indexing operation; we can infer the
+            // array's mapping from the element or vice versa, allowing e.g. parameter primitive collections to get inferred when an
+            // an indexer is used over them and then compared to a column.
+            // But we can't infer anything for other Path forms of JsonScalarExpression (e.g. a property lookup).
+            JsonScalarExpression { Path: [{ ArrayIndex: not null }] } e => ApplyTypeMappingOnJsonScalar(e, typeMapping),
+
             LikeExpression e => ApplyTypeMappingOnLike(e),
             ScalarSubqueryExpression e => e.ApplyTypeMapping(typeMapping),
             SqlBinaryExpression e => ApplyTypeMappingOnSqlBinary(e, typeMapping),
@@ -68,6 +77,7 @@ public SqlExpressionFactory(SqlExpressionFactoryDependencies dependencies)
             SqlFunctionExpression e => e.ApplyTypeMapping(typeMapping),
             SqlParameterExpression e => e.ApplyTypeMapping(typeMapping),
             SqlUnaryExpression e => ApplyTypeMappingOnSqlUnary(e, typeMapping),
+
             _ => sqlExpression
         };
     }
@@ -82,7 +92,7 @@ private SqlExpression ApplyTypeMappingOnLike(LikeExpression likeExpression)
                     likeExpression.Match, likeExpression.Pattern)
                 : ExpressionExtensions.InferTypeMapping(
                     likeExpression.Match, likeExpression.Pattern, likeExpression.EscapeChar))
-            ?? Dependencies.TypeMappingSource.FindMapping(likeExpression.Match.Type, Dependencies.Model);
+            ?? _typeMappingSource.FindMapping(likeExpression.Match.Type, Dependencies.Model);
 
         return new LikeExpression(
             ApplyTypeMapping(likeExpression.Match, inferredTypeMapping),
@@ -184,9 +194,9 @@ private SqlExpression ApplyTypeMappingOnSqlBinary(
             {
                 inferredTypeMapping = ExpressionExtensions.InferTypeMapping(left, right)
                     // We avoid object here since the result does not get typeMapping from outside.
-                    ?? (left.Type != typeof(object)
-                        ? Dependencies.TypeMappingSource.FindMapping(left.Type, Dependencies.Model)
-                        : Dependencies.TypeMappingSource.FindMapping(right.Type, Dependencies.Model));
+                    ?? _typeMappingSource.FindMapping(
+                        left.Type != typeof(object) ? left.Type : right.Type,
+                        Dependencies.Model);
                 resultType = typeof(bool);
                 resultTypeMapping = _boolTypeMapping;
                 break;
@@ -236,7 +246,7 @@ private SqlExpression ApplyTypeMappingOnIn(InExpression inExpression)
                 : inExpression.Subquery != null
                     ? ExpressionExtensions.InferTypeMapping(inExpression.Item, inExpression.Subquery.Projection[0].Expression)
                     : inExpression.Item.TypeMapping)
-            ?? Dependencies.TypeMappingSource.FindMapping(inExpression.Item.Type, Dependencies.Model);
+            ?? _typeMappingSource.FindMapping(inExpression.Item.Type, Dependencies.Model);
 
         var item = ApplyTypeMapping(inExpression.Item, itemTypeMapping);
         if (inExpression.Values != null)
@@ -253,6 +263,41 @@ private SqlExpression ApplyTypeMappingOnIn(InExpression inExpression)
             : inExpression;
     }
 
+    private SqlExpression ApplyTypeMappingOnJsonScalar(
+        JsonScalarExpression jsonScalarExpression,
+        RelationalTypeMapping? typeMapping)
+    {
+        if (jsonScalarExpression is not { Json: var array, Path: [{ ArrayIndex: { } index }] })
+        {
+            return jsonScalarExpression;
+        }
+
+        // The index expression isn't inferred and is always just an int. Apply the default type mapping to it.
+        var indexWithTypeMapping = ApplyDefaultTypeMapping(index);
+        var newPath = indexWithTypeMapping == index ? jsonScalarExpression.Path : new[] { new PathSegment(indexWithTypeMapping) };
+
+        // If a type mapping is being applied from the outside, it applies to the element resulting from the array indexing operation;
+        // we can infer the array's type mapping from it. Otherwise there's nothing to do but apply the default type mapping to the array.
+        if (typeMapping is null)
+        {
+            return new JsonScalarExpression(
+                ApplyDefaultTypeMapping(array),
+                newPath,
+                jsonScalarExpression.Type,
+                _typeMappingSource.FindMapping(jsonScalarExpression.Type),
+                jsonScalarExpression.IsNullable);
+        }
+
+        // TODO: blocked on #30730: we need to be able to construct a JSON collection type mapping based on the element's.
+        // For now, hacking to apply the default type mapping instead.
+        return new JsonScalarExpression(
+            ApplyDefaultTypeMapping(array), // Hack, until #30730
+            newPath,
+            jsonScalarExpression.Type,
+            typeMapping,
+            jsonScalarExpression.IsNullable);
+    }
+
     /// <inheritdoc />
     public virtual SqlBinaryExpression? MakeBinary(
         ExpressionType operatorType,
@@ -350,7 +395,7 @@ public virtual SqlFunctionExpression Coalesce(SqlExpression left, SqlExpression
         var resultType = right.Type;
         var inferredTypeMapping = typeMapping
             ?? ExpressionExtensions.InferTypeMapping(left, right)
-            ?? Dependencies.TypeMappingSource.FindMapping(resultType, Dependencies.Model);
+            ?? _typeMappingSource.FindMapping(resultType, Dependencies.Model);
 
         var typeMappedArguments = new List<SqlExpression>
         {
@@ -405,7 +450,7 @@ public virtual CaseExpression Case(SqlExpression? operand, IReadOnlyList<CaseWhe
             // Since we never look at type of Operand/Test after this place,
             // we need to find actual typeMapping based on non-object type.
             ?? new[] { operand.Type }.Concat(whenClauses.Select(wc => wc.Test.Type))
-                .Where(t => t != typeof(object)).Select(t => Dependencies.TypeMappingSource.FindMapping(t, Dependencies.Model))
+                .Where(t => t != typeof(object)).Select(t => _typeMappingSource.FindMapping(t, Dependencies.Model))
                 .FirstOrDefault();
 
         var resultTypeMapping = elseResult?.TypeMapping
@@ -543,7 +588,7 @@ public virtual ExistsExpression Exists(SelectExpression subquery, bool negated)
     /// <inheritdoc />
     public virtual InExpression In(SqlExpression item, SqlExpression values, bool negated)
     {
-        var typeMapping = item.TypeMapping ?? Dependencies.TypeMappingSource.FindMapping(item.Type, Dependencies.Model);
+        var typeMapping = item.TypeMapping ?? _typeMappingSource.FindMapping(item.Type, Dependencies.Model);
 
         item = ApplyTypeMapping(item, typeMapping);
         values = ApplyTypeMapping(values, typeMapping);

src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs

@@ -17,36 +17,28 @@ public class JsonScalarExpression : SqlExpression
     /// <summary>
     ///     Creates a new instance of the <see cref="JsonScalarExpression" /> class.
     /// </summary>
-    /// <param name="jsonColumn">A column containg JSON value.</param>
-    /// <param name="property">A property representing the result of this expression.</param>
+    /// <param name="json">An expression representing a JSON value.</param>
+    /// <param name="type">The <see cref="System.Type" /> of the expression.</param>
+    /// <param name="typeMapping">The <see cref="RelationalTypeMapping" /> associated with the expression.</param>
     /// <param name="path">A list of path segments leading to the scalar from the root of the JSON stored in the column.</param>
     /// <param name="nullable">A value indicating whether the expression is nullable.</param>
     public JsonScalarExpression(
-        ColumnExpression jsonColumn,
-        IProperty property,
-        IReadOnlyList<PathSegment> path,
-        bool nullable)
-        : this(jsonColumn, path, property.ClrType.UnwrapNullableType(), property.FindRelationalTypeMapping()!, nullable)
-    {
-    }
-
-    internal JsonScalarExpression(
-        ColumnExpression jsonColumn,
+        SqlExpression json,
         IReadOnlyList<PathSegment> path,
         Type type,
-        RelationalTypeMapping typeMapping,
+        RelationalTypeMapping? typeMapping,
         bool nullable)
         : base(type, typeMapping)
     {
-        JsonColumn = jsonColumn;
+        Json = json;
         Path = path;
         IsNullable = nullable;
     }
 
     /// <summary>
-    ///     The column containing the JSON value.
+    ///     The expression containing the JSON value.
     /// </summary>
-    public virtual ColumnExpression JsonColumn { get; }
+    public virtual SqlExpression Json { get; }
 
     /// <summary>
     ///     The list of path segments leading to the scalar from the root of the JSON stored in the column.
@@ -61,46 +53,91 @@ internal JsonScalarExpression(
     /// <inheritdoc />
     protected override Expression VisitChildren(ExpressionVisitor visitor)
     {
-        var jsonColumn = (ColumnExpression)visitor.Visit(JsonColumn);
-        var jsonColumnMadeNullable = jsonColumn.IsNullable && !JsonColumn.IsNullable;
+        var newJson = (SqlExpression)visitor.Visit(Json);
+
+        var nullable = IsNullable;
+        if (newJson is ColumnExpression jsonColumnExpression)
+        {
+            nullable |= jsonColumnExpression.IsNullable;
+        }
+
+        PathSegment[]? newPath = null;
+
+        for (var i = 0; i < Path.Count; i++)
+        {
+            var segment = Path[i];
+            PathSegment newSegment;
+
+            if (segment.PropertyName is not null)
+            {
+                // PropertyName segments are (currently) constants, nothing to visit.
+                newSegment = segment;
+            }
+            else
+            {
+                var newArrayIndex = (SqlExpression)visitor.Visit(segment.ArrayIndex)!;
+                if (newArrayIndex == segment.ArrayIndex)
+                {
+                    newSegment = segment;
+                }
+                else
+                {
+                    newSegment = new PathSegment(newArrayIndex);
+
+                    if (newPath is null)
+                    {
+                        newPath = new PathSegment[Path.Count];
+                        for (var j = 0; j < i; i++)
+                        {
+                            newPath[j] = Path[j];
+                        }
+                    }
+                }
+            }
+
+            if (newPath is not null)
+            {
+                newPath[i] = newSegment;
+            }
+        }
 
         // TODO Call update: Issue#28887
-        return jsonColumn != JsonColumn
-            ? new JsonScalarExpression(
-                jsonColumn,
-                Path,
+        return newJson == Json && newPath is null
+            ? this
+            : new JsonScalarExpression(
+                newJson,
+                newPath ?? Path,
                 Type,
                 TypeMapping!,
-                IsNullable || jsonColumnMadeNullable)
-            : this;
+                nullable);
     }
 
     /// <summary>
     ///     Creates a new expression that is like this one, but using the supplied children. If all of the children are the same, it will
     ///     return this expression.
     /// </summary>
-    /// <param name="jsonColumn">The <see cref="JsonColumn" /> property of the result.</param>
+    /// <param name="json">The <see cref="Json" /> property of the result.</param>
     /// <returns>This expression if no children changed, or an expression with the updated children.</returns>
-    public virtual JsonScalarExpression Update(ColumnExpression jsonColumn)
-        => jsonColumn != JsonColumn
-            ? new JsonScalarExpression(jsonColumn, Path, Type, TypeMapping!, IsNullable)
+    public virtual JsonScalarExpression Update(SqlExpression json)
+        => json != Json
+            ? new JsonScalarExpression(json, Path, Type, TypeMapping!, IsNullable)
             : this;
 
     /// <inheritdoc />
     protected override void Print(ExpressionPrinter expressionPrinter)
     {
         expressionPrinter.Append("JsonScalarExpression(column: ");
-        expressionPrinter.Visit(JsonColumn);
+        expressionPrinter.Visit(Json);
         expressionPrinter.Append($""", "{string.Join(".", Path.Select(e => e.ToString()))}")""");
     }
 
     /// <inheritdoc />
     public override bool Equals(object? obj)
         => obj is JsonScalarExpression jsonScalarExpression
-            && JsonColumn.Equals(jsonScalarExpression.JsonColumn)
+            && Json.Equals(jsonScalarExpression.Json)
             && Path.SequenceEqual(jsonScalarExpression.Path);
 
     /// <inheritdoc />
     public override int GetHashCode()
-        => HashCode.Combine(base.GetHashCode(), JsonColumn, Path);
+        => HashCode.Combine(base.GetHashCode(), Json, Path);
 }

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

@@ -1739,7 +1739,7 @@ static IReadOnlyList<IProperty> GetMappedKeyProperties(IKey key)
 
         static bool JsonEntityContainedIn(JsonScalarExpression sourceExpression, JsonQueryExpression targetExpression)
         {
-            if (sourceExpression.JsonColumn != targetExpression.JsonColumn)
+            if (sourceExpression.Json != targetExpression.JsonColumn)
             {
                 return false;
             }

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -183,6 +183,9 @@
   <data name="InvalidTableToIncludeInScaffolding" xml:space="preserve">
     <value>The specified table '{table}' is not in a valid format. Specify tables using the format '[schema].[table]'.</value>
   </data>
+  <data name="JsonValuePathExpressionsNotSupported" xml:space="preserve">
+    <value>A non-constant array index or property name was used when navigating inside a JSON document; this is only supported starting with SQL Server 2017.</value>
+  </data>
   <data name="LogByteIdentityColumn" xml:space="preserve">
     <value>The property '{property}' on entity type '{entityType}' is of type 'byte', but is set up to use a SQL Server identity column; this requires that values starting at 255 and counting down will be used for temporary key values. A temporary key value is needed for every entity inserted in a single call to 'SaveChanges'. Care must be taken that these values do not collide with real key values.</value>
     <comment>Warning SqlServerEventId.ByteIdentityColumnWarning string string</comment>