From 41c52487eeec920bfe3caae4eddb9c020d472d81 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Fri, 27 Jan 2017 21:00:33 -0600 Subject: [PATCH 1/6] Fixes route overloading for dynamic routes Addresses #353, now dynamic routes work alongside our newly minted overloaded routes! Also fixed an unintended side effect where methods were still being passed in as None for `Sanic.add_route`. --- sanic/router.py | 27 +++++++++++++++++++-------- sanic/sanic.py | 2 +- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/sanic/router.py b/sanic/router.py index ec67f690..e25572fb 100644 --- a/sanic/router.py +++ b/sanic/router.py @@ -210,29 +210,40 @@ class Router: url = host + url # Check against known static routes route = self.routes_static.get(url) + method_not_supported = InvalidUsage( + 'Method {} not allowed for URL {}'.format( + method, url), status_code=405) if route: + if route.methods and method not in route.methods: + raise method_not_supported match = route.pattern.match(url) else: + route_found = False # Move on to testing all regex routes for route in self.routes_dynamic[url_hash(url)]: match = route.pattern.match(url) - if match: + route_found |= match is not None + # Do early method checking + if match and method in route.methods: break else: # Lastly, check against all regex routes that cannot be hashed for route in self.routes_always_check: match = route.pattern.match(url) - if match: + route_found |= match is not None + # Do early method checking + if match and method in route.methods: break else: + # Route was found but the methods didn't match + if route_found: + raise method_not_supported raise NotFound('Requested URL {} not found'.format(url)) - if route.methods and method not in route.methods: - raise InvalidUsage( - 'Method {} not allowed for URL {}'.format( - method, url), status_code=405) - kwargs = {p.name: p.cast(value) for value, p in zip(match.groups(1), route.parameters)} - return route.handler, [], kwargs + route_handler = route.handler + if hasattr(route_handler, 'handlers'): + route_handler = route_handler.handlers[method] + return route_handler, [], kwargs diff --git a/sanic/sanic.py b/sanic/sanic.py index 1ed14b7f..00b1961b 100644 --- a/sanic/sanic.py +++ b/sanic/sanic.py @@ -90,7 +90,7 @@ class Sanic: def patch(self, uri, host=None): return self.route(uri, methods=["PATCH"], host=host) - def add_route(self, handler, uri, methods=None, host=None): + def add_route(self, handler, uri, methods=frozenset({'GET'}), host=None): """ A helper method to register class instance or functions as a handler to the application url From 13803bdb304e78355a8b45078ca8c36adedc6775 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Fri, 27 Jan 2017 22:05:46 -0600 Subject: [PATCH 2/6] Update for HTTPMethodView compatibility --- sanic/constants.py | 1 + sanic/sanic.py | 10 +++++++++- tests/test_views.py | 34 ++++++++++++++++++---------------- 3 files changed, 28 insertions(+), 17 deletions(-) create mode 100644 sanic/constants.py diff --git a/sanic/constants.py b/sanic/constants.py new file mode 100644 index 00000000..8bb97ed3 --- /dev/null +++ b/sanic/constants.py @@ -0,0 +1 @@ +HTTP_METHODS = ('GET', 'POST', 'PUT', 'HEAD', 'OPTIONS', 'PATCH', 'DELETE') \ No newline at end of file diff --git a/sanic/sanic.py b/sanic/sanic.py index 00b1961b..1e4de2fb 100644 --- a/sanic/sanic.py +++ b/sanic/sanic.py @@ -6,6 +6,7 @@ from inspect import isawaitable, stack, getmodulename from traceback import format_exc from .config import Config +from .constants import HTTP_METHODS from .exceptions import Handler from .exceptions import ServerError from .log import log @@ -90,6 +91,9 @@ class Sanic: def patch(self, uri, host=None): return self.route(uri, methods=["PATCH"], host=host) + def delete(self, uri, host=None): + return self.route(uri, methods=["DELETE"], host=host) + def add_route(self, handler, uri, methods=frozenset({'GET'}), host=None): """ A helper method to register class instance or @@ -98,9 +102,13 @@ class Sanic: :param handler: function or class instance :param uri: path of the URL - :param methods: list or tuple of methods allowed + :param methods: list or tuple of methods allowed, these are overridden + if using a HTTPMethodView :return: function or class instance """ + # Handle HTTPMethodView differently + if hasattr(handler, 'view_class'): + methods = frozenset(HTTP_METHODS) self.route(uri=uri, methods=methods, host=host)(handler) return handler diff --git a/tests/test_views.py b/tests/test_views.py index 24647cf6..3c695500 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -1,43 +1,45 @@ +import pytest as pytest + from sanic import Sanic from sanic.response import text, HTTPResponse from sanic.views import HTTPMethodView from sanic.blueprints import Blueprint from sanic.request import Request from sanic.utils import sanic_endpoint_test +from sanic.constants import HTTP_METHODS -def test_methods(): +@pytest.mark.parametrize('method', HTTP_METHODS) +def test_methods(method): app = Sanic('test_methods') class DummyView(HTTPMethodView): def get(self, request): - return text('I am get method') + return text('I am GET method') def post(self, request): - return text('I am post method') + return text('I am POST method') def put(self, request): - return text('I am put method') + return text('I am PUT method') + + def head(self, request): + return text('I am HEAD method') + + def options(self, request): + return text('I am OPTIONS method') def patch(self, request): - return text('I am patch method') + return text('I am PATCH method') def delete(self, request): - return text('I am delete method') + return text('I am DELETE method') app.add_route(DummyView.as_view(), '/') - request, response = sanic_endpoint_test(app, method="get") - assert response.text == 'I am get method' - request, response = sanic_endpoint_test(app, method="post") - assert response.text == 'I am post method' - request, response = sanic_endpoint_test(app, method="put") - assert response.text == 'I am put method' - request, response = sanic_endpoint_test(app, method="patch") - assert response.text == 'I am patch method' - request, response = sanic_endpoint_test(app, method="delete") - assert response.text == 'I am delete method' + request, response = sanic_endpoint_test(app, method=method) + assert response.text == 'I am {} method'.format(method) def test_unexisting_methods(): From dea8e16f49223886b50032fe36f49e836b62ef23 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Fri, 27 Jan 2017 22:07:31 -0600 Subject: [PATCH 3/6] Force method to lower --- sanic/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sanic/utils.py b/sanic/utils.py index 1943652c..644a2a22 100644 --- a/sanic/utils.py +++ b/sanic/utils.py @@ -9,7 +9,8 @@ async def local_request(method, uri, cookies=None, *args, **kwargs): url = 'http://{host}:{port}{uri}'.format(host=HOST, port=PORT, uri=uri) log.info(url) async with aiohttp.ClientSession(cookies=cookies) as session: - async with getattr(session, method)(url, *args, **kwargs) as response: + async with getattr( + session, method.lower())(url, *args, **kwargs) as response: response.text = await response.text() response.body = await response.read() return response From ae0876876ee36a2a632415f77d2cec49309cae47 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Fri, 27 Jan 2017 22:13:16 -0600 Subject: [PATCH 4/6] Switch them to verifying headers instead --- tests/test_views.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_views.py b/tests/test_views.py index 3c695500..4e5b17f0 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -16,30 +16,30 @@ def test_methods(method): class DummyView(HTTPMethodView): def get(self, request): - return text('I am GET method') + return text('', headers={'method': 'GET'}) def post(self, request): - return text('I am POST method') + return text('', headers={'method': 'POST'}) def put(self, request): - return text('I am PUT method') + return text('', headers={'method': 'PUT'}) def head(self, request): - return text('I am HEAD method') + return text('', headers={'method': 'HEAD'}) def options(self, request): - return text('I am OPTIONS method') + return text('', headers={'method': 'OPTIONS'}) def patch(self, request): - return text('I am PATCH method') + return text('', headers={'method': 'PATCH'}) def delete(self, request): - return text('I am DELETE method') + return text('', headers={'method': 'DELETE'}) app.add_route(DummyView.as_view(), '/') request, response = sanic_endpoint_test(app, method=method) - assert response.text == 'I am {} method'.format(method) + assert response.headers['method'] == method def test_unexisting_methods(): From d3344da9c59314e5260ae4f8c8cc1f30e5d69e3f Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Fri, 27 Jan 2017 22:15:34 -0600 Subject: [PATCH 5/6] Add a pesky newline --- sanic/constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanic/constants.py b/sanic/constants.py index 8bb97ed3..0d659f0c 100644 --- a/sanic/constants.py +++ b/sanic/constants.py @@ -1 +1 @@ -HTTP_METHODS = ('GET', 'POST', 'PUT', 'HEAD', 'OPTIONS', 'PATCH', 'DELETE') \ No newline at end of file +HTTP_METHODS = ('GET', 'POST', 'PUT', 'HEAD', 'OPTIONS', 'PATCH', 'DELETE') From 0a5fa72099dfebbe3b05db9fd881acb386e3c805 Mon Sep 17 00:00:00 2001 From: Eli Uriegas Date: Sun, 29 Jan 2017 15:16:07 -0600 Subject: [PATCH 6/6] Add logic to make dynamic route merging work This is by no means the final solution but it's a start in the right direction. Eventually what needs to happen is we need to reduce the complexity of the routing. CompsitionView can probably be removed later on in favor of better Route objects. Also in the next version of sanic we need to move merge_route and add_parameter out of the add_route logic and just have them as standalone methods. The tests should cover everything that we need so that if any changes are made we can identify regression. --- sanic/router.py | 25 +++++++++++++++++++- tests/test_dynamic_routes.py | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 tests/test_dynamic_routes.py diff --git a/sanic/router.py b/sanic/router.py index e25572fb..1f26171e 100644 --- a/sanic/router.py +++ b/sanic/router.py @@ -149,7 +149,22 @@ class Router: handler=view, methods=methods.union(route.methods)) return route - route = self.routes_all.get(uri) + if parameters: + # TODO: This is too complex, we need to reduce the complexity + if properties['unhashable']: + routes_to_check = self.routes_always_check + ndx, route = self.check_dynamic_route_exists( + pattern, routes_to_check) + else: + routes_to_check = self.routes_dynamic[url_hash(uri)] + ndx, route = self.check_dynamic_route_exists( + pattern, routes_to_check) + if ndx != -1: + # Pop the ndx of the route, no dups of the same route + routes_to_check.pop(ndx) + else: + route = self.routes_all.get(uri) + if route: route = merge_route(route, methods, handler) else: @@ -165,6 +180,14 @@ class Router: else: self.routes_static[uri] = route + @staticmethod + def check_dynamic_route_exists(pattern, routes_to_check): + for ndx, route in enumerate(routes_to_check): + if route.pattern == pattern: + return ndx, route + else: + return -1, None + def remove(self, uri, clean_cache=True, host=None): if host is not None: uri = host + uri diff --git a/tests/test_dynamic_routes.py b/tests/test_dynamic_routes.py new file mode 100644 index 00000000..24ba79b5 --- /dev/null +++ b/tests/test_dynamic_routes.py @@ -0,0 +1,46 @@ +from sanic import Sanic +from sanic.response import text +from sanic.utils import sanic_endpoint_test +from sanic.router import RouteExists +import pytest + + +@pytest.mark.parametrize("method,attr, expected", [ + ("get", "text", "OK1 test"), + ("post", "text", "OK2 test"), + ("put", "text", "OK2 test"), + ("delete", "status", 405), +]) +def test_overload_dynamic_routes(method, attr, expected): + app = Sanic('test_dynamic_route') + + @app.route('/overload/', methods=['GET']) + async def handler1(request, param): + return text('OK1 ' + param) + + @app.route('/overload/', methods=['POST', 'PUT']) + async def handler2(request, param): + return text('OK2 ' + param) + + request, response = sanic_endpoint_test( + app, method, uri='/overload/test') + assert getattr(response, attr) == expected + + +def test_overload_dynamic_routes_exist(): + app = Sanic('test_dynamic_route') + + @app.route('/overload/', methods=['GET']) + async def handler1(request, param): + return text('OK1 ' + param) + + @app.route('/overload/', methods=['POST', 'PUT']) + async def handler2(request, param): + return text('OK2 ' + param) + + # if this doesn't raise an error, than at least the below should happen: + # assert response.text == 'Duplicated' + with pytest.raises(RouteExists): + @app.route('/overload/', methods=['PUT', 'DELETE']) + async def handler3(request): + return text('Duplicated')