Set values of array and immutable collection properties directly #42420

layomia · 2019-11-06T23:42:37Z

When deserializing, the serializer is throwing a NotSupportedException whenever we see an array or immutable collection (non-dictionary) property which isn't initally null. See https://dotnetfiddle.net/hdhEDG.

Arrays and immutable collections are deserialized using internal converters that create new instances populated the JSON array.
This issue doesn't apply to dictionaries, or ClassType.Enumerable types where we don't use converters:

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

Lines 107 to 112 in f9564cd

    
           else if (state.Current.IsProcessingProperty(ClassType.Enumerable)) 
        
           { 
        
               // We added the items to the list already. 
        
               state.Current.EndProperty(); 
        
               return false; 
        
           }

.

layomia · 2019-11-06T23:45:13Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs

-                    if (currentEnumerable == null ||
-                        // ImmutableArray<T> is a struct, so default value won't be null.
-                        jsonPropertyInfo.IsImmutableArray)
+                    if (currentEnumerable == null)


ImmutableArray<T> will no longer come down this path.

Can you please explain why?

Because it's immutable, we use a converter then we set the property directly as highlighted in https://github.com/dotnet/corefx/pull/42420/files#r345507426.

layomia · 2019-11-06T23:49:41Z

src/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

-                if (!jsonPropertyInfo.IsPropertyPolicy &&
-                    !state.Current.JsonPropertyInfo.IsImmutableArray)
+                // Clear the value if present to ensure we don't confuse TempEnumerableValues with the collection.
+                if (!jsonPropertyInfo.IsPropertyPolicy && jsonPropertyInfo.CanBeNull)


We shouldn't attempt to set any value-type enumerable to null, not just ImmutableArray. Same below.

Given the intention to set the collection to null, there might be an issue with this check since value-type collections are excluded and this isn't accounted for anywhere. I'll dig into this as part of https://github.com/dotnet/corefx/issues/42399.

I assume we want to throw when we see a null literal in a JSON array and the corresponding CLR element type is a value type. Wouldn't we just throw JsonException if CanBeNull=true here?

I assume we want to throw when we see a null literal in a JSON array and the corresponding CLR element type is a value type.

Yes, that's what we should (and currently) do.

In this area of the code, we are attempting to clear the collection property by setting it to null, so that we don't get confused in our null checks to state.Current.Temp[Dictionary|Enumerable]Values (are we using a collection converter?) later on. The jsonPropertyInfo.CanBeNull clause of this check guards against attempting to set a value type collection (e.g. ImmutableArray which is a struct) to null which I believe will give an InvalidCastException.
This means that we don't clear value type collections (these collections seem edge-casey and one-off in the BCL - I only know of ImmutableArray).

With the inital comment, I was noting that this may be a bug since the intention was to clear all collections, and that I plan to look into this in a future PR for https://github.com/dotnet/corefx/issues/42399. Maybe we don't actually need to clear the collection here and should just make sure our state.Current.Temp[Dictionary|Enumerable]Values checks are correct/vigilant without this "clearing" dependency.

The issue for https://github.com/dotnet/corefx/issues/42399 is that we should throw JsonException when we see null where we expect a JSON array for a value type collection, e.g. here.

corresponding CLR element type is a value type.

I would imagine this shouldn't be true if it's a nullable value type (int?, DateTime?, etc.). What's the behavior for that case?

What's the behavior for that case?

Those work as expected. We execute tests for this via:

corefx/src/System.Text.Json/tests/Serialization/TestClasses.SimpleTestClassWithNullables.cs

Line 10 in ac02476

public abstract class SimpleBaseClassWithNullables

I was thinking about nullable value types as the T of the collections like immutable array, regular array. So DateTime?[] or ImmutableArray<DateTime?>. Do these work as expected too?

As an aside, looks like nullable ImmutableArray breaks (https://dotnetfiddle.net/14qZfX):

Console.WriteLine(JsonSerializer.Deserialize<ImmutableArray<DateTime>?>("null") == null);

Unhandled exception. System.ArgumentNullException: Value cannot be null. at System.RuntimeType.MakeGenericType(Type[] instantiation)

I was thinking about nullable value types as the T of the collections like immutable array, regular array. So DateTime?[] or `ImmutableArray<DateTime?>. Do these work as expected too?

Yes we have tests for these:

corefx/src/System.Text.Json/tests/Serialization/NullableTests.cs

Line 15 in 6878693

public static void DictionaryWithNullableValue()

corefx/src/System.Text.Json/tests/Serialization/NullableTests.cs

Line 198 in 6878693

public static void EnumerableWithNullableValue()

Some for array of T?:

corefx/src/System.Text.Json/tests/Serialization/TestClasses.SimpleTestClassWithNullables.cs

Lines 30 to 47 in 6878693

public short?[] MyInt16Array { get; set; }

public int?[] MyInt32Array { get; set; }

public long?[] MyInt64Array { get; set; }

public ushort?[] MyUInt16Array { get; set; }

public uint?[] MyUInt32Array { get; set; }

public ulong?[] MyUInt64Array { get; set; }

public byte?[] MyByteArray { get; set; }

public sbyte?[] MySByteArray { get; set; }

public char?[] MyCharArray { get; set; }

public decimal?[] MyDecimalArray { get; set; }

public bool?[] MyBooleanTrueArray { get; set; }

public bool?[] MyBooleanFalseArray { get; set; }

public float?[] MySingleArray { get; set; }

public double?[] MyDoubleArray { get; set; }

public DateTime?[] MyDateTimeArray { get; set; }

public DateTimeOffset?[] MyDateTimeOffsetArray { get; set; }

public Guid?[] MyGuidArray { get; set; }

public SampleEnum?[] MyEnumArray { get; set; }

As an aside, looks like nullable ImmutableArray breaks (https://dotnetfiddle.net/14qZfX)

Yes, this is an extension of the known issue with assigning JSON null to value-type collections: https://github.com/dotnet/corefx/issues/42399#issuecomment-553190419.

ericstj

LGTM but you should get either @steveharter or @ahsonkhan to sign off.

src/System.Text.Json/tests/Serialization/Array.ReadTests.cs

ahsonkhan · 2019-11-12T23:47:00Z

...xt.Json/src/System/Text/Json/Serialization/Converters/DefaultImmutableEnumerableConverter.cs

@@ -102,19 +102,16 @@ public static void RegisterImmutableCollection(Type immutableCollectionType, Typ
            options.TryAddCreateRangeDelegate(delegateKey, createRangeDelegate);
        }

-        public static bool IsImmutableEnumerable(Type type, out bool IsImmutableArray)


Why did we have to special case ImmutableArray initially?

There are places in the code where we perform null checks on collections. Since value-types can't be null and the only "known" value type collection was ImmutableArray, it was special cased. The changes in this PR use the more appropriate and general JsonPropertyInfo.CanBeNull property which makes sure "unknown" value collection types are accounted for as well.

ahsonkhan · 2019-11-12T23:52:07Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs

@@ -132,7 +140,7 @@ private static void HandleStartArray(JsonSerializerOptions options, ref ReadStac
                state.Pop();
            }

-            ApplyObjectToEnumerable(value, ref state);
+            ApplyObjectToEnumerable(value, ref state, setPropertyDirectly);


With this change, if state.Current.IsProcessingProperty(ClassType.Enumerable) is true, i.e. setPropertyDirectly is true, will we jump to this code path in ApplyObjectToEnumerable?

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

Lines 230 to 234 in ac02476

else

{

Debug.Assert(state.Current.JsonPropertyInfo != null);

state.Current.JsonPropertyInfo.SetValueAsObject(state.Current.ReturnValue, value);

}

Why do we need setPropertyDirectly in ApplyObjectToEnumerable but not in ApplyValueToEnumerable?

With this change, if state.Current.IsProcessingProperty(ClassType.Enumerable) is true, i.e. setPropertyDirectly is true, will we jump to this code path in ApplyObjectToEnumerable?

Yes.

Why do we need setPropertyDirectly in ApplyObjectToEnumerable but not in ApplyValueToEnumerable?

We probably need such semantics in ApplyValueToEnumerable to fix issues such as https://github.com/dotnet/corefx/issues/41598 and https://github.com/dotnet/corefx/issues/42399.
Also, the two methods both add elements to collections, and set populated collections to class properties. There should be a separation of these responsibilities, and I'll dig deeper into this to fix the afforementioned issues.

ahsonkhan · 2019-11-12T23:53:57Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs

-                    if (currentEnumerable == null ||
-                        // ImmutableArray<T> is a struct, so default value won't be null.
-                        jsonPropertyInfo.IsImmutableArray)
+                    if (currentEnumerable == null)


Can you please explain why?

layomia added the area-System.Text.Json label Nov 6, 2019

layomia added this to the 5.0 milestone Nov 6, 2019

layomia requested review from ahsonkhan, steveharter and jozkee November 6, 2019 23:42

layomia self-assigned this Nov 6, 2019

Set values of array and immutable collection properties directly

0bfa3eb

layomia force-pushed the collection branch from 776a145 to 0bfa3eb Compare November 6, 2019 23:43

layomia commented Nov 6, 2019

View reviewed changes

ericstj approved these changes Nov 12, 2019

View reviewed changes

ahsonkhan reviewed Nov 13, 2019

View reviewed changes

Address review feedback

4f8ce77

ahsonkhan approved these changes Nov 13, 2019

View reviewed changes

layomia merged commit a528006 into dotnet:master Nov 13, 2019

ahsonkhan mentioned this pull request Nov 13, 2019

Large json serialization/deserialization tests #39668

Merged

jozkee mentioned this pull request Feb 7, 2020

JsonSerializer throws InvalidCastException dotnet/runtime#2387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set values of array and immutable collection properties directly #42420

Set values of array and immutable collection properties directly #42420

layomia commented Nov 6, 2019 •

edited

Loading

layomia Nov 6, 2019

ahsonkhan Nov 12, 2019

layomia Nov 13, 2019

layomia Nov 6, 2019

steveharter Nov 12, 2019

layomia Nov 12, 2019 •

edited

Loading

ahsonkhan Nov 12, 2019 •

edited

Loading

layomia Nov 12, 2019

ahsonkhan Nov 13, 2019 •

edited

Loading

layomia Nov 13, 2019 •

edited

Loading

ericstj left a comment

ahsonkhan Nov 12, 2019

layomia Nov 13, 2019 •

edited

Loading

ahsonkhan Nov 12, 2019

layomia Nov 13, 2019 •

edited

Loading

ahsonkhan Nov 12, 2019

	else if (state.Current.IsProcessingProperty(ClassType.Enumerable))
	{
	// We added the items to the list already.
	state.Current.EndProperty();
	return false;
	}

	public short?[] MyInt16Array { get; set; }
	public int?[] MyInt32Array { get; set; }
	public long?[] MyInt64Array { get; set; }
	public ushort?[] MyUInt16Array { get; set; }
	public uint?[] MyUInt32Array { get; set; }
	public ulong?[] MyUInt64Array { get; set; }
	public byte?[] MyByteArray { get; set; }
	public sbyte?[] MySByteArray { get; set; }
	public char?[] MyCharArray { get; set; }
	public decimal?[] MyDecimalArray { get; set; }
	public bool?[] MyBooleanTrueArray { get; set; }
	public bool?[] MyBooleanFalseArray { get; set; }
	public float?[] MySingleArray { get; set; }
	public double?[] MyDoubleArray { get; set; }
	public DateTime?[] MyDateTimeArray { get; set; }
	public DateTimeOffset?[] MyDateTimeOffsetArray { get; set; }
	public Guid?[] MyGuidArray { get; set; }
	public SampleEnum?[] MyEnumArray { get; set; }

	else
	{
	Debug.Assert(state.Current.JsonPropertyInfo != null);
	state.Current.JsonPropertyInfo.SetValueAsObject(state.Current.ReturnValue, value);
	}

Set values of array and immutable collection properties directly #42420

Set values of array and immutable collection properties directly #42420

Conversation

layomia commented Nov 6, 2019 • 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

layomia Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

layomia Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia commented Nov 6, 2019 •

edited

Loading

layomia Nov 12, 2019 •

edited

Loading

ahsonkhan Nov 12, 2019 •

edited

Loading

ahsonkhan Nov 13, 2019 •

edited

Loading

layomia Nov 13, 2019 •

edited

Loading

layomia Nov 13, 2019 •

edited

Loading

layomia Nov 13, 2019 •

edited

Loading