Merge pull request #360 from seemethere/fix_route_overloading_for_dynamic_routes

Fixes route overloading for dynamic routes

sanic/constants.py

@@ -0,0 +1 @@
+HTTP_METHODS = ('GET', 'POST', 'PUT', 'HEAD', 'OPTIONS', 'PATCH', 'DELETE')

sanic/router.py

@@ -150,7 +150,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:
@@ -166,6 +181,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
@@ -211,29 +234,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

sanic/sanic.py

@@ -7,6 +7,7 @@ from traceback import format_exc
 import warnings
 
 from .config import Config
+from .constants import HTTP_METHODS
 from .exceptions import Handler
 from .exceptions import ServerError
 from .log import log
@@ -91,7 +92,10 @@ 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 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
         functions as a handler to the application url
@@ -99,9 +103,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
 

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

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/<param>', methods=['GET'])
+    async def handler1(request, param):
+        return text('OK1 ' + param)
+
+    @app.route('/overload/<param>', 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/<param>', methods=['GET'])
+    async def handler1(request, param):
+        return text('OK1 ' + param)
+
+    @app.route('/overload/<param>', 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/<param>', methods=['PUT', 'DELETE'])
+        async def handler3(request):
+            return text('Duplicated')

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('', 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('', headers={'method': 'HEAD'})
+
+        def options(self, request):
+            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="get")
+    request, response = sanic_endpoint_test(app, method=method)
-    assert response.text == 'I am get method'
+    assert response.headers['method'] == 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'
 
 
 def test_unexisting_methods():