-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New JsonValue derived class for JSON primitives #116798
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
New JsonValue derived class for JSON primitives #116798
Conversation
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 introduces a new JsonValueOfJsonPrimitive
class to optimize handling of JSON primitive values by storing raw UTF-8 bytes and escaping info, refactors reader and converter logic to use it, and adds comprehensive tests for primitive behaviors and conversions.
- Add
JsonValueOfJsonPrimitive
to store only needed data for primitives and preserve original escaping inWriteTo
. - Update
JsonValueConverter
,JsonDocument
, andUtf8JsonReader
to delegate GUID parsing to a newJsonReaderHelper.TryGetValue
helper. - Extend tests in
JsonValueTests.cs
to cover deserialization,TryGetValue
,WriteTo
, deep-cloning, and conversion for all primitive types using a newWrappedT<T>
wrapper.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonValueTests.cs | Added tests for primitive JSON values (string, bool, number), WriteTo , cloning, deep-equals, and conversion; introduced WrappedT<T> helper. |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonValueConverter.cs | Replaced token-type switch with instantiation of JsonValueOfJsonPrimitive for primitives. |
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs | Replaced inline GUID parsing logic with JsonReaderHelper.TryGetValue call. |
src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs | Added new overload TryGetValue(ReadOnlySpan<byte>, bool, out Guid) to centralize GUID parsing. |
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs | New internal sealed class implementing optimized storage and parsing for JSON primitives. |
src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs | Switched inline GUID parsing in TryGetValue to JsonReaderHelper.TryGetValue . |
src/libraries/System.Text.Json/src/System.Text.Json.csproj | Included JsonValueOfJsonPrimitive.cs in project file. |
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonValueTests.cs
Outdated
Show resolved
Hide resolved
@eiriktsarpalis I can also take this PR one step further by adding separate derived classes for JSON string and numbers, and introducing the JsonNumber struct like we discussed offline. |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
This reverts commit 30ca4ab.
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Show resolved
Hide resolved
Keeping this fix more targeted by matching the logic in JsonValueOfElement. |
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueOfJsonPrimitive.cs
Outdated
Show resolved
Hide resolved
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.
Thanks!
/backport to release/10.0-preview7 |
Started backporting to release/10.0-preview7: https://github.com/dotnet/runtime/actions/runs/16439281496 |
Adds a new derived class that JsonValueConverter can use when the value is a primitive. Previously
JsonElementOfValue
was being used and more recentlyJsonValue
. There are downsides with both. The JsonElement-backed class incurs overhead for theJsonDocument
it needs to create andJsonValue
does not allow common conversions inTryGet
. The new class in this PR solves both by only storing necessary information and allowing conversions. It also preserves original escaping inWriteTo
.Fixes #116730