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

Add a note on how root_path should be used #230

Closed
wants to merge 1 commit into from

Conversation

pgjones
Copy link
Contributor

@pgjones pgjones commented Jan 21, 2021

This follows WSGI, as root_path is considered the equivalent to
SCRIPT_NAME and ensures that the root_path can be used as a global
prefix to all routes in the application.

@davidism
Copy link

davidism commented Jan 21, 2021

I guess it depends on how you deal with these internally, but the spec probably shouldn't say to base this off raw_path. The raw path is optional and may be inconsistent depending on how proxies decode things. root_path should be taken off the beginning of path, the value that's already been decoded.

I think there should be some more nuance than just "remove prefix", as the prefix should either end with a slash, or be followed by a slash in the path, in order for path to still be valid afterwards. For example, if root_path was /a, and you got a request to /abc/d, you wouldn't want to pass on /bc/d.

This follows WSGI, as root_path is considered the equivalent to
SCRIPT_NAME and ensures that the root_path can be used as a global
prefix to all routes in the application.
@pgjones
Copy link
Contributor Author

pgjones commented Jan 22, 2021

I've updated to try and make this clearer, and whilst slightly adjusting the rule.

/foo/bar "" No /foo/bar
/foo/bar /foo No /bar
/foo/bar /bar Yes
/foo/bar /foo/bar Yes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure this is the right logic...

Choose a reason for hiding this comment

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

/foo/bar should succeed and the path would be / (since it would be empty then have a leading slash prepended).

/foo/bar /foo No /bar
/foo/bar /bar Yes
/foo/bar /foo/bar Yes
/foo/bar/ /foo/bar No /

Choose a reason for hiding this comment

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

You should add an example of requesting /foobar with root_path="/foo" to show that it will 404.

@andrewgodwin
Copy link
Member

This is not how path is specified - see #229. I'm not willing to accept this unless we have all servers in agreement.

@pgjones
Copy link
Contributor Author

pgjones commented Jan 27, 2021

Closing, will adapt Quart and WSGI middleware instead.

@pgjones pgjones closed this Jan 27, 2021
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.

3 participants