JsonObjectCreationHandling.Populate not working with parameterized constructors

[eiriktsarpalis]

Thanks, this looks like a real bug. FWIW it's not specific to primary constructors or records, this fails too:

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

var singleUserJson = """{"Username":"Filip","PhoneNumbers":["123456"]}""";
var userFromJson = JsonSerializer.Deserialize<User>(singleUserJson);
Console.WriteLine(userFromJson.PhoneNumbers.Count); // 0

public class User
{
    public User(string name)
        => Name = name;

    public string Name { get; }

    [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
    public List<string> PhoneNumbers { get; } = new();
}

Originally posted by @eiriktsarpalis in dotnet/docs#37329 (comment)

[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

---

Thanks, this looks like a real bug. FWIW it's not specific to primary constructors or records, this fails too:

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

var singleUserJson = """{"Username":"Filip","PhoneNumbers":["123456"]}""";
var userFromJson = JsonSerializer.Deserialize<User>(singleUserJson);
Console.WriteLine(userFromJson.PhoneNumbers.Count); // 0

public class User
{
    public User(string name)
        => Name = name;

    public string Name { get; }

    [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
    public List<string> PhoneNumbers { get; } = new();
}

Originally posted by @eiriktsarpalis in dotnet/docs#37329 (comment)

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

[fekberg]

It behaves the same even if you mark the parameter as optional (default) value.

Here's another case where it does not work, when using in combination with JsonDerivedType and JsonPolymorphic.

If you create a set of records like this:

[JsonDerivedType(typeof(User), typeDiscriminator: "user")]
[JsonDerivedType(typeof(InactiveUser), typeDiscriminator: "inactive")]
[JsonDerivedType(typeof(DisabledUser), typeDiscriminator: "disabled")]
[JsonPolymorphic(TypeDiscriminatorPropertyName = "$discriminator")]
[JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
record User
{
    public string Username { get; set; }

    public List<string> PhoneNumbers { get; } = new();
}

record DisabledUser(DateTimeOffset DisabledSince)
    : User();

record InactiveUser(DateTimeOffset InactiveSince) 
    : User();

The following will not populate PhoneNumbers:

var jsonAsString = """
[
    {
        "$discriminator": "user",
        "Username": "Filip",
        "PhoneNumbers": [
            "12345"
        ]
    },
    {
        "$discriminator": "inactive",
        "InactiveSince": "2023-10-02T11:53:24.2010948+00:00",
        "Username": "Sofie",
        "PhoneNumbers": [
            "6789"
        ]
    },
    {
        "$discriminator": "disabled",
        "DisabledSince": "2023-10-02T11:53:24.2011285+00:00",
        "Username": "Mila",
        "PhoneNumbers": []
    },
    {
        "$discriminator": "disabled",
        "DisabledSince": "2023-10-02T11:53:24.2011561+00:00",
        "Username": "Elise",
        "PhoneNumbers": []
    }
]    
""";

var usersFromJson = JsonSerializer.Deserialize<User[]>(jsonAsString);

If you change the User record to instead define the JsonObjectCreationHandling on the property, it works for the base User, but not for the derived InactiveUser (because it has a constructor parameter):

[JsonDerivedType(typeof(User), typeDiscriminator: "user")]
[JsonDerivedType(typeof(InactiveUser), typeDiscriminator: "inactive")]
[JsonDerivedType(typeof(DisabledUser), typeDiscriminator: "disabled")]
[JsonPolymorphic(TypeDiscriminatorPropertyName = "$discriminator")]
record User
{
    public string Username { get; set; }

    [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
    public List<string> PhoneNumbers { get; } = new();
}

[krwq]

The bug is here:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs#L338

It should be CanDeserializeOrPopulate - possibly more places need similar change.

[eiriktsarpalis]

On closer inspection, the problem appears to be less trivial than I originally thought. The converter used for serializing types with parameterized constructors will deserialize both constructor parameters and properties before it instantiates the deserialized object.

This is not accidental, instantiating the object depends on all constructor parameters being deserialized, but at the same time deserializing properties with populate semantics depends on the object being instantiated ahead of time, so the two requirements are fundamentally in conflict.

I can't think of any good fixes other than buffering the object's JSON payload and replaying deserialization for properties after instantiation has completed, however that would require a substantial rearchitecting of the parameterized constructor implementation which is probably too risky for a .NET 8 servicing update. My recommendation is to simply mark populate semantics as unsupported in .NET 8 and work on a proper fix in future releases.

[krwq]

Current problem can be worked around like this:

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

JsonSerializerOptions options = new()
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver()
    {
        Modifiers = { PopulateForCollectionsAndDictionariesFix }
    }
};
var singleUserJson = """{"Name":"Filip","PhoneNumbers":["123456"]}""";
var userFromJson = JsonSerializer.Deserialize<User>(singleUserJson, options);
Console.WriteLine(userFromJson.PhoneNumbers.Count); // 1


static void PopulateForCollectionsAndDictionariesFix(JsonTypeInfo typeInfo)
{
    foreach (var property in typeInfo.Properties)
    {
        if (property.ObjectCreationHandling == JsonObjectCreationHandling.Populate && property.Set == null)
        {
            // adding a setter causes CanDeserialize flag to be true and populate to work
            property.Set = (obj, val) => throw new JsonException("Setter should not be used since this property is Populate");
        }
    }
}

public class User
{
    public User(string name)
        => Name = name;

    public string Name { get; }

    [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
    public List<string> PhoneNumbers { get; } = new();
}

I'd prefer we rather went ahead with doing nothing and allow people taking workaround until fix comes in than disallowing this entirely. I think the entire fix is literally replacing CanDeserialize with CanDeserializeAndPopulate in the file I mentioned earlier.

[eiriktsarpalis]

@krwq this doesn't work if you try to use async:

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

var stream = new MemoryStream("""{"Name":"Filip","PhoneNumbers":["123456"]}"""u8.ToArray());
var userFromJson = await JsonSerializer.DeserializeAsync<User>(stream, options); // exception


static void PopulateForCollectionsAndDictionariesFix(JsonTypeInfo typeInfo)
{
    foreach (var property in typeInfo.Properties)
    {
        if (property.ObjectCreationHandling == JsonObjectCreationHandling.Populate && property.Set == null)
        {
            // adding a setter causes CanDeserialize flag to be true and populate to work
            property.Set = (obj, val) => throw new JsonException("Setter should not be used since this property is Populate");
        }
    }
}

public class User
{
    public User(string name)
        => Name = name;

    public string Name { get; }

    [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)]
    public List<string> PhoneNumbers { get; } = new();
}

Fundamentally the issue lies with async deserialization and how it handles parameterized constructors:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs

Lines 207 to 213 in d3569b9

	if (argumentState.FoundPropertyCount > 0)
	{
	for (int i = 0; i < argumentState.FoundPropertyCount; i++)
	{
	JsonPropertyInfo jsonPropertyInfo = argumentState.FoundPropertiesAsync![i].Item1;
	object? propValue = argumentState.FoundPropertiesAsync![i].Item2;
	string? dataExtKey = argumentState.FoundPropertiesAsync![i].Item3;

[eiriktsarpalis]

#92937 added validation explicitly prohibiting the scenario, moving to 9.0.0 for a proper fix.

[EnCey]

It's unfortunate that this won't be in .NET 9 because it's one of the two reasons that force us to create boilerplate constructors: the first is nullability, which has been addressed in preview 6 (🎉) and the other is creating a collection with a specific comparer.

Since it's not mentioned here or in the docs I want to point it out, as it is a frequent use case for us and requires various workarounds in STJ.

We frequently deal with case-insensitive dictionaries or sets and need to create a parameterized JSON constructor simply to deep-copy the dictionary/set created by STJ into one that uses the correct comparer. If we were able to pre-populate the collection property, the need for boilerplate code and deep copies would go away.

[eiriktsarpalis]

@EnCey would you be interested in contributing a fix? We have about 3 weeks left before .NET 9 development freezes.

[EnCey]

What would be an acceptable fix @eiriktsarpalis ?

Based on your post above:

I can't think of any good fixes other than buffering the object's JSON payload and replaying deserialization for properties after instantiation has completed, however that would require a substantial rearchitecting of the parameterized constructor implementation which is probably too risky for a .NET 8 servicing update […]

I suspect that I wouldn't be able to implement such a fundamental change (in time) that meets your quality targets. I'm not familiar with the code base, but I assume performance and memory use are very important to you and a naive solution won't fly.

If the use case of case-insensitive collections is deemed important enough, perhaps a simpler solution would be acceptable? Something like an attribute or option that allows a user to specify a comparer for collections, used by STJ when instantiating the collection?

[eiriktsarpalis]

Something like an attribute or option that allows a user to specify a comparer for collections, used by STJ when instantiating the collection?

That would require adding new API, unfortunately we're out of time when it comes to adding new features to .NET 9.