Added guard to throw InvalidOperrationException. #95723
Conversation
Added guard to throw InvalidOperrationException if the argument typeToConvert and Converter are incompatible. The exception message is "Type 'NameOfType' is not compatible with converter 'ConverterName'." and "Type 'NameOfType' is not compatible with converter 'ConverterName'. NOTES: The test threw a NotSupportedException on the following line and could not reproduce this issue, so the JsonSerializerOptions instantiation was changed. OptionsTests.ConverterRead_VerifyInvalidTypeToConvertFails() lineno 1432 KeyValuePair<int, int> kvp = converter.Read(ref reader, typeToConvert, options); Fix dotnet#36605
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsAdded guard to throw InvalidOperrationException if the argument typeToConvert and Converter are incompatible. The exception message is "Type 'NameOfType' is not compatible with converter 'ConverterName'." and "Type 'NameOfType' is not compatible with converter 'ConverterName'. NOTES: Fix #36605
|
| @@ -151,6 +151,11 @@ internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, J | |||
|
|
|||
| internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, scoped ref ReadStack state, out T? value, out bool isPopulatedValue) | |||
| { | |||
| if (typeToConvert != typeof(T)) | |||
There was a problem hiding this comment.
Should the newly exposed CanConvert(Type typeToConvert) method be used here?
Does TryWrite method need similar guard?
| @@ -151,6 +151,11 @@ internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, J | |||
|
|
|||
| internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, scoped ref ReadStack state, out T? value, out bool isPopulatedValue) | |||
| { | |||
| if (typeToConvert != typeof(T)) | |||
| { | |||
| ThrowHelper.ThrowInvalidOperationException($"Type '{typeToConvert}' is not compatible with converter '{GetType()}'."); | |||
There was a problem hiding this comment.
The exception message needs to be localizable. Besides afaic you should add a throw method in the ThrowHelper class like others.
There was a problem hiding this comment.
If we were designing STJ from scratch today, we would most likely be leaving out the typeToConvert parameter: it's mostly redundant since all converters must be generic and is only really useful in very limited (and somewhat unsound) polymorphic serialization applications.
Most converter implementations flat out ignore the typeToConvert parameter, so I honestly don't see a lot of benefit in adding this check in what is the serializer's hot path method. As to what is causing the NRE's described in #36605 these are better captured in #50205 and adding this check wouldn't suffice.
Thank you for taking the time to provide this PR, but unfortunately this isn't a change we would be considering.
Added guard to throw InvalidOperrationException if the argument typeToConvert and Converter are incompatible. The exception message is "Type 'NameOfType' is not compatible with converter 'ConverterName'." and "Type 'NameOfType' is not compatible with converter 'ConverterName'.
NOTES:
The test threw a NotSupportedException on the following line and could not reproduce this issue, so the JsonSerializerOptions instantiation was changed. OptionsTests.ConverterRead_VerifyInvalidTypeToConvertFails() lineno 1432 KeyValuePair<int, int> kvp = converter.Read(ref reader, typeToConvert, options);
Fix #36605