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

Fix optional route bug #1010

Closed
wants to merge 1 commit into from
Closed

Fix optional route bug #1010

wants to merge 1 commit into from

Conversation

JulienRobitaille
Copy link

@JulienRobitaille JulienRobitaille commented Jul 25, 2020

Optional routes are not working anymore because the ? gets escaped by the change introduced in: f12e237

This code handle that scenario by looking at the last character of the path before doing the escape.
When the last character is a ? it will do the escape on the path and prevent the escape of the needed ?.

I've added test to verify that optional routes are now working alongside the regexp meta character in path changes.

If you feel this change is not welcome, please close the PR but since it was a "very clearly well identified bug" It felt appropriate.

This PR fixes #1008

@tomchristie
Copy link
Member

So, you probably don't want an optional trailing slash. Starlette will handle trailing slash redirects appropriately for you, so you can add either /optional-path/ or /optional-path depending on which you want the canonical form to be, and you'll automatically get redirect behaviour from the other variant.

@evanlurvey
Copy link

I personally like this slash patcher more. Doesn't create dup docs or dup routes just fixes the regex.

def slash_patcher(app: FastAPI):
    app.router.redirect_slashes = False
    for r in app.routes:
        if r.path_regex.pattern.endswith("/$"):
            r.path_regex = re.compile(r.path_regex.pattern.replace("/$", "/?$"))
        else:
            r.path_regex = re.compile(r.path_regex.pattern.replace("$", "/?$"))

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.

router.get("/?") no longer works
3 participants