Open commonsensesoftware opened 2 years ago
Triage: We should look at doing something similar to MVC
https://github.com/dotnet/aspnetcore/blob/2d2295183b26b98c386d31c29403328e9c1f53ef/src/Mvc/Mvc.ApiExplorer/src/DefaultApiDescriptionProvider.cs#L284
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.
@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.
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
@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.
@adityamandaleeka, this was in the .NET 8 Planning milestone but it's in the servicing project board. I've removed the milestone so your team can decide where this should go.
Is there an existing issue for this?
Describe the bug
EndpointMetadataApiDescriptionProvider
misses creatingApiDescriptionParameter
instances that are defined asBindingSource.Custom
orBindingSource.Special
. As seen here,EndpointMetadataApiDescriptionProvider
only considers parameter candidates via theParameterInfo
acquired through Reflection.Some
BindingSource.Special
parameters should be ignored, such asCancellationToken
. API Versioning defines theApiVersion
asBindingSource.Special
because the value can typically come from one or more places (even zero is technically possible). The value ofApiVersion
is set and retrieved throughIApiVersioningFeature
.This behavior is a deviation from
DefaultApiDescriptionProvider
, which considers and creates anApiParameterDescription
for route parameters, even if they don't have a formalParameterInfo
partner (as seen here).Declaring the formal parameter
ApiVersion version
will not work becauseTryParse
is delegated toIApiVersionParser
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 anApiParameterDescription
for every route parameter in theRoutePattern
(e.g. template); regardless of whether there is a corresponding method parameter.Expected Behavior
EndpointMetadataApiDescriptionProvider
should have parity withDefaultApiDescriptionProvider
wherever possible (obviously modeling binding isn't available for Minimal APIs).Consider the following:
This should produce
ApiDescription.RelativePath
withv1/weatherforecast/{city}
, but instead producesv{version}/weatherforecast/{city}
. Worse still, there is noApiParameterDescription
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