Skip to content

Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default - #41539

Merged
layomia merged 3 commits into
dotnet:masterfrom
layomia:web_default
Sep 3, 2020
Merged

Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default#41539
layomia merged 3 commits into
dotnet:masterfrom
layomia:web_default

Conversation

@layomia

@layomialayomia commented Aug 28, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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).

@layomialayomia added this to the 5.0.0 milestone Aug 28, 2020
@layomialayomia self-assigned this Aug 28, 2020
jozkee
jozkee approved these changes Aug 29, 2020
View reviewed changes

@jozkeejozkee 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.

Otherwise; LGTM, thanks.

Comment threadsrc/libraries/System.Net.Http.Json/tests/FunctionalTests/TestClasses.cs
@steveharter

steveharter commented Aug 31, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@layomialayomia mentioned this pull request Sep 1, 2020
Merged
ahsonkhan
ahsonkhan reviewed Sep 1, 2020
View reviewed changes
Comment threadsrc/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs
@layomia

layomia commented Sep 2, 2020

Copy link
Copy Markdown
ContributorAuthor

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.

@layomialayomia modified the milestones: 5.0.0, 6.0.0Sep 2, 2020
@layomia

layomia commented Sep 3, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@layomia
layomia merged commit dbacdc2 into dotnet:masterSep 3, 2020
@layomia
layomia deleted the web_default branch September 3, 2020 21:16
@ahsonkhan

ahsonkhan commented Sep 14, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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

ericstj commented Sep 14, 2020

Copy link
Copy Markdown
Member

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
@layomialayomia 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
@nathanpovonathanpovo mentioned this pull request Oct 11, 2020
Closed
@layomia

layomia commented Oct 14, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@layomialayomia added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Oct 14, 2020
@ghostghost 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

ahsonkhan commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

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

ericstj commented Oct 14, 2020

Copy link
Copy Markdown
Member

Is that what's warranting a breaking change doc?

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

@layomialayomia modified the milestones: 6.0.0, 5.0.0Oct 15, 2020
@layomialayomia 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
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkeejozkeejozkee approved these changes
@terrajobstterrajobstAwaiting requested review from terrajobst
+3 more reviewers
@ahsonkhanahsonkhanahsonkhan left review comments
@pranavkmpranavkmpranavkm approved these changes
@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@layomialayomia

Labels

area-System.Text.Jsonbreaking-changeIssue 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