Skip to content

[API Proposal]: Add async parse overload for JsonNode #88248

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

Closed
DoctorKrolic opened this issue Jun 30, 2023 · 3 comments · Fixed by #90006
Closed

[API Proposal]: Add async parse overload for JsonNode #88248

DoctorKrolic opened this issue Jun 30, 2023 · 3 comments · Fixed by #90006
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@DoctorKrolic
Copy link
Contributor

Background and motivation

Both JsonSerializer and JsonDocument have async parse method overloads when deserializing from Stream. However, JsonNode and JsonElement do not. This is especially odd since JsonNode.Parse under the hood calls JsonElement.ParseElement which under the hood uses JsonDocument which already has necessary asyn overloads.

I won't write a paragraph about why async is preferred when using IO operations.

API Proposal

Basically copy of existing Stream-related signature, but with Async suffix in name, warpped return type into Task and additional optional CancellationToken parameter:

namespace System.Text.Json;

public class JsonNode
{
    public static Task<JsonNode?> ParseAsync(
        Stream utf8Json,
        JsonNodeOptions? nodeOptions = null,
        JsonDocumentOptions documentOptions = default,
        CancellationToken cancellationToken = default);
}

API Usage

Nothing revolutional really...

using System.IO;
using System.Text.Json.Nodes;

using var stream = File.OpenRead("myFile.json");
var node = await JsonNode.ParseAsync(stream);
...

Alternative Designs

No response

Risks

No response

@DoctorKrolic DoctorKrolic added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 30, 2023
@ghost ghost added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jun 30, 2023
@ghost
Copy link

ghost commented Jun 30, 2023

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

Issue Details

Background and motivation

Both JsonSerializer and JsonDocument have async parse method overloads when deserializing from Stream. However, JsonNode and JsonElement do not. This is especially odd since JsonNode.Parse under the hood calls JsonElement.ParseElement which under the hood uses JsonDocument which already has necessary asyn overloads.

I won't write a paragraph about why async is preferred when using IO operations.

API Proposal

Basically copy of existing Stream-related signature, but with Async suffix in name, warpped return type into Task and additional optional CancellationToken parameter:

namespace System.Text.Json;

public class JsonNode
{
    public static Task<JsonNode?> ParseAsync(
        Stream utf8Json,
        JsonNodeOptions? nodeOptions = null,
        JsonDocumentOptions documentOptions = default,
        CancellationToken cancellationToken = default);
}

API Usage

Nothing revolutional really...

using System.IO;
using System.Text.Json.Nodes;

using var stream = File.OpenRead("myFile.json");
var node = await JsonNode.ParseAsync(stream);
...

Alternative Designs

No response

Risks

No response

Author: DoctorKrolic
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 30, 2023

Seems reasonable, I think we could easily implement this method using existing internal methods. TL;DR we could just compose the existing JsonDocument.ParseAsync method with the internal JsonNodeConverter.Create method.

Slightly related, we could also consider adding async serialization support for the JsonDocument/Element/Node converters so that JsonSerializer.DeserializeAsync works as expected for these types.

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 30, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Jun 30, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Jun 30, 2023
@bartonjs
Copy link
Member

bartonjs commented Aug 3, 2023

Video

Looks good as proposed

namespace System.Text.Json;

public class JsonNode
{
    public static Task<JsonNode?> ParseAsync(
        Stream utf8Json,
        JsonNodeOptions? nodeOptions = null,
        JsonDocumentOptions documentOptions = default,
        CancellationToken cancellationToken = default);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2023
@DoctorKrolic DoctorKrolic changed the title [API Proposal]: Add async parse overloads for JsonNode and JsonElement [API Proposal]: Add async parse overloads for JsonNode Aug 4, 2023
@DoctorKrolic DoctorKrolic changed the title [API Proposal]: Add async parse overloads for JsonNode [API Proposal]: Add async parse overload for JsonNode Aug 4, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants