Bugfix/113926 fix invalid jsonnode deserialization #121265
Conversation
…rom "JsonValue" to "JsonValue<T>".
… from "JsonValue" to "JsonValue<T>".
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
There was a problem hiding this comment.
This PR is repeating the mistakes from #114863. You can follow #114863 (review) literally, and the code should be simpler than final state of #114863. The PR was mostly working and you should start from it.
|
|
||
| namespace System.Text.Json.Extensions | ||
| { | ||
| internal static class Utf8JsonReaderExtensions |
There was a problem hiding this comment.
As mentioned in #114863 (comment), the read/write methods must not be directly called by JsonValue converters. They must resolve and call any registered JsonConverter<TPrimitive>.
| /// <param name="value">The underlying value of the new <see cref="JsonValue"/> instance.</param> | ||
| /// <param name="options">Options to control the behavior.</param> | ||
| /// <returns>The new instance of the <see cref="JsonValue"/> class that contains the specified value.</returns> | ||
| internal static JsonValue Create(DateOnly value, JsonNodeOptions? options = null) => new JsonValuePrimitive<DateOnly>(value, JsonMetadataServices.DateOnlyConverter, options); |
There was a problem hiding this comment.
It is incorrect to use any JsonMetadataServices.XXConverter either. The converter from JsonSerializationOptions should be used.
|
|
||
| namespace System.Text.Json.Serialization.Metadata | ||
| { | ||
| internal sealed class SimpleConverterTypeComparer : IEqualityComparer<Type> |
There was a problem hiding this comment.
What is this at all? Type can just be compared by reference when used in dictionary.
As I wrote in root message of current PR, I understand that current changes is more difficult than in PR #114863 , but it's impossible to make it simplier by adding |
Non-generic Anyway, this PR is not going to work. The primitive value converters should not be touched nor directly used. |
This PR fixes #113926 as recommended by @eiriktsarpalis here.
Really, I'm not sure, that current implementation is simpler than first one. Maybe I don't understand something about generics, but from my point of view it is really impossible to fix the current error in the simpler way proposed