From f331e677ea212462681c68a1de748c0cbea097e1 Mon Sep 17 00:00:00 2001 From: Phil Carbone Date: Thu, 17 Sep 2026 20:46:05 -0400 Subject: [PATCH] Materialize required complex collection absent from JSON document as empty - A required complex collection whose key is absent from the stored JSON document (e.g. a row persisted before the collection was added to the type) was materialized as null: the nested-property fixup ran with a null value and overwrote the instance's collection. The JSON materializer now tracks whether each nested property was present in the document and assigns an empty collection when a required complex collection is absent. An explicit JSON null is still materialized as null. - CheckForNullComplexProperties threw PropertyDoesNotBelong instead of NullRequiredComplexProperty for a null required complex collection on a complex collection element, because it looked up the containing complex property on the element's own entry. - Regression tests in ComplexCollectionJsonUpdateTestBase, with SQL Server and SQLite baselines. Fixes #38625 --- ...sitor.ShaperProcessingExpressionVisitor.cs | 105 +++++++++++++++--- .../Internal/InternalEntryBase.cs | 7 ++ ...AdHocPrecompiledQueryRelationalTestBase.cs | 7 ++ .../ComplexCollectionJsonUpdateTestBase.cs | 105 ++++++++++++++++++ ...omplexCollectionJsonUpdateSqlServerTest.cs | 17 +++ .../ComplexCollectionJsonUpdateSqliteTest.cs | 15 +++ 6 files changed, 238 insertions(+), 18 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs index e4f18df01e0..55443eff6ae 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs @@ -58,6 +58,9 @@ private static readonly MethodInfo CollectionAccessorGetOrCreateMethodInfo private static readonly MethodInfo CollectionAccessorAddMethodInfo = typeof(IClrCollectionAccessor).GetTypeInfo().GetDeclaredMethod(nameof(IClrCollectionAccessor.Add))!; + private static readonly MethodInfo CollectionAccessorCreateMethodInfo + = typeof(IClrCollectionAccessor).GetTypeInfo().GetDeclaredMethod(nameof(IClrCollectionAccessor.Create))!; + private static readonly PropertyInfo ObjectArrayIndexerPropertyInfo = typeof(object[]).GetProperty("Item")!; @@ -1609,6 +1612,7 @@ private Expression CreateJsonShapers( var innerShapersMap = new Dictionary(); var innerFixupMap = new Dictionary(); var trackingInnerFixupMap = new Dictionary(); + var innerAbsentPropertyFixupMap = new Dictionary(); // Go over all structural properties (complex properties and navigations - if we're an (owned) entity), which represent JSON // nested types; generate shapers and fixup to wire the materialized related instance into the parent's property. @@ -1716,6 +1720,15 @@ private Expression CreateJsonShapers( innerFixupMap[navigationJsonPropertyName] = fixup; + // A required complex collection absent from the document (e.g. added to the type after the row was persisted) must + // materialize as empty rather than null; an explicit JSON null is still materialized as null. See #38625. + if (nestedStructuralProperty is IComplexProperty { IsNullable: false } + && !nestedStructuralProperty.IsShadowProperty()) + { + innerAbsentPropertyFixupMap[navigationJsonPropertyName] = + GenerateAbsentCollectionFixupForJson(structuralType.ClrType, nestedStructuralProperty); + } + var trackedFixup = Lambda( Block(typeof(void), expressionsForTracking), shaperEntityParameter, @@ -1750,7 +1763,8 @@ private Expression CreateJsonShapers( jsonReaderDataShaperLambdaParameter, innerShapersMap, innerFixupMap, - trackingInnerFixupMap).Rewrite(structuralTypeShaperMaterializer); + trackingInnerFixupMap, + innerAbsentPropertyFixupMap).Rewrite(structuralTypeShaperMaterializer); var entityShaperMaterializerVariable = Variable( structuralTypeShaperMaterializer.Type, @@ -1899,7 +1913,8 @@ private sealed class JsonEntityMaterializerRewriter( ParameterExpression jsonReaderDataParameter, IDictionary innerShapersMap, IDictionary innerFixupMap, - IDictionary trackingInnerFixupMap) + IDictionary trackingInnerFixupMap, + IDictionary innerAbsentPropertyFixupMap) : ExpressionVisitor { private static readonly PropertyInfo JsonEncodedTextEncodedUtf8BytesProperty @@ -1912,6 +1927,9 @@ private static readonly MethodInfo JsonEncodedTextEncodeMethod // which happens at the end (after we read everything to guarantee that we can instantiate the entity private readonly Dictionary _navigationVariableMap = []; + // tracks whether a navigation appeared in the JSON document at all, to distinguish absent from explicitly null + private readonly Dictionary _navigationReadVariableMap = []; + public BlockExpression Rewrite(BlockExpression jsonEntityShaperMaterializer) => (BlockExpression)VisitBlock(jsonEntityShaperMaterializer); @@ -2090,7 +2108,7 @@ void ProcessFixup(IDictionary fixupMap) { foreach (var fixup in fixupMap) { - var navigationEntityParameter = _navigationVariableMap[fixup.Key]; + var navigationVariable = _navigationVariableMap[fixup.Key]; // Inject the fixup code for each property; we have this as a set of lambdas in the fixup map. // In the normal case, simply Invoke the lambda, passing it the structural type to be fixed up as a parameter. @@ -2098,30 +2116,49 @@ void ProcessFixup(IDictionary fixupMap) // we unwrap the lambda and integrate its body directly. // We should ideally do this for all cases (no need for the extra lambda Invoke), but there are some issues around us writing // to readonly fields. - if (jsonStructuralTypeVariable.Type - .IsValueType /*&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null*/) - { - var fixupBody = ReplacingExpressionVisitor.Replace( + var isValueType = jsonStructuralTypeVariable.Type + .IsValueType /*&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null*/; + + var fixupExpression = isValueType + ? ReplacingExpressionVisitor.Replace( originals: [fixup.Value.Parameters[0], fixup.Value.Parameters[1]], - replacements: [jsonStructuralTypeVariable, _navigationVariableMap[fixup.Key]], - fixup.Value.Body); + replacements: [jsonStructuralTypeVariable, navigationVariable], + fixup.Value.Body) + : Invoke(fixup.Value, jsonStructuralTypeVariable, navigationVariable); - finalBlockExpressions.Add(fixupBody); + if (innerAbsentPropertyFixupMap.TryGetValue(fixup.Key, out var absentPropertyFixup)) + { + Check.DebugAssert( + absentPropertyFixup.Parameters is [{ } absentFixupParameter] + && absentFixupParameter.Type == jsonStructuralTypeVariable.Type, + "The absent-property fixup must take only the instance being materialized"); + Check.DebugAssert( + _navigationReadVariableMap.ContainsKey(fixup.Key), + "No read flag was generated for a property with an absent-property fixup"); + + var absentPropertyFixupExpression = isValueType + ? ReplacingExpressionVisitor.Replace( + absentPropertyFixup.Parameters[0], jsonStructuralTypeVariable, absentPropertyFixup.Body) + : Invoke(absentPropertyFixup, jsonStructuralTypeVariable); + + fixupExpression = IfThenElse( + _navigationReadVariableMap[fixup.Key], + fixupExpression, + absentPropertyFixupExpression); } - else + + if (!isValueType) { // If the structural type being fixed up is nullable, then we need to add null checks before we run fixup logic. // For regular entities, whose fixup is done as part of the "Materialize*" method, the checks are done there // (the same will be done for the "optimized" scenario, where we populate properties directly rather than store in variables). // But in this case fixups are standalone, so the null safety must be added here. - finalBlockExpressions.Add( - IfThen( - NotEqual(jsonStructuralTypeVariable, Constant(null, jsonStructuralTypeVariable.Type)), - Invoke( - fixup.Value, - jsonStructuralTypeVariable, - _navigationVariableMap[fixup.Key]))); + fixupExpression = IfThen( + NotEqual(jsonStructuralTypeVariable, Constant(null, jsonStructuralTypeVariable.Type)), + fixupExpression); } + + finalBlockExpressions.Add(fixupExpression); } } } @@ -2211,6 +2248,15 @@ void ProcessFixup(IDictionary fixupMap) _navigationVariableMap[innerShaperMapElementKey] = propertyVariable; + Expression markAsRead = Empty(); + if (innerAbsentPropertyFixupMap.ContainsKey(innerShaperMapElementKey)) + { + var propertyReadVariable = Variable(typeof(bool)); + finalBlockVariables.Add(propertyReadVariable); + _navigationReadVariableMap[innerShaperMapElementKey] = propertyReadVariable; + markAsRead = Assign(propertyReadVariable, Constant(true)); + } + var moveNext = Call(managerVariable, Utf8JsonReaderManagerMoveNextMethod); var captureState = Call(managerVariable, Utf8JsonReaderManagerCaptureStateMethod); var assignment = Assign(propertyVariable, innerShaperMapElement.Value); @@ -2227,6 +2273,7 @@ void ProcessFixup(IDictionary fixupMap) captureState, assignment, managerRecreation, + markAsRead, Empty())); } @@ -2907,6 +2954,28 @@ private Expression GetOrCreateCollectionObjectLambda(Type entityType, IPropertyB prm); } + private LambdaExpression GenerateAbsentCollectionFixupForJson(Type clrType, IPropertyBase structuralProperty) + { + var entityParameter = Parameter(clrType); + var setter = structuralProperty.GetMemberInfo(forMaterialization: true, forSet: true); + + return Lambda( + Block( + typeof(void), + entityParameter.MakeMemberAccess(setter) + .Assign( + Convert( + Call( + _parentVisitor.Dependencies.LiftableConstantFactory.CreateLiftableConstant( + structuralProperty.GetCollectionAccessor(), + LiftableConstantExpressionHelpers.BuildClrCollectionAccessorLambda(structuralProperty), + structuralProperty.Name + "StructuralPropertyCollectionAccessor", + typeof(IClrCollectionAccessor)), + CollectionAccessorCreateMethodInfo), + setter.GetMemberType()))), + entityParameter); + } + private Expression AddToCollectionStructuralProperty( ParameterExpression entity, ParameterExpression relatedEntity, diff --git a/src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs b/src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs index 20c4b693d31..1d72c16deff 100644 --- a/src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs +++ b/src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs @@ -1721,10 +1721,17 @@ void CheckForNullComplexProperties() { foreach (var complexProperty in structuralType.GetFlattenedComplexProperties()) { + Check.DebugAssert( + complexProperty.DeclaringType is not IComplexType { ComplexProperty.IsCollection: true } + || complexProperty.DeclaringType == structuralType, + "Properties of a complex collection element type are only flattened into the element's own entry"); + if (!complexProperty.IsNullable && this[complexProperty] == null && complexProperty.ComplexType.GetProperties().Any(p => !p.IsNullable) && (complexProperty.DeclaringType is not IComplexType complexType + // A collection element has its own entry; its containing property lives on the parent's entry + || complexType.ComplexProperty.IsCollection || GetCurrentValue(complexType.ComplexProperty) != null)) { throw new InvalidOperationException( diff --git a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs index a36e2612871..6c43c004364 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs @@ -371,6 +371,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) b.ToJson(); b.Property(x => x.Name).HasJsonPropertyName("1!NOT VALID;"); b.Property(x => x.Name2).HasJsonPropertyName("1-NOT VALID!"); + b.ComplexCollection(x => x.Items, cb => cb.HasJsonPropertyName("1!NOT VALID COLLECTION;")); }); } @@ -385,6 +386,12 @@ public class InvalidNameNestedEntity { public string Name { get; set; } = ""; public string Name2 { get; set; } = ""; + public List Items { get; set; } = []; + } + + public class InvalidNameNestedItem + { + public string Value { get; set; } = ""; } [Fact] diff --git a/test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs index bfd94bd347c..e2bbbe930c6 100644 --- a/test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs @@ -667,6 +667,111 @@ public virtual Task Set_nullable_complex_property_with_nested_collection_to_null } }); + [Fact] // Issue #38625 + public virtual Task Complex_collection_absent_from_json_is_materialized_as_empty() + => TestHelpers.ExecuteWithStrategyInTransactionAsync( + CreateContext, + UseTransaction, + async context => + { + // Simulate a row persisted before the Others sub-collection was added to the type, + // so the key is absent from the stored document. + await SetStoredDocumentAsync( + context, "Widgets", "Deep", id: 1, """{"Mid":{"Items":[{"Title":"Item1","Inner":[{"Value":"inner-0"}]}]}}"""); + + // Tracking and no-tracking queries go through separate fixup paths. + AssertMaterialized(await context.Set().OrderBy(w => w.Id).FirstAsync()); + AssertMaterialized(await context.Set().AsNoTracking().OrderBy(w => w.Id).FirstAsync()); + + static void AssertMaterialized(WidgetWithDeepJson widget) + { + var item = Assert.Single(widget.Deep.Mid.Items); + + Assert.Equal("Item1", item.Title); + Assert.Equal("inner-0", Assert.Single(item.Inner).Value); + + // A complex collection that is absent from the JSON document materializes as an empty collection, not null. + Assert.NotNull(item.Others); + Assert.Empty(item.Others); + } + }); + + [Fact] // Issue #38625 + public virtual Task Complex_collection_explicitly_null_in_json_is_materialized_as_null() + => TestHelpers.ExecuteWithStrategyInTransactionAsync( + CreateContext, + UseTransaction, + async context => + { + // Unlike an absent key, an explicit null in the document is preserved. + await SetStoredDocumentAsync( + context, "Widgets", "Deep", id: 1, + """{"Mid":{"Items":[{"Title":"Item1","Inner":[{"Value":"inner-0"}],"Others":null}]}}"""); + + var widget = await context.Set().OrderBy(w => w.Id).FirstAsync(); + Assert.Null(Assert.Single(widget.Deep.Mid.Items).Others); + + var untrackedWidget = await context.Set().AsNoTracking().OrderBy(w => w.Id).FirstAsync(); + Assert.Null(Assert.Single(untrackedWidget.Deep.Mid.Items).Others); + }); + + [Fact] // Issue #38625 + public virtual Task Save_changes_after_loading_row_with_complex_collection_absent_from_json() + => TestHelpers.ExecuteWithStrategyInTransactionAsync( + CreateContext, + UseTransaction, + async context => + { + await SetStoredDocumentAsync( + context, "Widgets", "Deep", id: 1, """{"Mid":{"Items":[{"Title":"Item1","Inner":[{"Value":"inner-0"}]}]}}"""); + + var widget = await context.Set().OrderBy(w => w.Id).FirstAsync(); + + // Modifying an unrelated scalar must not make the row unsaveable. + widget.Deep.Mid.Items[0].Title = "Item1-updated"; + + ClearLog(); + await context.SaveChangesAsync(); + }, + async context => + { + using (SuspendRecordingEvents()) + { + var widget = await context.Set().OrderBy(w => w.Id).FirstAsync(); + var item = Assert.Single(widget.Deep.Mid.Items); + Assert.Equal("Item1-updated", item.Title); + Assert.Empty(item.Others); + } + }); + + [Fact] // Issue #38625 + public virtual Task Saving_null_required_complex_collection_in_complex_collection_element_throws() + => TestHelpers.ExecuteWithStrategyInTransactionAsync( + CreateContext, + UseTransaction, + async context => + { + var widget = await context.Set().OrderBy(w => w.Id).FirstAsync(); + widget.Deep.Mid.Items[0].Others = null!; + + Assert.Equal( + CoreStrings.NullRequiredComplexProperty(nameof(DeepItem), nameof(DeepItem.Others)), + (await Assert.ThrowsAsync(() => context.SaveChangesAsync())).Message); + }); + + private static Task SetStoredDocumentAsync(DbContext context, string table, string column, int id, string json) + { + // Identifiers are delimited by the provider so that any provider can run this test; the document is inlined rather than + // parameterized so that providers with a dedicated JSON store type need no cast. Braces are escaped for string.Format. + var sqlGenerationHelper = context.GetService(); + + return context.Database.ExecuteSqlRawAsync( + $"UPDATE {Q(table)} SET {Q(column)} = '{json.Replace("{", "{{").Replace("}", "}}")}' WHERE {Q("Id")} = {id}"); + + string Q(string name) + => sqlGenerationHelper.DelimitIdentifier(name); + } + protected virtual void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction) => facade.UseTransaction(transaction.GetDbTransaction()); diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs index 82ce7f641d7..1aa2848bb1b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs @@ -361,6 +361,23 @@ OUTPUT 1 """); } + public override async Task Save_changes_after_loading_row_with_complex_collection_absent_from_json() + { + await base.Save_changes_after_loading_row_with_complex_collection_absent_from_json(); + + AssertSql( + """ +@p0='{"Mid":{"Items":[{"Title":"Item1-updated","Inner":[{"Value":"inner-0"}],"Others":[]}]}}' (Nullable = false) (Size = 87) +@p1='1' + +SET IMPLICIT_TRANSACTIONS OFF; +SET NOCOUNT ON; +UPDATE [Widgets] SET [Deep] = @p0 +OUTPUT 1 +WHERE [Id] = @p1; +"""); + } + public class ComplexCollectionJsonUpdateSqlServerFixture : ComplexCollectionJsonUpdateFixtureBase { protected override ITestStoreFactory TestStoreFactory diff --git a/test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs index f0da51d2c1c..0cbcd4550b0 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs @@ -320,6 +320,21 @@ public override async Task Set_nullable_complex_property_with_nested_collection_ """); } + public override async Task Save_changes_after_loading_row_with_complex_collection_absent_from_json() + { + await base.Save_changes_after_loading_row_with_complex_collection_absent_from_json(); + + AssertSql( + """ +@p0='{"Mid":{"Items":[{"Title":"Item1-updated","Inner":[{"Value":"inner-0"}],"Others":[]}]}}' (Nullable = false) (Size = 87) +@p1='1' + +UPDATE "Widgets" SET "Deep" = @p0 +WHERE "Id" = @p1 +RETURNING 1; +"""); + } + public class ComplexCollectionJsonUpdateSqliteFixture : ComplexCollectionJsonUpdateFixtureBase { protected override ITestStoreFactory TestStoreFactory