fix(starlette): Stop duplicating scope["root_path"] in URLs#6579
2 issues
code-review: Found 2 issues (2 medium)
Medium
Mount-based test_request_url tests can't detect the root_path duplication bug they target - `tests/integrations/starlette/test_starlette.py:1500-1525`
The starlette test_request_url (and the identical fastapi test_request_url at tests/integrations/fastapi/test_fastapi.py:1051, which is part of this PR) construct TestClient(app) without root_path="/root" and rely on a Mount("/root", app=mounted_app) to reach /root/nomessage. Because SentryAsgiMiddleware._run_app skips the inner mounted invocation via _asgi_middleware_applied, the scope used for URL construction is always the outer one where root_path="". With root_path="", _get_url produces "" + "/root/nomessage" == "/root/nomessage" under both path_includes_root_path=True and False, so these tests pass whether or not the fix is present and cannot detect the duplication regression. The litestar/starlite/quart tests reproduce the bug correctly by passing root_path="/root" to TestClient (no Mount). Consider aligning the starlette/fastapi tests with that pattern (e.g. drop the Mount and use TestClient(app, root_path="/root") requesting /nomessage) so they exercise a non-empty outer root_path.
Also found at:
sentry_sdk/integrations/asgi.py:117
test_request_url missing send_default_pii=True makes span_streaming branch KeyError on url.full - `tests/integrations/starlite/test_starlite.py:586-592`
Add send_default_pii=True to sentry_init here. _get_request_attributes in _asgi_common.py only sets url.full inside if should_send_default_pii():, so for span_streaming=True the server span attributes will not contain url.full. The assertion server_span["attributes"]["url.full"] will then raise a KeyError rather than meaningfully validating the URL. The equivalent Starlette test (test_starlette.py:1491) correctly sets send_default_pii=True.
⏱ 10m 53s · 3.5M in / 146.3k out · $5.07
Annotations
Check warning on line 1525 in tests/integrations/starlette/test_starlette.py
sentry-warden / warden: code-review
Mount-based test_request_url tests can't detect the root_path duplication bug they target
The starlette `test_request_url` (and the identical fastapi `test_request_url` at tests/integrations/fastapi/test_fastapi.py:1051, which is part of this PR) construct `TestClient(app)` without `root_path="/root"` and rely on a `Mount("/root", app=mounted_app)` to reach `/root/nomessage`. Because `SentryAsgiMiddleware._run_app` skips the inner mounted invocation via `_asgi_middleware_applied`, the scope used for URL construction is always the outer one where `root_path=""`. With `root_path=""`, `_get_url` produces `"" + "/root/nomessage" == "/root/nomessage"` under both `path_includes_root_path=True` and `False`, so these tests pass whether or not the fix is present and cannot detect the duplication regression. The litestar/starlite/quart tests reproduce the bug correctly by passing `root_path="/root"` to `TestClient` (no Mount). Consider aligning the starlette/fastapi tests with that pattern (e.g. drop the Mount and use `TestClient(app, root_path="/root")` requesting `/nomessage`) so they exercise a non-empty outer `root_path`.
Check warning on line 117 in sentry_sdk/integrations/asgi.py
sentry-warden / warden: code-review
[NV2-DS5] Mount-based test_request_url tests can't detect the root_path duplication bug they target (additional location)
The starlette `test_request_url` (and the identical fastapi `test_request_url` at tests/integrations/fastapi/test_fastapi.py:1051, which is part of this PR) construct `TestClient(app)` without `root_path="/root"` and rely on a `Mount("/root", app=mounted_app)` to reach `/root/nomessage`. Because `SentryAsgiMiddleware._run_app` skips the inner mounted invocation via `_asgi_middleware_applied`, the scope used for URL construction is always the outer one where `root_path=""`. With `root_path=""`, `_get_url` produces `"" + "/root/nomessage" == "/root/nomessage"` under both `path_includes_root_path=True` and `False`, so these tests pass whether or not the fix is present and cannot detect the duplication regression. The litestar/starlite/quart tests reproduce the bug correctly by passing `root_path="/root"` to `TestClient` (no Mount). Consider aligning the starlette/fastapi tests with that pattern (e.g. drop the Mount and use `TestClient(app, root_path="/root")` requesting `/nomessage`) so they exercise a non-empty outer `root_path`.
Check warning on line 592 in tests/integrations/starlite/test_starlite.py
sentry-warden / warden: code-review
test_request_url missing send_default_pii=True makes span_streaming branch KeyError on url.full
Add `send_default_pii=True` to `sentry_init` here. `_get_request_attributes` in `_asgi_common.py` only sets `url.full` inside `if should_send_default_pii():`, so for `span_streaming=True` the server span attributes will not contain `url.full`. The assertion `server_span["attributes"]["url.full"]` will then raise a `KeyError` rather than meaningfully validating the URL. The equivalent Starlette test (`test_starlette.py:1491`) correctly sets `send_default_pii=True`.