Skip to content
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.

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

Annotate System.Text.Json for nullable #528

Merged
merged 28 commits into from
Dec 31, 2019
Merged

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Dec 4, 2019

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

CC @ahsonkhan @steveharter @stephentoub @safern

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

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.

…ion/JsonSerializer.Read.Stream.cs


Remove not needed attribute
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Dec 31, 2019

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

@buyaa-n buyaa-n merged commit 81bf79f into dotnet:master Dec 31, 2019
@danmoseley
Copy link
Member

Yay!

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

ahsonkhan commented Jan 4, 2020

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
Contributor Author

buyaa-n commented Jan 4, 2020

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 😄

@ahsonkhan ahsonkhan added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Feb 20, 2020
@ahsonkhan
Copy link
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:

public class MyPoco
{
    public char Character { get; set; }
}

public static void TestDeserializeToChar()
{
    // Before (3.1): NullReferenceException
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": null}");

    // Before (3.1): IndexOutOfRangeException
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"\"}");

    // Before (3.1): Set Character to the first character - 'a'
    // After (5.0): JsonException
    JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"abc\"}");
}

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

public static void TestSerializeNullType()
{
    // 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
Member

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

@danmoseley danmoseley 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-n buyaa-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
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants