Improve handling of null values in JSON properties mapped to primitive collections (#38498)

Fixes #38454

Author: AndriySvyryd

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs

@@ -3,6 +3,7 @@
 
 using System.Collections;
 using System.Runtime.CompilerServices;
+using System.Text;
 using System.Text.Json;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Query.Internal;
@@ -79,6 +80,9 @@ private static readonly MethodInfo MaterializeJsonNullableValueStructuralTypeMet
         private static readonly MethodInfo MaterializeJsonEntityCollectionMethodInfo
             = typeof(ShaperProcessingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(MaterializeJsonEntityCollection))!;
 
+        private static readonly MethodInfo ReadPrimitiveCollectionFromJsonMethodInfo
+            = typeof(ShaperProcessingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(ReadPrimitiveCollectionFromJson))!;
+
         private static readonly MethodInfo InverseCollectionFixupMethod
             = typeof(ShaperProcessingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(InverseCollectionFixup))!;
 
@@ -956,6 +960,45 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
             dataReaderContext.HasNext = false;
         }
 
+        /// <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>
+        [EntityFrameworkInternal]
+        public static object? ReadPrimitiveCollectionFromJson(
+            string? json,
+            JsonValueReaderWriter readerWriter,
+            bool nullable,
+            string propertyName)
+        {
+            if (json == null)
+            {
+                return null;
+            }
+
+            // Preserve the diagnostics of the converter path (JsonValueReaderWriter.FromJsonString), which rejects
+            // empty/whitespace JSON strings before tokenizing.
+            if (string.IsNullOrWhiteSpace(json))
+            {
+                throw new InvalidOperationException(CoreStrings.EmptyJsonString);
+            }
+
+            // A primitive collection mapped to a column is read by parsing the JSON string with the collection's
+            // JsonValueReaderWriter (which doesn't handle the 'null' token). The stored value may be a JSON 'null'
+            // token (e.g. the literal string "null"), which must be materialized as null for an optional property,
+            // rather than letting the reader/writer throw. See issues #34881 and #38454.
+            var manager = new Utf8JsonReaderManager(new JsonReaderData(Encoding.UTF8.GetBytes(json)), null);
+            manager.MoveNext();
+
+            return manager.CurrentReader.TokenType == JsonTokenType.Null
+                ? nullable
+                    ? null
+                    : throw new InvalidOperationException(RelationalStrings.NullValueInRequiredJsonProperty(propertyName))
+                : readerWriter.FromJson(ref manager);
+        }
+
         /// <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

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

@@ -10,6 +10,7 @@
 using Microsoft.EntityFrameworkCore.Query.Internal;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage.Json;
+using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
 using static System.Linq.Expressions.Expression;
 
 namespace Microsoft.EntityFrameworkCore.Query;
@@ -3000,7 +3001,90 @@ Expression valueExpression
             var converter = typeMapping.Converter;
 
             var converterExpression = default(Expression);
-            if (converter != null)
+            var primitiveCollectionJsonHandled = false;
+
+            // #34881/#38454: A primitive collection mapped to a column is stored as a JSON string and read via the collection's
+            // JsonValueReaderWriter. That reader/writer doesn't handle a JSON 'null' token, so we peek the first token here and
+            // short-circuit to null (or throw for a required property) before invoking the reader/writer, rather than letting it
+            // throw a cryptic "Invalid token type: 'Null'". This applies both when materializing an entity (the property is
+            // available) and when projecting the collection column directly (no property; a collection type mapping is
+            // identified by its ElementTypeMapping).
+            var jsonPrimitiveCollectionReaderWriter = converter is { ConvertsNulls: false }
+                ? property is IProperty { IsPrimitiveCollection: true } primitiveCollectionProperty
+                    ? primitiveCollectionProperty.GetJsonValueReaderWriter() ?? primitiveCollectionProperty.GetTypeMapping().JsonValueReaderWriter
+                    : property is null && typeMapping.ElementTypeMapping is not null
+                        ? typeMapping.JsonValueReaderWriter
+                        : null
+                : null;
+
+            if (jsonPrimitiveCollectionReaderWriter is not null)
+            {
+                Expression jsonReaderWriterExpression;
+                if (property is IProperty jsonProperty)
+                {
+                    var liftableConstantParameter = Parameter(typeof(MaterializerLiftableConstantContext), "c");
+                    jsonReaderWriterExpression = _parentVisitor.Dependencies.LiftableConstantFactory.CreateLiftableConstant(
+                        jsonPrimitiveCollectionReaderWriter,
+                        Lambda<Func<MaterializerLiftableConstantContext, object>>(
+                            Coalesce(
+                                Call(
+                                    LiftableConstantExpressionHelpers.BuildMemberAccessForProperty(
+                                        jsonProperty, liftableConstantParameter),
+                                    PropertyGetJsonValueReaderWriterMethod),
+                                Property(
+                                    Call(
+                                        LiftableConstantExpressionHelpers.BuildMemberAccessForProperty(
+                                            jsonProperty, liftableConstantParameter),
+                                        PropertyGetTypeMappingMethod),
+                                    nameof(CoreTypeMapping.JsonValueReaderWriter))),
+                            liftableConstantParameter),
+                        jsonProperty.Name + "JsonReaderWriter",
+                        typeof(JsonValueReaderWriter));
+                }
+                else
+                {
+                    // No property is available (e.g. projecting the collection column directly), so we can't reference the
+                    // reader/writer via the property. Use its ConstructorExpression, which is a quotable expression tree that
+                    // reconstructs the reader/writer (the same mechanism CollectionToJsonStringConverter uses).
+                    jsonReaderWriterExpression = jsonPrimitiveCollectionReaderWriter.ConstructorExpression;
+                    if (jsonReaderWriterExpression.Type != typeof(JsonValueReaderWriter))
+                    {
+                        jsonReaderWriterExpression = Convert(jsonReaderWriterExpression, typeof(JsonValueReaderWriter));
+                    }
+                }
+
+                if (valueExpression.Type != typeof(string))
+                {
+                    valueExpression = Convert(valueExpression, typeof(string));
+                }
+
+                // When there's no property this is a projection, which has no notion of "required", so a JSON 'null'
+                // token is always materialized as null rather than throwing.
+                Expression readExpression = Call(
+                    ReadPrimitiveCollectionFromJsonMethodInfo,
+                    valueExpression,
+                    jsonReaderWriterExpression,
+                    Constant((property as IProperty)?.IsNullable ?? true),
+                    Constant((property as IProperty)?.Name ?? string.Empty));
+
+                if (readExpression.Type != type)
+                {
+                    readExpression = Convert(readExpression, type);
+                }
+
+                if (nullable)
+                {
+                    // The column itself may be SQL NULL (DbNull), distinct from a JSON 'null' token in a non-null string.
+                    readExpression = Condition(
+                        Call(dbDataReader, IsDbNullMethod, indexExpression),
+                        Default(type),
+                        readExpression);
+                }
+
+                valueExpression = readExpression;
+                primitiveCollectionJsonHandled = true;
+            }
+            else if (converter != null)
             {
                 // if IProperty is available, we can reliably get the converter from the model and then incorporate FromProvider(Typed) delegate
                 // into the expression. This way we have consistent behavior between precompiled and normal queries (same code path)
@@ -3074,12 +3158,14 @@ Expression valueExpression
                 }
             }
 
-            if (valueExpression.Type != type)
+            if (!primitiveCollectionJsonHandled
+                && valueExpression.Type != type)
             {
                 valueExpression = Convert(valueExpression, type);
             }
 
-            if (nullable)
+            if (!primitiveCollectionJsonHandled
+                && nullable)
             {
                 Expression replaceExpression;
                 if (converter?.ConvertsNulls == true)
@@ -3277,6 +3363,30 @@ private Expression CreateReadJsonPropertyValueExpression(
                     nullExpression,
                     resultExpression);
             }
+            else if (property.GetElementType() is not null)
+            {
+                // A required primitive collection nested in a JSON document can't be materialized from a JSON 'null'
+                // token. Throw a clear, property-named error instead of the cryptic reader/writer "Invalid token type".
+                if (resultExpression.Type != property.ClrType)
+                {
+                    resultExpression = Convert(resultExpression, property.ClrType);
+                }
+
+                resultExpression = Condition(
+                    Equal(
+                        Property(
+                            Field(
+                                jsonReaderManagerParameter,
+                                Utf8JsonReaderManagerCurrentReaderField),
+                            Utf8JsonReaderTokenTypeProperty),
+                        Constant(JsonTokenType.Null)),
+                    Throw(
+                        New(
+                            typeof(InvalidOperationException).GetConstructor([typeof(string)])!,
+                            Constant(RelationalStrings.NullValueInRequiredJsonProperty(property.Name))),
+                        property.ClrType),
+                    resultExpression);
+            }
 
             if (_detailedErrorsEnabled)
             {

src/EFCore.Relational/Storage/RelationalTypeMapping.cs

@@ -678,15 +678,16 @@ protected virtual string GenerateNonNullSqlLiteral(object value)
     /// <returns>The default provider value.</returns>
     public virtual object? GetDefaultProviderValue()
     {
+        var providerType = (Converter?.ProviderClrType ?? ClrType).UnwrapNullableType();
+
+        // A primitive collection is serialized to a JSON string in its column, so its default value must be an empty JSON
+        // array rather than an empty string (which isn't valid JSON).
         if (ElementTypeMapping is not null
-            && Converter?.GetType() is { IsGenericType: true } converterType
-            && converterType.GetGenericTypeDefinition() == typeof(CollectionToJsonStringConverter<>))
+            && JsonValueReaderWriter != null)
         {
             return "[]";
         }
 
-        var providerType = (Converter?.ProviderClrType ?? ClrType).UnwrapNullableType();
-
         return providerType == typeof(string)
             ? string.Empty
             : providerType.IsArray

test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalFixture.cs

@@ -7,6 +7,7 @@
 using Post = Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase.Post;
 using JsonRoot = Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase.JsonRoot;
 using JsonBranch = Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase.JsonBranch;
+using EntityWithPrimitiveCollection = Microsoft.EntityFrameworkCore.Query.PrecompiledQueryRelationalTestBase.EntityWithPrimitiveCollection;
 
 namespace Microsoft.EntityFrameworkCore.Query;
 
@@ -96,7 +97,17 @@ protected override async Task SeedAsync(PrecompiledQueryRelationalTestBase.Preco
         };
 
         context.Posts.AddRange(post11, post12, post21, post22, post23);
+
+        context.EntitiesWithPrimitiveCollection.AddRange(
+            new EntityWithPrimitiveCollection { Id = 1, Tags = ["a", "b"] },
+            new EntityWithPrimitiveCollection { Id = 2, Tags = ["x", "y"] });
         await context.SaveChangesAsync();
+
+        // Overwrite the second entity's collection column with the JSON 'null' token (as legacy/external data might),
+        // rather than a SQL NULL, so the materializer's null-token peek path is exercised under precompilation.
+        await context.Database.ExecuteSqlRawAsync(
+            TestStore.NormalizeDelimitersInRawString(
+                "UPDATE [EntitiesWithPrimitiveCollection] SET [Tags] = 'null' WHERE [Id] = 2"));
     }
 
     public abstract PrecompiledQueryTestHelpers PrecompiledQueryTestHelpers { get; }

test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalTestBase.cs

@@ -1230,10 +1230,39 @@ public virtual Task Unsafe_accessor_gets_generated_once_for_multiple_queries()
             interceptorCodeAsserter: code => Assert.Equal(
                 2, code.Split("private static extern ref int UnsafeAccessor_Microsoft_EntityFrameworkCore_Query_Blog_Id_Set").Length));
 
+    // #34881/#38454: A primitive collection mapped to a column is read via a JsonValueReaderWriter that is emitted as a
+    // liftable constant. This test makes sure that liftable constant is handled correctly under query precompilation,
+    // including the JSON 'null' token peek path (the second entity's column holds the literal 'null' token).
+    [Fact]
+    public virtual Task Materialize_entity_with_primitive_collection_mapped_to_column()
+        => Test(
+            """
+var entities = await context.EntitiesWithPrimitiveCollection.OrderBy(e => e.Id).ToListAsync();
+
+Assert.Equal(2, entities.Count);
+Assert.Equal(new[] { "a", "b" }, entities[0].Tags);
+Assert.Null(entities[1].Tags);
+""");
+
+    // Projecting the collection column directly reaches the materializer without an IProperty, so the JsonValueReaderWriter
+    // is emitted via its (quotable) ConstructorExpression rather than a property-based liftable constant. This makes sure
+    // that path is quotable under precompilation, including the JSON 'null' token peek.
+    [Fact]
+    public virtual Task Project_primitive_collection_mapped_to_column()
+        => Test(
+            """
+var tags = await context.EntitiesWithPrimitiveCollection.OrderBy(e => e.Id).Select(e => e.Tags).ToListAsync();
+
+Assert.Equal(2, tags.Count);
+Assert.Equal(new[] { "a", "b" }, tags[0]);
+Assert.Null(tags[1]);
+""");
+
     public class PrecompiledQueryContext(DbContextOptions options) : DbContext(options)
     {
         public DbSet<Blog> Blogs { get; set; } = null!;
         public DbSet<Post> Posts { get; set; } = null!;
+        public DbSet<EntityWithPrimitiveCollection> EntitiesWithPrimitiveCollection { get; set; } = null!;
 
         protected override void OnModelCreating(ModelBuilder modelBuilder)
         {
@@ -1247,6 +1276,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
                 });
             modelBuilder.Entity<Blog>().HasMany(x => x.Posts).WithOne(x => x.Blog).OnDelete(DeleteBehavior.Cascade);
             modelBuilder.Entity<Post>().Property(x => x.Id).ValueGeneratedNever();
+            modelBuilder.Entity<EntityWithPrimitiveCollection>().Property(x => x.Id).ValueGeneratedNever();
         }
     }
 
@@ -1337,5 +1367,15 @@ public class Post
         public Blog? Blog { get; set; }
     }
 
+    public class EntityWithPrimitiveCollection
+    {
+        public int Id { get; set; }
+
+        // Mapped to a column as a JSON string via the primitive-collection convention (CollectionToJsonStringConverter).
+        // An array (rather than List<T>) is used so entity materialization assigns the collection directly instead of
+        // going through the PopulateList optimization (which is unrelated to the JSON null-token handling under test).
+        public string[]? Tags { get; set; }
+    }
+
     public static readonly IEnumerable<object[]> IsAsyncData = [[false], [true]];
 }