Merge pull request #3266 from pallets/internal-server-error-type

always pass InternalServerError instance to 500 handler

CHANGES.rst

@@ -11,6 +11,17 @@ Unreleased
 
 -   Bump minimum Werkzeug version to >= 0.15.
 -   Drop support for Python 3.4.
+-   Error handlers for ``InternalServerError`` or ``500`` will always be
+    passed an instance of ``InternalServerError``. If they are invoked
+    due to an unhandled exception, that original exception is now
+    available as ``e.original_exception`` rather than being passed
+    directly to the handler. The same is true if the handler is for the
+    base ``HTTPException``. This makes error handler behavior more
+    consistent. :pr:`3266`
+
+    -   :meth:`Flask.finalize_request` is called for all unhandled
+        exceptions even if there is no ``500`` error handler.
+
 -   :meth:`flask.RequestContext.copy` includes the current session
     object in the request context copy. This prevents ``session``
     pointing to an out-of-date object. (`#2935`_)

docs/errorhandling.rst

@@ -139,10 +139,94 @@ raises the exception. However, the blueprint cannot handle 404 routing errors
 because the 404 occurs at the routing level before the blueprint can be
 determined.
 
-.. versionchanged:: 0.11
 
-   Handlers are prioritized by specificity of the exception classes they are
+Generic Exception Handlers
-   registered for instead of the order they are registered in.
+``````````````````````````
+
+It is possible to register error handlers for very generic base classes
+such as ``HTTPException`` or even ``Exception``. However, be aware that
+these will catch more than you might expect.
+
+An error handler for ``HTTPException`` might be useful for turning
+the default HTML errors pages into JSON, for example. However, this
+handler will trigger for things you don't cause directly, such as 404
+and 405 errors during routing. Be sure to craft your handler carefully
+so you don't lose information about the HTTP error.
+
+.. code-block:: python
+
+    from flask import json
+    from werkzeug.exceptions import HTTPException
+
+    @app.errorhandler(HTTPException)
+    def handle_exception(e):
+        """Return JSON instead of HTML for HTTP errors."""
+        # start with the correct headers and status code from the error
+        response = e.get_response()
+        # replace the body with JSON
+        response.data = json.dumps({
+            "code": e.code,
+            "name": e.name,
+            "description": e.description,
+        })
+        response.content_type = "application/json"
+        return response
+
+
+An error handler for ``Exception`` might seem useful for changing how
+all errors, even unhandled ones, are presented to the user. However,
+this is similar to doing ``except Exception:`` in Python, it will
+capture *all* otherwise unhandled errors, including all HTTP status
+codes. In most cases it will be safer to register handlers for more
+specific exceptions. Since ``HTTPException`` instances are valid WSGI
+responses, you could also pass them through directly.
+
+.. code-block:: python
+
+    from werkzeug.exceptions import HTTPException
+
+    @app.errorhandler(Exception)
+    def handle_exception(e):
+        # pass through HTTP errors
+        if isinstance(e, HTTPException):
+            return e
+
+        # now you're handling non-HTTP exceptions only
+        return render_template("500_generic.html", e=e), 500
+
+Error handlers still respect the exception class hierarchy. If you
+register handlers for both ``HTTPException`` and ``Exception``, the
+``Exception`` handler will not handle ``HTTPException`` subclasses
+because it the ``HTTPException`` handler is more specific.
+
+Unhandled Exceptions
+````````````````````
+
+When there is no error handler registered for an exception, a 500
+Internal Server Error will be returned instead. See
+:meth:`flask.Flask.handle_exception` for information about this
+behavior.
+
+If there is an error handler registered for ``InternalServerError``,
+this will be invoked. As of Flask 1.1.0, this error handler will always
+be passed an instance of ``InternalServerError``, not the original
+unhandled error. The original error is available as ``e.original_error``.
+Until Werkzeug 1.0.0, this attribute will only exist during unhandled
+errors, use ``getattr`` to get access it for compatibility.
+
+.. code-block:: python
+
+    @app.errorhandler(InternalServerError)
+    def handle_500(e):
+        original = getattr(e, "original_exception", None)
+
+        if original is None:
+            # direct 500 error, such as abort(500)
+            return render_template("500.html"), 500
+
+        # wrapped unhandled error
+        return render_template("500_unhandled.html", e=original), 500
+
 
 Logging
 -------

src/flask/app.py

@@ -1811,11 +1811,36 @@ class Flask(_PackageBoundObject):
         return handler(e)
 
     def handle_exception(self, e):
-        """Default exception handling that kicks in when an exception
+        """Handle an exception that did not have an error handler
-        occurs that is not caught.  In debug mode the exception will
+        associated with it, or that was raised from an error handler.
-        be re-raised immediately, otherwise it is logged and the handler
+        This always causes a 500 ``InternalServerError``.
-        for a 500 internal server error is used.  If no such handler
+
-        exists, a default 500 internal server error message is displayed.
+        Always sends the :data:`got_request_exception` signal.
+
+        If :attr:`propagate_exceptions` is ``True``, such as in debug
+        mode, the error will be re-raised so that the debugger can
+        display it. Otherwise, the original exception is logged, and
+        an :exc:`~werkzeug.exceptions.InternalServerError` is returned.
+
+        If an error handler is registered for ``InternalServerError`` or
+        ``500``, it will be used. For consistency, the handler will
+        always receive the ``InternalServerError``. The original
+        unhandled exception is available as ``e.original_exception``.
+
+        .. note::
+            Prior to Werkzeug 1.0.0, ``InternalServerError`` will not
+            always have an ``original_exception`` attribute. Use
+            ``getattr(e, "original_exception", None)`` to simulate the
+            behavior for compatibility.
+
+        .. versionchanged:: 1.1.0
+            Always passes the ``InternalServerError`` instance to the
+            handler, setting ``original_exception`` to the unhandled
+            error.
+
+        .. versionchanged:: 1.1.0
+            ``after_request`` functions and other finalization is done
+            even for the default 500 response when there is no handler.
 
         .. versionadded:: 0.3
         """
@@ -1833,10 +1858,16 @@ class Flask(_PackageBoundObject):
                 raise e
 
         self.log_exception((exc_type, exc_value, tb))
-        handler = self._find_error_handler(InternalServerError())
+        server_error = InternalServerError()
-        if handler is None:
+        # TODO: pass as param when Werkzeug>=1.0.0 is required
-            return InternalServerError()
+        # TODO: also remove note about this from docstring and docs
-        return self.finalize_request(handler(e), from_error_handler=True)
+        server_error.original_exception = e
+        handler = self._find_error_handler(server_error)
+
+        if handler is not None:
+            server_error = handler(server_error)
+
+        return self.finalize_request(server_error, from_error_handler=True)
 
     def log_exception(self, exc_info):
         """Logs an exception.  This is called by :meth:`handle_exception`

tests/test_user_error_handler.py

@@ -6,6 +6,7 @@ tests.test_user_error_handler
 :copyright: © 2010 by the Pallets team.
 :license: BSD, see LICENSE for more details.
 """
+import pytest
 from werkzeug.exceptions import Forbidden
 from werkzeug.exceptions import HTTPException
 from werkzeug.exceptions import InternalServerError
@@ -25,7 +26,13 @@ def test_error_handler_no_match(app, client):
 
     @app.errorhandler(500)
     def handle_500(e):
-        return type(e).__name__
+        assert isinstance(e, InternalServerError)
+        original = getattr(e, "original_exception", None)
+
+        if original is not None:
+            return "wrapped " + type(original).__name__
+
+        return "direct"
 
     @app.route("/custom")
     def custom_test():
@@ -35,9 +42,14 @@ def test_error_handler_no_match(app, client):
     def key_error():
         raise KeyError()
 
+    @app.route("/abort")
+    def do_abort():
+        flask.abort(500)
+
     app.testing = False
     assert client.get("/custom").data == b"custom"
-    assert client.get("/keyerror").data == b"KeyError"
+    assert client.get("/keyerror").data == b"wrapped KeyError"
+    assert client.get("/abort").data == b"direct"
 
 
 def test_error_handler_subclass(app):
@@ -194,3 +206,84 @@ def test_default_error_handler():
     assert c.get("/forbidden").data == b"forbidden"
     # Don't handle RequestRedirect raised when adding slash.
     assert c.get("/slash", follow_redirects=True).data == b"slash"
+
+
+class TestGenericHandlers(object):
+    """Test how very generic handlers are dispatched to."""
+
+    class Custom(Exception):
+        pass
+
+    @pytest.fixture()
+    def app(self, app):
+        @app.route("/custom")
+        def do_custom():
+            raise self.Custom()
+
+        @app.route("/error")
+        def do_error():
+            raise KeyError()
+
+        @app.route("/abort")
+        def do_abort():
+            flask.abort(500)
+
+        @app.route("/raise")
+        def do_raise():
+            raise InternalServerError()
+
+        app.config["PROPAGATE_EXCEPTIONS"] = False
+        return app
+
+    def report_error(self, e):
+        original = getattr(e, "original_exception", None)
+
+        if original is not None:
+            return "wrapped " + type(original).__name__
+
+        return "direct " + type(e).__name__
+
+    @pytest.mark.parametrize("to_handle", (InternalServerError, 500))
+    def test_handle_class_or_code(self, app, client, to_handle):
+        """``InternalServerError`` and ``500`` are aliases, they should
+        have the same behavior. Both should only receive
+        ``InternalServerError``, which might wrap another error.
+        """
+
+        @app.errorhandler(to_handle)
+        def handle_500(e):
+            assert isinstance(e, InternalServerError)
+            return self.report_error(e)
+
+        assert client.get("/custom").data == b"wrapped Custom"
+        assert client.get("/error").data == b"wrapped KeyError"
+        assert client.get("/abort").data == b"direct InternalServerError"
+        assert client.get("/raise").data == b"direct InternalServerError"
+
+    def test_handle_generic_http(self, app, client):
+        """``HTTPException`` should only receive ``HTTPException``
+        subclasses. It will receive ``404`` routing exceptions.
+        """
+
+        @app.errorhandler(HTTPException)
+        def handle_http(e):
+            assert isinstance(e, HTTPException)
+            return str(e.code)
+
+        assert client.get("/error").data == b"500"
+        assert client.get("/abort").data == b"500"
+        assert client.get("/not-found").data == b"404"
+
+    def test_handle_generic(self, app, client):
+        """Generic ``Exception`` will handle all exceptions directly,
+        including ``HTTPExceptions``.
+        """
+
+        @app.errorhandler(Exception)
+        def handle_exception(e):
+            return self.report_error(e)
+
+        assert client.get("/custom").data == b"direct Custom"
+        assert client.get("/error").data == b"direct KeyError"
+        assert client.get("/abort").data == b"direct InternalServerError"
+        assert client.get("/not-found").data == b"direct NotFound"