System.Text.Json deserialization failing on classes with field encapsulation

[KrzysztofBranicki]

Description

When creating class that represents domain entity when we want to encapsulate collection so that it is impossible to add item to the collection without executing code that is responsible for validating domain invariants, we get exception on deserialization. Exception is thrown despite all names in the one and only constructor being correct.

Classes being serialized:

public class Invoice
{
    private  List<InvoiceItem> items;

    public IReadOnlyCollection<InvoiceItem> Items => items;

    public Invoice(List<InvoiceItem> items)
    {
        this.items = items;
    }

    public void AddItem(InvoiceItem invoiceItem)
    {
        //Some domain logic validation
        items.Add(invoiceItem);
    }
}

public class InvoiceItem
{ }

Serialization code:

var invoice = new Invoice(new List<InvoiceItem>());
var serialized = JsonSerializer.Serialize(invoice);
var deserialized = JsonSerializer.Deserialize<Invoice>(serialized);

Exception:

Unhandled exception. System.InvalidOperationException: Each parameter in constructor 'Void .ctor(System.Collections.Generic.List`1[SerializationTest.InvoiceItem])' on type 'SerializationTest.Invoice' mus
t bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. The match can be case-insensitive.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(ConstructorInfo constructorInfo, Type parentType)
   at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& val
ue)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at SerializationTest.Program.Main(String[] args) in C:\Users\kbranicki\RiderProjects\SerializationTest\SerializationTest\Program.cs:line 13

Configuration

.NET SDK version 5.0.202
Not listing OS or processor architecture as I don't think this is related to specific OS/hardware configuration.

Regression?

I have no idea if this worked in previous version of System.Text.Json so I don't know if it is regression. It definitely works in Newtonsoft.Json without any special configuration.

Other information

I assume that problem lies in constructor parameter (List<InvoiceItem>) having different type than the public property (IReadOnlyCollection<InvoiceItem>) that is exposing encapsulated list. So theoretically I can change constructor parameter to be IReadOnlyCollection<InvoiceItem> and do ToList() but this will do an extra allocations and defeat the whole purpose of migrating from Newtonsoft.Json as the purpose is performance improvements (because it's clearly not the features :)).

I know that System.Text.Json was never meant to be one to one replacement for Newtonsoft.Json but this is not some egzotic feature I'm describing here. It is relevant to everyone who wants to model their Aggregates according to DDD best practices and needs to serialize them for persistence in DB. Also I never saw in any docs/articles stating that the only scenario in mind when designing System.Text.Json was serializing DTOs.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When creating class that represents domain entity when we want to encapsulate collection so that it is impossible to add item to the collection without executing code that is responsible for validating domain invariants, we get exception on deserialization. Exception is thrown despite all names in the one and only constructor being correct.

Classes being serialized:

public class Invoice
{
    private  List<InvoiceItem> items;

    public IReadOnlyCollection<InvoiceItem> Items => items;

    public Invoice(List<InvoiceItem> items)
    {
        this.items = items;
    }

    public void AddItem(InvoiceItem invoiceItem)
    {
        //Some domain logic validation
        items.Add(invoiceItem);
    }
}

public class InvoiceItem
{ }

Serialization code:

var invoice = new Invoice(new List<InvoiceItem>());
var serialized = JsonSerializer.Serialize(invoice);
var deserialized = JsonSerializer.Deserialize<Invoice>(serialized);

Exception:

Unhandled exception. System.InvalidOperationException: Each parameter in constructor 'Void .ctor(System.Collections.Generic.List`1[SerializationTest.InvoiceItem])' on type 'SerializationTest.Invoice' mus
t bind to an object property or field on deserialization. Each parameter name must match with a property or field on the object. The match can be case-insensitive.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(ConstructorInfo constructorInfo, Type parentType)
   at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& val
ue)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at SerializationTest.Program.Main(String[] args) in C:\Users\kbranicki\RiderProjects\SerializationTest\SerializationTest\Program.cs:line 13

Configuration

.NET SDK version 5.0.202
Not listing OS or processor architecture as I don't think this is related to specific OS/hardware configuration.

Regression?

I have no idea if this worked in previous version of System.Text.Json so I don't know if it is regression. It definitely works in Newtonsoft.Json without any special configuration.

Other information

I assume that problem lies in constructor parameter (List<InvoiceItem>) having different type than the public property (IReadOnlyCollection<InvoiceItem>) that is exposing encapsulated list. So theoretically I can change constructor parameter to be IReadOnlyCollection<InvoiceItem> and do ToList() but this will do an extra allocations and defeat the whole purpose of migrating from Newtonsoft.Json as the purpose is performance improvements (because it's clearly not the features :)).

I know that System.Text.Json was never meant to be one to one replacement for Newtonsoft.Json but this is not some egzotic feature I'm describing here. It is relevant to everyone who wants to model their Aggregates according to DDD best practices and needs to serialize them for persistence in DB. Also I never so in any docs/articles that the only scenario in mind when designing System.Text.Json was serializing DTOs.

Author:	KrzysztofBranicki
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[Tornhoof]

You can do something like this, as STJ uses lists for deserialization of ReadOnlyCollection or ReadOnlyList (is there a reason you use ROC instead of the ROL?). Unfortunately as the ctor needs to be public, you could hide the ctor from your UI via ObsoleteAttribute. Still I agree that the check for the types should be IsAssignableFrom

public Invoice(List<InvoiceItem> items)
{
	this.items = items;
}

[JsonConstructor]
[Obsolete]
public Invoice(IReadOnlyCollection<InvoiceItem> items)
{
	this.items = items as List<InvoiceItem> ?? this.items.ToList();
}

[KrzysztofBranicki]

Actually in the codebase where I have this problem the type of the property is ROL. I used ROC in the reproduction sample to make the problem even more apparent but I expect this to work exactly same way no matter if my public property is ROL, ROC or IEnumerable. Agree that this limitation can be overcome with some additional code that developer needs to remember about but I have project that is big enough (in terms of number of developers and size of codebase) that I can't give the go-ahead to make this change because it is simply not worth extra comotion and potential bugs.

[Tornhoof]

Agree that this limitation can be overcome with some additional code that developer needs to remember about but I have project that is big enough (in terms of number of developers and size of codebase) that I can't give the go-ahead to make this change because it is simply not worth extra comotion and potential bugs.

Not saying you should, but if you want to reduce the developer overhead and need go that route, go look the source generator direction, imho that's a classic example for that, adding boilerplate code.

[KrzysztofBranicki]

Yes it can be done with source generators but I'm not a fan of having source generators as part of domain model, especially in a case where this boilerplate isn't really justified by anything other than a missing feature in a framework that we don't really need to use. We can stay with Newtonsoft.Json and avoid unnecessary boilerplate (generated or not). Just wanted to point out this unsupported scenario to appropriate team. If the team responsible wants to increase adoption of this serializer then this scenario should be seriously considered as everyone who is doing DDD (or actually any kind of proper OOP) and is using serializer to serialize the domain model (not just DTOs) will stumble on it.

[eiriktsarpalis]

Still I agree that the check for the types should be IsAssignableFrom.

That would certainly fix the particular example, since both type and subtype happen to utilize identical JSON constracts. However it might not be roundtrippable general case, as might be demonstrated in this (artificial) example:

public class MyClass
{
    public object MyProperty => "string";

    public MyClass(int myProperty) { }
}

We've received related feedback in #43563 and #54854. I'm wondering if it might make sense to expose a setting that switches off constructor parameter validation, as there are many valid use cases where it might be obstructing users cc @layomia.

[layomia]

I believe the requirements of this issue are generally covered by #44428. I'll close this issue as a duplicate.

@eiriktsarpalis yes I believe such a switch would be beneficial as an "I know what I'm doing" mode. This is considered in #44428.