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 ?