-
-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not overwrite "path"
and "root_path"
scope keys
#2352
Conversation
I'm going to try to make this PR cleaner, but it already solves the problem. |
"path"
and "root_path"
scope keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from my question above, LGTM! You have filled the holes I left in my previous PR :)
Still not convinced about the naming current_root_path
and current_path
, but that's minor 😄 Suggestion (not ideal): route_root_path
/ route_path
.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to keep the original root_path
and path
in the child scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't they always there anyway? Those lines were just modifying them, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
6ae76f3
to
f684f3b
Compare
@encode/maintainers can someone review this, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks very nice, I like the regular expressions here.
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
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
Fixes the change in behaviour in starlette v0.33 via PR [#2352](encode/starlette#2352)
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>
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>
scope["root_path"]
andscope["path"]
differs from ASGI spec #1336