dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.74k forks source link

Position information lost when calling `JsonSerializer.Deserialize` overloads accepting `Utf8JsonReader`. #97893

Open gregsdennis opened 9 months ago

gregsdennis commented 9 months ago

Description

It seems that when attempting to deserialize a type that's nested within another, and there's a failure (e.g. wrong JSON type) on the nested data, the reported position information is incorrect.

Reproduction Steps

Repro: ConsoleApp1.zip

It might not be minimal, but it's pretty small.

Expected behavior

The position in the JSON source should be reported correctly.

Actual behavior

The position in the JSON source is not reported correctly.

Regression?

I'm not sure if this is a regression for source gen, but it definitely works without source gen.

Known Workarounds

We've created a number of JsonSerializerOptions extension methods in Json.More to mitigate this for now, but it's a less desired approach.

Configuration

Libraries multitargeting .Net Standard 2.0 & .Net 8 Tests running in .Net 6 and .Net 8 (fails in both)

Repro is just .Net 8

Other information

Some offline context from @eiriktsarpalis:

JsonSerializer.Deserialize methods acceping a Ut8fJsonReader. The methods need to create a new Utf8JsonReaderthat is scoped to the particular JSON value you are trying to deserialize, which in the process loses information on the data that has been read so far.

ghost commented 9 months ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description It seems that when attempting to deserialize a type that's nested within another, and there's a failure (e.g. wrong JSON type) on the nested data, the reported position information is incorrect. ### Reproduction Steps Repro: [ConsoleApp1.zip](https://github.com/dotnet/runtime/files/14144780/ConsoleApp1.zip) It might not be minimal, but it's pretty small. ### Expected behavior The position in the JSON source should be reported correctly. ### Actual behavior The position in the JSON source is not reported correctly. ### Regression? I'm not sure if this is a regression for source gen, but it definitely works without source gen. ### Known Workarounds We've created a number of `JsonSerializerOptions` extension methods in `Json.More` to mitigate this for now, but it's a less desired approach. ### Configuration Libraries multitargeting .Net Standard 2.0 & .Net 8 Tests running in .Net 6 and .Net 8 (fails in both) Repro is just .Net 8 ### Other information Some offline context from @eiriktsarpalis: > `JsonSerializer.Deserialize` methods acceping a `Ut8fJsonReader`. The methods need to create a new `Utf8JsonReader `that is scoped to the particular JSON value you are trying to deserialize, which in the process loses information on the data that has been read so far.
Author: gregsdennis
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 9 months ago

Minimal reproduction:

var reader = new Utf8JsonReader("""
    [
        42
    ]
    """u8);

reader.Read();
reader.Read();

try
{
    JsonSerializer.Deserialize<string>(ref reader);
}
catch (JsonException e)
{
    Console.WriteLine(e.LineNumber); // 0, should be 1
    Console.WriteLine(e.BytePositionInLine); // 2, should be 6
}