fix(routing): normalize trailing slashes on mounted routes#3377
Closed
pctablet505 wants to merge 2 commits into
Closed
fix(routing): normalize trailing slashes on mounted routes#3377pctablet505 wants to merge 2 commits into
pctablet505 wants to merge 2 commits into
Conversation
Make Mount routes match both the trailing-slash and no-trailing-slash forms of the mount point, so /users and /users/ behave consistently. Fixes Kludex#869
Author
|
Closing this for now — I want to rethink the approach before taking up review time here. Sorry for the noise. |
This file contains hidden or 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
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.
Fixes #869
Mount('/users', routes=[Route('/', users), Route('/{username}', user)])currently only matches when the request path ends with a slash.GET /usersreturns a 307 redirect to/users/, andGET /users/bob/returns a 307 redirect to/users/bob, while a top-levelRoute('/', ...)responds to both/and an empty path. This asymmetry betweenRouteandMounthas been reported repeatedly: external webhook callers that don't follow redirects fail, Ariadne's GraphQL mount is unreachable at/graphql, and people have resorted to settingapp.router.redirect_slashes = Falseor adding middleware to rewrite the path.In #823 the maintainer explained that requiring the slash was intentional, because a mount point is meant to be
/path-to-mount/{...}and/some-pathshould not match/some-path{...}. That constraint is still respected here:/usersastill 404s.This change rewrites the catch-all
pathregex thatMountbuilds fromcompile_path(self.path + "/{path:path}")so the trailing slash is optional. When the mount point is hit without a slash, the router appends the slash toscope["path"]internally before dispatching to the mounted app, soRoute('/', ...)resolves directly andRoute('/{username}', ...)works with or without a trailing slash. The existingtest_mount_urlsassertion is updated to expect/usersto stay/usersinstead of redirecting to/users/, and a newtest_mount_trailing_slash_with_child_routescovers the child-route cases.The main trade-off is that
/usersno longer redirects to/users/; it now serves the mounted content directly. That removes the 307 pain for mounts but changes behavior for anyone relying on the old redirect. OtherRouteredirects andredirect_slashesbehavior are unaffected.Verified by running
pytest tests/test_routing.py.