Skip to content

405 Method Not Allowed, but Allow header lists the method that was requested #795

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
KalleOlaviNiemitalo opened this issue Dec 8, 2021 · 9 comments

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Dec 8, 2021

In version 1 of my API, I defined PUT and POST methods for the same route. In version 2, I removed the PUT method and left only POST. Now when I send a PUT request with version 2, I get a response with the 405 Method Not Allowed status, as expected. However, the response has an unexpected "Allow: POST, PUT" header. That should be just "Allow: POST" because the PUT method is no longer allowed in version 2.

Using Microsoft.AspNet.WebApi.Versioning/4.1.1 (i.e. release v5.0.0 in this repo) with Microsoft.AspNet.WebApi/5.2.7 (i.e. tag v3.2.7 in aspnet/AspNetWebStack) on .NET Framework. I defined an API controller with two action methods:

[ApiVersion("1")]
[ApiVersion("2")]
public class FileController : ApiController
{
    [HttpPost]
    [Route("v{apiVersion:apiVersion}/newFile")]
    public async Task<HttpResponseMessage> NewFile(HttpRequestMessage request) {}

    [Obsolete("Use the POST method.")]
    [HttpPut]
    [Route("v{apiVersion:apiVersion}/newFile")]
    [ApiVersion("1")] // Removed in version 2.
    public Task<HttpResponseMessage> NewFilePut(HttpRequestMessage request) {}
}

I tried moving the ApiVersion attributes from the FileController class to the NewFile method but that made no difference.

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Dec 8, 2021

Now I tried just hardcoding the API versions for this controller:

[ApiVersionNeutral]
public class FileController : ApiController
{
    [HttpPost]
    [Route("v1/newFile")]
    [Route("v2/newFile")]
    public async Task<HttpResponseMessage> NewFile(HttpRequestMessage request) {}

    [Obsolete("Use the POST method.")]
    [HttpPut]
    [Route("something-entirely-different")]
    public Task<HttpResponseMessage> NewFilePut(HttpRequestMessage request) {}
}

But got the same "Allow: POST, PUT" in the response. How does that make sense? I don't see why the HttpPut attribute of NewFilePut should affect the Allow header in the response at v2/newFile when the route of NewFilePut does not even match v2/newFile.

It seems that ApiVersionActionSelector.ActionSelectorCacheItem collects allowedMethods from all action methods of the API controller even though they may have different routes.

@commonsensesoftware
Copy link
Collaborator

Upon a cursory examination, I believe you are right. The methods would need to be bucketized with the corresponding API versions. It appears that this may only happen when you interleave versions on the same controller. If you separate things into 2 controllers, I think you'll get the expected results. I know that is inconvenient, but it may prove to be a viable workaround.

I'll have to look some more. I suspect there isn't a test case for this particular configuration. That will confirm the bug and set things up to be fixed.

@KalleOlaviNiemitalo
Copy link
Author

Microsoft.AspNetCore.Mvc.Versioning looks like it might not have the same bug, thanks to the IsMappedTo check here:

static HashSet<string> AllowedMethodsFromCandidates( IEnumerable<ActionDescriptor> candidates, ApiVersion? apiVersion )
{
var httpMethods = new HashSet<string>( StringComparer.OrdinalIgnoreCase );
foreach ( var candidate in candidates )
{
if ( candidate.IsMappedTo( apiVersion ) )
{
httpMethods.AddRange( GetHttpMethods( candidate ) );
}
}
return httpMethods;
}

@commonsensesoftware
Copy link
Collaborator

Agreed. Good 👀. This is an area where things are, unfortunately, quite different in implementation between the platforms. The implementation will be different, but the basic logic should be the same.

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Dec 9, 2021

If you separate things into 2 controllers, I think you'll get the expected results.

OK, that largely works:

[ApiVersion("1")]
public class FileV1Controller : ApiController
{
    [HttpPost]
    [Route("v{apiVersion:apiVersion}/newFile")]
    public string NewFile()
    {
        return "NewFile v1";
    }

    [HttpPut]
    [Route("v{apiVersion:apiVersion}/newFile")]
    public string NewFilePut()
    {
        return "NewFilePut v1";
    }
}

[ApiVersion("2")]
public class FileV2Controller : ApiController
{
    [HttpPost]
    [Route("v{apiVersion:apiVersion}/newFile")]
    public string NewFile()
    {
        return "NewFile v2";
    }
}

The results:

  • POST v1/newFile → HTTP/1.1 200 OK
  • PUT v1/newFile → HTTP/1.1 200 OK
  • POST v2/newFile → HTTP/1.1 200 OK
  • PUT v2/newFile → HTTP/1.1 405 Method Not Allowed; Allow: POST

But if I add another method to FileV2Controller:

    [HttpGet]
    [Route("v{apiVersion:apiVersion}/checksums")]
    public string Checksums()
    {
        return "Checksums";
    }

then it goes wrong again:

  • POST v1/newFile → HTTP/1.1 200 OK
  • PUT v1/newFile → HTTP/1.1 200 OK
  • POST v2/newFile → HTTP/1.1 200 OK
  • PUT v2/newFile → HTTP/1.1 405 Method Not Allowed; Allow: POST, GET

It seems that, to get the correct Allow header in the response, all action methods in a controller must have the same API versions and the same route template.

At this time, I think I can make my API easier to maintain by removing Microsoft.AspNet.WebApi.Versioning and replacing the v{apiVersion:apiVersion} path segments with v{apiVersion:int:range(1,2)}, than by rearranging the controllers.

@commonsensesoftware
Copy link
Collaborator

Thanks for the follow up. What you are observing is the correct and expected behavior. Contrary to popular belief, APIs are collated by the controller name and not route templates. A RESTful API is resource-oriented so it logically makes sense that things would be grouped that way, although there is no way to enforce that a service author implements it that way. While it might seem logical and simple to use route templates, in practice, it is not. The template is incomplete and has additional metadata that cannot necessarily be easily used for collation. For example, consider that order/{id} and order/{id:int} are semantically equivalent, but are different route templates. That's just the tip of the iceberg. Consider order/{id} vs order/{num}. As much as it would be nice to group by route template, there is no intelligible way to ensure route template equivalency.

Even if APIs were grouped purely by individual routes, what would you call the API? A route template alone does not convey that, but a controller can. As an example, you would expect an Order API to have more than one associate route template. How are order/{id} and order/{id}/item/list related? Are any route templates with the same prefix related? Maybe, but not necessarily. Hopefully that illustrates some of the complexities with trying to group by route template.

It should also be noted that the default controller name convention is to use the name of the type by dropping off the Controller suffix if it was specified. API Versioning adds a secondary convention which trims off any trailing numbers. This allows different controller types for the same service to exist in the same namespace, which is a restriction of the implementation details (namely .NET). For example, Values1Controller and Values2Controller. As you've specified them, your controller names will be FileV. Since the name is the same for both, they are grouped together. However, if you documented them with OpenAPI or needed the controller name in the route template, this would likely not produce the result you're hoping for. This can be remedied by removing the V, including the literal name in the route template, or using the [ControllerName("file")] attribute to explicitly specify the name.

You're, of course, free to use whatever versioning method you want. Using a your own custom route constraint is certainly fine, but API Versioning has a lot more moving parts that mere route parameter processing. Another possible approach is swapping out ApiVersioningOptions.ErrorResponses with your own IErrorResponseProvider implementation. You'd only be looking to replace or update 405 errors and can produce your own result or modify the base implementation with the desired Allow header.

@KalleOlaviNiemitalo
Copy link
Author

I already had a custom IErrorResponseProvider but the candidate action descriptors were not easily available from there.

@KalleOlaviNiemitalo
Copy link
Author

You write that it is expected that the Allow header collects HTTP methods from all action descriptors of the controller. However, when I use ASP.NET Web API without Microsoft.AspNet.WebApi.Versioning, the HTTP methods come only from actions whose route templates and constraints match the request URI. It is quite surprising that the versioning package changes this.

Does Microsoft.AspNetCore.Mvc.Versioning likewise construct the Allow header from all action descriptors of the controller? I did not test that, but from the source code, it seemed to only consider the matching candidates rather than all actions.

@commonsensesoftware
Copy link
Collaborator

It's more completed than that. The action descriptors can come from many controllers. It should be possible to bucketize the HTTP methods by API version. Currently, this only appears to be an issue if you interleave. Regardless, it should do the right thing.

The ASP.NET Core version no longer has this problem. 405 wasn't even originally supported in the early days. With the way that Endpoint Routing works, 405 occurs before API Versioning does it's filtering. In the next major iteration, you'll get the generic 405 instead of the API Versioning one because there won't be a chance for API Versioning to interject. This will be the same for 415.

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