-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fix nullable complex property with PropertyValues.ToObject() creating instance instead of null #37302
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
…f null When calling OriginalValues.ToObject() or CurrentValues.ToObject() on an entity with a nullable complex property that is null, the method was creating an instance of the complex type with default values instead of returning null. The fix adds tracking for null nullable complex properties in ArrayPropertyValues and sets them to null after materialization in ToObject(). Co-authored-by: AndriySvyryd <[email protected]>
…operty tracking Co-authored-by: AndriySvyryd <[email protected]>
| // Tracks nullable non-collection complex properties that should be null when materializing via ToObject(). | ||
| // This is needed because value type properties inside nullable complex types store default values (not null) | ||
| // when the complex property is null, making it impossible to detect nullness from the values array alone. | ||
| private HashSet<IComplexProperty>? _nullComplexProperties; |
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 Make this bool[], with values in the same order as nullable non-collection complex properties for the given type base
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.
Changed to bool[] with values matching NullableComplexProperties ordering. Added NullableComplexProperties list to PropertyValues base class. Commit d13bfc9.
| { | ||
| if (!complexProperty.IsCollection && complexProperty.IsNullable) | ||
| { | ||
| var complexValue = GetValueInternal(InternalEntry, complexProperty); |
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 GetValueInternal cannot be used for non-collection complex properties
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.
Fixed to use InternalEntry[nullableComplexProperties[i]] indexer instead of GetValueInternal. Commit d13bfc9.
| context.SaveChanges(); | ||
|
|
||
| context.ChangeTracker.Clear(); | ||
|
|
||
| // Reload the product | ||
| var loadedProduct = context.Set<ProductWithNullableComplexType>().Single(p => p.Name == "Test Product"); |
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 No need to do these actions, just use the newly added product. Also remove comments in this test
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.
Simplified test to use the newly added entity directly without SaveChanges/reload, and removed all comments. Commit d13bfc9.
| [ComplexType] | ||
| protected class ProductPrice | ||
| { | ||
| public decimal Amount { get; init; } | ||
| public int CurrencyId { get; init; } | ||
| } |
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 Remove, use an existing class instead
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.
Removed ProductWithNullableComplexType and ProductPrice. Added OptionalMilk property to the existing Building class instead. Commit d13bfc9.
…ndexer - Changed _nullComplexProperties from HashSet to bool[] with indices matching NullableComplexProperties ordering - Added NullableComplexProperties property to PropertyValues base class - Fixed EntryPropertyValues.Clone() to use InternalEntry[] indexer instead of GetValueInternal for complex properties - Simplified test to use newly added product without reload, removed comments - Removed ProductWithNullableComplexType, added OptionalMilk to Building class Co-authored-by: AndriySvyryd <[email protected]>
Description
When calling
OriginalValues.ToObject()orCurrentValues.ToObject()on an entity with a nullable complex property that is null, the returned object incorrectly contains an instantiated complex type with default values instead of null.Root cause: When cloning property values, value type properties inside nullable complex types (e.g.,
decimal Amount) store default values rather than null when the complex property is null. The materializer cannot distinguish between "complex property is null" and "complex property has default values."Changes
PropertyValues: AddedNullableComplexPropertieslist to track nullable non-collection complex properties for the structural type.ArrayPropertyValues: Added_nullComplexPropertyFlagsas abool[]with indices matchingNullableComplexPropertiesordering.ToObject()now sets tracked properties to null after materialization.Clone()copies the flags array.EntryPropertyValues:Clone()now uses the entry indexer to check nullable non-collection complex properties and sets the flags in the cloned instance when null.OptionalMilkproperty to existingBuildingclass for testing nullable complex properties.Nullable_complex_property_with_null_value_returns_null_when_using_ToObject()Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.