Additional tests for System.Text.Json #32705

alanisaac · 2020-02-23T04:14:25Z

Adding additional tests and removed unreachable code in order to improve code coverage for System.Text.Json, as suggested in #32341.

dnfclas · 2020-02-23T04:14:38Z

All CLA requirements met.

danmoseley · 2020-02-23T04:59:25Z

Hi thanks for the contribution! Btw we don't really use "draft" PRs here. Is this ready for review?

alanisaac · 2020-02-23T13:54:06Z

@danmosemsft

Thanks for letting me know, I've marked it as ready for review.

alanisaac · 2020-02-23T16:37:51Z

I added some additional changes. I'll leave this PR alone from here, and continue working on additional coverage improvements in another PR.

Note 1

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Lines 291 to 293 in fd181c0

    
           JsonDictionaryConverter<T> dictionaryConverter = (JsonDictionaryConverter<T>)this; 
        
           if (ClassType == ClassType.Value)

In the previous line, we cast the current instance to a JsonDictionaryConverter<T>, whose ClassType in practice is always ClassType.Dictionary. Therefore, this conditional branch should be able to be removed safely. I think this tracks with the purpose of this method of writing data for [JsonExtensionData], which to my understanding always needs to be a Dictionary<> anyway.

However, to make sure this is safely enforced, I thought it would be good to explicitly mark JsonDictionaryConverter<T>.ClassType sealed as well, hence this change.

Note 2

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs

Lines 68 to 71 in fd181c0

    
           if (inputType == null) 
        
           { 
        
               inputType = value.GetType(); 
        
           }

I'm fairly confident this is unreachable. There are two calling methods:

SerializeAsync(Stream, object?, Type, JsonSerializerOptions?, CancellationToken) already has a defensive check that throws if inputType is null.
SerializeAsync<TValue>(Stream, TValue, JsonSerializerOptions?, CancellationToken cancellationToken) determines inputType from typeof(TValue). I tried to think through if there was a way that typeof(TValue) could be null through reflection, but I couldn't come up with anything -- MakeGenericMethod at least requires a non-null type parameter.

Note 3

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs

Lines 21 to 24 in 59dde05

    
           public static Task SerializeAsync<TValue>(Stream utf8Json, TValue value, JsonSerializerOptions options = null, CancellationToken cancellationToken = default) 
        
           { 
        
               return WriteAsyncCore(utf8Json, value, typeof(TValue), options, cancellationToken); 
        
           }

That call chain throws a NRE if Stream utf8Json is null, but the non-generic version checks that argument and throws an ArgumentNullException. For comparison, both the generic and non-generic versions of DeserializeAsync on the read side have a null check, for example:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs

Lines 30 to 39 in 59dde05

    
           public static ValueTask<TValue> DeserializeAsync<TValue>( 
        
               Stream utf8Json, 
        
               JsonSerializerOptions options = null, 
        
               CancellationToken cancellationToken = default) 
        
           { 
        
               if (utf8Json == null) 
        
                   throw new ArgumentNullException(nameof(utf8Json)); 
        
               return ReadAsync<TValue>(utf8Json, typeof(TValue), options, cancellationToken); 
        
           }

ahsonkhan · 2020-02-25T20:37:46Z

...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs

@@ -21,6 +21,9 @@ public static partial class JsonSerializer
        /// <param name="cancellationToken">The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the write operation.</param>
        public static Task SerializeAsync<TValue>(Stream utf8Json, TValue value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
        {
+            if (utf8Json == null)


Nice catch. We should update the docs with the new exceptions:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serializeasync?view=netcore-3.1#System_Text_Json_JsonSerializer_SerializeAsync_System_IO_Stream_System_Object_System_Type_System_Text_Json_JsonSerializerOptions_System_Threading_CancellationToken_

If you get the chance, please add the exceptions this (and other SerializeAsync overloads) throw, here (if you are up for it, this would need a PR in the docs repo):
https://github.com/dotnet/dotnet-api-docs/blob/1e6b2e6052185c6744054acc10cd06e435a4fb8b/xml/System.Text.Json/JsonSerializer.xml#L546-L563

See other overloads that have exception docs as an example:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Serialize__1_System_Text_Json_Utf8JsonWriter___0_System_Text_Json_JsonSerializerOptions_

cc @carlossanlop

Will do, thanks for the pointers to the docs code and example.

Created issue dotnet/dotnet-api-docs#3961

ahsonkhan · 2020-02-25T20:39:54Z

src/libraries/System.Text.Json/tests/Serialization/Stream.ReadTests.cs

        {
            await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync<string>((Stream)null));
+            await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync((Stream)null, (Type)null));


While we are doing this, let's add all permutations. For example: have the stream parameter be null, but the type parameter be non-null.

fixed in 7a6e0cb

ahsonkhan · 2020-02-25T20:44:45Z

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs

+
+            ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(dataUtf8);
+
+            var json = new Utf8JsonReader(sequence, isFinalBlock: false, state: default);


nit: Consider looping through the entire sequence and creating slices of arbitrary lengths (and re-entering the reader with "partial data"). See other "single value" tests like: TestSingleStringsMultiSegment

Tried something out with 823f147, let me know if that doesn't cover what you were thinking.

ahsonkhan · 2020-02-25T20:49:03Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -290,45 +290,27 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json
            bool success;
            JsonDictionaryConverter<T> dictionaryConverter = (JsonDictionaryConverter<T>)this;

-            if (ClassType == ClassType.Value)


I am not sure if it is OK to remove this (it's possible that it is OK to do, but is surprising to me). What happens when the user defines and register their own JsonConverter<T> for dictionaries?

@steveharter, @layomia - please review this when you get a chance. I suspect this branch was needed for something (which means we need to provide the test case).

I touched on this in Note 1 in the above comments, but I'm not sure it's possible. The line immediately above this one casts the current instance to a JsonDictionaryConverter<T> which is marked internal, so my assumption was users wouldn't be able to inherit it (unless there's something I'm not thinking of or aware of?)

I can certainly leave this in there if that's the call, but I think then testing and getting coverage on these lines would be difficult without introducing an InternalsVisibleTo relationship (or similar internals-testing-pattern) between the src and test project. That or adding an inherited class in the src project for the purposes of testing.

There is a bug with writing extension data if there is a custom converter for the extension property: #32903. This is the reason why the L291 is not problematic (InvalidCastException) - we throw an NRE up stack.

We'll likely need this branch when we fix that bug, so for this PR we should revert this change.

reverted in 256de92

layomia · 2020-02-28T20:00:46Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonDictionaryConverter.cs

@@ -9,7 +9,7 @@ namespace System.Text.Json.Serialization
    /// </summary>
    internal abstract class JsonDictionaryConverter<T> : JsonResumableConverter<T>
    {
-        internal override ClassType ClassType => ClassType.Dictionary;
+        internal sealed override ClassType ClassType => ClassType.Dictionary;


We should seal the overrides in JsonCollectionConverter and JsonObjectConverter as well.

fixed in 02ab96b

layomia · 2020-02-28T20:11:23Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -290,45 +290,27 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json
            bool success;
            JsonDictionaryConverter<T> dictionaryConverter = (JsonDictionaryConverter<T>)this;

-            if (ClassType == ClassType.Value)


There is a bug with writing extension data if there is a custom converter for the extension property: #32903. This is the reason why the L291 is not problematic (InvalidCastException) - we throw an NRE up stack.

We'll likely need this branch when we fix that bug, so for this PR we should revert this change.

…chable code" This reverts commit 1e8e33c.

layomia

Thanks, @alanisaac!

ahsonkhan · 2020-03-02T22:50:08Z

src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs

+                {
+                    long tokenLength = jsonReader.HasValueSequence ? jsonReader.ValueSequence.Length : jsonReader.ValueSpan.Length;
+                    Assert.Equal(expectedTokenLength, tokenLength);
+                    Assert.Equal(expectedValue, jsonReader.GetDateTime());


nit: We missed the asserts for TryGetDateTime and TryGetDateTimeOffset APIs.

alanisaac added 2 commits February 22, 2020 21:21

added multi-segment DateTime tests

4b5f5c2

added missing null argument test

a4c5847

Dotnet-GitSync-Bot added the area-System.Text.Json label Feb 23, 2020

alanisaac mentioned this pull request Feb 23, 2020

Help get System.Text.Json test coverage to 100% (or close to it) #32341

Open

23 tasks

alanisaac marked this pull request as ready for review February 23, 2020 13:52

alanisaac requested review from ahsonkhan, jozkee, layomia and steveharter as code owners February 23, 2020 13:52

alanisaac added 3 commits February 23, 2020 10:47

added null argument tests and missing check for SerializeAsync

a8374d9

removed unreachable defensive code block

8090c69

sealed class type for dictionary converters and removed unreachable code

1e8e33c

ahsonkhan reviewed Feb 25, 2020

View reviewed changes

ahsonkhan added this to the 5.0 milestone Feb 25, 2020

ahsonkhan added the test-enhancement Improvements of test source code label Feb 25, 2020

ahsonkhan reviewed Feb 25, 2020

View reviewed changes

alanisaac added 2 commits February 25, 2020 21:30

added additional test permutation

7a6e0cb

added slices to DateTime conversion tests

823f147

layomia reviewed Feb 28, 2020

View reviewed changes

alanisaac added 2 commits February 28, 2020 20:52

Revert "sealed class type for dictionary converters and removed unrea…

256de92

…chable code" This reverts commit 1e8e33c.

sealed class type for internal json converters

02ab96b

layomia assigned alanisaac Feb 29, 2020

layomia approved these changes Feb 29, 2020

View reviewed changes

layomia merged commit 11b37df into dotnet:master Feb 29, 2020

alanisaac mentioned this pull request Feb 29, 2020

Add ArgumentNullException docs to JsonSerializer.SerializeAsync overloads dotnet/dotnet-api-docs#3961

Closed

alanisaac deleted the system-text-json-coverage branch February 29, 2020 19:53

alanisaac mentioned this pull request Feb 29, 2020

Added ArgumentNullException documentation to System.Text.Json.JsonSerializer.SerializeAsync dotnet/dotnet-api-docs#3962

Merged

jaredpar mentioned this pull request Mar 2, 2020

Fast patching NuGet restore issue dotnet/arcade#4957

Closed

ahsonkhan reviewed Mar 2, 2020

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional tests for System.Text.Json #32705

Additional tests for System.Text.Json #32705

alanisaac commented Feb 23, 2020 •

edited

Loading

dnfclas commented Feb 23, 2020 •

edited

Loading

danmoseley commented Feb 23, 2020

alanisaac commented Feb 23, 2020

alanisaac commented Feb 23, 2020 •

edited

Loading

ahsonkhan Feb 25, 2020 •

edited

Loading

alanisaac Feb 26, 2020

alanisaac Feb 29, 2020

ahsonkhan Feb 25, 2020

alanisaac Feb 26, 2020

ahsonkhan Feb 25, 2020

alanisaac Feb 26, 2020 •

edited

Loading

ahsonkhan Feb 25, 2020

alanisaac Feb 26, 2020

layomia Feb 28, 2020

alanisaac Feb 29, 2020

layomia Feb 28, 2020

alanisaac Feb 29, 2020

layomia Feb 28, 2020

layomia left a comment

ahsonkhan Mar 2, 2020


		ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(dataUtf8);

		var json = new Utf8JsonReader(sequence, isFinalBlock: false, state: default);

Additional tests for System.Text.Json #32705

Additional tests for System.Text.Json #32705

Conversation

alanisaac commented Feb 23, 2020 • edited Loading

dnfclas commented Feb 23, 2020 • edited Loading

danmoseley commented Feb 23, 2020

alanisaac commented Feb 23, 2020

alanisaac commented Feb 23, 2020 • edited Loading

Note 1

Note 2

Note 3

ahsonkhan Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanisaac Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanisaac commented Feb 23, 2020 •

edited

Loading

dnfclas commented Feb 23, 2020 •

edited

Loading

alanisaac commented Feb 23, 2020 •

edited

Loading

ahsonkhan Feb 25, 2020 •

edited

Loading

alanisaac Feb 26, 2020 •

edited

Loading