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.

Sign up for GitHub

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

Jump to bottom

Add write-raw APIs to Utf8JsonWriter #54254

Merged
6 commits merged into from
Jul 3, 2021
Merged

Add write-raw APIs to Utf8JsonWriter #54254

6 commits merged into from
Jul 3, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Jun 15, 2021

Fixes #1784.

FYI @ahsonkhan

@layomia layomia added this to the 6.0.0 milestone Jun 15, 2021
@layomia layomia self-assigned this Jun 15, 2021
@layomia layomia requested a review from jozkee as a code owner June 15, 2021 23:42
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 15, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #1784.

FYI @ahsonkhan

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 6.0.0

ahsonkhan
ahsonkhan previously requested changes Jun 16, 2021
View reviewed changes
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonWriterOptions.SkipValidation needs to be honored, particularly when it is set to false.

@davidfowl
Copy link
Member

davidfowl commented Jun 16, 2021 •
edited
Loading

I used a similar API in JSON.NET ~8 years ago and it caused my first security bug 🤣

@layomia
Copy link
Contributor Author

layomia commented Jun 23, 2021

@ahsonkhan thanks for reviewing! Will address the feedback shortly.

@layomia layomia mentioned this pull request Jul 1, 2021
Merged
@ahsonkhan ahsonkhan self-requested a review July 1, 2021 22:46
steveharter
steveharter approved these changes Jul 2, 2021
View reviewed changes
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; misc XLM doc\test feedback.

src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Raw.cs Outdated Show resolved Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Raw.cs Outdated Show resolved Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Raw.cs Show resolved Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Utf8JsonWriterTests.WriteRaw.cs Show resolved Hide resolved
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 2, 2021
@steveharter
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@layomia layomia dismissed ahsonkhan’s stale review July 2, 2021 16:48

I've addressed @ahsonkhan's feedback.

@ghost
Copy link

ghost commented Jul 2, 2021

Hello @layomia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit ad51267 into dotnet:main Jul 3, 2021
@layomia layomia mentioned this pull request Jul 19, 2021
Merged
This was referenced Jul 28, 2021
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steveharter steveharter steveharter approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@jozkee jozkee Awaiting requested review from jozkee

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan

Assignees

@layomia layomia

Labels
area-System.Text.Json new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Writing raw JSON values when using Utf8JsonWriter
5 participants
@layomia @davidfowl @steveharter @eiriktsarpalis @ahsonkhan