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

Consider (head at https://github.com/dotnet/corefx/commit/4fbef813c6e3c1ce1a23e5dc1528e65abb1b8c0a)

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

https://github.com/dotnet/corefx/pull/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.

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 19 (19 by maintainers)

Most upvoted comments

@ahsonkhan Filed https://github.com/dotnet/docs/issues/14005. Let me know if there’s anything you’d like to change/add 😄

  • 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.

I’ll wait for a decision on whether JsonException, NotSupportedException or not throwing at all is the best option, before updating https://github.com/dotnet/corefx/pull/40268.

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