Skip to content

Materialize required complex collection absent from JSON document as empty - #39014

Open
philcarbone wants to merge 1 commit into
dotnet:mainfrom
philcarbone:fix/38625-json-absent-complex-collection
Open

philcarbone wants to merge 1 commit into
dotnet:mainfrom
philcarbone:fix/38625-json-absent-complex-collection

Conversation

@philcarbone

@philcarbone philcarbone commented Sep 18, 2026

Copy link
Copy Markdown

Fixes #38625

When a complex type mapped to JSON via ComplexProperty().ToJson() contains a required (non-nullable) complex collection whose key is absent from the stored document (typically a row persisted before the collection was added to the type), the materializer produced null for that collection. Reading the collection then throws NullReferenceException, and saving the tracked entity after any change throws from PrepareToSave, so such rows are effectively poison-on-touch.

Per the discussion on the issue, an absent required complex collection now materializes as an empty collection (option (b)). An explicit JSON null is still materialized as null, so Set_complex_collection_to_null_mapped_to_json and friends are unaffected.

Changes

  • ShaperProcessingExpressionVisitor / JsonEntityMaterializerRewriter: 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 had 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. Nullable complex collections and owned navigations keep their current behavior: an optional collection may legitimately be null, and this keeps the behavior change to the case that is actually broken (a required collection that can neither be read nor saved). It would be a one-line change to extend the coalescing to nullable complex collections too, if the team prefers that.
  • InternalEntryBase.CheckForNullComplexProperties: the "is the containing complex object null?" guard called 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 this threw the misleading PropertyDoesNotBelong exception seen in the issue instead of NullRequiredComplexProperty. The guard is now skipped for collection element types. This is the exception a user now gets if they explicitly set a required nested complex collection to null and save; happy to split it into its own PR if preferred.

Behavioral change

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

Owned JSON navigation collections have the same absent-key behavior today; I left them alone to keep this to the reported scenario, but the same mechanism would apply if the team wants it. The Cosmos provider has its own JSON materializer and may need an equivalent look; I can file a follow-up.

Tests

Added to ComplexCollectionJsonUpdateTestBase, using the existing WidgetWithDeepJson fixture and rewriting the stored document with raw SQL to simulate an old-shape row, with SQL baselines in the SQL Server and SQLite test classes for the save case:

  • Complex_collection_absent_from_json_is_materialized_as_empty (tracking and no-tracking)
  • Complex_collection_explicitly_null_in_json_is_materialized_as_null (an explicit null is preserved; only an absent key is coalesced)
  • Save_changes_after_loading_row_with_complex_collection_absent_from_json
  • Saving_null_required_complex_collection_in_complex_collection_element_throws

The first, third and fourth fail on main without the fix (the first and third with the symptoms in the issue, the fourth with the PropertyDoesNotBelong message) and pass with it. The second passes before and after; it guards the absent-versus-explicit-null distinction.

Invalid_identifier_json_property_name (precompiled queries) now also has a required complex collection whose JSON name is not a valid C# identifier, since the read flag introduced here is a shaper block variable and the precompiled-query translator emits those names verbatim.

Locally: EFCore.Tests, EFCore.Relational.Tests, EFCore.InMemory.FunctionalTests and the full EFCore.Sqlite.FunctionalTests suite are green with these changes. On SQL Server 2022 (Docker), ComplexCollectionJsonUpdateSqlServerTest passes 24/24 and the JSON and complex-type query/update classes are green apart from the *JsonType* fixtures, which need the native json type from SQL Server 2025.

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'd 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 can't 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 and is green on SQLite and SQL Server there.


  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@philcarbone
philcarbone force-pushed the fix/38625-json-absent-complex-collection branch from 6853d6b to fce76bb Compare September 18, 2026 02:26
@philcarbone
philcarbone marked this pull request as ready for review September 18, 2026 02:44
@philcarbone
philcarbone requested a review from a team as a code owner September 18, 2026 02:44
Copilot AI lite review requested due to automatic review settings September 18, 2026 02:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 AI review requested due to automatic review settings September 18, 2026 02:58
@philcarbone
philcarbone force-pushed the fix/38625-json-absent-complex-collection branch from fce76bb to 5f3594c Compare September 18, 2026 02:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

…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 dotnet#38625
Copilot AI review requested due to automatic review settings September 18, 2026 03:10
@philcarbone
philcarbone force-pushed the fix/38625-json-absent-complex-collection branch from 5f3594c to f331e67 Compare September 18, 2026 03:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

philcarbone commented Sep 18, 2026

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants