-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix primitive converters to honor JsonNumberHandling when obtained via GetConverter #120805
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
Conversation
…hods Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
|
@copilot, there are test failures: |
... Fixed in commit cafaf2f. The GetConverter tests now skip in source generation scenarios since GetConverter() requires reflection which isn't available in source-generated contexts. |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
eiriktsarpalis
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.
LGTM, although there is a distinct possibility that this change will regress the STJ microbenchmarks we have in dotnet/performance. We can re-evaluate if and when regression numbers start coming in.
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 primitive number converters obtained via JsonSerializerOptions.GetConverter(Type) did not honor the JsonNumberHandling setting when their public Read() and Write() methods were called directly. The fix adds checks in each converter to delegate to the internal custom handling methods when NumberHandling is not Strict.
Key changes:
- Updated all 14 primitive number converters (integers and floating-point types) to check
NumberHandlingbefore proceeding with default behavior - Fixed infinite recursion issues in
Int128Converter,UInt128Converter, andHalfConverterby replacing recursiveRead()calls with directReadCore()calls - Added 5 new tests to verify the fix works correctly with
GetConverter()
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| NumberHandlingTests.cs | Added 5 new tests verifying converters obtained via GetConverter respect NumberHandling settings |
| ByteConverter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| SByteConverter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| Int16Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| Int32Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| Int64Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| Int128Converter.cs | Added NumberHandling check and fixed infinite recursion in ReadNumberWithCustomHandling |
| UInt16Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| UInt32Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| UInt64Converter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| UInt128Converter.cs | Added NumberHandling check and fixed infinite recursion in ReadNumberWithCustomHandling |
| DecimalConverter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| SingleConverter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| DoubleConverter.cs | Added NumberHandling check to delegate to custom handling methods when needed |
| HalfConverter.cs | Added NumberHandling check and fixed infinite recursion in ReadNumberWithCustomHandling |
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
|
@eiriktsarpalis, next steps here? |
|
Let's go ahead and merge. Any noticeable regressions will no doubt get reported by the performance team. |
|
@copilot, there's a test failure in CI, but only in the .NET Framework build: It looks like the test relies on floating points value formatting and that's a bit different between the versions. |
Changed test value from 123.45f to 123.5f to use an exactly representable float value. This fixes test failure on .NET Framework where 123.45f is formatted as "123.449997" instead of "123.45". Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Fixed in commit (will be shown after push). Changed the test value from |
Head branch was pushed to by a user without write access
Fixed an issue where primitive number converters obtained via
JsonSerializerOptions.GetConverter(Type)did not honor theNumberHandlingsetting when their publicRead()andWrite()methods were called directly.Root Cause
When users called
GetConverter()to obtain a converter and then directly invoked itsRead()orWrite()methods, the converter would not respect theJsonSerializerOptions.NumberHandlingsetting. This was because the public methods were directly calling reader methods likeGetInt32()without checking the options.Solution
Updated all numeric primitive converters to check
JsonSerializerOptions.NumberHandlingin their publicRead()andWrite()methods:NumberHandlingis notStrict, delegate to the internalReadNumberWithCustomHandling()/WriteNumberWithCustomHandling()methodsChanges Made
Affected converters:
ByteConverter,SByteConverter,Int16Converter,Int32Converter,Int64Converter,Int128Converter,UInt16Converter,UInt32Converter,UInt64Converter,UInt128ConverterHalfConverter,SingleConverter,DoubleConverter,DecimalConverterFixed infinite recursion:
Int128Converter,UInt128Converter, andHalfConverterto callReadCore()directly instead of recursing back toRead()Fixed test precision issue:
123.45fto123.5fto use exactly representable floating-point values, ensuring consistent behavior across .NET Framework and .NET Core/.NETTesting
Added comprehensive test coverage for all 14 modified primitive types:
Read tests (AllowReadingFromString):
Write tests (WriteAsString):
Special value tests (AllowNamedFloatingPointLiterals):
Test Results:
Original prompt
This section details on the original issue you should resolve
<issue_title>System.Text.Json - Generic converter type creation does not honour JsonNumberHandling preferences</issue_title>
<issue_description>### Description
When using the
JsonSerializerOptions.GetConverter(Type)method to generate generic converter types withinJsonConverterFactoryetc. it would appear that theJsonConvertercreated does not honour the underlyingJsonSerializerOptions.NumberHandlingpreferences.This can be most problematic when trying to have a factory for types such as generic collections where you might want to handle numbers that come from both int or string types.
Instead what happens is that
InvalidOperationException - Cannot get the value of a token type 'String' as a number.gets thrown and you need to handle this exception with fallback behaviourReproduction Steps
Expected behavior
I would expect because the options being used to generate the converter has the
NumberHandlingset asAllowReadingFromStringthen the converter generated (in this caseInt32Converter) to correctly handle string based numbers in the json payload.Actual behavior
And
InvalidOperationExceptionexcpetion is thrown with the error messageCannot get the value of a token type 'String' as a number.gets thrown and you need to handle this exception with fallback behaviour.Looking into the logic for the converter here it would appear that the base
JsonPrimitiveConverteris not correctly persisting the state to the converter and therefore not invokingReadNumberWithCustomHandlingRegression?
Unsure
Known Workarounds
A possible workaround is to just use standard
JsonSerializer.Deserializetechniques however this is not as performant given I have a locally persisted converter as documented here in the documentation for Factory patternsConfiguration
NET8 using System.Text.Json v9.0.10
Other information
No response</issue_description>
<agent_instructions>The
WriteandReadmethods of the primitive converters should try to take number handling configuration present in theJsonSerializerOptionsinto account.</agent_instructions>Comments on the Issue (you are @copilot in this section)
@eiriktsarpalis > Looking into the logic for the [converter here](https://github.com/dotnet/runtime/blob/e6fbfa2be5d9f333474fc9f1d71dc2762b26f0af/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/Int32Converter.cs#L39) it would appear that the base `JsonPrimitiveConverter` is not correctly persisting the state to the converter and therefore not invoking `ReadNumberWithCustomHandling`The original designers of STJ went intentionally with this approach which is why the
Write/ReadNumberWithCustomHandlingmethods exist.I suspect the motivation was related to performance when number handling isn't turned on, but it's conceivable we could try to make this behave more uniformly provided the performance regressions aren't significant.EDIT: Actually, it turns out that the internal methods are essential since the public
Write/Readmethods aren't passing the necessary context needed to determine the number handling of the present property.</comment_new>Fixes #120798
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.