JsonSerializer throwing "System.NotSupportedException : Collection was of a fixed size" for some array/collection parsing

[buyaa-n]

Before adding SimpleTestClass MySampleTestClass { get; set; } property to the SimpleTestStruct struct and json test was running successfully, after adding object type JsonSerializer cannot parse it anymore
Update: The original test failure is fixed with recent update, but the below case is still failing so updated the test case
Exception: System.NotSupportedException : Collection was of a fixed size.

[Fact]
public void TestMethod1()
{
    var state = new AppState();
    string serialized = "{\"Values\":[1,2,3]}";
    object deserializedState = System.Text.Json.Serialization.JsonSerializer.Parse(serialized, typeof(AppState));
}

public class AppState
{
    public int[] Values { get; set; }
    public AppState()
    {
        Values = Array.Empty<int>();
    }
}

System.NotSupportedException : Collection was of a fixed size.
Stack Trace:
/_/src/System.Private.CoreLib/src/System/Array.CoreCLR.cs(482,0): at System.SZArrayHelper.Add[T](T value)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.HandleArray.cs(264,0): 
at System.Text.Json.JsonSerializer.ApplyValueToEnumerable[TProperty](TProperty& value, ReadStack& state, Utf8JsonReader& reader)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoNotNullable.cs(67,0) : 
at System.Text.Json.JsonPropertyInfoNotNullable`3.ReadEnumerable(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonPropertyInfoNotNullable.cs(25,0)  : 
at System.Text.Json.JsonPropertyInfoNotNullable`3.Read(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.HandleValue.cs(28,0): 
at System.Text.Json.JsonSerializer.HandleValue(JsonTokenType tokenType, JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs(48,0): 
at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(22,0) : 
at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(74,0): 
at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(31,0): 
at System.Text.Json.JsonSerializer.Parse[TValue](String json, JsonSerializerOptions options)
D:\dotnet\corefx\src\System.Text.Json\tests\Serialization\Object.ReadTests.cs(312,0): 
at System.Text.Json.Serialization.Tests.ObjectTests.ReadStructObjectValueTest()

[ahsonkhan]

cc @YohDeadfall

[ahsonkhan]

@YohDeadfall, we are shooting for 6/26 for the preview 7 code freeze. Please let us know if you can get the fix in for this before then.

[buyaa-n]

Another case, Serializer not parsing struct with an object havign single value correctly
Note: below example runs successflully when object is key value pair like string json = @"{""MyObject"":{""One"":12}}";

public struct MyStructWithObject{
    public object MyObject { get; set; }
}
[Fact]
public static void ReadStructObjectValueTest()
{
    string json = @"{""MyObject"":{""One""}}";
    MyStructWithObject obj = JsonSerializer.Parse<MyStructWithObject>(json);

    Assert.Equal("One", ((JsonElement)obj.MyObject).GetString());
}

System.Text.Json.JsonException : '}' is invalid after a property name. Expected a ':'. Path: $.MyObject | LineNumber: 0 | BytePositionInLine: 18.
---- System.Text.Json.JsonReaderException : '}' is invalid after a property name. Expected a ':'. LineNumber: 0 | BytePositionInLine: 18.
 Stack Trace:
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\ThrowHelper.Serialization.cs(105,0): at System.Text.Json.ThrowHelper.ReThrowWithPath(JsonException exception, String path)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs(118,0): at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(22,0)
  : at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(74,0):
at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
D:\dotnet\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(31,0):
at System.Text.Json.JsonSerializer.Parse[TValue](String json, JsonSerializerOptions options)
D:\dotnet\corefx\src\System.Text.Json\tests\Serialization\Object.ReadTests.cs(282,0): at System.Text.Json.Serialization.Tests.ObjectTests.ReadStructObjectValueTest()

[wcontayon]

The exception is raised here
https://github.com/dotnet/corefx/blob/41998315ef96832cfe93d7e0eca451eb0d4a6f47/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs#L257-L265

On line 255, the convertion produce sometimes, an IList with a fixed size, and the line 264 will raise the exception.
I tried a sample solution by replacing the 264 with the code below, and it seems to work :)

    var tempList = list.ToList();
    tempList.Add(value);
    list = (IList<TProperty>)tempList;

@ahsonkhan , @buyaa-n Please can you give me your opinion

[ahsonkhan]

string json = @"{""MyObject"":{""One""}}";

System.Text.Json.JsonException : '}' is invalid after a property name. Expected a ':'. Path: $.MyObject | LineNumber: 0 | BytePositionInLine: 18.

That is expected. The json string you passed in is invalid JSON. A JSON object cannot contain just a string value. It must have property name: value pairs.

[danmoseley]

@YohDeadfall, we are shooting for 6/26 for the preview 7 code freeze. Please let us know if you can get the fix in for this before then

Specifically must have completed CI and merged by noon.

[buyaa-n]

@wcontayon i am not that familiar with the code base, so not sure why that llist has fixed size or if it is really need to be fixed size. But creating new list to add a new value is most likely not an option.

@ahsonkhan yes you are right, never mind

[YohDeadfall]

Sorry for not answering early, was very busy. I will take a look at it tomorrow and fix ASAP.

[wcontayon]

@wcontayon i am not that familiar with the code base, so not sure why that llist has fixed size or if it is really need to be fixed size. But creating new list to add a new value is most likely not an option.

@ahsonkhan yes you are right, never mind

I suspected that too, but I was not sure, thanks @buyaa-n

[mrpmorris]

A more simple test-case.
Note: The problem is resolved if you mark the property setter private.

[TestClass]
public class UnitTest1
{
	[TestMethod]
	public void TestMethod1()
	{
		var state = new AppState();
		string serialized = "{\"Values\":[1,2,3]}";
		object deserializedState = System.Text.Json.Serialization.JsonSerializer.Parse(serialized, typeof(AppState));
	}

	public class AppState
	{
		public int[] Values { get; set; }

		public AppState()
		{
			Values = Array.Empty<int>();
		}
	}
}

[YohDeadfall]

What behavior is expected here? Should the serializer replace the original collection?

From my point of view if there is something in the collection it must not be overwritten, only added to it. Since arrays doesn't support addition there is no way to insert a value without replacing the whole array. So if a public setter exists, read array of elements and concatenate it with the previous. No setter - throw an exception as it's going now. But it's a non obvious way and it complicates the logic a lot.

/cc @layomia

[martincostello]

In my opinion as per dotnet/corefx#38640, if the value of ICollection.IsReadOnly returns true, then a new list should be allocated and populated instead.

[steveharter]

See also related issue https://github.com/dotnet/corefx/issues/38528. We need to determine replace vs. add semantics. I believe Json.NET has replace

cc @JamesNK

[steveharter]

cc @Anipik

[JamesNK]

public struct SimpleTestStruct : ITestClass
{
    public short MyInt16 { get; set; }
    public int MyInt32 { get; set; }
    public long MyInt64 { get; set; }
    ...
    public SampleStruct MySampleStruct { get; set; }
    public SimpleTestClass MySampleTestClass { get; set; }
    ...
}

public class SimpleTestClass : ITestClass
{
    public short MyInt16 { get; set; }
    public int MyInt32 { get; set; }
    ...   
    public SampleEnum MyEnum { get; set; }
    public SampleInt64Enum MyInt64Enum { get; set; }
}

I don't see a collection here 🤷‍♂

[ahsonkhan]

I don't see a collection here

As @mrpmorris mentioned, this is the simplified test case which reproduces the issue being discussed here:
https://github.com/dotnet/corefx/issues/38435#issuecomment-502366899

[TestClass]
public class UnitTest1
{
	[TestMethod]
	public void TestMethod1()
	{
		var state = new AppState();
		string serialized = "{\"Values\":[1,2,3]}";
		object deserializedState = System.Text.Json.Serialization.JsonSerializer.Parse(serialized, typeof(AppState));
	}

	public class AppState
	{
		public int[] Values { get; set; }

		public AppState()
		{
			Values = Array.Empty<int>();
		}
	}
}

[YohDeadfall]

public class ClassWithCollection
{
    public int[] Array { get; set; } = { 1, 2 };
    public ICollection<int> CollectionBackedByList { get; set; } = new List<int> { 1, 2 };
    public ICollection<int> CollectionBackedByArray { get; set; } = new int[] { 1, 2 };
}

const string json = @"{""Array"":[3,4,5,6],""CollectionBackedByList"":[3,4,5,6],""CollectionBackedByArray"":[3,4,5,6]}";

var resultJil = Jil.JSON.Deserialize<ClassWithCollection>(json);
var resultUtf8 = Utf8Json.JsonSerializer.Deserialize<ClassWithCollection>(json);
var resultNewton = Newtonsoft.Json.JsonConvert.DeserializeObject<ClassWithCollection>(json);

Tested using three popular serializers and they behave near the same. All of them replace a collection if it's readonly (Array and CollectionBackedByArray cases), but if a collection is mutable (i.e. CollectionBackedByList) then only Newtonsoft appends new values. Utf8Json and Jil still replace the original object.

Probably we should align the behavior with Jil and Utf8Json because it's common for all collections and more intuitive when there is a public setter (it says that you're allowed to replace collection and should do it).

[YohDeadfall]

public class OuterObject
{
    public InnerObject Inner { get; set; } = InnerObject.Create();
}

public class InnerObject
{
    public InnerObject() : this(true) { }

    private InnerObject(bool instantiatedBySerializer) => InstantiatedBySerializer = instantiatedBySerializer;

    public static InnerObject Create() => new InnerObject(false);

    public bool InstantiatedBySerializer { get; }
    public int Value { get; set; }
}

const string json = @"{""Inner"":{""Value"":42}}";

var result = new OuterObject();
var resultJil = Jil.JSON.Deserialize<OuterObject>(json);
var resultUtf8 = Utf8Json.JsonSerializer.Deserialize<OuterObject>(json);
var resultNewton = Newtonsoft.Json.JsonConvert.DeserializeObject<OuterObject>(json);
var resultSystem = System.Text.Json.Serialization.JsonSerializer.Parse<OuterObject>(json);

Serializer	InstantiatedBySerializer
Jil	true
Utf8Json	true
Newtonsoft.Json	false
System.Text.Json	true

It looks like Newtonsoft reuses existing objects, not collections only. And the System.Text.Json reads a new instance every time like Jil and Utf8Json.

[steveharter]

@YohDeadfall thanks the research and analysis. Does this capture the summary:

• Property setter: replace
  • The deserializer will replace any previous instance (either created by code in the ctor, or from the deserializer if previously created due to the same property being repeated in the JSON).
• No property setter: use existing instance
  • If value is null or collection is immutable (including Array), then throw.
  • Clear() if IList or IDictionary (should we really do this?)

[martincostello]

I would rather it not throw if it is read-only.

We've been assigning Array.Empty<T>() to properties at initialization time by default so they're non-null by default when creating the objects in code and so we don't have to pay to allocate a List<T> on object creation only for it to then be overwritten during deserialization.

Changing the behaviour would mean updating all our models to use List<T> instead or not initializing such properties in the constructors at all, meaning lots of calls sites being updated to now initialize properties that previously had a default value (e.g. reams of test code manually constructing objects directly).

[buyaa-n]

public struct SimpleTestStruct : ITestClass
{
public short MyInt16 { get; set; }
public int MyInt32 { get; set; }
public long MyInt64 { get; set; }
...
public SampleStruct MySampleStruct { get; set; }
public SimpleTestClass MySampleTestClass { get; set; }
...
}
I don't see a collection here 🤷‍♂

@JamesNK with 3 dots (...) I meant there is more properties exist, it is a big class didn't wanted to paste all as people could find from codebase by name, anyways the test case is simplified and the SimpleTestStruct class is attached to the issue

[JamesNK]

Sure, no problem. I didn't notice the file link.

[JamesNK]

only Newtonsoft appends new values. Utf8Json and Jil still replace the original object.

I agree that Newtonsoft is the odd one out. Replacing the original object makes more sense to do by default.

In the future if you want to extend behavior to support appending then you can add it as a setting.

[ahsonkhan]

No property setter: use existing instance

On de-serializing, if there is no property setter, why not just ignore the property all together (don't throw, don't clear, don't mutate), for all properties, which would include collections?

[mrpmorris]

No property setter: use existing instance

On de-serializing, if there is no property setter, why not just ignore the property all together (don't throw, don't clear, don't mutate), for all properties, which would include collections?

I need it to be able to deserialize properties with private setters, as long as it can do that I don't mind, but this bug won't deserialize a public property with public setter if it's an array.

[YohDeadfall]

@ahsonkhan Because there are cases when you have your own collection which can't be instantiated by a third party. Mostly it's because a collection needs to know about it's owner. So the best strategy here is:

• replace a collections if there is a public setter;
• add values to the collection if there is no public setter and the collection isn't read-only;
• ignore it otherwise.

[YohDeadfall]

Moreover most API doesn't allow you to assign a new value to a collection property because collection isn't a value, but items in it is. So we should support this scenario and do not introduce incompatibility with other serializers.

[superyyrrzz]

I ran into this issue even with public setter, and finally figured out the root cause: JsonSerializerOptions.IgnoreNullValues is true. Changing it to default false resolved my issue.