System.Text.Json cannot deserialize to IReadOnlySet

[StasJS]

Description

Reading through https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types JsonSerializer.Deserialize will support deserialization to a ton of different collection interfaces.

Strangely, IReadOnlySet is not mentioned at all in the page I linked. Not even to say it's not supported, as some collection types have e.g. LinkedListNode<T>.

When trying to deserialize to an IReadOnlySet, JsonSerializer throws a NotSupportedException

Reproduction Steps

• Run dotnet new console
• Write Program.cs

using System.Collections.Immutable;
using System.Text.Json;

// Both of these work as expected
var setResult = JsonSerializer.Deserialize<ISet<int>>("[1]");
var immutableSetResult = JsonSerializer.Deserialize<IImmutableSet<int>>("[1]");

// throws NotSupportedException
JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]");

Expected behavior

JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]"); ought to work, like it does for ISet or other readonly collections e.g. IReadOnlyList.

I would also expect IReadOnlySet to be featured in this page https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types one way or another.

Actual behavior

JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]"); throws NotSupportedException and completely absent from https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types

Regression?

Don't know.

Known Workarounds

Use ISet or similar.

Configuration

> dotnet sdk check
Version      Status
------------------------
7.0.400      Up to date.

Microsoft.NETCore.App             7.0.10       Up to date.
Microsoft.WindowsDesktop.App      7.0.10       Up to date.

Windows 11 Pro x64

> [System.Environment]::OSVersion
Platform ServicePack Version      VersionString
-------- ----------- -------      -------------
 Win32NT             10.0.22623.0 Microsoft Windows NT 10.0.22623.0

Other information

No response

[ghost]

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

Issue Details

---

Description

Reading through https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types JsonSerializer.Deserialize will support deserialization to a ton of different collection interfaces.

Strangely, IReadOnlySet is not mentioned at all in the page I linked. Not even to say it's not supported, as some collection types have e.g. LinkedListNode<T>.

When trying to deserialize to an IReadOnlySet, JsonSerializer throws a NotSupportedException

Reproduction Steps

• Run dotnet new console
• Write Program.cs

using System.Collections.Immutable;
using System.Text.Json;

// Both of these work as expected
var setResult = JsonSerializer.Deserialize<ISet<int>>("[1]");
var immutableSetResult = JsonSerializer.Deserialize<IImmutableSet<int>>("[1]");

// throws NotSupportedException
JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]");

Expected behavior

JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]"); ought to work, like it does for ISet or other readonly collections e.g. IReadOnlyList.

I would also expect IReadOnlySet to be featured in this page https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types one way or another.

Actual behavior

JsonSerializer.Deserialize<IReadOnlySet<int>>("[1]"); throws NotSupportedException and completely absent from https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/supported-collection-types

Regression?

Don't know.

Known Workarounds

Use ISet or similar.

Configuration

> dotnet sdk check
Version      Status
------------------------
7.0.400      Up to date.

Microsoft.NETCore.App             7.0.10       Up to date.
Microsoft.WindowsDesktop.App      7.0.10       Up to date.

Windows 11 Pro x64

> [System.Environment]::OSVersion
Platform ServicePack Version      VersionString
-------- ----------- -------      -------------
 Win32NT             10.0.22623.0 Microsoft Windows NT 10.0.22623.0

Other information

No response

Author:	StasJS
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[eiriktsarpalis]

Because contract customization has not been fully implemented for collection types, it is not possible to work around this issue by customizing the JsonTypeInfo.CreateObject delegate. Instead, implementing a custom converter for the interface is necessary. Until this is addressed, my recommendation is to deserialize on models that use concrete collection types.

We should audit all built-in collection interfaces and check whether they are supported by the serializer. One possibility is to honor the newly added CollectionBuilderAttribute and use the delegate as specified by the interface declaration.

[huoyaoyuan]

Since HashSet<T> and ISet<T> are supported, it's reasonable to also include IReadOnlySet<T>.

[StasJS]

Thanks for the update. @eiriktsarpalis do you see there being value in reconciling the documentation with current behaviour?
It sounds like any fix won't ship for quite some time.

[eiriktsarpalis]

Yes, I think we should be documenting what types are supported.

[JohnathanBarclay]

Since HashSet<T> and ISet<T> are supported, it's reasonable to also include IReadOnlySet<T>.

Other read-only collection types e.g. IReadOnlyCollection<T> & IReadOnlyList<T> are also supported, so this seems like an oversight.

[KrzysztofBranicki]

@eiriktsarpalis

Until this is addressed, my recommendation is to deserialize on models that use concrete collection types.

There is no concrete type ReadOnlySet<T> like there is e.g. ReadOnlyCollection<T>, also in this documentation documentation you say that ReadOnlyCollection<T> does not support deserialization anyway (which is a separate question on it's own why that is).

[eiriktsarpalis]

There is no concrete type ReadOnlySet like there is e.g. ReadOnlyCollection

Don't ImmutableHashSet<T> or ImmutableSortedSet<T> work?

[KrzysztofBranicki]

Don't ImmutableHashSet<T> or ImmutableSortedSet<T> work?

They do if you are willing to accept worst performance of those collections in scenarios where they are just used in DTOs (no expected mutations). And if someone doesn't care about performance he may just stick with Newtonsoft.Json.

[eiriktsarpalis]

If that is a concern, then HashSet<T> and SortedSet<T> are alternatives, the trade-off being that they expose mutable APIs.

[KrzysztofBranicki]

If that is a concern, then HashSet<T> and SortedSet<T> are alternatives, the trade-off being that they expose mutable APIs.

Mutable events are not acceptable in event-driven architecture.

[eiriktsarpalis]

How about using mutable DTOs that are separate from your domain events? Alternatively you could try implementing a custom read-only set that wraps a regular set.

[KrzysztofBranicki]

Thanks for the advice but, both options don't make sense in our use case. We will probably try to implement our own JsonConverter with hope that we will be able to remove it year from now when 9.0 is released.

[marekott]

Hi @eiriktsarpalis, I have come up with custom converter for IReadOnlySet as follows. Could you advice if you see any potential problems or corner cases where it could fail?

internal class ReadOnlySetConverterFactory : JsonConverterFactory
{
    private readonly Type readOnlySetType = typeof(IReadOnlySet<>);
    
    public override bool CanConvert(Type typeToConvert) => typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == readOnlySetType;

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
    {
        var genericTypeParameter = typeToConvert.GetGenericArguments()[0];
        var hashSetConverter = options.GetConverter(typeof(HashSet<>).MakeGenericType(genericTypeParameter));
        
        var converter = (JsonConverter)Activator.CreateInstance(
            type: typeof(ReadOnlySetConverter<>).MakeGenericType(genericTypeParameter), BindingFlags.Instance | BindingFlags.Public,
            binder: null,
            args: new object[] { hashSetConverter },
            culture: null)!;
        
        return converter;
    }
    
    private class ReadOnlySetConverter<TValue> : JsonConverter<IReadOnlySet<TValue>>
    {
        private readonly JsonConverter<HashSet<TValue>> hashSetConverter;
        private readonly Type hashSetType = typeof(HashSet<TValue>);

        public ReadOnlySetConverter(JsonConverter<HashSet<TValue>> hashSetConverter) => this.hashSetConverter = hashSetConverter;

        public override IReadOnlySet<TValue>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => 
            hashSetConverter.Read(ref reader, hashSetType, options);

        public override void Write(Utf8JsonWriter writer, IReadOnlySet<TValue> value, JsonSerializerOptions options) => 
            hashSetConverter.Write(writer, value.ToHashSet(), options);
    }
}

[eiriktsarpalis]

Superficially, this looks good to me. The only caveat is that it wouldn't work in Native AOT due to use of unsupported reflection.

[AbakumovAlexandr]

If that is a concern, then HashSet<T> and SortedSet<T> are alternatives, the trade-off being that they expose mutable APIs.

@eiriktsarpalis After reading the discussion above, I still can't answer this question: why the framework couldn't use HashSet where the declared type is IReadOnlySet at least for now (until performance issues of the immutable set are resolved)?

Is it just to prevent clients from breaking DTO's contract like this:

var set = (HashSet)dto.ReadOnlySetProperty;
set.Remove(...);// Do mutable operations on dto.ReadOnlySetProperty collection declared to be immutable

?

Ok. Then, the solution is: don't break the declared DTO contract.

But this proposal would perfectly fix the situation for 'legit' clients which doesn't brake the declared immutable contract and which don't try to cast a property of an immutable type to its mutable counterpart, wouldn't it?

[CyrusNajmabadi]

A value's actual runtime type is fair game for use.

[AbakumovAlexandr]

A value's actual runtime type is fair game for use.

@CyrusNajmabadi Ok, If somebody has the practice of using a runtime type instead of a declared one in a class's API, what's an issue with the proposal even in this case? If somebody wants it - let him do it. But at the same time let the others ('legit' clients as I referred them above) declare IReadOnlySet property and use it as a read-only set, if they want to.

Or one of the System.Text.Json design goals is to intentionally limit the use of practice you're considering as fair game? Then, the question would be: why, if it's fair game?

[CyrusNajmabadi]

what's an issue with the proposal even in this case?

Choosing a type that is ok for the consumer to mutate when the producer does not want that.

[AbakumovAlexandr]

@CyrusNajmabadi

Choosing a type that is ok for the consumer to mutate when the producer does not want that.

If it's something improper, then:

    public class HashSet<T> : ICollection<T>, ISet<T>, IReadOnlyCollection<T>, IReadOnlySet<T>, ISerializable, IDeserializationCallback

For what purpose does HashSet declare itself to be IReadOnlySet, i.e. ability to be used as the read-only set? According to your logic, this opens an ability for someone to check the runtime type and mutate the set while a producer does not want that.

Is it a wrong declaration which should be removed?

[CyrusNajmabadi]

HashSet can serve as a read-only set when the producer of the set can decide they are ok with it being mutated. It is not appropriate when the producer does not want that and that possibility is forced on them.

[AbakumovAlexandr]

@CyrusNajmabadi

It is not appropriate when the producer does not want that and that possibility is forced on them.

Could you provide some info on which read-only collection types is it forced on in System.Text.Json exactly?

It's not forced on IReadOnlyCollection properties:

It's not forced on IReadOnlyList properties:

System.Text.Json constructs mutable Lists for those at runtime.

[StasJS]

@CyrusNajmabadi I quite agree with @AbakumovAlexandr on this one. The ship has already sailed for the argument you are making via the JsonConverter treatment of existing readonly collections.

@CyrusNajmabadi I would like to hear what solution you would propose for this issue. Hopefully you agree that the current state is awkward and it could be better?

[CyrusNajmabadi]

I would like to hear what solution you would propose for this issue

I would have it deserialize into a ReadOnlySet<T>: #103306

[StasJS]

@CyrusNajmabadi yes that recent PR introducing ReadOnlySet seems like the perfect solve for this I would think. Is anyone in a position to put a code change together? I can give it a go at some point soon but have not contributed to this repo before.

[eiriktsarpalis]

Keep in mind that ReadOnlySet<T> is only available in net9.0 whereas STJ needs to support net8.0. For consistency with how list and dictionary interfaces are handled, HashSet<T> would be the preferred implementation.

[StasJS]

If HashSet is picked to meet dotnet 8 timelines presumably it can't be changed for dotnet 9 and beyond to what would be the natural/obvious choice once ReadOnlySet ships due to risk of breaking code along the lines @CyrusNajmabadi has mentioned?

That sounds a little awkward

[eiriktsarpalis]

From my perspective using mutable implementations is not a huge issue from the perspective of deserialization. The deserializer produces the type and passes ownership off to the caller once completed. If the caller wants to take the risk of downcasting to the runtime type to make mutations it's on them.

[AbakumovAlexandr]

If HashSet is picked to meet dotnet 8 timelines presumably it can't be changed for dotnet 9 ... due to risk of breaking code along the lines @CyrusNajmabadi has mentioned?

That sounds a little awkward

I don't think .NET Framework \ .NET Core ever guaranteed compatibility across versions when a runtime type is used instead of a declared contract type. To my knowledge, it always was solely on a client code risk.

Honestly, I'd be surprised if any class library project would pick covering such the practice as its design goal. It always looks like a direct invasion into internal undocumented framework implementation details and shouldn't be guaranteed, at least from a maintainability perspective.

[eiriktsarpalis]

Moving 10.0.0. This should be addressed holisitically including extending support for all collection types, adding support for CollectionBuilderAttribute and IEnumerable constructors (#80688)