Skip to content

Use System.Text.Json's copy ctor - #21016

Merged
pranavkm merged 1 commit into
masterfrom
prkrishn/copy-ctor
Apr 20, 2020
Merged

pranavkm merged 1 commit into
masterfrom
prkrishn/copy-ctor

Conversation

@pranavkm

Copy link
Copy Markdown
Contributor

No description provided.

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 20, 2020
@pranavkm
pranavkm requested a review from jozkee April 20, 2020 15:49
@pranavkm pranavkm added this to the 5.0.0-preview4 milestone Apr 20, 2020
@davidfowl

Copy link
Copy Markdown
Member

Is this something we should patch?

@pranavkm

Copy link
Copy Markdown
Contributor Author

The copy ctor and the property being tested are new in 5.0.

@davidfowl

Copy link
Copy Markdown
Member

I see

jsonSerializerOptions = new JsonSerializerOptions(jsonSerializerOptions)
{
Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hopefully the new CreateForWeb option uses the right defaults and then we don't need this at all 👀

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rynowak @pranavkm It seems like you want us to include this in the new JsonSerializerOptions for web. So, the web options will have PropertyNameCaseInsensitive = true, PropertyNamingPolicy = JsonNamingPolicy.CamelCase, and Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping.

Is that correct?

I also pointed this out on dotnet/runtime#34626 (comment) in case you missed it.
cc @layomia

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also cc @terrajobst.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, when I spoke to @GrabYourPitchforks, the recommendation was that using UnsafeRelaxedJsonEscaping is only safe if the JSON wasn't interspersed with markup. Web might not convey that this is meant for REST \ API scenarios.

@jozkee jozkee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants