append_slash_notfound_view might should use request.invoke_subrequest instead of request.redirect

[mcdonc]

Right now the append slash notfound view redirects to the right URL, which means the redirected request is always a GET request. If we used invoke_subrequest instead, the verb could be maintained.

[mmerickel]

This would be bw-incompat technically because it would allow serving the content from 2 different urls instead of telling the browser which is the canonical url.

[digitalresistor]

I'm a 👎 on invoking a sub request because the whole point of doing the redirect is to provide the browser/end-user with the canonical URL.

[mcdonc]

Devils' advocate: folks already configure the same view to be invoked via two separate URLs, and they find that useful. I'm not sure how this differs.

[sontek]

As someone who builds applications that are primarily behind authentication and require no SEO there is no benefit to me for differentiating between having a trailing / or not so I usually just use regex to allow an optional slash.

My biggest complaints for append_slash is that it is a global setting on the exception view so I can't decide when to allow the slash and when not to and it doesn't maintain VERB. What I'd love to see is more something like this:

    config.add_route('hello', '/hello/{name}', slash_optional=True)

and then have it generate the route automatically:

/hello/{name}{optional_slash:/?}

The caveat there is then you need to have a pregenerator attached to set a default value for optional_slash. I've found this approach to works pretty well and 100% opt-in which is nice.

[sontek]

As the minimal fix here, I'd like to see this raise an exception instead of failing blindly:

from wsgiref.simple_server import make_server
from pyramid.config import Configurator
from pyramid.response import Response
from pyramid.config.views import ViewDeriverInfo
from pyramid.httpexceptions import HTTPNotFound


def validate_view(view, info: ViewDeriverInfo):
    schema = info.options.get('schema')
    print("setting up view deriver")

    def wrapper(context, request):
        print("running the view derver!")

        if request.json.get('value') != schema:
            raise Exception("Validation Error")
        else:
            print("Validation passed")
            return view(context, request)

    if schema:
        return wrapper

    return view

validate_view.options = ('schema',)


def notfound(request):
    return HTTPNotFound('Not found, bro.')


def hello_world(request):
    return Response('Hello %(name)s!' % request.json)


def exception(request):
    print(request.exception)
    return Response('Error :(')


if __name__ == '__main__':
    config = Configurator()
    config.add_view_deriver(validate_view)
    config.add_route('hello', '/hello/')
    config.add_view(hello_world, route_name='hello', request_method='POST', schema=1)
    config.add_notfound_view(notfound, append_slash=True)
    config.add_view(exception, context=Exception)
    app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 8080, app)
    print('http://127.0.0.1:8080/hello/sontek')
    server.serve_forever()

If you hit that app in a way that would require a slash redirect:

curl -L -X POST -d '{"name": "sontek", "value": 1}' -H "Content-Type: application/json" http://localhost:8080/hello;echo

bad things happen and Pyramid doesn't protect you against it

[mmerickel]

Regarding the original issue, Pyramid's current behavior is consistent with Django so I'm hesitant to change it. https://docs.djangoproject.com/en/1.9/ref/settings/#append-slash

[mcdonc]

Yeah just trying to avoid the "Note that the redirect may cause any data submitted in a POST request to be lost" caveat in those docs.

[sontek]

It doesn't work like Django's. Django doesn't attempt to append the slash on POST it raises an exception when you hit a route with POST that doesn't end in /. It at least used to. I haven't used Django in years.

[mmerickel]

Sure so we could stop appending slashes on non-HEAD/GET requests. Is that what you mean?

[sontek]

@mmerickel Yeah, run that example I included above to see the experience you get with it appending to POST

[mmerickel]

Interesting. It looks like curl is behaving oddly here. Pyramid returns a 302 and curl follows the redirect but it does not switch the method to GET (which is what most browsers do, despite the rfc). Instead curl drops the body but uses a POST. The behavior works as expected with the following patch to return a 307 instead of 302 (explicitly tell client to re-use POST):

diff --git a/pyramid/view.py b/pyramid/view.py
index 88c6397..2dc8da6 100644
--- a/pyramid/view.py
+++ b/pyramid/view.py
@@ -22,6 +22,7 @@ from pyramid.exceptions import PredicateMismatch
 from pyramid.httpexceptions import (
     HTTPFound,
     HTTPNotFound,
+    HTTPTemporaryRedirect,
     default_exceptionresponse_view,
     )

@@ -275,7 +276,7 @@ class AppendSlashNotFoundViewFactory(object):
     .. deprecated:: 1.3

     """
-    def __init__(self, notfound_view=None, redirect_class=HTTPFound):
+    def __init__(self, notfound_view=None, redirect_class=HTTPTemporaryRedirect):
         if notfound_view is None:
             notfound_view = default_exceptionresponse_view
         self.notfound_view = notfound_view

[mmerickel]

First, a custom append_slash is already pretty easily achievable in several ways.

• You can supply an alternate class as the redirect.
• You can attach different notfound views to different routes/predicates if you need different logic for certain views.
• You can use your own decorator on the notfound view instead of append_slash. This decorator could easily perform a subrequest.

I'm closing this because 1) I haven't seen a consensus on what people want to happen versus the current default and 2) there are so many workarounds to achieving various alternatives.

[jvanasco]

I'm a big 👎 unless this is configurable and can be avoided, or this functionality is offered as a second option.

We have to utilize the appended slash factory on GET requests with a CMS, because many people will try to make a link pretty and share it on social media or a blogpost/article without a trailing slash. This happens a lot with writers/authors who do not understand that links with and without the slash are uniquely different. The append_slash corrects that mistake with a redirect when people visit it. When they click bookmark/etc it happens on the correct url.

In the case of a POST to the CMS, the request is either originating with:

• A form submission on our website
• A developer making an API request

In the first case, an internal form is pointing to the wrong page and should be caught by a unit test and corrected in the template.

In the second case, either the developer made a mistake, the documentation made a mistake, or the API changed. Across all those scenarios, the standard/expected response is redirecting someone to the GET version of the endpoint -- which might show an error code and would populate the client's request history with a redirect notifying them of the change.

While, I can quickly reimplement the old behavior with a custom factory (that was the original factory), the suggested functionality is essentially summarized as "instead of notifying browsers/clients their request was wrong, support the malformed request as being correct."

[mcdonc]

FWIW, the append_slash_notfound_factory was not created to do a redirect and lose the verb intentionally; we just didn't have subrequests at the time.

What would be the worst case scenario if it did a subrequest on a POST and the request worked instead of raising a 404?

[mcdonc]

A compromise: leave the default behavior as-is (append_slash=True as an arg to add_notfound_view will do a 302 redirect). And preserve the current behavior where passing something that implements IResponse as append_slash will use that class as a factory for the redirect. But also make it possible to configure a notfound view to do a subrequest which will append the slash instead of returning a redirect response:

from pyramid.view import UseSubrequest, notfound_view_config
from pyramid.response import Response

@notfound_view_config(append_slash=UseSubrequest)
def foo_notfound(request):
    return Response(not found')

In this configuration, if someone hits a URL that would 404, it will first check the routing table for a slash-appended route, and if it finds one, it'll do something like:

                        subreq = request.copy()
                        subreq.path_info = request.path + '/'
                        return request.invoke_subrequest(
                            subreq,
                            use_tweens=True
                        )

Instead of the default codepath, which is something like:

                        qs = request.query_string
                        if qs:
                            qs = '?' + qs
                        return self.redirect_class(
                            location=request.path + '/' + qs
                        )

[mcdonc]

See #3311 ; reopening.

[jvanasco]

Keeping the existing behavior as-is and facilitating this new functionality with a new kwarg on setup is a great solution!

[mcdonc]

HTTP response code 307 (and the ability to configure the append-slash machinery to use it) makes this moot.