[philcarbone]

Fixes #38625

When a complex type is mapped to JSON via ComplexProperty().ToJson() and the stored document is missing the key for a required collection inside it (typically a row persisted before that collection was added to the type), the materializer produced null for the collection. Reading it then throws NullReferenceException, and saving the tracked entity after any change throws from PrepareToSave, so such rows are poison-on-touch. This covers both kinds of collection the issue shows: the nested complex collection (Parcels) and the primitive collection (InspectionTagIds) in the repro.

Per the discussion on the issue, an absent required collection now materializes as an empty collection (option (b), per @AndriySvyryd's note that the materializer does not preserve default property values). Only the absent case changes: an explicit JSON null keeps whatever behavior it has today, which is not the same for the two collection kinds. There is a question about that below.

Changes

• ShaperProcessingExpressionVisitor / JsonEntityMaterializerRewriter (complex collections). The nested-property fixups run unconditionally after the JSON read loop, so a property that never appeared in the document had its fixup invoked with null, overwriting whatever the CLR initializer produced. The rewriter now tracks whether each nested property was actually read; for a required complex collection that was not read, an absent-property fixup assigns a new empty collection created through the property's IClrCollectionAccessor (the same accessor used for []), via a liftable constant so precompiled queries keep working.
• ShaperProcessingExpressionVisitor (primitive collections). Scalar properties are read in the same loop and left at their default when the key is absent, which is how the List<Guid> in the issue's repro comes back null. A required primitive collection now gets the same treatment: a read flag, and after the loop an empty collection of the type the JSON reader/writer itself would have created. That rule (array, concrete type with a public parameterless constructor, else List<T>) lived as a local function in TypeMappingSourceBase.TryFindJsonCollectionMapping; it moves to a shared helper so the shaper and the type mapping cannot drift apart. ReadOnlyCollection<T> is built the way the reader/writer builds it, from a list, which is the one read-only shape it supports besides arrays. A property configured with its own JsonValueReaderWriter is skipped entirely, since only that reader/writer knows what it would have created, and so is a declared type that nothing can instantiate; in both cases an absent key still materializes as null, as before, and such a row therefore stays unsaveable. Handling those properly would mean asking the reader/writer for an empty value, which looks like an API addition rather than something to slip into this fix.
• InternalEntryBase, both null-collection checks. CheckForNullComplexProperties and CheckForNullCollection guard with "is the containing complex object null?", calling GetCurrentValue(complexType.ComplexProperty) on the current entry even when that entry is a complex collection element. The containing property lives on the parent's entry, so both threw the internal PropertyDoesNotBelong message, which is exactly the confusing exception pasted in the issue, instead of NullRequiredComplexProperty / NullRequiredPrimitiveCollection. Both now skip the guard for collection element types. This is a message fix, not a new check: saving in this state already threw, just unintelligibly.

Behavioral change

For a required collection inside a JSON-mapped complex type, a document lacking that collection's key now materializes an empty collection where it previously produced null and an unsaveable entity. Documents written by EF always contain the key, so this affects rows written before the collection existed on the type, or by another writer. Nullable collections, explicit JSON null, and owned JSON navigations are unchanged.

This matches what the Cosmos provider already does for an absent key: it creates a nested collection before its property read loop (source).

One question for the team: explicit JSON null

This change is about an absent key and deliberately leaves an explicit null alone, but that exposes a divergence that already exists on main and is worth settling:

document	required primitive collection	required complex collection
key absent	empty (this PR; was null)	empty (this PR; was null)
"X": null	throws NullValueInRequiredJsonProperty	materializes null, then throws at SaveChanges

So for an explicit null the primitive side already fails fast with a named error, while the complex side hands back an entity that cannot be saved. Cosmos, for comparison, flattens an explicit null to empty for both. I have not touched any of that here, and Complex_collection_explicitly_null_in_json_is_materialized_as_null records today's complex behavior rather than proposing it. Happy to make the complex side throw the same error as the primitive one, or flatten both to empty like Cosmos, whichever the team prefers.

The primitive-collection read loop is shared with owned JSON entities and with the root of a JSON document, so required primitive collections there get the same treatment, covered by Primitive_collection_absent_from_json_in_owned_entity_is_materialized_as_empty. That is deliberate: it is the same bug and the same fix, and carving owned types out would be arbitrary. It does mean an application that reads null from such a key today, for instance to detect rows predating the property, sees an empty collection instead, and the next save writes "X":[] into the document. That seems worth a breaking-change note; tell me where you would like it and I will write it.

Still out of scope, and reported separately where they are bugs: owned JSON navigation collections have the same absent-key behavior (#39060), and a required non-collection complex property that is absent from the document still materializes as null.

Tests

Added to ComplexCollectionJsonUpdateTestBase, rewriting the stored document with raw SQL to simulate an old-shape row, with SQL baselines in the SQL Server and SQLite classes for the save cases:

• Complex_collection_absent_from_json_is_materialized_as_empty (tracking and no-tracking)
• Complex_collection_absent_from_json_in_value_type_complex_type_is_materialized_as_empty (a struct complex type, which takes the inlined fixup path rather than the invoked one)
• Save_changes_after_loading_row_with_complex_collection_absent_from_json
• Saving_null_required_complex_collection_in_complex_collection_element_throws
• Primitive_collection_absent_from_json_is_materialized_as_empty (tracking and no-tracking; the document has three elements, the middle one carrying the key, so a read flag cannot bleed across elements). The element type carries one property per way the collection to create is decided, so each branch of the rule is exercised: List<int> for a concrete type with a parameterless constructor, int[] for an array, IList<int> for the List<T> fallback, List<string> for reference elements, ReadOnlyCollection<int> for the shape the reader/writer builds from a list, plus the two deliberate non-cases, an optional collection and one with its own JsonValueReaderWriter, which stay null
• Primitive_collection_absent_from_json_in_owned_entity_is_materialized_as_empty
• Save_changes_after_loading_row_with_primitive_collection_absent_from_json
• Saving_null_required_primitive_collection_in_complex_collection_element_throws
• Complex_collection_explicitly_null_in_json_is_materialized_as_null

All of them fail without the corresponding change except the last, which passes before and after: it is there to pin the absent-versus-explicit-null distinction, not as a regression test. The two "saving null" tests fail on main with the PropertyDoesNotBelong message rather than the intended one.

Invalid_identifier_json_property_name (precompiled queries) now also has a required complex collection and a required primitive collection whose JSON names are not valid C# identifiers, so the read flags and the empty-collection creation added here are exercised through the precompiled-query translator as well. That is also how the identifier bug fixed separately in #39062 surfaced.

Servicing

Any application that adds a collection to an existing JSON-mapped complex type hits this on every pre-existing row, with no model-level workaround; the issue has details. Since main is now 12.0, I would like to ask for a backport to release/11.0 so this makes 11.0 GA, and for servicing consideration for 10.0.x, in the same vein as the complex-collection fixes that shipped in 10.0.3 (#37373), 10.0.6 (#37725) and 10.0.10 (#38299). External PRs cannot target release branches, so those backports would need to come from the team once this is approved. A ready cherry-pick for release/10.0, with a quirk switch (Microsoft.EntityFrameworkCore.Issue38625) and the tests adapted to that branch's conventions, is on my fork at backport/38625-release-10.0; it predates the primitive-collection part and I will refresh it if the team wants it.

[Copilot]

🟡 Changes recommended

Add coverage for explicit nested JSON nulls and no-tracking materialization.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes JSON materialization of absent required complex collections and corrects null validation for complex collection elements.

Changes:

• Materializes absent required collections as empty.
• Corrects validation for complex collection elements.
• Adds regression tests and SQL baselines.

File summaries

File	Summary
test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs	Adds SQL Server baselines.
test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs	Adds SQLite baselines.
test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs	Adds regression tests.
src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs	Fixes null validation for collection elements.
src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs	Tracks JSON property presence and applies empty-collection fixups.

Review details

Suppressed comments (1)

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

• The new read flag controls the only distinction between an absent key and an explicit JSON null, but the added tests exercise only the absent case. The existing Set_complex_collection_to_null_mapped_to_json test covers a top-level JSON collection and does not enter this nested innerAbsentPropertyFixupMap path, so a regression could incorrectly coalesce an explicit nested null to an empty collection while all current tests pass. Please add a base test that stores "Others":null and asserts that the nested property remains null.

                        if (innerAbsentPropertyFixupMap.ContainsKey(innerShaperMapElementKey))
                        {
                            var propertyReadVariable = Variable(typeof(bool), innerShaperMapElementKey + "Read");
                            finalBlockVariables.Add(propertyReadVariable);
                            _navigationReadVariableMap[innerShaperMapElementKey] = propertyReadVariable;

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical issues remain with precompiled queries and the regression-test JSON setup.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

Regression, provider, and precompiled-query coverage is included with no unresolved blocking issues.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[philcarbone]

I had opened #39017 as a release/10.0 backport, but external PRs can't target release branches, so the bot closed it. The cherry-pick is ready on my fork if it saves anyone time: philcarbone/efcore:backport/38625-release-10.0. It adds an AppContext quirk (Microsoft.EntityFrameworkCore.Issue38625), switches the new tests to [ConditionalFact], and drops the precompiled-query test extension that branch doesn't have; it passes the JSON and complex-type suites on SQLite and SQL Server there. Once this PR is approved I'd ask the team to backport to release/11.0 and release/10.0.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The changes span materialization, change tracking, and multiple provider test paths, warranting final human review.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🟢 Approval recommended

The reviewed changes are covered by regression and provider tests with no unresolved blocking issues.

Review effort: Lite
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

The unresolved array-collection coverage request and complex materializer changes warrant final human review.

Review effort: Lite
Findings: 1

Open (1)

• Add absent-key regression coverage for array-valued required collections · New

Resolved since last review (1)

• Inconsistent construction for custom collections with IList<T> constructors

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate collection-construction compatibility issue remains unresolved.

Review effort: Lite
Findings: 1

Open (1)

• Add absent-key regression coverage for array-valued required collections

[Copilot]

Copilot review overview

🟢 Approval recommended

The reviewed changes have complete regression coverage and no unresolved blocking issues.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Add absent-key regression coverage for array-valued required collections

[AndriySvyryd]

Happy to make the complex side throw the same error as the primitive one, or flatten both to empty like Cosmos, whichever the team prefers.

We should make this consistent with Cosmos

I had opened #39017 as a release/10.0 backport, but external PRs can't target release branches, so the bot closed it.

Query changes carry an inherent risk and currently that haven't been many reports of this issue, so we aren't going to port it to 10.x or 11.x.

[Copilot]

Copilot review overview

🟡 Changes recommended

Explicit JSON null must retain its existing behavior for both complex and primitive collections.

Review effort: Lite
Findings: 2

Open (2)

• Preserve explicit null for required complex collections · New
• Keep null errors for required primitive collections · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Explicit JSON null behavior must remain consistent with the stated scope and existing behavior.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Keep null errors for required primitive collections
• Preserve explicit null for required complex collections

[AndriySvyryd]

Thanks for your contribution!