Materialize required complex collection absent from JSON document as empty - #39014
philcarbone wants to merge 1 commit into
Conversation
6853d6b to
fce76bb
Compare
There was a problem hiding this comment.
🟡 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 existingSet_complex_collection_to_null_mapped_to_jsontest covers a top-level JSON collection and does not enter this nestedinnerAbsentPropertyFixupMappath, 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":nulland 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
fce76bb to
5f3594c
Compare
There was a problem hiding this comment.
🟡 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
5f3594c to
f331e67
Compare
|
I had opened #39017 as a |
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 producednullfor that collection. Reading the collection then throwsNullReferenceException, and saving the tracked entity after any change throws fromPrepareToSave, 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
nullis still materialized asnull, soSet_complex_collection_to_null_mapped_to_jsonand 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 withnull, 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'sIClrCollectionAccessor(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 benull, 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 calledGetCurrentValue(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 misleadingPropertyDoesNotBelongexception seen in the issue instead ofNullRequiredComplexProperty. 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 tonulland 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 JSONnull, 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 existingWidgetWithDeepJsonfixture 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 explicitnullis preserved; only an absent key is coalesced)Save_changes_after_loading_row_with_complex_collection_absent_from_jsonSaving_null_required_complex_collection_in_complex_collection_element_throwsThe first, third and fourth fail on
mainwithout the fix (the first and third with the symptoms in the issue, the fourth with thePropertyDoesNotBelongmessage) 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.FunctionalTestsand the fullEFCore.Sqlite.FunctionalTestssuite are green with these changes. On SQL Server 2022 (Docker),ComplexCollectionJsonUpdateSqlServerTestpasses 24/24 and the JSON and complex-type query/update classes are green apart from the*JsonType*fixtures, which need the nativejsontype 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
mainis now 12.0, I'd like to ask for a backport torelease/11.0so 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 forrelease/10.0, with a quirk switch (Microsoft.EntityFrameworkCore.Issue38625) and the tests adapted to that branch's conventions, is on my fork atbackport/38625-release-10.0and is green on SQLite and SQL Server there.