From b5a3069a25c83965edab7dfc1cc1a94d7ba25520 Mon Sep 17 00:00:00 2001 From: thepetk Date: Fri, 7 Aug 2026 15:49:58 +0100 Subject: [PATCH] fix REST API metrics middleware route discovery and root_path handling --- src/app/main.py | 11 +++-- tests/unit/app/test_main_middleware.py | 68 ++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/app/main.py b/src/app/main.py index b1a2f90a7..378b5589f 100644 --- a/src/app/main.py +++ b/src/app/main.py @@ -10,7 +10,8 @@ from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse from ogx_client import APIConnectionError, AsyncOgxClient -from starlette.routing import Mount, Route, WebSocketRoute +from fastapi.routing import iter_route_contexts + from starlette.types import ASGIApp, Message, Receive, Scope, Send import version @@ -212,7 +213,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # requests with the full prefixed path (/api/lightspeed/v1/infer) but # app_routes_paths contains only application-level paths (/v1/infer). # Strip the prefix so the path check and metric labels match the routes. - root_path = scope.get("root_path", "") + root_path: str = app.root_path path: str = scope["path"] if root_path and path.startswith(root_path + "/"): path = path[len(root_path) :] @@ -292,9 +293,9 @@ async def send_wrapper(message: Message) -> None: routers.include_routers(app) app_routes_paths = [ - route.path - for route in app.routes - if isinstance(route, (Mount, Route, WebSocketRoute)) + rc.original_route.path + for rc in iter_route_contexts(app.routes) + if hasattr(rc.original_route, "path") and rc.original_route.path ] # Register pure ASGI middlewares. Middleware execution order is the reverse of diff --git a/tests/unit/app/test_main_middleware.py b/tests/unit/app/test_main_middleware.py index f0b76885a..bd364a7a1 100644 --- a/tests/unit/app/test_main_middleware.py +++ b/tests/unit/app/test_main_middleware.py @@ -9,7 +9,14 @@ from pytest_mock import MockerFixture from starlette.types import Message, Receive, Scope, Send -from app.main import GlobalExceptionMiddleware, RestApiMetricsMiddleware +from app.main import ( + GlobalExceptionMiddleware, + RestApiMetricsMiddleware, + app_routes_paths, +) +from app.main import ( + app as fastapi_app, +) from models.api.responses.error import InternalServerErrorResponse @@ -189,6 +196,7 @@ async def test_rest_api_metrics_strips_root_path( ) -> None: """Middleware must strip root_path so prefixed requests still match routes.""" mocker.patch("app.main.app_routes_paths", ["/v1/infer"]) + mocker.patch.object(fastapi_app, "root_path", "/api/lightspeed") mock_measure_duration = mocker.patch( "app.main.recording.measure_response_duration", return_value=nullcontext() ) @@ -201,9 +209,9 @@ async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None: middleware = RestApiMetricsMiddleware(ok_app) collector = _ResponseCollector() - # Simulate 3scale forwarding /api/lightspeed/v1/infer with root_path set. + # Simulate 3scale forwarding /api/lightspeed/v1/infer — scope carries no root_path. await middleware( - _make_scope("/api/lightspeed/v1/infer", root_path="/api/lightspeed"), + _make_scope("/api/lightspeed/v1/infer"), _noop_receive, collector, ) @@ -241,3 +249,57 @@ async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None: assert collector.status_code == 200 mock_measure_duration.assert_called_once_with("/v1/infer") mock_record_call.assert_called_once_with("/v1/infer", 200) + + +@pytest.mark.asyncio +async def test_rest_api_metrics_uses_app_root_path_not_scope( + mocker: MockerFixture, +) -> None: + """Middleware must read root_path from app.root_path, not scope["root_path"]. + + The scope carries an empty root_path while app.root_path holds the real prefix. + If the middleware reads from the scope it will not strip the prefix, the path + will not match any route, and no metric will be recorded — causing both + mock_measure_duration and mock_record_call assertions to fail. + """ + mocker.patch("app.main.app_routes_paths", ["/v1/infer"]) + mocker.patch.object(fastapi_app, "root_path", "/api/lightspeed") + mock_measure_duration = mocker.patch( + "app.main.recording.measure_response_duration", return_value=nullcontext() + ) + mock_record_call = mocker.patch("app.main.recording.record_rest_api_call") + + async def ok_app(_scope: Scope, _receive: Receive, send: Send) -> None: + await send({"type": "http.response.start", "status": 200, "headers": []}) + await send({"type": "http.response.body", "body": b"ok"}) + + middleware = RestApiMetricsMiddleware(ok_app) + collector = _ResponseCollector() + + # scope["root_path"] is explicitly empty while app.root_path is "/api/lightspeed". + # The middleware must use app.root_path to strip the prefix correctly. + scope = _make_scope("/api/lightspeed/v1/infer") + scope["root_path"] = "" + await middleware(scope, _noop_receive, collector) + + assert collector.status_code == 200 + mock_measure_duration.assert_called_once_with("/v1/infer") + mock_record_call.assert_called_once_with("/v1/infer", 200) + + +# --------------------------------------------------------------------------- +# app_routes_paths population +# --------------------------------------------------------------------------- + + +def test_app_routes_paths_contains_application_routes() -> None: + """app_routes_paths must include routes registered via include_router. + + FastAPI >= 0.137 stores included routers as _IncludedRouter objects that + the old isinstance(route, (Mount, Route, WebSocketRoute)) filter silently + drops. iter_route_contexts() resolves them correctly. If this test fails + with only 4 entries (the FastAPI built-ins), the fix has been reverted. + """ + assert "/liveness" in app_routes_paths + assert "/readiness" in app_routes_paths + assert len(app_routes_paths) > 4