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

[curtiscook]

I think this is related to https://github.com/encode/starlette/pull/932/files

Formerly you could use router.get("/?") to represent router.get("") & router.get("/"), but now it returns a 404 error as of the latest version

I'm seeing this in FastApi (mentioned here fastapi/fastapi#1648 )

[JulienRobitaille]

This seems to be linked with these changes in 0.13.05
0.13.4...0.13.5#diff-34fba745b50527bfb4245d02afd59246R110
0.13.4...0.13.5#diff-34fba745b50527bfb4245d02afd59246R120

It also seems to be linked with path including regexp meta character. This would make sens since ? is a regexp symbol.
0.13.4...0.13.5#diff-06c0af743b62c3e720fe92d0ce2b7f7bR86-R92

Commit introducing these changes:
f12e237

[tarioch]

It's also failing with others such as /.*
I'm using this to match any url which is then used by the single page app to navigate.

[tomchristie]

Also duplicated in #1010.

[tomchristie]

The Route() and Mount() API hasn't ever been intended to support general regex within the path argument. It's documented as using URI templating, but an implementation bug meant that an ability to use regex on the path was being made available.

Of the cases mentioned, I'd suggest...

• Use eg. /{parameter_name:path} for wildcard matching a segment, rather than /.*. (The segment will be available in request.path_params['parameter_name'].)
• You shouldn't need to use a trailing /?, since uvicorn automatically handles trail slash redirects.

[curtiscook]

You shouldn't need to use a trailing /?, since uvicorn automatically handles trail slash redirects.

This tends to drop CORS headers in the 307 redirect, so in actuality route.get("") and route.get("/") are fundamentally different despite documentation suggesting to use the later for the root endpoint. Likewise route.get("/?") formerly would serve both routes instead of performing a redirect.

At any rate, this difference in behavior- whether an implementation bug or not deserves more than a minor version upgrade, as it introduces a breaking change in the routing behavior. Additionally, it deserves better documentation

[tomchristie]

Could we try to aim for a little more goodwill here, please?

I understand why it's frustrating. Having said that, the PR was a bugfix release, it wasn't obvious that folks were relying on buggy behaviour, and nobody flagged it up as problematic. It had never occurred to me that eg. /? was something that anyone might be doing, since we don't document anything about paths supporting regexs in our routing docs.

[curtiscook]

Sorry, I appreciate all the hard work you have put into this lib and I feel strongly because it generally works well enough to use.

I disliked the response because it felt dismissive of how people are using starlette and that something that seemed like a minor change was actually breaking for a subset of users, especially with immediately closing the issue without discussion.

I do think that there are improvements to be had and probably most of them are in docs (i.e. docs should use consistent trailing slashes and note how the trailing slash redirects work)

If it were me personally, I would revert the change and reapply it with a larger version and additional documentation around known behavioral changes. I know you have a ton of issues to fix so this probably won't happen.

Best
-C

[tomchristie]

If it were me personally, I would revert the change and reapply it with a larger version and additional documentation around known behavioral changes.

Yeah, that's worth considering. Good point.

[curtiscook]

It had never occurred to me that eg. /? was something that anyone might be doing, since we don't document anything about paths supporting regexs in our routing docs.

People are really creative .. frequently more than you'd expect

I actually got the suggestion for "/?" from the FastAPI author based on starette supporting regex.

As mentioned before, the 307 behavior could use a little more documentation

[tarioch]

Of the cases mentioned, I'd suggest...

• Use eg. /{parameter_name:path} for wildcard matching a segment, rather than /.*. (The segment will be available in request.path_params['parameter_name'].)

Just wanted to give a positive feedback, this one works perfectly for my use case, thanks a lot.

[inikolaev]

I'm using TestClient for test my FastAPI routes and it looks like TestClient doesn't drop trailing slash, which means that behavior is different from uvicorn and makes me worry now after this change. Should TestClient behave the same way and drop trailing slash?

[tomchristie]

@inikolaev I'm not sure what you mean. I don't believe either uvicorn or TestClient will remove trailing slashes (nor should they)

[inikolaev]

@inikolaev I'm not sure what you mean. I don't believe either uvicorn or TestClient will remove trailing slashes (nor should they)

Ok, I probably have misinterpreted you comment:

You shouldn't need to use a trailing /?, since uvicorn automatically handles trail slash redirects.

But then, does TestClient handle these redirects? Now if my test makes a request with a trailing slash it fails because route cannot be found, but it was working before when I used /?.

[tomchristie]

TestClient handles redirects yes.

it fails because route cannot be found

Have a dig into exactly what's happening there. What URL are you accessing? What is the route mounted as? What status code is being returned?

[inikolaev]

TestClient handles redirects yes.

it fails because route cannot be found

Have a dig into exactly what's happening there. What URL are you accessing? What is the route mounted as? What status code is being returned?

Ok, I didn't remove /? in one of the routes, but now I started to receive 307 redirect when making a request with trailing slash. Though when trying to create a reproducer I can't reproduce it, weird.

[inikolaev]

TestClient handles redirects yes.

it fails because route cannot be found

Have a dig into exactly what's happening there. What URL are you accessing? What is the route mounted as? What status code is being returned?

Ok, I didn't remove /? in one of the routes, but now I started to receive 307 redirect when making a request with trailing slash. Though when trying to create a reproducer I can't reproduce it, weird.

Ok, my bad, my reproducer also works by returning 307, but I checked for response.ok.

I debugged through the code and noticed that router explicitly send 307 without a slash. Is it possible to do internal redirect instead? I don't really care whether endpoint ends with slash or not - I would like to handle it regardless and avoid this extra round trip. Or does uvicorn handles this redirect and it doesn't actually getting sent over the wire?

[curtiscook]

You can stack router.<method>

ie

@router.get("/foo")
@router.get("/foo/")
async def foo():

Or

@router.get("")
@router.get("/")
async def root_foo():

Otherwise starlette will 307 to the one you define

[Reapor-Yurnero]

You can stack router.<method>

ie

@router.get("/foo")
@router.get("/foo/")
async def foo():

Or

@router.get("")
@router.get("/")
async def root_foo():

Otherwise starlette will 307 to the one you define

That was a very good workaround