From 42548cf9a51caa0d57102faa2547b7dedd516841 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 24 May 2022 06:20:23 -0500 Subject: [PATCH 01/16] Add Mount(..., middleware=[...]) --- docs/middleware.md | 31 +++++++++++++++ starlette/routing.py | 13 +++++-- tests/test_routing.py | 87 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 127 insertions(+), 4 deletions(-) diff --git a/docs/middleware.md b/docs/middleware.md index 817f63662..10543595b 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -41,6 +41,37 @@ application would look like this: * Routing * Endpoint +Middleware can also be added to `Mount`, which allows you to apply middleware to a single route, a group of routes or any mounted ASGI application: + +```python +from starlette.applications import Starlette +from starlette.middleware.gzip import GzipMiddleware + + +routes = [ + Mount( + "/", + routes=[ + Route( + "/example", + endpoint=..., + ) + ], + middleware=[GzipMiddleware] + ) +] + +app = Starlette( + routes=routes, + middleware=[ + Middleware(HTTPSRedirectMiddleware), + ], +) +``` + +Note that since this is run after routing, modifying the path in the middleware will have no effect. +There is also no built-in error handling for route middleware, so your middleware will need to handle exceptions and resource cleanup itself. + The following middleware implementations are available in the Starlette package: ## CORSMiddleware diff --git a/starlette/routing.py b/starlette/routing.py index 67f12e311..3ea56ec5f 100644 --- a/starlette/routing.py +++ b/starlette/routing.py @@ -14,6 +14,7 @@ from starlette.convertors import CONVERTOR_TYPES, Convertor from starlette.datastructures import URL, Headers, URLPath from starlette.exceptions import HTTPException +from starlette.middleware import Middleware from starlette.requests import Request from starlette.responses import PlainTextResponse, RedirectResponse from starlette.types import ASGIApp, Receive, Scope, Send @@ -334,6 +335,8 @@ def __init__( app: typing.Optional[ASGIApp] = None, routes: typing.Optional[typing.Sequence[BaseRoute]] = None, name: typing.Optional[str] = None, + *, + middleware: typing.Optional[typing.Sequence[Middleware]] = None, ) -> None: assert path == "" or path.startswith("/"), "Routed paths must start with '/'" assert ( @@ -341,9 +344,13 @@ def __init__( ), "Either 'app=...', or 'routes=' must be specified" self.path = path.rstrip("/") if app is not None: - self.app: ASGIApp = app + self._base_app: ASGIApp = app else: - self.app = Router(routes=routes) + self._base_app = Router(routes=routes) + self.app = self._base_app + if middleware is not None: + for cls, options in reversed(middleware): + self.app = cls(app=self.app, **options) self.name = name self.path_regex, self.path_format, self.param_convertors = compile_path( self.path + "/{path:path}" @@ -351,7 +358,7 @@ def __init__( @property def routes(self) -> typing.List[BaseRoute]: - return getattr(self.app, "routes", []) + return getattr(self._base_app, "routes", []) def matches(self, scope: Scope) -> typing.Tuple[Match, Scope]: if scope["type"] in ("http", "websocket"): diff --git a/tests/test_routing.py b/tests/test_routing.py index e8adaca48..cf1638a31 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -5,8 +5,20 @@ import pytest from starlette.applications import Starlette +from starlette.middleware import Middleware +from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint +from starlette.requests import Request from starlette.responses import JSONResponse, PlainTextResponse, Response -from starlette.routing import Host, Mount, NoMatchFound, Route, Router, WebSocketRoute +from starlette.routing import ( + BaseRoute, + Host, + Mount, + NoMatchFound, + Route, + Router, + WebSocketRoute, + ) +from starlette.testclient import TestClient from starlette.websockets import WebSocket, WebSocketDisconnect @@ -746,3 +758,76 @@ def __call__(self, request): ) def test_route_name(endpoint: typing.Callable, expected_name: str): assert Route(path="/", endpoint=endpoint).name == expected_name + + +def assert_middleware_header_route(request: Request): + assert getattr(request.state, "middleware_touched") == "Set by middleware" + return Response() + + +class AddHeadersMiddleware(BaseHTTPMiddleware): + async def dispatch( + self, request: Request, call_next: RequestResponseEndpoint + ) -> Response: + setattr(request.state, "middleware_touched", "Set by middleware") + response: Response = await call_next(request) + response.headers["X-Test"] = "Set by middleware" + return response + + + +mounted_routes_with_middleware = Mount( + "/http", + routes=[ + Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + ], + middleware=[Middleware(AddHeadersMiddleware)], + ) + + +mounted_app_with_middleware = Mount( + "/http", + app=Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + middleware=[Middleware(AddHeadersMiddleware)], + ) + + +@pytest.mark.parametrize( + "route", + [ + mounted_routes_with_middleware, + mounted_routes_with_middleware, + ], +) +def test_mount_middleware_url_path_for(route: BaseRoute) -> None: + """Checks that url_path_for still works with middleware on Mounts""" + router = Router([route]) + assert router.url_path_for("route") == "/http/" + + +def test_add_route_to_app_after_mount( + test_client_factory: typing.Callable[..., TestClient], +) -> None: + """Checks that Mount will pick up routes + added to the underlying app after it is mounted + """ + inner_app = Router() + app = Mount("/http", app=inner_app) + inner_app.add_route( + "/inner", + endpoint=lambda request: Response(), + methods=["GET"], + ) + client = test_client_factory(app) + response = client.get("/http/inner") + assert response.status_code == 200 From 01bbcdc71cd7ea0ec38022b32c6615e63fac6bd7 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 24 May 2022 06:22:22 -0500 Subject: [PATCH 02/16] fmt --- tests/test_routing.py | 57 +++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index cf1638a31..8f4797c41 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -10,14 +10,14 @@ from starlette.requests import Request from starlette.responses import JSONResponse, PlainTextResponse, Response from starlette.routing import ( - BaseRoute, - Host, - Mount, - NoMatchFound, - Route, - Router, - WebSocketRoute, - ) + BaseRoute, + Host, + Mount, + NoMatchFound, + Route, + Router, + WebSocketRoute, +) from starlette.testclient import TestClient from starlette.websockets import WebSocket, WebSocketDisconnect @@ -775,31 +775,30 @@ async def dispatch( return response - mounted_routes_with_middleware = Mount( - "/http", - routes=[ - Route( - "/", - endpoint=assert_middleware_header_route, - methods=["GET"], - name="route", - ), - ], - middleware=[Middleware(AddHeadersMiddleware)], - ) + "/http", + routes=[ + Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + ], + middleware=[Middleware(AddHeadersMiddleware)], +) mounted_app_with_middleware = Mount( - "/http", - app=Route( - "/", - endpoint=assert_middleware_header_route, - methods=["GET"], - name="route", - ), - middleware=[Middleware(AddHeadersMiddleware)], - ) + "/http", + app=Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + middleware=[Middleware(AddHeadersMiddleware)], +) @pytest.mark.parametrize( From d8b626dfd0ffc0d70f7069c2768a53d79064c885 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 24 May 2022 06:43:50 -0500 Subject: [PATCH 03/16] add missing test --- tests/test_routing.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_routing.py b/tests/test_routing.py index 8f4797c41..3f9c13f55 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -801,6 +801,24 @@ async def dispatch( ) +@pytest.mark.parametrize( + "route", + [ + mounted_routes_with_middleware, + mounted_routes_with_middleware, + mounted_app_with_middleware, + ], +) +def test_mount_middleware( + test_client_factory: typing.Callable[..., TestClient], + route: BaseRoute, + ) -> None: + test_client = test_client_factory(Router([route])) + response = test_client.get("/http") + assert response.status_code == 200 + assert response.headers["X-Test"] == "Set by middleware" + + @pytest.mark.parametrize( "route", [ From 73dc39e95c51b7f183ef67d0371000168ebb4784 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 24 May 2022 06:44:54 -0500 Subject: [PATCH 04/16] fmt --- tests/test_routing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index 3f9c13f55..e3ca5097c 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -812,7 +812,7 @@ async def dispatch( def test_mount_middleware( test_client_factory: typing.Callable[..., TestClient], route: BaseRoute, - ) -> None: +) -> None: test_client = test_client_factory(Router([route])) response = test_client.get("/http") assert response.status_code == 200 From 14d300549a1f2cb71c7d789390a2685508267531 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 24 May 2022 10:37:15 -0700 Subject: [PATCH 05/16] move docs --- docs/middleware.md | 56 +++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/docs/middleware.md b/docs/middleware.md index 10543595b..522863526 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -41,37 +41,6 @@ application would look like this: * Routing * Endpoint -Middleware can also be added to `Mount`, which allows you to apply middleware to a single route, a group of routes or any mounted ASGI application: - -```python -from starlette.applications import Starlette -from starlette.middleware.gzip import GzipMiddleware - - -routes = [ - Mount( - "/", - routes=[ - Route( - "/example", - endpoint=..., - ) - ], - middleware=[GzipMiddleware] - ) -] - -app = Starlette( - routes=routes, - middleware=[ - Middleware(HTTPSRedirectMiddleware), - ], -) -``` - -Note that since this is run after routing, modifying the path in the middleware will have no effect. -There is also no built-in error handling for route middleware, so your middleware will need to handle exceptions and resource cleanup itself. - The following middleware implementations are available in the Starlette package: ## CORSMiddleware @@ -264,6 +233,31 @@ around explicitly, rather than mutating the middleware instance. - It's not possible to use `BackgroundTasks` with `BaseHTTPMiddleware`. - Using `BaseHTTPMiddleware` will prevent changes to `contextlib.ContextVar`s from propagating upwards. That is, if you set a value for a `ContextVar` in your endpoint and try to read it from a middleware you will find that the value is not the same value you set in your endpoint (see [this test](https://github.com/encode/starlette/blob/621abc747a6604825190b93467918a0ec6456a24/tests/middleware/test_base.py#L192-L223) for an example of this behavior). +## Applying middleware on mounts + +Middleware can also be added to `Mount`, which allows you to apply middleware to a single route, a group of routes or any mounted ASGI application: + +```python +from starlette.applications import Starlette +from starlette.middleware.gzip import GzipMiddleware + + +routes = [ + Mount( + "/", + routes=[ + Route( + "/example", + endpoint=..., + ) + ], + middleware=[GzipMiddleware] + ) +] + +app = Starlette(routes=routes) +``` + ## Using middleware in other frameworks To wrap ASGI middleware around other ASGI applications, you should use the From a75a5237fd41bdb44e30548c1e80484b13cd3f18 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 27 May 2022 08:04:09 -0700 Subject: [PATCH 06/16] replace basehttpmiddleware --- tests/test_routing.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index e3ca5097c..99c768194 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -6,7 +6,6 @@ from starlette.applications import Starlette from starlette.middleware import Middleware -from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint from starlette.requests import Request from starlette.responses import JSONResponse, PlainTextResponse, Response from starlette.routing import ( @@ -19,6 +18,7 @@ WebSocketRoute, ) from starlette.testclient import TestClient +from starlette.types import ASGIApp, Message, Receive, Scope, Send from starlette.websockets import WebSocket, WebSocketDisconnect @@ -760,19 +760,24 @@ def test_route_name(endpoint: typing.Callable, expected_name: str): assert Route(path="/", endpoint=endpoint).name == expected_name -def assert_middleware_header_route(request: Request): - assert getattr(request.state, "middleware_touched") == "Set by middleware" - return Response() +class AddHeadersMiddleware: + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + scope["add_headers_middleware"] = True + + async def modified_send(msg: Message) -> None: + if msg["type"] == "http.response.start": + msg["headers"].append((b"X-Test", b"Set by middleware")) + await send(msg) + await self.app(scope, receive, modified_send) -class AddHeadersMiddleware(BaseHTTPMiddleware): - async def dispatch( - self, request: Request, call_next: RequestResponseEndpoint - ) -> Response: - setattr(request.state, "middleware_touched", "Set by middleware") - response: Response = await call_next(request) - response.headers["X-Test"] = "Set by middleware" - return response + +def assert_middleware_header_route(request: Request) -> Response: + assert request.scope["add_headers_middleware"] is True + return Response() mounted_routes_with_middleware = Mount( From f65dfc80fca65b1fce1e49ec99e933e7a02d51e7 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 15 Jun 2022 13:41:21 -0500 Subject: [PATCH 07/16] lint --- tests/test_routing.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_routing.py b/tests/test_routing.py index 2019399c9..c105bf663 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -874,6 +874,7 @@ def test_add_route_to_app_after_mount( response = client.get("/http/inner") assert response.status_code == 200 + def test_exception_on_mounted_apps(test_client_factory): def exc(request): raise Exception("Exc") From 4369ee736ee9cb608db14f5066946d9cfcbf647b Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sat, 13 Aug 2022 10:17:38 -0500 Subject: [PATCH 08/16] add comment to docs --- docs/middleware.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/middleware.md b/docs/middleware.md index 803f70518..7c5980a78 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -708,6 +708,14 @@ routes = [ app = Starlette(routes=routes) ``` +Note that middleware used in this way is *not* wrapped in exception handling middleware like the middleware applied to the `Starlette` application is. +This is often not a problem because it only applies to middleware that inspect or modify the `Response`, and even then you probably don't want to apply this logic to error responses. +If you do want to apply the middelware logic to error responses only on some routes you have a couple of options: + +* Add an `ExceptionMiddleware` onto the `Mount` +* Add a `try/except` block to your middleware and return an error response from there +* Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to `Starlette` that does the heavy lifting. + ## Third party middleware #### [asgi-auth-github](https://github.com/simonw/asgi-auth-github) From 10f47ae948bf69e4221c2560786b8fa8618b8971 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sat, 13 Aug 2022 15:31:55 -0500 Subject: [PATCH 09/16] save --- docs/middleware.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/middleware.md b/docs/middleware.md index 7c5980a78..26cad7124 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -714,7 +714,7 @@ If you do want to apply the middelware logic to error responses only on some rou * Add an `ExceptionMiddleware` onto the `Mount` * Add a `try/except` block to your middleware and return an error response from there -* Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to `Starlette` that does the heavy lifting. +* Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to the `Starlette` application that does the heavy lifting. ## Third party middleware From d7c3f2a89c81c241c075f77aea8d5c4d6d5da609 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 1 Sep 2022 00:39:14 -0500 Subject: [PATCH 10/16] Update docs/middleware.md Co-authored-by: Marcelo Trylesinski --- docs/middleware.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/middleware.md b/docs/middleware.md index 26cad7124..048d6f376 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -689,7 +689,9 @@ Middleware can also be added to `Mount`, which allows you to apply middleware to ```python from starlette.applications import Starlette -from starlette.middleware.gzip import GzipMiddleware +from starlette.middleware import Middleware +from starlette.middleware.gzip import GZipMiddleware +from starlette.routing import Mount, Route routes = [ From e6fad81470aec876ca45a8689087a5ccec14ec47 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 1 Sep 2022 00:39:20 -0500 Subject: [PATCH 11/16] Update docs/middleware.md Co-authored-by: Marcelo Trylesinski --- docs/middleware.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/middleware.md b/docs/middleware.md index 048d6f376..6dde29065 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -703,7 +703,7 @@ routes = [ endpoint=..., ) ], - middleware=[GzipMiddleware] + middleware=[Middleware(GZipMiddleware)] ) ] From f6de20f0cf2faf4381b69998c96859974ef7e4d1 Mon Sep 17 00:00:00 2001 From: Marcelo Trylesinski Date: Sat, 3 Sep 2022 14:11:04 +0200 Subject: [PATCH 12/16] Apply suggestions from code review Co-authored-by: Florimond Manca --- docs/middleware.md | 4 ++-- tests/test_routing.py | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/middleware.md b/docs/middleware.md index 6dde29065..3300b574a 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -712,11 +712,11 @@ app = Starlette(routes=routes) Note that middleware used in this way is *not* wrapped in exception handling middleware like the middleware applied to the `Starlette` application is. This is often not a problem because it only applies to middleware that inspect or modify the `Response`, and even then you probably don't want to apply this logic to error responses. -If you do want to apply the middelware logic to error responses only on some routes you have a couple of options: +If you do want to apply the middleware logic to error responses only on some routes you have a couple of options: * Add an `ExceptionMiddleware` onto the `Mount` * Add a `try/except` block to your middleware and return an error response from there -* Split up marking and processing into two middlewares, one that gets put on `Mount` which simply marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to the `Starlette` application that does the heavy lifting. +* Split up marking and processing into two middlewares, one that gets put on `Mount` which marks the response as needing processing (for example by setting `scope["log-response"] = True`) and another applied to the `Starlette` application that does the heavy lifting. ## Third party middleware diff --git a/tests/test_routing.py b/tests/test_routing.py index c105bf663..1b316bed8 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -829,7 +829,6 @@ def assert_middleware_header_route(request: Request) -> Response: @pytest.mark.parametrize( "route", [ - mounted_routes_with_middleware, mounted_routes_with_middleware, mounted_app_with_middleware, ], From 72fbaa650d6f0473d75cd80bfd149f60c32fe102 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sat, 3 Sep 2022 12:17:10 -0500 Subject: [PATCH 13/16] fix duplicate parametrization --- tests/test_routing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index 1b316bed8..b6cf38458 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -847,12 +847,12 @@ def test_mount_middleware( "route", [ mounted_routes_with_middleware, - mounted_routes_with_middleware, + mounted_app_with_middleware, ], ) def test_mount_middleware_url_path_for(route: BaseRoute) -> None: """Checks that url_path_for still works with middleware on Mounts""" - router = Router([route]) + router = Router([mounted_routes_with_middleware]) assert router.url_path_for("route") == "/http/" From eee6a6f0b3dedf415089d4c67fbe1c12bca99442 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sat, 3 Sep 2022 12:32:36 -0500 Subject: [PATCH 14/16] fix bad test for url_path_for --- tests/test_routing.py | 89 +++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 42 deletions(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index b6cf38458..c6ce08b4d 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -8,15 +8,7 @@ from starlette.middleware import Middleware from starlette.requests import Request from starlette.responses import JSONResponse, PlainTextResponse, Response -from starlette.routing import ( - BaseRoute, - Host, - Mount, - NoMatchFound, - Route, - Router, - WebSocketRoute, -) +from starlette.routing import Host, Mount, NoMatchFound, Route, Router, WebSocketRoute from starlette.testclient import TestClient from starlette.types import ASGIApp, Message, Receive, Scope, Send from starlette.websockets import WebSocket, WebSocketDisconnect @@ -800,34 +792,44 @@ def assert_middleware_header_route(request: Request) -> Response: return Response() -mounted_routes_with_middleware = Mount( - "/http", +mounted_routes_with_middleware = Starlette( routes=[ - Route( - "/", - endpoint=assert_middleware_header_route, - methods=["GET"], - name="route", + Mount( + "/http", + routes=[ + Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + ], + middleware=[Middleware(AddHeadersMiddleware)], ), - ], - middleware=[Middleware(AddHeadersMiddleware)], + Route("/home", homepage), + ] ) -mounted_app_with_middleware = Mount( - "/http", - app=Route( - "/", - endpoint=assert_middleware_header_route, - methods=["GET"], - name="route", - ), - middleware=[Middleware(AddHeadersMiddleware)], +mounted_app_with_middleware = Starlette( + routes=[ + Mount( + "/http", + app=Route( + "/", + endpoint=assert_middleware_header_route, + methods=["GET"], + name="route", + ), + middleware=[Middleware(AddHeadersMiddleware)], + ), + Route("/home", homepage), + ] ) @pytest.mark.parametrize( - "route", + "app", [ mounted_routes_with_middleware, mounted_app_with_middleware, @@ -835,25 +837,28 @@ def assert_middleware_header_route(request: Request) -> Response: ) def test_mount_middleware( test_client_factory: typing.Callable[..., TestClient], - route: BaseRoute, + app: Starlette, ) -> None: - test_client = test_client_factory(Router([route])) + test_client = test_client_factory(app) + + response = test_client.get("/home") + assert response.status_code == 200 + assert "X-Test" not in response.headers + response = test_client.get("/http") assert response.status_code == 200 assert response.headers["X-Test"] == "Set by middleware" -@pytest.mark.parametrize( - "route", - [ - mounted_routes_with_middleware, - mounted_app_with_middleware, - ], -) -def test_mount_middleware_url_path_for(route: BaseRoute) -> None: - """Checks that url_path_for still works with middleware on Mounts""" - router = Router([mounted_routes_with_middleware]) - assert router.url_path_for("route") == "/http/" +def test_mount_routes_with_middleware_url_path_for() -> None: + """Checks that url_path_for still works with mounted routes with Middleware""" + assert mounted_routes_with_middleware.url_path_for("route") == "/http/" + + +def test_mount_asgi_app_with_middleware_url_path_for() -> None: + """Mounted ASGI apps do not work with url path for, middleware does not change this""" + with pytest.raises(NoMatchFound): + mounted_app_with_middleware.url_path_for("route") def test_add_route_to_app_after_mount( @@ -866,7 +871,7 @@ def test_add_route_to_app_after_mount( app = Mount("/http", app=inner_app) inner_app.add_route( "/inner", - endpoint=lambda request: Response(), + endpoint=homepage, methods=["GET"], ) client = test_client_factory(app) From aec580f8491cd87fde4d02614d0e9893df07796e Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sat, 3 Sep 2022 12:44:42 -0500 Subject: [PATCH 15/16] Add test explaining behavior --- tests/test_routing.py | 60 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_routing.py b/tests/test_routing.py index c6ce08b4d..d2b59b487 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -5,6 +5,7 @@ import pytest from starlette.applications import Starlette +from starlette.exceptions import HTTPException from starlette.middleware import Middleware from starlette.requests import Request from starlette.responses import JSONResponse, PlainTextResponse, Response @@ -890,3 +891,62 @@ def exc(request): with pytest.raises(Exception) as ctx: client.get("/sub/") assert str(ctx.value) == "Exc" + + +def test_mounted_middleware_does_not_catch_exception( + test_client_factory: typing.Callable[..., TestClient], +) -> None: + # https://github.com/encode/starlette/pull/1649#discussion_r960236107 + def exc(request: Request) -> Response: + raise HTTPException(status_code=403, detail="auth") + + class NamedMiddleware: + def __init__(self, app: ASGIApp, name: str) -> None: + self.app = app + self.name = name + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + async def modified_send(msg: Message) -> None: + if msg["type"] == "http.response.start": + msg["headers"].append((f"X-{self.name}".encode(), b"true")) + await send(msg) + + await self.app(scope, receive, modified_send) + + app = Starlette( + routes=[ + Mount( + "/mount", + routes=[ + Route("/err", exc), + Route("/home", homepage), + ], + middleware=[Middleware(NamedMiddleware, name="Mounted")], + ), + Route("/err", exc), + Route("/home", homepage), + ], + middleware=[Middleware(NamedMiddleware, name="Outer")], + ) + + client = test_client_factory(app) + + resp = client.get("/home") + assert resp.status_code == 200, resp.content + assert "X-Outer" in resp.headers + + resp = client.get("/err") + assert resp.status_code == 403, resp.content + assert "X-Outer" in resp.headers + + resp = client.get("/mount/home") + assert resp.status_code == 200, resp.content + assert "X-Mounted" in resp.headers + + # this is the "surprising" behavior bit + # the middleware on the mount never runs because there + # is nothing to catch the HTTPException + # since Mount middlweare is not wrapped by ExceptionMiddleware + resp = client.get("/mount/err") + assert resp.status_code == 403, resp.content + assert "X-Mounted" not in resp.headers From 93ec37fe66dbfae5fe6cbadbaeed0cefd456bf24 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 6 Sep 2022 07:21:55 -0500 Subject: [PATCH 16/16] fix linting --- tests/test_routing.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_routing.py b/tests/test_routing.py index d2b59b487..750f32496 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -857,7 +857,9 @@ def test_mount_routes_with_middleware_url_path_for() -> None: def test_mount_asgi_app_with_middleware_url_path_for() -> None: - """Mounted ASGI apps do not work with url path for, middleware does not change this""" + """Mounted ASGI apps do not work with url path for, + middleware does not change this + """ with pytest.raises(NoMatchFound): mounted_app_with_middleware.url_path_for("route")