url_for() doesn't return a working URI for a route with the trailing slash and strict_slashes=True

[hatarist]

Describe the bug
Say we have a route with the trailing slash and strict_slashes=True:
@app.route('/endpoint/', strict_slashes=True)
this way, url_for() will always trim the trailing slash, returning a non-working URL.

Code snippet

from sanic import Sanic
from sanic.response import html

app = Sanic()

# @app.route('/endpoint', strict_slashes=True)
@app.route('/endpoint/', strict_slashes=True)
async def endpoint(request):
    return html('OK')


@app.route('/')
async def index(request):
    # print(request.app.router.routes_all)
    return html('<a href="' + request.app.url_for('endpoint') + '">endpoint</a>')


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

Expected behavior
In my example, request.app.url_for('endpoint') is expected to return '/endpoint/'.

Environment (please complete the following information):

• Version 0.8.3

Additional context
The current implementation of url_for() just blindly removes the trailing slash, see app.py, lines 617-618.

[ahopkins]

I confirmed the behavior. @hatarist Do you think you could take a swing at a PR?

[vltr]

To be honest, I was never a fan of the strict_slashes option ... But I know this can lead to large discussions, so I won't discuss that.

My only concern is that the Sanic router doesn't actually save on Router.add the value of the strict_slashes parameter, which can be set manually (as a parameter, exactly like @hatarist example) or, if None, assume the value of app.strict_slashes, that we may not know if it was overwritten by the parameter.

IMHO, to fix this, we might need to add more complexity to the router (saving the actual value of strict_slashes to the Route namedtuple) 😬

As a matter of fact, looking closely, I don't think the Router may even care about the strict_slashes option. I need to take a closer look at this, in the router level, to understand what this means ... But this just doesn't feels right 😐

[harshanarayana]

@vltr I agree with you on the amount of changes required to address this the right way. However, the current implementation contradicts with the strict_slashes behavior that is already enforced on other parts of the sanic framework.

I believe it might be a good idea to move the Route namedtuple into a class of it's own with __slots__ so that we can add some helpful features into it which includes handling the strict_slashes etc.

When I was looking at the router to add this comment, I noticed that the current implementation we have for add(self, uri, methods, handler, host=None, strict_slashes=False, version=None, name=None) is a bit confusing to understand and IMHO, it can be simplified to make it more readable and easy to modify as part of development if required.

[ahopkins]

I believe it might be a good idea to move the Route namedtuple into a class of it's own with slots so that we can add some helpful features into it which includes handling the strict_slashes etc.

+1 I like this idea @harshanarayana

Without sidetracking this conversation too much ... Let's open this discussion on the forums about what to do with the router. The December release is coming up fast, and if we want to have a chance at putting in a new router implementation (or resolving some of the existing issues) by March, let's make some decisions soon.

[harshanarayana]

@ahopkins @vltr If everyone is okay with it, the quick way to fix this issue without changing too much and the core router implementation is to remove the forced removal of the trailing / in the url_for method.
If not that, then we should be able to leverage the app.strict_slashes. However, this means we are enforcing the strict_slashes directly at the app level rather than the route level.

The last and not so good way to fix is to leverage the **kwargs param provided in the url_for. Take an additional argument strict_slashes and based on this handle the return URL the right way.

[vltr]

I agree with @ahopkins to take this discussion to the community forums. We can also take the discussion on #1386 regarding routing there as well - @hatarist I think you will be onboard since you gave a lot of insigts on the related issue. Just a couple of notes here:

However, the current implementation contradicts with the strict_slashes behavior that is already enforced on other parts of the sanic framework.

Indeed, this is something that will need a lot of attention to fix "sanic-wise".

I believe it might be a good idea to move the Route namedtuple into a class of it's own with __slots__ so that we can add some helpful features into it which includes handling the strict_slashes etc.

That I couldn't agree more. Interacting with the Route itself would need some careful work because we have Blueprints and etc, but in general I really like this idea and it would help to decouple the Router logic - which I would love to see specially with a proper interface and without bouncing methods here and there - see #1317 and PR #1373 as examples of technical debts within the Router).

... the quick way to fix this issue without changing too much and the core router implementation is ...

I don't think we should do any quick fix presuming this or that because it will eventually fail 😔

The last and not so good way to fix is to leverage the **kwargs param provided in the url_for.

I don't like this option either: the developer will end up setting strict_slashes all over the place for a design choice and, when properly fixed, this will still hang on a lot of codes as the correct way to do it.

[myusko]

@ahopkins Anyone already is working on it? if not, I'll be happy to work on it.

[vltr]

@MichaelYusko I think we need to take this matter to the forums for a better discussion, because the "correct way" of fixing this is way bellow the url_for method ...

[myusko]

@vltr Thanks, okay will jump into the forum;)

[vltr]

@MichaelYusko actually we need to create that discussion in the forum ... Oh my. Sorry, my head is over the clouds today.

[vltr]

I'll start the discussion and post the link here ASAP ...

[myusko]

@vltr Gotcha, no worries;) start of the week is always hard ;-)

[vltr]

@MichaelYusko yeah, sort of ... Busy day in here.

Also, @hatarist @ahopkins @harshanarayana take a look at the community forums for that matter.

[myusko]

@vltr which a status of the issue?

[ahopkins]

@5onic No one is working on anything router related right now (as far as I know... after I am done with ASGI, I want to turn some attention to it). Feel free to make the PR. This is also a question whether @huge-success/sanic-core-devs think this is something that we should also patch 18.12LTS.

[myusko]

@ahopkins Cool, I'll start work on it in 3-4 days.

[Tronic]

What happened to this? Trailing slashes still go missing.