Skip to content
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

allow HEAD requests to default routes #1618

Closed
samuelcolvin opened this issue Feb 10, 2017 · 32 comments
Closed

allow HEAD requests to default routes #1618

samuelcolvin opened this issue Feb 10, 2017 · 32 comments
Labels
Milestone

Comments

@samuelcolvin
Copy link
Member

samuelcolvin commented Feb 10, 2017

Just checked and both flask and django by default allow HEAD requests (but not POST, PUT etc.) to the default routed endpoints.

The closest equivalent in aiohttp is add_get but understandably that doesn't allow HEAD requests and returns 405.

aiohttp should add a method to add routes which accept the "noop" or safe http methods eg. GET, HEAD and OPTIONS.

To be consistent with other servers and act in the spirit of http, this method should be the default. AFAIK returning 405 to HEAD requests to GET endpoints is wrong.

I'm trying to think about the best name for such a method which is both clear and not verbose, it's not obvious. Wikipedia calls these "safe methods" but add_safe could be a confusing; making people think aiohttp is giving some kind of protection which it isn't.

Perhaps the simplest options would be just add.

So code would be

app.router.add('/', handler)
app.router.add_post('/post', post_handler)
app.router.add_put('/put', put_handler)

What exactly it should do with OPTIONS requests is debatable. The fact it should gracefully accept HEAD requests is a no-brainer for me.

@samuelcolvin
Copy link
Member Author

bump, any news one this? I think it's important and requires a fix.

@fafhrd91
Copy link
Member

i am not entirely sure what you are proposing?
Default methods or way how to add default methods?

@samuelcolvin
Copy link
Member Author

I'm proposing a new method add which allows HEAD requests as well as GET requests.

Then change in documentation to encourage people to use that method instead of add_get with a comment saying "If you really want views which do not allow HEAD requests (which is contrary to the spirit of http) you can use the add_get method."

@samuelcolvin
Copy link
Member Author

Perhaps easiest if I do a pull request then you can comment on that.

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

Shouldn't allow GET also allow HEAD?

@samuelcolvin
Copy link
Member Author

yes it should, but I do understand why it's confusing if add_get() allowed HEAD so we need another method but add_get_or_head() would be annoying so I think just add() would work.

I'm easy, could just modify add_get().

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 17, 2017

i think i'm missing something. you can do add_route('HEAD', ...)

@samuelcolvin
Copy link
Member Author

app.router.add_get('/', index, name='index')
app.router.add_route('HEAD', '/', index)

Is a pretty ugly preferred way of adding a standard endpoint.

@fafhrd91
Copy link
Member

we can do something like router.add_get(...., enable_head=True)
and add add_head() which can override default head
how that sound?

@samuelcolvin
Copy link
Member Author

samuelcolvin commented Feb 17, 2017

makes sense, for me enable_head should be True by default, but I understand this is a significant change.

Perhaps have enable_head=False by default but give a warning if enable_head is not supplied saying "default behaviour will change in future to enabled_head=True"?

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

Sounds good for me, just only may be allow_head=True would be a bit more correct.

@fafhrd91
Copy link
Member

False is good default. i fill allow_head implies something else, i prefer enable_head.
we can also add setting for dispatcher which can define default behavior of add_get()

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

It follows the same naming policy as HTTP has: you Allow methods via same header, not enable them. Same story for CORS. If with this option False we throw HTTP 405 on HEAD request then it's quite logical name.

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 17, 2017

but this would look strange:

app.router.add_get(..., allow_head=False)
app.router.add_head(...)

@fafhrd91
Copy link
Member

maybe enable_default_head?

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

The enable_default_head a bit more specific. But this looks strange in any way since you first disallow/disable HEAD and then you explicitly add it support. Why? (rhetorical question)

What if instead of:

app.router.add_get(..., allow_head=False)
app.router.add_head(...)

Have:

app.router.add_get(..., head_handler=custom_handler)

A bit out of rules, but since GET and HEAD are heavy bounded, could this be legal?

@fafhrd91
Copy link
Member

i like head_handler!

@samuelcolvin ?

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

Brrr...ignore my last post.

Case 1: we're fine to handle HEAD requests with GET handler.

app.router.add_get(..., allow_head=True)  # default since some time

Case 2: we need to disallow HEAD requests for GET endpoints

app.router.add_get(..., allow_head=False)  # what we have now

Case 3: we want to have custom HEAD handler at the same endpoint

app.router.add_get(...)
app.router.add_head(...)

E.g. you don't need to explicitly disable default HEAD handler or care about if you define own a bit later. add_head just simply overrides what we have by default.

API remains consistent and clear without mandatory flags or additional handlers.

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

Ah, one more:

Case 4: I don't know what I want to have

app.router.add_get(..., allow_head=False)
app.router.add_head(...)

(:

Yes, you, probably, can write something like that, but why? (not a rhetorical question)

@fafhrd91
Copy link
Member

but i like head_handler,
signature could be like this def add_get(head_handler=default_handler, *args, **kwargs):
this would cover all cases

@samuelcolvin
Copy link
Member Author

I don't understand what head_handler is. Is it a custom handler for process head requests?

That sounds wrong, HEAD requests should be processed by the same handler as get so the response code and headers are the same as GET.

@kxepal
Copy link
Member

kxepal commented Feb 17, 2017

@samuelcolvin
Btw, that's a good point. Different handler may produce different response which will violate HEAD semantic. So it seems it just about one flag?

@fafhrd91
Copy link
Member

ok, it seems it is about flag only.

@fafhrd91
Copy link
Member

then allow_head seems reasonable.

@samuelcolvin
Copy link
Member Author

I think just allow_head is fine.

Perhaps default_allow_head as a kwarg to Application so one can change the default behaviour?

@fafhrd91
Copy link
Member

technically default_allow_head belongs to UrlDispatcher

@samuelcolvin
Copy link
Member Author

True, but it's very rare to initialise UrlDispatcher directly, either an Application kwarg or a method:

app.router.set_default(allow_head=True)
app.router.add_get('/', index)  # this will allow head requests

I can imagine there might be other default behaviour to configure on router in the future.

@fafhrd91
Copy link
Member

@asvetlov do you have ideas about default_allow_head or router.set_default(allow_head=True)

@fafhrd91
Copy link
Member

@samuelcolvin would you like to work on this feature?

@samuelcolvin
Copy link
Member Author

yes will do.

@fafhrd91
Copy link
Member

cool, thanks.

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants