Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Make sure original Utf8JsonReader options are honored when re-creating the reader. #42867

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

ahsonkhan
Copy link

@ahsonkhan ahsonkhan commented Feb 20, 2020

Ports dotnet/runtime#31791 from 5.0/master
Fixes dotnet/runtime#882

Description

Generate the JsonReaderOptions from the the user-defined, passed-in JsonSerializerOptions, and use that to create the Utf8JsonReader, rather than the default options. The fix is a couple of lines, with the rest of the PR adding tests and test enhancement.

Customer Impact

This bug was initially reported in user-defined JsonConverter<T> where the user couldn't deserialize a deeply nested object graph (beyond the default max depth of 64). The issue, however, is not just specific to converters (nor just to MaxDepth) and will be hit whenever the caller passes in a Utf8JsonReader to the JsonSerializer.Deserialize method with non-default JsonReaderOptions set (which currently includes MaxDepth, JsonCommentHandling, and AllowTrailingCommas).

For example, this code snippet incorrectly fails with an exception:

var localReader = new Utf8JsonReader(Encoding.UTF8.GetBytes("[null,]"), 
                                       new JsonReaderOptions { AllowTrailingCommas = true });

string[] returnedValue = JsonSerializer.Deserialize<string[]>(ref localReader);

// System.Text.Json.JsonException : The JSON array contains a trailing comma at the end 
// which is not supported in this mode. Change the reader options. Path: $[1] | 
// LineNumber: 0 | BytePositionInLine: 6.

It is clear that we should be honoring the user-defined options. Otherwise, there is no way for callers to deserialize payloads containing trailing commas or comments when using the Utf8JsonReader with the JsonSerializer. We should make sure to meet user expectations here (otherwise, even debugging the issue would become difficult for them).

There is no reasonable workaround for the user. In fact, the JsonConverter<T> extension point is the primary workaround to change default serialization behavior, which increases the need for this fix since that isn't working as expected.

Regression

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

Testing

Added several permutations within the test including customer reported scenario. The fix for the bug is only a couple of lines and only affects the read/deserialize, but I wanted to make sure we add tests to exercise the various options for both read and write. We already have decent negative test cases (where the default options throw for invalid JSON), but were missing positive test cases (where custom options worked to make the reader more flexible).

The writer tests are only there for completeness and fill a gap from when the JsonSerializer APIs that accept a writer were originally added (see #38700 (comment)). Those tests pass in master and 3.1 already with no source changes.

Risk

Low given it is a very targeted fix with no regression. This fix only affects the deserialization behavior. One scenario where a customer could be impacted is if they accidentally relied on the default reader options to catch/throw for JSON that might contain comments/trailing commas, even when they were setting the options to be flexible. The fix for that would be to make sure the options match what you expect.

cc @ericstj

@ahsonkhan ahsonkhan added Servicing-consider Issue for next servicing release review area-System.Text.Json labels Feb 20, 2020
@ahsonkhan ahsonkhan added this to the 3.1.x milestone Feb 20, 2020
@ahsonkhan
Copy link
Author

cc @joperezr, @Anipik to help with packaging/assembly versioning

@joperezr
Copy link
Member

You need to include a couple of extra changes. First, you need to rev these two versions

<PackageVersion>4.7.1</PackageVersion>
<AssemblyVersion>4.0.1.1</AssemblyVersion>

into 4.7.2 and 4.0.1.2.

Then, you need to also make sure that the package is getting built, so go into here:

<!-- add specific builds / pkgproj's here to include in servicing builds -->
<Project Include="$(MSBuildThisFileDirectory)System.Runtime.CompilerServices.Unsafe\pkg\System.Runtime.CompilerServices.Unsafe.pkgproj">
<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
</Project>
<Project Include="$(MSBuildThisFileDirectory)System.IO.Pipelines\pkg\System.IO.Pipelines.pkgproj">
<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
</Project>

and add an extra Project there that looks like:

    <Project Include="$(MSBuildThisFileDirectory)System.Text.Json\pkg\System.Text.Json.pkgproj">
      <AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
    </Project>

That should take care of versioning and packaging.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 25, 2020
@danmoseley
Copy link
Member

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

@joperezr
Copy link
Member

Due to this comment #42870 (comment) please don't rev up assembly version as that would cause issues with serialization scenarios. cc: @ericstj

@ericstj
Copy link
Member

ericstj commented Feb 27, 2020

@joperezr we've already done that for this library in past updates. My comments were more geared towards some different patterns we can follow in 5.0. I don't think we should change our policy in 3.x.

@danmoseley danmoseley modified the milestones: 3.1.x, 3.1.4 Mar 19, 2020
@Anipik Anipik merged commit 3a5bc0a into dotnet:release/3.1 Mar 25, 2020
@ahsonkhan ahsonkhan deleted the PortOptionsFix branch April 22, 2020 03:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants