EndpointMetadataApiDescriptionProvider Misses Route Parameters

[commonsensesoftware]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

EndpointMetadataApiDescriptionProvider misses creating ApiDescriptionParameter instances that are defined as BindingSource.Custom or BindingSource.Special. As seen here, EndpointMetadataApiDescriptionProvider only considers parameter candidates via the ParameterInfo acquired through Reflection.

Some BindingSource.Special parameters should be ignored, such as CancellationToken. API Versioning defines the ApiVersion as BindingSource.Special because the value can typically come from one or more places (even zero is technically possible). The value of ApiVersion is set and retrieved through IApiVersioningFeature.

This behavior is a deviation from DefaultApiDescriptionProvider, which considers and creates an ApiParameterDescription for route parameters, even if they don't have a formal ParameterInfo partner (as seen here).

Declaring the formal parameter ApiVersion version will not work because TryParse is delegated to IApiVersionParser so that a developer can opt to change the behavior if they want to. Parsing the value at this point in the pipeline is also too late and affects routing or would result in parsing the value more than once.

Ultimately, EndpointMetadataApiDescriptionProvider should create an ApiParameterDescription for every route parameter in the RoutePattern (e.g. template); regardless of whether there is a corresponding method parameter.

It should be noted that this behavior does not affect routing and it only happens when a developer elects to version solely by URL segment.

Expected Behavior

EndpointMetadataApiDescriptionProvider should have parity with DefaultApiDescriptionProvider wherever possible (obviously modeling binding isn't available for Minimal APIs).

Consider the following:

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddEndpointsApiExplorer();
builder.Services.AddApiVersioning(options => options.ApiVersionReader = new UrlSegmentApiVersionReader())
                .AddApiExplorer(options => options.options.SubstituteApiVersionInUrl = true);

app.MapGet( "v{version:apiVersion}/weatherforecast/{city}", () =>
    {
        return Enumerable.Range( 1, 5 ).Select( index =>
            new WeatherForecast
            (
                DateTime.Now.AddDays( index ),
                Random.Shared.Next( -20, 55 ),
                summaries[Random.Shared.Next( summaries.Length )]
            ) );
    } )
   .WithApiVersionSet(app.NewApiVersionSet().HasApiVersion(1.0).Build());

This should produce ApiDescription.RelativePath with v1/weatherforecast/{city}, but instead produces v{version}/weatherforecast/{city}. Worse still, there is no ApiParameterDescription for {version}. API Versioning relies on the existence of the parameter description and substitutes the token in the template with the corresponding API version value.

This behavior can be reproduced without API Versioning from a pure API Explorer perspective, but the API will not be reachable since the route parameter wouldn't be matched.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

6.0.300

Anything else?

This was first reported in dotnet/aspnet-api-versioning#830.

ASP.NET Core 6.0

.NET SDK (reflecting any global.json):
 Version:   6.0.300
 Commit:    8473146e7d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.22000
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.300\

Host (useful for support):
  Version: 6.0.5
  Commit:  70ae3df4a6

.NET SDKs installed:
  6.0.202 [C:\Program Files\dotnet\sdk]
  6.0.203 [C:\Program Files\dotnet\sdk]
  6.0.300 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.25 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.25 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 6.0.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

[BrennanConroy]

Triage: We should look at doing something similar to MVC

aspnetcore/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs

Line 284 in 2d22951

foreach (var routeParameter in routeParameters)

where we still set the ApiParameterDescription when there isn't a parameter in the delegate.

Second, we need to think about the ApiDescription.RelativePath part.

[commonsensesoftware]

@BrennanConroy I don't need a date, but will this be patched in 6.0? I need to provide guidance to customers around how to address this situation in the interim.

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[captainsafia]

Ultimately, EndpointMetadataApiDescriptionProvider should create an ApiParameterDescription for every route parameter in the RoutePattern (e.g. template); regardless of whether there is a corresponding method parameter.

I don't necessarily agree with this perspective. The way I see it, the method signature should be the source of truth for API annotations about a particular endpoint, not the route parameter.

@commonsensesoftware Have you looked into leveraging route groups (.NET 7) in the API versioning package at all? It might help alleviate the particular issue here. I'm imagining that calling:

ApiVersion apiV1 = new ApiVersion(1);

Would great a RouteGroupBuilder under the hood that users can attach their mappings to.

cc: @halter73

[commonsensesoftware]

@captainsafia Unfortunately, the source of truth is not the method signature. HTTP is the API. C#, just like any other language, has an impedance mismatch. It's no different than the incongruence between C# (say - a la EF) and SQL. There are just some things that cannot be expressed in C# because it's not HTTP.

An API version is special in terms of how it matches up in the code. The call site (e.g. method) not necessarily have to be present for the endpoint to be invoked. This is the same as the CancellationToken. Every method can support it, regardless of whether you formally declare it. The incongruence, however, can be highlighted in other ways. It's perfectly acceptable for a HTTP API to accept the query parameter api-version and the header x-api-version. API Versioning understands this nuance, validates either is specified or both are the same, and resolves a single value that is provided to the bound C# method. Two method arguments are neither required nor wanted.

API authors need to describe their APIs terms of HTTP because that's how their clients will call them. An author trusts that API Versioning will call the correct endpoint so they may not have any interest in declaring the code parameter because it would never be used. However, a client needs to know which parameters they can specify, which might be more than one (ex: header and query string).

A route group will not address this specific issue. Grouping constructs are useful and will be integrated into API Versioning, but it won't help here. As it stands, an ApiVersion parameter can currently only be provided via a ModelBinder. Model binding isn't supported for Minimal APIs, but as I recall there is a new platform hook to address this (offhand, the name escapes me). Serialization is also impossible because the value doesn't even have to come from the HTTP request. It's possible and supported for code to explicitly set the resolved API version at a particular point in the pipeline.

There's really no compelling reason to not have to two flavors of API Explorer be incongruent. This is a violation of POLA. It is completely reasonable that a route parameter can exist that isn't defined in the method signature. Furthermore, any unknown route parameter value can always default to using the parameter name defined in the parsed template and assume the value is a string if the platform cannot determine otherwise.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.