Error Being thrown for Duplicate properties

[Anipik]

https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs#L80
This error gets thrown when we try to deserialize Dicitonary with duplicate properties (As it increases the count). However it might now throw and give random output if the cacheCount is lesss than propertyRefCount

  ===========================================================================================================
    Discovering: System.Text.Json.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Text.Json.Tests (found 885 of 908 test cases)
    Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 12)
  Process terminated. Assertion failed.
     at System.Text.Json.JsonClassInfo.UpdateSortedPropertyCache(ReadStackFrame& frame) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonClassInfo.cs:line 80
     at System.Text.Json.JsonSerializer.HandleEndObject(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.HandleObject.cs:line 59
     at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.cs:line 90
     at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs:line 22
     at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs:line 74
     at System.Text.Json.JsonSerializer.Parse[TValue](String json, JsonSerializerOptions options) in C:\git\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs:line 31
     at System.Text.Json.Serialization.Tests.DictionaryTests.DeserializeDictionaryWithDuplicateProperties() in C:\git\corefx\src\System.Text.Json\tests\Serialization\DictionaryTests.cs:line 624
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)

[ahsonkhan]

@Anipik, when do you plan to submit a fix for this issue?

[Anipik]

@ahsonkhan i tested two strategies here 1) with hashset 2) with linq. The change is not effecting the performance. (It might need a little bit more testing). I recalled you wanted to take a look at the hashset change.
i have a blueprint here https://github.com/Anipik/corefx/commit/4ffb38892b953da0281dc6a6f2e127041333d233

i am side tracked by a .NetFramework bug, but i think i will be able to make a final PR(after performance stuff) by wednesday

[ahsonkhan]

Sounds good. cc @steveharter

[steveharter]

@JamesNK if duplicate properties exist in JSON for an array or list, I believe Json.NET replaces the whole array\list. Are these semantics well-known and assumed (vs. concatenating the items). Thanks.

@Anipik for now, I assume we want replace semantics to match Json.NET.

[JamesNK]

@JamesNK if duplicate properties exist in JSON for an array or list, I believe Json.NET replaces the whole array\list. Are these semantics well-known and assumed (vs. concatenating the items). Thanks.

In Json.NET there is a setting for it. It concatenates by default, but in hindsight I think replacing is better.

IMO you should replace by default, and if you want to you could add a setting post 3.0 to support adding collection values into existing collections and populating properties onto existing objects.

[steveharter]

aside: "replace" vs. "add" semantics applies both to cases where there at pre-existing values in the object (set from the ctor, for example) and also having duplicate properties in the JSON. I think these should be treated the same.

If we have replace semantics, then we have to decide whether to re-set the property with a new list\etc instance or attempt to call "Clear".

However, not all collections support "Clear" (readonly), and arrays don't. So it appears the most straightforward option is to call the setter again with a new instance (list or array) on the property. This requires the property have a setter plus it will replace any instance the user may have created, which might not be a nice thing for polymorphic scenarios (for example, the ctor may have assigned a custom collection on a IList property where we would replace with List<object>).

I suppose we could attempt to get the count\length and only replace if >0 so we can preserve the existing instance (if there is one)?

[JamesNK]

I think these should be treated the same.

That's what Json.NET does. ObjectCreationHandling covers both objects and collections.

https://www.newtonsoft.com/json/help/html/DeserializeObjectCreationHandling.htm

If we have replace semantics, then we have to decide whether to re-set the property with a new list\etc instance or attempt to call "Clear".

Set a new list.

[Anipik]

The issue with the lists and arrays is tracked by https://github.com/dotnet/corefx/issues/38435 so closing this one.