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

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

Open
Tracked by #107787
snjosephms opened this issue Jan 16, 2023 · 8 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@snjosephms
Copy link

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 ghost added the untriaged New issue has not been triaged by the area owner label Jan 16, 2023
@ghost
Copy link

ghost commented Jan 16, 2023

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
Copy link
Contributor

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 eiriktsarpalis added partner-impact This issue impacts a partner who needs to be kept updated enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Jan 18, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jan 18, 2023
@eiriktsarpalis
Copy link
Member

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.

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Jan 18, 2023
@snjosephms
Copy link
Author

snjosephms commented Jan 19, 2023

@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
Copy link
Member

krwq commented Jan 19, 2023

@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
Copy link

justintoth commented Sep 20, 2023

@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
Copy link
Member

@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)!;
    }
}

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Sep 21, 2023
@eiriktsarpalis eiriktsarpalis removed the partner-impact This issue impacts a partner who needs to be kept updated label Sep 21, 2023
@eiriktsarpalis eiriktsarpalis changed the title Workaround for providing support for ConcurrentBag types Add support for collection types with parameterless or IEnumerable<T> constructors. Sep 21, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: 9.0.0, Future Jul 5, 2024
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants