From c103f5da3cec03e8b94abad7ba536d567bfb230a Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 4 Jan 2016 17:48:08 +0200 Subject: [PATCH 01/34] Introduce Resource concept --- aiohttp/web_urldispatcher.py | 153 ++++++++++++++++++++--------------- 1 file changed, 90 insertions(+), 63 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 3bfcaaac50d..46a12fa4f35 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -22,7 +22,8 @@ __all__ = ('UrlDispatcher', 'UrlMappingMatchInfo', - 'Route', 'PlainRoute', 'DynamicRoute', 'StaticRoute', 'View') + 'Resource', 'PlainResource', 'DynamicResource', + 'Route', 'StaticRoute', 'View') PY_35 = sys.version_info >= (3, 5) @@ -53,26 +54,9 @@ def _defaultExpectHandler(request): request.transport.write(b"HTTP/1.1 100 Continue\r\n\r\n") -class Route(metaclass=abc.ABCMeta): - - def __init__(self, method, handler, name, *, expect_handler=None): - if expect_handler is None: - expect_handler = _defaultExpectHandler - assert asyncio.iscoroutinefunction(expect_handler), \ - 'Coroutine is expected, got {!r}'.format(expect_handler) - - self._method = method - self._handler = handler +class Resource(metaclass=abc.ABCMeta): + def __init__(self, *, name=None): self._name = name - self._expect_handler = expect_handler - - @property - def method(self): - return self._method - - @property - def handler(self): - return self._handler @property def name(self): @@ -87,10 +71,6 @@ def match(self, path): def url(self, **kwargs): """Construct url for route with additional params.""" - @asyncio.coroutine - def handle_expect_header(self, request): - return (yield from self._expect_handler(request)) - @staticmethod def _append_query(url, query): if query is not None: @@ -99,10 +79,10 @@ def _append_query(url, query): return url -class PlainRoute(Route): +class PlainResource(Resource): - def __init__(self, method, handler, name, path, *, expect_handler=None): - super().__init__(method, handler, name, expect_handler=expect_handler) + def __init__(self, path, *, name=None): + super().__init__(name=name) self._path = path def match(self, path): @@ -117,16 +97,14 @@ def url(self, *, query=None): def __repr__(self): name = "'" + self.name + "' " if self.name is not None else "" - return " {handler!r}".format( - name=name, method=self.method, path=self._path, - handler=self.handler) + return " {handler!r}" - .format(name=name, method=self.method, - formatter=self._formatter, handler=self.handler)) + return (" {handler!r}".format( + method=self.method, resource=self._resource, + handler=self.handler) + + @property + def method(self): + return self._method + + @property + def handler(self): + return self._handler + + @property + def name(self): + return self._resource.name + + @property + def resource(self): + return self._resource + + def match(self, path): + """Return dict with info for given path or + None if route cannot process path.""" + return self._resource.match(path) + + def url(self, **kwargs): + """Construct url for route with additional params.""" + return self._resource.url(**kwargs) + + @asyncio.coroutine + def handle_expect_header(self, request): + return (yield from self._expect_handler(request)) class StaticRoute(Route): @@ -179,7 +204,7 @@ def url(self, *, filename, query=None): while filename.startswith('/'): filename = filename[1:] url = self._prefix + filename - return self._append_query(url, query) + return Resource._append_query(url, query) def _sendfile_cb(self, fut, out_fd, in_fd, offset, count, loop, registered): @@ -497,31 +522,13 @@ def register_route(self, route): self._routes[name] = route self._urls.append(route) - def add_route(self, method, path, handler, - *, name=None, expect_handler=None): - + def add_resource(self, path, *, name=None): if not path.startswith('/'): raise ValueError("path should be started with /") - - assert callable(handler), handler - if asyncio.iscoroutinefunction(handler): - pass - elif inspect.isgeneratorfunction(handler): - pass - elif isinstance(handler, type) and issubclass(handler, AbstractView): - pass - else: - handler = asyncio.coroutine(handler) - - method = upstr(method) - if method not in self.METHODS: - raise ValueError("{} is not allowed HTTP method".format(method)) - if not ('{' in path or '}' in path or self.ROUTE_RE.search(path)): - route = PlainRoute( - method, handler, name, path, expect_handler=expect_handler) - self.register_route(route) - return route + resource = PlainResource(path, name=name) + # self.register_route(route) + return resource pattern = '' formatter = '' @@ -549,9 +556,29 @@ def add_route(self, method, path, handler, except re.error as exc: raise ValueError( "Bad pattern '{}': {}".format(pattern, exc)) from None - route = DynamicRoute( - method, handler, name, compiled, - formatter, expect_handler=expect_handler) + resource = DynamicResource(compiled, formatter, name=name) + # self.register_route(route) + return resource + + def add_route(self, method, path, handler, + *, name=None, expect_handler=None): + + assert callable(handler), handler + if asyncio.iscoroutinefunction(handler): + pass + elif inspect.isgeneratorfunction(handler): + pass + elif isinstance(handler, type) and issubclass(handler, AbstractView): + pass + else: + handler = asyncio.coroutine(handler) + + method = upstr(method) + if method not in self.METHODS: + raise ValueError("{} is not allowed HTTP method".format(method)) + + resource = self.add_resource(path, name=name) + route = Route(method, handler, resource, expect_handler=expect_handler) self.register_route(route) return route From 1931a9b045c43e6428af857a0ea4b1cc179313ab Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 4 Jan 2016 20:11:44 +0200 Subject: [PATCH 02/34] Work on impl --- NEW_ROUTER.txt | 69 ++++++++++++++++++++++++++++++++++++ aiohttp/web_urldispatcher.py | 60 +++++++++++++++++++------------ 2 files changed, 106 insertions(+), 23 deletions(-) create mode 100644 NEW_ROUTER.txt diff --git a/NEW_ROUTER.txt b/NEW_ROUTER.txt new file mode 100644 index 00000000000..c5d95c99c70 --- /dev/null +++ b/NEW_ROUTER.txt @@ -0,0 +1,69 @@ +The router should be 99% compatible with current implementation. + +99% means all example and the most of tests should work without +modifications but subtle API backward implatible changes are allowed. + +First generation (v1) router maps (method, path) pair to web-handler. +Mapping is named *route*. Routes may have unique names. + +The main mistake with the design is coupling route to (method, path) +pair while really URL construction operates with *resources* +(*location* is a synonym). HTTP method is not part of URI but applyed +on sending HTTP request. + +Having different route names for the same path is confusing. Moreover +named routes constructed for the same path should have unique +nonoverlapping names which is cumbersome is certain situations. + +From other side sometimes it's desirable to bind several HTTP methods +to the same web handler. In v1 router it can be solved by passing '*' +as HTTP method. Class based views require '*' method also usually. + +The proposal introduces *Resource* class (with PlainResource and +DynamicResource descendants). Resource has the following attributes: +name, match() and url(). + + +router.add_route('get', '/path/index', handler0) +router.add_route('get', '/path/{to}', handler1, name='a') +router.add_route('post', '/path/{to}', handler2, name='a') + + +resource1 = router.add_resource('/path/{to}', name='a') +resource2 = router.add_or_find_resource('/path/{to}', name='a') +resource1 is resource2 + + +resource.add_handler('get', handler1) +resource.add_handler('post', handler2) +resource.add_view(MyCBView) +resource.add_handler('*', sink) +resource.add_handler('post', handler3) # raise + +resource['post'] = handler2 + +resource.add_app(sub) + + +resource.url(['a', 'b']) + +router['a'].url(parts={'a': 'c', 'b': 'd'}) + +router['a'].url(a='c', b='d').query(md, z=1) + +router['a'].url(filename='f.txt').query(md, z=1) + +router['a'].url().query(md, z=1) + + + + +dt = Application() +dt.router.add_route() +dt.middlewares.append() +dt['dfdsf'] = 23423 + + +def setup(app): + res = app.route.add_resource('/debugtoolbar') + res.add_app(dt) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 46a12fa4f35..ad0f1b8ee11 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -23,7 +23,7 @@ __all__ = ('UrlDispatcher', 'UrlMappingMatchInfo', 'Resource', 'PlainResource', 'DynamicResource', - 'Route', 'StaticRoute', 'View') + 'Route', 'PlainRoute', 'DynamicRoute', 'StaticRoute', 'View') PY_35 = sys.version_info >= (3, 5) @@ -113,7 +113,8 @@ def match(self, path): if match is None: return None else: - return match.groupdict() + return {key: unquote(value) for key, value in + match.groupdict().items()} def url(self, *, parts, query=None): url = self._formatter.format_map(parts) @@ -173,6 +174,9 @@ def handle_expect_header(self, request): return (yield from self._expect_handler(request)) +PlainRoute = DynamicRoute = Route + + class StaticRoute(Route): def __init__(self, name, prefix, directory, *, @@ -456,8 +460,8 @@ class UrlDispatcher(AbstractRouter, collections.abc.Mapping): def __init__(self): super().__init__() - self._urls = [] - self._routes = {} + self._resources = [] + self._named_resources = {} @asyncio.coroutine def resolve(self, request): @@ -465,16 +469,13 @@ def resolve(self, request): method = request.method allowed_methods = set() - for route in self._urls: - match_dict = route.match(path) + for resource in self._resources: + match_dict = resource.match(path) if match_dict is None: continue route_method = route.method if route_method == method or route_method == hdrs.METH_ANY: - # Unquote separate matching parts - match_dict = {key: unquote(value) for key, value in - match_dict.items()} return UrlMappingMatchInfo(match_dict, route) allowed_methods.add(route_method) @@ -485,27 +486,41 @@ def resolve(self, request): return _NotFoundMatchInfo() def __iter__(self): - return iter(self._routes) + return iter(self._named_resources) def __len__(self): - return len(self._routes) + return len(self._named_resources) def __contains__(self, name): - return name in self._routes + return name in self._named_resources def __getitem__(self, name): - return self._routes[name] + return self._named_resources[name] + + def resources(self): + # TODO: should distinguish between iterating over resources and views + return RoutesView(self._resources) def routes(self): - return RoutesView(self._urls) + return RoutesView(self._resources) + + def named_resources(self): + return MappingProxyType(self._named_resources) def named_routes(self): - return MappingProxyType(self._routes) + # TODO: it's ambiguous but it really resources. + # DEPRECATE! + return MappingProxyType(self._named_resources) def register_route(self, route): assert isinstance(route, Route), 'Instance of Route class is required.' + self._reg_resource(route.resource) + + def _reg_resource(self, resource): + assert isinstance(resource, Resource), \ + 'Instance of Resource class is required.' - name = route.name + name = resource.name if name is not None: parts = self.NAME_SPLIT_RE.split(name) @@ -515,19 +530,19 @@ def register_route(self, route): 'the name should be a sequence of ' 'python identifiers separated ' 'by dash, dot or column'.format(name)) - if name in self._routes: + if name in self._named_resources: raise ValueError('Duplicate {!r}, ' 'already handled by {!r}' - .format(name, self._routes[name])) - self._routes[name] = route - self._urls.append(route) + .format(name, self._named_resources[name])) + self._named_resources[name] = resource + self._resources.append(resource) def add_resource(self, path, *, name=None): if not path.startswith('/'): raise ValueError("path should be started with /") if not ('{' in path or '}' in path or self.ROUTE_RE.search(path)): resource = PlainResource(path, name=name) - # self.register_route(route) + self._reg_resource(resource) return resource pattern = '' @@ -557,7 +572,7 @@ def add_resource(self, path, *, name=None): raise ValueError( "Bad pattern '{}': {}".format(pattern, exc)) from None resource = DynamicResource(compiled, formatter, name=name) - # self.register_route(route) + self.register_resource(resource) return resource def add_route(self, method, path, handler, @@ -579,7 +594,6 @@ def add_route(self, method, path, handler, resource = self.add_resource(path, name=name) route = Route(method, handler, resource, expect_handler=expect_handler) - self.register_route(route) return route def add_static(self, prefix, path, *, name=None, expect_handler=None, From db48cec1125d7cb2eb1632401475dc5fdf1af54d Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sun, 10 Jan 2016 03:01:45 +0200 Subject: [PATCH 03/34] Make a progress on implementing new route system --- aiohttp/web_urldispatcher.py | 187 ++++++++++++++++++++++++----------- 1 file changed, 130 insertions(+), 57 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index ad0f1b8ee11..5bb7ccc3103 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -55,8 +55,46 @@ def _defaultExpectHandler(request): class Resource(metaclass=abc.ABCMeta): + METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} + def __init__(self, *, name=None): self._name = name + self._routes = [] + + def add_route(self, method, handler, *, + expect_handler=None): + + if expect_handler is None: + expect_handler = _defaultExpectHandler + + assert asyncio.iscoroutinefunction(expect_handler), \ + 'Coroutine is expected, got {!r}'.format(expect_handler) + + assert callable(handler), handler + if asyncio.iscoroutinefunction(handler): + pass + elif inspect.isgeneratorfunction(handler): + pass + elif isinstance(handler, type) and issubclass(handler, AbstractView): + pass + else: + handler = asyncio.coroutine(handler) + + method = upstr(method) + if method not in self.METHODS: + raise ValueError("{} is not allowed HTTP method".format(method)) + + # TODO: add check for duplicated methods + + route = Route(method, handler, + expect_handler=expect_handler, + resource=self) + self.register_route(route) + return route + + def register_route(self, route): + assert isinstance(route, Route), 'Instance of Route class is required.' + self._routes.append(route) @property def name(self): @@ -71,6 +109,23 @@ def match(self, path): def url(self, **kwargs): """Construct url for route with additional params.""" + def resolve(self, method, path): + allowed_methods = set() + + match_dict = self.match(path) + if match_dict is None: + return None, allowed_methods + + for route in self._routes: + route_method = route.method + + if route_method == method or route_method == hdrs.METH_ANY: + return UrlMappingMatchInfo(match_dict, route), allowed_methods + + allowed_methods.add(route_method) + else: + return None, allowed_methods + @staticmethod def _append_query(url, query): if query is not None: @@ -126,13 +181,38 @@ def __repr__(self): .format(name=name, formatter=self._formatter)) +class PrefixResource(Resource): + + def __init__(self, prefix, *, name=None): + if not prefix.endswith('/'): + prefix += '/' + assert prefix.endswith('/'), prefix + super().__init__(name=name) + self._prefix = prefix + self._prefix_len = len(self._prefix) + + def match(self, path): + if not path.startswith(self._prefix): + return None + return {'filename': path[self._prefix_len:]} + + def url(self, *, filename, query=None): + while filename.startswith('/'): + filename = filename[1:] + url = self._prefix + filename + return Resource._append_query(url, query) + + def __repr__(self): + name = "'" + self.name + "' " if self.name is not None else "" + return " Date: Sun, 10 Jan 2016 03:16:58 +0200 Subject: [PATCH 04/34] Introduce ResourceRoute --- aiohttp/web_urldispatcher.py | 150 ++++++++++++++++++++++++----------- 1 file changed, 102 insertions(+), 48 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 5bb7ccc3103..cd60f38308f 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -86,14 +86,14 @@ def add_route(self, method, handler, *, # TODO: add check for duplicated methods - route = Route(method, handler, - expect_handler=expect_handler, - resource=self) + route = ResourceRoute(method, handler, self, + expect_handler=expect_handler) self.register_route(route) return route def register_route(self, route): - assert isinstance(route, Route), 'Instance of Route class is required.' + assert isinstance(route, ResourceRoute), \ + 'Instance of Route class is required, got {!r}'.format(route) self._routes.append(route) @property @@ -208,22 +208,13 @@ def __repr__(self): prefix=self._prefix) -class Route(metaclass=abc.ABCMeta): - +class BaseRoute(metaclass=abc.ABCMeta): def __init__(self, method, handler, *, - expect_handler=None, - resource=None): - + expect_handler=None): self._method = method self._handler = handler - self._resource = resource self._expect_handler = expect_handler - def __repr__(self): - return " {handler!r}".format( - method=self.method, resource=self._resource, - handler=self.handler) - @property def method(self): return self._method @@ -232,6 +223,35 @@ def method(self): def handler(self): return self._handler + @abc.abstractmethod # pragma: no branch + def match(self, path): + """Return dict with info for given path or + None if route cannot process path.""" + + @abc.abstractmethod # pragma: no branch + def url(self, **kwargs): + """Construct url for route with additional params.""" + + @asyncio.coroutine + def handle_expect_header(self, request): + return (yield from self._expect_handler(request)) + + _append_query = Resource._append_query + + +class ResourceRoute(BaseRoute): + """A route with resource""" + + def __init__(self, method, handler, resource, *, + expect_handler=None): + super().__init__(method, handler, expect_handler=expect_handler) + self._resource = resource + + def __repr__(self): + return " {handler!r}".format( + method=self.method, resource=self._resource, + handler=self.handler) + @property def name(self): return self._resource.name @@ -249,47 +269,81 @@ def url(self, **kwargs): """Construct url for route with additional params.""" return self._resource.url(**kwargs) - @asyncio.coroutine - def handle_expect_header(self, request): - return (yield from self._expect_handler(request)) +class Route(BaseRoute): + """Old fashion route""" -class PlainRoute(Route): - # for backward compatibility only, not required for new API - - def __init__(self, method, handler, name, path, *, - expect_handler=None, - resource=None): - super().__init__(method, handler, - expect_handler=expect_handler, - resource=resource) + def __init__(self, method, handler, name, *, expect_handler=None): + if expect_handler is None: + expect_handler = _defaultExpectHandler + assert asyncio.iscoroutinefunction(expect_handler), \ + 'Coroutine is expected, got {!r}'.format(expect_handler) + + super().__init__(method, handler, expect_handler=expect_handler) self._name = name + + @property + def name(self): + return self._name + + +class PlainRoute(Route): + + def __init__(self, method, handler, name, path, *, expect_handler=None): + super().__init__(method, handler, name, expect_handler=expect_handler) self._path = path + def match(self, path): + # string comparison is about 10 times faster than regexp matching + if self._path == path: + return {} + else: + return None + + def url(self, *, query=None): + return self._append_query(self._path, query) + + def __repr__(self): + name = "'" + self.name + "' " if self.name is not None else "" + return " {handler!r}".format( + name=name, method=self.method, path=self._path, + handler=self.handler) + class DynamicRoute(Route): - # for backward compatibility only, not required for new API - - def __init__(self, method, handler, name, path, *, - expect_handler=None, - resource=None): - super().__init__(method, handler, - expect_handler=expect_handler, - resource=resource) - self._name = name - self._path = path + def __init__(self, method, handler, name, pattern, formatter, *, + expect_handler=None): + super().__init__(method, handler, name, expect_handler=expect_handler) + self._pattern = pattern + self._formatter = formatter -class StaticRoute(Route): + def match(self, path): + match = self._pattern.match(path) + if match is None: + return None + else: + return match.groupdict() - def __init__(self, name, prefix, directory, *, + def url(self, *, parts, query=None): + url = self._formatter.format_map(parts) + return self._append_query(url, query) + + def __repr__(self): + name = "'" + self.name + "' " if self.name is not None else "" + return (" {handler!r}" + .format(name=name, method=self.method, + formatter=self._formatter, handler=self.handler)) + + +class StaticRoute(ResourceRoute): + + def __init__(self, name, prefix, directory, resource, *, expect_handler=None, chunk_size=256*1024, - response_factory=StreamResponse, - resource=None): + response_factory=StreamResponse): super().__init__( - 'GET', self.handle, - expect_handler=expect_handler, - resource=resource) + 'GET', self.handle, resource, + expect_handler=expect_handler) self._directory = os.path.abspath(directory) + os.sep self._chunk_size = chunk_size self._response_factory = response_factory @@ -433,7 +487,7 @@ def __repr__(self): class SystemRoute(Route): def __init__(self, status, reason): - super().__init__(hdrs.METH_ANY, None) + super().__init__(hdrs.METH_ANY, None, None) self._status = status self._reason = reason @@ -677,11 +731,11 @@ def add_static(self, prefix, path, *, name=None, expect_handler=None, :param prefix - url prefix :param path - folder with files """ - route = StaticRoute(name, prefix, path, + resource = PrefixResource(prefix, name=name) + self._reg_resource(resource) + route = StaticRoute(name, prefix, path, resource, expect_handler=expect_handler, chunk_size=chunk_size, response_factory=response_factory) - resource = PrefixResource(prefix, name=name) - self._reg_resource(resource) resource.register_route(route) return route From 82e6bc919b8f217e38b3eab1f0dbfd14aec36e3c Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sun, 10 Jan 2016 03:50:09 +0200 Subject: [PATCH 05/34] Implement ResourceAdapter --- aiohttp/web_urldispatcher.py | 125 ++++++++++++++++++++++++----------- 1 file changed, 87 insertions(+), 38 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index cd60f38308f..20d3de11f03 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -54,11 +54,67 @@ def _defaultExpectHandler(request): request.transport.write(b"HTTP/1.1 100 Continue\r\n\r\n") -class Resource(metaclass=abc.ABCMeta): - METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} +class BaseResource(metaclass=abc.ABCMeta): def __init__(self, *, name=None): self._name = name + + @property + def name(self): + return self._name + + @abc.abstractmethod # pragma: no branch + def match(self, path): + """Return dict with info for given path or + None if route cannot process path.""" + + @abc.abstractmethod # pragma: no branch + def url(self, **kwargs): + """Construct url for route with additional params.""" + + @abc.abstractmethod # pragma: no branch + def resolve(self, method, path): + """Resolve resource + + Return match_dict, allowed_methods pair.""" + + @staticmethod + def _append_query(url, query): + if query is not None: + return url + "?" + urlencode(query) + else: + return url + + +class ResourceAdapter(BaseResource): + + def __init__(self, route): + assert isinstance(route, Route), \ + 'Instance of Route class is require, got {!r}'.format(route) + super().__init__(name=route.name) + self._route = route + + def match(self, path): + return self._route.match(path) + + def url(self, **kwargs): + return self._route.url(**kwargs) + + def resolve(self, method, path): + match_dict = self._route.match(path) + allowed_methods = {self._route.method} + if match_dict: + return (UrlMappingMatchInfo(match_dict, self._route), + allowed_methods) + else: + return None, allowed_methods + + +class Resource(BaseResource): + METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} + + def __init__(self, *, name=None): + super().__init__(name=name) self._routes = [] def add_route(self, method, handler, *, @@ -96,19 +152,11 @@ def register_route(self, route): 'Instance of Route class is required, got {!r}'.format(route) self._routes.append(route) - @property - def name(self): - return self._name - @abc.abstractmethod # pragma: no branch def match(self, path): """Return dict with info for given path or None if route cannot process path.""" - @abc.abstractmethod # pragma: no branch - def url(self, **kwargs): - """Construct url for route with additional params.""" - def resolve(self, method, path): allowed_methods = set() @@ -126,13 +174,6 @@ def resolve(self, method, path): else: return None, allowed_methods - @staticmethod - def _append_query(url, query): - if query is not None: - return url + "?" + urlencode(query) - else: - return url - class PlainResource(Resource): @@ -236,7 +277,7 @@ def url(self, **kwargs): def handle_expect_header(self, request): return (yield from self._expect_handler(request)) - _append_query = Resource._append_query + _append_query = staticmethod(Resource._append_query) class ResourceRoute(BaseRoute): @@ -336,14 +377,17 @@ def __repr__(self): formatter=self._formatter, handler=self.handler)) -class StaticRoute(ResourceRoute): +class StaticRoute(Route): - def __init__(self, name, prefix, directory, resource, *, + def __init__(self, name, prefix, directory, *, expect_handler=None, chunk_size=256*1024, response_factory=StreamResponse): + assert prefix.startswith('/'), prefix + assert prefix.endswith('/'), prefix super().__init__( - 'GET', self.handle, resource, - expect_handler=expect_handler) + 'GET', self.handle, name, expect_handler=expect_handler) + self._prefix = prefix + self._prefix_len = len(self._prefix) self._directory = os.path.abspath(directory) + os.sep self._chunk_size = chunk_size self._response_factory = response_factory @@ -355,6 +399,17 @@ def __init__(self, name, prefix, directory, resource, *, if bool(os.environ.get("AIOHTTP_NOSENDFILE")): self._sendfile = self._sendfile_fallback + def match(self, path): + if not path.startswith(self._prefix): + return None + return {'filename': path[self._prefix_len:]} + + def url(self, *, filename, query=None): + while filename.startswith('/'): + filename = filename[1:] + url = self._prefix + filename + return self._append_query(url, query) + def _sendfile_cb(self, fut, out_fd, in_fd, offset, count, loop, registered): if registered: @@ -380,15 +435,10 @@ def _sendfile_system(self, req, resp, fobj, count): """ Write `count` bytes of `fobj` to `resp` starting from `offset` using the ``sendfile`` system call. - `req` should be a :obj:`aiohttp.web.Request` instance. - `resp` should be a :obj:`aiohttp.web.StreamResponse` instance. - `fobj` should be an open file object. - `offset` should be an integer >= 0. - `count` should be an integer > 0. """ transport = req.transport @@ -414,7 +464,6 @@ def _sendfile_fallback(self, req, resp, fobj, count): Mimic the :meth:`_sendfile_system` method, but without using the ``sendfile`` system call. This should be used on systems that don't support the ``sendfile`` system call. - To avoid blocking the event loop & to keep memory usage low, `fobj` is transferred in chunks controlled by the `chunk_size` argument to :class:`StaticRoute`. @@ -654,14 +703,13 @@ def named_routes(self): return MappingProxyType(self._named_resources) def register_route(self, route): - assert isinstance(route, Route), \ - 'Instance of Route class is require, got {!r}'.format(route) - assert route.resource is None, "Cannot register already registered" - self._reg_resource(route.resource) + resource = ResourceAdapter(route) + self._reg_resource(resource) def _reg_resource(self, resource): - assert isinstance(resource, Resource), \ - 'Instance of Resource class is required, got {!r}'.format(resource) + assert isinstance(resource, BaseResource), \ + 'Instance of BaseResource class is required, got {!r}'.format( + resource) name = resource.name @@ -731,11 +779,12 @@ def add_static(self, prefix, path, *, name=None, expect_handler=None, :param prefix - url prefix :param path - folder with files """ - resource = PrefixResource(prefix, name=name) - self._reg_resource(resource) - route = StaticRoute(name, prefix, path, resource, + assert prefix.startswith('/') + if not prefix.endswith('/'): + prefix += '/' + route = StaticRoute(name, prefix, path, expect_handler=expect_handler, chunk_size=chunk_size, response_factory=response_factory) - resource.register_route(route) + self.register_route(route) return route From 4868b1920ccb749ac4fc69fb47e08e53d9f6f713 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sun, 10 Jan 2016 04:01:16 +0200 Subject: [PATCH 06/34] Drop PrefixResource --- aiohttp/web_urldispatcher.py | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 20d3de11f03..f1c65dbecc0 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -222,33 +222,6 @@ def __repr__(self): .format(name=name, formatter=self._formatter)) -class PrefixResource(Resource): - - def __init__(self, prefix, *, name=None): - if not prefix.endswith('/'): - prefix += '/' - assert prefix.endswith('/'), prefix - super().__init__(name=name) - self._prefix = prefix - self._prefix_len = len(self._prefix) - - def match(self, path): - if not path.startswith(self._prefix): - return None - return {'filename': path[self._prefix_len:]} - - def url(self, *, filename, query=None): - while filename.startswith('/'): - filename = filename[1:] - url = self._prefix + filename - return Resource._append_query(url, query) - - def __repr__(self): - name = "'" + self.name + "' " if self.name is not None else "" - return " Date: Sun, 10 Jan 2016 04:26:58 +0200 Subject: [PATCH 07/34] Fix reprs --- aiohttp/web_urldispatcher.py | 54 ++++++++++++++++++++++++++++++------ tests/test_urldispatch.py | 11 ++++---- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index f1c65dbecc0..936fdd7decd 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -54,7 +54,7 @@ def _defaultExpectHandler(request): request.transport.write(b"HTTP/1.1 100 Continue\r\n\r\n") -class BaseResource(metaclass=abc.ABCMeta): +class BaseResource(Sized, Iterable): def __init__(self, *, name=None): self._name = name @@ -90,7 +90,7 @@ class ResourceAdapter(BaseResource): def __init__(self, route): assert isinstance(route, Route), \ - 'Instance of Route class is require, got {!r}'.format(route) + 'Instance of Route class is required, got {!r}'.format(route) super().__init__(name=route.name) self._route = route @@ -109,6 +109,16 @@ def resolve(self, method, path): else: return None, allowed_methods + @property + def route(self): + return self._route + + def __len__(self): + return 1 + + def __iter__(self): + yield self._route + class Resource(BaseResource): METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} @@ -174,6 +184,16 @@ def resolve(self, method, path): else: return None, allowed_methods + @property + def routes(self): + return self._routes + + def __len__(self): + return len(self._routes) + + def __iter__(self): + return iter(self._routes) + class PlainResource(Resource): @@ -597,19 +617,35 @@ def _raise_allowed_methods(self): class RoutesView(Sized, Iterable, Container): - __slots__ = '_urls' - - def __init__(self, urls): - self._urls = urls + def __init__(self, resources): + self._routes = [] + for resource in resources: + for route in resource: + self._routes.append(route) def __len__(self): - return len(self._urls) + return len(self._routes) def __iter__(self): - yield from self._urls + yield from self._routes def __contains__(self, route): - return route in self._urls + return route in self._routes + + +class ResourcesView(Sized, Iterable, Container): + + def __init__(self, resources): + self._resources = resources + + def __len__(self): + return len(self._resources) + + def __iter__(self): + yield from self._resources + + def __contains__(self, resource): + return resource in self._resources class UrlDispatcher(AbstractRouter, collections.abc.Mapping): diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 860d693551a..ca524bdaffd 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -321,20 +321,21 @@ def test_contains(self): def test_plain_repr(self): handler = self.make_handler() - self.router.add_route('GET', '/get/path', handler, name='name') - self.assertRegex(repr(self.router['name']), + route = PlainRoute('GET', handler, 'name', '/get/path') + self.assertRegex(repr(route), r" Date: Sun, 10 Jan 2016 04:33:15 +0200 Subject: [PATCH 08/34] Fix more tests --- aiohttp/web_urldispatcher.py | 6 +----- tests/test_urldispatch.py | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 936fdd7decd..105cd65b428 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -103,7 +103,7 @@ def url(self, **kwargs): def resolve(self, method, path): match_dict = self._route.match(path) allowed_methods = {self._route.method} - if match_dict: + if match_dict is not None: return (UrlMappingMatchInfo(match_dict, self._route), allowed_methods) else: @@ -184,10 +184,6 @@ def resolve(self, method, path): else: return None, allowed_methods - @property - def routes(self): - return self._routes - def __len__(self): return len(self._routes) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index ca524bdaffd..c1eb4537ff0 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -241,7 +241,7 @@ def test_double_add_url_with_the_same_name(self): def test_route_plain(self): handler = self.make_handler() route = self.router.add_route('GET', '/get', handler, name='name') - route2 = self.router['name'] + route2 = next(iter(self.router['name'])) url = route2.url() self.assertEqual('/get', url) self.assertIs(route, route2) @@ -255,7 +255,7 @@ def test_route_dynamic(self): route = self.router.add_route('GET', '/get/{name}', handler, name='name') - route2 = self.router['name'] + route2 = next(iter(self.router['name'])) url = route2.url(parts={'name': 'John'}) self.assertEqual('/get/John', url) self.assertIs(route, route2) From 783b77b5518f4688042d3bd14306aafabd64d73c Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sun, 10 Jan 2016 04:42:34 +0200 Subject: [PATCH 09/34] Make tests green --- aiohttp/web_urldispatcher.py | 2 +- tests/test_urldispatch.py | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 105cd65b428..6bf26903790 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -278,7 +278,7 @@ def __init__(self, method, handler, resource, *, self._resource = resource def __repr__(self): - return " {handler!r}".format( + return " {handler!r}".format( method=self.method, resource=self._resource, handler=self.handler) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index c1eb4537ff0..02f1fadc56e 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -13,7 +13,8 @@ from aiohttp.web_urldispatcher import (_defaultExpectHandler, DynamicRoute, PlainRoute, - SystemRoute) + SystemRoute, + ResourceRoute) class TestUrlDispatcher(unittest.TestCase): @@ -271,10 +272,10 @@ def test_add_static(self): route = self.router.add_static('/st', os.path.dirname(aiohttp.__file__), name='static') - route2 = self.router['static'] - url = route2.url(filename='/dir/a.txt') + resource = self.router['static'] + url = resource.url(filename='/dir/a.txt') self.assertEqual('/st/dir/a.txt', url) - self.assertIs(route, route2) + self.assertIs(route, resource.route) def test_plain_not_match(self): handler = self.make_handler() @@ -436,9 +437,10 @@ def go(): req = self.make_request('GET', '/get/john') match_info = yield from self.router.resolve(req) + self.assertEqual({'name': 'john'}, match_info) self.maxDiff = None self.assertRegex(repr(match_info), - ">") + ">") self.loop.run_until_complete(go()) @@ -482,7 +484,7 @@ def handler(request): route = self.router.add_route( 'GET', '/', self.make_handler(), expect_handler=handler) self.assertIs(route._expect_handler, handler) - self.assertIsInstance(route, PlainRoute) + self.assertIsInstance(route, ResourceRoute) def test_custom_expect_handler_dynamic(self): @@ -493,7 +495,7 @@ def handler(request): route = self.router.add_route( 'GET', '/get/{name}', self.make_handler(), expect_handler=handler) self.assertIs(route._expect_handler, handler) - self.assertIsInstance(route, DynamicRoute) + self.assertIsInstance(route, ResourceRoute) def test_expect_handler_non_coroutine(self): @@ -658,6 +660,7 @@ def test_named_routes_abc(self): self.assertIsInstance(self.router.named_routes(), Mapping) self.assertNotIsInstance(self.router.named_routes(), MutableMapping) + @unittest.expectedFailure def test_named_routes(self): named_routes = self.fill_named_routes() From 40c9afcb728514d085d2d21479307c3eb5852ca1 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 11 Jan 2016 20:18:55 +0200 Subject: [PATCH 10/34] Drop ReourceAdapter.route --- aiohttp/web_urldispatcher.py | 4 ---- tests/test_urldispatch.py | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 6bf26903790..23186a0ac63 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -109,10 +109,6 @@ def resolve(self, method, path): else: return None, allowed_methods - @property - def route(self): - return self._route - def __len__(self): return 1 diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 02f1fadc56e..3ae79c6688d 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -275,7 +275,7 @@ def test_add_static(self): resource = self.router['static'] url = resource.url(filename='/dir/a.txt') self.assertEqual('/st/dir/a.txt', url) - self.assertIs(route, resource.route) + self.assertIs(route, next(iter(resource))) def test_plain_not_match(self): handler = self.make_handler() @@ -336,7 +336,7 @@ def test_dynamic_repr(self): def test_static_repr(self): self.router.add_static('/get', os.path.dirname(aiohttp.__file__), name='name') - self.assertRegex(repr(self.router['name'].route), + self.assertRegex(repr(next(iter(self.router['name']))), r" Date: Thu, 14 Jan 2016 14:44:37 +0200 Subject: [PATCH 11/34] Fix test for named resources --- aiohttp/web_urldispatcher.py | 24 +++--------------------- tests/test_urldispatch.py | 29 ++++++++++++++++++++--------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 23186a0ac63..cad86ba88f1 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -8,6 +8,7 @@ import os import sys import inspect +import warnings from collections.abc import Sized, Iterable, Container from urllib.parse import urlencode, unquote @@ -625,21 +626,6 @@ def __contains__(self, route): return route in self._routes -class ResourcesView(Sized, Iterable, Container): - - def __init__(self, resources): - self._resources = resources - - def __len__(self): - return len(self._resources) - - def __iter__(self): - yield from self._resources - - def __contains__(self, resource): - return resource in self._resources - - class UrlDispatcher(AbstractRouter, collections.abc.Mapping): DYN = re.compile(r'^\{(?P[a-zA-Z][_a-zA-Z0-9]*)\}$') @@ -688,10 +674,6 @@ def __contains__(self, name): def __getitem__(self, name): return self._named_resources[name] - def resources(self): - # TODO: should distinguish between iterating over resources and views - return RoutesView(self._resources) - def routes(self): return RoutesView(self._resources) @@ -700,8 +682,8 @@ def named_resources(self): def named_routes(self): # TODO: it's ambiguous but it really resources. - # DEPRECATE! - return MappingProxyType(self._named_resources) + warnings.warn("Use .named_resources instead", DeprecationWarning) + return self.named_resources() def register_route(self, route): resource = ResourceAdapter(route) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 3ae79c6688d..7f1db1c4af2 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -14,7 +14,8 @@ DynamicRoute, PlainRoute, SystemRoute, - ResourceRoute) + ResourceRoute, + BaseResource) class TestUrlDispatcher(unittest.TestCase): @@ -646,7 +647,7 @@ def test_routes_abc(self): self.assertIsInstance(self.router.routes(), Iterable) self.assertIsInstance(self.router.routes(), Container) - def fill_named_routes(self): + def fill_named_resources(self): route1 = self.router.add_route('GET', '/plain', self.make_handler(), name='route1') route2 = self.router.add_route('GET', '/variable/{name}', @@ -654,18 +655,28 @@ def fill_named_routes(self): route3 = self.router.add_static('/static', os.path.dirname(aiohttp.__file__), name='route3') - return route1, route2, route3 + return route1.name, route2.name, route3.name def test_named_routes_abc(self): self.assertIsInstance(self.router.named_routes(), Mapping) self.assertNotIsInstance(self.router.named_routes(), MutableMapping) - @unittest.expectedFailure + def test_named_resources_abc(self): + self.assertIsInstance(self.router.named_resources(), Mapping) + self.assertNotIsInstance(self.router.named_resources(), MutableMapping) + def test_named_routes(self): - named_routes = self.fill_named_routes() + self.fill_named_resources() + + with self.assertWarns(DeprecationWarning): + self.assertEqual(3, len(self.router.named_routes())) + + def test_named_resources(self): + names = self.fill_named_resources() - self.assertEqual(3, len(self.router.named_routes())) + self.assertEqual(3, len(self.router.named_resources())) - for route in named_routes: - self.assertIn(route.name, self.router.named_routes()) - self.assertEqual(route, self.router.named_routes()[route.name]) + for name in names: + self.assertIn(name, self.router.named_routes()) + self.assertIsInstance(self.router.named_routes()[name], + BaseResource) From 907e2696f0b50c722292c590357a61abf7a5280b Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 17:08:23 +0200 Subject: [PATCH 12/34] Fix ResourceAdapter bug --- aiohttp/web_urldispatcher.py | 3 ++- tests/test_urldispatch.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index cad86ba88f1..fd1bf5d5703 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -104,7 +104,7 @@ def url(self, **kwargs): def resolve(self, method, path): match_dict = self._route.match(path) allowed_methods = {self._route.method} - if match_dict is not None: + if match_dict is not None and method == self._route.method: return (UrlMappingMatchInfo(match_dict, self._route), allowed_methods) else: @@ -688,6 +688,7 @@ def named_routes(self): def register_route(self, route): resource = ResourceAdapter(route) self._reg_resource(resource) + return resource def _reg_resource(self, resource): assert isinstance(resource, BaseResource), \ diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 7f1db1c4af2..0bb043da48d 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -680,3 +680,26 @@ def test_named_resources(self): self.assertIn(name, self.router.named_routes()) self.assertIsInstance(self.router.named_routes()[name], BaseResource) + + def test_resource_adapter_not_match(self): + route = PlainRoute('GET', lambda req: None, None, '/path') + resource = self.router.register_route(route) + self.assertIsNone(resource.match('/another/path')) + + def test_resource_adapter_resolve_not_math(self): + route = PlainRoute('GET', lambda req: None, None, '/path') + resource = self.router.register_route(route) + self.assertEqual((None, {'GET'}), + resource.resolve('GET', '/another/path')) + + def test_resource_adapter_resolve_bad_method(self): + route = PlainRoute('POST', lambda req: None, None, '/path') + resource = self.router.register_route(route) + self.assertEqual((None, {'POST'}), + resource.resolve('GET', '/path')) + + def test_resource_adapter_iter(self): + route = PlainRoute('GET', lambda req: None, None, '/path') + resource = self.router.register_route(route) + self.assertEqual(1, len(resource)) + self.assertEqual([route], list(resource)) From 816d386b5714e59a599d03a34a8f1064bf6dedcd Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 17:21:12 +0200 Subject: [PATCH 13/34] More tests --- tests/test_urldispatch.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 0bb043da48d..e225ea69ed7 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -703,3 +703,10 @@ def test_resource_adapter_iter(self): resource = self.router.register_route(route) self.assertEqual(1, len(resource)) self.assertEqual([route], list(resource)) + + def test_resource_iter(self): + resource = self.router.add_resource('/path') + r1 = resource.add_route('GET', lambda req: None) + r2 = resource.add_route('POST', lambda req: None) + self.assertEqual(2, len(resource)) + self.assertEqual([r1, r2], list(resource)) From a3c9bd25e661a693c84f250120e4b883991a8813 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 17:28:04 +0200 Subject: [PATCH 14/34] Add Route.resource property Revert back router.register_route signature --- aiohttp/web_urldispatcher.py | 18 ++++++++++-------- tests/test_urldispatch.py | 13 +++++++++---- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index fd1bf5d5703..32647ed2983 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -94,6 +94,7 @@ def __init__(self, route): 'Instance of Route class is required, got {!r}'.format(route) super().__init__(name=route.name) self._route = route + route._resource = self def match(self, path): return self._route.match(path) @@ -237,10 +238,12 @@ def __repr__(self): class BaseRoute(metaclass=abc.ABCMeta): def __init__(self, method, handler, *, - expect_handler=None): + expect_handler=None, + resource=None): self._method = method self._handler = handler self._expect_handler = expect_handler + self._resource = resource @property def method(self): @@ -250,6 +253,10 @@ def method(self): def handler(self): return self._handler + @property + def resource(self): + return self._resource + @abc.abstractmethod # pragma: no branch def match(self, path): """Return dict with info for given path or @@ -271,8 +278,8 @@ class ResourceRoute(BaseRoute): def __init__(self, method, handler, resource, *, expect_handler=None): - super().__init__(method, handler, expect_handler=expect_handler) - self._resource = resource + super().__init__(method, handler, expect_handler=expect_handler, + resource=resource) def __repr__(self): return " {handler!r}".format( @@ -283,10 +290,6 @@ def __repr__(self): def name(self): return self._resource.name - @property - def resource(self): - return self._resource - def match(self, path): """Return dict with info for given path or None if route cannot process path.""" @@ -688,7 +691,6 @@ def named_routes(self): def register_route(self, route): resource = ResourceAdapter(route) self._reg_resource(resource) - return resource def _reg_resource(self, resource): assert isinstance(resource, BaseResource), \ diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index e225ea69ed7..ad603f8343a 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -683,24 +683,29 @@ def test_named_resources(self): def test_resource_adapter_not_match(self): route = PlainRoute('GET', lambda req: None, None, '/path') - resource = self.router.register_route(route) + self.router.register_route(route) + resource = route.resource + self.assertIsNotNone(resource) self.assertIsNone(resource.match('/another/path')) def test_resource_adapter_resolve_not_math(self): route = PlainRoute('GET', lambda req: None, None, '/path') - resource = self.router.register_route(route) + self.router.register_route(route) + resource = route.resource self.assertEqual((None, {'GET'}), resource.resolve('GET', '/another/path')) def test_resource_adapter_resolve_bad_method(self): route = PlainRoute('POST', lambda req: None, None, '/path') - resource = self.router.register_route(route) + self.router.register_route(route) + resource = route.resource self.assertEqual((None, {'POST'}), resource.resolve('GET', '/path')) def test_resource_adapter_iter(self): route = PlainRoute('GET', lambda req: None, None, '/path') - resource = self.router.register_route(route) + self.router.register_route(route) + resource = route.resource self.assertEqual(1, len(resource)) self.assertEqual([route], list(resource)) From df4b843e35d920dc87ccb63a2ef8f1fd382a16a3 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 17:37:31 +0200 Subject: [PATCH 15/34] Add wildcard support --- aiohttp/web_urldispatcher.py | 15 ++++++++------- tests/test_urldispatch.py | 8 ++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 32647ed2983..be914b873ad 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -103,13 +103,14 @@ def url(self, **kwargs): return self._route.url(**kwargs) def resolve(self, method, path): - match_dict = self._route.match(path) - allowed_methods = {self._route.method} - if match_dict is not None and method == self._route.method: - return (UrlMappingMatchInfo(match_dict, self._route), - allowed_methods) - else: - return None, allowed_methods + route_method = self._route.method + allowed_methods = {route_method} + if route_method == method or route_method == hdrs.METH_ANY: + match_dict = self._route.match(path) + if match_dict is not None: + return (UrlMappingMatchInfo(match_dict, self._route), + allowed_methods) + return None, allowed_methods def __len__(self): return 1 diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index ad603f8343a..dfb3162e8c6 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -702,6 +702,14 @@ def test_resource_adapter_resolve_bad_method(self): self.assertEqual((None, {'POST'}), resource.resolve('GET', '/path')) + def test_resource_adapter_resolve_wildcard(self): + route = PlainRoute('*', lambda req: None, None, '/path') + self.router.register_route(route) + resource = route.resource + match_info, allowed = resource.resolve('GET', '/path') + self.assertEqual(allowed, {'*'}) # TODO: expand wildcard + self.assertIsNotNone(match_info) + def test_resource_adapter_iter(self): route = PlainRoute('GET', lambda req: None, None, '/path') self.router.register_route(route) From d9dc9ab052a56f1b3207fc1f85a6b466f38aa162 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 17:45:22 +0200 Subject: [PATCH 16/34] Move method and handler checks into BaseRoute --- aiohttp/web_urldispatcher.py | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index be914b873ad..661d33285b4 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -120,7 +120,6 @@ def __iter__(self): class Resource(BaseResource): - METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} def __init__(self, *, name=None): super().__init__(name=name) @@ -135,20 +134,6 @@ def add_route(self, method, handler, *, assert asyncio.iscoroutinefunction(expect_handler), \ 'Coroutine is expected, got {!r}'.format(expect_handler) - assert callable(handler), handler - if asyncio.iscoroutinefunction(handler): - pass - elif inspect.isgeneratorfunction(handler): - pass - elif isinstance(handler, type) and issubclass(handler, AbstractView): - pass - else: - handler = asyncio.coroutine(handler) - - method = upstr(method) - if method not in self.METHODS: - raise ValueError("{} is not allowed HTTP method".format(method)) - # TODO: add check for duplicated methods route = ResourceRoute(method, handler, self, @@ -238,9 +223,28 @@ def __repr__(self): class BaseRoute(metaclass=abc.ABCMeta): + METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} + def __init__(self, method, handler, *, expect_handler=None, resource=None): + + method = upstr(method) + if method not in self.METHODS: + raise ValueError("{} is not allowed HTTP method".format(method)) + + if handler is not None: + assert callable(handler), handler + if asyncio.iscoroutinefunction(handler): + pass + elif inspect.isgeneratorfunction(handler): + pass + elif (isinstance(handler, type) and + issubclass(handler, AbstractView)): + pass + else: + handler = asyncio.coroutine(handler) + self._method = method self._handler = handler self._expect_handler = expect_handler @@ -639,10 +643,6 @@ class UrlDispatcher(AbstractRouter, collections.abc.Mapping): ROUTE_RE = re.compile(r'(\{[_a-zA-Z][^{}]*(?:\{[^{}]*\}[^{}]*)*\})') NAME_SPLIT_RE = re.compile('[.:-]') - METHODS = {hdrs.METH_ANY, hdrs.METH_POST, - hdrs.METH_GET, hdrs.METH_PUT, hdrs.METH_DELETE, - hdrs.METH_PATCH, hdrs.METH_HEAD, hdrs.METH_OPTIONS} - def __init__(self): super().__init__() self._resources = [] From 1602c3c120f1d638c9cb64aac59b005ca9519fad Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 18:03:37 +0200 Subject: [PATCH 17/34] Refactor expect_handler checks --- aiohttp/web_urldispatcher.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 661d33285b4..28c9f67c7ed 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -128,12 +128,6 @@ def __init__(self, *, name=None): def add_route(self, method, handler, *, expect_handler=None): - if expect_handler is None: - expect_handler = _defaultExpectHandler - - assert asyncio.iscoroutinefunction(expect_handler), \ - 'Coroutine is expected, got {!r}'.format(expect_handler) - # TODO: add check for duplicated methods route = ResourceRoute(method, handler, self, @@ -229,6 +223,12 @@ def __init__(self, method, handler, *, expect_handler=None, resource=None): + if expect_handler is None: + expect_handler = _defaultExpectHandler + + assert asyncio.iscoroutinefunction(expect_handler), \ + 'Coroutine is expected, got {!r}'.format(expect_handler) + method = upstr(method) if method not in self.METHODS: raise ValueError("{} is not allowed HTTP method".format(method)) @@ -309,11 +309,6 @@ class Route(BaseRoute): """Old fashion route""" def __init__(self, method, handler, name, *, expect_handler=None): - if expect_handler is None: - expect_handler = _defaultExpectHandler - assert asyncio.iscoroutinefunction(expect_handler), \ - 'Coroutine is expected, got {!r}'.format(expect_handler) - super().__init__(method, handler, expect_handler=expect_handler) self._name = name From 7b8a2308d48ca3c069a57df52ae839274425eda0 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 18:08:05 +0200 Subject: [PATCH 18/34] Drop meaningless code --- aiohttp/web_urldispatcher.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 28c9f67c7ed..3301de69c95 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -140,11 +140,6 @@ def register_route(self, route): 'Instance of Route class is required, got {!r}'.format(route) self._routes.append(route) - @abc.abstractmethod # pragma: no branch - def match(self, path): - """Return dict with info for given path or - None if route cannot process path.""" - def resolve(self, method, path): allowed_methods = set() From 970a9059aded5f39937ef8951178cb249c45cc6e Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 18:13:10 +0200 Subject: [PATCH 19/34] Reduce source diff --- aiohttp/web_urldispatcher.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 3301de69c95..2596b36ffa3 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -419,10 +419,15 @@ def _sendfile_system(self, req, resp, fobj, count): """ Write `count` bytes of `fobj` to `resp` starting from `offset` using the ``sendfile`` system call. + `req` should be a :obj:`aiohttp.web.Request` instance. + `resp` should be a :obj:`aiohttp.web.StreamResponse` instance. + `fobj` should be an open file object. + `offset` should be an integer >= 0. + `count` should be an integer > 0. """ transport = req.transport @@ -448,6 +453,7 @@ def _sendfile_fallback(self, req, resp, fobj, count): Mimic the :meth:`_sendfile_system` method, but without using the ``sendfile`` system call. This should be used on systems that don't support the ``sendfile`` system call. + To avoid blocking the event loop & to keep memory usage low, `fobj` is transferred in chunks controlled by the `chunk_size` argument to :class:`StaticRoute`. From a86213bf157425a5f9eceed72fa0177a2d87a554 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 19:29:10 +0200 Subject: [PATCH 20/34] Better coverage --- aiohttp/web_urldispatcher.py | 3 ++- tests/test_urldispatch.py | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 2596b36ffa3..ab92f43ed4c 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -233,7 +233,8 @@ def __init__(self, method, handler, *, if asyncio.iscoroutinefunction(handler): pass elif inspect.isgeneratorfunction(handler): - pass + warnings.warn("Bare generators are forbidden, " + "use @coroutine wrapper", DeprecationWarning) elif (isinstance(handler, type) and issubclass(handler, AbstractView)): pass diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index dfb3162e8c6..8e8ed2a3f49 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -15,7 +15,8 @@ PlainRoute, SystemRoute, ResourceRoute, - BaseResource) + BaseResource, + View) class TestUrlDispatcher(unittest.TestCase): @@ -723,3 +724,18 @@ def test_resource_iter(self): r2 = resource.add_route('POST', lambda req: None) self.assertEqual(2, len(resource)) self.assertEqual([r1, r2], list(resource)) + + def test_deprecate_bare_generators(self): + resource = self.router.add_resource('/path') + + def gen(request): + yield + + with self.assertWarns(DeprecationWarning): + resource.add_route('GET', gen) + + def test_view_route(self): + resource = self.router.add_resource('/path') + + route = resource.add_route('GET', View) + self.assertIs(View, route.handler) From 9961f348f5a55821d99413593417244c97f42cfa Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 19:29:49 +0200 Subject: [PATCH 21/34] Fix deprecation text --- aiohttp/web_urldispatcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index ab92f43ed4c..b8b50284423 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -233,7 +233,7 @@ def __init__(self, method, handler, *, if asyncio.iscoroutinefunction(handler): pass elif inspect.isgeneratorfunction(handler): - warnings.warn("Bare generators are forbidden, " + warnings.warn("Bare generators are deprecated, " "use @coroutine wrapper", DeprecationWarning) elif (isinstance(handler, type) and issubclass(handler, AbstractView)): From 8ba842478c7e0098e5fb78ffd63c82e578e40dc0 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 19:44:44 +0200 Subject: [PATCH 22/34] Finish tests coverage --- tests/test_urldispatch.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 8e8ed2a3f49..9212d8b2a9c 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -1,5 +1,6 @@ import asyncio import os +import re import unittest from collections.abc import Sized, Container, Iterable, Mapping, MutableMapping from unittest import mock @@ -739,3 +740,32 @@ def test_view_route(self): route = resource.add_route('GET', View) self.assertIs(View, route.handler) + + def test_resource_route_match(self): + resource = self.router.add_resource('/path') + route = resource.add_route('GET', lambda req: None) + self.assertEqual({}, route.match('/path')) + + def test_plain_route_url(self): + route = PlainRoute('GET', lambda req: None, None, '/path') + self.router.register_route(route) + self.assertEqual('/path?arg=1', route.url(query={'arg': 1})) + + def test_dynamic_route_url(self): + route = DynamicRoute('GET', lambda req: None, None, + '', '/{path}') + self.router.register_route(route) + self.assertEqual('/path?arg=1', route.url(parts={'path': 'path'}, + query={'arg': 1})) + + def test_dynamic_route_match_not_found(self): + route = DynamicRoute('GET', lambda req: None, None, + re.compile('/path/(?P.+)'), '/path/{to}') + self.router.register_route(route) + self.assertEqual(None, route.match('/another/path')) + + def test_dynamic_route_match_found(self): + route = DynamicRoute('GET', lambda req: None, None, + re.compile('/path/(?P.+)'), '/path/{to}') + self.router.register_route(route) + self.assertEqual({'to': 'to'}, route.match('/path/to')) From ae3b9e58cd7c68a1d22279254d595d2cbaf6c22a Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 19:53:13 +0200 Subject: [PATCH 23/34] Add deprecation warning for router.register_route --- aiohttp/web_urldispatcher.py | 1 + 1 file changed, 1 insertion(+) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index b8b50284423..64fbcb68d0c 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -687,6 +687,7 @@ def named_routes(self): return self.named_resources() def register_route(self, route): + warnings.warn("Use resource-based interface", DeprecationWarning) resource = ResourceAdapter(route) self._reg_resource(resource) From 5dc49ed964741cab93e72d54fe646c53d1bba5b0 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 19:56:21 +0200 Subject: [PATCH 24/34] Add test for deprecation register_route --- tests/test_urldispatch.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 9212d8b2a9c..ba2749ecdec 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -769,3 +769,8 @@ def test_dynamic_route_match_found(self): re.compile('/path/(?P.+)'), '/path/{to}') self.router.register_route(route) self.assertEqual({'to': 'to'}, route.match('/path/to')) + + def test_deprecate_register_route(self): + route = PlainRoute('GET', lambda req: None, None, '/path') + with self.assertWarns(DeprecationWarning): + self.router.register_route(route) From 5d8c895271488bdf954b148a423be9c03c1e7e9b Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Thu, 14 Jan 2016 21:47:09 +0200 Subject: [PATCH 25/34] Work on docs, add a rationale for the change --- CHANGES.txt | 5 +++++ aiohttp/web_urldispatcher.py | 2 +- docs/glossary.rst | 13 +++++++++++++ docs/web_reference.rst | 37 ++++++++++++++++++++++++++---------- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 035e7bad284..465bdcfa7d5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -41,3 +41,8 @@ CHANGES deprecate it for session.request() and family #736 - Enable access log by default #735 + +- Deprecate app.router.register_route() (the method was not documented + intentionally BTW). + +- Deprecate app.router.named_routes() in favor of app.router.named_resources() diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 64fbcb68d0c..3f0d56bcc14 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -682,7 +682,7 @@ def named_resources(self): return MappingProxyType(self._named_resources) def named_routes(self): - # TODO: it's ambiguous but it really resources. + # NB: it's ambiguous but it's really resources. warnings.warn("Use .named_resources instead", DeprecationWarning) return self.named_resources() diff --git a/docs/glossary.rst b/docs/glossary.rst index 9c2e4600716..c49c523bea1 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -47,6 +47,19 @@ It makes communication faster by getting rid of connection establishment for every request. + resource + + A concept reflects the HTTP **path**, every resource corresponsd + to *URI*. + + May have an unique name. + + Contains :term:`route`\'s for different HTTP methods. + + route + + A part of :term:`resource`, resource's *path* coupled with HTTP method. + web-handler An endpoint that returns HTTP response. diff --git a/docs/web_reference.rst b/docs/web_reference.rst index 91a663b4079..c2c828993d4 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1307,26 +1307,43 @@ Router is any object that implements :class:`AbstractRouter` interface. .. versionadded:: 0.18 - .. method:: named_routes() + .. method:: named_resources() Returns a :obj:`dict`-like :class:`types.MappingProxyType` *view* over - *all* named routes. + *all* named **resources**. + + The view maps every named resources's **name** to the + :class:`BaseResource` instance. It supports the usual + :obj:`dict`-like operations, except for any mutable operations + (i.e. it's **read-only**):: - The view maps every named route's :attr:`Route.name` attribute to the - :class:`Route`. It supports the usual :obj:`dict`-like operations, except - for any mutable operations (i.e. it's **read-only**):: + len(app.router.named_resources()) - len(app.router.named_routes()) + for name, resource in app.router.named_resources().items(): + print(name, resource) - for name, route in app.router.named_routes().items(): - print(name, route) + "name" in app.router.named_resources() - "route_name" in app.router.named_routes() + app.router.named_resources()["name"] - app.router.named_routes()["route_name"] + .. method:: named_routes() + + An alias for :meth:`named_resources` starting from aiohttp 0.21. .. versionadded:: 0.19 + .. versionchanged:: 0.21 + + The method is an alias for :meth:`named_resources`, so it + iterates over resources instead of routes. + + .. deprecated:: 0.21 + + Please use named **resources** instead of named **routes**. + + Several routes which belongs to the same resource shares the + resource name. + .. _aiohttp-web-route: From 2ab380075b8d248d7c636d0870ba11bf27099cab Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Fri, 15 Jan 2016 14:07:36 +0200 Subject: [PATCH 26/34] Work on docs --- docs/new_router.rst | 84 ++++++++++++++++++++++++++++++++++++++++++ docs/web_reference.rst | 15 ++++++++ 2 files changed, 99 insertions(+) create mode 100644 docs/new_router.rst diff --git a/docs/new_router.rst b/docs/new_router.rst new file mode 100644 index 00000000000..d7101abc9de --- /dev/null +++ b/docs/new_router.rst @@ -0,0 +1,84 @@ +============================= +Router refactoring in 0.21 +============================= + + +Rationale +--------- + +First generation (v1) of router mapped ``(method, path)`` pair to +:term:`web-handler`. Mapping is named **route**. Routes used to have +unique names if any. + +The main mistake with the design is coupling the **route** to +``(method, path)`` pair while really URL construction operates with +**resources** (**location** is a synonym). HTTP method is not part of URI +but applyed on sending HTTP request only. + +Having different **route names** for the same path is confusing. Moreover +**named routes** constructed for the same path should have unique +nonoverlapping names which is cumbersome is certain situations. + +From other side sometimes it's desirable to bind several HTTP methods +to the same web handler. For *v1* router it can be solved by passing '*' +as HTTP method. Class based views require '*' method also usually. + + +Implementation +-------------- + +The change introduces **resource** as first class citizen:: + + resource = router.add_resource('/path/{to}', name='name') + +*Resource* has a **path** (dynamic or constant) and optional **name**. + +The name is **unique** in router context. + +*Resource* has **routes**. + +*Route* correspons to *HTTP method* and :term:`web-handler` for the method:: + + route = resource.add_route('GET', handler) + +User still may use wildcard for accepting all HTTP methods (maybe we +will add something like ``resource.add_wildcard(handler)`` later). + +Since **names** belongs to **resources** now ``app.router['name']`` +returns a **resource** instance instead of :class:`aiohttp.web.Route`. + +**resource** has ``.url()`` method, so +``app.router['name'].url(parts={'a': 'b'}, query={'arg': 'param'})`` +still works as usual. + + +The change allows to rewrite static file handling and implement nested +applications as well. + +Decoupling of *HTTP location* and *HTTP method* makes life easier. + +Backward compatibility +----------------------- + +The refactoring is 99% compatible with previous implementation. + +99% means all example and the most of current code works without +modifications but we have subtle API backward incompatibles. + +``app.router['name']`` returns a :class:`aiohttp.web.BaseResource` +instance instead of :class:`aiohttp.web.Route` but resource has the +same ``resource.url(...)`` most useful method, so end user should feel no +difference. + +``resource.match(...)`` is supported as well (while we believe it's +not used widely). + +``app.router.add_route(method, path, handler, name='name')`` now is just +shortcut for:: + + resource = app.router.add_resource(path, name=name) + route = resource.add_route(methoc, handler) + return route + +``app.router.register_route(...)`` is still supported, it creates +:class:`aiohttp.web.ResourceAdapter` for every call (but it's deprecated now). diff --git a/docs/web_reference.rst b/docs/web_reference.rst index c2c828993d4..aa7f251084f 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1174,6 +1174,21 @@ Router is any object that implements :class:`AbstractRouter` interface. .. seealso:: :ref:`Route classes ` + .. method:: add_resource(path, *, name=None) + + Append a :term:`resource` to the end of route table. + + *path* may be either *constant* string like ``'/a/b/c'`` or + *variable rule* like ``'/a/{var}'`` (see + :ref:`handling variable pathes`) + + :param str path: resource path spec. + + :param str name: optional resource name. + + :return: created resource instance (:class:`PlainResource` or + :class:`DynamicResource`). + .. method:: add_route(method, path, handler, *, \ name=None, expect_handler=None) From 98cf6a20610c906cc0e27a82fe1a67543ed1ddfb Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sat, 16 Jan 2016 20:08:38 +0200 Subject: [PATCH 27/34] More docs --- docs/web_reference.rst | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/docs/web_reference.rst b/docs/web_reference.rst index aa7f251084f..2eff7d802be 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1360,12 +1360,52 @@ Router is any object that implements :class:`AbstractRouter` interface. resource name. +.. _aiohttp-web-resource: + +Resource +^^^^^^^^ + +Default router :class:`UrlDispatcher` operates with :term:`resource`\s. + +Resource is an item in *routing table* which has a *path*, an optional +unique *name* and at least one :term:`route`. + +:term:`web-handler` lookup is performed in the following way: + +1. Router iterates over *resources* one-by-one. +2. If *resource* matches to requested URL the resource iterates over + own *routes*. +3. If route matches to requested HTTP method (or ``'*'`` wildcard) the + route's handler is used as found :term:`web-handler`. The lookup is + finished. +4. Otherwise router tries next resource from the *routing table*. +5. If the end of *routing table* is reached and no *resource* / + *route* pair found the *router* returns special :class:`SystemRoute` + instance with either *HTTP 404 Not Found* or *HTTP 405 + Method Not Allowed* status code. Registerd :term:`web-handler` for + *system route* raises corresponding :ref:`web exception + `. + +User should never instantiate resource classes but give it by +:meth:`UrlDispatcher.add_resource` call. + +After that he may add a :term:`route` by calling :meth:`Resource.add_route`. + +:meth:`UrlDispatcher.add_route` is just shortcut for:: + + router.add_resource(path).add_route(method, handler) + .. _aiohttp-web-route: Route ^^^^^ -Default router :class:`UrlDispatcher` operates with *routes*. +Route has HTTP method (wildcard ``'*'`` is an option), +:term:`web-handler` and optional *expect handler*. + +Every route belong to some resource. + + User should not instantiate route classes by hand but can give *named route instance* by ``router[name]`` if he have added route by From 4cb301625cb3513a20c0e4c2b8da1b9501101818 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sat, 16 Jan 2016 20:57:41 +0200 Subject: [PATCH 28/34] Add a check on double route adding --- aiohttp/web_urldispatcher.py | 5 +++++ tests/test_urldispatch.py | 14 ++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 3f0d56bcc14..e0c6b362b93 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -129,6 +129,11 @@ def add_route(self, method, handler, *, expect_handler=None): # TODO: add check for duplicated methods + for route in self._routes: + if route.method == method or route.method == hdrs.METH_ANY: + raise RuntimeError("Added route will never be executed, " + "method {route.method} is " + "already registered".format(route=route)) route = ResourceRoute(method, handler, self, expect_handler=expect_handler) diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index ba2749ecdec..02f7f36211d 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -774,3 +774,17 @@ def test_deprecate_register_route(self): route = PlainRoute('GET', lambda req: None, None, '/path') with self.assertWarns(DeprecationWarning): self.router.register_route(route) + + def test_error_on_double_route_adding(self): + resource = self.router.add_resource('/path') + + resource.add_route('GET', lambda: None) + with self.assertRaises(RuntimeError): + resource.add_route('GET', lambda: None) + + def test_error_on_adding_route_after_wildcard(self): + resource = self.router.add_resource('/path') + + resource.add_route('*', lambda: None) + with self.assertRaises(RuntimeError): + resource.add_route('GET', lambda: None) From 7341c815d4b6c687d27d564fdf01080bedfcde72 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sat, 16 Jan 2016 20:57:57 +0200 Subject: [PATCH 29/34] Drop obsolete TODO --- aiohttp/web_urldispatcher.py | 1 - 1 file changed, 1 deletion(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index e0c6b362b93..d4a13511a9b 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -128,7 +128,6 @@ def __init__(self, *, name=None): def add_route(self, method, handler, *, expect_handler=None): - # TODO: add check for duplicated methods for route in self._routes: if route.method == method or route.method == hdrs.METH_ANY: raise RuntimeError("Added route will never be executed, " From dc0e742c8a57ca65919318607dcff7c59b84d1c6 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sat, 16 Jan 2016 21:34:33 +0200 Subject: [PATCH 30/34] Introduce AbstractResource and AbstractRoute --- aiohttp/web_urldispatcher.py | 16 ++++++++-------- docs/web_reference.rst | 11 +++++++++++ tests/test_urldispatch.py | 4 ++-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index d4a13511a9b..345ac4e43fd 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -55,7 +55,7 @@ def _defaultExpectHandler(request): request.transport.write(b"HTTP/1.1 100 Continue\r\n\r\n") -class BaseResource(Sized, Iterable): +class AbstractResource(Sized, Iterable): def __init__(self, *, name=None): self._name = name @@ -87,7 +87,7 @@ def _append_query(url, query): return url -class ResourceAdapter(BaseResource): +class ResourceAdapter(AbstractResource): def __init__(self, route): assert isinstance(route, Route), \ @@ -119,7 +119,7 @@ def __iter__(self): yield self._route -class Resource(BaseResource): +class Resource(AbstractResource): def __init__(self, *, name=None): super().__init__(name=name) @@ -215,7 +215,7 @@ def __repr__(self): .format(name=name, formatter=self._formatter)) -class BaseRoute(metaclass=abc.ABCMeta): +class AbstractRoute(metaclass=abc.ABCMeta): METHODS = hdrs.METH_ALL | {hdrs.METH_ANY} def __init__(self, method, handler, *, @@ -278,7 +278,7 @@ def handle_expect_header(self, request): _append_query = staticmethod(Resource._append_query) -class ResourceRoute(BaseRoute): +class ResourceRoute(AbstractRoute): """A route with resource""" def __init__(self, method, handler, resource, *, @@ -305,7 +305,7 @@ def url(self, **kwargs): return self._resource.url(**kwargs) -class Route(BaseRoute): +class Route(AbstractRoute): """Old fashion route""" def __init__(self, method, handler, name, *, expect_handler=None): @@ -696,8 +696,8 @@ def register_route(self, route): self._reg_resource(resource) def _reg_resource(self, resource): - assert isinstance(resource, BaseResource), \ - 'Instance of BaseResource class is required, got {!r}'.format( + assert isinstance(resource, AbstractResource), \ + 'Instance of AbstractResource class is required, got {!r}'.format( resource) name = resource.name diff --git a/docs/web_reference.rst b/docs/web_reference.rst index 2eff7d802be..272a8b0e216 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1395,6 +1395,17 @@ After that he may add a :term:`route` by calling :meth:`Resource.add_route`. router.add_resource(path).add_route(method, handler) +Resource classes hierarhy:: + + AbstractResource + Resource + PlainResource + DynamicResource + ResourceAdapter + + + + .. _aiohttp-web-route: Route diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 02f7f36211d..061a36f8c8c 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -16,7 +16,7 @@ PlainRoute, SystemRoute, ResourceRoute, - BaseResource, + AbstractResource, View) @@ -681,7 +681,7 @@ def test_named_resources(self): for name in names: self.assertIn(name, self.router.named_routes()) self.assertIsInstance(self.router.named_routes()[name], - BaseResource) + AbstractResource) def test_resource_adapter_not_match(self): route = PlainRoute('GET', lambda req: None, None, '/path') From 983bc343961516f2606d5455070cc5b6aa5b8600 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Sat, 16 Jan 2016 23:22:01 +0200 Subject: [PATCH 31/34] More docs --- aiohttp/web_urldispatcher.py | 6 +-- docs/web_reference.rst | 74 ++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 345ac4e43fd..cac7896609a 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -67,17 +67,17 @@ def name(self): @abc.abstractmethod # pragma: no branch def match(self, path): """Return dict with info for given path or - None if route cannot process path.""" + None if route cannot process the path.""" @abc.abstractmethod # pragma: no branch def url(self, **kwargs): - """Construct url for route with additional params.""" + """Construct url for resource with additional params.""" @abc.abstractmethod # pragma: no branch def resolve(self, method, path): """Resolve resource - Return match_dict, allowed_methods pair.""" + Return (UrlMappingMatchInfo, allowed_methods) pair.""" @staticmethod def _append_query(url, query): diff --git a/docs/web_reference.rst b/docs/web_reference.rst index 272a8b0e216..7805b80e49b 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1404,6 +1404,80 @@ Resource classes hierarhy:: ResourceAdapter +.. class:: AbstractResource + + A base class for all resources. + + Inherited from :class:`collections.abc.Sized` and + :class:`collections.abc.Iterable`. + + .. attribute:: name + + Read-only *name* of resource or ``None``. + + .. method:: match(path) + + :param str path: *path* part of requested URL. + + :return: :class:`dict` with info for given *path* or + ``None`` if route cannot process the path. + + .. method:: resolve(method, path) + + Resolve resource by finding appropriate :term:`web-handler` for + ``(method, path)`` combination. + + Calls :meth:`match` internally. + + :param str method: requested HTTP method. + + :return: (*match_info*, *allowed_methods*) pair. + + *allowed_methods* is a :class:`set` or HTTP methods accepted by + resource. + + *match_info* is either :class:`UrlMappingMatchInfo` if + request is resolved or ``None`` if no :term:`route` is + found. + + .. method:: url(**kwargs) + + Construct an URL for route with additional params. + + **kwargs** depends on a list accepted by inherited resource + class parameters. + + :return: :class:`str` -- resulting URL. + + +.. class:: Resource + + A base class for new-style resources, inherits :class:`AbstractResource`. + + +.. class:: PlainResource + + A new-style resource, inherited from :class:`Resource`. + + The class corresponds to resources with plain-text matching, + ``'/path/to'`` for example. + + +.. class:: DynamicResource + + A new-style resource, inherited from :class:`Resource`. + + The class corresponds to resources with + :ref:`variable ` matching, + e.g. ``'/path/{to}/{param}'`` etc. + + +.. class:: ResourceAdapter + + An adapter for old-style routes. + + The adapter is used by ``router.register_route()`` call, the method + is deprecated and will be removed eventually. .. _aiohttp-web-route: From 283af3eb17867babd21266853db4b163a764a2ad Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 18 Jan 2016 19:47:17 +0200 Subject: [PATCH 32/34] Drop abstract route, introduce abstract expect-handler --- aiohttp/abc.py | 4 ++-- aiohttp/web.py | 2 +- aiohttp/web_urldispatcher.py | 12 ++++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/aiohttp/abc.py b/aiohttp/abc.py index 535b4720951..19d5f7c260d 100644 --- a/aiohttp/abc.py +++ b/aiohttp/abc.py @@ -23,8 +23,8 @@ def handler(self): @property # pragma: no branch @abstractmethod - def route(self): - """Return route for match info""" + def expect_header(self, request): + """Expect handler for 100-continue processing""" class AbstractView(metaclass=ABCMeta): diff --git a/aiohttp/web.py b/aiohttp/web.py index f0246042f92..26cdd0daeac 100644 --- a/aiohttp/web.py +++ b/aiohttp/web.py @@ -78,7 +78,7 @@ def handle_request(self, message, payload): expect = request.headers.get(hdrs.EXPECT) if expect and expect.lower() == "100-continue": resp = ( - yield from match_info.route.handle_expect_header(request)) + yield from match_info.expect_handler(request)) if resp is None: handler = match_info.handler diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index cac7896609a..b7539d6c435 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -44,6 +44,10 @@ def handler(self): def route(self): return self._route + @property + def expect_handler(self): + return self._route.handle_expect_header + def __repr__(self): return "".format(super().__repr__(), self._route) @@ -569,6 +573,10 @@ def handler(self): def _not_found(self, request): raise HTTPNotFound() + @property + def expect_handler(self): + return self.route.handle_expect_header + def __repr__(self): return "" @@ -590,6 +598,10 @@ def handler(self): def _not_allowed(self, request): raise HTTPMethodNotAllowed(self._method, self._allowed_methods) + @property + def expect_handler(self): + return self.route.handle_expect_header + def __repr__(self): return ("" .format(self._method, From 45a92b6919b80c275ab2aa63e5716e88db24bf07 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 18 Jan 2016 20:48:26 +0200 Subject: [PATCH 33/34] Drop NEW_ROUTER.txt --- NEW_ROUTER.txt | 69 -------------------------------------------------- 1 file changed, 69 deletions(-) delete mode 100644 NEW_ROUTER.txt diff --git a/NEW_ROUTER.txt b/NEW_ROUTER.txt deleted file mode 100644 index c5d95c99c70..00000000000 --- a/NEW_ROUTER.txt +++ /dev/null @@ -1,69 +0,0 @@ -The router should be 99% compatible with current implementation. - -99% means all example and the most of tests should work without -modifications but subtle API backward implatible changes are allowed. - -First generation (v1) router maps (method, path) pair to web-handler. -Mapping is named *route*. Routes may have unique names. - -The main mistake with the design is coupling route to (method, path) -pair while really URL construction operates with *resources* -(*location* is a synonym). HTTP method is not part of URI but applyed -on sending HTTP request. - -Having different route names for the same path is confusing. Moreover -named routes constructed for the same path should have unique -nonoverlapping names which is cumbersome is certain situations. - -From other side sometimes it's desirable to bind several HTTP methods -to the same web handler. In v1 router it can be solved by passing '*' -as HTTP method. Class based views require '*' method also usually. - -The proposal introduces *Resource* class (with PlainResource and -DynamicResource descendants). Resource has the following attributes: -name, match() and url(). - - -router.add_route('get', '/path/index', handler0) -router.add_route('get', '/path/{to}', handler1, name='a') -router.add_route('post', '/path/{to}', handler2, name='a') - - -resource1 = router.add_resource('/path/{to}', name='a') -resource2 = router.add_or_find_resource('/path/{to}', name='a') -resource1 is resource2 - - -resource.add_handler('get', handler1) -resource.add_handler('post', handler2) -resource.add_view(MyCBView) -resource.add_handler('*', sink) -resource.add_handler('post', handler3) # raise - -resource['post'] = handler2 - -resource.add_app(sub) - - -resource.url(['a', 'b']) - -router['a'].url(parts={'a': 'c', 'b': 'd'}) - -router['a'].url(a='c', b='d').query(md, z=1) - -router['a'].url(filename='f.txt').query(md, z=1) - -router['a'].url().query(md, z=1) - - - - -dt = Application() -dt.router.add_route() -dt.middlewares.append() -dt['dfdsf'] = 23423 - - -def setup(app): - res = app.route.add_resource('/debugtoolbar') - res.add_app(dt) From d880f9e6345e54c12c5759570bd14f55d0d00441 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Mon, 18 Jan 2016 21:12:41 +0200 Subject: [PATCH 34/34] Fix typo --- aiohttp/abc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/abc.py b/aiohttp/abc.py index 19d5f7c260d..9f51f223e7b 100644 --- a/aiohttp/abc.py +++ b/aiohttp/abc.py @@ -23,7 +23,7 @@ def handler(self): @property # pragma: no branch @abstractmethod - def expect_header(self, request): + def expect_handler(self): """Expect handler for 100-continue processing"""