dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.07k stars 4.69k forks source link

Add support for collection types with parameterless or IEnumerable<T> constructors. #80688

Open snjosephms opened 1 year ago

snjosephms commented 1 year ago

We are migrating our code base from Newtonsoft to System.Text.Json.

While testing this migration effort, we have encountered the following error while deserializing a type which uses ConcurrentBag type: "Message=The collection type 'System.Collections.Concurrent.ConcurrentBag`1' is abstract, an interface, or is read only, and could not be instantiated and populated."

I see that this also documented here - https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types?pivots=dotnet-7-0#systemcollectionsconcurrent-namespace

Is there any workaround for providing support for ConcurrentBag type?

This is a major blocker in our migration effort as all hot path scenarios are dependent on this type

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-collections See info in area-owners.md if you want to be subscribed.

Issue Details
We are migrating our code base from Newtonsoft to System.Text.Json. While testing this migration effort, we have encountered the following error while deserializing a type which uses **ConcurrentBag** type: "Message=The collection type 'System.Collections.Concurrent.ConcurrentBag`1' is abstract, an interface, or is read only, and could not be instantiated and populated." I see that this also documented here - https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types?pivots=dotnet-7-0#systemcollectionsconcurrent-namespace Is there any workaround for providing support for ConcurrentBag type? This is a major blocker in our migration effort as all hot path scenarios are dependent on this type
Author: snjosephms
Assignees: -
Labels: `area-System.Collections`
Milestone: -
Tornhoof commented 1 year ago

you want a custom JsonConverter for your ConcurrentBag, see https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to?pivots=dotnet-7-0 for details.

eiriktsarpalis commented 1 year ago

Here's a simple way to write a custom converter for ConcurrentBag:

public class ConcurrentBagConverter<T> : JsonConverter<ConcurrentBag<T>>
{
    private readonly JsonConverter<IEnumerable<T>> _enumerableConverter = (JsonConverter<IEnumerable<T>>)JsonSerializerOptions.Default.GetConverter(typeof(IEnumerable<T>));

    public override ConcurrentBag<T>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        IEnumerable<T>? result = _enumerableConverter.Read(ref reader, typeToConvert, options);
        return result is null ? null : new ConcurrentBag<T>(result);
    }

    public override void Write(Utf8JsonWriter writer, ConcurrentBag<T> value, JsonSerializerOptions options)
        => _enumerableConverter.Write(writer, value, options);
}

In the future we should add support for collection types that satisfy common conventions such as

  1. Having a public parameterless constructor and a public Add method or
  2. Having a public constructor that accepts one IEnumerable<T> parameter (or IEnumerable<KeyValuePair<TKey, TValue>> for dictionary types.

Related to #71944.

snjosephms commented 1 year ago

@eiriktsarpalis thanks for sharing the sample, In this sample would passing the same JsonSerializerOptions in the overridden JsonConverter.Read or JsonConverter.Write be the cause of a potential stackoverflow error (resulting in a perf hit)? Below is how I had written my implementation ConcurrentBagJsonConverter

internal class ConcurrentBagJsonConverter : JsonConverter<ConcurrentBag<FlushDocument>>
        {
            public override ConcurrentBag<FlushDocument> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType != JsonTokenType.StartArray)
                    throw new InvalidOperationException();

                ConcurrentBag<FlushDocument> bag = new ConcurrentBag<FlushDocument>();
                while (reader.Read())
                {
                    if (reader.TokenType == JsonTokenType.EndArray)
                        break;

                    bag.Add(JsonSerializer.Deserialize<FlushDocument>(ref reader, options));
                }

                return bag;
            }

            public override void Write(Utf8JsonWriter writer, ConcurrentBag<FlushDocument> value, JsonSerializerOptions options)
            {
                // Passing JsonSerializerOptions *without* CustomConverter to avoid stackoverflow
                System.Text.Json.JsonSerializer.Serialize(writer, value, ocSBMsgSerializerOptions);
            }

        }
krwq commented 1 year ago

@snjosephms that should work fine since we're not using same type for serialization/deserialization as in your case. if we change your Serialize call to pass in IEnumerable type that should not require extra options as well

justintoth commented 1 year ago

@eiriktsarpalis How would you go about registering your ConcurrentBagConverter? I'm having trouble with the syntax, because it expects a generic type to be passed in, when we would want to use this converter for all usages of ConcurrentBag, regardless of the generic type.

EDIT: For others who find this thread, the answer was to create a factory class that can be added to the options Converters list:

public class ConcurrentBagConverterFactory : JsonConverterFactory
{
    private static Type[] ConvertibleTypes = new Type[]
    {
        typeof(bool),
        typeof(byte),
        typeof(char),
        typeof(decimal),
        typeof(double),
        typeof(float),
        typeof(int),
        typeof(long),
        typeof(sbyte),
        typeof(short),
        typeof(uint),
        typeof(ulong),
        typeof(ushort),
    };

    public static bool CanConvertImpl(Type typeToConvert)
    {
        if (typeToConvert.IsGenericType
            && typeToConvert.GetGenericTypeDefinition() == typeof(ConcurrentBag<>))
        {
            var keyType = typeToConvert.GenericTypeArguments[0];
            return keyType.IsEnum || ConvertibleTypes.Any(convertibleType => keyType == convertibleType);
        }

        var baseType = typeToConvert.BaseType;
        if (!(baseType is null))
        {
            return CanConvertImpl(baseType);
        }

        return false;
    }

    public override bool CanConvert(Type typeToConvert)
    {
        return CanConvertImpl(typeToConvert);
    }

    public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
    {
        var converterType = typeof(ConcurrentBagConverter<>)
            .MakeGenericType(typeToConvert.GenericTypeArguments[0]);

        var converter = (JsonConverter)Activator.CreateInstance(
            converterType,
            BindingFlags.Instance | BindingFlags.Public,
            binder: null,
            new object[] {
                //_converterOptions, _namingPolicy
            },
            culture: null);

        return converter;
    }
}
eiriktsarpalis commented 1 year ago

@justintoth I would probably simplify this to the following, full code:

var options = new JsonSerializerOptions { Converters = { new ConcurrentBagConverter() } };
var bag = JsonSerializer.Deserialize<ConcurrentBag<int>>("[1, 2, 3]", options);
Console.WriteLine(bag.Count);

public class ConcurrentBagConverter<T> : JsonConverter<ConcurrentBag<T>>
{
    private readonly JsonConverter<IEnumerable<T>> _enumerableConverter;
    public ConcurrentBagConverter(JsonConverter<IEnumerable<T>> enumerableConverter)
        => _enumerableConverter = enumerableConverter;

    public override ConcurrentBag<T>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        IEnumerable<T>? result = _enumerableConverter.Read(ref reader, typeof(IEnumerable<T>), options);
        return result is null ? null : new ConcurrentBag<T>(result);
    }

    public override void Write(Utf8JsonWriter writer, ConcurrentBag<T> value, JsonSerializerOptions options)
        => _enumerableConverter.Write(writer, value, options);
}

public class ConcurrentBagConverter : JsonConverterFactory
{
    public override bool CanConvert(Type typeToConvert) => typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(ConcurrentBag<>);

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
    {
        Debug.Assert(CanConvert(typeToConvert));
        Type elementType = typeToConvert.GetGenericArguments()[0];
        Type ienumerableType = typeof(IEnumerable<>).MakeGenericType(elementType);
        Type converterType = typeof(ConcurrentBagConverter<>).MakeGenericType(elementType);
        JsonConverter ienumerableConverter = options.GetConverter(ienumerableType);
        return (JsonConverter)Activator.CreateInstance(converterType, ienumerableConverter)!;
    }
}
dotnet-policy-service[bot] commented 3 months ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.