Handlers with path variables are used even if parent route exists

[samdbmg]

Describe the bug
Prior to Sanic 21, a route handler for /foo/ would be used instead of one for /foo/<var>/ if a request was made for GET /foo/, but now the second handler is called with an empty string for <var>.

Is this the intended behaviour - as in I should handle the case where the variable is empty instead of having a separate route? If so I'd have expected my example to throw a sanic_routing.exceptions.RouteExists to prevent me making this mistake.

Code snippet

from sanic import Sanic
from sanic.response import json

app = Sanic("My Hello, world app")

@app.route('/foo/')
async def test(request):
    return json({'hello': 'world'})

@app.route('/foo/<var>/')
async def myreq(request, var):
    return json({'hello': var})

if __name__ == '__main__':
    app.run()

Expected behavior
Running the snippet above, I'd expect to see:

❯ curl localhost:8000/foo/bar/
{"hello":"bar"}%        
❯ curl localhost:8000/foo/
{"hello":"world"}%                                                                                                                                                            

(which I what happens on sanic 20.12.3)

Instead I get (on 21.6.2 and also 21.3.0):

❯ curl localhost:8000/foo/bar/
{"hello":"bar"}%                                                                                   
❯ curl localhost:8000/foo/
{"hello":""}% 

Environment (please complete the following information):

• OS: macOS Catalina 10.15.7
• Version: Sanic 21.6.2
• Python 3.7.9

Apologies if this should be on sanic_routing - I don't really understand enough about the router to know!

[Tronic]

@samdbmg This is probably a bug in one of the recently done router changes. I am away from my dev machine and cannot try myself, but could you check if setting Sanic(..., strict_slashes=True) or including/omitting the trailing slash has any effect on that?

[ahopkins]

This is not a bug, at least not in the current version. If anything, it was a bug that existed prior to 21.3.

When you define a route like this:

@app.route('/foo/')

Sanic will look at that and take off the trailing slash because it is ambiguous. Therefore, when it is matching a request, it is looking for something that has only one segment in the path: foo. When you then hit the endpoint with a trailing slash: /foo/, Sanic now needs to decide what to do. First it will try and match the route as is. Since it cannot find a match, it will try it without the trailing slash. Lo and behold, there is a match, so it will respond.

Now, when you happen to have another endpoint like this that is looking for a 2 segments:

@app.route('/foo/<var>/')

The game changes. On the first pass through, Sanic finds and endpoint that matches. When you hit the endpoint as /foo/, it is really two segments there: ("foo", "",). When there is a potential match, Sanic will choose that.

If you really want /foo/ to match the test handler and not myreq, you should do this:

@app.route("/foo/", strict_slashes=True)
async def test(request):
    return json({"hello": "world"})


@app.route("/foo/<var>/")
async def myreq(request, var):
    return json({"hello": var})

The problem with this is that you now will NOT match on: /foo:

$ curl localhost:9999/foo
⚠️ 404 — Not Found
==================
Requested URL /foo not found

To correct this:

@app.route("/foo")
@app.route("/foo/", strict_slashes=True)
async def test(request):
    return json({"hello": "world"})

$ curl localhost:9999/foo
{"hello":"world"}

$ curl localhost:9999/foo/
{"hello":"world"}

$ curl localhost:9999/foo/qqq
{"hello":"qqq"}

Anytime that you start adding trailing slashes to a request, you need to be careful. There is an RFC that discusses this in greater detail if you are interested: https://datatracker.ietf.org/doc/html/rfc3986/#section-6 Also, there is the older: https://datatracker.ietf.org/doc/html/rfc2616#section-3.2.3

A trailing slash on a request is significant. It should be treated as such. Here is another explanation I gave that may be relevant/helpful: https://community.sanicframework.org/t/route-paths-how-do-they-work/825

Because this is intended and expected behavior, I am closing the issue. Please feel free to continue the conversation if this answer is not satisfactory.

[ahopkins]

FWIW, there is also an alternative. And, this may or may not work for you depending upon what you expected @app.route("/foo/<var>/") to match.

By default, this:

@app.route("/foo/<var>/")

is interpreted as:

@app.route("/foo/<var:str>/")

If you change it to a different type (for example `alpha)

@app.route("/foo/<var:alpha>/")

Then, on the first pass through of matching /foo/, it will not matching because an empty string "" is not alphabetic characters only. You can try some of the other types, or add a custom regex if you prefer. If that first pass does not match myreq, then it would fall through to a second pass and match test (assuming no strict slashes).

See User Guide for more

[Tronic]

Another example of why strict_slashes should always be enabled (in new projects). Maybe need to put some emphasis on that on the manual?

I personally don't ever expect web servers to do what the strict_slashes=False setting does. It could make sense to redirect a non-slashed request on a folder name to an URL with a trailing slash, but never to return the folder contents on an URL with a missing trailing slash. Also, the whole problem should exist only for manually typed in URLs.

[samdbmg]

Brill, cheers for the detailed explanation, and clarifying that the old behaviour was the bug, rather than the new. I'd be inclined to agree with @Tronic that the manual should probably spend a few words on the implications of strict_slashes/some form of the very useful explanation in #2239 (comment) - it's quite hard to find the forum post from cold!
If nobody beats me to it I'll drop a docs PR in when I get some time.