Fix continuation of JSON deserialization

[devsko]

Fixes #42070

When deserialization continues with a new chunk of data in the middle of a null token, a JsonException is thrown.
This PR attempts to fix this issue.

I verified that the change fixes the original repro.
I also verified that the two test cases (but with payload just beneath 2GB) mentioned by @am11 in this comment work as expected with the changes in this PR.

The risk of regressions is low. Only continuation situations at null tokens are affected that throw or result in unexpected null values without the change.

[layomia]

Thanks for the PR! I requested a few more tests.

[ahsonkhan]

These are great tests and a clever way to get a lot more continuation coverage! Nicely done.

If you are up for it, I think it would be really valuable to do a similar thing with the Utf8JsonReader overload of JsonSerialier.Deserialize (probably in a separate PR), which also supports continuation/partial buffers via the ReadOnlySequence<byte> (where the whole JSON is available in memory, but split in non-contiguous chunks).

JsonSerializer.Deserialize docs
Utf8JsonReader ctor docs

[devsko]

Thanks!
I will have a look.

[ahsonkhan]

I verified that the change fixes the original repro.
I also verified that the two test cases (but with payload just beneath 2GB) mentioned by @am11 in this comment work as expected with the changes in this PR.

If feasible (and quick to run), can we also add to the test test a standalone copy of the "regression" test that was initially reported on the issue: #42070 (comment)

That way, that exact customer reported scenario is guarded against in the test.

Similar to these (that were also inspired from the reported issue):

runtime/src/libraries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.ObjectModel.Read.cs

Line 19 in 6072e4d

// Regression test for https://github.com/dotnet/runtime/issues/30686.

runtime/src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.Date.cs

Lines 150 to 152 in 5285c14

	[Fact]
	// https://github.com/dotnet/runtime/issues/30095
	public static void TestingDateTimeMinValue_UtcOffsetGreaterThan0()

runtime/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs

Lines 3091 to 3097 in 5285c14

	// https://github.com/dotnet/runtime/issues/30746
	[Theory]
	[InlineData(true, true)]
	[InlineData(true, false)]
	[InlineData(false, true)]
	[InlineData(false, false)]
	public void Writing3MBBase64Bytes(bool formatted, bool skipValidation)

runtime/src/libraries/System.Text.Json/tests/Serialization/Object.ReadTests.cs

Lines 111 to 112 in 5285c14

	// Scenario described in https://github.com/dotnet/runtime/issues/29021
	array = JsonSerializer.Deserialize<object[]>("[1, false]");

runtime/src/libraries/System.Text.Json/tests/Serialization/ReadValueTests.cs

Lines 671 to 673 in 5285c14

	// From https://github.com/dotnet/runtime/issues/882
	[Fact]
	public static void OptionsFollowToConverter()

[devsko]

can we also add to the test test a standalone copy of the "regression" test that was initially reported

I was thinking about that but there are some obstacles. The original repro depends on some third party packages. Besides that it is not deterministic because nobody knew the root cause at this time. Sometimes it runs for minutes until randomly a null token gets hit by the continuation.

[frankbuckley]

I was thinking about that but there are some obstacles.

@devsko / @ahsonkhan - thank you for looking into this.

I have added code to save failing examples to the repro - the failing json examples are around 100KB. Maybe a few of these could be used for regression tests?

Also, the captured output includes some examples of a NullReferenceException - in addition to examples of the more common InvalidOperationException - for example, 0228.

@am11 noted a NullReferenceException and I have seen this twice with real data.

Does the fix for the InvalidOperationExceptions also fix the NullReferenceExceptions?

[devsko]

I have added code to save failing examples to the repro - the failing json examples are around 100KB. Maybe a few of these could be used for regression tests?

Great! Thank you. I will add a test for each observed exception type.

Does the fix for the InvalidOperationExceptions also fix the NullReferenceExceptions?

It does.

[layomia]

We store other test payloads in this Strings.resx file. Based on some other entries, the data name should be SomeDescriptivePayloadSize e.g. BigPayloadWithNulls101KB.

The usage would be string json = SR.BigPayloadWithNulls101KB;

[layomia]

A couple test comments, otherwise LGTM.

[layomia]

nit: this test can be written as

// From https://github.com/dotnet/runtime/issues/42070
[Theory]
[MemberData(nameof(ContinuationAtNullTokenTestData))]
public static async Task ContinuationAtNullToken(string payload)
{
    using (Stream stream = new MemoryStream(Encoding.UTF8.GetBytes(payload)))
    {
        CustomerCollectionResponse response = await JsonSerializer.DeserializeAsync<CustomerCollectionResponse>(stream, new JsonSerializerOptions { IgnoreNullValues = true });
        Assert.Equal(50, response.Customers.Count);
    }
}

private static IEnumerable<object[]> ContinuationAtNullTokenTestData()
{
    yield return new object[] { SR.CustomerSearchApi108KB };
    yield return new object[] { SR.CustomerSearchApi107KB };
}

[layomia]

The first four tests in this file are very similar, the only difference being the inner type passed to outer, and also an Assert.Null test if the inner type is a reference type. To avoid the duplication: can we consolidate them into one generic test helper, and run the test like this?

[Theory]
[MemberData(nameof(TestData))]
public static async Task ContinuationShouldWorkAtAnyPosition_ParameterlessCtor(int paddingLength, bool ignoreNullValues)
{
    await RunTest<TestClass<NestedClass>, NestedClass>(paddingLength, ignoreNullValues);
    await RunTest<TestClass<NestedValueType>, NestedValueType>(paddingLength, ignoreNullValues);
    await RunTest<TestValueType<NestedClass>, NestedClass>(paddingLength, ignoreNullValues);
    await RunTest<TestValueType<NestedValueType>, NestedValueType>(paddingLength, ignoreNullValues);
}

private static async Task RunTest<TInner, TInnermost>(int paddingLength, bool ignoreNullValues)
    where TInner : ITestInterface<TInnermost>, new()
    where TInnermost : INestedInterface, new()
{
    var stream = new MemoryStream();
    {
        var obj = new Outer<TInner>
        {
            S = new string('x', paddingLength),
            C = new()
            {
                A = "Hello",
                B = null,
                C = 42,
                D = null,
                E = 3.14e+17f,
                F = null,
                G = true,
                H = null,
                I = new int[] { 42, 17 },
                J = new()
                {
                    A = null,
                    B = 7,
                }
            }
        };
        await JsonSerializer.SerializeAsync(stream, obj, new JsonSerializerOptions { Converters = { new OuterConverter<TestClass<NestedClass>>() } });
    }

    stream.Position = 0;
    {
        var readOptions = new JsonSerializerOptions
        {
            DefaultBufferSize = 128,
            IgnoreNullValues = ignoreNullValues,
        };

        Outer<TestClass<NestedClass>> obj = await JsonSerializer.DeserializeAsync<Outer<TestClass<NestedClass>>>(stream, readOptions);

        Assert.Equal(new string('x', paddingLength), obj.S);
        Assert.Equal("Hello", obj.C.A);
        Assert.Null(obj.C.B);
        Assert.Equal(42, obj.C.C);
        Assert.Null(obj.C.D);
        Assert.Equal(3.14e17f, obj.C.E);
        Assert.Null(obj.C.F);
        Assert.True(obj.C.G);
        Assert.Null(obj.C.H);
        Assert.Collection(obj.C.I, v => Assert.Equal(42, v), v => Assert.Equal(17, v));

        bool innerIsRefType = typeof(TInner).IsValueType == false;
        if (innerIsRefType)
        {
            Assert.NotNull(obj.C.J);
        }

        Assert.Null(obj.C.J.A);
        Assert.Equal(7, obj.C.J.B);
    }
}

private interface ITestInterface<TNested>
{
    public string A { get; set; }
    public string B { get; set; }
    public int C { get; set; }
    public int? D { get; set; }
    public float E { get; set; }
    public float? F { get; set; }
    public bool G { get; set; }
    public bool? H { get; set; }
    public int[] I { get; set; }
    public TNested J { get; set; }
}

private class TestClass<TNested> : ITestInterface<TNested>
{
    public string A { get; set; }
    public string B { get; set; }
    public int C { get; set; }
    public int? D { get; set; }
    public float E { get; set; }
    public float? F { get; set; }
    public bool G { get; set; }
    public bool? H { get; set; }
    public int[] I { get; set; }
    public TNested J { get; set; }
}

private class TestValueType<TNested> : ITestInterface<TNested>
{
    public string A { get; set; }
    public string B { get; set; }
    public int C { get; set; }
    public int? D { get; set; }
    public float E { get; set; }
    public float? F { get; set; }
    public bool G { get; set; }
    public bool? H { get; set; }
    public int[] I { get; set; }
    public TNested J { get; set; }
}

private interface INestedInterface
{
    public string A { get; set; }
    public int B { get; set; }
}

private class NestedClass : INestedInterface
{
    public string A { get; set; }
    public int B { get; set; }
}

private struct NestedValueType : INestedInterface
{
    public string A { get; set; }
    public int B { get; set; }
}

I didn't look further, but there might be a way to abstract further and reduce duplicate LOC for the parameterized ctor test; up to you.

[devsko]

The first four tests in this file are very similar

I already completely refactored this for the same reason. Follow up PR is coming soon.

[layomia]

Above feedback on tests can be addressed in follow up. Merging now to get this PR ready for ship-room consideration for .NET 5.

Thanks @devsko

[layomia]

I already started to work on a follow up PR for the other continuations.

@devsko - can you include a test for when partial buffer ends on a valid prefix for null, but turns out to be invalid JSON on continuation? This is likely going to be JsonException, but it would be good to encode the behavior in a test.

e.g.

Initial buffer: {"Foo":"Bar","Baz":nu

Next buffer: lllt,"Qux":"Quux"}