dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

Developers should be able to pass state to custom converters. #63795

Open eiriktsarpalis opened 2 years ago

eiriktsarpalis commented 2 years ago

Background and Motivation

The current model for authoring custom converters in System.Text.Json is general-purpose and powerful enough to address most serialization customization requirements. Where it falls short currently is in the ability to accept user-provided state scoped to the current serialization operation; this effectively blocks a few relatively common scenaria:

  1. Custom converters requiring dependency injection scoped to the current serialization. A lack of a reliable state passing mechanism can prompt users to rebuild the converter cache every time a serialization operation is performed.

  2. Custom converters do not currently support streaming serialization. Built-in converters avail of the internal "resumable converter" abstraction, a pattern which allows partial serialization and deserialization by marshalling the serialization state/stack into a state object that gets passed along converters. It lets converters suspend and resume serialization as soon as the need to flush the buffer or read more data arises. This pattern is implemented using the internal JsonConveter<T>.TryWrite and JsonConverter<T>.TryRead methods.

    Since resumable converters are an internal implementation detail, custom converters cannot support resumable serialization. This can create performance problems in both serialization and deserialization:

    • In async serialization, System.Text.Json will delay flushing the buffer until the custom converter (and any child converters) have completed writing.
      • In async deserialization, System.Text.Json will need to read ahead all JSON data at the current depth to ensure that the custom converter has access to all required data at the first read attempt.

    We should consider exposing a variant of that abstraction to advanced custom converter authors.

  3. Custom converters are not capable of passing internal serialization state, often resulting in functional bugs when custom converters are encountered in an object graph (cf. https://github.com/dotnet/runtime/issues/51715, #67403, https://github.com/dotnet/runtime/issues/77345)

Proposal

Here is a rough sketch of how this API could look like:

namespace System.Text.Json.Serialization;

public struct JsonWriteState
{
    public CancellationToken { get; }
    public Dictionary<string, object> UserState { get; }
}

public struct JsonReadState
{
    public CancellationToken { get; }
    public Dictionary<string, object> UserState { get; }
}

public abstract class JsonStatefulConverter<T> : JsonConverter<T>
{
     public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref JsonWriteState state);
     public abstract T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options, ref JsonReadState state);

      // Override the base methods: implemented in terms of the new virtuals and marked as sealed
      public sealed override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) {}
      public sealed override T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options) {}
}

public abstract class JsonResumableConverter<T> : JsonConverter<T>
{
     public abstract bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref JsonWriteState state);
     public abstract bool TryRead(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options, ref JsonReadState state, out T? result);

      // Override the base methods: implemented in terms of the new virtuals and marked as sealed
      public sealed override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) {}
      public sealed override T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options) {}
}

public partial class JsonSerializer
{
     // Overloads to existing methods accepting state
     public static string Serialize<T>(T value, JsonSerializerOptions options, ref JsonWriteState state);
     public static string Serialize<T>(T value, JsonTypeInfo<T> typeInfo, ref JsonWriteState state);
     public static T? Deserialize<T>(string json, JsonSerializerOptions options, ref JsonReadState state);
     public static T? Deserialize<T>(string json, JsonTypeInfo<T> typeInfo, ref JsonReadState state);

     // New method groups enabling low-level streaming serialization
     public static bool TrySerialize(T value, JsonTypeInfo<T> typeInfo, ref JsonWriteState state);
     public static bool TryDeserialize(string json, JsonTypeInfo<T> typeInfo, ref JsonReadState state);
}

Users should be able to author custom converters that can take full advantage of async serialization, and compose correctly with the contextual serialization state. This is particularly important in the case of library authors, who might want to extend async serialization support for custom sets of types. It could also be used to author top-level async serialization methods that target other data sources (e.g. using System.IO.Pipelines cf. https://github.com/dotnet/runtime/issues/29902)

Usage Examples

MyPoco value = new() { Value = "value" };
JsonWriteState state = new() { UserState = { ["sessionId"] = "myId" }};
JsonSerializer.Serialize(value, options, state); // { "sessionId" : "myId", "value" : "value" }

public class MyConverter : JsonStatefulConverter<MyPoco>
{
    public override void Write(Utf8JsonWriter writer, MyPoco value, JsonSerializerOptions options, ref JsonWriteState state)
    {
         writer.WriteStartObject();
         writer.WriteString("sessionId", (string)state.UserState["sessionId"]);
         writer.WriteString("value", value.Value);
     }
}

Alternative designs

We might want to consider the viability attaching the state values as properties on Utf8JsonWriter and Utf8JsonReader. It would avoid the need of introducing certain overloads, but on the flip side it could break scenaria where the writer/reader objects are being passed to nested serialization operations.

Goals

Progress

ghost commented 2 years ago

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

Issue Details
## Background and Motivation Async serialization in System.Text.Json is a powerful feature that lets users serialize and deserialize from streaming JSON data, without the need to load the entire payload in-memory. At the converter level, this is achieved with "resumable converters", a pattern that allows for partial serialization and deserialization by marshalling the serialization state/stack into a mutable struct that gets passed along converters. It lets converters suspend and resume serialization as soon as the need to flush the buffer or read more data arises. This pattern is implemented using the internal [`JsonConveter.TryWrite`](https://github.com/dotnet/runtime/blob/a13ee96335ea4c41cfe273855611caec4c35cae8/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs#L321) and [`JsonConverter.TryRead`](https://github.com/dotnet/runtime/blob/a13ee96335ea4c41cfe273855611caec4c35cae8/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs#L148) methods. Since resumable converters are an internal implementation detail, custom converters cannot support resumable serialization. This can create performance problems in both serialization and deserialization: * In async serialization, System.Text.Json will delay flushing the buffer until the custom converter (and any child converters) have completed writing. * In async deserialization, System.Text.Json will need to read ahead all JSON data at the current depth to ensure that the custom converter has access to all required data at the first read attempt. Because custom converters cannot pass the serialization state, System.Text.Json suffers from a class of functional bugs that arise because of the serialization state resetting every time a custom converter is encountered in the object graph (cf. https://github.com/dotnet/runtime/issues/51715). ## Proposal Users should be able to author custom converters that can take full advantage of async serialization, and compose correctly with the contextual serialization state. This is particularly important in the case of library authors, who might want to extend async serialization support for custom sets of types. It could also be used to author top-level async serialization methods that target other data sources (e.g. using System.IO.Pipelines cf. https://github.com/dotnet/runtime/issues/29902) This is a proposal to publicize aspects of the `TryWrite`/`TryRead` pattern to the users of System.Text.Json. It should be noted that this is a feature aimed exclusively towards "advanced" users, primarily third-party library and framework authors. ## Goals * Support custom resumable converters. * Support custom converters that are passing the serialization state to child converters. * Support async serialization using data sources other than `Stream` (à la https://github.com/dotnet/runtime/issues/29902). ## Progress - [ ] Author prototype - [ ] API proposal & review - [ ] Implementation & tests - [ ] Conceptual documentation & blog posts.
Author: eiriktsarpalis
Assignees: -
Labels: `area-System.Text.Json`, `User Story`, `Priority:2`, `Cost:M`, `Team:Libraries`
Milestone: 7.0.0
eiriktsarpalis commented 2 years ago

Tagging @steveharter who might be interested in this.

jeffhandley commented 2 years ago

I'm updating this feature's milestone to Future as it is not likely to make it into .NET 7.

eiriktsarpalis commented 2 years ago

I've updated the issue description and examples to broaden the scope to stateful converters. Sketch of API proposal and a basic example has been included. PTAL.

layomia commented 2 years ago

There are scenarios where a custom converter might want metadata info about the type (including it's properties) or property it is processing, e.g. https://github.com/dotnet/runtime/issues/35240#issuecomment-617453603. I understand that with JsonSerializerOptions.GetTypeInfo(Type), it is now possible to retrieve type metadata within converters, but should there be a first class mechanism, e.g. adding relevant type/property metadata to the state object passed to converters?

I believe I've also come across scenarios where a converter might want to know where in the object graph it is being invoked, i.e the root object vs property values. Is that also state that should be passed?

eiriktsarpalis commented 2 years ago

Yes that's plausible. Effectively we should investigate what parts of ReadStack/WriteStack could/should be exposed to the user. I'm not sure if we could meaningfully expose what amounts to WriteStack.Current, since that only gets updated consistently when the internal converters are being called. Prototyping is certainly required to validate feasibility.

thomaslevesque commented 2 years ago

To be honest, with the proposed design, this feature would only be moderately useful, because it requires passing state explicitly to the Serialize/Deserialize methods. This doesn't help when you don't have control over these callsites, as is the case in ASP.NET Core JSON input/output formatters.

I realize what I'm talking about isn't really "state", at least not callsite-specific state, but it's what was requested in #71718, which has been closed in favor this issue... I don't think the proposed design addresses the requirements of #71718.

eiriktsarpalis commented 2 years ago

I realize what I'm talking about isn't really "state", at least not callsite-specific state, but it's what was requested in https://github.com/dotnet/runtime/issues/71718, which has been closed in favor this issue... I don't think the proposed design addresses the requirements of https://github.com/dotnet/runtime/issues/71718.

Yes, this proposal specifically concerns state scoped to the operation rather than the options instance. The latter is achievable if you really need it whereas the former is outright impossible.

stevejgordon commented 2 years ago

@eiriktsarpalis I must admit that I'd not read this through and assumed it solved the suggestion from my original proposal. I agree that this solves a different problem (which I've not personally run into). It would not help at all with the scenario I have when building a library. Is there any reason not to re-open #71718 to complement this? I think both are valid scenarios that should be possible to achieve with less ceremony.

eiriktsarpalis commented 2 years ago

Agree, I've reopened the issue based on your feedback.

osexpert commented 1 year ago

"We might want to consider the viability attaching the state values as properties on Utf8JsonWriter and Utf8JsonReader" I see when Deserialize from Stream, an Utf8JsonReader instance is made for every iteration of a while-loop, so does not seem like a good fit.

dennis-yemelyanov commented 8 months ago

Was just looking for something like this. I'd like to be able to extract some value from the object during serialization and make it available after the serialization is done. It seems like currently the only way to achieve this is to be instantiating a new converter for each serialization, which is not great for perf.

Is this still planned to be implemented at some point?

brantburnett commented 8 months ago

Personally, I'm more interested in the performance benefits of the resuming bits of this proposal. While I realize they're somewhat related, I wonder if the lack of progress here could be partially related to the scope of including both resuming and user state in the same proposal. Should it be separated so the more valuable one (whichever that is) could be done independently, so long as the design has a path forward to the other?

eiriktsarpalis commented 8 months ago

Should it be separated so the more valuable one (whichever that is) could be done independently, so long as the design has a path forward to the other?

I think both are equally valuable. At the same time, we should be designing the serialization state types in a way that satisfy the requirements for both use cases.

andrewjsaid commented 5 months ago

What is the purpose of the JsonReadState and JsonWriteState parameters being passed in as ref? The dictionary is already mutable so it's probably not that...

eiriktsarpalis commented 5 months ago

The types already exist as internal implementation detail and hold additional state which wouldn't be visible to users.

JustDre commented 1 week ago

Given that #64182 was closed in deference to this issue, I don't know why the solution needs to involve async converters when DeserializeAsyncEnumerable does not and yet provides significant memory savings over DeserializeAsync (whenever streaming deserialization is appropriate). It seems the ability to call DeserializeAsyncEnumerable at any arbitrary point of an incoming stream, and leaving the stream open at the appropriate byte location upon exit, would be immensely useful. While the solutions discussed above are optimal from a resource perspective, forcing all developers to fully support async converters seems like a bigger lift than necessary, simply to support non-root array deserialization. If an overload can be implemented to achieve this, then once async converters become available, developers can opt in to implement them as appropriate.

JustDre commented 1 week ago

Maybe it's not important, but issue #77018 (that tracks this one) is currently closed. I don't see it being tracked by an equivalent issue for .NET 9, but since it's days (hours?) away from release, it probably needs to be tracked by the equivalent for .NET 10.