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

dotnet 7 System.Text.Json JsonSerializer.Serialize exception: is marked required but does not specify a setter. #82879

Closed
kzhui125 opened this issue Mar 2, 2023 · 15 comments

Comments

@kzhui125
Copy link

kzhui125 commented Mar 2, 2023

Description

using JsonSerializer.Serialize to Serialize object.

Reproduction Steps

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

TestDto t = new() { Items = new() };

string jsonString = JsonSerializer.Serialize(t);
Console.WriteLine(jsonString);

public class TestDto
{
    public string? Field1 { get; set; }
    [JsonIgnore]
    public required List<string> Items { get; set; }
}

Expected behavior

no exception

Actual behavior

exception throws

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 2, 2023
@ghost
Copy link

ghost commented Mar 2, 2023

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

using JsonSerializer.Serialize to Serialize object.

Reproduction Steps

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

TestDto t = new() { Items = new() };

string jsonString = JsonSerializer.Serialize(t);
Console.WriteLine(jsonString);

public class TestDto
{
    public string? Field1 { get; set; }
    [JsonIgnore]
    public required List<string> Items { get; set; }
}

Expected behavior

no exception

Actual behavior

exception throws

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: kzhui125
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@krwq krwq added this to the 8.0.0 milestone Mar 2, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Mar 2, 2023
@krwq krwq added bug untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Mar 2, 2023
@eiriktsarpalis
Copy link
Member

I believe this is by design. The required keyword is picked up by the serializer and has special meaning (equivalent to JsonRequiredAttribute). However "required" is the exact opposite to "ignore" so the two configurations are mutually exclusive. If the property needs to be ignored you should remove the required keyword. If the property is required for initialization then you should remove the JsonIgnore annotation.

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

using Newtonsoft.Json;

TestDto t = new() { Field1 = "Value1", Items = new() };
List<TestDto> ts = new() { t };

string json = JsonConvert.SerializeObject(t);
Console.WriteLine();

public class TestDto
{
    public required string Field1 { get; set; }

    [JsonIgnore]
    public required List<string> Items { get; set; }
}

when using Newtonsoft.Json it works

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

The property is required for object initialization (what the required keyword mean), but not required for json serialization.

They are two different things.

@eiriktsarpalis
Copy link
Member

I would guess that's because Json.NET doesn't support required properties yet.

@eiriktsarpalis
Copy link
Member

The property is required for object initialization (what the required keyword mean), but not required for json serialization.

They are two different things.

They are not. Deserialization if anything does perform object initialization, albeit indirectly. You can always bypass required/init properties when using reflection (which is why you see this working in the case of Json.NET), however you can't do it in the case of source generators. STJ honors required properties both because it preserves invariants of the deserialized class and because it has to in the case of source generators.

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

Deserialization throws exception make sense. since deserialization will create object, and the required modifier should be respected.

But serialization is object to json. The object is already created(required modifier was also respected). Serialization throw exception make people confuse, since it doesn't create object, it only create string from concat object properties.

I meet this issue when I want to return dictionary as json to frontend, but I don't want to return the original list (duplicate).

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

SomeVm vm = new() { SomeDtos = new() };
foreach (var dto in vm.SomeDtos)
{
    dto.ItemsDict = dto.Items
        .ToDictionary(i => i.Key, i => i);
}

string jsonString = JsonSerializer.Serialize(vm);
Console.WriteLine(jsonString);


public class SomeVm
{
    public required List<SomeDto> SomeDtos { get; set; }
    public string? Field1 { get; set; }
}

public class SomeDto
{
    public string? Field1 { get; set; }

    [JsonIgnore]
    public required List<SomeItemDto> Items { get; set; }
    public Dictionary<string, SomeItemDto>? ItemsDict { get; set; }
}

public class SomeItemDto
{
    public required string Key { get; set; }
    public string? Value { get; set; }
}

Thanks for your help.

@eiriktsarpalis
Copy link
Member

JsonIgnoreAttribute is not specific to serialization -- it also controls deserialization. It's precisely this mode (disabling deserialization for a property marked required) that triggers the error you are seeing.

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

Maybe JsonIgnore should have higher priority than C# required modifier?

If have JsonIgnore , then required is omitted.

Or we should have more detailed documentation.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Mar 2, 2023

Maybe JsonIgnore should have higher priority than C# required modifier?
If have JsonIgnore , then required is omitted.

That's not something that would work with the source generator.

Or we should have more detailed documentation.

Does this work? https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/required-properties

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

The page talks about JsonSerializer.Deserialize will throw exception, but doesn’t mention JsonSerializer.Serialize

@eiriktsarpalis
Copy link
Member

The page talks about JsonSerializer.Deserialize will throw exception, but doesn’t mention JsonSerializer.Serialize

The error stems from validating the JSON contract for the type, which happens during both serialization and deserialization operations.

@kzhui125
Copy link
Author

kzhui125 commented Mar 2, 2023

image

maybe we should update the document, to make it clear.

@krwq
Copy link
Member

krwq commented Mar 2, 2023

@kzhui125 you can use this workaround to fix this for reflection serializer:
#68895 (comment)
The contract resolver modifier makes it so that serializer cannot see ignored properties and therefore it won't know about the compatibility and since required is not enforced with reflection it will just work.

The reason this is not working by default is because that will never work with source gen since it needs to generate constructor and that code won't compile without required property.

I'll close this issue as by design because the fact serializer can workaround this doesn't mean it's the right thing to do.

@krwq krwq closed this as completed Mar 2, 2023
@eiriktsarpalis eiriktsarpalis removed the bug label Mar 2, 2023
@eiriktsarpalis
Copy link
Member

image

maybe we should update the document, to make it clear.

That's not accurate. As mentioned the exception stems from contract validation and is not part of the serialization contract for required properies. This should be evident from the stacktrace of the reproduction:

Unhandled exception. System.InvalidOperationException: JsonPropertyInfo 'Items' defined in type 'TestDto' 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.GetTypeInfo[T](JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)

We might want to update the documentation for JsonPriopertyInfo.IsRequired to mention that it is not compatible with properties that don't have a setter or are ignoring their setter.

@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Mar 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants