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

[API Proposal]: Add ReadOnlySequence overload for WriteRawValue #68223

Closed
Tracked by #77020
BrennanConroy opened this issue Apr 19, 2022 · 7 comments · Fixed by #76444
Closed
Tracked by #77020

[API Proposal]: Add ReadOnlySequence overload for WriteRawValue #68223

BrennanConroy opened this issue Apr 19, 2022 · 7 comments · Fixed by #76444
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@BrennanConroy
Copy link
Member

BrennanConroy commented Apr 19, 2022

Background and motivation

When using the Utf8JsonWriter.WriteRawValue API, you can't pass multiple segments in to write a single value, you have to allocate a contiguous array and do a single call to WriteRawValue. In our use case we already have a ReadOnlySequence<byte> and want to write it as a raw json value.

But because the API only accepts string or span, we need to write the following:

if (result.RawSerializedData.IsSingleSegment)
{
    writer.WriteRawValue(result.RawSerializedData.First.Span, skipInputValidation: true);
}
else
{
    writer.WriteRawValue(result.RawSerializedData.ToArray(), skipInputValidation: true);
}

API Proposal

namespace System.Text.Json
{
    public sealed partial class Utf8JsonWriter
    {
+        public void WriteRawValue(ReadOnlySequence<byte> json, bool skipInputValidation = false);
+        public void WriteRawValue(ReadOnlySequence<char> json, bool skipInputValidation = false);
    }
}

API Usage

void MyFunc(Utf8JsonWriter writer, ReadOnlySequence<byte> sequence)
{
    writer.WriteRawValue(sequence);
}

Alternative Designs

No response

Risks

No response

@BrennanConroy BrennanConroy added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json labels Apr 19, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 19, 2022
@ghost
Copy link

ghost commented Apr 19, 2022

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

When using the Utf8JsonWriter.WriteRawValue API, you can't pass multiple segments in to write a single value, you have to allocate a contiguous array and do a single call to WriteRawValue. In our use case we already have a ReadOnlySequence<byte> and want to write it as a raw json value.

But because the API only accepts string or span, we need to write the following:

if (result.RawSerializedData.IsSingleSegment)
{
    writer.WriteRawValue(result.RawSerializedData.First.Span, skipInputValidation: true);
}
else
{
    writer.WriteRawValue(result.RawSerializedData.ToArray(), skipInputValidation: true);
}

API Proposal

namespace System.Text.Json
{
    public sealed partial class Utf8JsonWriter
    {
+        public void WriteRawValue(ReadOnlySequence<byte> json, bool skipInputValidation = false);
+        public void WriteRawValue([StringSyntax(StringSyntaxAttribute.Json)] ReadOnlySequence<char> json, bool skipInputValidation = false);
    }
}

API Usage

void MyFunc(Utf8JsonWriter writer, ReadOnlySequence<byte> sequence)
{
    writer.WriteRawValue(sequence);
}

Alternative Designs

No response

Risks

No response

Author: BrennanConroy
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@teo-tsirpanis
Copy link
Contributor

There isn't an implicit conversion from a string to a ReadOnlySequence<char> so the [StringSyntax(StringSyntaxAttribute.Json)] is not needed. And you forgot an issue title. 😅

@BrennanConroy
Copy link
Member Author

And you forgot an issue title. 😅

Haha, I was too focused on filling out the API template!

@BrennanConroy BrennanConroy changed the title [API Proposal]: [API Proposal]: Add ReadOnlySequence overload for WriteRawValue Apr 19, 2022
@krwq krwq added this to the 8.0.0 milestone Jun 7, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 7, 2022
@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 7, 2022
@krwq
Copy link
Member

krwq commented Jun 7, 2022

Suggestion looks good to me as proposed, although we unlikely will have time to do it in this release, marking as 8.0.0 so it's on our radar for vNext.

@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

  • We don't want to add the overload ReadOnlySequence<char> because we'd have to deal with transcoding surrogate pairs across elements in the sequence, which is quite a bit of work that nobody seems to be asking for right now.
namespace System.Text.Json;

public sealed partial class Utf8JsonWriter
{
    public void WriteRawValue(ReadOnlySequence<byte> json, bool skipInputValidation = false);
}

@terrajobst terrajobst 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 16, 2022
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Sep 29, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 30, 2022
@lateapexearlyspeed
Copy link
Contributor

Hi, I would try to add this API by this PR, could you please review?

Also, I modified parameter name from json to utf8Json because it is for byte version rather than char (string), which seems being followed by existing overloads.
Correct me, thanks !

public void WriteRawValue(ReadOnlySequence{byte} utf8Json, bool skipInputValidation = false);

@eiriktsarpalis
Copy link
Member

Also, I modified parameter name from json to utf8Json because it is for byte version rather than char (string), which seems being followed by existing overloads.

Change sounds reasonable to me, thanks for correcting it.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 24, 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 help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants