-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[release/10.0] Fix optional complex property default values tracking #37664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/10.0
Are you sure you want to change the base?
Conversation
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Extract ShouldDetectComplexPropertyChange helper to avoid duplication and improve readability. This addresses code review feedback to make the conditional logic more maintainable. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Per review feedback from @AndriySvyryd, inline both calls to ShouldDetectComplexPropertyChange helper method. The second call in LocalDetectChanges had a simpler condition (setModified: true hardcoded), so removing the helper makes the code clearer and more maintainable. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
AndriySvyryd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Update progress by rewriting the PR summary using this template:
Fixes #{number}
**Description**
{Issue description}
**Customer impact**
{How does the reported issue affects customer's app? Are there workarounds?}
**How found**
{Was it customer reported or found during verification? How many customers are affected?}
**Regression**
{Is it a regression from a released version? Which one?}
**Testing**
{How the changes were tested}
**Risk**
{Low/Medium/High, argument why}
Per review feedback: simplified TODO comment to be more concise and moved the type pattern check (entry is InternalEntryBase entryBase) earlier in the condition chain for better readability. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Updated PR description using the servicing PR template. The description now includes all required sections: Description, Customer impact, How found, Regression, Testing, and Risk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR ports the fix from PR #37387 and adds a quirk mode switch Microsoft.EntityFrameworkCore.Issue37387 to allow users to opt out of the behavior change. The fix addresses issue #37386 where default values in optional complex properties were not being tracked or saved when the complex property transitioned from null to non-null.
Changes:
- Added quirk mode switch to allow reverting to old behavior if needed
- Enhanced change detection to mark all inner properties as modified when optional complex properties transition from null to non-null
- Added comprehensive test coverage with provider-specific skips for known limitations
- Updated test infrastructure to support 4 nested test operations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/EFCore/ChangeTracking/Internal/ChangeDetector.cs | Added quirk mode switch Issue37387 and implemented fix to detect and mark complex property changes from null to non-null, ensuring default values are persisted |
| test/EFCore.Specification.Tests/ComplexTypesTrackingTestBase.cs | Added test Can_save_default_values_in_optional_complex_property_with_multiple_properties with 3 nested operations, added test entities, and updated test infrastructure signature to support 4 nested operations |
| test/EFCore.InMemory.FunctionalTests/ComplexTypesTrackingInMemoryTest.cs | Updated test infrastructure signature and skipped new test due to InMemory provider limitation (issue #31464) |
| test/EFCore.SqlServer.FunctionalTests/ComplexTypesTrackingSqlServerTest.cs | Skipped new test for proxy configuration due to notification change tracking limitation (issue #36175) |
Fixes #37386
Port of #37387
Description
When optional complex properties transition from
nullto non-null with default-valued properties (e.g.,entity.ComplexProp = new() { IntValue = 0 }), those default values were not being tracked or persisted to the database.Customer impact
Applications using optional complex properties would lose data when setting a complex property from null to a new instance with default values. The properties with default values (like 0, false, default(DateTimeOffset)) would not be saved to the database, resulting in NULL values in the database columns instead of the expected default values.
Workaround: Users could explicitly mark the entity or properties as modified after setting the complex property, or set non-default values temporarily.
How found
2 customer reports on 10.0.x
Regression
No. Optional complex properties were introduced in EF 10.
Testing
Added a comprehensive test
Risk
Low - The fix is targeted and only affects optional nullable complex properties. The quirk mode switch provides an escape hatch for any unforeseen issues.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.