Skip to content

Annotate System.Text.Json for nullable - #528

Merged
buyaa-n merged 28 commits into
dotnet:masterfrom
buyaa-n:json_nullable
Dec 31, 2019
Merged

Annotate System.Text.Json for nullable#528
buyaa-n merged 28 commits into
dotnet:masterfrom
buyaa-n:json_nullable

Conversation

@buyaa-n

Copy link
Copy Markdown
Contributor

Recreating PR as old one #114 no more accessible, gone with old private fork

CC @ahsonkhan@steveharter@stephentoub@safern

@ahsonkhanahsonkhan left a comment

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.

48/125 files reviewed (JsonDocument, JsonElement, JsonNode, and related types).

Here's some feedback so far.

Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Node/JsonNull.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/JsonException.cs Outdated
…ion/JsonSerializer.Read.Stream.cs
Remove not needed attribute
@buyaa-n

Copy link
Copy Markdown
ContributorAuthor

Failed CI legs doesn't look like caused from this PR changes, gonna merge this

@buyaa-n
buyaa-n merged commit 81bf79f into dotnet:masterDec 31, 2019
@danmoseley

Copy link
Copy Markdown
Contributor

Yay!

@buyaa-n
buyaa-n deleted the json_nullable branch January 2, 2020 19:09
@ahsonkhan

ahsonkhan commented Jan 4, 2020

Copy link
Copy Markdown
Contributor

gonna merge this

Awesome! Thanks for your patience and effort in getting this merged, @buyaa-n. I appreciate you taking your time with it and getting the details right. Nullability annotations of a complex library like JSON hopefully help explore and refine the guidelines.

@buyaa-n

Copy link
Copy Markdown
ContributorAuthor

Awesome! Thanks for your patience and effort in getting this merged, @buyaa-n. I appreciate you taking your time with it and getting the details right. Nullability annotations of a complex library like JSON hopefully help explore and refine the guidelines.

Thanks for your delicate review, learnt a lot. Have fun with nullability aware coding 😄

@ahsonkhanahsonkhan added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Feb 20, 2020
@ahsonkhan

Copy link
Copy Markdown
Contributor

Moving comment from #114 (comment) (since that PR was closed).

Highlighting the breaking change here (which became visible when enabling nullability analysis).

Deserializing into a char is now more strict to match user expectations. The payload must contain a single-character string for deserialization to succeed:

publicclassMyPoco{publiccharCharacter{get;set;}}publicstaticvoidTestDeserializeToChar(){// Before (3.1): NullReferenceException// After (5.0): JsonExceptionJsonSerializer.Deserialize<MyPoco>("{\"Character\": null}");// Before (3.1): IndexOutOfRangeException// After (5.0): JsonExceptionJsonSerializer.Deserialize<MyPoco>("{\"Character\": \"\"}");// Before (3.1): Set Character to the first character - 'a'// After (5.0): JsonExceptionJsonSerializer.Deserialize<MyPoco>("{\"Character\": \"abc\"}");}

Passing in null for the Type parameter while serializing now correctly throws ArgumentNullException rather than returning "null" JSON:

publicstaticvoidTestSerializeNullType(){// Before (3.1): Returned a string with value "null"// After (5.0): ArgumentNullException: Value cannot be null. (Parameter 'type')JsonSerializer.Serialize(null,null);// Same with other Serialize{Async} overloads// Before (3.1): Returned a byte[] with value "null"// After (5.0): ArgumentNullException: Value cannot be null. (Parameter 'type')JsonSerializer.SerializeToUtf8Bytes(null,null);}

@danmoseley

Copy link
Copy Markdown
Contributor

@buyaa-n@layomia Does this need a breaking change issue?

@danmoseleydanmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 30, 2020
@buyaa-nbuyaa-n removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 29, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@buyaa-n@danmoseley@ahsonkhan@stephentoub@steveharter@layomia@bartonjs@jozkee@safern@ericstj@Dotnet-GitSync-Bot