add allow_head to add_get #1668
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1668 +/- ##
==========================================
- Coverage 96.77% 95.15% -1.63%
==========================================
Files 37 37
Lines 7360 7365 +5
Branches 1268 1269 +1
==========================================
- Hits 7123 7008 -115
- Misses 127 222 +95
- Partials 110 135 +25
Continue to review full report at Codecov.
|
aiohttp/web_urldispatcher.py
Outdated
| @@ -755,6 +756,9 @@ def routes(self): | |||
| def named_resources(self): | |||
| return MappingProxyType(self._named_resources) | |||
|
|
|||
| def set_defaults(self, *, allow_head): | |||
There was a problem hiding this comment.
I doubt if we need default for the flag.
It might lead to many confusing cases, e.g. if third party library has something like register_routes(router, prefix) method its behavior varies depending on outer flag.
There was a problem hiding this comment.
Humm, ok I'll remove it if you prefer. But the current setup is wrong according to the RFC and without the option to set the default you have to explicitly set allow_head=True on every route you add:
The server SHOULD send the same
header fields in response to a HEAD request as it would have sent if
the request had been a GET, except that the payload header fields
(Section 3.3) MAY be omitted.
For me, the simplest, clearest and most correct option would be to have allow_head=True by default.
There was a problem hiding this comment.
problem is, we currently do not control when handler writes body, so in most cases
developer needs to support head explicitly
There was a problem hiding this comment.
The developer can choose to support head explicitly but nothing breaks if they don't.
Allowing HEAD to endpoints which act like they got a GET request and still write body doesn't break anything.
Both django and flask allow users to still (by default) write body even for HEAD requests. I'm not saying they're always right, but their solution is more correct than aiohttp currently.
Here's an example:
app.py:
import asyncio
from aiohttp import web
async def handle(request):
print('preparing slow response...')
await asyncio.sleep(2, loop=request.app.loop)
return web.Response(body=b'this is the body\n')
app = web.Application()
app.router.add_get('/', handle, allow_head=True)
web.run_app(app, host='localhost', port=8000)shell:
~ ➤ curl -v http://localhost:8000
* Rebuilt URL to: http://localhost:8000/
* Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.47.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Length: 17
< Content-Type: application/octet-stream
< Date: Wed, 22 Feb 2017 10:08:03 GMT
< Server: Python/3.5 aiohttp/2.0.0a0
<
this is the body
* Connection #0 to host localhost left intact
~ ➤ curl -I http://localhost:8000
HTTP/1.1 200 OK
Content-Length: 17
Content-Type: application/octet-stream
Date: Wed, 22 Feb 2017 10:08:10 GMT
Server: Python/3.5 aiohttp/2.0.0a0
~ ➤
Nothing went wrong here.
The app could choose to notice the HEAD request and not spend a lot of time/resources writing body, but I don't see any problem if those not concerned by this (fairly niche) optimisation don't bother with it.
In summary: your current approach explicitly contravenes the HTTP RFC. The issue you're worried about is an optimisation which will apply in a very small percentage of requests and where app authors already have all the tools they need to achieve the optimization.
There was a problem hiding this comment.
Ok. Let's remove url dispatch part and change default allow_head to True
There was a problem hiding this comment.
Great. I've updated the PR.
|
|
|
very good |
What do these changes do?
Add the
allow_headkeyword argument foradd_getto encourage aiohttp server applications to correctly implement http and allowHEADrequests to endpoints generally used forGETrequests.Also adds
app.router.set_defaults(allow_head=True)to switch behaviour of all endpoints for the app.Related issue number
#1618
Checklist
CONTRIBUTORS.txtCHANGES.rst#issue_numberformat at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.