Skip to content

ApiVersionCollator does not filter actions for IApiBehaviorMetadata leading to actions with same controller name as api controller names getting grouped with it and leading to "Unsupported API Version" errors. #839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rwasef1830 opened this issue Jun 4, 2022 · 5 comments
Assignees

Comments

@rwasef1830
Copy link

rwasef1830 commented Jun 4, 2022

Hello,
ApiVersionCollator groups actions by controller name and adds versioning metadata, however it fails to filter the actions list to those that are actually defined as having ApiBehavior (as defined by having IApiBehaviorMetadata which is added by [ApiController] attribute on such controllers or base classes thereof).

This leads to the side effect that if a user had 2 controllers with the same name, but different area and different namespace and different everything, one of them is an API and the other is not, accessing the non-API controller will lead to thrown errors from Endpoint routing about unsupported or unspecified API version (depending on whether ApiVersioningOptions.AssumeDefaultVersionWhenUnspecified is true or false respectively).

This behavior is very confusing and unreasonable for users with no clear way to debug or disambiguate, and there is no logging in the API collation process to troubleshoot this issue.

This kind of scenario is very common in applications such as ecommerce where there will be an API for retrieving products while at the same time an administration controller with the same name to manage such products on the store.

Note: It appears there's some filtering in another area of the library to check for this case, but this filtering is not done on the routing side which I guess is a simple oversight. Anyway, thanks for a very useful library regardless.

Here is a workaround to save others time:

class ApiBehaviorVersionCollator : ApiVersionCollator
{
    public ApiBehaviorVersionCollator(IControllerNameConvention namingConvention) : base(namingConvention)
    {
    }

    public override void OnProvidersExecuted(ActionDescriptorProviderContext context)
    {
        var newContext = new ActionDescriptorProviderContext();
        foreach (var action in context.Results)
        {
            if (!action.EndpointMetadata.OfType<IApiBehaviorMetadata>().Any())
            {
                continue;
            }

            newContext.Results.Add(action);
        }

        base.OnProvidersExecuted(newContext);
    }
}

then in the Startup sequence after calling AddApiVersioning().AddMvc(), search in the service collection for the descriptor having implementation type ApiVersionCollator and replace it with the one above.

Hope this helps someone.

@commonsensesoftware
Copy link
Collaborator

Do you happen to have a simple repro for this? It would seem there is a particular combination that results in this behavior.

API Versioning doesn't specifically care about IApiBehaviorMetadata, but some time ago it added support for IApiControllerFilter and IApiControllerSpecification. The filter is an aggregation of all specifications. There are two out-of-the-box specifications, one for IApiBehaviorMetadata and one for ODataAttributeRoutingAttribute. Specifications can be added or replaced as desired, or the entire filter can be replaced.

Using the filter in the ApiVersionCollator isn't an oversight; it shouldn't be required. The Controller Model requires MVC and has its own construction process via the Application Model. The process of discovering and adding API Versioning metadata occurs via the ApiVersioningApplicationModelProvider which implements IApplicationModelProvider and uses IApiControllerFilter. The application model is built and processed before IActionDescriptorProvider are ever executed. This should negate the need to run through the filter again. Any controller that isn't versioned will yield empty metadata.

... 🤔

As I'm typing this out and reviewing the code, it would seem that the real issue is that otherwise unversioned endpoints are receiving collated metadata that they shouldn't. This would be a reason to run through the filter again. I think this might actually be a 🐞 . I'm a bit surprised it hasn't come up long before now. A little more discussion and perhaps a repro would confirm it. Fortunately, the fix is trivial.

@rwasef1830
Copy link
Author

@commonsensesoftware here you go: https://github.com/rwasef1830/asp-versioning-bug-839-repro
Try to visit the administration product controller, and the api product controller (both are inside areas).

@commonsensesoftware
Copy link
Collaborator

Great. Thanks. I'll look into it.

@commonsensesoftware
Copy link
Collaborator

Confirmed. It's a 🐞 . It'll be fixed in the next release. Your current workaround is feasible for your scenario.

This turned out be a little trickier than I thought. The reason IApiControllerFilter and IApiControllerSpecification can't be used here is that none of the Application Model information is available on an ActionDescriptor. The next best test is to see if any API Versioning metadata is applied to the action endpoint. If yes, then it must be versioned (and previously passed the filter). If no or empty, then it's assumed not to be.

@commonsensesoftware
Copy link
Collaborator

6.0 has been officially released and contains the fix for this issue. Thanks for reporting it as well as for providing a repro. If the issue returns, feel free to reopen the issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants