JsonSchemaExporter.GetJsonSchemaAsNode is inconsistent with required properties + constructors + record structs

[rynowak]

Description

JsonSchemaExporer.GetJsonSchemaAsNode has an extra heuristic that will mark constructor parameters of classes as required in the schema it creates.

This means that it does not match the runtime behavior. Properties are marked as required that are not required at runtime.

However, this heuristic does not apply to structs. Switching the same definition between record class and record struct results in different schemas but the same deserialization behavior. (See repro).

This is similar to the discussion here: dotnet/extensions#6080

The JsonTypeInfo is different for record struct and record because the record struct will have a parameterless constructor. As a result there are behavior differences when constructor parameter definitions/attributes result in different schemas for classes and structs.

--

This heuristic is unhelpful for my scenario 😢 and in an ideal world I could turn it off entirely.

I'm using the schemas to understand what the serializer will accept, and to diff changes to our code over time for breaking change detection. Any place where the schema generation is adding additional heuristics or assumptions that don't match the runtime behavior is another thing I have to work around.

I discovered this as part of a change where someone flipped a class to a struct, and the properties became non-required in the schema.

Reproduction Steps

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


var opts = new JsonSerializerOptions(JsonSerializerDefaults.Web)
{
    PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
    WriteIndented = true,
};

void Test<T>(string json)
{
    JsonTypeInfo info = JsonSerializerOptions.Default.GetTypeInfo(typeof(T));
    foreach (var property in info.Properties)
    {
        if (property.IsRequired)
        {
            Console.WriteLine($"Property {property.Name} in {typeof(T).Name} is required.");
        }
        else
        {
            Console.WriteLine($"Property {property.Name} in {typeof(T).Name} is optional.");
        }
    }

    var schema = info.GetJsonSchemaAsNode();
    Console.WriteLine($"JSON Schema for {typeof(T).Name}:\n{schema.ToJsonString(opts)}");

    try
    {
        var obj = JsonSerializer.Deserialize<T>(json);
        Console.WriteLine($"Deserialized {typeof(T).Name} successfully: {obj}");
    }
    catch (JsonException ex)
    {
        Console.WriteLine($"Failed to deserialize {typeof(T).Name}: {ex.Message}");
    }
}

var json = "{}";
Test<AClass>(json);
Test<AStruct>(json);

public sealed record AClass(string Name, DateTimeOffset Birthday);
public readonly record struct AStruct(string Name, DateTimeOffset Birthday);

Expected behavior

I'd really like the schema output to match the runtime behavior. In this example that would be AClass having no required properties.

Actual behavior

Property Name in AClass is optional.
Property Birthday in AClass is optional.
JSON Schema for AClass:
{
  "type": [
    "object",
    "null"
  ],
  "properties": {
    "Name": {
      "type": "string"
    },
    "Birthday": {
      "type": "string",
      "format": "date-time"
    }
  },
  "required": [
    "Name",
    "Birthday"
  ]
}
Deserialized AClass successfully: AClass { Name = , Birthday = 1/1/0001 12:00:00 AM +00:00 }
Property Name in AStruct is optional.
Property Birthday in AStruct is optional.
JSON Schema for AStruct:
{
  "type": "object",
  "properties": {
    "Name": {
      "type": "string"
    },
    "Birthday": {
      "type": "string",
      "format": "date-time"
    }
  }
}
Deserialized AStruct successfully: AStruct { Name = , Birthday = 1/1/0001 12:00:00 AM +00:00 }

Regression?

No response

Known Workarounds

I could TransformSchemaNode and re-implement my own version of required.

Configuration

.NET SDK:
 Version:           9.0.300
 Commit:            15606fe0a8
 Workload version:  9.0.300-manifests.c678e91b
 MSBuild version:   17.14.5+edd3bbf37

Other information

No response

[rynowak]

@eiriktsarpalis

[dotnet-policy-service]

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

[eiriktsarpalis]

Hey, as you might have already guessed the current behavior was chosen intentionally. We regard the default behavior of treating every constructor parameter as optional to be a bug, since the serializer effectively violates the contract of unmarshalled type by jamming default values in unspecified constructor parameters. This was addressed in .NET 9, however because of the huge potential for breaking changes the fix is actually opt-in via the RespectRequiredConstructorParameters flag. We strongly recommend that every new application turns on this flag.

When it comes to schema generation, this is something we had debated with the ASP.NET Core team when OpenAPI support was being worked on. The decision was made to invariably report required constructor parameters as required in the schema for two reasons:

1. It produces a richer schema that avoids violating the contract of the unmarshalled class and
2. Restricting the schema to a subset of what the serializer accepts is sound from a validation perspective: any valid JSON per the generated schema is guaranteed to be accepted by the serializer.

While we could recalibrate the schema to honor the setting of RespectRequiredConstructorParameters, I don't think this would be desirable from the perspective of the team owning the OpenAPI components. cc @captainsafia

My recommendation would be to always turn on RespectRequiredConstructorParameters (together with its sibling RespectNullableAnnotations flag), if possible. This should ensure a 1:1 correspondence between the C# type system, the STJ serializer, and the schema generator. If not, you might want to consider marking your constructor parameters as optional since the schema exporter will never surface these as required.

[rynowak]

Restricting the schema to a subset of what the serializer accepts is sound from a validation perspective: any valid JSON per the generated schema is guaranteed to be accepted by the serializer.

This part feels really misleading to me. Unless they added something schema based, ASP.NET Core validation doesn't work this way. So the schema will say that fields are required, and unless you set RespectRequiredConstructorParameters, nothing validates them.

My recommendation would be to always turn on RespectRequiredConstructorParameters (together with its sibling RespectNullableAnnotations flag),

This is actually how we got here 😢. Doing a risky migration like this across a large multi-team codebase requires tooling and the ability to understand deltas in behavior. I was hoping that the schemas could help provide visibility about changes in behavior - intended or not.

[eiriktsarpalis]

Restricting the schema to a subset of what the serializer accepts is sound from a validation perspective: any valid JSON per the generated schema is guaranteed to be accepted by the serializer.

This part feels really misleading to me. Unless they added something schema based, ASP.NET Core validation doesn't work this way. So the schema will say that fields are required, and unless you set RespectRequiredConstructorParameters, nothing validates them.

Even if ASP.NET Core doesn't insert server side validation on the basis of the generated OpenAPI contract (@captainsafia or @mikekistler might be able to confirm), a restricted schema is still helpful from the perspective of generating accurate clients.

[mikekistler]

I encountered this same problem some time back. I think the solution is to supply default values on any primary constructor parameters that you actually want to be optional, e.g.

public sealed record AClass(string Name = default, DateTimeOffset Birthday = default);

I believe the schema for AClass when defined this way will be the same whether it is record class or record struct.

But I don't disagree that having an option to disable the heuristic would also be nice.