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

Handling trailing slashes properly with mounted routes #869

Open
ramnes opened this issue Mar 20, 2020 · 17 comments
Open

Handling trailing slashes properly with mounted routes #869

ramnes opened this issue Mar 20, 2020 · 17 comments

Comments

@ramnes
Copy link

ramnes commented Mar 20, 2020

Hello there!

In the documentation, there is this example about large applications. If we make it a little more complete, we can get something like this:

import uvicorn
from starlette.applications import Starlette
from starlette.responses import JSONResponse
from starlette.routing import Mount, Route


async def homepage(request):
    return JSONResponse(status_code=200)


async def users(request):
    return JSONResponse([{"username": "bob"}, {"username": "alice"}])


async def user(request):
    username = request.path_params["username"]
    if username == "bob":
        return JSONResponse({"username": "bob"})
    if username == "alice":
        return JSONResponse({"username": "alice"})
    return JSONResponse(status_code=404)


routes = [
    Route('/', homepage),
    Mount('/users', routes=[
        Route('/', users, methods=['GET', 'POST']),
        Route('/{username}', user),
    ])
]

app = Starlette(routes=routes)
uvicorn.run(app)

But the problem with this example is that all routes are behaving differently:

  • curl localhost:8000 answers as expected
  • curl localhost:8000/ answers as expected
  • curl localhost:8000/users emits a 307
  • curl localhost:8000/users/ answers as expected
  • curl localhost:8000/users/bob answers as expected
  • curl localhost:8000/users/bob/ emits a 307

So one route answers as expected whether you give it a trailing slash or not, another one only answers directly if you give it the trailing slash, and another one only answers directly if you do not give it the trailing slash.

Of course, the fact that the homepage is answering with or without slash is only due to how HTTP works, because you can't send a GET without a route, and all HTTP clients will convert this to GET /.

At this point I'm just paraphrasing #823, where the discussion ended with:

That's intentional, because a mount point should always be in the form /path-to-mount/{...}

Alright, let's take that for a fact, but then... what if I want all my mounted routes to answer without a trailing slash? This is particularly important for API development, where you want to provide consistent endpoints.

I started by trying to give an empty route, i.e. Route('', users, methods=['GET', 'POST']), but that simply does not work: AssertionError: Routed paths must start with '/'.

Reconsidering the answer in #823, I then tried to mount on / directly, i.e. doing this, thinking that it would work around my problem:

routes = [
    Route('/', homepage),
    Mount('/', routes=[
        Route('/users', users, methods=['GET', 'POST']),
        Route('/users/{username}', user),
    ])
]

That seems weird here, because you could simply add the routes at the top level, but in my real use-case, I'm using routers that I import and mount. And that works great... until you add another Mount, that will never be resolved, because the routing stops at the first mount point with a matching prefix, as explained in #380 (comment).

Then I saw #633 (comment), and tried to add /? everywhere, just like this:

routes = [
    Route('/', homepage),
    Mount('/users', routes=[
        Route('/?', users, methods=['GET', 'POST']),
        Route('/{username}/?', user),
    ])
]

This is not user-friendly at all, but it helped on the /users/{username} route, which now answers as expected whether you give it a trailing slash or not. Alas, the /users route still emits a 307. So basically, at this point, I just don't know how to handle this, and I'm pretty frustrated by spending so much time on such a trivial thing.

What I ended up doing, since I'm using routers, is defining the whole path in my routes, with a big hack at the application level:

routes = (
    users_router.routes,
    + other_router.routes
)

If there isn't something obvious that I missed and would resolve my problem in a simpler way, please consider one or more of the following (by my preference order):

  • Making all routes accessible without redirection with or without the trailing slash
  • "Denormalizing" all routes when they're mounted, so that you only have a list of all paths and do not stop on the first mount point that matches (basically what my hack is doing)
  • Making the developer in complete control of trailing slashes by not emitting any implicit 307
  • Having this behavior configurable and documented
  • Handling empty routes
  • Handling /?

Sorry for the long text, I couldn't find a simpler way to expose the whole problem and thinking behind it.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@AndreyKis
Copy link

AndreyKis commented Apr 7, 2020

One interesting thing to point out here.
In my case, 307 response breaks external API which is supposed to call my webhook. This external API doesn't work with redirects and fails to call my endpoint.

As far as I understood, I could get rid of 307 by using /?, but this doesn't work for me. The reason seems to be the usage of HTTPEndpoint based class.

Example:
routes = [ ..., Mount("/path/?", PathClass) ]

where PathClass defined as following:

class PathClass(HTTPEndpoint): ...

Actually my scenario causes me to change my desired API architecture as otherwise it won't work properly.
To give some context, here is what I am trying to do in that class :
query_param = request.query_params.get('query_param')

And if I try to send request to /path?query_param=query_param/, I do not get 307, but query_param variable has this trailing slash appended.
While omitting trailing slash in request results in 307, which, as I mentioned, breaks external API.

So for now on, I use request.path_params instead, which kind of solves the problem, but that looks a bit restrictive to me.
I also still need to tell my external API to trigger my endpoint with trailing slash as /? doesn't seem to work with HTTPEndpoint based class

@andrewkcarter
Copy link

Similar conversation over here, and they implemented a solution: fastapi/fastapi#414

@VirgileD
Copy link

Hello, I've got the same question, i.e. I want to be able to request a route like "/resources", not "/resources/" when e.g. I post a resource. When I ant to rertieve a resource of id XX I'll use "/resources/XX". But It seems I can't reaaly do that. I don't really want to use FastApi.
Is there something I can do?

@zhqu1148980644
Copy link

the same problem, mounted /xxx, and it returns a 307, only /xxx/ returns correctly.

@haibane-tenshi
Copy link

Ran into the same issue when integrating with Ariadne's ASGI.

routes = [
    Mount('/graphql', graphql),
]

results in

  • /graphql gives hard 404
  • /graphql/ works as expected

Since app is not even part of my project I cannot do anything here. In fact there is actually no way to even force a redirect, except by creating a manual route like this:

routes = [
    Route('/graphql', lambda _: RedirectResponse('/graphql/')),
    Mount('/graphql', graphql),
]

Which is a suboptimal solution as already discussed.

@aksel
Copy link

aksel commented May 21, 2021

From my understanding, /users/? is no longer supported either.

From what I can gather, compile_path now escapes any regex tokens:

>>> from starlette.routing import compile_path
>>> r_path, *_ = compile_path("/users/?")
>>> r_path
re.compile('^/users/\\?$')
>>> r_path.match("/users")
None
>>> r_path.match("/users/")
None
>>> r_path.match("/users/?")
<re.Match object; span=(0, 8), match='/users/?'>

@nilansaha
Copy link

Ran into the same issue when integrating with Ariadne's ASGI.


routes = [

    Mount('/graphql', graphql),

]

results in

  • /graphql gives hard 404

  • /graphql/ works as expected

Since app is not even part of my project I cannot do anything here. In fact there is actually no way to even force a redirect, except by creating a manual route like this:


routes = [

    Route('/graphql', lambda _: RedirectResponse('/graphql/')),

    Mount('/graphql', graphql),

]

Which is a suboptimal solution as already discussed.

Running into the same thing with ariadne. We use Apollo Federation and the gateway reports my service is down every once in a while. How is this not solved yet. Did any of you guys find a solution for ariadne mounts ?

@cgorski
Copy link

cgorski commented Aug 30, 2021

Creating some middleware worked for me:

from starlette.requests import Request
from starlette.types import ASGIApp, Receive, Scope, Send

class GraphQLRedirect:
    def __init__(
        self,
        app: ASGIApp
    ) -> None:
        self.app = app
        
    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        if(scope['path'] == '/graphql'):
            scope['path'] = '/graphql/'
            scope['raw_path'] = b'/graphql/'
        await self.app(scope, receive, send)

and

app = FastAPI(redoc_url=None, docs_url=None, openapi_url=None)

app.add_middleware(GraphQLRedirect)

type_defs = load_schema_from_path("schema.graphql")
schema = make_executable_schema(
    type_defs, query, snake_case_fallback_resolvers
)

app.mount("/graphql", GraphQL(schema))

No more 307s!

@nilansaha
Copy link

Thanks I just added the backslash to the GQL gateway config and solved my case. But this is an issue that needs to be solved in starlette itself. I have no idea why this is not picked up yet.

@steve-marmalade
Copy link

steve-marmalade commented Jan 25, 2022

We switched from FastAPI to Starlette (since we've completely migrated to GraphQL via Ariadne), and just hit this issue as the 307 Redirect caused our web-app build to crash. Took me a while to find this page and would definitely 👍 a fix for this in Starlette

@Peticali
Copy link

Peticali commented Apr 3, 2022

Happening here too, but makes the domain redirect to localhost

image
getting domain/login/ (my route is binded to /login)

image
redirect response

@Judekeyser
Copy link

Still happening today with the latest version.

@kym6464
Copy link

kym6464 commented Aug 21, 2023

RE "Making the developer in complete control of trailing slashes by not emitting any implicit 307" this seems to be possible as of version 0.31.0

app = Starlette(...)
app.router.redirect_slashes = False

@Judekeyser
Copy link

It see it nowhere in the documentation. Is it stable? Should it be written somewhere? (The changelog mentions a redirect slashes" support since 0.7... not sure it's the same thing.)

@kym6464
Copy link

kym6464 commented Aug 21, 2023

It's not documented (based on a code search). I stumbled upon it after spending several hours on this issue. It would make sense to expose this option in the Starlette constructor

That, combined with @cgorski's suggestion, and I think I have a solid workaround (at least for my use case).

@Jdsleppy
Copy link
Contributor

Jdsleppy commented Sep 8, 2023

For my use case (only one layer of Mounts) this has been a sufficient workaround at least after a couple minutes of testing. I'll update if I run across any issues. mount() will "flatten" one level of mounts and routes. You could write a recursive version to handle nested Mounts. You could also make something to wrap all the app.routes and flatten every Mount it encounters. Lots of ways to do it.

def mount(m: Mount) -> list[Route]:
    new_routes = []
    # This is where I "assert" that there are no nested Mounts inside Mounts
    old_routes: list[Route] = m.routes
    for r in old_routes:
        new_route = Route(
            f"{m.path}{r.path}".rstrip("/") or "/",
            r.endpoint,
            methods=r.methods,
            name=f"{m.name}:{r.name}",
        )
        new_routes.append(new_route)

    return new_routes


app = Starlette(
    routes=[
        Route("/health", routes.health_check, name="health"),
        Route("/robots.txt", routes.robots_txt, name="robots_txt"),
        Route("/light_theme", routes.light_theme, name="light_theme"),
        Mount(
            "/static",
            app=StaticFiles(
                directory=pathlib.Path(__file__).parent / ".." / "static" / "build"
            ),
            name="static",
        ),
    ]
    + mount(Mount("/auth", app=auth_router, name="auth"))
    + mount(Mount("/widgets", app=widget_router, name="widgets"))
    + mount(Mount("/foobars", app=foobar_router, name="foobars"))
    + mount(Mount("/", app=home_router, name="home")),
    # etc.
)

@ajoy39
Copy link

ajoy39 commented Apr 17, 2024

Checking in from 2024 and this is still an issue, and when trying to use a starlette app with Google Cloud Run it becomes even more so as cloud run does not like 307s and will sometimes just eat the response

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

No branches or pull requests