dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

Records, positional parameters, data annotations, model validation and OpenAPI inconsistency #57486

Open CaringDev opened 3 weeks ago

CaringDev commented 3 weeks ago

Is there an existing issue for this?

Describe the bug

When creating a .NET 9 (Preview 7) WepAPI project through means of dotnet new webapi -controllers, the new OpenAPI support is added by default.

Many developers have come to love DataAnnotations for increasing the likelihood incoming data matches their expectations.

After adding a record such as public record NormalNoOpenApi([StringLength(2)] string Payload); and a controller method e.g. public void Post(NormalNoOpenApi _) the data is indeed validated. However, the relevant schema entry in the OpenAPI document is missing the length constraints "maxLength": 2, "minLength": 0,:

"NormalNoOpenApi": {
  "required": ["payload"],
  "type": "object",
  "properties": {
    "payload": { "type": "string" }
  }
}

On the other hand, changing the record to public record NoValidation([property: StringLength(2)] string Payload); will lead to the correct schema but throw at runtime:

System.InvalidOperationException: Record type 'PRDA.NoValidation' has validation metadata defined on property 'Payload' that will be ignored. 'Payload' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter.

Going the extra mile and changing the record again: public record NoGood([property: StringLength(2)][param: StringLength(2)] string Payload); does not help: the schema is correct but we get the same exception.

Only changing to 'normal' properties helps:

public record Good
{
    [StringLength(2)] public required string Payload { get; init; }
}

Expected Behavior

The OpenAPI document should match the actual model validation behavior.

Steps To Reproduce

see https://github.com/CaringDev/PRDA for more context

Exceptions (if any)

System.InvalidOperationException: Record type '' has validation metadata defined on property '' that will be ignored. '' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter.

.NET Version

9.0.100-preview.7.24407.12

Anything else?

Microsoft.AspNetCore.OpenApi = 9.0.0-preview.7.24406.2

CaringDev commented 3 weeks ago

related: https://github.com/dotnet/aspnetcore/issues/39631 but now affecting the OpenAPI document generation

captainsafia commented 2 weeks ago

@CaringDev Thanks for filing this issue!

At the present moment, the ModelMetadata APIs that are used by the ApiExplorer to represent the parameter binding behaviors of type do not expose the properties that can be used to map properties back to constructor parameters (ref). This presents a cliff because there's no way to derive this information without explicitly reconstructing the mappings from properties to parameters yourself.

Conversely, as you've mentioned, we don't respect validation attributes on properties in MVC's model binding layer. I'm not sure why this constraint exists, other than perhaps there are stronger guarantees about the existence of a valid constructor for a given record type? cc: @javiercn for any insights as to why this is the case

We should evaluate what it would look like to make the currently internal APIs in ModelMetadata public so that implementors can consume the property -> parameter mappings.

Eirenarch commented 2 weeks ago

I'd like to point out that this also affects other projects that build on top of the ApiExplorer like Swashbuckle so fixing this would be helpful for the greater ecosystem and not only the new OpenAPI support

captainsafia commented 2 weeks ago

I'd like to point out that this also affects other projects that build on top of the ApiExplorer like Swashbuckle so fixing this would be helpful for the greater ecosystem and not only the new OpenAPI support

Yes -- making the existing parameter to property mappings public in ModelMetada is how this would be achieved.

However, this would have to wait until .NET 10 at this point since it requires API review.

Eirenarch commented 2 weeks ago

Yes -- making the existing parameter to property mappings public in ModelMetada is how this would be achieved.

I don't understand why you need to expose the parameter to property mapping. I admit I don't know how the API explorer works but I wrote my own Swashbuckle filter for that and it gets as an argument OpenApiSchema model which I assume comes from the API explorer it contains a list of properties (also of type OpenApiSchema) and I basically match the parameters that I get via reflection to the properties and set the attributes on the OpenApiSchema objects themselves. I assume there are more cases to consider but it seems to me that the first step should be to set things like Min/MaxLength and Pattern in the API explorer and return them. No ne APIs needed at least for that part. Or maybe I'm mistaken and this OpenApiSchema that I get in a Swashbuckle filter does not come from the API explorer?

captainsafia commented 2 weeks ago

Exposing the property-to-parameter mapping effectively provides a shortcut for the parameter to property lookup that you're currently doing in your code. The benefit is that the property-to-parameter mapping provides richer metadata about the properties being bound.

As for OpenApiSchema, that's not a type exposed by ApiExplorer. It's part of the Microsoft.OpenApi package. OpenApi implementers like Swashbuckle and Microsoft.AspNetCore.OpenApi consume metadata from ApiExplorer and map it to OpenApiSchema.

The property-to-parameter mappings expose an API for accessing the validation attributes that can then be mapped to the validation keywords in the OpenApiSchema (in external components, not in ApiExplorer itself).