Skip to content
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

[WIP] Handle scope["root_path"] and scope["path"] as per ASGI spec #1774

Closed
wants to merge 2 commits into from

Conversation

frankie567
Copy link
Member

Rationale

This is an exploratory work as how to solve issue #1336. The issue details the problem quite well but in a nutshell:

  • The issue arises when using a Mount router alongside the root_path option.
  • We consider that path is the remainder of the whole path striped from root_path. As pointed out here, this is not how the ASGI spec was imagined: path should contain root_path.
  • As a consequence, every route matching is done assuming the `root_path* is not present in the path.
  • Besides, we use this behavior to recursively handle Mount routers, by adding the router prefix on root_path and stripping path until we match the single route:

remaining_path = "/" + matched_params.pop("path")
matched_path = path[: -len(remaining_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,
"endpoint": self.app,
}
return Match.FULL, child_scope

Notice that we define an app_root_path to keep track of the original root_path.

The way I understand it, we should keep both path and root_path untouched. It's a shame of course because we won't be able to handle Mount elegantly as we do now.

Current work status

First of all, I added a unit test to reproduce the issue.

Then, I've tried a first approach to solve it which consists of two things:

  • Instead of using path and root_path directly, use internal variables current_path and current_root_path (admittedly, this is a very uninspired naming) to allow our recursive approach
  • Take care of stripping the root_path from the path so we can match properly, both in Route and Mount

This solves the specific problem but breaks other parts of the code. Obviously, every parts of the routing which makes use of the path will need to account for this root_path, e.g. in WebSocketRoute or in Router, in the part where we make slash redirects.

An idea I had to tackle this would be to have a common prepare_scope which could be called by the Route classes to prepare the path, add the "internal" variables, while keeping the original path and root_path untouched.

Before going further, I would be interested in your views and opinions about this so we can take the right path.

Cheers!

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 27, 2022

@frankie567 Do you mind if I keep this open?

@frankie567
Copy link
Member Author

Oh yes, sure, @Kludex! I closed it in order to not keep too much stale things, but definitely willing to work on this when needed 🙂

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 27, 2022

Sorry for the long waiting time.

I find this too complex, and I need time to sit, and get more knowledge around it.

@Kludex Kludex reopened this Dec 27, 2022
@frankie567
Copy link
Member Author

No worries 🙂 Feel free to ping me when needed!

@Kludex Kludex added this to the Version 1.x milestone Feb 9, 2023
@Kludex
Copy link
Sponsor Member

Kludex commented Jul 5, 2023

@frankie567 I know that I said that I'd like to keep this around, but I'm going to close it for now (hope it's fine).

In any case, I'm sorry for the long time to do something here.

@Kludex Kludex closed this Jul 5, 2023
@frankie567
Copy link
Member Author

No problem, @Kludex 😄 Keep up the great work!

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.

2 participants