Skip to content

Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default #41539

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
layomia merged 3 commits into from
Sep 3, 2020
Merged

Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default #41539

layomia merged 3 commits into from
Sep 3, 2020

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Aug 28, 2020 •
edited
Loading

Follow up from #39363 / #30255.

During API review for the number handling feature, it was discussed that number types being written as strings is a common pattern in many JSON producers (e.g API endpoints) across the web. Adding JsonNumberHandling.AllowReadingFromString as a web default makes it easier for .NET web applications to consume these numbers in a consistent manner (same options for client/shared/server).

@layomia layomia added this to the 5.0.0 milestone Aug 28, 2020
@layomia layomia requested a review from steveharter as a code owner August 28, 2020 22:35
@layomia layomia self-assigned this Aug 28, 2020
jozkee
jozkee approved these changes Aug 29, 2020
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Otherwise; LGTM, thanks.

@steveharter
Copy link
Contributor

steveharter commented Aug 31, 2020 •
edited
Loading

I assume this was requested by the ASP.NET team (@pranavkm can you verify)? I didn't see this ask in the two linked issues above.

Update: per offline discussion, this was recommended during the API review and then discussed again offline with ASP.NET.

@layomia layomia mentioned this pull request Sep 1, 2020
Merged
@layomia
Copy link
Contributor Author

layomia commented Sep 2, 2020

Moving to 6.0 as the accompanying perf change I'd like to get in along with this PR will not meet the bar for back-porting to the .NET 5 release - #41679.

@layomia layomia modified the milestones: 5.0.0, 6.0.0 Sep 2, 2020
@layomia
Copy link
Contributor Author

layomia commented Sep 3, 2020

Test failures are unrelated: System.Net.Http.Functional.Tests.

@layomia layomia merged commit dbacdc2 into dotnet:master Sep 3, 2020
@layomia layomia deleted the web_default branch September 3, 2020 21:16
@ahsonkhan
Copy link
Contributor

ahsonkhan commented Sep 14, 2020 •
edited
Loading

Moving to 6.0

Changing such defaults is a breaking change, even if we are making things more forgiving, given these are default settings that folks will rely on, because being strict/validate on its own is a feature. If this leniency is important for Web, we should consider porting the change to 5.0 to avoid having to make a breaking change in the future.

@terrajobst, @ericstj

@ericstj
Copy link
Member

ericstj commented Sep 14, 2020

I believe @layomia is already doing this.

layomia added a commit to layomia/dotnet_runtime that referenced this pull request Sep 15, 2020
@layomia
Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web…
43c91e6 
… default (dotnet#41539)

* Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default

* Add test assertion for number handling option in S.N.H.Json

* Test number-as-string behavior in System.Net.Http.Json
@layomia layomia mentioned this pull request Sep 15, 2020
Merged
layomia added a commit that referenced this pull request Sep 15, 2020
@layomia
Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web…
ab41ed8 
… default (#41539) (#42240)

* Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default

* Add test assertion for number handling option in S.N.H.Json

* Test number-as-string behavior in System.Net.Http.Json
@nathanpovo nathanpovo mentioned this pull request Oct 11, 2020
Closed
@layomia
Copy link
Contributor Author

layomia commented Oct 14, 2020

Filed a breaking change doc to advertise the significance of this change to ASP.NET Core apps - dotnet/docs#21067.

@layomia layomia added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Oct 14, 2020
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 14, 2020
@ahsonkhan
Copy link
Contributor

ahsonkhan commented Oct 14, 2020

The AllowReadingFromString and Web default serializer option are all new in 5.0. Is the default behavior for aspnet apps changing between 3.1 and 5.0 because of them? Is that what's warranting a breaking change doc?

@ericstj
Copy link
Member

ericstj commented Oct 14, 2020

Is that what's warranting a breaking change doc?

@ahsonkhan, check out the doc issue that @layomia linked it explains it pretty well.

@layomia layomia modified the milestones: 6.0.0, 5.0.0 Oct 15, 2020
@layomia layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 15, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkee jozkee jozkee approved these changes

@terrajobst terrajobst Awaiting requested review from terrajobst

+3 more reviewers

@ahsonkhan ahsonkhan ahsonkhan left review comments

@pranavkm pranavkm pranavkm approved these changes

@steveharter steveharter steveharter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@layomia layomia

Labels

area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a previous release.

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@layomia @steveharter @ahsonkhan @ericstj @pranavkm @jozkee