Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Mount(..., middleware=[...]) #1649

Merged
merged 34 commits into from Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
42548cf
Add Mount(..., middleware=[...])
adriangb May 24, 2022
01bbcdc
fmt
adriangb May 24, 2022
d8b626d
add missing test
adriangb May 24, 2022
73dc39e
fmt
adriangb May 24, 2022
14d3005
move docs
adriangb May 24, 2022
8eb2699
Merge branch 'master' into mount-middleware
adriangb May 24, 2022
baab334
Merge branch 'master' into mount-middleware
adriangb May 27, 2022
bbca389
Merge remote-tracking branch 'upstream/master' into mount-middleware
adriangb May 27, 2022
a75a523
replace basehttpmiddleware
adriangb May 27, 2022
578f618
Merge branch 'mount-middleware' of https://github.com/adriangb/starle…
adriangb May 27, 2022
ba59a35
Merge branch 'master' into mount-middleware
adriangb May 30, 2022
3cadfb2
Merge branch 'master' into mount-middleware
adriangb Jun 2, 2022
1eace2b
Merge branch 'master' into mount-middleware
adriangb Jun 8, 2022
ca50340
Merge branch 'master' into mount-middleware
adriangb Jun 15, 2022
f65dfc8
lint
adriangb Jun 15, 2022
5edb100
Merge branch 'master' into mount-middleware
adriangb Jul 2, 2022
1ef66e6
Merge branch 'master' into mount-middleware
adriangb Jul 12, 2022
fb93ef5
Merge branch 'master' into mount-middleware
adriangb Jul 20, 2022
273cc73
Merge remote-tracking branch 'upstream/master' into mount-middleware
adriangb Aug 13, 2022
4369ee7
add comment to docs
adriangb Aug 13, 2022
10f47ae
save
adriangb Aug 13, 2022
feeba5e
Merge branch 'master' into mount-middleware
Kludex Aug 18, 2022
0bb54e4
Merge branch 'master' into mount-middleware
adriangb Aug 31, 2022
d7c3f2a
Update docs/middleware.md
adriangb Sep 1, 2022
e6fad81
Update docs/middleware.md
adriangb Sep 1, 2022
adb52ab
Merge branch 'master' into mount-middleware
adriangb Sep 1, 2022
0f7a2c4
Merge branch 'master' into mount-middleware
Kludex Sep 3, 2022
f6de20f
Apply suggestions from code review
Kludex Sep 3, 2022
72fbaa6
fix duplicate parametrization
adriangb Sep 3, 2022
eee6a6f
fix bad test for url_path_for
adriangb Sep 3, 2022
aec580f
Add test explaining behavior
adriangb Sep 3, 2022
93ec37f
fix linting
adriangb Sep 6, 2022
5f936e9
Merge branch 'master' into mount-middleware
adriangb Sep 6, 2022
0079756
Merge branch 'master' into mount-middleware
abersheeran Sep 17, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions docs/middleware.md
Expand Up @@ -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:
florimondmanca marked this conversation as resolved.
Show resolved Hide resolved

```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),
],
)
```
florimondmanca marked this conversation as resolved.
Show resolved Hide resolved

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
Expand Down
13 changes: 10 additions & 3 deletions starlette/routing.py
Expand Up @@ -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
Expand Down Expand Up @@ -334,24 +335,30 @@ 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 (
app is not None or routes is not None
), "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}"
)

@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"):
Expand Down
104 changes: 103 additions & 1 deletion tests/test_routing.py
Expand Up @@ -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


Expand Down Expand Up @@ -746,3 +758,93 @@ 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):
florimondmanca marked this conversation as resolved.
Show resolved Hide resolved
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,
Kludex marked this conversation as resolved.
Show resolved Hide resolved
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also assert that calling a route not under the Mount does not apply the middleware? E.g. testing a test_client.get("/").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in eee6a6f

assert response.status_code == 200
assert response.headers["X-Test"] == "Set by middleware"


@pytest.mark.parametrize(
"route",
[
mounted_routes_with_middleware,
mounted_routes_with_middleware,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated as well, so I think we can get rid of the @parametrize.

Copy link
Member Author

@adriangb adriangb Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in eee6a6f

],
)
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