ApiExplorer doesn't select the correct action when using MapToApiVersion

[doubret]

I have this simple controller :

[ApiVersion("1.0")]
[ApiVersion("2.0")]
[ApiVersion("3.0")]
[RoutePrefix("api/v{v:apiVersion}/healthcheck")]
public class HealthCheckController : ApiController
{
    [HttpGet]
    [AllowAnonymous]
    [Route("")]
    public HttpResponseMessage Get()
    {
        return new HttpResponseMessage { Content = new StringContent("OK") };
    }

    [HttpGet, AllowAnonymous]
    [Route("")]
    [MapToApiVersion("2.0")]
    public ActionResponse GetV2()
    {
        return ActionResponse.Create(null, "toto");
    }
}

The VersionedApiExplorer fails to pick the correct action for version 2.0.

This is because in the InitializeApiDescriptions method, when it sees the Get() method of my controller, it considers that the method fits the version, then later it sees the GetV2 method but doesn't select it because it already found an action for the route.

As a quick fix i modified the InitializeApiDescriptions like this :

                    if (!apiDescriptionGroup.ApiDescriptions.Contains(description, Comparer))
                    {
                        description.GroupName = apiDescriptionGroup.Name;
                        apiDescriptionGroup.ApiDescriptions.Add(description);
                    }
                    else
                    {
                        var model = description.ActionDescriptor.GetApiVersionModel();
                        var mapTo = model.DeclaredApiVersions.Contains(apiVersion);

                        if (mapTo)
                        {
                            var previous = apiDescriptionGroup.ApiDescriptions.Single(x => Comparer.Equals(x, description));

                            apiDescriptionGroup.ApiDescriptions.Remove(previous);

                            description.GroupName = apiDescriptionGroup.Name;
                            apiDescriptionGroup.ApiDescriptions.Add(description);
                        }
                    }

Probably not the best solution but at least it solved the problem for now.

[commonsensesoftware]

If I understand correctly, the version 2.0 is discovered and explored, but the API descriptor does not correspond to the correct method. In other words, Get is reported as the ActionDescriptor for version 2.0 instead of GetV2.

I had to completely re-implement the built-in IApiExplorer so I'm not totally surprised that edge case issues like this exist. Thanks for not only reporting it, but helping track down where the issue is. I'll look into it and add your setup to the test suite.

[doubret]

Yes it is the problem, sort of.
From what I saw actions are processed one by one but it can't work this way, all actions must be taken into account when looking for candidates because ApiVersion attribute is set on the controller and MapToVersion is set on actions.
In the end only routes and actions should matter, not controllers.

Is there a method to know the exact versions for a given action ? Something like all versions specified at their controller level if there's no MapToVersion attribute (the versions specified in the MapToVersion attribute otherwise), minus versions for the same route that are already assigned to other actions. This way it would greatly simplify the candidates selection.

I thought it was done by GetApiVersionModel() but apparently not.

Thanks for the great work !

[doubret]

Another solution, more simple would be to process action in 2 pass. First pass only actions with the MapToVersion attributes, then in the second pass the actions that don't have the attribute (taking versions defined at the controller level).

This way you always give precedence to the MapToAttribute...

[commonsensesoftware]

Confirmed. This is a bug. I had a test case for using MapToApiVersion, but it was only affecting the inclusion of the action. This particular variation is an issue where the wrong action is selected. Your proposed fixed was nearly spot on. I only made slight optimization to reduce the number of collection enumeration/lookups.

You're right that ultimately, only actions matter. This is why the routing infrastructure was completely changed in ASP.NET Core and there is no longer IHttpController or HttpControllerDescriptor. Unfortunately, it's no so easy to get all the versions for an action. Things are heavily reliant on the routing infrastructure for aggregation. During normal runtime execution, the best candidate is matched and the API versions of all other candidates are aggregated and cached with the matching candidate.

GetApiVersionModel is the right way to get this information. The MapToApiVersionAttribute isn't the only way this information can be applied. It can also be done using conventions. Unfortunately, when you aren't making a web request, things don't go through the router. Without all of the possible candidates, there's nothing to aggregate. The GetApiVersionModel method can always build a model for the current action and/or controller; however, the API versioning infrastructure will update this model exactly once at runtime per action/controller with all aggregated information. This is why you don't necessarily see all API version model information returned - depending on when/where/how it's called. Without routing, it's impossible to match candidates. The router's whole job is to match URLs to possible controller types. API versioning can't jump in until that happens.

The Web API implementation is quite painful. The API explorer has to basically simulate and reproduce what the router normally does. This is part of the reason it's taken so long to implement this feature. Fortunately, things were careful rethought for ASP.NET Core. In comparison, that implementation was almost easy. The API explorer aggregates the results in to ApiDescriptionGroups. It would seem that should negate any need to update the model backing GetApiVersionModel; however, if that's something that people think they need for documentation, I'm open to supporting that.

In terms of the matching logic, your thought process was pretty close to what's needed. In short, an action could be implicitly matched. If the same semantically equivalent action is discovered again, it is skipped. There should be only one variation. If an equivalent action is discovered, but it is explicitly mapped to the current version, by attribute or convention, then that action should replace any currently matched action. Any future implicitly matched actions should never change explicit matches. If there are true duplicates, then one of them should be skipped, which matches the default behavior. At runtime, such a configuration will cause a 500 because the routes are ambiguous by both route path and API version (e.g. a developer mistake).

Expect to see a PR with this fix soon. I'll try to publish a new package as soon as possible. Thanks.

[doubret]

In my opinion, route matching and version matching should be different concerns.

Given an action we should be able to easily know the exact versions that map to it. This is almost achieved by GetApiVersionModel but not completely (as you need to look at both action level and controller level, and in theory potentially all other actions and controllers but in practice probably rarely).

Given a route we should be able to find actions behind a given route. It's already done by the selector and if more than one actions matches a route it's considered an error.

With this in mind, it looks like the VersionedApiExplorer could be simplified a lot.

About the different options to declare versions (attributes and builder) I guess they are both merged into a common structure and not accounted for separately.

Am I missing something here ?

[commonsensesoftware]

Given that we are talking about an HTTP stack, API versions are tied to a URL (e.g. the surface area). Mapping a controller, action, class, or method is an implementation detail that is specific to using ASP.NET. Furthermore, the implementation can be graphed onto URLs in all kinds of ways, many of which you and I probably think is bizarre, but someone else thinks is interesting.

In general, things are quite simplified by letting routing do it it's thing. It finds all the candidates given a URL. Normally, this would be an error when there are duplicate candidates, but API versioning disambiguates them by API version. Routing is mess in Web API. Direct routing (aka attribute routing) was bolted on and doesn't just behave the way you would hope.

The design of Web API assumes that there can be only one controller and implementation per route. IHttpControllerSelector.GetControllerMapping returns a dictionary of controller name to HttpControllerDescriptor. I had to beat the API into submission because this just won't do in API versioning since there are potentially many controller implementations for a given route. Fortunately, this issue and limitation doesn't exist in ASP.NET Core. The IHttpControllerSelector also requires a HttpRequestMessage in order to select a controller. I won't defend these aspects of the Web API design. Unfortunately, there's only so much I can do about it. The best starting place was to fork the working API explorer created by the ASP.NET team and then rework it to support API versioning. The original implementation was not extensible at all and many of the key things that it uses relies on internal types (>_<).

You actually do not need to look at both the controller and action when you use GetApiVersionModel. When you request the action level, it will include the [immediate] controller level values. Controllers can be thought of as the service. Operations on a service do not have API versions, their defining services do. An action, however, can map to a specific API version implementation. The supported and deprecated API versions, therefore, always come from the defining controller.

For API version discovery, the infrastructure doesn't care about attributes. It only cares about IApiVersionProvider. Attributes are one way that a provider is realized. They can also be realized through conventions. In fact, you can even mix and match. Attributes will declaratively take precedence over conventions. The ApiVersionModel has several constructors and extension methods as shortcuts to resolve defined API versions or aggregate them into a single model.

The ApiVersionModel and all related extension methods aren't just there for extension and customization. It's a core foundation that is used throughout the libraries. With the single exception of updating a controller or action with the aggregated model when necessary, all other functionality uses the same public surface area used by adopters.

[doubret]

Thanks for the detailed explanations.

Let's take this simple controller without ambigous route :

[ApiVersion("1.0")]
[ApiVersion("2.0")]
[ApiVersion("3.0")]
[RoutePrefix("api/v{v:apiVersion}/test")]
public class TestController : ApiController
{
	[HttpGet]
	[Route("action-1")]
	public int Action1()
	{
		return 0;
	}

	[HttpGet]
	[Route("action-2")]
	[MapToApiVersion("2.0")]
	public string Action2()
	{
		return "toto";
	}
}

When i call GetApiVersionModel() for Action1 declared, implemented and supported versions are empty (i would expect that at least one of the properties list versions 1.0, 2.0, and 3.0).

For Action2, declared, implemented and supported indicate 2.0 (that makes sense because this action is bound to version 2.0 only).

I think i understand the differences between declared, implemented and supported versions but why is there no property to retrieve effective versions from an action descriptor ?
Is there an extension method available to get that ?

[doubret]

I continue to think about it... ;-)

Let's take this controller :

[ApiVersion("1.0")]
[ApiVersion("2.0")]
[ApiVersion("3.0")]
[RoutePrefix("api/v{v:apiVersion}/test")]
public class TestController : ApiController
{
	[HttpGet]
	[AllowAnonymous]
	[Route("action")]
	public int Action1()
	{
		return 0;
	}

	[HttpGet, AllowAnonymous]
	[Route("action")]
	[MapToApiVersion("2.0")]
	public string Action2()
	{
		return "toto";
	}
}

In this case, if we only consider versioning information, we cannot extract the desired semantics.
That is, we only know that Action1 can be invoked for versions 1, 2, 3 and that Action2 can be invoked only for version 2.
In order to know that Action2 overrides Action1 when version == 2 we need to consider routes because it's the only way to know that Action2 and Action1 are related together.

In this case, versioning is not self contained and introduces some coupling with routing.

Now if we write the same controller like this :

[ApiVersion("1.0")]
[ApiVersion("2.0")]
[ApiVersion("3.0")]
[RoutePrefix("api/v{v:apiVersion}/test")]
public class TestController : ApiController
{
	[HttpGet]
	[AllowAnonymous]
	[Route("action")]
	[MapToApiVersion("1.0")]
	[MapToApiVersion("3.0")]
	public int Action1()
	{
		return 0;
	}

	[HttpGet, AllowAnonymous]
	[Route("action")]
	[MapToApiVersion("2.0")]
	public string Action2()
	{
		return "toto";
	}
}

This way, we can tell for sure the versions available for each action without having to know how an action relates to another.
This requires a little more work on the developper side but simplifies greatly the versioning concern as it removes the coupling with routing.

Of course, if no MapToVersion is declared on an action, we can take the api versions declared on the controller.

The concept is different, MapToVersion doesn't make sense anymore in the second exemple.
It's more like allowing [ApiVersion] attributes to be set at action level, and overriding [ApiVersion] attributes set at controller level.

Is this something you already thought about ?

[commonsensesoftware]

I've ultimately been working on proving out these ideas for at least two years. Obviously being more explicit reduces complexity and helps eliminate a bunch of other problems. I started with everything being explicit and then loosened things more and more by convention. I for one recommend being explicit so that it's easier to rationalize. However, I feel it's important to give service authors choices and not require them to eat my dogma, though I'm happy to share my opinion.

On that note, a similar argument can be made that version interleaving should not be allowed or supported. If a controller represents a service, then you version a service not an action. If all versions are implemented with separate classes, then it's pretty easy to rationalize about. In general, I recommend that approach, but there are a few cases were you just might want to change one thing or add one new action. It's your choice as a service author to realize it with a new controller type or use version interleaving to map specific actions differently by version. To minimize the amount of decoration required to describe API versioning, I went with the decision to implicitly match everything unless there is a more specific, explicitly mapped alternative. This behavior has parity with controllers. Once API versioning is turned on, all controllers have a version, even if they don't decorate anything. The default value is provided by ApiVerisoningOptions.DefaultApiVersion. This behavior is largely to support grandfathering in existing services without changing all the code, though it can be used as a convenience for new services. If I haven't mentioned it before, there is no such thing as an unversioned service. The service either supports specific API versions - implicitly or explicitly, or the service supports all version (e.g. version-neutral).

I had similar debates with [internal] early adopters when the considering the public refactoring for the ApiVersionAttribute. In the earlier versions, it was possible specify multiple values for a single declaration. This is still possible, but not provided out-of-the-box. The primary reason was to keep things simple and easy to understand. In many cases, service authors won't interleave versions so no list is necessary. In the cases where service authors do interleave, it's unlikely they'd have more than 5 versions and very unlikely they'd have more than 10. From experience, I usually see about 1-3. Declaring the attribute multiple times, therefore, wasn't a huge deal and is arguably easier to read.

As you have deduced, MapToApiVersion and ApiVersion are actually the same thing. In the original implementation, there was no MapToApiVersion; there was only ApiVersion. This led to confusion because declaring ApiVersion at the action level was never used to select controllers (this is way before the ASP.NET Core days). It was difficult come up with a new name, but I ultimately settled on MapToApiVersion to try and make it clear that the action is being mapped to a particular API version. At the end of the day both attributes implement IApiVersionProvider. The declared API versions are the versions explicitly declared on a controller or action. The supported and deprecated versions always come from the controller level. These values are aggregated from all candidates at runtime. The declared API versions are never aggregated (or it would break the selection process).

You also have to consider convention-based routing and/or a mix with attribute-based routing. It's not hard for things to get hairy quickly. Good discussion. I hope that helps clarify things.