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

Already on GitHub? Sign in to your account

HTTP 405 response must include an Allow header #902

Open
thijsvandien opened this Issue Sep 21, 2013 · 6 comments

Comments

Projects
None yet
2 participants

When an HTTPError(405) is raised, either manually or automatically because of not implementing a handler, Tornado does not set the officially required Allow header. Worse, if I set it myself, it is erased by RequestHandler.send_error. Bluntly said, it means that Tornado does not follow HTTP standards.

When addressing this, please consider adding a possibility to override the list of supported methods, rather than looking only at the present handlers; I built my own mechanism to limit their usage at certain URLs:

class BaseHandler(RequestHandler):
    def initialize(self, *args, **kwargs):
        self.allowed_methods = kwargs.get('allowed_methods', type(self).SUPPORTED_METHODS)

    def prepare(self):
        if self.request.method not in self.allowed_methods: raise HTTPError(405)
Owner

bdarnell commented Sep 21, 2013

Is this just spec lawyering or is there client software that does something useful with the Allow header?

Returning an Allow header based on SUPPORTED_METHODS will not generally be useful since the standard methods are always in the list whether they're implemented or not (although you could customize them in your own handlers instead of adding a separate allowed_methods kwarg).

You can set the Allow header in the error response if you do it from write_error() (instead of before the error is raised).

Thank you for your rapid response. Your hint at write_error will certainly be of help.

I do find Allow useful in my APIs, though I think there's nothing wrong with a bit of "spec lawyering." HTTP is well-defined for a reason, and I wouldn't like to have to fill the gaps myself. It's not to criticize Tornado; I just think following the spec is important for its continued success.

How to determine what methods are considered supported is a different matter. If there is such attribute in every RequestHandler, perhaps it wouldn't be unreasonable to expect a user to set it appropriately. It seems strange to me, that currently most classes "lie" about their abilities. Maybe I have a bad understanding of what SUPPORTED_METHODS is really supposed to mean.

A straight-forward solution would be the intersection of SUPPORTED_METHODS and the implemented methods. That makes the assumption, however, that none of those methods will ever throw an HTTPError(405). There may be cases where a static attribute won't suffice. I wanted to use a RequestHandler multiple times, but only parts of it. There's other ways of doing that (now using a class decorator to set SUPPORTED_METHODS). But there may even be cases where it can only be determined at runtime (session, time, etc.).

One solution I might propose is to replace SUPPORTED_METHODS by an instance method get_supported_methods which by default returns a list of implemented methods, but may be overridden to return any subset thereof at any time.

Owner

bdarnell commented Sep 22, 2013

SUPPORTED_METHODS is a whitelist to ensure that we don't call (via getattr()) a method that wasn't expecting to be called this way (e.g. PREPARE / HTTP/1.1). Handlers that support non-standard methods can set SUPPORTED_METHODS = RequestHandler.SUPPORTED_METHODS + ['WHATEVER']. It wasn't designed to be enumerated to discover available methods, although I'd be reluctant to introduce a separate enumerable interface.

Even though this is in the spec (and narrowly escaped removal in httpbis), I'm not convinced it's a sensible requirement. Why is 405 unique among 4xx errors (I think) in that it MUST point the client toward a set of related requests that might work? If a framework like Tornado cannot easily satisfy the Allow header requirement, surely it's better to return 405 anyway than to fall back to a less-specific error code like 400.

Sorry for coming back to this late. I can see your objections to changing the implementation. However, I do think it's a subject for documentation. It would be good to have clearly written what Tornado does and what it doesn't, especially with respect to formal specifications. E.g. ETags come automatically and an Allow header is to be added manually.

Owner

bdarnell commented Dec 3, 2013

Yeah, I think we need a guide like that (especially when it comes to caching - getting the Vary header right can be tricky and must be left to the application).

@bdarnell bdarnell added the web label Jul 16, 2014

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