Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change in Exception views handling should be reflected by docs #3670

Open
Sim175 opened this issue Jun 4, 2021 · 3 comments
Open

Change in Exception views handling should be reflected by docs #3670

Sim175 opened this issue Jun 4, 2021 · 3 comments

Comments

@Sim175
Copy link

Sim175 commented Jun 4, 2021

Ever since commit f9813e0, the default behavior is to let exceptions raised within exception views bubble up instead of attempting to handle them until a Response is returned. For example, you could previously expect to be able to raise HTTPFound in your HTTPForbidden handler to redirect to a login page; but now, the HTTPFound exception goes all the way up and causes a 500 error to be produced instead.
However, the docs still indicate that you should always be able to return or raise an HTTPException in any view callable :

"Instances of an HTTP exception object may either be returned or raised from within view code." (https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/views.html#using-special-exceptions-in-view-callables)

Specifically, the second example provided here (raise) wouldn't work in exception view code, while the first example (return) would produce a 302 response.

Suggestion: mention that exception view code should only return HTTP exceptions unless the default execution policy is overridden.

@quantum-omega
Copy link

Coworker of @Sim175 here.

Here's a minimal Pyramid project to reproduce the issue:

from wsgiref.simple_server import make_server
from pyramid.authorization import Authenticated
from pyramid.config import Configurator
from pyramid.response import Response
from pyramid.httpexceptions import HTTPSeeOther
from pyramid.security import Denied


def index(request):
    return Response('Hello World!')


def login(request):
    return Response('Authentication form would be here')


def forbidden(request):
    # This just generates an exception:
    raise HTTPSeeOther(location=request.route_url('login'))
    # Using this instead works as intended:
    return HTTPSeeOther(location=request.route_url('login'))


class DenyAllSecurityPolicy():
    def authenticated_userid(self, request):
        return None

    def forget(self, request, **kw):
        return []

    def identity(sel, request):
        return None

    def permits(self, request, context, permission):
        return Denied("Always denies")

    def remember(self, request, userid, **kw):
        return []


if __name__ == '__main__':
    with Configurator() as config:
        config.set_security_policy(DenyAllSecurityPolicy())
        config.add_route('index', '/')
        config.add_route('login', '/login')
        config.add_view(index, route_name='index', permission=Authenticated)
        config.add_view(login, route_name='login')
        config.add_forbidden_view(forbidden)
        app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 6543, app)
    server.serve_forever()

We don't disagree with how Pyramid handles things but prior to 2.0 this pattern used to work and the documentation suggests that it should.

@digitalresistor
Copy link
Member

If you are relying on this behavior and you are not using pyramid_retry (which I think still has this behavior) you could always create your own execution policy that re-introduces the old way of doing things.

Here is an example #3468 (comment) that shows how to create your own execution policy.

We could be more explicit in the documentation that exception views should return a valid response object, and exceptions raised from an exception won't be handled again. All the examples of exception views show the view returning a response already.

Related: #3468

@quantum-omega
Copy link

I can't think of a situation where a view couldn't be trivially changed to just use return instead of raise, but thanks for pointing us in the right direction if ever it becomes a problem.

We could be more explicit in the documentation that exception views should return a valid response object, and exceptions raised from an exception won't be handled again.

I think that this is exactly what is needed. Just doing that would prevent errors in usage as I didn't find anywhere where it was mentioned that exception views should behave differently than other views. These are the places where I feel that this information should be added:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants