Fix persisting null optional complex property with default values#37944
Fix persisting null optional complex property with default values#37944AndriySvyryd merged 5 commits intodotnet:mainfrom
Conversation
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes change tracking for optional complex properties when transitioning from non-null to null while inner value-type properties are at their CLR defaults, ensuring the database columns are updated to NULL as expected.
Changes:
- Update
ChangeDetector.DetectComplexPropertyChangeto mark all flattened inner properties as modified on both null→non-null and non-null→null transitions. - Add a new spec test covering non-null→null transition for an optional complex property with multiple default-valued inner properties.
- Add/adjust provider-specific overrides to skip/route the new test appropriately (SqlServer proxies, InMemory limitation, Cosmos async-only).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/ChangeTracking/Internal/ChangeDetector.cs | Marks inner complex properties as modified when nullable complex reference toggles null/non-null, ensuring persistence of nulling out columns. |
| test/EFCore.Specification.Tests/ComplexTypesTrackingTestBase.cs | Adds coverage for setting an optional multi-property complex to null after saving default-valued inner properties. |
| test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs | Skips the new test for proxies scenario due to notification change tracking limitation (#36175). |
| test/EFCore.InMemory.FunctionalTests/ComplexTypesTrackingInMemoryTest.cs | Skips the new test due to existing InMemory complex-type query/materialization limitations (#31464). |
| test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs | Routes the new test through the async-only Cosmos path (skip sync). |
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes change tracking for optional (nullable) complex properties when transitioning from non-null to null, ensuring that inner properties with default values are treated as modified so database columns are correctly set to NULL.
Changes:
- Update
ChangeDetector.DetectComplexPropertyChangeto mark all flattened inner properties as modified when a nullable complex property changes nullness in either direction (null ↔ non-null). - Add a new specification test covering non-null → null transitions for optional complex properties with default-valued inner properties.
- Skip/route the new test appropriately for providers/scenarios where the underlying capability isn’t supported (InMemory, SQL Server change-tracking proxies, Cosmos sync).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/ChangeTracking/Internal/ChangeDetector.cs | Marks inner complex properties as modified when a nullable complex property changes nullness (handles non-null → null scenario). |
| test/EFCore.Specification.Tests/ComplexTypesTrackingTestBase.cs | Adds coverage for nulling an optional complex property whose inner properties are default values. |
| test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs | Skips the new test for SQL Server change-tracking proxy scenario (known limitation). |
| test/EFCore.InMemory.FunctionalTests/ComplexTypesTrackingInMemoryTest.cs | Skips the new test for InMemory provider due to existing complex-type limitations. |
| test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs | Routes the new test through async path only (Cosmos doesn’t support sync operations). |
You can also share your feedback on Copilot code review. Take the survey.
test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs
Outdated
Show resolved
Hide resolved
Cosmos doesn't support rolling back, to ensure tests don't interfere with each other we need to clean the database Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes change tracking for optional complex properties when transitioning from non-null → null, ensuring inner properties with CLR default values are still treated as modified so database columns get set to NULL. This complements the earlier null → non-null fix in #37387.
Changes:
- Update
ChangeDetector.DetectComplexPropertyChangeto mark inner properties modified for both nullability transitions (null ↔ non-null). - Add a new specification test covering non-null → null for an optional multi-property complex type with default-valued inner properties.
- Skip or adapt the new test for providers/fixtures with known limitations (InMemory, SQL Server proxies, Cosmos sync), and add Cosmos cleanup logic.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/EFCore/ChangeTracking/Internal/ChangeDetector.cs |
Extends complex-property null transition detection to mark inner properties modified for non-null → null as well. |
test/EFCore.Specification.Tests/ComplexTypesTrackingTestBase.cs |
Adds a new cross-provider test validating persistence when optional complex is set to null. |
test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs |
Skips the new test for the proxies fixture due to notification change tracking limitations. |
test/EFCore.InMemory.FunctionalTests/ComplexTypesTrackingInMemoryTest.cs |
Skips the new test due to known InMemory complex-type limitations. |
test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs |
Runs the new test async-only and attempts cleanup after execution strategy completion. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Fixes EF Core change tracking for optional complex properties when transitioning from non-null to null, ensuring inner properties with CLR default values are treated as modified and persisted as NULL in the database (issue #37890), complementing #37387.
Changes:
- Update complex-property change detection to mark inner properties as modified for both null→non-null and non-null→null transitions.
- Add a new specification test covering non-null→null transitions with default-valued inner properties.
- Add/adjust provider-specific overrides (skip for InMemory/proxy SqlServer; async-only for Cosmos) and ensure Cosmos test cleanup runs in a
finally.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/ChangeTracking/Internal/ChangeDetector.cs | Marks complex type inner properties as modified when an optional complex property toggles null/non-null. |
| test/EFCore.Specification.Tests/ComplexTypesTrackingTestBase.cs | Adds regression test for non-null→null with default-valued inner properties. |
| test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs | Skips the new test for the proxy-based fixture (notification tracking limitation). |
| test/EFCore.InMemory.FunctionalTests/ComplexTypesTrackingInMemoryTest.cs | Skips the new test due to existing InMemory complex-type limitations. |
| test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs | Runs new test only for async; guarantees DB cleanup via try/finally. |
You can also share your feedback on Copilot code review. Take the survey.
|
Thanks for your contribution! |
…property with default values Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
When optional complex properties transition from non-null to null with default-valued properties, those default values were not being tracked and therefore not set to null in the database.
This PR builds on top of the fix #37387 that fixes the opposite scenario.
Fixes #37890