-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Draft
Copilot
wants to merge
4
commits into
main
Choose a base branch
from
copilot/fix-generic-converter-issue
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+256
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hods Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Copilot finished work on behalf of
eiriktsarpalis
October 16, 2025 16:12
@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>
Copilot finished work on behalf of
stephentoub
October 17, 2025 15:34
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixed an issue where primitive number converters obtained via
JsonSerializerOptions.GetConverter(Type)
did not honor theNumberHandling
setting 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.NumberHandling
setting. 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.NumberHandling
in their publicRead()
andWrite()
methods:NumberHandling
is notStrict
, delegate to the internalReadNumberWithCustomHandling()
/WriteNumberWithCustomHandling()
methodsChanges Made
Affected converters:
ByteConverter
,SByteConverter
,Int16Converter
,Int32Converter
,Int64Converter
,Int128Converter
,UInt16Converter
,UInt32Converter
,UInt64Converter
,UInt128Converter
HalfConverter
,SingleConverter
,DoubleConverter
,DecimalConverter
Tests
NumberHandlingTests.cs
to specifically verify the fix works withGetConverter()
GetConverter()
requires reflectionOriginal 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 withinJsonConverterFactory
etc. it would appear that theJsonConverter
created does not honour the underlyingJsonSerializerOptions.NumberHandling
preferences.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
NumberHandling
set asAllowReadingFromString
then the converter generated (in this caseInt32Converter
) to correctly handle string based numbers in the json payload.Actual behavior
And
InvalidOperationException
excpetion 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
JsonPrimitiveConverter
is not correctly persisting the state to the converter and therefore not invokingReadNumberWithCustomHandling
Regression?
Unsure
Known Workarounds
A possible workaround is to just use standard
JsonSerializer.Deserialize
techniques 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
Write
andRead
methods of the primitive converters should try to take number handling configuration present in theJsonSerializerOptions
into 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/ReadNumberWithCustomHandling
methods 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
/Read
methods 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.