Serializer should throw NotSupportedException when collections can be instantiated but not populated

[layomia]

Consider (head at dotnet/corefx@4fbef81)

using System;
using System.Text.Json;
using Microsoft.Extensions.Primitives;

namespace stringvaluestest
{
    class Program
    {
        static void Main(string[] args)
        {
            string json = @"[""My Val""]";
            JsonSerializer.Deserialize<StringValues>(json);
        }
    }
}

Output

Unhandled exception. System.NotSupportedException: Specified method is not supported.
   at Microsoft.Extensions.Primitives.StringValues.System.Collections.Generic.ICollection<System.String>.Add(String item)
   at System.Text.Json.JsonPropertyInfoCommon`4.CreateDerivedEnumerableInstance(JsonPropertyInfo collectionPropertyInfo, IList sourceList, String jsonPath, JsonSerializerOptions options)
   at System.Text.Json.Serialization.Converters.DefaultDerivedEnumerableConverter.CreateFromList(ReadStack& state, IList sourceList, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.HandleEndArray(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
   at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
   at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at stringvaluestest.Program.Main(String[] args) in D:\console_apps\stringvaluestest\Program.cs:line 14

dotnet/corefx#39001 adds support for types that implement BCL enumerables that are natively supported in the serializer.

For cases like StringValue where we cannot invoke the add method of the implemented type (ICollection<string>.Add in this case), we should throw a JsonException instead of a NotSupportedException:

For enumerables that are not supported: readonly collections like StringValues (implements ICollection<string>), and collections that do not implement any of:

• IList
• ICollection<T>
• Stack<T>
• Queue<T>
• IDictionary
• IDictionary<string, TValue>,
  the serializer should throw a NotSupportedException.

These collections can be supported in the future.

[ahsonkhan]

where we cannot invoke the add method of the implemented type (ICollection<string>.Add in this case), we should throw a JsonException instead of a NotSupportedException:

Why do you think that's better? Isn't it clearer to convey that serializing a type like StringValues is not supported, and you need a custom converter to handle it?

[khellang]

we should throw a JsonException instead of a NotSupportedException

It's not the serializer that throws this exception, it's StringValues itself:

https://github.com/aspnet/Extensions/blob/cc9a033c6a8a4470984a4cc8395e42b887c07c2e/src/Primitives/src/StringValues.cs#L314-L317

How can the serializer know which ICollection<T>.Add methods are safe to call and which are not? I'm with @ahsonkhan here; I think this should be left as-is to signal that a custom converter is warranted.

[khellang]

Oh, I think I get what you're saying now; you want to catch the NotSupportedException and wrap it in a JsonException? I guess that makes sense. Makes it easier to reason about types of exceptions that can be thrown when calling JsonSerializer.Deserialize<T>.

[martincostello]

The IsReadOnly property on StringValues returns true, maybe it shouldn't even be trying to call Add()? Then you don't even need to catch anything.

https://github.com/aspnet/Extensions/blob/cc9a033c6a8a4470984a4cc8395e42b887c07c2e/src/Primitives/src/StringValues.cs#L75-L78

[khellang]

@martincostello That's a good point. Added to dotnet/corefx#40268.

[steveharter]

I checked semantics of Json.NET -- it ignores readonly collections (e.g. exception for StringValue).

@JamesNK did "ignore" semantics for readonly collections pan out OK for Json.NET? Any regrets? Thanks

[layomia]

Since this collection is readonly, it falls under the not-supported category, the serializer (not StringValues) should be throwing a NotSupportedException with an appropriate message.

The other exception we throw on (de)serialization failure is JsonException with a message saying we were unable to convert the value. This is used when the collection is typically supported but the serializer's state does not permit (de)serialization e.g. having a null converter, a null key name when processing a dictionary, a null delegate to create an instance object etc.

In

https://github.com/dotnet/corefx/blob/2811879c01bf6f3b95aa4ee9b8994cfb62038610/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs#L159

https://github.com/dotnet/corefx/blob/2811879c01bf6f3b95aa4ee9b8994cfb62038610/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs#L187

we throw a JsonException instead of a NotSupportedException. It would be great if dotnet/corefx#40268 could fix this as well. @khellang I'll add some notes to the PR. Thanks!

[steveharter]

Since this collection is readonly, it falls under the not-supported category, the serializer (not StringValues) should be throwing a NotSupportedException with an appropriate message.
…
we throw a JsonException instead of a NotSupportedException. It would be great if dotnet/corefx#40268 could fix this as well

I think JsonException is preferred in the case over NotSupportedException because we typically throw that when the exception is caused by unsupported JSON.

However, the other option is to ignore -- see previous comment and comparison with Json.Net.

[khellang]

I'll wait for a decision on whether JsonException, NotSupportedException or not throwing at all is the best option, before updating dotnet/corefx#40268.

[JamesNK]

@JamesNK did "ignore" semantics for readonly collections pan out OK for Json.NET? Any regrets? Thanks

It isn't a common complaint that I know of so I guess so. Keep in mind that Json.NET has very good support for creating readonly collection's via their constructor (and various other customizations to support F# and immutable collections).

I don't know exactly why StringValues doesn't throw an error when it is deserialized. It might be because it is a struct and a readonly collection. That's an odd type 🤷‍♂

[steveharter]

@layomia and I talked offline. Conclusion:

• We may want to support StringValues in the future; thus we shouldn't ignore when readonly.
• JsonException is not a fit either since we may want to support additional cases in the future (e.g. probe for constructors that take an IEnumerable).
• Thus NotSupportedException is best (as the PR currently has)
• We shouldn't have general try\catch-all (not a good practice) -- let other exceptions that may be thrown propagate.

[khellang]

• We may want to support StringValues in the future; thus we shouldn't ignore when readonly.

What does this mean? You can't support StringValues in the future, using the Add method. It throws NotSupportedException (because it's IsReadOnly).

As I see it, supporting StringValues and this issue is two different things. If you want to support StringValues in the future, you have to add support for calling the proper constructor, not using the Add method, like @JamesNK mentioned:

Keep in mind that Json.NET has very good support for creating readonly collection's via their constructor

Also, this is about much more than StringValues. Any collection that has IsReadOnly probably doesn't want you to call its Add method:

https://github.com/dotnet/corefx/blob/66273ed9944c7c964d3d51771ed8d4b4ddce3e8c/src/System.Collections.Specialized/src/System/Collections/Specialized/NameValueCollection.cs#L265-L266

https://github.com/dotnet/corefx/blob/66273ed9944c7c964d3d51771ed8d4b4ddce3e8c/src/Common/src/CoreLib/System/Collections/ObjectModel/ReadOnlyCollection.cs#L65-L68

https://github.com/dotnet/corefx/blob/66273ed9944c7c964d3d51771ed8d4b4ddce3e8c/src/Common/src/CoreLib/System/Collections/ObjectModel/ReadOnlyCollection.cs#L79-L82

Why rely on exceptions for flow control? If you ask me, that's more of an anti-pattern than wrapping execptions with a clear message and a documented exception type.

[layomia]

If you want to support StringValues in the future, you have to add support for calling the proper constructor, not using the Add method

This is understood. The point was we should throw the exception, not ignore (creating an empty instance like Json.NET does)
@khellang, please see the updated description for the scope of this issue.

Why rely on exceptions for flow control? If you ask me, that's more of an anti-pattern than wrapping execptions with a clear message and a documented exception type.

We should preemptively check if the instance is readonly and throw NotSupportedException if it is. Aside from getting an exception when you try to add to a readonly collection, I don't foresee any other exception being thrown with the current logic.
Any exception that does occur should be propagated to the caller.

[khellang]

We should preemptively check if the instance is readonly and throw NotSupportedException if it is.

This is what the PR did, but as mentioned in dotnet/corefx#40268 (comment), it seemed like that wasn't desired.

TBH, I'm a bit confused about what needs to be done here. If you don't want to special-case IsReadOnly and don't want to catch and wrap exceptions. Is it just a matter of changing the existing JsonException to NotSupportedException?

[layomia]

@khellang thanks for looking into this. Your PR was indeed the fix needed except for wrapping the exceptions. dotnet/corefx#40268 (comment) contained a typo at first: "Since we are now special casing for ReadOnly" (intended) read at first as "Since we are not special casing for ReadOnly"

I'll create a PR closing this issue.

[ahsonkhan]

Re-opening until fixed in 3.0.

[ahsonkhan]

Fixed in release/3.0 by dotnet/corefx#40438.

[khellang]

@ahsonkhan Filed dotnet/docs#14005. Let me know if there's anything you'd like to change/add 😄

[ahsonkhan]

Let me know if there's anything you'd like to change/add

Nope, it's perfect :)