JSON Serialization on records with a Primary Constructor with Properties created from the constructor parameter definition works fine; auto-generated properties does not work unless the properties are decorated with JsonIgnore.

simkin2004 commented 2 months ago

Description

record class are reference types with Value-type Equals semantics.

How I define that record syntactically should not determine whether it is serializable or not.

This code does not serialize. public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public DateOnly Date { get; } = date; }

This code serializes. public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly Date) {}

This code serializes. public record WeatherForecastRecordWithJsonignore(DateOnly date) { [JsonIgnore] public DateOnly Date { get; set; } = date; }

This code serializes. public class WeatherForecastRecordWithJsonignore(DateOnly date) { public DateOnly Date { get; set; } = date; }

Reproduction Steps

I can provide a WeatherForecast ASP.NET Web API that demonstrates the problem and the behavior that it causes is a 406-NotAcceptable in ASP.NET .

This code does not serialize. public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public DateOnly Date { get; } = date; }

This code serializes. public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly Date) {}

This code serializes. public record WeatherForecastRecordWithJsonignore(DateOnly date) { [JsonIgnore] public DateOnly Date { get; set; } = date; }

This code serializes. public class WeatherForecastRecordWithJsonignore(DateOnly date) { public DateOnly Date { get; set; } = date; }

Expected behavior

All examples serialize properly.

Actual behavior

record with primary constructor and user-defined properties with JsonIgnore does not serialize properly.

Regression?

Unknown whether earlier fixes patched this problem. Unable to install earlier releases as I do not have access to install them.

Known Workarounds

Use JsonIgnoreAttribute. Use class instead of record and write value equality semantics. Use record with auto-created properties from the constructor and suppress styling issues because it uses the "wrong" casing for constructor parameters.

Configuration

.NET 8.0 + ASP.NET Core All OS All architectures Not specific to the configuration.

Other information

No response

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

eiriktsarpalis commented 2 months ago

The reason why you're seeing this issue is because you're defining a positional record, which means that the DateOnly date declaration compiles to both a constructor parameter and a property. The serializer subsequently fails because of the ambiguity between the date and Date properties, which is by design behavior. You can work around the issue either by eliminating the secondary property:

public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly Date);

or use a regular class with a primary constructor which doesn't result in a public property being generated:

public class WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date)
{
    public DateOnly Date { get; } = date;
}

chrisoverzero commented 2 months ago

(acch, eiriktsarpalis just beat me to it)

How I define that record syntactically should not determine whether it is serializable or not.

This is simply not the case.

public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date)
{
  public DateOnly Date { get; } = date;
}

…is different from:

public class WeatherForecastClass(DateOnly date)
{
  public DateOnly Date { get; set; } = date;
}

…in a very important way. The constructor parameters to records become properties. When trying to serialize, the error message is one of these:

Members 'date' and 'Date' on type 'WeatherForecastRecordWithPrimaryConstructorAndProperties' cannot both bind with parameter 'date' in the deserialization constructor.

The JSON property name for 'WeatherForecastRecordWithPrimaryConstructorAndProperties.date' collides with another property.

(Depending on the settings to the serialization options.) This is because the parameter date has become a property named exactly that, date. If you'd like to use a record class, the most ergonomic use is likely one of these:

public record class WeatherForecast(DateOnly Date);

public record class WeatherForecast
{
  public DateOnly Date { get; init; }
}

simkin2004 commented 2 months ago

When you apply the Principle of Least Astonishment, does it make sense that public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public DateOnly Date { get; } = date; }

compiles to effectively:

public record WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { this.date = date; this.Date = date; } public DateOnly date { get; private set;} public DateOnly Date { get; private set;} }

while public class WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public DateOnly Date { get; } = date; }

compiles to effectively:

public class WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { public WeatherForecastRecordWithPrimaryConstructorAndProperties(DateOnly date) { this.Date = date; } public DateOnly Date { get; private set;} }

chrisoverzero commented 2 months ago

When I apply the Principle of Least Astonishment, I’m amazed that any computer program works.

That a record class and a class behave differently is comparatively not a surprise at all. They’re each doing what they were designed to do.

Use record with auto-created properties from the constructor and suppress styling issues because it uses the "wrong" casing for constructor parameters.

The overwhelming convention is to make these constructor parameters PascalCased. The tools I’ve ever tried have adapted to this. Can I ask, what makes this a blocker for you?

simkin2004 commented 2 months ago

My point is not that they 'behave' differently, it is that a -reference type- COMPILES differently for the same syntax and that the compilation difference is -not- documented in the record keyword documentation.

A "record class" is a special subtype of a class. If I apply Liskov's notion, then the compiler (and the execution) behavior should be the same for a record and a class. If you wanted "record" to have completely different compiler semantics, I would argue that it should be a separate keyword, clear and separate from "class" or "struct" which could have very well-defined semantics.

Ultimately, the issue isn't a blocker for me. I've found (and published above) several different workarounds, but wanted to get the "why". While I still don't find the "why" compelling, it was how it was done.

As for analyzers, I will have to get special permission to use the pre-release versions of the analyzers that support this as some of the ones my team/company uses have not had a "normal" release in several years.

Thank you for the explanation. I will close the issue.

eiriktsarpalis commented 2 months ago

A "record class" is a special subtype of a class.

Records constitute a separate kind in C# (even though they do compile to classes and structs in IL). For example, a record type cannot inherit from a class type or vice versa.

If you wanted "record" to have completely different compiler semantics, I would argue that it should be a separate keyword, clear and separate from "class" or "struct" which could have very well-defined semantics.

Maybe I'm misunderstanding what you're saying, but isn't the record keyword doing precisely that?

simkin2004 commented 2 months ago

If record is a separate kind, then the "record" keyword modifier is a -BAD- naming overload of the "class" and "struct" keywords because they change the underlying COMPILER semantics for "class" and "struct". I know that this isn't going to happen, but "reference record" and "value record" would have been more appropriate.

I thought more about this last night and have a suggestion.

Since the implementation that I provided with the Pascal Cased property is invalid for record for serialization:

Throw a compiler error that prevents that record class from being compiled if it has a Pascal Cased property with the same name as a Camel Cased primary constructor parameter since it will cause Serialization to fail.
Document in "record" documentation that a property with the same name as a camel cased Primary Constructor parameter is an invalid implementation and will cause serialization issues.

dotnet / runtime