Bugfix/113926 fix invalid jsonnode deserialization #114863
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
|
|
||
| namespace System.Text.Json.Nodes | ||
| namespace System.Text.Json.Nodes.Generics |
There was a problem hiding this comment.
Please avoid including large refactor without first discussing with the owners. It can have many unintentional effects.
|
|
||
| namespace System.Text.Json.Serialization.Converters.Node.Generics | ||
| { | ||
| internal sealed class JsonValuePrimitiveBooleanConverter : JsonValuePrimitiveConverter<bool> |
There was a problem hiding this comment.
It's not efficient to create dedicated converter class for each generic instantiation.
Instead, create a converter factory for Generic<T>, and invokes the converter of T. Check how NullableConverter, FSharpOptionConverter and JsonCollectionConverter invokes JsonConverter<TElement>.
There was a problem hiding this comment.
I thought the performance of this approach was better
There was a problem hiding this comment.
While it may improve performance to inline one level of virtual call, it can be easily compensated by other stuff like the ConcurrentDictionary lookup. It's way not maintainable.
There was a problem hiding this comment.
I also noticed a behavioral discrepancy. If the caller specifies a converter for primitive type, it will be respected by the "container" converters (collections/nullable), but not JsonObject. Even if we decide to keep this behavior, it should be achieved in generic way, by acquiring corresponding JsonPrimitiveConverter<T>.
I'd suggest to hold off and let the area owner make a decision on this.
There was a problem hiding this comment.
You are right, and this code already removed
| @@ -1417,5 +1420,311 @@ internal bool TryGetGuidCore(out Guid value) | |||
| value = default; | |||
| return false; | |||
| } | |||
|
|
|||
| internal char GetChar() | |||
There was a problem hiding this comment.
These changes can be reverted once you are invoking the corresponding JsonConverter<Primitive>.
There was a problem hiding this comment.
Removed by force push 9047de2
| @@ -16,9 +19,12 @@ internal sealed class JsonNodeConverter : JsonConverter<JsonNode?> | |||
| { | |||
| private static JsonNodeConverter? s_nodeConverter; | |||
| private static JsonArrayConverter? s_arrayConverter; | |||
| private static ConcurrentDictionary<Type, JsonConverter>? s_jsonValuePrimitiveConverters; | |||
There was a problem hiding this comment.
Do not store converters like this. Use a converter factory to create converter for each JsonValuePrimitive<T>. Check NullableConverterFactory.
There was a problem hiding this comment.
Thanks. Fixed by force push 9047de2
Maximys
force-pushed
the
bugfix/113926-fix-invalid-jsonnode-deserialization
branch
from
cf9d968 to
9047de2
Compare
Current PR fix #113926 .
Please pay attention on next lines:
which was extracted from existed classes:
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/HalfConverter.cs
Line 43 in c0c5520
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/Int128Converter.cs
Line 41 in c0c5520
runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/UInt128Converter.cs
Line 41 in c0c5520
If I correctly understand, this lines is dead and represend result of copy-paste. If so, I can delete this condition, but may be I does not understand some corner case.