JsonElement and JsonDocument should be deserializable from JSON payloads

[terrajobst]

This works for JsonElement today:

var element = JsonSerializer.Deserialize<JsonElement>("{\"data\": 12}");
Console.WriteLine(element);

but fails for JsonDocument:

var document = JsonSerializer.Deserialize<JsonDocument>("{\"data\": 12}");
Console.WriteLine(document);

System.NotSupportedException: 'Deserialization of reference types without parameterless constructor is not supported. Type 'System.Text.Json.JsonDocument''

Equally, Serialize should also work for both, but only works for JsonElement.

/cc @steveharter

[terrajobst]

@ahsonkhan asked why we would make it work for both. My argument is that JsonDocument also controls the lifetime, so it seems that should work to allow users to return the array to the pool.

[ahsonkhan]

As a workaround, you could create your own converter and register it with the options, based on the JsonElement converter.
https://github.com/dotnet/corefx/blob/9b3271de320bce639379720bc3e913286ba8d481/src/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterJsonElement.cs#L7-L21

internal sealed class JsonConverterJsonDocument : JsonConverter<JsonDocument>
{
    public override JsonDocument Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        return JsonDocument.ParseValue(ref reader);
    }

    public override void Write(Utf8JsonWriter writer, JsonDocument value, JsonSerializerOptions options)
    {
        value.WriteTo(writer);
    }
}

var options = new JsonSerializerOptions();
options.Converters.Add(new JsonConverterJsonDocument());
using (var document = JsonSerializer.Deserialize<JsonDocument>("{\"data\": 12}", options))
{
    // {"data": 12}
    Console.WriteLine(document.RootElement);
}

There is a question of lifetime semantics though, particularly who is responsible of disposing of the JsonDocument. In the case of JsonElement, we clone and detach from the document, so the user doesn't have any concerns with disposing. That isn't true for JsonDocument.

cc @bartonjs

[terrajobst]

That isn't true for JsonDocument.

Well, it's sort of true for JsonDocument too, but we can decide that the control is entirely on the user's side, which I think makes sense. In that vein, maybe we should only allow JsonDocument?

[ahsonkhan]

Well, it's sort of true for JsonDocument too, but we can decide that the control is entirely on the user's side, which I think makes sense.

Agreed. That makes sense to me too.

In that vein, maybe we should only allow JsonDocument?

I don't think restricting it to just JsonDocument is necessary. One of the reasons we support JsonElement is for deserializing JSON to the System.Object properties (including things like Dictionary<string, object>). This defers deserializing to a concrete .NET type to when the caller needs it and is in a context where the type required is known (and avoids us having to do type inference).

[bartonjs]

Deserializing to JsonDocument is weird, it's just a different entrypoint to JsonDocument.Parse (or JsonDocument.ParseValue); but there's nothing fundamentally wrong with it, I guess. Since you asked for it as a whole document it makes sense that it could be pool backed and you are responsible for disposing it. (It holds true for all Disposable objects, really... you caused it to be born by deserializing it, you own the lifetime)

Serializing a JsonDocument is likewise a bit weird, but has easy semantics, since it's just Serialize(doc.RootElement)

[terrajobst]

Sounds like we agree to allow JsonDocument to be serialized. Are we OK with serializing to JsonElement even though that means burning a rented array?

[ahsonkhan]

Are we OK we serializing to JsonElement even though that means burning a rented array?

What do you mean by burning a rented array?

If the concern is about using up the array pool and not returning things back, we are cloning the element before returning it (which uses a GC-tracked/regular array) and we are disposing the original document. This returns the original array back to the pool.

[terrajobst]

Got it. In since we do have to support JsonElement as part of the POCO fallback for object, that seems reasonable. So we should just add support for JsonDocument.

[marcusturewicz]

I'd like to take this one. Any guidance or things to be aware of?