From 66405fb55be365e14f8c465b4c8995ac3c181d6f Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Wed, 17 Feb 2021 08:39:01 +0200 Subject: [PATCH 1/4] Add trailing slash when defined and strict_slashes --- sanic/app.py | 7 ++++++- tests/test_url_for.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/sanic/app.py b/sanic/app.py index 1a690db7bd..115855b097 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -394,7 +394,12 @@ def url_for(self, view_name: str, **kwargs): kwargs["file_uri"] = filename - if uri != "/" and uri.endswith("/"): + if ( + uri != "/" + and uri.endswith("/") + and not route.strict + and not route.raw_path[:-1] + ): uri = uri[:-1] if not uri.startswith("/"): diff --git a/tests/test_url_for.py b/tests/test_url_for.py index bf9a4722ab..d623cc4ad3 100644 --- a/tests/test_url_for.py +++ b/tests/test_url_for.py @@ -88,3 +88,19 @@ async def test_route3(request, ws): # TODO: add test with a route with multiple hosts # TODO: add test with a route with _host in url_for +@pytest.mark.parametrize( + "path,strict,expected", + ( + ("/foo", False, "/foo"), + ("/foo/", False, "/foo"), + ("/foo", True, "/foo"), + ("/foo/", True, "/foo/"), + ), +) +def test_trailing_slash_url_for(app, path, strict, expected): + @app.route(path, strict_slashes=strict) + def handler(*_): + ... + + url = app.url_for("handler") + assert url == expected From 72e66c4979a542e39ffc5946a206c8635162f4b1 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Wed, 17 Feb 2021 14:46:16 +0200 Subject: [PATCH 2/4] Add partial matching, and fix some issues with url_for --- sanic/app.py | 19 ++++++++---- sanic/mixins/routes.py | 22 ++++++++------ sanic/request.py | 6 ++-- sanic/router.py | 17 +++++++++-- tests/test_blueprints.py | 2 +- tests/test_cookies.py | 2 ++ tests/test_routes.py | 65 ++++++++++++++++++++++++++++++++++++++-- 7 files changed, 111 insertions(+), 22 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index 115855b097..b4eb62f403 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -59,7 +59,6 @@ Signal, serve, serve_multiple, - trigger_events, ) from sanic.websocket import ConnectionClosed, WebSocketProtocol @@ -384,15 +383,15 @@ def url_for(self, view_name: str, **kwargs): if getattr(route.ctx, "static", None): filename = kwargs.pop("filename", "") # it's static folder - if "file_uri" in uri: - folder_ = uri.split(" Dict[str, str]: :return: Incoming cookies on the request :rtype: Dict[str, str] """ + if self._cookies is None: cookie = self.headers.get("Cookie") if cookie is not None: diff --git a/sanic/router.py b/sanic/router.py index 7c06079ef1..e5535ee6eb 100644 --- a/sanic/router.py +++ b/sanic/router.py @@ -9,12 +9,13 @@ from sanic_routing.route import Route # type: ignore from sanic.constants import HTTP_METHODS -from sanic.exceptions import MethodNotSupported, NotFound +from sanic.exceptions import MethodNotSupported, NotFound, SanicException from sanic.handlers import RouteHandler from sanic.request import Request ROUTER_CACHE_SIZE = 1024 +ALLOWED_LABELS = ("__file_uri__",) class Router(BaseRouter): @@ -33,7 +34,7 @@ class Router(BaseRouter): @lru_cache(maxsize=ROUTER_CACHE_SIZE) def _get( self, path, method, host - ) -> Tuple[RouteHandler, Dict[str, Any], str, str, bool,]: + ) -> Tuple[RouteHandler, Dict[str, Any], str, str, bool]: try: route, handler, params = self.resolve( path=path, @@ -204,3 +205,15 @@ def routes_dynamic(self): @property def routes_regex(self): return self.regex_routes + + def finalize(self, *args, **kwargs): + super().finalize(*args, **kwargs) + + for route in self.dynamic_routes.values(): + if any( + label.startswith("__") and label not in ALLOWED_LABELS + for label in route.labels + ): + raise SanicException( + f"Invalid route: {route}. Parameter names cannot use '__'." + ) diff --git a/tests/test_blueprints.py b/tests/test_blueprints.py index 88055b578a..d7bf231cd3 100644 --- a/tests/test_blueprints.py +++ b/tests/test_blueprints.py @@ -752,7 +752,7 @@ def test_static_blueprint_name(static_file_directory, file_name): app.blueprint(bp) uri = app.url_for("static", name="static.testing") - assert uri == "/static/test.file" + assert uri == "/static/test.file/" _, response = app.test_client.get("/static/test.file") assert response.status == 404 diff --git a/tests/test_cookies.py b/tests/test_cookies.py index 864fbb631a..1d74443c11 100644 --- a/tests/test_cookies.py +++ b/tests/test_cookies.py @@ -15,6 +15,8 @@ def test_cookies(app): @app.route("/") def handler(request): + print(request.headers) + print(request.cookies) cookie_value = request.cookies["test"] response = text(f"Cookies are: {cookie_value}") response.cookies["right_back"] = "at you" diff --git a/tests/test_routes.py b/tests/test_routes.py index f347740019..77970278f4 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -1,15 +1,20 @@ import asyncio +import re from unittest.mock import Mock import pytest -from sanic_routing.exceptions import ParameterNameConflicts, RouteExists +from sanic_routing.exceptions import ( + InvalidUsage, + ParameterNameConflicts, + RouteExists, +) from sanic_testing.testing import SanicTestClient from sanic import Blueprint, Sanic from sanic.constants import HTTP_METHODS -from sanic.exceptions import NotFound +from sanic.exceptions import NotFound, SanicException from sanic.request import Request from sanic.response import json, text @@ -1113,3 +1118,59 @@ def handler(request): assert str(excinfo.value) == ( "Expected either string or Iterable of " "host strings, not {!r}" ).format(host) + + +def test_route_with_regex_group(app): + @app.route("/path/to/") + async def handler(request, ext): + return text(ext) + + _, response = app.test_client.get("/path/to/file.txt") + assert response.text == "txt" + + +def test_route_with_regex_named_group(app): + @app.route(r"/path/to/txt)>") + async def handler(request, ext): + return text(ext) + + _, response = app.test_client.get("/path/to/file.txt") + assert response.text == "txt" + + +def test_route_with_regex_named_group_invalid(app): + @app.route(r"/path/to/txt)>") + async def handler(request, ext): + return text(ext) + + with pytest.raises(InvalidUsage) as e: + app.router.finalize() + + assert e.match( + re.escape("Named group (wrong) must match your named parameter (ext)") + ) + + +def test_route_with_regex_group_ambiguous(app): + @app.route("/path/to/") + async def handler(request, ext): + return text(ext) + + with pytest.raises(InvalidUsage) as e: + app.router.finalize() + + assert e.match( + re.escape( + "Could not compile pattern file(?:\.)(txt). Try using a named " + "group instead: '(?Pyour_matching_group)'" + ) + ) + + +def test_route_with_bad_named_param(app): + @app.route("/foo/<__bar__>") + async def handler(request): + return text("...") + + with pytest.raises(SanicException): + app.router.finalize() From cda31752f50f37a0d139330371d84b4fd39fe838 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Wed, 17 Feb 2021 15:43:00 +0200 Subject: [PATCH 3/4] Cover additional edge cases --- sanic/app.py | 23 +++++++------- sanic/mixins/routes.py | 2 +- sanic/request.py | 4 +++ sanic/router.py | 30 ++++--------------- .../test_route_resolution_benchmark.py | 4 +-- tests/test_app.py | 4 +-- tests/test_request.py | 9 ++++++ tests/test_request_stream.py | 1 + tests/test_url_building.py | 20 +++++++++++++ 9 files changed, 57 insertions(+), 40 deletions(-) diff --git a/sanic/app.py b/sanic/app.py index b4eb62f403..6b6b1e3638 100644 --- a/sanic/app.py +++ b/sanic/app.py @@ -568,25 +568,27 @@ async def handle_request(self, request: Request): # Define `response` var here to remove warnings about # allocation before assignment below. response = None - name = None try: # Fetch handler from router ( + route, handler, kwargs, - uri, - name, - ignore_body, ) = self.router.get(request) - request.name = name + request._match_info = kwargs + request.route = route + request.name = route.name + request.uri_template = f"/{route.path}" + request.endpoint = request.name if ( request.stream and request.stream.request_body - and not ignore_body + and not route.ctx.ignore_body ): - if self.router.is_stream_handler(request): + + if hasattr(handler, "is_stream"): # Streaming handler: lift the size limit request.stream.request_max_size = float("inf") else: @@ -597,15 +599,15 @@ async def handle_request(self, request: Request): # Request Middleware # -------------------------------------------- # response = await self._run_request_middleware( - request, request_name=name + request, request_name=route.name ) + # No middleware results if not response: # -------------------------------------------- # # Execute Handler # -------------------------------------------- # - request.uri_template = f"/{uri}" if handler is None: raise ServerError( ( @@ -614,12 +616,11 @@ async def handle_request(self, request: Request): ) ) - request.endpoint = request.name - # Run response handler response = handler(request, **kwargs) if isawaitable(response): response = await response + if response: response = await request.respond(response) else: diff --git a/sanic/mixins/routes.py b/sanic/mixins/routes.py index ef22339fb0..ba7a040405 100644 --- a/sanic/mixins/routes.py +++ b/sanic/mixins/routes.py @@ -611,7 +611,7 @@ def _generate_name(self, *objects) -> str: else: break - if not name: # noq + if not name: # noqa raise ValueError("Could not generate a name for handler") if not name.startswith(f"{self.name}."): diff --git a/sanic/request.py b/sanic/request.py index e48340721a..8af7a8656a 100644 --- a/sanic/request.py +++ b/sanic/request.py @@ -12,6 +12,8 @@ Union, ) +from sanic_routing.route import Route + if TYPE_CHECKING: from sanic.server import ConnInfo @@ -104,6 +106,7 @@ class Request: "parsed_forwarded", "raw_url", "request_middleware_started", + "route", "stream", "transport", "uri_template", @@ -151,6 +154,7 @@ def __init__( self._match_info = {} self.stream: Optional[Http] = None self.endpoint: Optional[str] = None + self.route: Optional[Route] = None def __repr__(self): class_name = self.__class__.__name__ diff --git a/sanic/router.py b/sanic/router.py index e5535ee6eb..c7b4ebe243 100644 --- a/sanic/router.py +++ b/sanic/router.py @@ -34,7 +34,7 @@ class Router(BaseRouter): @lru_cache(maxsize=ROUTER_CACHE_SIZE) def _get( self, path, method, host - ) -> Tuple[RouteHandler, Dict[str, Any], str, str, bool]: + ) -> Tuple[Route, RouteHandler, Dict[str, Any]]: try: route, handler, params = self.resolve( path=path, @@ -51,14 +51,14 @@ def _get( ) return ( + route, handler, params, - route.path, - route.name, - route.ctx.ignore_body, ) - def get(self, request: Request): + def get( + self, request: Request + ) -> Tuple[Route, RouteHandler, Dict[str, Any]]: """ Retrieve a `Route` object containg the details about how to handle a response for a given request @@ -67,8 +67,7 @@ def get(self, request: Request): :type request: Request :return: details needed for handling the request and returning the correct response - :rtype: Tuple[ RouteHandler, Tuple[Any, ...], Dict[str, Any], str, str, - Optional[str], bool, ] + :rtype: Tuple[ Route, RouteHandler, Dict[str, Any]] """ return self._get( request.path, request.method, request.headers.get("host") @@ -151,23 +150,6 @@ def add( return routes[0] return routes - def is_stream_handler(self, request) -> bool: - """ - Handler for request is stream or not. - - :param request: Request object - :return: bool - """ - try: - handler = self.get(request)[0] - except (NotFound, MethodNotSupported): - return False - if hasattr(handler, "view_class") and hasattr( - handler.view_class, request.method.lower() - ): - handler = getattr(handler.view_class, request.method.lower()) - return hasattr(handler, "is_stream") - @lru_cache(maxsize=ROUTER_CACHE_SIZE) def find_route_by_view_name(self, view_name, name=None): """ diff --git a/tests/benchmark/test_route_resolution_benchmark.py b/tests/benchmark/test_route_resolution_benchmark.py index 467254a4b9..7d857bb3e0 100644 --- a/tests/benchmark/test_route_resolution_benchmark.py +++ b/tests/benchmark/test_route_resolution_benchmark.py @@ -39,7 +39,7 @@ async def test_resolve_route_no_arg_string_path( iterations=1000, rounds=1000, ) - assert await result[0](None) == 1 + assert await result[1](None) == 1 @mark.asyncio async def test_resolve_route_with_typed_args( @@ -72,4 +72,4 @@ async def test_resolve_route_with_typed_args( iterations=1000, rounds=1000, ) - assert await result[0](None) == 1 + assert await result[1](None) == 1 diff --git a/tests/test_app.py b/tests/test_app.py index f82eb5da42..b443cd40d6 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -4,7 +4,7 @@ from inspect import isawaitable from os import environ -from unittest.mock import patch +from unittest.mock import Mock, patch import pytest @@ -118,7 +118,7 @@ async def handler(): def test_app_handle_request_handler_is_none(app, monkeypatch): def mockreturn(*args, **kwargs): - return None, {}, "", "", False + return Mock(), None, {} # Not sure how to make app.router.get() return None, so use mock here. monkeypatch.setattr(app.router, "get", mockreturn) diff --git a/tests/test_request.py b/tests/test_request.py index 2ac35efe7d..43d963867f 100644 --- a/tests/test_request.py +++ b/tests/test_request.py @@ -74,3 +74,12 @@ async def get(request): "/", headers={"SOME-OTHER-REQUEST-ID": f"{REQUEST_ID}"} ) assert request.id == REQUEST_ID * 2 + + +def test_route_assigned_to_request(app): + @app.get("/") + async def get(request): + return response.empty() + + request, _ = app.test_client.get("/") + assert request.route is list(app.router.routes.values())[0] diff --git a/tests/test_request_stream.py b/tests/test_request_stream.py index 0261bc983e..e478e1f56e 100644 --- a/tests/test_request_stream.py +++ b/tests/test_request_stream.py @@ -59,6 +59,7 @@ async def post(self, request): result = "" while True: body = await request.stream.read() + print(f"{body=}") if body is None: break result += body.decode("utf-8") diff --git a/tests/test_url_building.py b/tests/test_url_building.py index 5d9fcf411b..2898dbd9fc 100644 --- a/tests/test_url_building.py +++ b/tests/test_url_building.py @@ -344,3 +344,23 @@ def test_methodview_naming(methodview_app): assert viewone_url == "/view_one" assert viewtwo_url == "/view_two" + + +@pytest.mark.parametrize( + "path,version,expected", + ( + ("/foo", 1, "/v1/foo"), + ("/foo", 1.1, "/v1.1/foo"), + ("/foo", "1", "/v1/foo"), + ("/foo", "1.1", "/v1.1/foo"), + ("/foo", "1.0.1", "/v1.0.1/foo"), + ("/foo", "v1.0.1", "/v1.0.1/foo"), + ), +) +def test_versioning(app, path, version, expected): + @app.route(path, version=version) + def handler(*_): + ... + + url = app.url_for("handler") + assert url == expected From 7039cfc40fefbaffad8cbc47aa98d16c26bcf4bc Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Sun, 28 Feb 2021 20:35:45 +0200 Subject: [PATCH 4/4] cleanup tests --- tests/test_cookies.py | 2 -- tests/test_exceptions_handler.py | 1 - tests/test_reloader.py | 1 - tests/test_request_stream.py | 2 -- tests/test_routes.py | 1 - 5 files changed, 7 deletions(-) diff --git a/tests/test_cookies.py b/tests/test_cookies.py index 1d74443c11..864fbb631a 100644 --- a/tests/test_cookies.py +++ b/tests/test_cookies.py @@ -15,8 +15,6 @@ def test_cookies(app): @app.route("/") def handler(request): - print(request.headers) - print(request.cookies) cookie_value = request.cookies["test"] response = text(f"Cookies are: {cookie_value}") response.cookies["right_back"] = "at you" diff --git a/tests/test_exceptions_handler.py b/tests/test_exceptions_handler.py index 9c7241829a..e6fd42eb4f 100644 --- a/tests/test_exceptions_handler.py +++ b/tests/test_exceptions_handler.py @@ -126,7 +126,6 @@ def test_html_traceback_output_in_debug_mode(): assert response.status == 500 soup = BeautifulSoup(response.body, "html.parser") html = str(soup) - print(html) assert "response = handler(request, **kwargs)" in html assert "handler_4" in html diff --git a/tests/test_reloader.py b/tests/test_reloader.py index d2e5ff6b73..90b29343c0 100644 --- a/tests/test_reloader.py +++ b/tests/test_reloader.py @@ -59,7 +59,6 @@ def complete(*args): def scanner(proc): for line in proc.stdout: line = line.decode().strip() - print(">", line) if line.startswith("complete"): yield line diff --git a/tests/test_request_stream.py b/tests/test_request_stream.py index e478e1f56e..e39524c623 100644 --- a/tests/test_request_stream.py +++ b/tests/test_request_stream.py @@ -59,7 +59,6 @@ async def post(self, request): result = "" while True: body = await request.stream.read() - print(f"{body=}") if body is None: break result += body.decode("utf-8") @@ -647,7 +646,6 @@ async def read_chunk(): data = await reader.read(4096) assert data buffer += data - print(res) assert buffer[size : size + 2] == b"\r\n" ret, buffer = buffer[:size], buffer[size + 2 :] return ret diff --git a/tests/test_routes.py b/tests/test_routes.py index 77970278f4..08a5e92cdf 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -194,7 +194,6 @@ def handler(request): return text("OK") else: - print(func) raise Exception(f"Method: {method} is not callable") client_method = getattr(app.test_client, method)