[release/3.0] Honor IsReadOnly when deserializing collections

[khellang]

Ports #40366 to 3.0.
Closes https://github.com/dotnet/corefx/issues/40237

// @layomia @steveharter @ahsonkhan

Description

Previously, the System.Text.Json serializer would try to call Add on any collection, even if it has an IsReadOnly property that returns true. This would lead to a NotSupportedException. It also used to throw a JsonException if a collection was unsupported.

After this PR, the serializer will no longer attempt to call Add if the collection has IsReadOnly == true and will always throw a NotSupportedException with a descriptive message for unsupported collections.

Customer Impact

Customers will now have to catch/handle/deal with NotSupportedException in addition to JsonException when deserializing. Read-only collections will have a better exception message telling the customer that the collection is unsupported.

Regression?

No.

Risk

Low. Most collections that returns true for IsReadOnly will throw NotSupportedException when trying to call Add anyway. This will just give them a better exception message.

[ahsonkhan]

Once merged, we need to submit a breaking-change issue to document the change in exception type from preview7 to now.
https://github.com/dotnet/docs/issues/new?template=doc-breaking-change.md
https://github.com/dotnet/docs/issues?q=is%3Aopen+is%3Aissue+label%3Abreaking-change

[ahsonkhan]

LGTM. Thanks for fixing this @khellang! Waiting for approval before merging for preview 9.

[khellang]

Looks like there's a failing test on this branch that wasn't present on master? 🤔

[ahsonkhan]

    System.Text.Json.Serialization.Tests.CustomConverterTests.CustomUnsupportedIEnumerableConverter [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.Text.Json.JsonException)
      Actual:   typeof(System.NotSupportedException): The collection type 'System.Text.Json.Serialization.Tests.StringIEnumerableWrapper' on 'System.Text.Json.Serialization.Tests.UnsupportedDerivedTypesWrapper_IEnumerable.IEnumerableWrapper' is not supported.
      ---- System.NotSupportedException : The collection type 'System.Text.Json.Serialization.Tests.StringIEnumerableWrapper' on 'System.Text.Json.Serialization.Tests.UnsupportedDerivedTypesWrapper_IEnumerable.IEnumerableWrapper' is not supported.
      Stack Trace:
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs(158,0): at System.Text.Json.JsonPropertyInfoCommon`4.CreateDerivedEnumerableInstance(JsonPropertyInfo collectionPropertyInfo, IList sourceList, String jsonPath, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/Converters/DefaultDerivedEnumerableConverter.cs(13,0): at System.Text.Json.Serialization.Converters.DefaultDerivedEnumerableConverter.CreateFromList(ReadStack& state, IList sourceList, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs(110,0): at System.Text.Json.JsonSerializer.HandleEndArray(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs(109,0): at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs(24,0): at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs(65,0): at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs(31,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
        ----- Inner Stack Trace -----
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs(158,0): at System.Text.Json.JsonPropertyInfoCommon`4.CreateDerivedEnumerableInstance(JsonPropertyInfo collectionPropertyInfo, IList sourceList, String jsonPath, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/Converters/DefaultDerivedEnumerableConverter.cs(13,0): at System.Text.Json.Serialization.Converters.DefaultDerivedEnumerableConverter.CreateFromList(ReadStack& state, IList sourceList, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleArray.cs(110,0): at System.Text.Json.JsonSerializer.HandleEndArray(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs(109,0): at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs(24,0): at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs(65,0): at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
        /_/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs(31,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)

[khellang]

Ah, looks like it's a bit of a race condition. When the CI ran (successfully) on #40366, #40411 hadn't been merged. After it was merged, one test started failing. Should this be fixed on master and ported over here as well?

[ahsonkhan]

Ah, looks like it's a bit of a race condition. When the CI ran (successfully) on #40366, #40411 hadn't been merged. After it was merged, one test started failing.

Yep, I believe master is failing too.

Should this be fixed on master and ported over here as well?

Yes. I'd recommend submitting the PR in master and add the commit to this PR after that passes CI.

[khellang]

Yes. I'd submit the PR in master and add the commit to this PR after.

Submitted #40439.

[ahsonkhan]

@khellang - can you cherry-pick the commit from #40439 into this PR please?

Here's the hash: cf5c1b0

[ericstj]

I'm ok with this change. If we believe the NotSupportedException is correct, it is better to make that change now than have it be breaking in vNext. I agree that this is common enough for folks to hit that we need a better exception.

[leecow]

tactics approved - preview9