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 allow disabling redirect_slashes at the FastAPI app level #3432

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

cyberlis
Copy link
Contributor

Right now redirect_slashes parameter in sub routers created with APIRouter ignored, because only main router's redirect_slashes inside FastAPI class is taken into account. FastAPI class creates new router without redirect_slashes argument and that's why router always uses default value redirect_slashes=True and always redirects on trailing slashes.
https://github.com/tiangolo/fastapi/blob/master/fastapi/applications.py#L69

Here is how to reproduce this problem

from fastapi import FastAPI
from fastapi.routing import APIRouter

router = APIRouter(redirect_slashes=False)

@router.get('/hello/')
def hello_page() -> str:
    return 'Hello, World!'

app = FastAPI()
app.include_router(router)

And if you request page http://127.0.0.1:8080/hello (without trailing slash) you will be redirected even with redirect_slashes=False

uvicorn main:app
INFO:     Started server process [96046]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     127.0.0.1:57330 - "GET /hello HTTP/1.1" 307 Temporary Redirect
INFO:     127.0.0.1:57330 - "GET /hello/ HTTP/1.1" 200 OK

I added redirect_slashes to __init__ method of FastAPI class and passed it to router's constructor.
That way I can change trailing slashes redirect behaviour globally for application instance.

@cyberlis
Copy link
Contributor Author

cyberlis commented Aug 2, 2021

please review @tiangolo

@StashOfCode
Copy link

Hey @tiangolo, any news on this pr? It solves a lot of hassle with 'slash/no slash' related redirects @Kludex @jaystone776

Copy link

@sk- sk- left a comment

Choose a reason for hiding this comment

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

LGTM

@sk-
Copy link

sk- commented Oct 6, 2022

@Kludex @tiangolo is there any chance this PR could be reviewed? I did review the code myself and it looks good and tested.

Some others have also spent the time writing a PR solving the same issue. See #5392.

Our motivation for having this merged:
We have stumbled across this issue, because some times customers make the mistake of adding/removing the trailing slash which increases the latency and later on they complain that our provided endpoints are slow.

It'd be great if we could control whether redirect_slashes is set.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 434f67c at: https://63457ff2dc12071d5f3bbfa3--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cf73051) 100.00% compared to head (4bfaa00) 100.00%.

❗ Current head 4bfaa00 differs from pull request most recent head 3ecac8b. Consider uploading reports for the commit 3ecac8b to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #3432   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       541    +1     
  Lines        13969     13972    +3     
=========================================
+ Hits         13969     13972    +3     
Impacted Files Coverage Δ
fastapi/applications.py 100.00% <ø> (ø)
tests/test_router_redirect_slashes.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 316ad61 at: https://63494c3b2c995530f46aa4ab--fastapi.netlify.app

@cyberlis
Copy link
Contributor Author

@Kludex @tiangolo please review PR

@github-actions
Copy link
Contributor

📝 Docs preview for commit 4bfaa00 at: https://6351aa4b4b09510865226f15--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 56c7bd4 at: https://639ce3792b35ad02dfe481d1--fastapi.netlify.app

@xpinguin
Copy link

xpinguin commented Apr 27, 2023

Hi guys, could you please update us about the status of this PR? We're waiting for this feature in master for a long time already and custom hacks aren't good. Thanks a lot!

@cyberlis
Copy link
Contributor Author

Hi guys, could you please update us about the status of this PR? We're waiting for this feature in master for a long time already and custom hacks aren't good. Thanks a lot!

I'll update it tomorrow

@github-actions
Copy link
Contributor

📝 Docs preview for commit 22499af at: https://644c43c0587f9e1ec747a7b2--fastapi.netlify.app

@cyberlis
Copy link
Contributor Author

cyberlis commented Apr 28, 2023

Hey @tiangolo, @Kludex
This PR is ready to be merged
It has tests and people still need it

Could you review it, please

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Apr 29, 2023

Starlette class doesn't have this parameter. It only exists on the Router class in Starlette. jfyk @tiangolo

@tiangolo tiangolo changed the title Add redirect_slashes parameter to FastAPI and pass it to router ✨ Add allow disabling redirect_slashes at the FastAPI app level Jun 22, 2023
@tiangolo
Copy link
Owner

Makes sense, thank you @cyberlis! 🚀

Thanks everyone for the reviews, especially for commenting that you tried out the code and confirming that this works and is useful for you. 🙇 🍰

This will be available in FastAPI 0.98.0, released in the next hours. 🎉

@tiangolo tiangolo enabled auto-merge (squash) June 22, 2023 10:37
@tiangolo tiangolo merged commit e94c13c into tiangolo:master Jun 22, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants