Replace collection instances on deserialize

[steveharter]

Fixes https://github.com/dotnet/corefx/issues/38435 the original issue with "Collection was of a fixed size" for pre-populated arrays.

In addition the semantics for how collections are materialized has changed:

• Pre-populated collections with a setter are replaced on deserialization.
• Pre-populated collections without a setter are ignored on deserialization.

Previously, pre-populated collections were appended to (when possible).

Replaces PR #38797. cc @YohDeadfall

[layomia]

Should this be something like "we replace the contents of this collection"?

[steveharter]

Thanks. I will change in future PR to avoid CI reset.

[YohDeadfall]

Looks as a good fix, but this limits the serializer and makes it incompatible with others (see https://github.com/dotnet/corefx/issues/38435#issuecomment-504686914).

Sometimes business logic requires to have a collection which can't be null. So the user is forced to add a validator for that case or somehow handle this scenario in code. The last one can be done via the null coalesce operator (collection?.Any()), but it makes migration to .NET 3.0 a bit harder.

So if there is no time to make it better, then I'm okay with it.

[YohDeadfall]

In #38797 I tried to fix the issue without introducing the breaking change.

[steveharter]

Looks as a good fix, but this limits the serializer and makes it incompatible with others

I created https://github.com/dotnet/corefx/issues/39477 to track that. This likely won't be for 3.0 since we already added full collection support throughout the product for all types of immutable collections, etc including derived types (cc @layomia).

[serhatozgel]

I'm having an interesting issue with this one:

This test is failing:

[TestMethod]
        public void TestNullable()
        {
            string json = @"{""MyList"":[2,3]}";
            ClassWithPopulatedListAndSetter obj = JsonSerializer.Deserialize<ClassWithPopulatedListAndSetter>(json);
            Assert.AreEqual(2, obj.MyList.Length);
        }

        public class ClassWithPopulatedListAndSetter
        {
            private int[] myList = new int[] { 1 };

            public int[] MyList
            {
                get => myList ??= Array.Empty<int>();
                set => myList = value;
            }
        }

While this is passing:

[TestMethod]
        public void TestNullable()
        {
            string json = @"{""MyList"":[2,3]}";
            ClassWithPopulatedListAndSetter obj = JsonSerializer.Deserialize<ClassWithPopulatedListAndSetter>(json);
            Assert.AreEqual(2, obj.MyList.Length);
        }

        public class ClassWithPopulatedListAndSetter
        {
            private int[] myList = new int[] { 1 };

            public int[] MyList
            {
                get => myList;
                set => myList = value;
            }
        }

As you can see, I desperately want the returned value of the getter to be not null even when it is explicitly set to null before. I would expect both of the above cases to work but oddly enough, the first one is failing.

Please feel free to look into that one @steveharter, I'm not sure if this would be something that you'd like to change.

[ahsonkhan]

@serhatozgel - I am assuming you were trying this on 3.0/3.1. Is that correct? The issue you are seeing has already been fixed for 5.0 (I couldn't repro it against master).

Could you try the latest nightly build of the SDK or add a package reference to the latest S.T.Json NuGet package from our nightly build feed: https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json (e.g. version 5.0.0-alpha.1.19563.6).

Does that work for you?

This test is failing

How is it failing? Are you seeing System.NotSupportedException: Collection was of a fixed size.?

[serhatozgel]

@ahsonkhan It was indeed that error message and I tested with 3.0 and 3.1. I'm glad it is fixed for 5.0.
I'll try to try the nightly build and report back. Thanks a lot for your reply.