diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs index b666093d256..3ad8c4cfa86 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs +++ b/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,38 @@ static async Task InitializeReaderAsync( dataReaderContext.HasNext = false; } + /// + /// 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. + /// + [EntityFrameworkInternal] + public static object? ReadPrimitiveCollectionFromJson( + string? json, + JsonValueReaderWriter readerWriter, + bool nullable, + string propertyName) + { + if (json == null) + { + return null; + } + + // 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); + } + /// /// 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 diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs index 5810a9ce451..421dbe55c8a 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs +++ b/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,66 @@ 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 uses CollectionToJsonStringConverter, which parses the + // provider JSON string via the collection's JsonValueReaderWriter. That reader/writer doesn't handle a JSON 'null' + // token, so we must peek the first token here and short-circuit to null (or throw for a required property) before + // invoking the converter, rather than letting the reader/writer throw a cryptic "Invalid token type: 'Null'". + if (property is IProperty { IsPrimitiveCollection: true } primitiveCollectionProperty + && converter is not null + && converter.GetType() is { IsGenericType: true } converterClrType + && converterClrType.GetGenericTypeDefinition() == typeof(CollectionToJsonStringConverter<>)) + { + var liftableConstantParameter = Parameter(typeof(MaterializerLiftableConstantContext), "c"); + var jsonReaderWriterExpression = _parentVisitor.Dependencies.LiftableConstantFactory.CreateLiftableConstant( + primitiveCollectionProperty.GetJsonValueReaderWriter() ?? primitiveCollectionProperty.GetTypeMapping().JsonValueReaderWriter!, + Lambda>( + Coalesce( + Call( + LiftableConstantExpressionHelpers.BuildMemberAccessForProperty( + primitiveCollectionProperty, liftableConstantParameter), + PropertyGetJsonValueReaderWriterMethod), + Property( + Call( + LiftableConstantExpressionHelpers.BuildMemberAccessForProperty( + primitiveCollectionProperty, liftableConstantParameter), + PropertyGetTypeMappingMethod), + nameof(CoreTypeMapping.JsonValueReaderWriter))), + liftableConstantParameter), + primitiveCollectionProperty.Name + "JsonReaderWriter", + typeof(JsonValueReaderWriter)); + + if (valueExpression.Type != typeof(string)) + { + valueExpression = Convert(valueExpression, typeof(string)); + } + + Expression readExpression = Call( + ReadPrimitiveCollectionFromJsonMethodInfo, + valueExpression, + jsonReaderWriterExpression, + Constant(nullable), + Constant(primitiveCollectionProperty.Name)); + + 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 +3134,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 +3339,31 @@ private Expression CreateReadJsonPropertyValueExpression( nullExpression, resultExpression); } + else if (property.ClrType != typeof(string) + && property.ClrType.TryGetElementType(typeof(IEnumerable<>)) 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) { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs index 0c95c0550a4..8696ab91b4b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocJsonQuerySqlServerTestBase.cs @@ -473,6 +473,369 @@ INSERT INTO [Entities] ([Id], [Scenario], [OptionalReference], [RequiredReferenc """); } + #region NullablePrimitiveCollectionInJson + + [Fact] + public virtual async Task Project_json_with_null_nullable_primitive_collection() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreatingNullablePrimitiveCollectionInJson, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: SeedNullablePrimitiveCollectionInJson); + + using var context = contextFactory.CreateDbContext(); + var result = await context.Set().OrderBy(x => x.Id).ToListAsync(); + + Assert.Equal(2, result.Count); + + // Id == 1: JSON has "NullableStrings": null + Assert.Null(result[0].Reference.NullableStrings); + Assert.Equal(["a", "b"], result[0].Reference.RequiredStrings); + + // Id == 2: JSON has "NullableStrings": ["x", "y"] + Assert.Equal(["x", "y"], result[1].Reference.NullableStrings); + Assert.Equal(["c", "d"], result[1].Reference.RequiredStrings); + } + + protected virtual async Task SeedNullablePrimitiveCollectionInJson(DbContext ctx) + { + // nullable primitive collection is JSON null + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Reference]) +VALUES(1, '{"NullableStrings":null,"RequiredStrings":["a","b"]}') +"""); + + // nullable primitive collection has a value + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Reference]) +VALUES(2, '{"NullableStrings":["x","y"],"RequiredStrings":["c","d"]}') +"""); + } + + protected virtual void OnModelCreatingNullablePrimitiveCollectionInJson(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.OwnsOne(x => x.Reference, b => + { + b.ToJson().HasColumnType(JsonColumnType); + b.Property(x => x.NullableStrings).IsRequired(false); + b.Property(x => x.RequiredStrings).IsRequired(); + }); + }); + + protected class ContextNullablePrimitiveCollectionInJson(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public MyJsonEntity Reference { get; set; } + } + + public class MyJsonEntity + { + public List NullableStrings { get; set; } + public List RequiredStrings { get; set; } + } + } + + #endregion + + #region PrimitiveCollectionInColumn + + [Fact] + public virtual async Task Materialize_json_null_primitive_collection_mapped_to_column_is_null() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreatingPrimitiveCollectionInColumn, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: SeedPrimitiveCollectionInColumn); + + using var context = contextFactory.CreateDbContext(); + + // The primitive collection is mapped to its own column via CollectionToJsonStringConverter (which does NOT + // handle the JSON 'null' token). The materializer peeks the first token and short-circuits to null instead of + // letting JsonCollectionOfReferencesReaderWriter throw "Invalid token type: 'Null'". See #34881 / #38454. + var result = await context.Set().OrderBy(x => x.Id).ToListAsync(); + + Assert.Equal(2, result.Count); + Assert.Equal(["a", "b"], result[0].Tags); + Assert.Null(result[1].Tags); + } + + protected virtual async Task SeedPrimitiveCollectionInColumn(DbContext ctx) + { + // primitive collection column contains a JSON array + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Tags]) +VALUES(1, '["a","b"]') +"""); + + // primitive collection column contains a JSON null token + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Tags]) +VALUES(2, 'null') +"""); + } + + protected virtual void OnModelCreatingPrimitiveCollectionInColumn(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.PrimitiveCollection(x => x.Tags); + }); + + protected class ContextPrimitiveCollectionInColumn(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public List Tags { get; set; } + } + } + + #endregion + + #region JsonPropertyWithConverterThatConvertsNulls + + [Fact] + public virtual async Task Materialize_json_null_property_with_converter_that_converts_nulls() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreatingJsonPropertyWithConverterThatConvertsNulls, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: SeedJsonPropertyWithConverterThatConvertsNulls); + + using var context = contextFactory.CreateDbContext(); + var result = await context.Set().SingleAsync(x => x.Id == 1); + + Assert.Equal("e1", result.Reference.Name); + + // The JSON value is a 'Null' token. The property is nullable and its type mapping has a converter with + // ConvertsNulls == true, so the streaming JSON shaper's null guard routes the null through the converter's + // ConvertFromProvider(default) (which maps DB null to the "FROM_DB_NULL" sentinel) rather than returning + // default(string). See the converter?.ConvertsNulls branch in CreateReadJsonPropertyValueExpression. + Assert.Equal("FROM_DB_NULL", result.Reference.ConvertedNullable); + } + + protected virtual async Task SeedJsonPropertyWithConverterThatConvertsNulls(DbContext ctx) + => await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Reference]) +VALUES(1, '{"Name":"e1","ConvertedNullable":null}') +"""); + + protected virtual void OnModelCreatingJsonPropertyWithConverterThatConvertsNulls(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.OwnsOne(x => x.Reference, b => + { + b.ToJson().HasColumnType(JsonColumnType); + b.Property(x => x.ConvertedNullable).HasConversion( + new ValueConverter( + v => v, + v => v ?? "FROM_DB_NULL", + convertsNulls: true)); + }); + }); + + protected class ContextJsonPropertyWithConverterThatConvertsNulls(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public MyJsonEntity Reference { get; set; } + } + + public class MyJsonEntity + { + public string Name { get; set; } + public string ConvertedNullable { get; set; } + } + } + + #endregion + + #region JsonPropertyWithConverterThatDoesNotConvertNulls + + [Fact] + public virtual async Task Materialize_json_null_property_with_converter_that_does_not_convert_nulls() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreatingJsonPropertyWithConverterThatDoesNotConvertNulls, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: SeedJsonPropertyWithConverterThatDoesNotConvertNulls); + + using var context = contextFactory.CreateDbContext(); + var result = await context.Set().SingleAsync(x => x.Id == 1); + + Assert.Equal("e1", result.Reference.Name); + + // The JSON value is a 'Null' token. The property is nullable and its converter has ConvertsNulls == false, so + // the streaming JSON shaper's null guard returns default(string) (null) without invoking the converter. + Assert.Null(result.Reference.ConvertedNullable); + } + + protected virtual async Task SeedJsonPropertyWithConverterThatDoesNotConvertNulls(DbContext ctx) + => await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Reference]) +VALUES(1, '{"Name":"e1","ConvertedNullable":null}') +"""); + + protected virtual void OnModelCreatingJsonPropertyWithConverterThatDoesNotConvertNulls(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.OwnsOne(x => x.Reference, b => + { + b.ToJson().HasColumnType(JsonColumnType); + b.Property(x => x.ConvertedNullable).HasConversion( + new ValueConverter( + v => v, + v => "FROM_CONVERTER:" + v, + convertsNulls: false)); + }); + }); + + protected class ContextJsonPropertyWithConverterThatDoesNotConvertNulls(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public MyJsonEntity Reference { get; set; } + } + + public class MyJsonEntity + { + public string Name { get; set; } + public string ConvertedNullable { get; set; } + } + } + + #endregion + + #region 34881 + + [Fact] + public virtual async Task Materialize_legacy_serialized_json_null_primitive_collection_is_null() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreating34881, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: Seed34881); + + using var context = contextFactory.CreateDbContext(); + + // Legacy data serialized a null list as the JSON string "null" (rather than a SQL NULL). Utf8JsonReader + // tokenizes "null" as JsonTokenType.Null; materialization must yield a null collection. See issue #34881. + var result = await context.Set().OrderBy(x => x.Id).ToListAsync(); + + Assert.Equal(2, result.Count); + Assert.Equal(["a", "b"], result[0].DisplayPath); + Assert.Null(result[1].DisplayPath); + } + + protected virtual async Task Seed34881(DbContext ctx) + { + // legacy serialized array + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [DisplayPath]) +VALUES(1, N'["a","b"]') +"""); + + // legacy serialized null (the literal string "null", not a SQL NULL) + await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [DisplayPath]) +VALUES(2, N'null') +"""); + } + + protected virtual void OnModelCreating34881(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.PrimitiveCollection(x => x.DisplayPath); + }); + + protected class Context34881(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public IList DisplayPath { get; set; } + } + } + + #endregion + + #region RequiredPrimitiveCollectionInJson + + [Fact] + public virtual async Task Materialize_json_null_required_primitive_collection_in_json_throws() + { + var contextFactory = await InitializeNonSharedTest( + onModelCreating: OnModelCreatingRequiredPrimitiveCollectionInJson, + onConfiguring: b => b.ConfigureWarnings(ConfigureWarnings), + seed: SeedRequiredPrimitiveCollectionInJson); + + using var context = contextFactory.CreateDbContext(); + + // A required primitive collection nested in a JSON document whose value is a 'null' token yields a clear, + // property-named error rather than the cryptic reader/writer "Invalid token type: 'Null'". + var exception = await Assert.ThrowsAsync( + () => context.Set().OrderBy(x => x.Id).ToListAsync()); + + Assert.Equal(RelationalStrings.NullValueInRequiredJsonProperty("RequiredStrings"), exception.Message); + } + + protected virtual async Task SeedRequiredPrimitiveCollectionInJson(DbContext ctx) + => await ctx.Database.ExecuteSqlAsync( + $$""" +INSERT INTO [Entities] ([Id], [Reference]) +VALUES(1, '{"RequiredStrings":null}') +"""); + + protected virtual void OnModelCreatingRequiredPrimitiveCollectionInJson(ModelBuilder modelBuilder) + => modelBuilder.Entity(b => + { + b.ToTable("Entities"); + b.Property(x => x.Id).ValueGeneratedNever(); + b.OwnsOne(x => x.Reference, b => + { + b.ToJson().HasColumnType(JsonColumnType); + b.Property(x => x.RequiredStrings).IsRequired(); + }); + }); + + protected class ContextRequiredPrimitiveCollectionInJson(DbContextOptions options) : DbContext(options) + { + public class MyEntity + { + public int Id { get; set; } + public MyJsonEntity Reference { get; set; } + } + + public class MyJsonEntity + { + public List RequiredStrings { get; set; } + } + } + + #endregion + #region EnumLegacyValues [Theory, MemberData(nameof(IsAsyncData))]