-
Notifications
You must be signed in to change notification settings - Fork 5k
JSON: Don't mark unmapped properties as read #116807
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
JSON: Don't mark unmapped properties as read #116807
Conversation
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 fixes an issue where unmapped JSON properties were incorrectly being marked as "read" during deserialization, which affected duplicate detection and required property validation. Key changes include:
- Updating tests to include the new behavior with a class having a required property.
- Modifying the converter logic to skip marking unmapped properties as read.
- Adding corresponding test cases to verify the fix.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs | Added JsonSerializable attribute for ClassWithRequiredProperty in both metadata and default contexts. |
ConstructorTests.ParameterMatching.cs | Added a new test case to ensure unmapped properties do not interfere with duplicate detection. |
ObjectWithParameterizedConstructorConverter.cs | Adjusted condition and comment to prevent marking unmapped properties as read. |
Comments suppressed due to low confidence (2)
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs:632
- [nitpick] Consider enhancing the comment to reference issue
JsonSerializer.Deserialize()
may throwNullReferenceException
for unknown properties #116801 for additional context on why unmapped properties are skipped during duplicate detection.
if (!useExtensionProperty && jsonPropertyInfo != JsonPropertyInfo.s_missingProperty)
src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs:1825
- [nitpick] Consider extending the 'RequiredMemberWithUnmappedMember' test to explicitly validate that duplicate detection is bypassed for unmapped properties, if such verification is feasible.
[Fact]
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
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.
Thanks for the fast fix @PranavSenthilnathan
Thanks for fixing this @PranavSenthilnathan! Do you plan on backporting this to preview 6? We need it in order to be able to get the new aspnetcore Passkey feature working again |
/backport to release/10.0-preview6 |
Started backporting to release/10.0-preview6: https://github.com/dotnet/runtime/actions/runs/15763591698 |
During deserialization we keep track of the properties that are read in order to do duplicate detection and required property validation. When a parametrized constructor is specified for deserialization, there are four types of JSON properties:
(The last two cases are only differentiated by whether the class has a property with the ExtensionData attribute.)
#115856 added duplicate detection by keeping track of properties that were assigned. Paths to handle cases 1, 2 and 3 were added but case 4 was an edge case that was not considered. So when this case was hit we would try to find the mapping like we do for case 1 or 2 when we actually don't have one. This PR fixes this case by explicitly checking for it and skipping duplicate detection for it.
Fixes #116801