Fix error handler for Flask 1.1.0

flask_rest_api/error_handler.py

@@ -1,6 +1,6 @@
 """Exception handler"""
 
-from werkzeug.exceptions import HTTPException, InternalServerError
+from werkzeug.exceptions import HTTPException
 from flask import jsonify, current_app
 
 
@@ -10,21 +10,21 @@ class ErrorHandlerMixin:
     def _register_error_handlers(self):
         """Register error handlers in Flask app
 
-        This method registers a default error handler for HTTPException.
+        This method registers a default error handler for ``HTTPException``.
         """
         self._app.register_error_handler(
             HTTPException, self.handle_http_exception)
 
     def handle_http_exception(self, error):
         """Return a JSON response containing a description of the error
 
-        This method is registered at app init to handle `HTTPException`.
+        This method is registered at app init to handle ``HTTPException``.
 
-        - When `abort` is called in the code, this triggers a `HTTPException`,
-          and Flask calls this handler to generate a better response.
+        - When ``abort`` is called in the code, an ``HTTPException`` is
+          triggered and Flask calls this handler.
 
-        - When an exception is not caught in a view, Flask automatically calls
-          the handler registered for error 500.
+        - When an exception is not caught in a view, Flask makes it an
+          ``InternalServerError`` and calls this handler.
 
         flask_rest_api republishes webargs's
         :func:`abort <webargs.flaskparser.abort>`. This ``abort`` allows the
@@ -37,19 +37,18 @@ def handle_http_exception(self, error):
         - `errors` (``dict``): errors, typically validation issues on a form
         - `headers` (``dict``): additional headers
 
-        If the error is an ``HTTPException`` (typically if it was triggered by
-        ``abort``), this handler logs it with `INF0` level. Otherwise, it is an
-        unhandled exception and it is already logged as `ERROR` by Flask.
+        If the error was triggered by ``abort``, this handler logs it with
+        ``INF0`` level. Otherwise, it is an unhandled exception and it is
+        already logged as ``ERROR`` by Flask.
         """
         # TODO: use an error Schema
         # TODO: add a parameter to enable/disable logging?
 
-        # If error is not a HTTPException, then it is an unhandled exception.
-        # Make it a 500 (InternalServerError) and don't log.
-        do_log = True
-        if not isinstance(error, HTTPException):
-            error = InternalServerError()
-            do_log = False
+        # Don't log unhandled exceptions as Flask already logs them
+        # Unhandled exceptions are attached to the InternalServerError
+        # passed to the handler.
+        # https://flask.palletsprojects.com/en/1.1.x/changelog/#version-1-1-0
+        do_log = not hasattr(error, 'original_exception')
 
         payload, headers = self._prepare_error_response_content(error)
         if do_log:

setup.py

@@ -44,7 +44,7 @@
     python_requires='>=3.5',
     install_requires=[
         'werkzeug>=0.15',
-        'flask>=1.0',
+        'flask>=1.1.0',
         'marshmallow>=2.15.2',
         'webargs>=1.5.2',
         'apispec>=2.0.0',

tests/test_error_handler.py

@@ -12,24 +12,79 @@
 class TestErrorHandler:
 
     @pytest.mark.parametrize('code', default_exceptions)
-    def test_error_handler_registration(self, code, app):
-        """Check custom error handler is registered for all codes"""
+    def test_error_handler_on_abort(self, app, code):
 
         client = app.test_client()
+        logger = app.logger
+
+        message = 'What a bad request.'
+        errors = {
+            'dimensions': ['Too tall', 'Too wide'],
+            'color': ['Too bright']
+        }
 
-        @app.route('/')
-        def test():
+        @app.route('/abort_no_kwargs')
+        def test_abort_no_kwargs():
             abort(code)
 
+        @app.route('/abort_kwargs')
+        def test_abort_kwargs():
+            abort(code, message=message, errors=errors)
+
         Api(app)
 
-        with NoLoggingContext(app):
-            response = client.get('/')
-        assert response.status_code == code
+        # Test error handler logs as INFO with payload content
+        with mock.patch.object(logger, 'info') as mock_info:
+            response = client.get('/abort_no_kwargs')
+            assert mock_info.called
+            args, kwargs = mock_info.call_args
 
+        assert args == (str(code), )
+        assert kwargs == {}
+
+        assert response.status_code == code
         data = json.loads(response.get_data(as_text=True))
         assert data['status'] == str(default_exceptions[code]())
 
+        with mock.patch.object(logger, 'info') as mock_info:
+            response = client.get('/abort_kwargs')
+            assert mock_info.called
+            args, kwargs = mock_info.call_args
+
+        assert args == (' '.join([str(code), message, str(errors)]), )
+        assert kwargs == {}
+
+    def test_error_handler_on_unhandled_error(self, app):
+
+        client = app.test_client()
+        logger = app.logger
+
+        uncaught_exc = Exception('Oops, something really bad happened.')
+
+        @app.route('/uncaught')
+        def test_uncaught():
+            raise uncaught_exc
+
+        Api(app)
+
+        # Test Flask logs uncaught exception as ERROR
+        # and handle_http_exception does not log is as INFO
+        with mock.patch.object(logger, 'error') as mock_error:
+            with mock.patch.object(logger, 'info') as mock_info:
+                response = client.get('/uncaught')
+                assert mock_error.called
+                args, kwargs = mock_error.call_args
+                assert not mock_info.called
+
+        assert args == ('Exception on /uncaught [GET]', )
+        exc_info = kwargs['exc_info']
+        _, exc_value, _ = exc_info
+        assert exc_value == uncaught_exc
+
+        assert response.status_code == 500
+        data = json.loads(response.get_data(as_text=True))
+        assert data['status'] == str(InternalServerError())
+
     def test_error_handler_payload(self, app):
 
         client = app.test_client()
@@ -84,71 +139,3 @@ def test_headers():
             response.headers['WWW-Authenticate'] == 'Basic realm="My Server"')
         data = json.loads(response.get_data(as_text=True))
         assert data['message'] == 'Access denied'
-
-    def test_error_handler_uncaught_exception(self, app):
-        """Test uncaught exceptions result in 500 status code being returned"""
-
-        client = app.test_client()
-
-        @app.route('/')
-        def test():
-            raise Exception('Oops, something really bad happened.')
-
-        Api(app)
-
-        with NoLoggingContext(app):
-            response = client.get('/')
-        assert response.status_code == 500
-
-        data = json.loads(response.get_data(as_text=True))
-        assert data['status'] == str(InternalServerError())
-
-    def test_error_handler_logging(self, app):
-
-        client = app.test_client()
-        logger = app.logger
-
-        uncaught_exc = Exception('Oops, something really bad happened.')
-        message = 'What a bad request.'
-        errors = {
-            'dimensions': ['Too tall', 'Too wide'],
-            'color': ['Too bright']
-        }
-
-        @app.route('/uncaught')
-        def test_uncaught():
-            raise uncaught_exc
-
-        @app.route('/abort_no_kwargs')
-        def test_abort_no_kwargs():
-            abort(400)
-
-        @app.route('/abort_kwargs')
-        def test_abort_kwargs():
-            abort(400, message=message, errors=errors)
-
-        Api(app)
-
-        # Test Flask logs uncaught exception
-        with mock.patch.object(logger, 'error') as mock_error:
-            client.get('/uncaught')
-            assert mock_error.called
-            args, kwargs = mock_error.call_args
-            assert args == ('Exception on /uncaught [GET]', )
-            exc_info = kwargs['exc_info']
-            _, exc_value, _ = exc_info
-            assert exc_value == uncaught_exc
-
-        # Test error handler logs as INFO with payload content
-        with mock.patch.object(logger, 'info') as mock_info:
-            client.get('/abort_no_kwargs')
-            assert mock_info.called
-            args, kwargs = mock_info.call_args
-            assert kwargs == {}
-            assert args == ('400', )
-        with mock.patch.object(logger, 'info') as mock_info:
-            client.get('/abort_kwargs')
-            assert mock_info.called
-            args, kwargs = mock_info.call_args
-            assert kwargs == {}
-            assert args == (' '.join(['400', message, str(errors)]), )