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

Added first version of normalize_path_middleware #1118

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions aiohttp/middlewares.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import asyncio

# from aiohttp.web_exceptions import HTTPMovedPermanently, HTTPNotFound
from aiohttp.web_urldispatcher import SystemRoute


@asyncio.coroutine
def normalize_path_middleware(app, handler):
"""
Middleware that normalizes the path of a request. By normalizing it means:

- Add a trailing slash to the path.
- Double slashes are replaced by one.
"""

@asyncio.coroutine
def middleware(request):

router = request.app.router
if not isinstance(request.match_info.route, SystemRoute):
resp = yield from handler(request)

elif not request.path.endswith('/'):
request = request.clone(path=request.path + '/')
match_info = yield from router.resolve(request)
resp = yield from match_info.handler(request)

else:
resp = yield from handler(request)

return resp

return middleware
29 changes: 28 additions & 1 deletion aiohttp/web_reqrep.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from . import hdrs
from .helpers import reify, sentinel
from .protocol import Response as ResponseImpl
from .protocol import HttpVersion10, HttpVersion11
from .protocol import HttpVersion10, HttpVersion11, RawRequestMessage
from .streams import EOF_MARKER

__all__ = (
Expand Down Expand Up @@ -381,6 +381,33 @@ def post(self):
self._post = MultiDictProxy(out)
return self._post

def clone(self, *, method=None, path=None, headers=None, raw_headers=None):
"""
Creates and returns a new instance of Request object. If no parameters
are given, an exact copy is returned. If a parameter is not passed, it
will reuse the one from the current request object.

:param method: str http method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use autodoc markup

:param path: str url path to use
:param headers: CIMultidictProxy or compatible containing the headers.
:param raw_headers: tuple of two element tuples containing the headers
as bytearrays.
"""

message = RawRequestMessage(
method or self.method, path or self.path, self.version,
headers or self.headers, raw_headers or self.raw_headers,
self.keep_alive, None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oohh. Our very low level API is really bad.
I believe the compression param for this case is always None but why we need it here at all?
Nevermind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same... Same with the headers and raw_headers. Why do we need to send both replicated? It would be cleaner to just send the headers and do the conversion (if ever needed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. headers is CIMultiDict instance with str keys and values.
But raw_headers is a list or (header, value) pairs where both are unconverted bytes.
Not all possible header and value can be converted using UTF8 codec. Some old clients may send a data using custom encoding and the purpose of raw_headers is giving a way to handle this data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh I see... Still feels bit redundant


return Request(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request is a dict-like object and other middlewares can modify it
so all data from original one must be moved to its clone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep .copy() as is but create .clone() instead.
The methods have different semantics.

self._app,
message,
self._payload,
self._transport,
self._reader,
self._writer,
secure_proxy_ssl_header=self._secure_proxy_ssl_header)

def copy(self):
raise NotImplementedError

Expand Down
4 changes: 4 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
[pep8]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this option? 79 lines is default value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can have configurations at user home level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Ok, make sense.

max-line-length=80
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


[easy_install]
zip_ok = false

[flake8]
ignore = N801,N802,N803,E226
max-line-length=80
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

79


[tool:pytest]
timeout = 5
Expand Down
27 changes: 27 additions & 0 deletions tests/test_middlewares.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import asyncio

from aiohttp import middlewares, web


class TestNormalizePathMiddleware:

@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