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

S.T.J.JsonSerializer doesn't support properties marked with JsonRequiredAttribute that're initialized through ctor #78098

Open SicJG opened 1 year ago

SicJG commented 1 year ago

Description

  1. Using [JsonRequiredAttribute] published with .Net 7.0, on propertry that doesn't have public setter/initter, but initialized through ctor throws InvalidOperationException during deserialization
  2. Obligation of property setter/initter existance is not described in documention 1 2

Reproduction Steps

using System.Text.Json.Serialization;

namespace Test;

internal static class JsonRequired
{
    public sealed class RequiredAttributeCtor
    {
        [JsonPropertyName("value")]
        [JsonRequired]
        public int Value { get; }

        public RequiredAttributeCtor(int value)
        {
            Value = value;
        }
    }

    public static RequiredAttributeCtor? Run()
    {
        const string input = """{"value":1}""";
        return System.Text.Json.JsonSerializer.Deserialize<RequiredAttributeCtor>(input);
    }

}

Expected behavior

Local

Reproduction test returns instance of RequiredAttributeCtor type populated with data from input

Global

All of the required property initialization variations are supported


using System.Text.Json.Serialization;

namespace Test;

internal static class JsonRequired
{
    public sealed class RequiredAttributeCtor
    {
        [JsonPropertyName("value")]
        [JsonRequired]
        public int Value { get; }

        public RequiredAttributeCtor(int value)
        {
            Value = value;
        }
    }

    public sealed class RequiredAttributeSet
    {
        [JsonPropertyName("value")]
        [JsonRequired]
        public int Value { get; set; }
    }

    public sealed class RequiredAttributeInit
    {
        [JsonPropertyName("value")]
        [JsonRequired]
        public int Value { get; init; }
    }

    public sealed class RequiredKeywordSet
    {
        [JsonPropertyName("value")]
        public required int Value { get; set; }
    }

    public sealed class RequiredKeywordInit
    {
        [JsonPropertyName("value")]
        public required int Value { get; init; }
    }

    public static void Run()
    {
        const string input = """{"value":1}""";
        var _1 =  System.Text.Json.JsonSerializer.Deserialize<RequiredAttributeCtor>(input);
        var _2 =  System.Text.Json.JsonSerializer.Deserialize<RequiredAttributeSet>(input);
        var _3 =  System.Text.Json.JsonSerializer.Deserialize<RequiredAttributeInit>(input);
        var _4 =  System.Text.Json.JsonSerializer.Deserialize<RequiredKeywordSet>(input);
        var _5 =  System.Text.Json.JsonSerializer.Deserialize<RequiredKeywordInit>(input);
    }
}

Actual behavior

Running reproduction test throws

Unhandled exception. System.InvalidOperationException: JsonPropertyInfo 'value' defined in type 'Test.JsonRequired+RequiredAttributeCtor' 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.InitializePropertyCache()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureLocked|143_0()
   at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Boolean resolveIfMutable)
   at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type inputType)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Test.JsonRequired.Run() in C:\Projects\Sandbox\Test\JsonRequiredWithCtor.cs:line 48

Regression?

No response

Known Workarounds

No response

Configuration

SDK version: 7.0.100 OS: Windows 10 1909 Arch: x64

Other information

It seems like implementation of required keyword support in https://github.com/dotnet/runtime/pull/72937 was made in way that makes JsonRequiredAttribute usage absolutely simular to new "required" keyword usage, missing the fact that validating absence of input json property using attribute might be used in other scenarios.

ghost commented 1 year ago

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 1. Using [[JsonRequiredAttribute]](https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonrequiredattribute?view=net-7.0) published with .Net 7.0, on propertry that doesn't have public setter/initter, but initialized through ctor throws InvalidOperationException 2. Obligation of property setter/initter existance is not described in documention [1](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/required-properties) [2](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/required-properties) ### Reproduction Steps ```csharp using System.Text.Json.Serialization; namespace Test; internal static class JsonRequired { public sealed class RequiredAttributeCtor { [JsonPropertyName("value")] [JsonRequired] public int Value { get; } public RequiredAttributeCtor(int value) { Value = value; } } public static RequiredAttributeCtor? Run() { const string input = """{"value":1}"""; return System.Text.Json.JsonSerializer.Deserialize(input); } } ``` ### Expected behavior ### Local Reproduction test returns instance of RequiredAttributeCtor type populated with data from input ### Global All of the required property initialization variations are supported ```csharp using System.Text.Json.Serialization; namespace Test; internal static class JsonRequired { public sealed class RequiredAttributeCtor { [JsonPropertyName("value")] [JsonRequired] public int Value { get; } public RequiredAttributeCtor(int value) { Value = value; } } public sealed class RequiredAttributeSet { [JsonPropertyName("value")] [JsonRequired] public int Value { get; set; } } public sealed class RequiredAttributeInit { [JsonPropertyName("value")] [JsonRequired] public int Value { get; init; } } public sealed class RequiredKeywordSet { [JsonPropertyName("value")] public required int Value { get; set; } } public sealed class RequiredKeywordInit { [JsonPropertyName("value")] public required int Value { get; init; } } public static void Run() { const string input = """{"value":1}"""; var _1 = System.Text.Json.JsonSerializer.Deserialize(input); var _2 = System.Text.Json.JsonSerializer.Deserialize(input); var _3 = System.Text.Json.JsonSerializer.Deserialize(input); var _4 = System.Text.Json.JsonSerializer.Deserialize(input); var _5 = System.Text.Json.JsonSerializer.Deserialize(input); } } ``` ### Actual behavior Running reproduction test throws ``` Unhandled exception. System.InvalidOperationException: JsonPropertyInfo 'value' defined in type 'Test.JsonRequired+RequiredAttributeCtor' 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.InitializePropertyCache() at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure() at System.Text.Json.Serialization.Metadata.JsonTypeInfo.g__ConfigureLocked|143_0() at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Boolean resolveIfMutable) at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type inputType) at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options) at Test.JsonRequired.Run() in C:\Projects\Sandbox\Test\JsonRequiredWithCtor.cs:line 48 ``` ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration SDK version: 7.0.100 OS: Windows 10 1909 Arch: x64 ### Other information It seems like implementation of required keyword support in https://github.com/dotnet/runtime/pull/72937 was made in way that makes JsonRequiredAttribute usage absolutely simular to new "required" keyword usage, missing the fact that validating absence of input json property using attribute might be used in other scenarios.
Author: SicJG
Assignees: -
Labels: `area-System.Text.Json`
Milestone: -
krwq commented 1 year ago

Only scenario 1 is not working currently. I think this is caused by artificial check I added which prevents from using read-only properties with [Required] - seems I missed the case with parametrized ctor + read-only property test case. I think the fix is simply adjusting that check + tests.

krwq commented 1 year ago

@SicJG thanks for the report on this, I've sent a PR with a fix. Once it's merged you should be able to consume nightly NuGet package with a fix some time after that. I'll check if it qualifies for servicing fix for 7.0 but no promises on that.

eiriktsarpalis commented 1 year ago

I don't believe this meets the bar for a 7.0 fix. JsonRequiredAttribute can only be applied to properties and has no effect on constructor parameters, even though the serializer will -by convention- associate constructor parameters to property getters when deriving constructor parameter metadata. The current behavior is consistent with how the C# compiler treats the required keyword in getter-only properties, for example the following does not compile:

public class MyPoco
{
    public MyPoco(int value) => Value = value;

    public required int Value { get; }
}

That being said, we should make sure that JsonRequiredAttribute can be applied consistently to constructor parameters in the future -- however this should probably be implemented in the context of https://github.com/dotnet/runtime/issues/71944 as there is a lot of nuance to be accounted for.

MartyIX commented 1 year ago

Unfortunately, this is a blocker for us to use the JsonRequiredAttribute feature on .NET 7. Pity.

krwq commented 1 year ago

@MartyIX the isolated fix is in the PR https://github.com/dotnet/runtime/pull/78152.

There is also workaround which I didn't mention yet:

using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;

JsonSerializerOptions options = new()
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver()
    {
        Modifiers = { JsonRequiredWithPropertiesWithoutSetter }
    }
};

MyPoco obj = JsonSerializer.Deserialize<MyPoco>("""{"Value": 123}""", options);
Console.WriteLine(obj.Value);

void JsonRequiredWithPropertiesWithoutSetter(JsonTypeInfo typeInfo)
{
    if (typeInfo.Kind != JsonTypeInfoKind.Object)
        return;

    foreach (var property in typeInfo.Properties)
    {
        if (property.IsRequired && property.Set == null)
        {
            // this will never be called for parameterized constructors
            property.Set = (obj, val) => throw new JsonException("Required property doesn't have setter");
        }
    }
}

public class MyPoco
{
    public MyPoco(int value) => Value = value;

    [JsonRequired]
    public int Value { get; }
}
eiriktsarpalis commented 1 year ago

Moving to 9.0 per https://github.com/dotnet/runtime/pull/78152#issuecomment-1638468540

SicJG commented 1 month ago

Might be considered as closed due to implementation of JsonSerializerOptions.RespectRequiredConstructorParameters as well as JsonSerializerOptions.RespectNullableAnnotations Thank you for this functionality!

eiriktsarpalis commented 1 month ago

It seems we might still want to make this work if the flag hasn't been enabled though. Let's leave this open for now.