Skip to content

Revert back to JsonElement-backed JsonValue for deserialization #116774

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.

Sign up for GitHub

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

Closed
PranavSenthilnathan wants to merge 1 commit into from
Closed

Revert back to JsonElement-backed JsonValue for deserialization #116774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 5 additions & 30 deletions ...System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonValueConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,39 +27,14 @@ public override void Write(Utf8JsonWriter writer, JsonValue? value, JsonSerializ
return null;
}

switch (reader.TokenType)
{
case JsonTokenType.String:
case JsonTokenType.False:
case JsonTokenType.True:
case JsonTokenType.Number:
return ReadNonNullPrimitiveValue(ref reader, options.GetNodeOptions());
default:
JsonElement element = JsonElement.ParseValue(ref reader, options.AllowDuplicateProperties);
return JsonValue.CreateFromElement(ref element, options.GetNodeOptions());
}
JsonElement element = JsonElement.ParseValue(ref reader, options.AllowDuplicateProperties);
return JsonValue.CreateFromElement(ref element, options.GetNodeOptions());
}

internal static JsonValue ReadNonNullPrimitiveValue(ref Utf8JsonReader reader, JsonNodeOptions options)
internal static JsonValue? ReadNonNullPrimitiveValue(ref Utf8JsonReader reader, JsonNodeOptions options)
{
Debug.Assert(reader.TokenType is JsonTokenType.String or JsonTokenType.False or JsonTokenType.True or JsonTokenType.Number);

switch (reader.TokenType)
{
case JsonTokenType.String:
return JsonValue.Create(reader.GetString()!, options);
case JsonTokenType.False:
return JsonValue.Create(false, options);
case JsonTokenType.True:
return JsonValue.Create(true, options);
case JsonTokenType.Number:
// We can't infer CLR type for the number, so we parse it as a JsonElement.
JsonElement element = JsonElement.ParseValue(ref reader);
return JsonValue.CreateFromElement(ref element, options)!;
default:
Debug.Fail("Unexpected token type for primitive value.");
throw new JsonException();
}
JsonElement element = JsonElement.ParseValue(ref reader);
return JsonValue.CreateFromElement(ref element, options)!;
}

internal override JsonSchema? GetSchema(JsonNumberHandling _) => JsonSchema.CreateTrueSchema();
Expand Down
72 changes: 67 additions & 5 deletions src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonValueTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,24 +547,27 @@ private class Student

[Theory]
[MemberData(nameof(GetPrimitiveTypes))]
public static void PrimitiveTypes_ReturnExpectedTypeKind<T>(T value, JsonValueKind expectedKind)
public static void PrimitiveTypes_ReturnExpectedTypeKind<T>(WrappedT<T> wrapped, JsonValueKind expectedKind)
{
T value = wrapped.Value;
JsonNode node = JsonValue.Create(value);
Assert.Equal(expectedKind, node.GetValueKind());
}

[Theory]
[MemberData(nameof(GetPrimitiveTypes))]
public static void PrimitiveTypes_EqualThemselves<T>(T value, JsonValueKind _)
public static void PrimitiveTypes_EqualThemselves<T>(WrappedT<T> wrapped, JsonValueKind _)
{
T value = wrapped.Value;
JsonNode node = JsonValue.Create(value);
Assert.True(JsonNode.DeepEquals(node, node));
}

[Theory]
[MemberData(nameof(GetPrimitiveTypes))]
public static void PrimitiveTypes_EqualClonedValue<T>(T value, JsonValueKind _)
public static void PrimitiveTypes_EqualClonedValue<T>(WrappedT<T> wrapped, JsonValueKind _)
{
T value = wrapped.Value;
JsonNode node = JsonValue.Create(value);
JsonNode clone = node.DeepClone();

Expand All @@ -575,8 +578,9 @@ public static void PrimitiveTypes_EqualClonedValue<T>(T value, JsonValueKind _)

[Theory]
[MemberData(nameof(GetPrimitiveTypes))]
public static void PrimitiveTypes_EqualDeserializedValue<T>(T value, JsonValueKind _)
public static void PrimitiveTypes_EqualDeserializedValue<T>(WrappedT<T> wrapped, JsonValueKind _)
{
T value = wrapped.Value;
JsonNode node = JsonValue.Create(value);
JsonNode clone = JsonSerializer.Deserialize<JsonNode>(node.ToJsonString());

Expand All @@ -585,30 +589,81 @@ public static void PrimitiveTypes_EqualDeserializedValue<T>(T value, JsonValueKi
Assert.True(JsonNode.DeepEquals(clone, node));
}

private static readonly HashSet<Type> s_convertableTypes =
[
// True/False
typeof(bool), typeof(bool?),

// Number
typeof(byte), typeof(sbyte), typeof(short), typeof(ushort), typeof(int), typeof(uint),
typeof(long), typeof(ulong), typeof(float), typeof(double), typeof(decimal),

typeof(byte?), typeof(sbyte?), typeof(short?), typeof(ushort?), typeof(int?), typeof(uint?),
typeof(long?), typeof(ulong?), typeof(float?), typeof(double?), typeof(decimal?),

// String
typeof(char), typeof(char?),
typeof(string),
typeof(DateTimeOffset), typeof(DateTimeOffset?),
typeof(DateTime), typeof(DateTime?),
typeof(Guid), typeof(Guid?),
];

[Theory]
[MemberData(nameof(GetPrimitiveTypes))]
public static void PrimitiveTypes_Conversion<T>(WrappedT<T> wrapped, JsonValueKind _)
{
T value = wrapped.Value;
string json = JsonSerializer.Serialize(value);
bool canGetValue = s_convertableTypes.Contains(typeof(T));

JsonValue jsonValue = JsonSerializer.Deserialize<JsonValue>(json)!;
AssertExtensions.TrueExpression(jsonValue.TryGetValue(out T unused) == canGetValue);

JsonValue jsonNode = (JsonValue)JsonSerializer.Deserialize<JsonNode>(json)!;
AssertExtensions.TrueExpression(jsonNode.TryGetValue(out unused) == canGetValue);

// Ensure the eager evaluation code path also produces the same result
jsonNode = (JsonValue)JsonSerializer.Deserialize<JsonNode>(json, new JsonSerializerOptions { AllowDuplicateProperties = false })!;
AssertExtensions.TrueExpression(jsonNode.TryGetValue(out unused) == canGetValue);
}

public static IEnumerable<object[]> GetPrimitiveTypes()
{
yield return Wrap(false, JsonValueKind.False);
yield return Wrap(true, JsonValueKind.True);
yield return Wrap((bool?)false, JsonValueKind.False);
yield return Wrap((bool?)true, JsonValueKind.True);
yield return Wrap((byte)42, JsonValueKind.Number);
yield return Wrap((byte?)42, JsonValueKind.Number);
yield return Wrap((sbyte)42, JsonValueKind.Number);
yield return Wrap((sbyte?)42, JsonValueKind.Number);
yield return Wrap((short)42, JsonValueKind.Number);
yield return Wrap((short?)42, JsonValueKind.Number);
yield return Wrap((ushort)42, JsonValueKind.Number);
yield return Wrap((ushort?)42, JsonValueKind.Number);
yield return Wrap(42, JsonValueKind.Number);
yield return Wrap((int?)42, JsonValueKind.Number);
yield return Wrap((uint)42, JsonValueKind.Number);
yield return Wrap((uint?)42, JsonValueKind.Number);
yield return Wrap((long)42, JsonValueKind.Number);
yield return Wrap((long?)42, JsonValueKind.Number);
yield return Wrap((ulong)42, JsonValueKind.Number);
yield return Wrap((ulong?)42, JsonValueKind.Number);
yield return Wrap(42.0f, JsonValueKind.Number);
yield return Wrap((float?)42.0f, JsonValueKind.Number);
yield return Wrap(42.0, JsonValueKind.Number);
yield return Wrap((double?)42.0, JsonValueKind.Number);
yield return Wrap(42.0m, JsonValueKind.Number);
yield return Wrap((decimal?)42.0m, JsonValueKind.Number);
yield return Wrap('A', JsonValueKind.String);
yield return Wrap((char?)'A', JsonValueKind.String);
yield return Wrap("A", JsonValueKind.String);
yield return Wrap(new byte[] { 1, 2, 3 }, JsonValueKind.String);
yield return Wrap(new DateTimeOffset(2024, 06, 20, 10, 29, 0, TimeSpan.Zero), JsonValueKind.String);
yield return Wrap((DateTimeOffset?)new DateTimeOffset(2024, 06, 20, 10, 29, 0, TimeSpan.Zero), JsonValueKind.String);
yield return Wrap(new DateTime(2024, 06, 20, 10, 29, 0), JsonValueKind.String);
yield return Wrap((DateTime?)new DateTime(2024, 06, 20, 10, 29, 0), JsonValueKind.String);
yield return Wrap(Guid.Empty, JsonValueKind.String);
yield return Wrap((Guid?)Guid.Empty, JsonValueKind.String);
yield return Wrap(new Uri("http://example.com"), JsonValueKind.String);
Expand All @@ -624,7 +679,14 @@ public static IEnumerable<object[]> GetPrimitiveTypes()
yield return Wrap(new DateOnly(2024, 06, 20), JsonValueKind.String);
yield return Wrap(new TimeOnly(10, 29), JsonValueKind.String);
#endif
static object[] Wrap<T>(T value, JsonValueKind expectedKind) => [value, expectedKind];
static object[] Wrap<T>(T value, JsonValueKind expectedKind) => [new WrappedT<T> { Value = value }, expectedKind];
Copy link
Member Author

@PranavSenthilnathan PranavSenthilnathan Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XUnit was coercing T? to T for some types (like char? to char) so the tests weren't actually running for those cases. It doesn't do that when using the custom type.

}

public class WrappedT<T>
{
public T Value;

public override string ToString() => Value?.ToString();
}
}
}
Loading