Merge 1c584e1 into 4567dac

flask_rest_api/error_handler.py

@@ -1,7 +1,7 @@
 """Exception handler"""
 
 from werkzeug.exceptions import HTTPException
-from flask import jsonify, current_app
+from flask import jsonify
 
 
 class ErrorHandlerMixin:
@@ -34,30 +34,10 @@ def handle_http_exception(self, error):
         Extra information expected by this handler:
 
         - `message` (``str``): a comment
-        - `errors` (``dict``): errors, typically validation issues on a form
+        - `errors` (``dict``): errors, typically validation errors in
+            parameters and request body
         - `headers` (``dict``): additional headers
-
-        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?
-
-        # 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:
-            self._log_error(error, payload)
-        return jsonify(payload), error.code, headers
-
-    @staticmethod
-    def _prepare_error_response_content(error):
-        """Build payload and headers from error"""
         headers = {}
         payload = {'code': error.code, 'status': error.name}
 
@@ -82,12 +62,4 @@ def _prepare_error_response_content(error):
             if 'headers' in data:
                 headers = data['headers']
 
-        return payload, headers
-
-    @staticmethod
-    def _log_error(error, payload):
-        """Log error as INFO, including payload content"""
-        log_string_content = [str(error.code), ]
-        log_string_content.extend([
-            str(payload[k]) for k in ('message', 'errors') if k in payload])
-        current_app.logger.info(' '.join(log_string_content))
+        return jsonify(payload), error.code, headers

tests/test_error_handler.py

@@ -1,91 +1,41 @@
-import json
-from unittest import mock
-
 import pytest
 
 from werkzeug.exceptions import default_exceptions, InternalServerError
 from flask_rest_api import Api, abort
 
-from .utils import NoLoggingContext
-
 
 class TestErrorHandler:
 
     @pytest.mark.parametrize('code', default_exceptions)
     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('/abort_no_kwargs')
-        def test_abort_no_kwargs():
+        @app.route('/abort')
+        def test_abort():
             abort(code)
 
-        @app.route('/abort_kwargs')
-        def test_abort_kwargs():
-            abort(code, message=message, errors=errors)
-
         Api(app)
 
-        # 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 == {}
-
+        response = client.get('/abort')
         assert response.status_code == code
-        data = json.loads(response.get_data(as_text=True))
-        assert data['code'] == code
-        assert data['status'] == default_exceptions[code]().name
-
-        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 == {}
+        assert response.json['code'] == code
+        assert response.json['status'] == default_exceptions[code]().name
 
     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
+            raise Exception('Oops, something really bad happened.')
 
         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
-
+        response = client.get('/uncaught')
         assert response.status_code == 500
-        data = json.loads(response.get_data(as_text=True))
-        assert data['code'] == 500
-        assert data['status'] == InternalServerError().name
+        assert response.json['code'] == 500
+        assert response.json['status'] == InternalServerError().name
 
     def test_error_handler_payload(self, app):
 
@@ -116,28 +66,20 @@ def test_headers():
 
         Api(app)
 
-        with NoLoggingContext(app):
-            response = client.get('/message')
+        response = client.get('/message')
         assert response.status_code == 404
-        data = json.loads(response.get_data(as_text=True))
-        assert data['message'] == 'Resource not found'
+        assert response.json['message'] == 'Resource not found'
 
-        with NoLoggingContext(app):
-            response = client.get('/messages')
+        response = client.get('/messages')
         assert response.status_code == 422
-        data = json.loads(response.get_data(as_text=True))
-        assert data['errors'] == messages
+        assert response.json['errors'] == messages
 
-        with NoLoggingContext(app):
-            response = client.get('/errors')
+        response = client.get('/errors')
         assert response.status_code == 422
-        data = json.loads(response.get_data(as_text=True))
-        assert data['errors'] == errors
+        assert response.json['errors'] == errors
 
-        with NoLoggingContext(app):
-            response = client.get('/headers')
+        response = client.get('/headers')
         assert response.status_code == 401
         assert (
             response.headers['WWW-Authenticate'] == 'Basic realm="My Server"')
-        data = json.loads(response.get_data(as_text=True))
-        assert data['message'] == 'Access denied'
+        assert response.json['message'] == 'Access denied'

tests/test_etag.py

@@ -19,7 +19,6 @@
 from flask_rest_api.compat import MARSHMALLOW_VERSION_MAJOR
 
 from .mocks import ItemNotFound
-from .utils import NoLoggingContext
 
 
 HTTP_METHODS = ['OPTIONS', 'HEAD', 'GET', 'POST', 'PUT', 'PATCH', 'DELETE']
@@ -267,16 +266,15 @@ def test_etag_verify_check_etag_exception(
         app.config['TESTING'] = testing
         blp = Blueprint('test', __name__)
 
-        with NoLoggingContext(app):
-            with app.test_request_context('/', method=method):
-                if (debug or testing) and method in ['PUT', 'PATCH', 'DELETE']:
-                    with pytest.raises(
-                            CheckEtagNotCalledError,
-                            match='ETag not checked in endpoint'
-                    ):
-                        blp._verify_check_etag()
-                else:
+        with app.test_request_context('/', method=method):
+            if (debug or testing) and method in ['PUT', 'PATCH', 'DELETE']:
+                with pytest.raises(
+                        CheckEtagNotCalledError,
+                        match='ETag not checked in endpoint'
+                ):
                     blp._verify_check_etag()
+            else:
+                blp._verify_check_etag()
 
     @pytest.mark.parametrize('etag_disabled', (True, False))
     def test_etag_set_etag(self, app, schemas, etag_disabled):

tests/utils.py

@@ -23,23 +23,6 @@ def json(self):
         )
 
 
-class NoLoggingContext:
-    """Context manager to disable logging temporarily
-
-    Some tests purposely trigger errors. We don't want to log them.
-    """
-
-    def __init__(self, app):
-        self.app = app
-
-    def __enter__(self):
-        self.logger_was_disabled = self.app.logger.disabled
-        self.app.logger.disabled = True
-
-    def __exit__(self, et, ev, tb):
-        self.app.logger.disabled = self.logger_was_disabled
-
-
 def get_schemas(spec):
     if spec.openapi_version.major < 3:
         return spec.to_dict()["definitions"]