Merge pull request #67 from ziadsawalha/errors

feat(rest): add error formatter

simpl/rest.py

@@ -16,13 +16,31 @@
 
 import functools
 import itertools
+import json
+import logging
 
 import bottle
+try:
+    import yaml
+except ImportError:
+    yaml = None
 
+LOG = logging.getLogger(__name__)
 MAX_PAGE_SIZE = 10000000
 STANDARD_QUERY_PARAMS = ('offset', 'limit', 'sort', 'q', 'facets')
 
 
+class HTTPError(Exception):
+
+    """Include HTTP Code, description and reason in exception."""
+
+    def __init__(self, message, http_code=400, reason=None):
+        """Initialize normal error, but save http code and reason."""
+        super(HTTPError, self).__init__(message)
+        self.http_code = http_code
+        self.reason = reason
+
+
 def body(schema=None, types=None, required=False, default=None):
     """Decorator to parse and validate API body.
 
@@ -277,3 +295,68 @@ def process_params(request, standard_params=STANDARD_QUERY_PARAMS,
 def comma_separated_strings(value):
     """Parse comma-separated string into list."""
     return [str(k).strip() for k in value.split(",")]
+
+
+def error_formatter(error):
+    """Bottle error formatter.
+
+    This will take caught errors and output them in our opinionated format and
+    the requested media-type. We default to json if we don't recognize or
+    support the content.
+
+    The content format is:
+
+        error:             - this is the wrapper for the returned error object
+            code:          - the HTTP error code (ex. 404)
+            message:       - the HTTP error code message (ex. Not Found)
+            description:   - the plain english, user-friendly description. Use
+                             this to to surface a UI/CLI. non-technical message
+            reason:        - (optional) any additional technical information to
+                             help a technical user with troubleshooting
+
+    Usage as a default handler:
+
+        import bottle
+        from simple import rest
+
+        app = bottle.default_app()
+        app.default_error_handler = rest.error_formatter
+
+        # Meanwhile, elsewhere in a module nearby
+        raise rest.HTTPError("Ouch!", http_code=500, reason="Lapse of reason")
+    """
+    output = {}
+    accept = bottle.request.get_header("Accept") or ""
+    if "application/x-yaml" in accept:
+        error.headers.update({"content-type": "application/x-yaml"})
+        writer = functools.partial(yaml.safe_dump, default_flow_style=False)
+    else:  # default to JSON
+        error.headers.update({"content-type": "application/json"})
+        writer = json.dumps
+
+    description = error.body or error.exception
+    if isinstance(error.exception, AssertionError):
+        error.status = 400
+        description = str(error.exception)
+        LOG.error(error.exception)
+    elif isinstance(error.exception, HTTPError):
+        error.status = error.exception.http_code
+        description = str(error.exception)
+        if error.exception.reason:
+            output['reason'] = error.exception.reason
+        LOG.error(error.exception)
+    elif error.exception:
+        error.status = 500
+        description = "Unexpected error"
+
+    # Log unexpected args
+    if hasattr(error.exception, 'args'):
+        if len(error.exception.args) > 1:
+            LOG.warning('HTTPError: %s', error.exception.args)
+
+    output['description'] = description
+    output['code'] = error.status_code
+    output['message'] = error.status_line.split(' ', 1)[1]
+
+    error.apply(bottle.response)
+    return writer({'error': output})

test-requirements.txt

@@ -7,5 +7,6 @@ mock
 nose
 nose-ignore-docstring
 pylint
+pyyaml
 requests
 webtest

tests/test_rest.py

@@ -21,6 +21,7 @@
 import six
 
 from simpl import rest
+import webtest
 
 
 class TestBodyDecorator(unittest.TestCase):
@@ -304,5 +305,143 @@ def test_dfaults(self):
         self.assertEqual(results, {'status': 'INACTIVE', 'size': 1})
 
 
+class TestAPIBasics(unittest.TestCase):
+
+    """Test REST API routing and responses."""
+
+    def setUp(self):
+        """Init the tests by starting the bottle app and routes."""
+        super(TestAPIBasics, self).setUp()
+        app = bottle.Bottle()
+        app.default_error_handler = rest.error_formatter
+        self.app = webtest.TestApp(app)
+
+        app.route('/', ['GET'], self.dummy_ok)
+        app.route('/fail', ['GET'], self.dummy_fail)
+        app.route('/bad', ['GET'], self.bad_code)
+        app.route('/assert', ['GET'], self.dummy_assert)
+        app.route('/unhandled', ['GET'], self.dummy_unhandled)
+
+    @staticmethod
+    def dummy_ok():
+        """Dummy call for route testing."""
+        return
+
+    @staticmethod
+    def dummy_fail():
+        """Dummy call for route testing."""
+        raise rest.HTTPError("Broken!", http_code=418, reason="Woops!")
+
+    @staticmethod
+    def bad_code():
+        """Dummy call for route testing."""
+        raise rest.HTTPError("wat?!", http_code=419, reason="Because")
+
+    @staticmethod
+    def dummy_assert():
+        """Dummy call for route testing."""
+        assert False, "Not what we expected"
+
+    @staticmethod
+    def dummy_unhandled():
+        """Dummy call for route testing."""
+        raise Exception("Possibly sensitive data")
+
+    def test_default_404(self):
+        res = self.app.get('/foo', expect_errors=True)
+        self.assertEqual(res.status, '404 Not Found')
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 404,
+                'message': 'Not Found',
+                'description': "Not found: '/foo'",
+            }
+        }
+        self.assertEqual(res.json, expected)
+
+    def test_default_405(self):
+        res = self.app.post('/', expect_errors=True)
+        self.assertEqual(res.status, '405 Method Not Allowed')
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 405,
+                'message': 'Method Not Allowed',
+                'description': "Method not allowed.",
+            }
+        }
+        self.assertEqual(res.json, expected)
+
+    def test_raised_http_error(self):
+        res = self.app.get('/fail', expect_errors=True)
+        self.assertEqual(res.status, "418 I'm a teapot")
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 418,
+                'message': "I'm a teapot",
+                'description': 'Broken!',
+                'reason': "Woops!",
+            }
+        }
+        self.assertEqual(res.json, expected)
+
+    def test_bad_http_code(self):
+        res = self.app.get('/bad', expect_errors=True)
+        self.assertEqual(res.status, '419 Unknown')
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 419,
+                'message': "Unknown",
+                'description': "wat?!",
+                'reason': 'Because',
+            }
+        }
+        self.assertEqual(res.json, expected)
+
+    def test_asserts_become_500(self):
+        res = self.app.get('/assert', expect_errors=True)
+        self.assertEqual(res.status, '400 Bad Request')
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 400,
+                'message': 'Bad Request',
+                'description': "Not what we expected",
+            }
+        }
+        self.assertEqual(res.json, expected)
+
+    def test_unhandled_500(self):
+        res = self.app.get('/unhandled', expect_errors=True)
+        self.assertEqual(res.status, '500 Internal Server Error')
+        self.assertEqual(res.content_type, 'application/json')
+        expected = {
+            'error': {
+                'code': 500,
+                'message': 'Internal Server Error',
+                'description': "Unexpected error",
+            }
+        }
+        self.assertTrue(res.body.lower().find(b'internal') > 0)
+        self.assertEqual(res.body.lower().find(b'sensitive'), -1)
+        self.assertEqual(res.json, expected)
+
+    def test_yaml(self):
+        res = self.app.get('/foo',
+                           headers={'Accept': 'application/x-yaml'},
+                           expect_errors=True)
+        self.assertEqual(res.content_type, 'application/x-yaml')
+        expected = b"""\
+error:
+  code: 404
+  description: 'Not found: ''/foo'''
+  message: Not Found
+"""
+        self.assertEqual(res.body, expected)
+
+
 if __name__ == '__main__':
     unittest.main()