cannot override HTTPException #985

Closed
mmerickel opened this Issue Apr 17, 2013 · 4 comments

Projects

None yet

3 participants

@mmerickel
Member

Pyramid's default exception views are registered for WSGIHTTPException which subclasses HTTPException. This is unfortunate because WSGIHTTPException is not a public api, and HTTPException is, but you cannot add an exception view for the latter because it is less-specific than the default view.

The inheritance hierarchy here is not obvious, nor logical, if someone wants to catch the pyramid.httpexceptions classes within their API.

@mmerickel
Member

Tossed a test case up for completeness.

from pyramid.config import Configurator

from pyramid.httpexceptions import HTTPBadRequest
from pyramid.httpexceptions import HTTPException

from pyramid.view import view_config
from pyramid.view import notfound_view_config


@view_config(context=Exception, renderer='string')
def exc_view(exc, request):
    return 'exc view'

@view_config(context=HTTPException, renderer='string')
def httpexc_view(exc, request):
    return 'http exc view'

@notfound_view_config(renderer='string')
def notfound_view(exc, request):
    return 'not found'

@view_config(route_name='badrequest')
def badrequest_view(request):
    # improperly invokes default exc view, not one of our overrides
    raise HTTPBadRequest

@view_config(route_name='exc')
def myexc_view(request):
    # properly invokes our overridden exc_view
    raise Exception

def main(global_config, **settings):
    """ This function returns a Pyramid WSGI application.
    """
    config = Configurator(settings=settings)

    config.add_route('badrequest', '/badrequest')
    config.add_route('exc', '/exc')

    config.scan()
    return config.make_wsgi_app()

if __name__ == '__main__':
    from wsgiref.simple_server import make_server

    app = main({})

    server = make_server('0.0.0.0', 8080, app)
    server.serve_forever()
@bertjwregeer
Member

What downsides are there to changing the default exception view to use HTTPException going forward? Would this change break anything for backwards compatibility?

@mmerickel
Member

I'm having difficulty reproducing this in a unit test. My example pasted above fails as advertised, however the following is not triggering a failure.

diff --git a/pyramid/tests/pkgs/exceptionviewapp/__init__.py b/pyramid/tests/pkgs/exceptionviewapp/__init__.py
index f169e0c..f565c01 100644
--- a/pyramid/tests/pkgs/exceptionviewapp/__init__.py
+++ b/pyramid/tests/pkgs/exceptionviewapp/__init__.py
@@ -3,8 +3,9 @@ def includeme(config):
     config.add_route('route_raise_exception2', 'route_raise_exception2',
                      factory='.models.route_factory')
     config.add_route('route_raise_exception3', 'route_raise_exception3',
-                    factory='.models.route_factory2')
+                     factory='.models.route_factory2')
     config.add_route('route_raise_exception4', 'route_raise_exception4')
+    config.add_route('route_raise_500', 'route_raise_500')
     config.add_view('.views.maybe')
     config.add_view('.views.no', context='.models.NotAnException')
     config.add_view('.views.yes', context=".models.AnException")
@@ -21,3 +22,6 @@ def includeme(config):
                     route_name='route_raise_exception4')
     config.add_view('.views.whoa', context='.models.AnException',
                     route_name='route_raise_exception4')
+    config.add_view('.views.raise_500', route_name='route_raise_500')
+    config.add_view('.views.whoa', context=Exception,
+                    route_name='route_raise_500')
diff --git a/pyramid/tests/pkgs/exceptionviewapp/views.py b/pyramid/tests/pkgs/exceptionviewapp/views.py
index 33b9767..97c9df7 100644
--- a/pyramid/tests/pkgs/exceptionviewapp/views.py
+++ b/pyramid/tests/pkgs/exceptionviewapp/views.py
@@ -1,3 +1,4 @@
+from pyramid.httpexceptions import HTTPInternalServerError
 from webob import Response
 from .models import AnException

@@ -15,3 +16,6 @@ def whoa(request):

 def raise_exception(request):
     raise AnException()
+
+def raise_500(request):
+    raise HTTPInternalServerError
diff --git a/pyramid/tests/test_integration.py b/pyramid/tests/test_integration.py
index eda4ae9..25ad854 100644
--- a/pyramid/tests/test_integration.py
+++ b/pyramid/tests/test_integration.py
@@ -465,6 +465,10 @@ class TestExceptionViewsApp(IntegrationBase, unittest.TestCase):
         res = self.testapp.get('/route_raise_exception4', status=200)
         self.assertTrue(b'whoa' in res.body)

+    def test_route_raise_httpexception(self):
+        res = self.testapp.get('/route_raise_500', status=200)
+        self.assertTrue(b'whoa' in res.body)
+
 class TestConflictApp(unittest.TestCase):
     package = 'pyramid.tests.pkgs.conflictapp'
     def _makeConfig(self):
@mcdonc mcdonc added a commit that closed this issue Aug 30, 2013
@mcdonc mcdonc allow exception view registrations for HTTPException to override defa…
…ult exception view; closes #985
97ed56d
@mcdonc mcdonc closed this in 97ed56d Aug 30, 2013
@mcdonc
Member
mcdonc commented Aug 30, 2013

The issue was that an registration of an exception view for an interface attached to a subclass binds more tightly than a registration of an exception view for a superclass. I fixed it by changing the inheritance structure of httpexceptions, getting rid of WSGIHTTPException entirely and calling it HTTPException (with a bw compat shim).

@mcdonc mcdonc added a commit that referenced this issue Aug 30, 2013
@mcdonc mcdonc backport fix for #985 to 1.4-branch c455ea6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment