Fix to #30922 - Most cases of projecting a primitive collection don't…

… work (#31263)

Problem was that for cases that compose on the collection of primitives and then project it, we need identifier to properly bucket the results. On SqlServer we can use hidden key column that is a result of OPENJSON - we need something that is unique so we can't use the value itself.
Fix is to add identifier to the SelectExpression based on OPENJSON and also modify the logic that figures out if we need key (and thus need to revert to OPENJSON without WITH clause). Also some minor fixes around nullability - when projecting element of a collection of primitives using OUTER APPLY/JOIN we must set the projection binding to nullable, as the value can always be null, in case of empty collection.

Fixes #30922

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -995,6 +995,9 @@
   <data name="SetOperationsOnDifferentStoreTypes" xml:space="preserve">
     <value>Unable to translate set operation when matching columns on both sides have different store types.</value>
   </data>
+  <data name="SetOperationsRequireAtLeastOneSideWithValidTypeMapping" xml:space="preserve">
+    <value>A set operation 'setOperationType' requires valid type mapping for at least one of its sides.</value>
+  </data>
   <data name="SetPropertyMethodInvoked" xml:space="preserve">
     <value>The SetProperty&lt;TProperty&gt; method can only be used within 'ExecuteUpdate' method.</value>
   </data>

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

@@ -101,20 +101,21 @@ internal SelectExpression(SqlExpression? projection)
     }
 
     /// <summary>
-    ///     Creates a new instance of the <see cref="SelectExpression" /> class given a <see cref="TableExpressionBase" />, with a single
-    ///     column projection.
+    ///     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>
-    /// <param name="tableExpression">The table expression.</param>
-    /// <param name="columnName">The name of the column to add as the projection.</param>
-    /// <param name="columnType">The type of the column to add as the projection.</param>
-    /// <param name="columnTypeMapping">The type mapping of the column to add as the projection.</param>
-    /// <param name="isColumnNullable">Whether the column projected out is nullable.</param>
+    [EntityFrameworkInternal]
     public SelectExpression(
         TableExpressionBase tableExpression,
         string columnName,
         Type columnType,
         RelationalTypeMapping? columnTypeMapping,
-        bool? isColumnNullable = null)
+        bool? isColumnNullable = null,
+        string? identifierColumnName = null,
+        Type? identifierColumnType = null,
+        RelationalTypeMapping? identifierColumnTypeMapping = null)
         : base(null)
     {
         var tableReferenceExpression = new TableReferenceExpression(this, tableExpression.Alias!);
@@ -128,6 +129,24 @@ public SelectExpression(
             isColumnNullable ?? columnType.IsNullableType());
 
         _projectionMapping[new ProjectionMember()] = columnExpression;
+
+        if (identifierColumnName != null && identifierColumnType != null && identifierColumnTypeMapping != null)
+        {
+            var identifierColumn = new ConcreteColumnExpression(
+                identifierColumnName,
+                tableReferenceExpression,
+                identifierColumnType.UnwrapNullableType(),
+                identifierColumnTypeMapping,
+                identifierColumnType.IsNullableType());
+
+            _identifier.Add((identifierColumn, identifierColumnTypeMapping!.Comparer));
+        }
+        else
+        {
+            Debug.Assert(
+                identifierColumnName == null && identifierColumnType == null && identifierColumnTypeMapping == null,
+                "Either provide all identity information (column name, type and type mapping), or don't provide any.");
+        }
     }
 
     internal SelectExpression(IEntityType entityType, ISqlExpressionFactory sqlExpressionFactory)
@@ -2205,7 +2224,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
             : Array.Empty<ColumnExpression?>();
         var entityProjectionIdentifiers = new List<ColumnExpression>();
         var entityProjectionValueComparers = new List<ValueComparer>();
-        var otherExpressions = new List<SqlExpression>();
+        var otherExpressions = new List<(SqlExpression Expression, ValueComparer Comparer)>();
 
         // Push down into a subquery if limit/offset are defined. If not, any orderings can be discarded as set operations don't preserve
         // them.
@@ -2308,7 +2327,19 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
                     }
                 }
 
-                otherExpressions.Add(outerProjection);
+                // we need comparer (that we get from type mapping) for identifiers
+                // it may happen that one side of the set operation comes from collection parameter
+                // and therefore doesn't have type mapping (yet - we infer those after the translation is complete)
+                // but for set operation at least one side should have type mapping, otherwise whole thing would have been parameterized out
+                // this can only happen in compiled query, since we always parameterize parameters there - if this happens we throw
+                var outerTypeMapping = innerProjection1.Expression.TypeMapping ?? innerProjection2.Expression.TypeMapping;
+                if (outerTypeMapping == null)
+                {
+                    throw new InvalidOperationException(
+                        RelationalStrings.SetOperationsRequireAtLeastOneSideWithValidTypeMapping(setOperationType));
+                }
+
+                otherExpressions.Add((outerProjection, outerTypeMapping.KeyComparer));
             }
         }
 
@@ -2349,10 +2380,10 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
                     // If there are no other expressions then we can use all entityProjectionIdentifiers
                     _identifier.AddRange(entityProjectionIdentifiers.Zip(entityProjectionValueComparers));
                 }
-                else if (otherExpressions.All(e => e is ColumnExpression))
+                else if (otherExpressions.All(e => e.Expression is ColumnExpression))
                 {
                     _identifier.AddRange(entityProjectionIdentifiers.Zip(entityProjectionValueComparers));
-                    _identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e, e.TypeMapping!.KeyComparer)));
+                    _identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e.Expression, e.Comparer)));
                 }
             }
         }

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

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.CodeAnalysis;
+using System.Linq.Expressions;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.SqlServer.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
@@ -112,80 +114,129 @@ public Expression Process(Expression expression)
                 case ShapedQueryExpression shapedQueryExpression:
                     return shapedQueryExpression.UpdateQueryExpression(Visit(shapedQueryExpression.QueryExpression));
 
-                case SelectExpression
+                case SelectExpression selectExpression:
                 {
-                    Tables: [SqlServerOpenJsonExpression { ColumnInfos: not null } openJsonExpression, ..],
-                    Orderings:
-                    [
+                    var newTables = default(TableExpressionBase[]);
+                    var appliedCasts = new List<(SqlServerOpenJsonExpression, string)>();
+
+                    for (var i = 0; i < selectExpression.Tables.Count; i++)
+                    {
+                        var table = selectExpression.Tables[i];
+                        if ((table is SqlServerOpenJsonExpression { ColumnInfos: not null }
+                            or JoinExpressionBase { Table: SqlServerOpenJsonExpression { ColumnInfos: not null } })
+                            && selectExpression.Orderings.Select(o => o.Expression)
+                                .Concat(selectExpression.Projection.Select(p => p.Expression))
+                                .Any(x => IsKeyColumn(x, table)))
                         {
-                            Expression: SqlUnaryExpression
+                            // Remove the WITH clause from the OPENJSON expression
+                            var openJsonExpression = (SqlServerOpenJsonExpression)((table as JoinExpressionBase)?.Table ?? table);
+                            var newOpenJsonExpression = openJsonExpression.Update(
+                                openJsonExpression.JsonExpression,
+                                openJsonExpression.Path,
+                                columnInfos: null);
+
+                            TableExpressionBase newTable = table switch
+                            {
+                                InnerJoinExpression ij => ij.Update(newOpenJsonExpression, ij.JoinPredicate),
+                                LeftJoinExpression lj => lj.Update(newOpenJsonExpression, lj.JoinPredicate),
+                                CrossJoinExpression cj => cj.Update(newOpenJsonExpression),
+                                CrossApplyExpression ca => ca.Update(newOpenJsonExpression),
+                                OuterApplyExpression oa => oa.Update(newOpenJsonExpression),
+                                _ => newOpenJsonExpression,
+                            };
+
+                            if (newTables is not null)
+                            {
+                                newTables[i] = newTable;
+                            }
+                            else if (!table.Equals(newTable))
                             {
-                                OperatorType: ExpressionType.Convert,
-                                Operand: ColumnExpression { Name: "key", Table: var keyColumnTable }
+                                newTables = new TableExpressionBase[selectExpression.Tables.Count];
+                                for (var j = 0; j < i; j++)
+                                {
+                                    newTables[j] = selectExpression.Tables[j];
+                                }
+
+                                newTables[i] = newTable;
                             }
+
+                            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);
+                                appliedCasts.Add((newOpenJsonExpression, column.Name));
+                            }
+
+                            continue;
                         }
-                    ]
-                } selectExpression
-                    when keyColumnTable == openJsonExpression:
-                {
-                    // Remove the WITH clause from the OPENJSON expression
-                    var newOpenJsonExpression = openJsonExpression.Update(
-                        openJsonExpression.JsonExpression,
-                        openJsonExpression.Path,
-                        columnInfos: null);
-
-                    var newTables = selectExpression.Tables.ToArray();
-                    newTables[0] = newOpenJsonExpression;
-
-                    var newSelectExpression = selectExpression.Update(
-                        selectExpression.Projection,
-                        newTables,
-                        selectExpression.Predicate,
-                        selectExpression.GroupBy,
-                        selectExpression.Having,
-                        selectExpression.Orderings,
-                        selectExpression.Limit,
-                        selectExpression.Offset);
 
-                    // 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.
-                    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)
+                        if (newTables is not null)
                         {
-                            throw new InvalidOperationException(SqlServerStrings.QueryingOrderedBinaryJsonCollectionsNotSupported);
+                            newTables[i] = table;
                         }
-
-                        _castsToApply.Add((newOpenJsonExpression, column.Name), typeMapping);
                     }
 
+                    // SelectExpression.Update always creates a new instance - we should avoid it when tables haven't changed
+                    // see #31276
+                    var newSelectExpression = newTables is not null
+                        ? selectExpression.Update(
+                            selectExpression.Projection,
+                            newTables,
+                            selectExpression.Predicate,
+                            selectExpression.GroupBy,
+                            selectExpression.Having,
+                            selectExpression.Orderings,
+                            selectExpression.Limit,
+                            selectExpression.Offset)
+                        : selectExpression;
+
+                    // 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.
                     var result = base.Visit(newSelectExpression);
 
-                    foreach (var column in openJsonExpression.ColumnInfos)
+                    foreach (var appliedCast in appliedCasts)
                     {
-                        _castsToApply.Remove((newOpenJsonExpression, column.Name));
+                        _castsToApply.Remove(appliedCast);
                     }
 
                     return result;
                 }
 
-                case ColumnExpression { Table: SqlServerOpenJsonExpression openJsonTable, Name: var name } columnExpression
-                    when _castsToApply.TryGetValue((openJsonTable, name), out var typeMapping):
+                case ColumnExpression columnExpression:
                 {
-                    return _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, typeMapping);
+                    return columnExpression.Table switch
+                    {
+                        SqlServerOpenJsonExpression openJsonTable
+                            when _castsToApply.TryGetValue((openJsonTable, columnExpression.Name), out var typeMapping)
+                            => _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, typeMapping),
+                        JoinExpressionBase { Table: SqlServerOpenJsonExpression innerOpenJsonTable }
+                            when _castsToApply.TryGetValue((innerOpenJsonTable, columnExpression.Name), out var innerTypeMapping)
+                            => _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, innerTypeMapping),
+                        _ => base.Visit(expression)
+                    };
                 }
 
                 default:
                     return base.Visit(expression);
             }
+
+            static bool IsKeyColumn(SqlExpression sqlExpression, TableExpressionBase table)
+                => (sqlExpression is ColumnExpression { Name: "key", Table: var keyColumnTable }
+                    && keyColumnTable == table)
+                || (sqlExpression is SqlUnaryExpression { OperatorType: ExpressionType.Convert,
+                    Operand: SqlExpression operand } && IsKeyColumn(operand, table));
         }
     }
 }

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

@@ -161,8 +161,17 @@ protected override Expression VisitExtension(Expression extensionExpression)
         var elementClrType = sqlExpression.Type.GetSequenceType();
         var isColumnNullable = elementClrType.IsNullableType();
 
+#pragma warning disable EF1001 // Internal EF Core API usage.
         var selectExpression = new SelectExpression(
-            openJsonExpression, columnName: "value", columnType: elementClrType, columnTypeMapping: elementTypeMapping, isColumnNullable);
+            openJsonExpression,
+            columnName: "value",
+            columnType: elementClrType,
+            columnTypeMapping: elementTypeMapping,
+            isColumnNullable,
+            identifierColumnName: "key",
+            identifierColumnType: typeof(string),
+            identifierColumnTypeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"));
+#pragma warning restore EF1001 // Internal EF Core API usage.
 
         // OPENJSON doesn't guarantee the ordering of the elements coming out; when using OPENJSON without WITH, a [key] column is returned
         // with the JSON array's ordering, which we can ORDER BY; this option doesn't exist with OPENJSON with WITH, unfortunately.
@@ -186,7 +195,15 @@ protected override Expression VisitExtension(Expression extensionExpression)
                     _typeMappingSource.FindMapping(typeof(int))),
                 ascending: true));
 
-        var shaperExpression = new ProjectionBindingExpression(selectExpression, new ProjectionMember(), elementClrType);
+        var shaperExpression = (Expression)new ProjectionBindingExpression(selectExpression, new ProjectionMember(), elementClrType.MakeNullable());
+        if (shaperExpression.Type != elementClrType)
+        {
+            Check.DebugAssert(
+                elementClrType.MakeNullable() == shaperExpression.Type,
+                "expression.Type must be nullable of targetType");
+
+            shaperExpression = Expression.Convert(shaperExpression, elementClrType);
+        }
 
         return new ShapedQueryExpression(selectExpression, shaperExpression);
     }