Repository navigation
Conversation
|
the test failure seems to be unrelated. fails on master too for me |
|
@Kludex when you have time I would really appreciate your feedback on this one 🙏 I guess the option could also be renamed to |
|
We are in the same predicament where we cannot use proxy rewrite rules or configure the root_path directly in FastAPI code, and therefore have been unable to leverage Uvicorn/FastAPI technologies in our stack since version 0.26.0 of Uvicorn which changed the behaviour of root_path. +1 for this feature |
|
+1 for this, encountered the same issue |
|
+1 same issue using traefik and the strip prefix middleware |
|
Would it help move this PR forward if someone fixed the merge conflicts and got CI to pass? Or are there design arguments that speak against this change? |
0e645bd to
333d531
Compare
| raw_path, _, query_string = event.path.partition("?") | ||
| path = unquote(raw_path) | ||
| full_path = self.root_path + path | ||
| full_raw_path = self.root_path.encode("ascii") + raw_path.encode("ascii") |
There was a problem hiding this comment.
IT seems this wasn't consistent with wsproto_impl.py which did this at https://github.com/Kludex/uvicorn/pull/2493/changes#diff-5f163724db1eaba811cc5e4ed55e529af488e351ee52ccfb7c1ad0329a8f0d92L169-L171 so I just updated this to match its behavior
|
@Kludex I rebased this against main, would you be able to review it? It's still an open issue on multiple deploys for me and it would make life much easier if this option was available. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUvicorn adds ChangesASGI root path support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Uvicorn CLI
participant Main as uvicorn.main
participant Config as uvicorn.config.Config
participant Protocol as HTTP/WebSocket protocol
participant Scope as ASGI scope
CLI->>Main: pass asgi_root_path
Main->>Config: create configuration
Config->>Protocol: provide root-path values
Protocol->>Scope: set root_path and request paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/deployment/index.md`:
- Around line 198-205: Update the `/proxy/` Nginx location example to include
WebSocket proxying directives: set `proxy_http_version` to 1.1 and add the
`Upgrade` and `Connection $connection_upgrade` headers. Preserve the existing
`$connection_upgrade` map and all current proxy headers.
In `@uvicorn/config.py`:
- Line 225: Move the new asgi_root_path parameter in Config.__init__ to the end
of the existing parameter list so all prior positional arguments, including
limit_concurrency, retain their bindings. Add a regression test that constructs
Config positionally through limit_concurrency and verifies the value is assigned
correctly.
- Around line 306-308: Update Config initialization to reject configurations
where both root_path and asgi_root_path are set, rather than only logging and
continuing. Raise the established configuration error from the validation branch
so direct Config consumers cannot run with conflicting path values, and add
coverage for direct Config construction with both options.
In `@uvicorn/main.py`:
- Line 255: Update the help text for the --root-path option in the CLI
definition to explicitly state that the provided value prefixes both path and
raw_path, while retaining the existing description of serving the application
under that root path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92936948-ba86-44e1-9e25-cd9eb0af771a
📒 Files selected for processing (13)
docs/deployment/index.mddocs/settings.mdtests/protocols/test_http.pytests/protocols/test_websocket.pytests/test_cli.pytests/test_main.pyuvicorn/config.pyuvicorn/main.pyuvicorn/protocols/http/h11_impl.pyuvicorn/protocols/http/httptools_impl.pyuvicorn/protocols/websockets/websockets_impl.pyuvicorn/protocols/websockets/websockets_sansio_impl.pyuvicorn/protocols/websockets/wsproto_impl.py
| if root_path and asgi_root_path: | ||
| logger.error("Setting both 'root_path' and 'asgi_root_path' is not supported.") | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject the invalid combination for direct Config consumers.
This branch only logs an error and returns a usable Config. A caller that creates Config directly can then run with root_path prefixing request paths while asgi_root_path supplies a different ASGI scope value.
Reject this combination in Config, or enforce the invariant in every Config consumer. Add coverage for direct Config construction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@uvicorn/config.py` around lines 306 - 308, Update Config initialization to
reject configurations where both root_path and asgi_root_path are set, rather
than only logging and continuing. Raise the established configuration error from
the validation branch so direct Config consumers cannot run with conflicting
path values, and add coverage for direct Config construction with both options.
There was a problem hiding this comment.
Kept as a log in Config.__init__ on purpose: Config never raises for invalid
combinations in uvicorn (--reload + --workers and the reload-dirs case only log), and
hard failures are logger.error + sys.exit(STARTUP_FAILURE) outside __init__.
In 74900ea the exit moved to right after Config(...) in run() and now uses
STARTUP_FAILURE, matching test_run_invalid_app_config_combination.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline uvicorn/main.py --items all --type function --match 'run|main'
ast-grep outline tests/test_main.py --items all --type function --match 'invalid_app_config_combination|root_path|asgi_root_path'
rg -n -C 10 "asgi_root_path|STARTUP_FAILURE|root_path" uvicorn/main.py tests/test_main.pyLength of output: 16866
@amol-, I verified the update in uvicorn/main.py.
run() now exits with STARTUP_FAILURE immediately after Config(...). tests/test_main.py covers that path and confirms that Uvicorn does not import the app first.
The direct Config case remains. A caller can create Config(root_path="/app", asgi_root_path="/proxy") and provide it to Server. The constructor logs the error but returns a usable configuration.
The constructor convention explains the selected behavior. It does not enforce the stated invariant for direct Config consumers. I will keep the finding open.
✏️ Learnings added
Learnt from: amol-
Repo: Kludex/uvicorn PR: 2493
File: uvicorn/config.py:306-308
Timestamp: 2026-08-05T09:57:28.788Z
Learning: In Uvicorn, `Config.__init__` reports invalid configuration combinations by logging instead of raising. Command entry points such as `uvicorn.main.run()` perform hard failure with `sys.exit(STARTUP_FAILURE)` after configuration construction. Existing examples include reload-related configuration validation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
3 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="uvicorn/config.py">
<violation number="1" location="uvicorn/config.py:225">
P2: Existing positional `uvicorn.Config` callers can silently configure the wrong limits, timeouts, or SSL settings because this new optional parameter was inserted before existing parameters. Appending `asgi_root_path` after the current parameter list would preserve the public constructor’s positional compatibility.</violation>
<violation number="2" location="uvicorn/config.py:307">
P2: The mutual-exclusion check only logs an error and is only enforced in `uvicorn.run()` via `sys.exit(1)`. When someone uses the programmatic API `uvicorn.Server(uvicorn.Config(..., root_path=..., asgi_root_path=...))` — the documented way to embed the server — the invalid combination isn't rejected: the config logs an error and then keeps serving, and because of `self.asgi_root_path = asgi_root_path or root_path` the `asgi_root_path` value arbitrarily wins. That clashes with the PR's goal of rejecting simultaneous use and with the settings doc calling them mutually exclusive. Consider raising a `ValueError` from `Config.__init__` when both are set, which would make the rejection consistent across CLI and programmatic use and let `run()` drop its duplicated `sys.exit` branch.</violation>
</file>
<file name="docs/deployment/index.md">
<violation number="1" location="docs/deployment/index.md:194">
P3: Consider linking to the canonical `--asgi-root-path` documentation in docs/settings.md instead of re-explaining the option's semantics here, keeping the deployment page concise and pointing to the single source of truth. The nginx example itself is accurate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| date_header: bool = True, | ||
| forwarded_allow_ips: list[str] | str | None = None, | ||
| root_path: str = "", | ||
| asgi_root_path: str = "", |
There was a problem hiding this comment.
P2: Existing positional uvicorn.Config callers can silently configure the wrong limits, timeouts, or SSL settings because this new optional parameter was inserted before existing parameters. Appending asgi_root_path after the current parameter list would preserve the public constructor’s positional compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At uvicorn/config.py, line 225:
<comment>Existing positional `uvicorn.Config` callers can silently configure the wrong limits, timeouts, or SSL settings because this new optional parameter was inserted before existing parameters. Appending `asgi_root_path` after the current parameter list would preserve the public constructor’s positional compatibility.</comment>
<file context>
@@ -222,6 +222,7 @@ def __init__(
date_header: bool = True,
forwarded_allow_ips: list[str] | str | None = None,
root_path: str = "",
+ asgi_root_path: str = "",
limit_concurrency: int | None = None,
limit_max_requests: int | None = None,
</file context>
| self.reload_excludes: list[str] = [] | ||
|
|
||
| if root_path and asgi_root_path: | ||
| logger.error("Setting both 'root_path' and 'asgi_root_path' is not supported.") |
There was a problem hiding this comment.
P2: The mutual-exclusion check only logs an error and is only enforced in uvicorn.run() via sys.exit(1). When someone uses the programmatic API uvicorn.Server(uvicorn.Config(..., root_path=..., asgi_root_path=...)) — the documented way to embed the server — the invalid combination isn't rejected: the config logs an error and then keeps serving, and because of self.asgi_root_path = asgi_root_path or root_path the asgi_root_path value arbitrarily wins. That clashes with the PR's goal of rejecting simultaneous use and with the settings doc calling them mutually exclusive. Consider raising a ValueError from Config.__init__ when both are set, which would make the rejection consistent across CLI and programmatic use and let run() drop its duplicated sys.exit branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At uvicorn/config.py, line 307:
<comment>The mutual-exclusion check only logs an error and is only enforced in `uvicorn.run()` via `sys.exit(1)`. When someone uses the programmatic API `uvicorn.Server(uvicorn.Config(..., root_path=..., asgi_root_path=...))` — the documented way to embed the server — the invalid combination isn't rejected: the config logs an error and then keeps serving, and because of `self.asgi_root_path = asgi_root_path or root_path` the `asgi_root_path` value arbitrarily wins. That clashes with the PR's goal of rejecting simultaneous use and with the settings doc calling them mutually exclusive. Consider raising a `ValueError` from `Config.__init__` when both are set, which would make the rejection consistent across CLI and programmatic use and let `run()` drop its duplicated `sys.exit` branch.</comment>
<file context>
@@ -301,6 +303,9 @@ def __init__(
self.reload_excludes: list[str] = []
+ if root_path and asgi_root_path:
+ logger.error("Setting both 'root_path' and 'asgi_root_path' is not supported.")
+
if (reload_dirs or reload_includes or reload_excludes) and not self.should_reload:
</file context>
There was a problem hiding this comment.
Partially addressed in 74900ea: the exit now happens immediately after Config(...) in
run() and uses STARTUP_FAILURE instead of 1.
Not raising ValueError from Config.__init__, though: no other invalid config
combination raises there (--reload + --workers only warns), and raising would give CLI
users a traceback instead of uvicorn's one-line error. A programmatic Server(Config(...))
user still gets the explicit error log. Happy to switch if @Kludex prefers a hard failure.
|
|
||
| ### Subpath deployments | ||
|
|
||
| If your proxy serves the application from a subpath and forwards requests without stripping that prefix, use `--asgi-root-path` so ASGI `root_path` is set without duplicating the request path. |
There was a problem hiding this comment.
P3: Consider linking to the canonical --asgi-root-path documentation in docs/settings.md instead of re-explaining the option's semantics here, keeping the deployment page concise and pointing to the single source of truth. The nginx example itself is accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/deployment/index.md, line 194:
<comment>Consider linking to the canonical `--asgi-root-path` documentation in docs/settings.md instead of re-explaining the option's semantics here, keeping the deployment page concise and pointing to the single source of truth. The nginx example itself is accurate.</comment>
<file context>
@@ -189,6 +189,26 @@ http {
+### Subpath deployments
+
+If your proxy serves the application from a subpath and forwards requests without stripping that prefix, use `--asgi-root-path` so ASGI `root_path` is set without duplicating the request path.
+
+For example:
</file context>
There was a problem hiding this comment.
Linked --asgi-root-path to ../settings.md#http in 74900ea. Kept the sentence, since
it carries the deployment precondition (proxy forwards the prefix without stripping it)
that settings.md doesn't state
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Summary
Rephrases the
--root-pathhelp message to make more clear that the option will prefix all paths with its value and adding the--asgi-root-pathoption which does setroot_pathwithout altering the path itself.The naming of the new option was chosen to avoid introducing backward incompatibilities with users relying on the old option.
This is necessary when uvicorn is serving an application that is being proxied behind a proxy server to avoid having to rewrite urls in the proxy server which is more complex and error prone.
For example, we could use nginx with following configuration
and start unicorn with
the fact that the application is served in a subpath would be totally transparent to the web application as far as the web framework correctly handles
root_path.This could allow a misconfiguration if the nginx
locationand uvicornasgi-root-pathdon't match, but I think it's out of scope for uvicorn to detect deployment misconfigurations.This was discussed in #2490
Checklist
Summary by CodeRabbit
New Features
--asgi-root-pathoption for configuring the ASGI request scope independently from URL path handling.Bug Fixes
--root-pathand--asgi-root-path.