dotnet / runtime

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

Unable to deserialize JSON into a collection property without a getter (with a misleading error message) #104700

Open ezhevita opened 3 months ago

ezhevita commented 3 months ago

Description

When trying to deserialize a JSON string into a class containing a property with a collection type (Dictionary or IEnumerable) serializer throws an exception which says about a property missing a setter despite it being present.

Reproduction Steps

JsonSerializer.Deserialize<Test>("{\"Data\":[1]}");

public class Test
{
    [JsonRequired]
    public List<int> Data
    {
        set => FirstValue = value[0];
    }

    [JsonIgnore]
    public int FirstValue { get; private set; }
}

Expected behavior

JSON is deserialized correctly.

Actual behavior

The following exception is thrown:

System.InvalidOperationException: JsonPropertyInfo 'Data' defined in type 'Test' is marked required but does not specify a setter.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(JsonPropertyInfo jsonPropertyInfo)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureSynchronized|172_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
   at Program.<Main>$(String[] args)

Regression?

No, this behavior is present at least since .NET 7 when required properties were introduced.

Known Workarounds

Adding a getter to the property, it is allowed for it to return null or just outright to throw an exception.

Configuration

.NET 8.0.6 running on macOS 14.5 ARM64; issue is not specific to the configuration.

Other information

Similar to (although different scenario): https://github.com/dotnet/runtime/issues/92330

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.

eiriktsarpalis commented 3 months ago

I can reproduce, thanks. One workaround is to specify a getter it seems.

julien98Qc commented 3 months ago

Indeed, properties of collection types that only have setters are not supported.

In DetermineSerializationCapabilities(), CanDeserialize is set to false when an Enumerable or Dictionary has no getter. In Configure(), we trigger the exception based on IsRequired and CanDeserialize. The exception message assumes it is a setter problem, but that is not always the case.

The resolution might be one of the following:

I would go for the other verification in my humble opinion. I could work on this, just tell me which direction to go.

julien98Qc commented 3 months ago

@eiriktsarpalis Just wanted to let you know that I've made a PR for this issue.

julien98Qc commented 3 months ago

The issue is not about a misleading error message anymore but to make it work when required and no getter specified. Since then, I'm really confused as why it was purposefuly dermined not deserializable in DetermineSerializationCapabilities(). I don't have enough time to investigate further.

See this comment below for more context : https://github.com/dotnet/runtime/pull/104861#pullrequestreview-2204684810

ezhevita commented 3 months ago

I've updated the issue title to reflect that the expected solution is making this scenario work.

devsko commented 1 month ago

I don't think this is the correct way to address the problem. A fix should instead try to make the scenario work as expected. If you remove JsonRequired from the repro in #104700 then deserialization works as expected and this should too.

@eiriktsarpalis are you aware that the setter is not called even when [JsonRequired] is removed. No exception but still unexpected.

The following

Console.WriteLine(JsonSerializer.Deserialize<Test>("{\"Data\":[1]}")?.FirstValue);

public class Test
{
    //[JsonRequired]
    public List<int> Data
    {
        //get => throw new Exception();
        set => FirstValue = value[0]; // Is NOT called as long as no getter is declared
    }

    [JsonIgnore]
    public int FirstValue { get; private set; }
}
ShawnWu33 commented 3 weeks ago

Hi I am trying to looking into this issue as got some time in upcoming few days.

Upon a shallow investigation, the following logic in DetermineSerializationCapabilities() is where the CanDeserilize being set false.

if ((EffectiveConverter.ConverterStrategy & (ConverterStrategy.Enumerable | ConverterStrategy.Dictionary)) != 0)
{
    // Properties of collections types that only have setters are not supported.
    if (Get == null && Set != null && !_isUserSpecifiedSetter)
    {
        CanDeserialize = false;
    }
}

The comment // Properties of collections types that only have setters are not supported. is introduced in #72186. @eiriktsarpalis Could you shed a light on which direction you consider the proper fix should be? Do we want to fix what's underlying to let collection type that only have setters be supported?

Btw Should _isUserSpecifiedSetter being set as true in this case? Its false when running above case.