[JsonSerializer] Relax restrictions on ctor param type to immutable property type matching where reasonable

JSkimming commented 3 years ago

Description

I'm trying to deserialise an object from a json string. In this case the constructor allows a nullable value type as the parameter, setting a non-null property (defaulting if null).

I expect (since it is the behaviour with Newtonsoft.Json) the serializer to handle compatible constructor and bound value type properties (or fields) where the only difference is one is nullable.

The following demonstrates the issue:

public class Example
{
    public Example(Guid? aGuid) => AGuid = aGuid ?? Guid.Empty;
    public Guid AGuid { get; }
}

Example original = new Example(Guid.NewGuid());

// Works with Newtonsoft.Json.
string withJsonNet = JsonConvert.SerializeObject(original);
JsonConvert.DeserializeObject<Example>(withJsonNet);

// Fails with System.Text.Json.
string withSystemTextJson = System.Text.Json.JsonSerializer.Serialize(original);
System.Text.Json.JsonSerializer.Deserialize<Example>(withSystemTextJson);

An InvalidOperationException is thrown from the method System.Text.Json.JsonSerializer.Deserialize :

System.InvalidOperationException: Each parameter in constructor 'Void .ctor(System.Nullable`1[System.Guid])' on type 'Example' must 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.

Configuration

I'm building an ASP.NET Core app targeting netcoreapp3.1, and using version 5.0.0-rc.2.20475.5 of System.Text.Json. I'm also using version 12.0.3 of Newtonsoft.Json.

Other information

Stack Trace:

System.InvalidOperationException: Each parameter in constructor 'Void .ctor(System.Nullable`1[System.Guid])' on type 'Example' must 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& value)
   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 <custom code>

layomia commented 3 years ago

Why make the constructor parameter nullable? The AGuid property is non-nullable which means that null will never be serialized. Are you expecting null JSON tokens to bind with the parameter on deserialization? Where would this JSON come from?

JSkimming commented 3 years ago

Thanks for the reply @layomia

To present a minimal use-case the example is intentionally trivial.

My situation is rather more complex. We have several scenarios where having the parameter as nullable was desirable (and worked well using Newtonsoft.Json), some of which we can, and have, resolved by making the parameter non-nullable.

But there are other scenarios where a nullable parameter with a non-nullable property is still preferred. One being where we have an unknown number of legacy objects that are serialised to a store with null values. The use-cases have since been updated where new instances are not serialised with null, but we still want to support the de-serialization of the legacy objects.

If we had been using System.Text.Json at the time, we probably would have implemented the solution differently, but as I highlighted above Newtonsoft.Json worked.

Hope the context helps.

Ultimately though, this is a difference in behaviour with Newtonsoft.Json. It looks like you are trying to resolve edge cases in to make System.Text.Json "the standard JSON stack for .NET." (See Issue #43620). And this is one such edge case.

layomia commented 3 years ago

From @https://github.com/NN--- in https://github.com/dotnet/runtime/issues/46480:

Description
    public class Q
    {
        [JsonConstructor]
        public Q(int? x)
        {
            if (x is null) throw new Exception();
            X = x.Value;
        }

        public int X { get; }
    }
Given string:
var str = JsonSerializer.Deserialize<Q>("{\"X\":123}" ");
It is deserialized fine with Newtonsoft, but not with System.Text. The reason is that constructor parameter type is int? while the property has type of int. Newtonsoft doesn't validate it, giving an option to do anything in the constructor.

There are possible workarounds but they are not as simple as the original code:

Make property private and add a public non nullable:
    public class Q
    {
        [JsonConstructor]
        public Q(int? x)
        {
            if (x is null) throw new Exception();
            X = x.Value;
        }

        public int? X { get; }

        [JsonIgnore]
        public int NonNullableX => X!.Value;
    }
Annotate nullable as non nullable, however it requires explicit call to Value.:
    public class Q
    {
        [JsonConstructor]
        public Q(int? x)
        {
            if (x is null) throw new Exception();
            X = x.Value;
        }

        [DisallowNull][NotNull]
        public int? X { get; }
    }
Configuration

Regression?

Not a regression.

Other information

layomia commented 3 years ago

We could look into relaxing the matching algorithm here if it proves to be unwieldy. The workaround here is to refactor the POCO code slightly to make the property type and the constructor parameter type the same. This issue will not be treated as high priority until a blocked and non-trivial scenario is provided.

GabeDeBacker commented 3 years ago

Deserializing JSON into an object that can be bound to WPF\XAML is likely very common place and converting an incoming IEnumerable into a observable collection that XAML can use is also likely common.

Not supporting this limits System.Text.Json's use with an XAML\WPF\UWP apps.

JSkimming commented 3 years ago

This issue will not be treated as high priority until a blocked and non-trivial scenario is provided.

@layomia As I explained, the example is trivial to present a minimal use-case. I also went on to explain the non-trivial scenario:

we have an unknown number of legacy objects that are serialised to a store with null values. The use-cases have since been updated where new instances are not serialised with null, but we still want to support the de-serialization of the legacy objects.

We are also blocked. Our workaround is to stick with Newtonsoft.Json.

layomia commented 3 years ago

Thanks for the responses and expanding on the importance here. There are definitely various scenarios where loosening the matching restrictions is helpful. We can address this in the .NET 6.0 release.

layomia commented 3 years ago

From @GabeDeBacker in https://github.com/dotnet/runtime/issues/47422#:

Description

System.Text.Json deserialization requires that a property type match the constructor type for immutable properties even though the constructor can convert the type.

This is a simple example of a class that will convert an incoming IEnumerable to a ReadOnlyObservableCollection for XAML binding.
        [JsonConstructor]
        public Logger(IEnumerable<LogEntry> entries)
        {
            this.Entries = new ReadOnlyObservableCollection<LogEntry>(this.entries);
        }

        public ReadOnlyObservableCollection<LogEntry> Entries { get; }
When desrializing from JSON, this fails.

Changing the property to be IEnumerable allows the deserialization to succeed, but that means I would need to add “another” property to this class for XAML binding to work. (Which is what this class is used for). The below just doesn’t seem right and was not something I had to do when using NewtonSoft
        public Logger(IEnumerable<LogEntry> entries)
        {
            this.Entries = entries;
            this.ObersvableEntries = new ReadOnlyObservableCollection<LogEntry>(this.entries);
        }

        public IEnumerable<LogEntry> Entries { get; }

        [JsonIgnore]
        public ReadOnlyObservableCollection<LogEntry> ObersvableEntries { get; }

layomia commented 3 years ago

Today, we expect an exact match between the constructor parameter type and the immutable property type. This is too restrictive in two major cases:

mapping nullable value-type ctor args to immutable properties of a non-nullable version of the type, as described in the description above https://github.com/dotnet/runtime/issues/44428#issue-739367850:
```
public class Example
{
    public Guid AGuid { get; }
    public Example(Guid? aGuid) => AGuid = aGuid ?? Guid.Empty;
}
```
scenarios where a reasonable conversion can be made between the ctor param type and the destination immutable property type, for example the scenario in https://github.com/dotnet/runtime/issues/44428#issuecomment-767195623:
```
public class Logger
{
    public ReadOnlyObservableCollection<LogEntry> Entries { get; }
    public Logger(IEnumerable<LogEntry> entries) => Entries = new ReadOnlyObservableCollection<LogEntry>(entries);
}
```

We can loosen the restriction and support these scenarios by checking that the ctor parameter is assignable from the immutable property type. This new algorithm is in accordance with the serializer always round-tripping (i.e being able to deserialize whatever we serialize), and maintains a high probability that the incoming JSON is compatible with the target ctor param. This is important to avoid unintentional data loss.

If there are more reasonable scenarios that will not be satisfied with this proposal, we can evaluate them and perhaps adjust further.

layomia commented 3 years ago

We can also, or alternatively, consider a property on JsonConstructorAttribute, that indicates no restriction between the ctor parameter type and the immutable property type. This would allow them to be two arbitrarily different types, basically an "I know what I'm doing mode". It would still be required for their CLR names to match.

/// <summary>
/// When placed on a constructor, indicates that the constructor should be used to create
/// instances of the type on deserialization.
/// </summary>
[AttributeUsage(AttributeTargets.Constructor, AllowMultiple = false)]
public sealed class JsonConstructorAttribute : JsonAttribute
{
    /// <summary>
    /// When <see cref="true" />, indicates that no restriction should be placed on the types of a constructor
    /// parameter and a property when there is a case-insensitive match between their names.
    /// </summary>
    public bool UseRelaxedPropertyMatching { get; set; }

    /// <summary>
    /// Initializes a new instance of <see cref="JsonConstructorAttribute"/>.
    /// </summary>
    public JsonConstructorAttribute() { }
}

A global option can be considered as well, to support non-owned types where we can't decorate with an attribute:

public sealed class JsonSerializerOptions
{
    /// <summary>
    /// When <see cref="true" />, indicates that no restriction should be placed on the types of a constructor
    /// parameter and a property when there is a case-insensitive match between their names.
    /// </summary>
    public bool ConstructorUseRelaxedPropertyMatching { get; set; }
}

All of this design assumes that there will always be a requirement that every constructor parameter binds to an object property, per the original spec for this feature: https://github.com/dotnet/runtime/pull/33095.

GabeDeBacker commented 3 years ago

@layomia - Implementing either (or both) of what you mentioned above (the JsonConstructorAttribute argument or is the constructor argument assignable from the property type) would be great additions! Thanks for the conversation

layomia commented 3 years ago

We can also, or alternatively, consider a property on JsonConstructorAttribute, that indicates no restriction between the ctor parameter type and the immutable property type. This would allow them to be two arbitrarily different types, basically an "I know what I'm doing mode". It would still be required for their CLR names to match.

We would likely still need to support the "mapping nullable value-type ctor args to immutable properties of a non-nullable version of the type" case by default - https://github.com/dotnet/runtime/issues/44428#issue-739367850.

layomia commented 3 years ago

Co-assigning @GabeDeBacker to provide the implementation for this feature, as discussed offline.

GabeDeBacker commented 3 years ago

I did find an example of where a property type is not assignable from the constructor argument type. You cannot construct a SecureString from a string.

public class ClassThatStoresSecureStrings
{
    using System;
    using System.Security;

   public ClassThatStoresSecureStrings(string userId, string password)
   {
            // This code repeats for password.
            this.UserId = new SecureString();
            foreach (var ch in userId)
            {
                this.UserId.AppendChar(ch);
            }

            this.UserId.MakeReadOnly();
   }

   public SecureString UserId { get; }
   public SecureString Password {get; }
}

layomia commented 3 years ago

From @terrajobst in https://github.com/dotnet/runtime/issues/53303:

I have the following class:

public sealed class CrawledAreaOwnerEntry
{
    public CrawledAreaOwnerEntry(string area, string lead, IEnumerable<string> owners)
    {
        Area = area;
        Lead = lead;
        Owners = owners.ToArray();
    }

    public string Area { get; }
    public string Lead { get; }
    public IReadOnlyList<string> Owners { get; }
}

When deserializing it, I get the following exception:

System.InvalidOperationException: Each parameter in constructor 'Void .ctor(System.String, System.String, System.Collections.Generic.IEnumerable`1[System.String])' on type 
'IssueDb.CrawledAreaOwnerEntry' must 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& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.Converters.DictionaryDefaultConverter`3.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   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](JsonReaderState& readerState, Boolean isFinalBlock, ReadOnlySpan`1 buffer, JsonSerializerOptions options, ReadStack& state, JsonConverter converterBase)
   at System.Text.Json.JsonSerializer.ReadAsync[TValue](Stream utf8Json, Type returnType, JsonSerializerOptions options, CancellationToken cancellationToken)
   at IssueDb.Crawling.CrawledRepo.LoadAsync(String path) in /home/runner/work/issuesof.net/issuesof.net/src/IssueDb/Crawling/CrawledRepo.cs:line 43

It seems the serializer requires the types of the properties to be identical to the parameter. That feels overly restrictive to me; it seems we should only require that the property type is assignable to the parameter type.

layomia commented 3 years ago

Unassigning @GabeDeBacker https://github.com/dotnet/runtime/pull/47661#issuecomment-856936994.

layomia commented 3 years ago

We couldn't get to this in .NET 6.0 but can consider for .NET 7.

daiplusplus commented 3 years ago

Reposting my reply to #45190 (@layomia asked me to repost it here)

This is my use-case for wanting support for private primary-constructors and multiple primary-constructors, and partial mapping between properties and ctor parameters:

I know System.Text.Json won't invoke private members by default, but I'm surprised and disappointed that we cannot explicitly denote private constructors with [JsonConstructor].

Ideally I'd like to reuse the same DTO types in Newtonsoft.Json and System.Text.Json contexts (with suitable using JsonPropertyAttribute = System.Text.Json.Serialization.JsonPropertyNameAttribute in #if USE_STJ at the top of the .cs file), however my projects tend to use primary-constructors which STJ doesn't support, so I can't use the above trick just yet.

[ ] Allow [JsonConstructor] with private constructors, even when a public constructor is available.
- This is essential for cases where a type has different constructors for different states, with per-state validation logic (for an example, see my class SqlDbTypeRef in the expanding region below). Exposing a public constructor that has parameters for all properties would allow consumers to unwittingly circumvent the validation rules - and requiring us to reimplementing the validation logic in this new constructor is not endearing me to use STJ over NJ.
[ ] Allow [JsonPropertyName] on constructor parameters and use JsonPropertyNameAttribute.Name to map them to C# properties' JsonPropertyNameAttribute.Name instead of their C# property names.
- This make it easier to write DTOs that are more resilient to unintentional symbol renames when refactoring - especially as Visual Studio's C# Refactor-Rename tool does not currently rename constructor parameters that are assigned to POCO properties.
- Allowing explicit JSON property names on ctor parameters also allows us to tweak serialization vs. deserialization logic without needing too much effort.
- A use-case for this is when you want the same DTO to intentionally ignore or drop certain properties when deserializing but still serialize them and you're using code-gen or T4 to generate DTOs and having to use custom logic in the ctor is just too much effort.
[ ] Allow partial [JsonConstructor] parameter-to-property mapping.
- This is useful when the private [JsonConstructor] is also used by a public static factory method and additional details are routed through there but would not come from JSON - so the constructor has more parameters than JSON properties.

For example, I have a class that represents a SqlDbType with length, precision, and scale parameters - this is what I currently have working with Newtonsoft.Json:

Click to expand!

``` public partial class SqlDbTypeRef { ///

For types without parameters.

/// When the value used requires length, precision, or scale parameter values. public SqlDbTypeRef( SqlDbType dbType ) { SqlDbTypeParameters p = SqlDbTypeExtensions.GetParameters( dbType ); if( p == SqlDbTypeParameters.None ) { Int16? length = null; if( SqlDbTypeExtensions.HasImplicitLength( dbType, out Int16 implicitLength ) ) length = implicitLength; this.SqlDbType = dbType; this.Length = length; this.Precision = null; this.Scale = null; } else { throw new ArgumentException( message: "SqlDbType.{0} is parameterized: {1}".Fmt( dbType, p ), paramName: nameof(dbType) ); } } ///

For , , , , , .

public SqlDbTypeRef( SqlDbType dbType, Int16 length ) { SqlDbTypeParameters p = SqlDbTypeExtensions.GetParameters( dbType ); if( p == SqlDbTypeParameters.Length ) { this.SqlDbType = dbType; this.Length = length; this.Precision = null; this.Scale = null; } else { throw new ArgumentException( message: "SqlDbType.{0} requires {1}".Fmt( dbType, p ), paramName: nameof(dbType) ); } } ///

For , , .

public SqlDbTypeRef( SqlDbType dbType, Byte precision ) { SqlDbTypeParameters p = SqlDbTypeExtensions.GetParameters( dbType ); if( p == SqlDbTypeParameters.Precision ) { this.SqlDbType = dbType; this.Length = null; this.Precision = precision; this.Scale = null; } else { throw new ArgumentException( message: "SqlDbType.{0} requires {1}".Fmt( dbType, p ), paramName: nameof(dbType) ); } } ///

For .

public SqlDbTypeRef( SqlDbType dbType, Byte precision, Byte scale ) { SqlDbTypeParameters p = SqlDbTypeExtensions.GetParameters( dbType ); if( p == SqlDbTypeParameters.PrecisionAndScale ) { this.SqlDbType = dbType; this.Length = null; this.Precision = precision; this.Scale = scale; } else { throw new ArgumentException( message: "SqlDbType.{0} requires {1}".Fmt( dbType, p ), paramName: nameof(dbType) ); } } [JsonConstructor] private SqlDbTypeRef( [JsonProperty( "type" )] SqlDbType dbType, [JsonProperty( "length" )] Int16? length, [JsonProperty( "precision" )] Byte? precision, [JsonProperty( "scale" )] Byte? scale ) { this.SqlDbType = dbType; this.Length = length; this.Precision = precision; this.Scale = scale; } [JsonProperty( "type" )] public SqlDbType SqlDbType { get; } [JsonProperty( "length", NullValueHandling = NullValueHandling.Ignore )] public Int16? Length { get; } [JsonProperty( "precision", NullValueHandling = NullValueHandling.Ignore )] public Byte? Precision { get; } [JsonProperty( "scale", NullValueHandling = NullValueHandling.Ignore )] public Byte? Scale { get; } } ```

raffaeler commented 2 years ago

@layomia I landed here from https://github.com/dotnet/runtime/issues/59804

I am not sure if I understand correctly your proposal, so I try to rephrase it.

When I apply JsonConstructorAttribute, I would like the deserialization to just match the signature of the constructor marked with JsonConstructorAttribute with zero requirements over the properties.

Use that constructor even if its visibility is private/internal/whatever
Match the parameter types/names according to the json being deserialized
It will be up to the developer to initialize the object state (properties/fields)

Are you going in this direction?

Thank you!

layomia commented 2 years ago

@raffaeler

Use that constructor even if its visibility is private/internal/whatever

Yes this is an enhancement we plan to make as part of this work. In the source-gen case, only public and internal ctors can be used, so the generator will issue a diagnostic if the attribute is placed on a ctor that is not accessible to the generated code.

Match the parameter types/names according to the json being deserialized

It will be up to the developer to initialize the object state (properties/fields)

Given further feedback from scenarios like https://github.com/dotnet/runtime/issues/44428#issuecomment-900743092, I'm inclined to have the serializer perform no validation on ctor parameter/property binding wrt to names and data type, and just have the user perform whatever validation they need within the constructor, or using one of the deserialization callbacks (IJsonOnDerializing, IJsonOnDeserialized)

daiplusplus commented 2 years ago

@layomia why arbitrarily restrict it to public and internal - why not private ctors? If a private ctor has [JsonConstructor] then it still should be used. There are plenty of reasons for wanting to make a ctor private to prevent human users but still want JSON deserialization to use it. A good reason for this is that defining a private primary constructor is considerably less work than writing up a full contract-resolver/JsonConverter implementation.

What benefits, exactly, come from prohibiting private constructors?

raffaeler commented 2 years ago

@layomia

[...] I'm inclined to have the serializer perform no validation on ctor parameter/property binding [...]

Yes please. The whole point of JSON specs is to be greedy. Exceptions should be thrown only in extreme (wrong casts) cases. So I agree also with @Jehoel that you should not limit to any visibility modifier when applying the attribute.

Also, given you are going into the generation space, please provide the opportunity to extend it. I created my Json converter that dynamically transform a graph and it would be nice if I can exten the default generation from my converters. BTW, as I convert a whole tree in a single converter, the benchmark tells me that I'm faster.

Timovzl commented 2 years ago

In the source-gen case, only public and internal ctors can be used

@layomia It is still desirable to be able to use the source generator, even if we need or want to keep our constructors private. If no constructor is available to the source generator, could it please use FormatterServices.GetUninitializedObject() instead?

[...] I'm inclined to have the serializer perform no validation on ctor parameter/property binding [...]

My suggestion helps with this as well.

terrajobst commented 2 years ago

@raffaeler

Yes please. The whole point of JSON specs is to be greedy. Exceptions should be thrown only in extreme (wrong casts) cases. So I agree also with @Jehoel that you should not limit to any visibility modifier when applying the attribute.

I'd be careful with designs like this. For example, my team takes API design and breaking changes extremely seriously but even we don't review any changes to privates. If serialization magically binds to privates it becomes much more likely for developers to break serialization by making changes to state they reasonably thought to be unobservable. There aren't really any good reasons to bind to privates for serialization; in my experience it's virtually always preferable to limit the serializer to public state. And in cases where there isn't a good alternative, I'd rather we extend the serializer's feature set to enable it than to bind to privates.

Also, moving forward we want to support code generation. That's a lot harder because you can't access privates statically.

@layomia

I'm inclined to have the serializer perform no validation on ctor parameter/property binding wrt to names and data type, and just have the user perform whatever validation they need within the constructor, or using one of the deserialization callbacks (IJsonOnDerializing, IJsonOnDeserialized)

What do you mean by "no validation"? Presumably for naming you mean ignoring casing and not enforcing that the propery/parameter match, but you'd still match that the property from the JSON payload can be assigned to the parameter, right?

daiplusplus commented 2 years ago

@terrajobst

For example, my team takes API design and breaking changes extremely seriously but even we don't review any changes to privates.

With respect, working on a platform or framework is different to working on an end-application or web-service. Most .NET users use Newtonsoft.Json over System.Text, and in Newtonsoft it's already well established that [JsonConstructor] works with private constructors (e.g. this works just fine), and so the presence of the [JsonConstructor] attribute overrides any private accessibility modifiers - and means it should be reviewed for breaking changes, whereas your argument seems to be "it means we can't depend only on access-modifiers for examining breaking changes" which isn't a compelling argument. And I note that breaking-changes can happen just-as-easily inside other private members, e.g. if an ISerializable's GetObjectData calls into some private method.

If serialization magically binds to privates...

No-one is advocating for magic, undocumented, or otherwise astonishing behavior: we're just saying "the [JsonConstructor] attribute should trump access-modifiers".

...it becomes much more likely for developers to break serialization by making changes to state they reasonably thought to be unobservable.

A private constructor isn't comparable to a private field or private property: a constructor doesn't represent any kind of state.

But FWIW, back in .NET Framework 2.0 and WCF, the [DataMember] attribute does support private properties (it's just opt-in instead of opt-out).

There aren't really any good reasons to bind to privates for serialization; in my experience it's virtually always preferable to limit the serializer to public state.

Is that a default (i.e. require explicit opt-in to serialize private members, a-la [DataMember]), or are you suggesting it should be a hard restriction that cannot be overridden by application code?

I'd rather we extend the serializer's feature set to enable it than to bind to privates.

I'm unsure what you're describing here.

raffaeler commented 2 years ago

@terrajobst Thank you for your answer. I understand the reason you mentioned, but the attribute is an opt-in that you decide to adopt with all the pros/cons. There are times where you don't want to publish the constructor to the outside world and the simple method would be to instruct the serializer to use the provided constructor. I totally undetstand that this could prevent the code generation option, but not all code needs more perf.

Anyway, the point I am mostly concerned about my initial point. The current binding validations are too strict because they bind json properties to the name and types of the properties instead of relying only on the JsonConstructor parameters. This is a big stopper in many versioning scenarios (and please do not ask people to write a JsonConverter for these use-cases).

Also, please reconsider the JsonConstructor applied at class level for records so that we can finally use primary constructors where multiple ctors exist.

TIA

layomia commented 2 years ago

@layomia

I'm inclined to have the serializer perform no validation on ctor parameter/property binding wrt to names and data type, and just have the user perform whatever validation they need within the constructor, or using one of the deserialization callbacks (IJsonOnDerializing, IJsonOnDeserialized)

What do you mean by "no validation"? Presumably for naming you mean ignoring casing and not enforcing that the propery/parameter match, but you'd still match that the property from the JSON payload can be assigned to the parameter, right?

@terrajobst my initial thought was that the ctor binding logic should be loosened to allow a match between a parameter & a property if the parameter type is assignable to the property type. However, I keep seeing scenarios like the one pointed about by @GabeDeBacker above, where this would not be sufficient:

public class ClassThatStoresSecureStrings
{
    using System;
    using System.Security;

   public ClassThatStoresSecureStrings(string userId, string password)
   {
            // This code repeats for password.
            this.UserId = new SecureString();
            foreach (var ch in userId)
            {
                this.UserId.AppendChar(ch);
            }

            this.UserId.MakeReadOnly();
   }

   public SecureString UserId { get; }
   public SecureString Password {get; }
}

I think it's okay to rule this as an edge-case for now & circle back if there's significant feedback later. In either case, we'd still require that every ctor param matches to a property, and for their names to match (case insensitive match would be okay).

I also think that it's okay to allow [JsonConstructor] to work non-public ctors since several folks have expressed that they desire this functionality (https://github.com/dotnet/runtime/issues/31511, https://github.com/dotnet/runtime/issues/38327), and we already some precedent in form of support for non-public accessors on public properties when [JsonInclude] is used. However, it would be a breaking change since, unfortunately, [JsonConstructor] is silently ignored on non-public ctors today (in contrast to [JsonInclude] on non-public props/fields which throws InvalidOperationException). The breaking change is somewhat mitigated by the fact the serializer is likely to still throw NotSupportedException in the general case, as there'll be no ctor to use.

@layomia why arbitrarily restrict it to public and internal - why not private ctors?

As previously mentioned (https://github.com/dotnet/runtime/issues/44428#issuecomment-978538377). The source gen restriction of public/internal ctors only is not arbitrary, since source-gen doesn't have access to members that are not public or internal. In the future, if we design a source-gen mode where the code is generated directly on the serializable type, we could enable non-public/internal member support for source-gen.

My current feel for the implementation changes as a result of this issue/discussion would be the following:

Rather than require a ctor param type and the target property type to match exactly, we would allow binding if the ctor param type is assignable to the property type.
Allow [JsonConstructor] on non-public ctors with the reflection serializer.

raffaeler commented 2 years ago

@layomia I don't understand this point

Rather than require a ctor param type and the target property type to match exactly, we would allow binding if the ctor param type is assignable to the property type.

The whole point of only matching the ctor parameters is because there is not match with the parameters. In many use-cases I need to modify the data (including the type) of the serialized data to some other type that is expressed in the property. The ctor is the best point where we can resolve the mismatch.

Looking at this from another perspective: json has a different type system than .NET. If I appoint a ctor to be the "resolver" for a type system discrepancy, it would not make sense that any validation is applied to the final property which only gets the converted data.

layomia commented 2 years ago

@raffaeler - that's the whole issue with the request to loosen the binding logic. On one hand we have the fact that STJ asserts that serialization should be round-trippable, requiring that we have reasonable binding between ctor params and properties. This lets us know that payloads that can be deserialized must also be serializable. On the other hand we have somewhat niche scenarios like yours that need to circumvent this validation. I'll spend sometime codifying the various options which might lead to API review where we can get more feedback on what direction to take. cc @dotnet/area-system-text-json

raffaeler commented 2 years ago

@layomia I understand the dilemma but please do not forget the nature of the json serialization which is lazy and greedy in total contrast with the strong validation policy throwing exception that you would do on, for example, an xml serialization.

Maybe you can add an option to explicitly opt-in the ability to just map the ctor without validating the property mappings. It can either be a new JsonSerializerOptions or a JsonConstructor property.

fabio-s-franco commented 2 years ago

This is one of the most painful aspects of System.Text.Json. It has been by far the greatest time pit since I started using it.

To me it makes no sense whatsoever that there is a match between constructor name and type to a corresponding class member. Once we are inside the constructor, the control is already handed over, why care to which property and how a constructor parameter is being assigned to? This becomes specially annoying with inheritance.

Even if there is a conflict (when they do match), there are so many ways to make this simpler and less of debugging hell. Just default one of the following available choices:

Constructor precedence (my preferred default). Just pop the json node it if it successfully serializes to a constructor parameter.
Exception if exact type mismatch
Closest match to source json type. If not inferable, take constructor as precedence

To me this beats the whole purpose of JSON's interoperability. With all converters and all, this really shouldn't be necessary. Specially given we can specially decorate a constructor specifically for the purpose of deserialization. Maybe this could be a behavior (albeit still less strict) for when there is no specifically decorated JsonConstructor?

eiriktsarpalis commented 2 years ago

We won't have time to work on this for .NET 7, moving to future.

douglasg14b commented 2 years ago

This is sad and is one of just a couple features that Newtonsoft has by default that STJ does not. that continually stops us (and even other libs & frameworks) from ever using it.

We try every couple .Net versions, and have thus far been disappointed every time by how strict, to the point of blocking, it's usage continues to be when the whole point of JSON is (as others have mentioned) to be greedy.

I can only hope that one day STJ becomes a drop in replacement for Newtonsoft, if ever.

Though I'm not sure if reputation damage will pass for a while. Most .Net devs I work with turn up their nose to STJ by default at this point, even if it has improved, because of constant poor experiences using it :/

eiriktsarpalis commented 1 year ago

One possible solution might be to expose a flag on either JsonSerializerOptions or JsonConstructorAttribute that completely disables matching constructor parameters to property types, roughly:

namespace System.Text.Json;

public class JsonSerializerOptions
{
     public bool RequireMatchingPropertyForConstructorParamers { get; set; } = true;
}

public class JsonConstructorAttribute
{
     public bool RequireMatchingPropertyForConstructorParamers { get; set; } = true;
}

which would make the following POCOs legal from a serialization perspective:

JsonSerializer.Deserialize<MyPoco>("{ ... }"); // succeeds deserialization

public class MyPoco
{
    [JsonConstructor(RequireMatchingPropertyForConstructorParamers = true)]
    public MyPoco(int p1, int p2, int p3) { }
}

raffaeler commented 1 year ago

@eiriktsarpalis The JsonSerializerOptions is the only possible solution as the JsonConstructor is definitely broken (not be available for default ctors in C# records). The only possible alternative would be a new attribute on the type rather than on the ctor.

layomia commented 1 year ago

[Triage] This work should include a fix for https://github.com/dotnet/runtime/issues/56999. Repro below. Also see https://github.com/dotnet/runtime/issues/56999#issuecomment-895271264.

Description

I would expect the below to work, however it does not. Maybe because covariant return types are new and not yet handled in System.Text.Json yet. I think the example speaks for itself but let me know if more information is required.

Configuration

Which version of .NET is the code running on? .NET 5.0
What is the architecture (x64, x86, ARM, ARM64)? x64
Do you know whether it is specific to that configuration? It is not.

Non-working Example

Using covariant return type read-only properties passed through derived constructor does not work.

https://dotnetfiddle.net/IsNX3q

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

public class Program
{
  public static void Main()
  {
      var json = "{ \"prop\": { \"id\": \"abc\", \"num\": 2 } }";
      var obj = JsonSerializer.Deserialize<DerivedClass>(json);
      Console.WriteLine(obj?.Property?.Id ?? "null");
      Console.WriteLine(obj?.Property?.Number?.ToString() ?? "null");
  }

  public class BaseClass
  {
      public BaseClass(BaseProperty property)
      {
          Property = property;
      }

      [JsonPropertyName("prop")]
      public virtual BaseProperty Property { get; }
  }

  public class DerivedClass : BaseClass
  {
      public DerivedClass(DerivedProperty property)
          : base(property)
      {
      }

      public override DerivedProperty Property { get; }
  }

  public class BaseProperty
  {
      [JsonPropertyName("id")]
      public string Id { get; set; }
  }

  public class DerivedProperty : BaseProperty
  {
      [JsonPropertyName("num")]
      public int? Number { get; set; }
  }
}

Output:

null
null

Working Example

Using non-covariant return type read-only properties passed through derived constructor does work:

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

public class Program
{
  public static void Main()
  {
      var json = "{ \"prop\": { \"id\": \"abc\", \"num\": 2 } }";
      var obj = JsonSerializer.Deserialize<DerivedClass>(json);
      Console.WriteLine(obj?.Property?.Id ?? "null");
  }

  public class BaseClass
  {
      public BaseClass(BaseProperty property)
      {
          Property = property;
      }

      [JsonPropertyName("prop")]
      public virtual BaseProperty Property { get; }
  }

  public class DerivedClass : BaseClass
  {
      public DerivedClass(BaseProperty property)
          : base(property)
      {
      }
  }

  public class BaseProperty
  {
      [JsonPropertyName("id")]
      public string Id { get; set; }
  }
}

Output:

abc

mathieubergouniouxcab commented 1 year ago

Something that no one mentioned : If the error message could at least indicate what parameter caused the issue then that would be the bare minimum.

This question is asked countless times in StackOverflow, it's a huge waste of time for many devs, who have to go through each parameter one by one like cavemen, and wonder : "is it the spelling? Is it that it is nullable?", etc.

eiriktsarpalis commented 1 year ago

Hi @mathieubergouniouxcab would you be interested in contributing a PR that improves the error message?

mathieubergouniouxcab commented 1 year ago

Hi @mathieubergouniouxcab would you be interested in contributing a PR that improves the error message?

Hi Eirik, I'm perplexed by your answer. I thought System.Text.Json was funded by Microsoft?

eiriktsarpalis commented 1 year ago

It is also an open source project that happily accepts community contributions. The team can only deliver a limited amount of features and fixes per release -- this particular issue is not slated for .NET 8 hence my suggestion.

fabio-s-franco commented 1 year ago

Of course not, why would it be? It is only a huge waste of time for a lot of people and people have been struggling with it for at least three years only.

I have personally witnessed a fair amount of people go back to newtonsoft and miss all the other good things about this api because they did not want to put up with this anymore.

I wish I had more time to go down the rabbit hole as I consider it personal and would gladly submit a PR as By far the aspect that caused more waste of my time I have ever encountered. But since I really don't have enough time to work on this one, all it's left for now is hope.

And with that hope that I soon get some time to spend on side projects.

douglasg14b commented 1 year ago

I have personally witnessed a fair amount of people go back to newtonsoft and miss all the other good things about this api because they did not want to put up with this anymore.

This is the norm it seems. When System.Text.Json doesn't have sane defaults, global configuration, extremely poor drilling ergonomics, explicitly doesn't follow the greedy pattern of Json, and fails to address fundamental QoL issues that Newtonsoft has had for ages. It's just not a good fit. It should "Just Work", but it doesn't, not even close. And despite community complaints & requests, feature owners just keep on push back on making it actually nice to use because it needs to be "C#'y"

I keep trying out System.Text.Json every.single.relaease, year over year. And the same fundamental ergonomic & usability failures just keep on keeping on.

This is particularly egregious when I had a new C# dev (C++ & TS dev before) approach me yesterday and say that he really doesn't like or want to continue working with C#. His experience with System.Text.Json set the whole tone & stage for what he thinks C# is. While that is a bit premature, it's not exactly uncommon for new devs to have a terrible experience with some native support and then just write off the ecosystem entirely.

It's bad for image, it's bad for adoption, and it's bad for ergonomics. And there is active pressure to keep it that way, to explicitly keep it's surface area awkward and full of 'gotchas'. It should not be a default recommendation. It's great for advanced & internal use, don't get me wrong, but for your Java/Pythos/JS/...etc devs poking at C# to see if they like it, it's a turnoff.

Pushing people to make FOSS contributions to something that has user-facing design issues, that are explicit isn't really solution. Even moreso when one sees the graveyard of pushed-back feature requests for foundational concerns. That's gotta start at the top.

I love C#/.Net. Which is why I'm so passionate about this, it's frustrating.

/rant 😒

mathieubergouniouxcab commented 1 year ago

well @eiriktsarpalis I'm not going to enter the polemics, but I guess the bottled up frustration in the messages above is a good hint that maybe the logging part should be re-prioritized after 3 years (this thread started in Nov 2020) 😄

KrzysztofBranicki commented 1 year ago

Hi, are there any plans to address problems mentioned earlier in .NET 8.0? As I described in this issue https://github.com/dotnet/runtime/issues/55318 which was closed and linked here two years ago, this serializer is not very useful for scenarios when you want to use it for serializing Aggregates modeled using Domain Driven Design principles and store them in document database. That's because it can't handled encapsulation (which is quite important in DDD :)). I would understand that the goal of creating this serializer in the first place was to create fast general purpose JSON serializer for .NET and not something that is only for serializing simple DTOs in web API endpoints.

eiriktsarpalis commented 1 year ago

This is the norm it seems. When System.Text.Json doesn't have sane defaults, global configuration, extremely poor drilling ergonomics, explicitly doesn't follow the greedy pattern of Json, and fails to address fundamental QoL issues that Newtonsoft has had for ages. It's just not a good fit. It should "Just Work", but it doesn't, not even close. And despite community complaints & requests, feature owners just keep on push back on making it actually nice to use because it needs to be "C#'y"

@douglasg14b it would help if you could open issues that specifically break down the issues you are facing and we might even be able to offer recommendations, workarounds or even prioritize relevant improvements.

I'm not going to enter the polemics, but I guess the bottled up frustration in the messages above is a good hint that maybe the logging part should be re-prioritized after 3 years (this thread started in Nov 2020)

@mathieubergouniouxcab the System.Text.Json backlog currently consists of 210 open issues which we want to address at some point. While I understand the frustration of being impacted by one specific issue, we generally do try to prioritize based on impact and popularity and this issue has been cut from .NET 8.

mathieubergouniouxcab commented 1 year ago

the System.Text.Json backlog currently consists of [210 open issues]. We prioritize them based on impact and popularity

Understood. I haven't found one specifically dealing with logging which parameter is causing a deserialization fail. Could you confirm that such entry does not exist? If so, then I will create it -- in order for it to have its own thread and its own voting system (and stop polluting this one).

eiriktsarpalis commented 1 year ago

AFAIK you're the first to bring up that concern. I think it's reasonable to factor into a separate issue since it's of much smaller scope.

mathieubergouniouxcab commented 1 year ago

AFAIK you're the first to bring up that concern. I think it's reasonable to factor into a separate issue since it's of much smaller scope.

I found 8+ entries in your bug tracker related to the error "Each parameter in constructor (...) must bind to a field on deserialization". Devs so obsessed with grinding the issues one by one that they forgot it's 1,000 different flavours of the same issue. https://i.imgur.com/B0St5fv.png

eiriktsarpalis commented 1 year ago

Care to share the issues that specifically requests this concern?

Something that no one mentioned : If the error message could at least indicate what parameter caused the issue then that would be the bare minimum.

mathieubergouniouxcab commented 1 year ago

Care to share the issues that specifically requests this concern?

The google query I used :

site:https://github.com/dotnet/runtime/issues/ Each parameter in constructor must bind to an object property or field on deserialization

I only clicked on the 8 first ones but there's 20+. Here are some metrics to evaluate the impact :

Most of them complain that the typing is too strict (e.g. fails to recognize that a List can be deserialized as a IEnumerable )
very few mention issues witht he parameter's name. But imho it's a classic case of survivor bias, as in : we see only the "valid" issues that made it to the tracker. We don't see the stupid ones, e.g. typos in parameters names, even though they took just as long to spot and fix.

dotnet / runtime