Aiohttp: Double slash in path is meaningful (shouldn't be)

[mickeyl]

I have a server listening for requests at /infastructure/events. For some reason, it doesn't catch requests sent to infrastructure//events. As far as I know this should work – at least all other web frameworks I tried seem to handle this case.

mickey@helios ~ $ http POST localhost:8091/infrastructure/events foo=bar
HTTP/1.1 200 OK
CONNECTION: keep-alive
CONTENT-LENGTH: 27
CONTENT-TYPE: application/json
DATE: Sun, 10 May 2015 11:31:04 GMT
SERVER: Python/3.4 aiohttp/0.15.3

{
    "data": null,
    "result": true
}

mickey@helios ~ $ http POST localhost:8091/infrastructure//events foo=bar
HTTP/1.1 404 Not Found
CONNECTION: keep-alive
CONTENT-LENGTH: 14
CONTENT-TYPE: text/plain; charset=utf-8
DATE: Sun, 10 May 2015 11:31:16 GMT
SERVER: Python/3.4 aiohttp/0.15.3

404: Not Found

What's your take on it?

Thanks.

[asvetlov]

Normalization procedure from RFC 3986 doesn't allow to remove double-slashes in path, so http://example.com/path/to and http://example.com/path//to are different paths.
Why do we need join double slashes?

BTW, it can be done in custom middleware if needed.

[GMLudo]

@asvetlov maybe via a regex in the route ?

[asvetlov]

No. I think catching NotFound error, collapsing slashes and redirect is better.
The same technique should be used for adding trailing slash.

Maybe we need include the middleware in aiohttp batteries, though.

[GMLudo]

Ok, I see.
BTW, I already had issues with Flask because it generates automagically a 301 redirect for all endpoints with a slash at the end.
You're right, an optional middleware seems to be the right decision.

[kxepal]

infrastructure//events is not the same as infrastructure/events since // should be read as "path segment with zero length name". For most applications that makes no sense, however, you may find it quite often for "collection/element"` REST-like designs (because someone forgot to assert name against empty string).

+1 for optional middleware.

[asvetlov]

Is there a volunteer for making a PR?
I can kindly review it :)

[asvetlov]

Working on the issue at https://github.com/KeepSafe/aiohttp/tree/normalize_path_middleware

[argaen]

I'm gonna take ownership of that one and try to implement again the normalize_path_middleware :).

[asvetlov]

It requires web.Request.clone() first I think.
But if you want to cope with it -- you are welcome!

[argaen]

I'm a bit out of context now and don't see why yet. If I need it I'll go with it :)

[argaen]

Ahhhh just arrived to the case haha. Anyway, I was going with the trailing slash first and this is a basic test I've come with for the general use case:

    @asyncio.coroutine
    def test_add_trailing_when_necessary(self, create_app_and_client):
        app, client = yield from create_app_and_client()
        app.middlewares.append(middlewares.normalize_path_middleware)
        app.router.add_route('GET', '/resource1', lambda x: web.Response(text="OK"))
        app.router.add_route('GET', '/resource2/', lambda x: web.Response(text="OK"))

        resp = yield from client.get('/resource1')
        assert resp.status == 200

        resp = yield from client.get('/resource1/')
        assert resp.status == 404

        resp = yield from client.get('/resource2')
        assert resp.status == 200

        resp = yield from client.get('/resource2/')
        assert resp.status == 200

Can we agree this is the behavior we want? :)

[asvetlov]

normalize_path_middleware should accept parameters, perhaps we will have different normalization options in future.
But you approach for adding slash only if resource is already registered with trailing slash looks very reasonable.

[argaen]

I can see there is a defined copy method with NotImplementedError. Guess that's supposed to be the clone or was it intended for other purposes?

[asvetlov]

No, not implemented copy means that request is a dict-like object but without dict.copy() implementation.
For parameter changing let's use .clone() -- the method has a different signature than .copy()

[fafhrd91]

normalize_path_middleware middleware has been added

[nicktimko]

If anyone comes across this issue now or in the future:

• normalize_path_middleware docs
• tl;dr usage

from aiohttp import web

myapp = web.Application(middlewares=[
    normalize_path_middleware(append_slash=True, merge_slashes=True),
])

[lock[bot]]

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].