Skip to content
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

JsonElement.ToString() returns invalid json: "True"/"False" #107998

Closed
jelliott-MS opened this issue Sep 18, 2024 · 13 comments
Closed

JsonElement.ToString() returns invalid json: "True"/"False" #107998

jelliott-MS opened this issue Sep 18, 2024 · 13 comments

Comments

@jelliott-MS
Copy link

Description

The JsonElement.ToString() method returns Boolean.TrueString and Boolean.FalseString if the JsonValueKind is True/False.
https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.tostring?view=net-8.0#remarks

Boolean.TrueString and Boolean.FalseString evaluate to "True" and "False" respectively
https://learn.microsoft.com/en-us/dotnet/api/system.boolean.truestring?view=net-8.0#remarks

According to Json.org, the valid json values are "true" and "false"
https://www.json.org/json-en.html

Recommend to update this to return the correct "true"/"false" values.

Reproduction Steps

public class ThingValueConverter : JsonConverter<string>
{
    public override string Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        using var jsonDocument = JsonDocument.ParseValue(ref reader);
        return jsonDocument.RootElement.ToString();
    }

    public override void Write(Utf8JsonWriter writer, string value, JsonSerializerOptions options)
    {
        writer.WriteStringValue(value);
    }
}
internal class Thing
{
    [JsonConverter(typeof(ThingValueConverter))]
    public string Prop { get; set; }
}
[Fact]
public void Test()
{
    var json = @"{""Prop"":true}";
    var deserialized = System.Text.Json.JsonSerializer.Deserialize<Thing>(json);

    deserialized.Prop.Should().Be("true");
}

Expected behavior

The test should pass because the value should be "true"

Actual behavior

The test returns "True"

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 18, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

@gregsdennis
Copy link
Contributor

Duplicate of #42502 (yet the issue persists).

JsonElement.ToString() just spits out the original JSON text it was given, including the original whitespace that was there.

If you serialize the element, you get what you expect, but I think that's overkill.

@jelliott-MS
Copy link
Author

It does not spit the original Json text it was given, it returns bool.TrueString
https://github.com/dotnet/runtime/blob/f748f06b4f27db4a506408b0bd1d167faf0b2f14/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L1652C32-L1652C40

I'm honestly surprised this is still an issue 4 years after being pointed out. This is a super easy fix and it would comply with the json spec. I don't see any reason to keep the capital values; can you help explain the reason for not fixing?

"Its called out in the documentation" doesn't really explain why this bug has persisted.

@jelliott-MS
Copy link
Author

Another thing I noticed is the JsonElement.ToString() outputs correct json syntax for objects and arrays. This makes things even more confusing that the boolean outputs incorrect json.

@elgonzo
Copy link

elgonzo commented Sep 19, 2024

@jelliott-MS It's not just boolean values, JsonElement.ToString() doesn't output the correct json syntax for json string values and json null either.

@eiriktsarpalis
Copy link
Member

I find this behaviour to be unintuitive as well, but it seems to be very much by-design behavior. It would be a breaking change to alter now.

@jelliott-MS
Copy link
Author

@jelliott-MS It's not just boolean values, JsonElement.ToString() doesn't output the correct json syntax for json string values and json null either.

@elgonzo I'm aware of null, what does the toString() output for valuekind of string that isn't valid?

@jelliott-MS
Copy link
Author

@eiriktsarpalis Unfortunately I think you're correct about the breaking change. Thanks anyways, we have a workaround implemented

           using var jsonDocument = JsonDocument.ParseValue(ref reader);
            JsonElement jsonElement = jsonDocument.RootElement;
            return jsonElement.ValueKind switch
            {
                // Text.Json's ValueKind.Boolean.ToString() returns "True"/"False"
                JsonValueKind.True => "true",
                JsonValueKind.False => "false",
                JsonValueKind.Null or JsonValueKind.Undefined => null,
                _ => jsonElement.ToString(),
            };

@elgonzo
Copy link

elgonzo commented Sep 19, 2024

@elgonzo I'm aware of null, what does the toString() output for valuekind of string that isn't valid?

@jelliott-MS not including the surrounding double-quotes as required by the json syntax nor escaping back-slash and double-quote characters in the value as also required by the json syntax (https://dotnetfiddle.net/cR1O17).

If you don't like ToString() returning True for not being json syntax, should you not also not like for example ToString() returning Hello (or Fo"o\Bar as in my dotnetfiddle example), which isn't valid json syntax either...?

(As a side note, JsonElement.ToString() using the string representation True/False for boolean values is the exact same manner as the System.Boolean.ToString() method does, so there is consistency in JsonElement.ToString()'s behavior in this regard with .NET Boolean type https://dotnetfiddle.net/vNCpTH)

@eiriktsarpalis
Copy link
Member

we have a workaround implemented

How about using JsonElement.GetRawText()? Alternatively, if you want to apply your own formatting/indentation rules you can just pass it into JsonSerializer.Serialize.

@jelliott-MS
Copy link
Author

Are you suggesting GetRawText for the fallthrough or to use in place of the original ToString call? Will GetRawText return the correct json data for all types?

@eiriktsarpalis
Copy link
Member

GetRawText() should always return the underlying JSON used to construct the element, although like I said it might still preserve the original formatting.

@eiriktsarpalis
Copy link
Member

Closing as duplicate of #42502.

@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Sep 20, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Sep 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants