[API Proposal]: Expose TryRead() as public on JsonConverter

[rmja]

Background and motivation

The current JsonConverter has an interanal virtual OnTryRead() here.
For custom converter implementations that call e.g. reader.TrySkip() it would be neat to be able to actually return false if the conversion fails.
The suggestion is motivated by this SO question: https://stackoverflow.com/questions/63038334/how-do-i-handle-partial-json-in-a-jsonconverter-while-using-deserializeasync-on

API Proposal

The proposal is to change the visibilty on OnTryRead() (and maybe OnTryWrite()) to public instead of internal, but without the ReadStack parameter. Something like:

public class JsonConverter<T>
{
    public virtual bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, out T? value);
}

API Usage

void Run()
{
    Stream someHugeStream = Request.Body;  // This stream is so large that it causes `Utf8JsonReader.IsFinalBlock == false` when calling custom converters.

    var options = JsonSerializerOptions() { Converters = new MyCustomObjectConverter() };

    JsonSerializer.DeserializeAsync<List<MyCustomObject>>(someHugeStream, options);
}

public class MyCustomObjectConverter : JsonConverter<MyCustomObject>
{
    ...
    public override bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, out MyCumstomObject value)
    {
      // Handle case where `reader.IsFinalBlock == false`, for example by calling `reader.TrySkip()`
      ...
      if (reader.trySkip())
      {
          value = null;
          return false;
      }
      ...
      return true;
    }
}

Alternative Designs

No response

Risks

No response

[ghost]

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

The current JsonConverter has an interanal virtual OnTryRead() here.
For custom converter implementations that call e.g. reader.TrySkip() it would be neat to be able to actually return false if the conversion fails.
The suggestion is motivated by this SO question: https://stackoverflow.com/questions/63038334/how-do-i-handle-partial-json-in-a-jsonconverter-while-using-deserializeasync-on

API Proposal

The proposal is to change the visibilty on OnTryRead() (and maybe OnTryWrite()) to public instead of internal, but without the ReadStack parameter. Something like:

public class JsonConverter<T>
{
    public virtual bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, out T? value);
}

API Usage

void Run()
{
    Stream someHugeStream = Request.Body;  // This stream is so large that it causes `Utf8JsonReader.IsFinalBlock == false` when calling custom converters.

    var options = JsonSerializerOptions() { Converters = new MyCustomObjectConverter() };

    JsonSerializer.DeserializeAsync<List<MyCustomObject>>(someHugeStream, options);
}

public class MyCustomObjectConverter : JsonConverter<MyCustomObject>
{
    ...
    public override bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, out MyCumstomObject value)
    {
      // Handle case where `reader.IsFinalBlock == false`, for example by calling `reader.TrySkip()`
      ...
      if (reader.trySkip())
      {
          value = null;
          return false;
      }
      ...
      return true;
    }
}

Alternative Designs

No response

Risks

No response

Author:	rmja
Assignees:	-
Labels:	api-suggestion, area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

Related to #36785. Exposing some variation of the currently internal OnTryWrite and OnTryRead methods is something we are actively considering for .NET 7, to enable async serialization for custom converters.

Note however that it's not as simple as removing the ReadStack parameter, since some state will need to preserved for the converter to know how to resume execution once the buffer has been filled. A corollary of that is that resumable converters are very difficult to get right, since you're effectively handwriting your own async state machine. We'll be investigating potential prototypes for making the public APIs slightly more user friendly.

[steveharter]

Note that OnTryRead() and OnTryWrite() don't return false for failed conversions. Instead, for the Stream-only APIs, false is returned for OnTryRead when the current buffer is "out of data" and needs to be updated from the underlying Stream via ReadAsync and for OnTryWrite when the current buffer needs to be flushed to the underlying Stream via FlushAsync().

A custom converter doesn't have to worry about the "out of data" scenario since there is a "read ahead" feature for custom converters, and for the write case the buffer is flushed, if necessary, after the converter returns from Write(). This is different from the internal converters which have a finer granularity of per-property and per-element ReadAsync\WriteAsync support.

[ghost]

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

[eiriktsarpalis]

Per the request in #29902, we should consider offering similar methods on the JsonSerializer level. cc @davidfowl

[eiriktsarpalis]

Closing in favor of #63795.