Do not overwrite "path" and "root_path" scope keys
#2352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm going to try to make this PR cleaner, but it already solves the problem. |
Kludex
changed the title
"path" and "root_path" scope keys
frankie567
reviewed
Dec 1, 2023
| path_params = dict(scope.get("path_params", {})) | ||
| path_params.update(matched_params) | ||
| root_path = scope.get("root_path", "") | ||
| child_scope = { | ||
| "path_params": path_params, | ||
| "app_root_path": scope.get("app_root_path", root_path), | ||
| "root_path": root_path + matched_path, | ||
| "path": remaining_path, |
There was a problem hiding this comment.
Don't we need to keep the original root_path and path in the child scope?
There was a problem hiding this comment.
Aren't they always there anyway? Those lines were just modifying them, no?
There was a problem hiding this comment.
Yes, you're right. My point was if we were using this child_scope as the new scope for inner routers, but it seems actually that it's always merged with the root scope.
Kludex
force-pushed
the
fix-path-on-mount
branch
from
6ae76f3
to
f684f3b
Compare
frankie567
approved these changes
Dec 1, 2023
|
@encode/maintainers can someone review this, please? |
abersheeran
approved these changes
Dec 1, 2023
maartenbreddels
added a commit
to widgetti/solara
that referenced
this pull request
Dec 5, 2023
path now includes root_path so we need a different way to remove it. It seems like route_root_path gives this information. See encode/starlette#2361 encode/starlette#2352 encode/starlette#1336
maartenbreddels
added a commit
to widgetti/solara
that referenced
this pull request
Dec 8, 2023
path now includes root_path so we need a different way to remove it. It seems like route_root_path gives this information. See encode/starlette#2361 encode/starlette#2352 encode/starlette#1336
RobbeSneyders
pushed a commit
to spec-first/connexion
that referenced
this pull request
Dec 29, 2023
Fixes the change in behaviour in starlette v0.33 via PR [#2352](encode/starlette#2352)
8 tasks
edoakes
added a commit
to ray-project/ray
that referenced
this pull request
Feb 12, 2024
FastAPI has fixed the broken starlette interaction in 0.109.2: https://fastapi.tiangolo.com/release-notes/#01092 Starlette handling of root_path has changed in 0.33.0: encode/starlette#2352. Prior to this, it was incorrectly stripping the root_path from the path field (which we need to mirror). Added compatibility logic to handle both of these. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com>
tterrysun
pushed a commit
to tterrysun/ray
that referenced
this pull request
Feb 14, 2024
FastAPI has fixed the broken starlette interaction in 0.109.2: https://fastapi.tiangolo.com/release-notes/#01092 Starlette handling of root_path has changed in 0.33.0: encode/starlette#2352. Prior to this, it was incorrectly stripping the root_path from the path field (which we need to mirror). Added compatibility logic to handle both of these. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Lonnie Liu <95255098+aslonnie@users.noreply.github.com> Signed-off-by: tterrysun <terry@anyscale.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
scope["root_path"]andscope["path"]differs from ASGI spec #1336