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

Strip trailing slash ( / ) from URL route strings. #633

Closed
gnat opened this issue Sep 4, 2019 · 6 comments · Fixed by #707
Closed

Strip trailing slash ( / ) from URL route strings. #633

gnat opened this issue Sep 4, 2019 · 6 comments · Fixed by #707

Comments

@gnat
Copy link

gnat commented Sep 4, 2019

Hey all, an optional trailing slash seems to be pretty standard on most websites, but in Starlette, you must add separate routes to handle them. Example:

routes = [
    Route('/', PlainTextResponse("home")),
    Route('/test', PlainTextResponse("test")),
    Route('/test/', PlainTextResponse("I need to be added separately?!"))
]

app = Starlette(routes=routes)

Could a route = route.rstrip("/") please be added to https://github.com/encode/starlette/blob/master/starlette/routing.py in class Router before running the match so these routes don't have to be manually added? Thanks!

@gnat gnat changed the title Strip trailing slash ( / ) from route strings. Strip trailing slash ( / ) from URL route strings. Sep 4, 2019
@blueyed
Copy link
Contributor

blueyed commented Sep 11, 2019

Not sure about this myself - seems like a bit too much magic to have by default.

But you might want to create a PR with your suggested patch already, so this can be discussed at the code level.

@blueyed
Copy link
Contributor

blueyed commented Sep 11, 2019

btw: you can use "/path/?" to match an optional slash..

tomchristie added a commit that referenced this issue Nov 6, 2019
Currently we have behavior so that if:

* A `/some-path/`route exists.
* A `/some-path` route does not exist.

Then if a request to `/some-path` is made, we'll redirect to the trailing slash case.

We ought to also handle the converse case, so that if...

* A `/some-path`route exists.
* A `/some-path/` route does not exist.

Then if a request to `/some-path/` is made, we'll redirect to the no trailing slash case.

Closes #633
@tomchristie
Copy link
Member

tomchristie commented Nov 6, 2019

Absolutely, yup.

We already handle the converse case, but for some reason I've only got the behavior in one way right now.

routes = [
    Route('/', PlainTextResponse("home")),
    Route('/test/', PlainTextResponse("test")),
    # Route('/test', PlainTextResponse("I do not need to be added separately"))
]```

tomchristie added a commit that referenced this issue Nov 6, 2019
* Handle trailing slash redirects properly.

Currently we have behavior so that if:

* A `/some-path/`route exists.
* A `/some-path` route does not exist.

Then if a request to `/some-path` is made, we'll redirect to the trailing slash case.

We ought to also handle the converse case, so that if...

* A `/some-path`route exists.
* A `/some-path/` route does not exist.

Then if a request to `/some-path/` is made, we'll redirect to the no trailing slash case.

Closes #633

* Update routing.py

* Update routing.py
@achien
Copy link

achien commented Jan 28, 2020

I am seeing this issue again. From the history of routing.py it looks like #707 might have been reverted by #704?

@blueyed
Copy link
Contributor

blueyed commented Jan 28, 2020

Yeah, too bad #707 did not have a test.
@tomchristie please re-open / fix it.
@achien might be helpful to know which commit from #704 it was - have not checked it myself, but there are several.

@achien
Copy link

achien commented Jan 28, 2020

@blueyed it looks like the merge commit caused the regression https://github.com/encode/starlette/pull/704/files#diff-34fba745b50527bfb4245d02afd59246, specifically around line 617 of the old version and 564 of the new version.

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 a pull request may close this issue.

4 participants