aspnet-api-versioning icon indicating copy to clipboard operation
aspnet-api-versioning copied to clipboard

IApiVersionSelector.SelectVersion for method without ApiVersionAttribute gets ApiVersionModel with ApiVersion from other methods

Open kwaazaar opened this issue 3 years ago • 6 comments
trafficstars

My controller has 3 methods of which 2 have an ApiVersionAttribute specified. When my custom IApiVersionSelector is invoked, I expect the ApiVersionModel to have SupportedApiVersions and ImplementedApiVersions empty (or contain the DefaultApiVersion). Instead they contain the version implemented by the other 2 methods.

kwaazaar avatar May 09 '22 13:05 kwaazaar

Do you have an example or, better still, a repro that highlights the issue you are experiencing? I don't have a clear picture of exactly what is happening.

IApiVersionSelector.SelectApiVersion is typically provided an aggregated model - by design. Given a request and complete ApiVersionModel, choose the most appropriate ApiVersion. Aggregation is always done by controller (e.g. API) name.

commonsensesoftware avatar May 09 '22 16:05 commonsensesoftware

Hi @commonsensesoftware. I added a repro project here: https://github.com/kwaazaar/api-versioning-828-repro In the readme I tried to describe what I am trying to achieve.

kwaazaar avatar May 10 '22 09:05 kwaazaar

Thanks for the repro; it will be useful. I don't quite think I've seen this setup, but it should work. I already see a few clarifying points.

  1. FYI, the DefaultApiVersion is ApiVersion.Default, which is 1.0. If you want it explicit, no problem.
  2. You can't really prevent Status from being specified or parsed in an ApiVersion, but if there is no corresponding definition, it will never match a. An ApiVersion with Status always ranks lower than one without Status
  3. Once you opt into API Versioning, there is no such thing as unversioned*
  4. The built-in LowestImplementedApiVersionSelector appears to serve your selection needs without having to create a custom IApiVersionSelector

Versioning APIs should always be explicit for the sake of clients. Early on that meant that you shouldn't be able to have an unversioned API. It's really easy to make a mistake too. I also understood that retrofitting a large codebase with a bunch of attributes would be untenable. Attributes are one way of expressing the metadata, but not the only one. In the end, API Versioning doesn't care about attributes, it only cares about IApiVersionProvider instances, which can come from attributes. It can also come from conventions (or any other custom exposed source).

Knowing that grandfathering in existing services in a large codebase is a real thing, that led to several features (even if they aren't always used that way).

  • AssumeDefaultVersionWhenUnspecified = false : existing client prior to explicit versioning won't know to request an API version and will break if they don't. This feature allows opting in to allow the API version to be optional on request. (This feature was only ever meant for this purpose)
  • IApiVersionSelector : what should the rules for version selection be? Should it be the first version available? Should it be the current version? Should it be some constant version? What if the versions are asymmetrical across APIs? Some common scenarios are provided out of the box to choose from or you can roll your own.
  • DefaultApiVersion : if every API must be versioned, what version should it have if I don't decorate it? While there other uses, this was initially meant to facilitate not having to use attributes on tens, or even hundreds, of controllers or actions.

The consequence of these behaviors are that a controller (or action) without any attributes still has API versions. You simply can't have none - sort of. You can indicate that an API is version-neutral, which means that it accepts any and all API versions, including none. I have sometimes referred to it as opting out because it's easier for some people to understand, but - really - it means you accept everything. This has its own side effects - namely you cannot mix it with other types or API versions. Any attempt to do so will result in exceptions. The use case for version-neutral are APIs that will never change such a heartbeat endpoint like /ping or a DELETE method.

The only other way around these behaviors is to change to the IApiControllerFilter service and/or add/change one of the IApiControllerSpecification implementations. The IApiControllerFilter is just an aggregation of all the IApiControllerSpecification implementations. An IApiControllerSpecification determines whether an API controller should be considered for API Versioning. In ASP.NET MVC Core, a controller is a controller. There is no differentiation between a UI controller, Razor page, or API controller. This used to be problematic for people that mix and match UI with APIs. Once API Behaviors were introduced this made it easy to build a feature around. By default, there is an IApiControllerSpecification that considers whether a controller has [ApiController]. If it does, then it's considered for API versioning; otherwise, it's ignored. With minimal effort, that because a good way to disambiguate APIs from UI. There is a similar, but albeit different, specification to match OData controllers which typically do not have [ApiController] applied. You could use these capabilities to completely ignore certain controllers, but it's not without some work. You'd probably have to split out the implementation. Filtering is at the controller level only.

The final bit to the mystery likely has to do with how API versions are collated. Grouping is done on the controller name. This might seem strange, but it's the only reliable piece of information to collate on. For example, values/{id} and values/{id:int} are semantically equivalent, but do you want them grouped together? In the case probably. Now consider order/{id} and order/{id}/items. Should these be grouped together? It's no so clear. At some point in the future, I may introduce a service that allows you to define/redefine how collation is done, but it's more complicated than most consider.

In your repro, all the actions are on the WeatherForecast controller. Despite having no attribution, I would expect that weatherforecast/tomorrow has a declared version of 1.0 from DefautlApiVersion and will list the supported versions as 1.0, 2.0. Since collation pivots on name, no consideration is made for route template or HTTP method; that is by design. There are details that simply cannot be conveyed in an API version alone. The mental model for most developers are the API versions for the _"Weather Forecast" API, not specifically /weatherforecast/today. Other means such as OpenAPI (formerly Swagger) are better at conveying which APIs are available in which versions.

Finally, consider the terminology of collated API versions:

  • Declared - API versions explicitly declared by attribute or convention on a controller or action. A declared API version is also used for explicitly mappings (e.g. [MapToApiVersion])
  • Supported - Active, supported API versions
  • Deprecated - API versions that will be going away in the future (the new Sunset Policies allow conveying when)
  • Implemented - Simply Supported + Deprecated, which also includes advertised versions
  • Advertised - Active, supported API versions implemented elsewhere (useful for gateways with split implementations)
  • Advertise Deprecated - Deprecated API versions implemented elsewhere (useful for gateways with split implementations)

The aggregated ApiVersionModel does not retain declared or advertised API versions. In the context of aggregation, it doesn't make any sense. When going through IApiVersionSelector, an aggregated model of all possible candidates is produced with the current request for you to select the most appropriate API version. You know not assume any other information from the aggregated model. The current request is provided so that you can use out-of-band information to make a more appropriate decision.

I'll take a deeper look at the repro, but I'm pretty sure that is what is happening. Hopefully that provides some insight into what you are seeing.

commonsensesoftware avatar May 10 '22 16:05 commonsensesoftware

Thanks for the extensive reply :-)

In your repro, all the actions are on the WeatherForecast controller. Despite having no attribution, I would expect that weatherforecast/tomorrow has a declared version of 1.0 from DefautlApiVersion and will list the supported versions as 1.0, 2.0. Since collation pivots on name, no consideration is made for route template or HTTP method; that is by design.

If I understand correct, this would also mean that weatherforecast/tomorrow should work when passed any of the supported versions. This however is not the case and returns this error:

{"error":{"code":"UnsupportedApiVersion","message":"The HTTP resource that matches the request URI 'https://localhost:7227/WeatherForecast/tomorrow' with API version '2.0' does not support HTTP method 'GET'.","innerError":null}}

It does however return the api-supported-versions header with value 2.0.

Addition:

  • I tried adding VersionNeutral to (a base-class for) the controller: however this makes all API's versionneutral, even when they have an ApiVersion attribute set explicitly.
  • I tried adding ApiVersion-attribute to (a base-class for) the controller: this does not seem to have any effect on the api without an ApiVersion attribute.
  • I tried adding VersionNeutral to the tomorrow api: this works great. The CustomApiVersionSelector does however receive a model that states that VersionNeutral is false. Nevertheless, this works.
  • I added a new controller (WeatherOnMarsForecastController) that has no ApiVersion-attributes on any of its APIs. All those can be invoked without api-version parameter and get assigned the DefaultApiVersion by the CustomApiVersionSelector and also work. This means that my issue is really caused by having a single controller with APIs with and without ApiVersion attributes.

kwaazaar avatar May 11 '22 09:05 kwaazaar

Just a quick follow up. I realized I hadn't replied to your last post.

  • Yes, once an API is declared version-neutral there is no scenario where you can also have an explicitly versioned variant; it's ambiguous
  • ApiVersionAtribute is not inherited - by design. Inheritance is the wrong way to thing about API design IMHO; however, the primary reason it behaves like this is to avoid unexpected consequences. If you ControllerB inherits ControllerA, then ControllerB likely has the same route template and API versions. There is now an ambiguous situation where both controllers have the same API versions and routes. To ensure that can't happen, the attributes are not inherited. If you really want to use inheritance, I suggest limiting it to common functionality that is not publicly exposed as an action. Remember, you can't uninherit it an action down the line.

I'll take a deeper look at the repro.

commonsensesoftware avatar Jun 14 '22 17:06 commonsensesoftware

Having pulled down your repro and run it, I can't reproduce the results you're seeing. Everything seems to be working. I didn't change anything. A few clarifying points:

You can't have unversioned APIs

This was by design to solve the exact problem you are facing with a large codebase. If you opt into API Versioning, you shouldn't have to touch all of the controllers. Without any attribution or convention applied, the associated API version will be ApiVersioningOptions.DefaultApiVersion.

Version-neutral is the closest thing you can get to opting out or having an unversioned API, but it's a bit of a misnomer. A version-neutral API can accept any and all API versions, including none.

Weather Forecast Tomorrow 2.0 Fails

I couldn't reproduce this behavior. Requesting:

WeatherForecast/tomorrow?api-version=2.0

Returns exactly what I would expect and with success.

SelectVersion Does Not Receive Expected Model

At least for WeatherForecast/tomorrow, I always saw the model was empty. This would be expected for an API version-neutral endpoint. Since APIs are grouped by logical name (usually controller name), it does beg the question as to whether API versions should still be aggregated. API version reporting is also skipped for version-neutral endpoints.

An important distinction about how IApiVersionSelector.SelectVersion works is that you are not selecting a version for a known endpoint. The route policy aggregates the API versions from all known candidates and presents them to you with the current request. It's up to you to figure out which API version should match the request. The action or API itself is typically unknown at this point in the request, which is exactly why it provides a hook. I mean, if the policy knew the answer, it wouldn't have to ask. 😉 An aggregated model, therefore, will not indicate that it's version-neutral. A version-neutral model will not have any API versions to report. Aggregating it will not add any new result. Aggregation never reports a version-neutral model. I suppose it could if, and only if, all the models were API version-neutral, but that would be unlikely or even impossible. That should mean there are ambiguous routes (e.g. 2+ API candidates for a given request that are both version-neutral).

commonsensesoftware avatar Jun 14 '22 18:06 commonsensesoftware

Thanks for the repro; it was helpful. The discussion has gone idle, so I presume you have resolved your issue and/or digested my explanation. If your issue has not been resolved, I'm happy to reopen this issue and discuss it further. Thanks.

commonsensesoftware avatar Aug 24 '22 02:08 commonsensesoftware