strict_slashes is not respected from the application level

[chenjr0719]

Describe the bug
This issue is porting from sanic-org/sanic-openapi#102. When using Sanic(strict_slashes=True), it won't be apply to any blueprint.

Code snippet

from sanic import Sanic, Blueprint
from sanic.response import text

app = Sanic(strict_slashes=True)


bp = Blueprint("bp")


@bp.get("/test")
async def test(request):
    return text("test")


app.blueprint(bp)

for route in app.router.routes_all:
    print(route)

Output:

/test
/test/

Expected behavior
The strict_slashes in app level should be respected until user override it by setting Blueprint(..., strict_slashes=True) or in blueprint's route level.

Environment (please complete the following information):

• OS: Ubuntu 18.04
• Version: master(e36f398)

Additional context
None

[ahopkins]

I'm working on a new router idea that would, among other things, address this. So unless anyone else wants to address this separately, I'll have a fix for this in my proposed router.

[chenjr0719]

Cool, expecting your awesome work.

[ahopkins]

@chenjr0719.... the pressure is on 😱

[chenjr0719]

@ahopkins Oops, Please don't feel any pressure. If there are any help I can provide, please let me know. 😉

[ahopkins]

@chenjr0719 ;-) Was just joking. When I am done with the POC for it (probably not till next week at the earliest), I will definitely be looking for as much feedback and thoughts as possible since it will potentially be a big change (I am trying to make sure there will not be any API changes though).

[harshanarayana]

@chenjr0719 @ahopkins I think we might have to think a bit more on this specific issue. I managed to fix this with minor code changes. But this can be a breaking change in some cases.

app = Sanic()

@app.get("/test", strict_slashes=True)
def test(request):
    return text("OK")

assert app.test_client.get("/test")[1].status == 200

This was a valid behavior previously. But considering that there is a strict_slashes attribute, the behavior seems odd and needs to be fixed. Only /test/ should be a valid URL that will return 200 and /test should return 404 instead.

Also, what order do we take into account while overriding the strict_slashes behavior? i.e. Which takes priority?

app = Sanic(strict_slashes=True)

@app.get("/test")
def test(request):
    return  text("Test")

bp = Blueprint("bp")

@bp.get("/one", strict_slashes=False)
def one(request):
    return text("one")

@bp.get("/second")
def second(request):
    return text("second")

app.blueprint(bp)

print(app.router.route_all)

bp2 = Blueprint("bp2", strict_slashes=False)

@bp2.get("/third")
def third(request):
    return text("third")

What is the expected behavior in the above case?

[ahopkins]

@harshanarayana I agree, which is sort of why I am not sure we tackle it separate from any other router changes we want to make. I think it would be best to make them all at once.

[harshanarayana]

@ahopkins Works for me. However, I just opened a new PR with WIP tag to address this + the other dependency issue. Take a look when you can and let me know your feedback. It's pending few documentation changes

@chenjr0719

[chenjr0719]

@harshanarayana

What I think is like this hierarchy structure.

Application level - True/False(default)
├── Route level - None(default)/True/False
└── Blueprint level - None(default)/True/False
        └── BP's route level - None(default)/True/False

So, in the Python world, it would be:

# route
if route.strict_slashes is None:
    route.strict_slashes = app.strict_slashes

# blueprint route
if bp_route.strict_slashes is None:
    if bp.strict_slashes is None:
        bp_route.strict_slashes = app.strict_slashes
    else:
        bp_route.strict_slashes = bp.strict_slashes
    

Now, back to your cases:

from sanic import Sanic, Blueprint
from sanic.response import text

app = Sanic(strict_slashes=True)


@app.get("/test")
def test(request):
    return text("Test")


# this should follow app level
assert app.test_client.get("/test")[1].status == 200
assert app.test_client.get("/test/")[1].status == 404

bp = Blueprint("bp")


@bp.get("/one", strict_slashes=False)
def one(request):
    return text("one")


# this should follow bp route level
assert app.test_client.get("/one")[1].status == 200
assert app.test_client.get("/one/")[1].status == 200


@bp.get("/second")
def second(request):
    return text("second")


# this should follow bp level, but the bp level is None, so follow app level
assert app.test_client.get("/second")[1].status == 200
assert app.test_client.get("/second/")[1].status == 404

app.blueprint(bp)

bp2 = Blueprint("bp2", strict_slashes=False)


@bp2.get("/third")
def third(request):
    return text("third")


# this should follow bp level
assert app.test_client.get("/third")[1].status == 200
assert app.test_client.get("/third/")[1].status == 404

What do you think?

[harshanarayana]

@chenjr0719 Let me get back to you on this by EOD today. I am caught in the middle of a training session and haven't been able to look at it.

[ahopkins]

@chenjr0719 That looks awesome. I agree with the results of each. Without having the test cases in front of me, there are a few permutations that are still not accounted for in your layout. We should make sure to test them all for consistency.

[harshanarayana]

@ahopkins @chenjr0719

Application level - True/False(default)
├── Route level - None(default)/True/False
└── Blueprint level - None(default)/True/False
└── BP's route level - None(default)/True/False

In most of the places, the default argument is None but we override them to False as the first action inside the method. Provided I change that, the precedence of check can be as follows

route level 
|___ Blueprint level
       |__ App Level 

When deciding the behavior for the strict_slashes, we traverse the above order and if we find a non None value at any level, then we take that as the actual value for that specific route.

def _check_if_strict_slash_should_be_enforced(slash_options):
    for op_type in ["route", "bp", "app"]:
        if slash_options.get(op_type) is not None:
            return slash_options.get(op_type)
    else:
        return False

Case 1

app = Sanic(strict_slashes=True)

@app.get("/test")
def test(request):
    return text("Test")

assert app.test_client.get("/test")[1].status == 200  # I think this behavior should be 404 
assert app.test_client.get("/test/")[1].status == 404 # I think this behavior should be 200

In above, I think the /test endpoint needs to get a 404 and /test/ needs to get a 200. Since the strict_slashes is enforced at the app level, even if the route registration is missing the trailing /, it can be added while route registering. Otherwise, the app level strict_slashes will not have any value.

Case 2

app = Sanic(strict_slashes=True)
bp = Blueprint("bp")

@bp.get("/one", strict_slashes=False)
def one(request):
    return text("one")

# this should follow bp route level
assert app.test_client.get("/one")[1].status == 200
assert app.test_client.get("/one/")[1].status == 200

Agree on this. The strict_slashes at the BP Route level takes over and enforces the non strict_slashes behavior.

Case 3

app = Sanic(strict_slashes=True)

bp2 = Blueprint("bp2", strict_slashes=False)

@bp2.get("/third")
def third(request):
    return text("third")

app.blueprint(bp2)

# this should follow bp level
assert app.test_client.get("/third")[1].status == 200 
assert app.test_client.get("/third/")[1].status == 404 # I think this should return 200 as well. 

In above case the Blueprint level's strict_slashes should come into play causing the registration to follow the non strict_slashes behavior and both /third and /third/ should be valid.

General Items

1. If there is a strict_slashes=True anywhere in the hierarchy that we ultimately decide upon, every URL needs to terminate with a trailing / irrespective of how they are represented in the Route level.
2. If you want to override the strict_slashes behavior you can individually do that either at the route or BP level while creating the routes
3. strict_slashes once applied at the BP or Application level are set for the lifetime of the object. So technically, DO NOT change this value once decided. You can override them individually at route level as required.

[chenjr0719]

@harshanarayana

2. If you want to override the strict_slashes behavior you can individually do that either at the route or BP level while creating the routes

3. strict_slashes once applied at the BP or Application level are set for the lifetime of the object. So technically, DO NOT change this value once decided. You can override them individually at route level as required.

Totally agree both.

1. If there is a strict_slashes=True anywhere in the hierarchy that we ultimately decide upon, every URL needs to terminate with a trailing / irrespective of how they are represented in the Route level.

I don't think this is strict_slashes supposed to do. At current implementation, It depends on the uri of the route.

When the strict_slashes=False, no matter your uri ends with slash or not, it will create two endpoints, one with slash, another one without slash.

app = Sanic(strict_slashes=False)


@app.get("/test")
def test(request):
    return text("Test")


assert app.test_client.get("/test")[1].status == 200
assert app.test_client.get("/test/")[1].status == 200

When the strict_slashes=True and the uri ends without slash, there will be only one endpoint /test be created.

app = Sanic(strict_slashes=True)


@app.get("/test")
def test(request):
    return text("Test")


assert app.test_client.get("/test")[1].status == 200
assert app.test_client.get("/test/")[1].status == 404

In the other hand, when the uri ends with slash, it will be opposite to the above case.

app = Sanic(strict_slashes=True)


@app.get("/test/")
def test(request):
    return text("Test")


assert app.test_client.get("/test")[1].status == 404
assert app.test_client.get("/test/")[1].status == 200

I like your idea about when strict_slashes=True all uri should ends with slash. It can simplify most cases. But, it also changes the behavior of strict_slashes totally. I'm afraid it will cause a big impact to users.

I suggest to separate current discussion to two parts, one is about the override mechanism, another is how strict_slashes works.

1. About the override mechanism, I think we both agree it should be route -> blueprint -> application.
2. About how strict_slashes works, I have to say that we need more time and feedback to determine should we change it's behavior or not. And I think this might also related to url_for() doesn't return a working URI for a route with the trailing slash and strict_slashes=True #1420.

[ahopkins]

Yes, it would be a change in behavior. Which leads to hesitancy and reluctance to make a change. However, it also seems a bit counter intuitive.Besides the consistency aspect, if there is to be a change in behavior, we should do a survey of other frameworks. Being intuitive is also important.

[harshanarayana]

@chenjr0719 @ahopkins Agreed. I am changing the original PR to only include the changes required for addressing the strict_slashes option inheritance against the defined hierarchy.

As for the change of default behavior implemented currently, let me move that discussion to the thread in community and see what comes out of it.

[ahopkins]

Closing this. Any further discussion, let's move to forums.