Add ReadOnlySpan<char> overloads to JsonSerializer.Deserialize #41957

huoyaoyuan · 2020-09-08T08:53:28Z

Closes #1235 .

I'm not sure how to test it. The existing tests are focused on data instead of overloads.

Contribution pain: I have never been satisfied by build /t:GenerateReferenceSource these days, for different reasons. For System.Text.Json, the reason is that attribute on generic parameter is not supported.

Dotnet-GitSync-Bot · 2020-09-08T08:53:32Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

layomia · 2020-09-13T23:34:49Z

Contribution pain: I have never been satisfied by build /t:GenerateReferenceSource these days, for different reasons. For System.Text.Json, the reason is that attribute on generic parameter is not supported.

cc @safern on generic type parameters not being supported by /t:GenerateReferenceSource. Are we tracking this?

Opened #42186 to re-add linker attributes omitted from generic type params on JsonSerializer methods.

@huoyaoyuan what other pain points do you have with /t:GenerateReferenceSource?

layomia · 2020-09-13T23:46:54Z

I'm not sure how to test it. The existing tests are focused on data instead of overloads.

The way to test it is to add a new DeserializerWrapper, e.g:

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

Lines 23 to 34 in d1f48a2

    
           private class StringDeserializerWrapper : DeserializationWrapper 
        
           { 
        
               protected internal override Task<T> DeserializeWrapper<T>(string json, JsonSerializerOptions options = null) 
        
               { 
        
                   return Task.FromResult(JsonSerializer.Deserialize<T>(json, options)); 
        
               } 
        
               protected internal override Task<object> DeserializeWrapper(string json, Type type, JsonSerializerOptions options = null) 
        
               { 
        
                   return Task.FromResult(JsonSerializer.Deserialize(json, type, options)); 
        
               } 
        
           }

Use the new wrapper wherever the existing ones are used.

We also need to add linker trimming tests for the new overloads. See the exisiting ones here. Each overload should map to a new .cs file. To run the tests run the following from the repo's root directory:

build.cmd -s libs.tests -c release /p:TestAssemblies=false /p:TestTrimming=true

layomia

Thanks for the PR. I just have a few comments and a pointer on the tests.

src/libraries/System.Text.Json/ref/System.Text.Json.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

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

safern · 2020-09-14T17:37:22Z

cc @safern on generic type parameters not being supported by /t:GenerateReferenceSource. Are we tracking this?

We have an issue for APICompat, since it is assigned to me I will look at GenAPI as well dotnet/arcade#5925

ahsonkhan · 2020-09-15T18:49:40Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

+        /// <typeparamref name="TValue"/> is not compatible with the JSON,
+        /// or when there is remaining data in the Stream.


This overload doesn't take a stream. Doc comment needs to be updated.

I recommend using the content from the official msft docs that have gone through content review to seed the new API docs rather than copying from an overload to avoid slight discrepancies.
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.deserialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Deserialize__1_System_String_System_Text_Json_JsonSerializerOptions_

https://github.com/dotnet/dotnet-api-docs/blob/80088021b2151fd5af3cda6e759ec5e548c372c0/xml/System.Text.Json/JsonSerializer.xml#L281-L348

ahsonkhan · 2020-09-15T18:51:10Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

+        /// the <paramref name="returnType"/> is not compatible with the JSON,
+        /// or when there is remaining data in the Stream.


Same doc feedback here:
https://github.com/dotnet/dotnet-api-docs/blob/80088021b2151fd5af3cda6e759ec5e548c372c0/xml/System.Text.Json/JsonSerializer.xml#L89-L149

layomia

Suggested doc changes for consistency with what is in https://docs.microsoft.com/dotnet/api/system.text.json.jsonserializer.deserialize?view=netcore-3.1.

Linker trimming tests as described in #41957 (comment) are needed to merge this PR.

layomia · 2020-09-17T00:22:24Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

+        {
+            if (json == null)
+            {
+                throw new ArgumentNullException(nameof(json));


We shouldn't throw ArgumentNullException for a value type. Instead we should let the reader throw a JsonException similar to the behavior for an empty ReadOnlySpan<byte>:

JsonSerializer.Deserialize(default(ReadOnlySpan<byte>), typeof(int)); Unhandled exception. System.Text.Json.JsonException: The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. Path: $ | LineNumber: 0 | BytePositionInLine: 0.

This applies to the other new overload as well. We should add a test for this scenario for both overloads and assert on the content of the exception. The path information should be present in the exception string "$ | LineNumber: 0 | BytePositionInLine: 0".

This Comment suggests a special test for default span. Otherwise, default span will be treated same as empty span.
What's the final decision on this?

Otherwise, default span will be treated same as empty span.

Default and empty spans should be treated the same. Adding a test passing in default span is good to have, but the null check within the source implementation isn't necessary and should be removed.

In the implementation of Deserialize, a default/empty span should be allowed to be passed to the Utf8JsonReader, and it will validate that empty JSON is invalid and throw JsonException. That is the desired behavior.

layomia · 2020-09-17T00:22:57Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

+
+            if (returnType == null)
+            {
+                throw new ArgumentNullException(nameof(returnType));


Need test exercising this line.

Also, need to add to the doc comment that this method can throw ArgumentNullException in the list of Exceptions.

Similar to this:
https://github.com/dotnet/dotnet-api-docs/blob/fcf78e35bc4eca5734d3f248670b1ade805e2ed0/xml/System.Text.Json/JsonSerializer.xml#L75-L76

<exception cref="T:System.ArgumentNullException"> <paramref name="returnType" /> is <see langword="null" />.</exception>

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

Co-authored-by: Layomi Akinrinade <layomia@gmail.com>

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

ahsonkhan

Some docs feedback. Looks good otherwise.

ahsonkhan · 2020-09-22T20:04:46Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs

+        /// There is remaining data in the span beyond a single JSON value.</exception>
+        /// <exception cref="NotSupportedException">
+        /// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
+        /// for <typeparamref name="TValue"/> or its serializable members.


@layomia, @tdykstra, can we please add or its serializable members to the docs to all such methods, in the published api-docs?

It doesn't show that currently. For example:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.deserialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Deserialize_System_String_System_Type_System_Text_Json_JsonSerializerOptions_

https://github.com/dotnet/dotnet-api-docs/blob/fcf78e35bc4eca5734d3f248670b1ade805e2ed0/xml/System.Text.Json/JsonSerializer.xml#L86

Co-authored-by: Ahson Khan <ahkha@microsoft.com>

ahsonkhan · 2020-10-01T05:34:19Z

@layomia - anything else left to do for this PR or is it ready for merging?

layomia

LGTM - will address linker tests for the new overload in a follow up.

huoyaoyuan added 2 commits September 8, 2020 16:07

Add char span overloads to JsonSerializer.Deserialize.

74f885d

Add new methods to ref source.

66da760

huoyaoyuan requested review from jozkee, layomia and steveharter as code owners September 8, 2020 08:53

Dotnet-GitSync-Bot added area-System.Text.Json new-api-needs-documentation labels Sep 8, 2020

Fix ambiguous call in test.

d1f48a2

layomia reviewed Sep 13, 2020

View reviewed changes

layomia added this to the 6.0.0 milestone Sep 13, 2020

layomia mentioned this pull request Sep 14, 2020

Re-add linker attributes to generic type params of JsonSerializer methods #42186

Merged

Merge branch 'master' into deserialize-char-span

bad8305

ahsonkhan reviewed Sep 15, 2020

View reviewed changes

huoyaoyuan added 3 commits September 16, 2020 16:17

Fix documentation issue.

5ba6424

Guard and test for empty span.

317a965

Add span in deserialization test wrapper.

e00a471

layomia reviewed Sep 17, 2020

View reviewed changes

huoyaoyuan and others added 2 commits September 19, 2020 15:12

Apply doc suggestions

45d1705

Co-authored-by: Layomi Akinrinade <layomia@gmail.com>

Treat default span as empty, and cover returnType in test.

fe9673a

ahsonkhan reviewed Sep 22, 2020

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs Outdated Show resolved Hide resolved

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs Outdated Show resolved Hide resolved

ahsonkhan approved these changes Sep 22, 2020

View reviewed changes

ahsonkhan reviewed Sep 22, 2020

View reviewed changes

huoyaoyuan and others added 2 commits September 23, 2020 16:13

Apply doc suggestions

f234018

Co-authored-by: Ahson Khan <ahkha@microsoft.com>

Add exception doc for null returnType.

faaea17

ahsonkhan approved these changes Sep 23, 2020

View reviewed changes

layomia approved these changes Oct 14, 2020

View reviewed changes

layomia merged commit 7ca6d85 into dotnet:master Oct 14, 2020

huoyaoyuan deleted the deserialization-char-span branch October 14, 2020 15:46

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

		/// <typeparamref name="TValue"/> is not compatible with the JSON,
		/// or when there is remaining data in the Stream.

		/// the <paramref name="returnType"/> is not compatible with the JSON,
		/// or when there is remaining data in the Stream.

Add ReadOnlySpan<char> overloads to JsonSerializer.Deserialize #41957

Add ReadOnlySpan<char> overloads to JsonSerializer.Deserialize #41957

Uh oh!

Conversation

huoyaoyuan commented Sep 8, 2020

Uh oh!

Dotnet-GitSync-Bot commented Sep 8, 2020

Uh oh!

layomia commented Sep 13, 2020

Uh oh!

layomia commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

safern commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Oct 1, 2020

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

layomia commented Sep 13, 2020 •

edited

Loading