[release/10.0] Prevent DetectChanges failure for nulled nested complex collections - #38825
[release/10.0] Prevent DetectChanges failure for nulled nested complex collections#38825AndriySvyryd with Copilot wants to merge 3 commits into
Conversation
Co-authored-by: AndriySvyryd <[email protected]>
There was a problem hiding this comment.
Pull request overview
Fixes a change-tracking regression where setting an optional complex property (inside a JSON-mapped complex collection) to null could cause DetectChanges to throw when the complex property contained a nested collection with multiple entries, preventing the updated JSON graph from being persisted.
Changes:
- Updates complex-collection change tracking to reuse already-tracked nested entries during cleanup/reindexing, with a compatibility switch (
Microsoft.EntityFrameworkCore.Issue38632) to preserve prior behavior. - Adds a new relational specification test covering nulling an optional complex property that previously contained a nested collection.
- Adds provider-specific SQL baselines for SQL Server and SQLite for the new test scenario.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs | Adds SQL Server baseline for the new nulling scenario. |
| test/EFCore.Sqlite.FunctionalTests/Update/ComplexCollectionJsonUpdateSqliteTest.cs | Adds SQLite baseline for the new nulling scenario. |
| test/EFCore.Relational.Specification.Tests/Update/ComplexCollectionJsonUpdateTestBase.cs | Adds the new regression test and model/seed for an entity with an optional complex property containing a nested collection. |
| src/EFCore/ChangeTracking/Internal/InternalEntryBase.InternalComplexCollectionEntry.cs | Adjusts GetEntry behavior to prefer existing tracked entries (and introduces the Issue38632 compatibility switch). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: AndriySvyryd <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/EFCore.Cosmos.FunctionalTests/AddHocFullTextSearchCosmosTest.cs:236
- This test assertion update (Cosmos full-text-search) appears unrelated to the DetectChanges/complex-collection JSON fix described in the PR. Consider moving these Cosmos test changes to a separate PR/backport, or document why they are included here.
public async Task
Explicitly_setting_default_full_text_language_doesnt_clash_with_not_setting_it_on_other_entity_for_the_same_container()
{
var exception =
(await Assert.ThrowsAsync<CosmosException>(()
=> InitializeAsync<ContextDefaultFullTextSearchLanguageNoMismatchWhenNotSpecified>()));
Assert.Equal(HttpStatusCode.BadRequest, exception.StatusCode);
}
test/EFCore.Cosmos.FunctionalTests/AddHocFullTextSearchCosmosTest.cs:308
- This Cosmos test change is outside the PR's stated scope (JSON complex collection change detection). If it's required for the release/10.0 backport, please document the rationale; otherwise split it out to avoid bundling unrelated changes.
[ConditionalFact]
public async Task Default_full_text_language_is_used_for_full_text_properties_if_they_dont_specify_language_themselves()
{
var exception = (await Assert.ThrowsAsync<CosmosException>(()
=> InitializeAsync<ContextDefaultFullTextSearchLanguageUsedWhenPropertyDoesntSpecifyOneExplicitly>()));
Assert.Equal(HttpStatusCode.BadRequest, exception.StatusCode);
}
test/EFCore.Cosmos.FunctionalTests/AddHocFullTextSearchCosmosTest.cs:347
- This Cosmos full-text-search assertion update is not mentioned in the PR description and seems unrelated to the complex JSON collection DetectChanges fix. Please either justify it in the PR description or move it to a dedicated PR/backport.
[ConditionalFact]
public async Task Explicitly_setting_full_text_language_overrides_default()
{
var exception =
(await Assert.ThrowsAsync<CosmosException>(() => InitializeAsync<ContextExplicitFullTextLanguageOverridesTheDefault>()));
Assert.Equal(HttpStatusCode.BadRequest, exception.StatusCode);
}
test/EFCore.Cosmos.FunctionalTests/AddHocFullTextSearchCosmosTest.cs:150
- This PR is scoped to JSON complex collection change-detection, but this hunk changes Cosmos full-text-search tests (switching from message-based assertions to StatusCode). If this change is intentional for the backport, please call it out in the PR description (or link the relevant issue/PR); otherwise it should be moved to a separate PR/backport to keep release fixes focused.
This issue also appears in the following locations of the same file:
- line 228
- line 301
- line 340
public async Task Set_unsupported_full_text_search_default_language()
{
var exception = (await Assert.ThrowsAsync<CosmosException>(() => InitializeAsync<ContextSettingDefaultFullTextSearchLanguage>()));
Assert.Equal(HttpStatusCode.BadRequest, exception.StatusCode);
}
artl93
left a comment
There was a problem hiding this comment.
Approved. New 10 feature. Customer reported.
Fixes #38632
Backports #38667
Description
When an optional complex object is removed from a JSON-mapped complex collection, change detection can still need its former nested collection entries to update their positions, but this results in an exception before saving.
Customer impact
Applications cannot save entities mapped to JSON after removing an optional complex collection whose nested collection has multiple items. The save fails before updating the database.
A workaround requires manually disabling detection and forcing a full JSON rewrite, which is not readily discoverable.
How found
At least 4 user reported on 10.0.x.
Regression
Not a regression from an earlier EF Core version: the affected complex collection JSON feature was introduced in EF Core 10.
Testing
Test added.
Risk
Low. Only the failing scenario is affected. Quirk added.