Fixed deserialization of null arrays for issue #37606 by gnovack · Pull Request #37616 · dotnet/corefx

gnovack · 2019-05-12T22:40:08Z

This fixes #37606 by checking for null before adding to state.Current.ReturnValue
Also added tests for deserializing null arrays.

dnfclas · 2019-05-12T22:40:23Z

All CLA requirements met.

MarcoRossignoli · 2019-05-13T10:17:04Z

+            {
+                int[] obj = JsonSerializer.Parse<int[]>("null");
+                Assert.Null(obj);
+            }


I'm not used to see scope style on codebase but I could wrong @danmosemsft

I'm not either, but I don't think we're against it either - particularly in test code if it makes things clearer.

In this case though arguably it's a "code smell" suggesting there should be 3 separate test methods.

I'm not a huge fan of it myself, but it looked like an existing pattern in the test classes for short, repetitive tests like these.

Yes it's only like this in tests and is there to avoid multiple tests and prevent these sections from accidently colliding on variable names. Sometimes it's used to verify a baseline condition before testing the primary case.

steveharter · 2019-05-13T15:54:03Z

                    state.Current.TempEnumerableValues.Add(value);
                }
-                else
+                else if (state.Current.ReturnValue != null)


It seems like this is the wrong code path since the value should never be added to a list.

Instead this code path should be the one returning null:

corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleNull.cs

Line 20 in f5ed5a8

if (state.Current.JsonPropertyInfo == null)

however it isn't getting into that condition since the JsonPropertyInfo is not null. It is possible to expand this line\condition to cover this issue it may be better to refactor this code to support this condition without special casing (for example by not having a null JsonPropertyInfo ever) - not sure until I see both ways.

I see what you mean. It probably should never reach ApplyObjectToEnumerable.
I'll try to see if there's a clean way to catch root-level nulls within HandleNull.

Side note: Looks like this issue encompasses everything of ClassType.Enumerable.
The following test throws null-ref as well:

IEnumerable<object> obj = JsonSerializer.Parse<IEnumerable<object>>("null");

Side note: Looks like this issue encompasses everything of ClassType.Enumerable.

@gnovack, while you are investigating this space, can you please add tests for similar cases like the one you mentioned? Similarly, Parse<T> where T is different types: value types (int, DateTime, etc.) and reference types (string, some POCO class, Dictionary<TKey, TValue>, and as you mentioned IEnumerable<T>).

ahsonkhan · 2019-05-14T04:13:02Z

        }

+        [Fact]
+        public static void RootArrayIsNull()


It might be good to add a test for an array within a class as well.

public class Foo { public int[] Array {get; set;} } Foo obj = JsonSerializer.Parse<Foo>("{null}");

The above test throws a JsonException because "{null}" isn't a valid JSON string, but I'd be happy to add this test if you'd like. It would probably belong here:

corefx/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs

Line 152 in d0d6a73

public static void ReadObjectFail()

As far as testing a valid JSON object that contains a null array, it looks like that's covered by the following, which deserializes an object with various types of properties set to null (including an array):

corefx/src/System.Text.Json/tests/Serialization/Null.ReadTests.cs

Line 33 in d0d6a73

public static void DefaultIgnoreNullValuesOnRead()

Ah yes, you are right. Good point! I meant testing with a json payload like {"Array": null}, which you rightfully pointed out, is covered.

I'd be happy to add this test if you'd like

That isn't necessary, at least it isn't worth resetting CI over :)
I am sure the underlying reader will catch this, though it would be nice to add it to:

corefx/src/System.Text.Json/tests/Utf8JsonReaderTests.cs

Line 3228 in d0d6a73

public static IEnumerable<object[]> InvalidJsonStrings

gnovack · 2019-05-15T02:15:10Z

-            // If we don't have a valid property, that means we read "null" for a root object so just return.
-            if (state.Current.JsonPropertyInfo == null)
+            // If null is read at the top level and the type is nullable, then the root is "null" so just return.
+            if (reader.CurrentDepth == 0 && (state.Current.JsonPropertyInfo == null || state.Current.JsonPropertyInfo.CanBeNull))


Since this code flow ultimately results in a return from ReadCore, it should only ever be triggered at the top level (reader.CurrentDepth == 0)
Second condition is to ensure that the type we're dealing with is nullable.

There is a helper called IsLastFrame on the stack frame that could be used instead of CurrentDepth but otherwise looks correct.

IsLastFrame behaves slightly different from reader.CurrentDepth.
For example, when the reader encounters null in the following json { "MyProperty": null }, reader.CurrentDepth is 1, but IsLastFrame is true.

IsLastFrame appears to only update when a new frame is pushed onto the stack, which happens while parsing sub-objects/sub-arrays within an object.

ahsonkhan

~~Other than leftover comment to add a test for "array within a class" (i.e. the following JSON: "{null}"),~~
LGTM. Thanks for contributing.

Edit: ignore test comment

…otnet/corefx#37616) * Fixed deserialization of null root arrays. * Used reader.CurrentDepth to catch root level nulls. Commit migrated from dotnet/corefx@026fa6a

Fixed deserialization of null root arrays.

02eaf52

danmoseley requested a review from steveharter May 13, 2019 01:36

MarcoRossignoli reviewed May 13, 2019

View reviewed changes

steveharter reviewed May 13, 2019

View reviewed changes

ahsonkhan added the area-System.Text.Json label May 14, 2019

ahsonkhan added this to the 3.0 milestone May 14, 2019

ahsonkhan reviewed May 14, 2019

View reviewed changes

Used reader.CurrentDepth to catch root level nulls.

f4b22b9

gnovack commented May 15, 2019

View reviewed changes

steveharter approved these changes May 15, 2019

View reviewed changes

ahsonkhan approved these changes May 16, 2019

View reviewed changes

maryamariyan approved these changes May 16, 2019

View reviewed changes

ahsonkhan merged commit 026fa6a into dotnet:master May 16, 2019

Conversation

gnovack commented May 12, 2019

Uh oh!

dnfclas commented May 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoRossignoli May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

steveharter May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ahsonkhan May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ahsonkhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnfclas commented May 12, 2019 •

edited

Loading

MarcoRossignoli May 13, 2019 •

edited

Loading

steveharter May 13, 2019 •

edited

Loading

ahsonkhan May 16, 2019 •

edited

Loading

ahsonkhan left a comment •

edited

Loading