Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fixed deserialization of null arrays for issue #37606 #37616

Merged
ahsonkhan merged 2 commits into from
May 16, 2019
Merged

Fixed deserialization of null arrays for issue #37606 #37616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02eaf52
Fixed deserialization of null root arrays.
gnovack May 11, 2019
f4b22b9
Used reader.CurrentDepth to catch root level nulls.
gnovack May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleNull.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ private static bool HandleNull(ref Utf8JsonReader reader, ref ReadStack state, J
return false;
}

// 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))
Copy link
Copy Markdown
Contributor Author

@gnovack gnovack May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@steveharter steveharter May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@gnovack gnovack May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
Debug.Assert(state.IsLastFrame);
Debug.Assert(state.Current.ReturnValue == null);
Expand Down
38 changes: 37 additions & 1 deletion src/System.Text.Json/tests/Serialization/Null.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using Xunit;

namespace System.Text.Json.Serialization.Tests
Expand All @@ -17,7 +18,7 @@ public static void ClassWithNullProperty()

[Fact]
public static void RootObjectIsNull()
{
{
{
TestClassWithNull obj = JsonSerializer.Parse<TestClassWithNull>("null");
Assert.Null(obj);
Expand All @@ -27,6 +28,41 @@ public static void RootObjectIsNull()
object obj = JsonSerializer.Parse<object>("null");
Assert.Null(obj);
}

{
string obj = JsonSerializer.Parse<string>("null");
Assert.Null(obj);
}

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

{
Dictionary<string, object> obj = JsonSerializer.Parse<Dictionary<string, object>>("null");
Assert.Null(obj);
}

}

[Fact]
public static void RootArrayIsNull()
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}");

Copy link
Copy Markdown
Contributor Author

@gnovack gnovack May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
{
int[] obj = JsonSerializer.Parse<int[]>("null");
Assert.Null(obj);
}
Copy link
Copy Markdown
Member

@MarcoRossignoli MarcoRossignoli May 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@danmoseley danmoseley May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@gnovack gnovack May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@steveharter steveharter May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

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

[Fact]
Expand Down