Finish the implementation of the SaveChanges support for JSON-mapped complex collections #36432

AndriySvyryd · 2025-07-24T21:13:42Z

Add a way to configure a complex collection property as optional

Part of #31252
Fixes #36427
Fixes #36428

Copilot

Pull Request Overview

This PR completes the implementation of SaveChanges support for JSON-mapped complex collections in Entity Framework Core. The changes remove previous restrictions that prevented complex collections from being nullable and fix various bugs related to change tracking and update operations for JSON complex collections.

Key changes include:

Removed restrictions preventing complex collections from being marked as nullable/optional
Fixed change tracking logic for complex collections to properly detect modifications
Updated test infrastructure to use proper tracking patterns and baseline expectations
Enhanced JSON update handling for complex properties in the relational update pipeline

Reviewed Changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/EFCore/Metadata/Internal/ComplexProperty.cs`	Removed restriction preventing complex collections from being nullable
`src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs`	Fixed IsModified logic for complex properties and improved change tracking loops
`src/EFCore/Relational/Update/ModificationCommand.cs`	Enhanced JSON update handling and refactored column modification generation
`test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs`	Removed obsolete validation test for optional complex collections
Multiple test files	Updated test implementations from abstract to concrete classes and fixed SQL baselines

Files not reviewed (1)

src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported

src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs

roji · 2025-07-25T07:51:07Z

src/EFCore.Relational/Update/ModificationCommand.cs

-            || _entries is [var singleEntry]
-            && (singleEntry.SharedIdentityEntry is not null
-                || singleEntry.EntityType.GetComplexProperties().Any()
-                || singleEntry.EntityType.GetNavigations().Any(e => e.IsCollection && e.TargetEntityType.IsMappedToJson())))


Am not aware of the context enough, but so the code below used to run for all complex properties and all JSON-mapped collection navigations - and now it only runs for table-splitting complex properties only? Just want to make sure we don't need to do this for owned table splitting too.

In any case, maybe consider adding some documentation saying what this does and why it's relevant only to the complex table-splitting case?

Before JSON this block was used to detect table-splitting, with owned JSON support it was also used to detect JSON, but looking at the logic the two paths were disjointed, so I separated them for clarity. There shouldn't be any behavioral change from this.

src/EFCore.Relational/Update/ModificationCommand.cs

roji · 2025-07-25T08:01:19Z

src/EFCore.Relational/Update/ModificationCommand.cs

        }

-        if (jsonEntry)
+        if (_entries.Any(e => e.EntityType is IEntityType entityType


As a bit of further cleanup, instead of calling HandleJson() (which loops over all entries and deals with JSON ones) and then loop again just below over entries not mapped to JSON, we'd ideally just have one loop over all entries, that dispatches to different entry handlers (JSON, non-JSON, sproc...) as needed?

Also, in a similar vein, HandleJson() has a part where it goes over non-JSON-mapped entity entries to find the JSON mapped navigations and complex properties within, to add them to jsonColumnsUpdateMap. That also seems like it should just be part of the main loop, where we handle other aspects of non-JSON mapped entries.

(basically it seems like JSON handling was originally added here without much attempt to integrate it into the existing flow)

Obviously we don't have to continue cleanup/refactoring now, though, we can always improve later.

This would be somewhat involved, I'll do it as part of #36429 (comment)

src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs

roji · 2025-07-25T12:07:01Z

src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs

                _originalValues.AcceptChanges(this);

-                foreach (var complexCollection in _complexCollectionEntries)
+                for (var i = 0; i < _complexCollectionEntries.Length; i++)


Is this like an attempt to optimize? I think foreach over arrays is just as efficient as for, no?

No, this impacts behavior. These entries are mutable structs (I realize this is a smell), so when accessed with foreach the mutations only apply to the copy.

Oh I see...! No, then it's perfectly understandable (and I also think it's not necessarily a smell to have an array of structs here, to save on the needless allocations/references).

roji · 2025-07-25T12:09:14Z

src/EFCore/Metadata/Internal/ComplexProperty.cs

-
-        if (collection)
-        {
-            _isNullable = false;


Does that mean we allow nullable collections, e.g. to allow the null vs. empty distinction on JSON collections? We may need to check various other places (and add test coverage) to make sure this is actually fully supported.

I added test coverage in this PR. The query bugs around this are currently hidden by #36433

roji · 2025-07-25T12:22:18Z

test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs

        => (ComplexCollectionJsonContext)Fixture.CreateContext();

-    [ConditionalFact]
+    [ConditionalFact(Skip = "Issue #36433")]


I see you assigned it to me - is this a bug in query and change tracking? Let's chat so I understand it better.

Basically, it means that you need to make sure that all complex properties are populated before StartTrackingFromQuery is called on the containing entity.

Hmm, I thought that's exactly what #36430 did - is this based on top of it (recently merged to main)?

is this based on top of it (recently merged to main)?

Yes

…complex collections Part of #31252 Fixes #36427 Fixes #36428

AndriySvyryd requested review from Copilot and roji July 24, 2025 21:13

AndriySvyryd requested a review from a team as a code owner July 24, 2025 21:13

Copilot AI reviewed Jul 24, 2025

View reviewed changes

src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs Show resolved Hide resolved

src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs Show resolved Hide resolved

AndriySvyryd force-pushed the Issue31252_Update2 branch from 3a09405 to 3c85a3a Compare July 25, 2025 00:19

roji approved these changes Jul 25, 2025

View reviewed changes

Finish the implementation of the SaveChanges support for JSON-mappen …

4942acd

…complex collections Part of #31252 Fixes #36427 Fixes #36428

AndriySvyryd force-pushed the Issue31252_Update2 branch from 3c85a3a to 4942acd Compare July 25, 2025 18:14

AndriySvyryd enabled auto-merge (squash) July 25, 2025 18:14

AndriySvyryd merged commit c23462b into main Jul 25, 2025
7 checks passed

AndriySvyryd deleted the Issue31252_Update2 branch July 25, 2025 19:15

AndriySvyryd requested a review from artl93 July 25, 2025 19:49

Finish the implementation of the SaveChanges support for JSON-mapped complex collections #36432

Finish the implementation of the SaveChanges support for JSON-mapped complex collections #36432

Uh oh!

Conversation

AndriySvyryd commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AndriySvyryd commented Jul 24, 2025 •

edited

Loading

AndriySvyryd Jul 25, 2025 •

edited

Loading

AndriySvyryd Jul 25, 2025 •

edited

Loading