[release/3.1] Do not null ref or throw index out of range exception when deserializing to char.

[ahsonkhan]

Get the changes from 5.0/master found in dotnet/runtime#528

Description

Add a null/empty string check when deserializing JSON strings into a char property so that we don't null ref or throw an index of out of range exception. Intentionally left out the string.length > 1 check as that could potentially break someone.

Customer Impact

Most people are very unlikely to have a catch handler for NullReferenceException, IndexOutOfRangeException, or InvalidOperationException (and in the first two, it certainly doesn't make sense for them to). So in server scenarios customers still get an HTTP 500.
Now, the the JsonConverter<char> throws an InvalidOperationException which the wraps the into an outer JsonException so that customers can have handlers to catch that. For them, we’d turn a crash into success.

Deserializing into a char is now more reliable when the input JSON payload is unknown/provided by the user which may contain unexpected values.

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\": \"\"}");
}

There is a workaround for the user today, where they could create and register their own JsonConverter<char> which does the right checks. However, doing so, in all the entry points of the JsonSerializer (including within ASP.NET web app) would become cumbersome.

Regression

No. The bug existed in the deserialization behavior since the initial release of S.T.Json as part of 3.0.

Testing

Added appropriate unit tests and the behavior in master meets user expectations such as dotnet/runtime#32429
This only affects deserialization/reading.

Risk

Low given it is a very targeted fix with no regression. We are going from an unrecoverable/less user friendly exception to a more user-friendly exception that the caller can reason about. There is no breaking change here.

cc @ericstj

[danmoseley]

Do we have any customer reports of this since it went out last fall? Or was this just from code inspection. Perhaps we should give it a little time to bake in 5.0 previews.

[danmoseley]

Please wait to merge - @Anipik will do it when branch is open and this is signed off/green

[Anipik]

@joperezr do we need to do assembly version here ? It builds against netfx so I wasn't sure if assembly redirects will get broken on updating assembly version.

[ericstj]

We need to bump assembly version when shipping a new NuGet package.

[joperezr]

We'll only have to do the packaging/versioning change once per servicing release, and I thought that @ahsonkhan was already going to do this here #42867 for Json. If that's the case, we would only have to do it here in case it doesn't ship on the same servicing release as the other PR.

[ahsonkhan]

We'll only have to do the packaging/versioning change once per servicing release, and I thought that @ahsonkhan was already going to do this here #42867 for Json. If that's the case, we would only have to do it here in case it doesn't ship on the same servicing release as the other PR.

Can we update the packaging/versioning of S.T.Json in a separate PR after both servicing fixes get merged since we have more than one in this case.

[joperezr]

I suppose we could and we probably wouldn't need an extra approval from shiproom, but if one of the two PRs is already approved by shiproom and it is ready to go, I would just go and do it on that one and merge it first in order to make sure we don't forget doing it.

[ahsonkhan]

but if one of the two PRs is already approved by shiproom and it is ready to go, I would just go and do it on that one and merge it first in order to make sure we don't forget doing it.

Both are approved by shiproom and ready to go for the vNext milestone. Let's get those two merged (when the branch is open and @Anipik is ready) and then address the packaging/versioning in a separate PR (targeting the same vNext milestone).