Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
layomia opened this issue Aug 12, 2019 · 18 comments

Comments

@layomia
Copy link
Member

commented Aug 12, 2019

Consider (head at 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

#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 through a NotSupportedException.

These collections can be supported in the future.

@ahsonkhan

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

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

@steveharter

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

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

ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionPropertyInfo.DeclaredPropertyType, jsonPath);

ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionPropertyInfo.DeclaredPropertyType, jsonPath);

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

@steveharter

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

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 #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

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

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

@layomia layomia self-assigned this Aug 14, 2019

@JamesNK

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

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

if (IsReadOnly)
throw new NotSupportedException(SR.CollectionReadOnly);

bool ICollection<T>.IsReadOnly
{
get { return true; }
}

void ICollection<T>.Add(T value)
{
ThrowHelper.ThrowNotSupportedException(ExceptionResource.NotSupported_ReadOnlyCollection);
}

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 layomia changed the title Serializer should throw JsonException when an "add" method on a derived enumerable cannot be invoked Serializer should throw NotSupportedException when collections can be instantiated but not populated Aug 15, 2019

@layomia

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

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 #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

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@khellang thanks for looking into this. Your PR was indeed the fix needed except for wrapping the exceptions. #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

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Re-opening until fixed in 3.0.

@ahsonkhan ahsonkhan moved this from In progress to In PR in System.Text.Json - 3.0 Aug 20, 2019

@ahsonkhan

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Fixed in release/3.0 by #40438.

@ahsonkhan ahsonkhan closed this Aug 21, 2019

System.Text.Json - 3.0 automation moved this from In PR to Done Aug 21, 2019

@khellang

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.