Breaking changes for JsonSerializer.Deserialize #20956

gewarren · 2020-10-06T22:41:52Z

Fixes #20815.

buyaa-n · 2020-10-06T23:43:10Z

includes/core-changes/serialization/3.1/jsonserializer-deserialize-throws-jsonexception.md

+
+In previous .NET versions, when the type parameter is <xref:System.Char>, passing a null reference for the <xref:System.String> parameter of <xref:System.Text.Json.JsonSerializer.Deserialize%60%601(System.String,System.Text.Json.JsonSerializerOptions)?displayProperty=nameWithType> causes a <xref:System.NullReferenceException> to be thrown. Passing an empty string causes an <xref:System.IndexOutOfRangeException> to be thrown.
+
+In .NET Core 3.1 and later, when the type parameter is <xref:System.Char>, passing a null reference or an empty string causes a <xref:System.Text.Json.JsonException> to be thrown.


NIT: I think adding the example code might make it more clear, up to you

// Before (3.0): throws NullReferenceException // After (3.1): throw JsonException JsonSerializer.Deserialize<char>("null"); // Before (3.0): throws IndexOutOfRangeException // After (3.1): throw JsonException JsonSerializer.Deserialize<char>("");

Okay, I will add it. But isn't "null" just a string that contains the characters n, u, l, and l?

Good question, it might supposed to be JsonSerializer.Deserialize<char>(null);, but also "null" string might be serialized into null value instead of "null" string cc @layomia @ahsonkhan

Okay, I will add it. But isn't "null" just a string that contains the characters n, u, l, and l?

Yes, that is true. That example input string will showcase the change in behavior going from NullReferenceException to JsonException between 3.0 and 3.1.

I responded to this question in more detail below:
#20956 (comment)

it might supposed to be JsonSerializer.Deserialize<char>(null);,

The following snippet is invalid (it will fail to compile) and once fixed, it will throw an ArgumentNullException (which is the desired behavior and that hasn't changed since 3.0 to 5.0):

JsonSerializer.Deserialize<char>((string)null);

Another example that showcases the change in behavior is a null string in quotes, i.e. the six characters ", n, u, l, l, ".

...des/core-changes/serialization/5.0/deserializing-json-into-char-requires-single-character.md

buyaa-n

Left a NIT comment, overall looks good, thanks!

ahsonkhan · 2020-10-07T01:35:22Z

...des/core-changes/serialization/5.0/deserializing-json-into-char-requires-single-character.md

+JsonSerializer.Deserialize<char>("abc");
+
+// Correct usage.
+JsonSerializer.Deserialize<char>("a");


Both these are invalid JSON because they are missing the string within embedded quotes, These should be:

Suggested change

JsonSerializer.Deserialize<char>("abc");

// Correct usage.

JsonSerializer.Deserialize<char>("a");

JsonSerializer.Deserialize<char>("\"abc\"");

// Correct usage.

JsonSerializer.Deserialize<char>("\"a\"");

But then the first character is always ", so how do you pass a single-character string?

That is how you pass a single character string.

Here's an example valid, single-character JSON string:
"a"

In JSON, a string token is one that is surrounded with quotes.

Another valid JSON token, is a number (which isn't in quotes), so for example:
1

The way you would write that particular JSON number token in C# is as follows (note, there are no extra quotes):

JsonSerializer.Deserialize<int>("1");

More commonly, folks have JSON objects, like this:

JsonSerializer.Deserialize<SomeClass>("{ \"name\": \"value\", \"age\": 5 }");

https://www.json.org/json-en.html

Maybe if you think of it as a character array rather than strings, that might clear up the need for the quotes:

byte[] arr = {(byte)'"', (byte)'a', (byte)'"'}; Console.WriteLine(JsonSerializer.Deserialize<char>(arr.AsSpan())); // valid, single character 'a' byte[] another = {(byte)'1', (byte)'2', (byte)'3'}; Console.WriteLine(JsonSerializer.Deserialize<int>(another.AsSpan())); // valid, integer number 123

You can explore various values in jsonlint to see what is valid and invalid JSON payloads:
https://jsonlint.com/

Both these are invalid JSON because they are missing the string within embedded quotes, These should be:

Ah, forgot about that

ahsonkhan · 2020-10-07T02:48:17Z

includes/core-changes/serialization/3.1/jsonserializer-deserialize-throws-jsonexception.md

+
+// .NET Core 3.0: Throws IndexOutOfRangeException.
+// .NET Core 3.1 and later: Throws JsonException.
+JsonSerializer.Deserialize<char>("");


Suggested change

JsonSerializer.Deserialize<char>("");

JsonSerializer.Deserialize<char>("\"\"");

includes/core-changes/serialization/3.1/jsonserializer-deserialize-throws-jsonexception.md

ahsonkhan · 2020-10-07T03:01:55Z

@buyaa-n - can you please also update the examples in the breaking change issue #20815 (comment) to match the quoted string fixes I suggested above.

~~And please also include the JsonSerializer.Serialize change mentioned in dotnet/runtime#528 (comment) (in addition to the Deserialize changes you already covered)~~

Edit: Nevermind, I see the separate issue for that: #20823

ahsonkhan · 2020-10-07T03:29:05Z

Also, to clarify, there is only a single truly breaking change that needs to be documented (between version 3.1 and 5.0):

// This used to work "incorrectly" before, and now it throws an exception.
JsonSerializer.Deserialize<char>("\"abc\"");

Between 3.0 and 3.1 nothing needs to be documented as "breaking" (at least in this JSON deserialization scenario).
If we just want to document the char deserialization behavior, in general, to help readers understand the behavior better, then that is fine to do and up to you.

The other examples are changing one exception type to another and those are not considered breaking. In fact, going from NullRefException to a more user-friendly exception is encouraged.

using System;
using System.Text.Json;

namespace TestNull
{
    public class Program
    {
        public class MyPoco
        {
            public char Character { get; set; }
        }

        static void Main(string[] args)
        {
            // === Changing exception types like this is NOT considered a breaking change, no doc needed. === 
            // Before (3.0): NullReferenceException
            // After (3.1): JsonException
            // After (5.0): JsonException
            JsonSerializer.Deserialize<MyPoco>("{\"Character\": null}");

            // === Changing exception types like this is NOT considered a breaking change doc, no doc needed. === 
            // Before (3.0): NullReferenceException
            // After (3.1): JsonException
            // After (5.0): JsonException
            JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"null\"}");

            // === Changing exception types like this is NOT considered a breaking change doc, no doc needed. === 
            // Before (3.0): IndexOutOfRangeException
            // After (3.1): JsonException
            // After (5.0): JsonException
            JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"\"}");

            // === This is the ONLY change that needs to be documented as it is the only one that is breaking. === 
            // Before (3.0): Set Character to the first character - 'a'
            // Before (3.1): Set Character to the first character - 'a'
            // After (5.0): JsonException
            var val = JsonSerializer.Deserialize<MyPoco>("{\"Character\": \"abc\"}");
            Console.WriteLine(val.Character);
        }
    }
}

buyaa-n · 2020-10-07T05:51:41Z

The other examples are changing one exception type to another and those are not considered breaking. In fact, going from NullRefException to a more user-friendly exception is encouraged.

Whoops, they were within the examples of your breaking change comment so thought all of them breaking 😛, but you are right, I will update the issue accordingly, @gewarren let's remove the NRE, IOOR exceptions section from the doc, sorry for the confusion 🙏

buyaa-n · 2020-10-07T06:06:31Z

@gewarren the issues now update as needed

buyaa-n

Thank you @gewarren looks great

Fixes dotnet#20815

94a9ab8

gewarren requested a review from a team as a code owner October 6, 2020 22:41

dotnet-bot added this to the October 2020 milestone Oct 6, 2020

dotnet-bot added the 📚 Area - .NET Core Guide label Oct 6, 2020

gewarren requested review from BillWagner and buyaa-n October 6, 2020 22:47

call out char type for type parameter

6f6b801

buyaa-n reviewed Oct 6, 2020

View reviewed changes

...des/core-changes/serialization/5.0/deserializing-json-into-char-requires-single-character.md Show resolved Hide resolved

buyaa-n approved these changes Oct 6, 2020

View reviewed changes

gewarren added 2 commits October 6, 2020 17:15

add code examples

5474082

add period

9e856b8

ahsonkhan reviewed Oct 7, 2020

View reviewed changes

includes/core-changes/serialization/3.1/jsonserializer-deserialize-throws-jsonexception.md Outdated Show resolved Hide resolved

gewarren added 2 commits October 7, 2020 06:55

remove 3.1 breaking change

570b4d4

update code examples to include quotes

d2b5a56

buyaa-n approved these changes Oct 7, 2020

View reviewed changes

gewarren merged commit d5a359e into dotnet:master Oct 7, 2020

gewarren deleted the json-deserialize branch October 7, 2020 18:28

BillWagner added dotnet-core/prod and removed 📚 Area - .NET Core Guide labels Feb 10, 2021


		In previous .NET versions, when the type parameter is <xref:System.Char>, passing a null reference for the <xref:System.String> parameter of <xref:System.Text.Json.JsonSerializer.Deserialize%60%601(System.String,System.Text.Json.JsonSerializerOptions)?displayProperty=nameWithType> causes a <xref:System.NullReferenceException> to be thrown. Passing an empty string causes an <xref:System.IndexOutOfRangeException> to be thrown.

		In .NET Core 3.1 and later, when the type parameter is <xref:System.Char>, passing a null reference or an empty string causes a <xref:System.Text.Json.JsonException> to be thrown.

	JsonSerializer.Deserialize<char>("");
	JsonSerializer.Deserialize<char>("\"\"");

Breaking changes for JsonSerializer.Deserialize #20956

Breaking changes for JsonSerializer.Deserialize #20956

Uh oh!

Conversation

gewarren commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

gewarren Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

buyaa-n Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

buyaa-n left a comment

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

gewarren Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buyaa-n Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahsonkhan commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buyaa-n commented Oct 7, 2020

Uh oh!

buyaa-n left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gewarren commented Oct 6, 2020 •

edited

Loading

ahsonkhan Oct 7, 2020 •

edited

Loading

ahsonkhan Oct 7, 2020 •

edited

Loading

ahsonkhan Oct 7, 2020 •

edited

Loading

ahsonkhan commented Oct 7, 2020 •

edited

Loading

ahsonkhan commented Oct 7, 2020 •

edited

Loading

buyaa-n commented Oct 7, 2020 •

edited

Loading