Skip to content

fix(starlette): Stop duplicating scope["root_path"] in URLs#6579

Merged
alexander-alderman-webb merged 22 commits into
masterfrom
webb/asgi/double-mount-prefix
Jun 26, 2026
Merged

fix(starlette): Stop duplicating scope["root_path"] in URLs#6579
alexander-alderman-webb merged 22 commits into
masterfrom
webb/asgi/double-mount-prefix

Conversation

@alexander-alderman-webb

@alexander-alderman-webb alexander-alderman-webb commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

Add the root_path_in_path parameter to _get_url() and related functions.

If root_path_in_path is _RootPathInPath.EXCLUDED, the existing behavior that prepends scope["root_path"] when forming the url is preserved.

The other _RootPathInPath.EITHER option is used by the Starlette integration. It preserves Starlette’s behavior of accepting ASGI scopes where scope["path"] either includes scope["root_path"] or is already relative to it, using heuristics that match the libraries logic.

Issues

Closes #6577

Reminders

Comment thread sentry_sdk/integrations/_asgi_common.py Outdated
@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Codecov Results 📊

89950 passed | ⏭️ 6240 skipped | Total: 96190 | Pass Rate: 93.51% | Execution Time: 321m 17s

📊 Comparison with Base Branch

Metric Change
Total Tests 📈 +45
Passed Tests 📈 +45
Failed Tests
Skipped Tests

All tests are passing successfully.

✅ Patch coverage is 96.43%. Project has 2405 uncovered lines.
❌ Project coverage is 89.9%. Comparing base (base) to head (head).

Files with missing lines (1)
File Patch % Lines
sentry_sdk/integrations/_asgi_common.py 93.75% ⚠️ 1 Missing and 1 partials
Coverage diff
@@            Coverage Diff             @@
##          main       #PR       +/-##
==========================================
- Coverage    89.93%    89.90%    -0.03%
==========================================
  Files          192       192         —
  Lines        23784     23801       +17
  Branches      8210      8214        +4
==========================================
+ Hits         21389     21396        +7
- Misses        2395      2405       +10
- Partials      1342      1345        +3

Generated by Codecov Action

Comment thread tests/integrations/starlette/test_starlette.py
Comment thread tests/integrations/starlite/test_starlite.py Outdated
Comment thread tests/integrations/litestar/test_litestar.py Outdated
Comment thread sentry_sdk/integrations/asgi.py Outdated
Comment thread tests/integrations/django/asgi/test_asgi.py Outdated
@alexander-alderman-webb alexander-alderman-webb changed the title fix(asgi): Stop duplicating root_path in URLs fix(asgi): Stop duplicating scope["root_path"] in URLs Jun 19, 2026
Comment thread sentry_sdk/integrations/quart.py Outdated
@alexander-alderman-webb alexander-alderman-webb marked this pull request as ready for review June 19, 2026 12:51
@alexander-alderman-webb alexander-alderman-webb requested a review from a team as a code owner June 19, 2026 12:51
Comment thread sentry_sdk/integrations/quart.py Outdated
Comment thread sentry_sdk/integrations/quart.py Outdated

@wahajahmed010 wahajahmed010 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: #6579

Summary: Clean fix for ASGI URL duplication when scope["root_path"] is already included in scope["path"]. The approach is sound — thread a path_includes_root_path boolean through the URL construction pipeline.

✅ What works well

  • Backward compatible — default is True, preserving existing behavior for frameworks that don't include root_path in path
  • Framework-specific handling — correctly version-gated for Starlette (>=0.33), Quart (>=0.19), and hardcoded for Litestar/Starlite
  • Test coverage — all 6 integrations tested (Django ASGI, FastAPI, Litestar, Quart, Starlette, Starlite), both span streaming and static lifecycle modes
  • Clean threading — parameter flows through _get_url_get_request_data_get_request_attributes_get_transaction_name_and_source_get_segment_name_and_source

⚠️ Potential issues

  1. Django ASGI test — The test sets comm.scope["root_path"] = "/root" but Django defaults to path_includes_root_path=True. Does Django ASGI actually include root_path in path? If not, this test might pass but produce incorrect URLs in production. Worth verifying Django's ASGI behavior.
  2. No test for path_includes_root_path=False — The Starlette/FastAPI tests only exercise the True path (Starlette >= 0.33). The old behavior (False) has no dedicated test.
  3. Edge case: empty path — When path_includes_root_path=True and scope["path"] is empty, the URL would be just scheme+host with no path. Unlikely but worth noting.

Verdict

Solid PR. The core logic is correct and well-structured. The main risk is the Django default assumption — worth double-checking before merging.

Comment thread sentry_sdk/integrations/asgi.py Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a96d284. Configure here.

Comment thread sentry_sdk/integrations/django/asgi.py Outdated
Comment thread sentry_sdk/integrations/asgi.py Outdated
@alexander-alderman-webb alexander-alderman-webb marked this pull request as draft June 23, 2026 08:32
Comment thread sentry_sdk/integrations/django/asgi.py Outdated
@alexander-alderman-webb alexander-alderman-webb changed the title fix(asgi): Stop duplicating scope["root_path"] in URLs fix(starlette/fastapi): Stop duplicating scope["root_path"] in URLs Jun 26, 2026
@alexander-alderman-webb alexander-alderman-webb changed the title fix(starlette/fastapi): Stop duplicating scope["root_path"] in URLs fix(starlette): Stop duplicating scope["root_path"] in URLs Jun 26, 2026

@sentry-warden sentry-warden Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

url.path attribute always prepends root_path, ignoring path_includes_root_path

In _get_request_attributes() (_asgi_common.py ~line 152), attributes["url.path"] is always set to root_path + path regardless of path_includes_root_path. For frameworks where path_includes_root_path=True (e.g. Starlette >= 0.33), this re-introduces the exact duplication the PR is trying to fix, just for the url.path span attribute.

Evidence
  • _get_request_attributes accepts path_includes_root_path and correctly passes it to _get_url() for url.full.
  • However, attributes["url.path"] = asgi_scope.get("root_path", "") + asgi_scope.get("path", "") at ~line 152 unconditionally concatenates root_path and path.
  • When Starlette >= 0.33 sets path_includes_root_path=True (starlette.py line 148), scope["path"] already contains root_path, so this expression doubles the root path segment.
  • _get_url() at line 45 correctly guards with if path_includes_root_path, showing the intended pattern was not applied consistently.

Identified by Warden code-review

Comment thread sentry_sdk/integrations/_asgi_common.py

@sentry-warden sentry-warden Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No tests covering the new root_path_in_path behavior in any of the changed test files

The PR description states tests were added for root_path handling in each ASGI integration, but none of the changed test files (test_asgi.py, test_fastapi.py, test_starlette.py) contain any tests exercising root_path or the EITHER/EXCLUDED enum paths. Have you considered adding at least one test that sets root_path in the ASGI scope and asserts the resulting URL is not duplicated?

Evidence
  • grep -r 'root_path' tests/integrations/asgi/test_asgi.py returns no matches.
  • grep -r 'root_path' tests/integrations/starlette/test_starlette.py returns no matches.
  • grep -r 'root_path' tests/integrations/fastapi/test_fastapi.py returns no matches.
  • The _RootPathInPath.EITHER branch in _get_path (used for Starlette >= 0.33) has detection logic (path.startswith(root_path + "/"), path == root_path) with no test coverage to verify correctness.
  • The Starlette integration selects EITHER or EXCLUDED based on version at line 149–151 of starlette.py, but neither path has an integration-level test with root_path set.

Identified by Warden code-review

Comment thread sentry_sdk/integrations/asgi.py
@alexander-alderman-webb alexander-alderman-webb marked this pull request as ready for review June 26, 2026 10:33
@alexander-alderman-webb alexander-alderman-webb merged commit fee5f8a into master Jun 26, 2026
143 checks passed
@alexander-alderman-webb alexander-alderman-webb deleted the webb/asgi/double-mount-prefix branch June 26, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ASGI integration doubles the mount prefix in request.url for Starlette Mounted sub-apps (root_path + path)

3 participants