Skip to content

Cleanup MvcJsonHelper #6529

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
Merged
pranavkm merged 3 commits into from
Jan 10, 2019
Merged

Cleanup MvcJsonHelper #6529

pranavkm merged 3 commits into from
Jan 10, 2019

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jan 9, 2019

  • Remove dependency on JsonOutputFormatter
  • Cache JsonSerializer for the default case

@pranavkm pranavkm requested review from JamesNK, dougbu and rynowak January 9, 2019 18:52
pranavkm
pranavkm commented Jan 9, 2019
View reviewed changes
Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Cleaning up JsonHelper \ output formatter while we're making breaking changes to the type \ assembly

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonHelper.cs Outdated
Copy link
Contributor Author

@pranavkm pranavkm Jan 9, 2019

Choose a reason for hiding this comment

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

NewtonsoftJsonOutputFormatter applies two other settings to the writer it creates - CloseOutput = false, AutoCompleteOnClose = false, neither of which seemed important here?

Copy link
Member

@JamesNK JamesNK Jan 9, 2019

Choose a reason for hiding this comment

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

Correct

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonOutputFormatter.cs Outdated
Copy link
Contributor Author

@pranavkm pranavkm Jan 9, 2019

Choose a reason for hiding this comment

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

Inlined this. This API isn't used by anything now and I'm not sure it has much value

Copy link

@richardwan richardwan May 20, 2020

Choose a reason for hiding this comment

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

what should application code that used to call this method do to respond to this breaking change? we are in the middle of upgrading the newtonsoft lversion.

Copy link
Contributor Author

@pranavkm pranavkm May 20, 2020

Choose a reason for hiding this comment

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

@richardwan you could call jsonSerializer.Serialize(jsonWriter, value) in your application code. If you need to discuss this further, please file an issue.

@pranavkm
Cleanup MvcJsonHelper
af6eddd 
* Remove dependency on JsonOutputFormatter
* Cache JsonSerializer for the default case
@pranavkm pranavkm force-pushed the prkrishn/json-helper branch from c0cf8d9 to af6eddd Compare January 9, 2019 21:54
pranavkm
pranavkm commented Jan 9, 2019
View reviewed changes
Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

🆙 📅

JamesNK
JamesNK approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

:shipit:

dougbu
dougbu approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Minor comments but otherwise :shipit: after confirming thread-safety with @JamesNK

@rynowak
Copy link
Member

rynowak commented Jan 10, 2019

This is a super good cleanup. This is something I always wanted to change but was afraid of the breaking potential.

What's the reason for the difference in output, is it that we're HTML-encoding inside JSON.NET rather than letting the Razor infrastructure do it?

@dougbu
Copy link
Contributor

dougbu commented Jan 10, 2019

What's the reason for the difference in output, is it that we're HTML-encoding inside JSON.NET rather than letting the Razor infrastructure do it?

@pranavkm and I discussed this test output. It previously indicated a bug -- un-escaped HTML would have flowed through to responses because Razor doesn't do anything more with HtmlStrings.

@pranavkm pranavkm merged commit 55ec35b into master Jan 10, 2019
@pranavkm pranavkm deleted the prkrishn/json-helper branch January 10, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesNK JamesNK JamesNK approved these changes

@rynowak rynowak Awaiting requested review from rynowak

+2 more reviewers

@richardwan richardwan richardwan left review comments

@dougbu dougbu dougbu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pranavkm @rynowak @dougbu @JamesNK @richardwan