Jsonserializer and record types with "primary" constructor

[javiercn]

I've been playing with the new C# 9.0 features to evaluate how they integrate with ASP.NET Core. As part of that, I've been checking records and I found that the deserializer throws if I try to deserialize a record defined as follows:

    public record TodoTask(string Description);
}

The exception thrown is:

   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.Serialization.JsonConverter`1.ReadCoreAsObject(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](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)

This forces people to use the longwinded syntax for records when writing APIs, which is something we want to avoid. The closest I got is to do, which is far more verbose:

    public record TodoTask
    {
        public TodoTask(string description) => Description = description;
        public string Description { get; init; }
    }

[ericstj]

@steveharter can you have a look at how the serializer handles records? We should add some tests to cover this new language feature.

[steveharter]

This is not working because

• There is no public parameterless constructor for record types meaning the deserializer must call the constructor that contains the record properties as arguments (since it cannot construct an empty object and call the setters directly).
• The name of the constructor arguments for records is the exact name specified when declaring the record (i.e. "Description" in this case) and the constructor feature in the deserializer assumes the argument name is the camel-cased equivalent (i.e. "description").

To address without a workaround, I believe we need to relax the deserializer to allow an exact-match on constructor parameter name to property name (in addition to the existing camel-case to Pascal-case match on property name). @layomia do you agree?

[layomia]

@steveharter yes, that is a good approach to support this new language feature.

cc @pranavkm

[steveharter]

OK, I'm working on getting a PR out today.

[pranavkm]

@steveharter would it make sense to use PropertyNameCaseInsensitive to determine this? In MVC's case, this is set so it should have worked.

[layomia]

@pranavkm the matching in question is between the property's name and the corresponding constructor parameter's name, where we currently expect the names to conform to C#'s naming guidelines e.g. a property named MyProperty should map to a ctor parameter named myProperty. See the feature's spec for more info.

We see now with record types that the ctor parameter's name will be an exact match with the property's name, so we will make the change to check for an exact match as well.
(I believe anonymous types have the same property name-ctor parameter naming convention, so I think you might need to re-write a test for anonymous types which will now fail with your change @steveharter.)

None of these semantics relate to the incoming JSON, so I don't believe there's any reason for the matching to be influenced by PropertyNameCaseInsensitive.

[steveharter]

would it make sense to use PropertyNameCaseInsensitive to determine this?

Yes as @layomia explains there is an indirection between the JSON property name and ctor parameter. The indirection is that the ctor parameter name must bind to a CLR property name through a naming convention (must be exact-match or match when property is camel-cased). This means, for example, you can have a [JsonPropertyName] attribute on a CLR property name which produces whatever JSON property name you want, but that JSON will still deserialize that using the constructor and the parameter>property name indirection.

[pranavkm]

Thanks @steveharter \ @layomia. It would be great to make sure this behavior is well documented particularly because the behavior does not conform to how Json.Net works.

[steveharter]

Json.Net does support the binding between parameter name and property allowing a completely different JSON name for example.

However Json.Net doesn't require that binding (like STJ does) so this should be documented.

[pranavkm]

@steveharter do you know if this change made it to p8?

[layomia]

@pranavkm, it did - find "38f6ebc" here https://github.com/dotnet/runtime/commits/release/5.0-preview8.