From: Brett Smith <brett@curoverse.com>
Date: Tue, 5 Aug 2014 19:18:14 +0000 (-0400)
Subject: 3415: API exceptions from Python SDK include more error information.
X-Git-Tag: 1.1.0~2373^2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/f9d2cd963c314188c0351253c00d6dc82f276ed7?hp=46f565cf2dd89a3ec6ad78b1237b3a4b0db6404b

3415: API exceptions from Python SDK include more error information.

The apiclient module doesn't give us a lot of opportunities to
customize error handling.  Request objects can have response
callbacks, but they only get access to the response headers, not body,
which we need to pass along JSON errors.  After that, apiclient.http
imports apiclient.errors.HttpError directly, and raises that directly
whenever there's a permanent error in an HTTP response.

arvados.api already makes a few monkeypatches to apiclient, and this
commit adds one more: it customizes HttpError's __new__ method to
return a new customized subclass instead.  This is pretty evil,
because it will mess with any other instantiations of HttpError in
client programs.  Its only mitigating grace is that the new subclass
is fully API-compatible with the original.
---

diff --git a/sdk/python/arvados/api.py b/sdk/python/arvados/api.py
index eb09664d29..8a71b90c0d 100644
--- a/sdk/python/arvados/api.py
+++ b/sdk/python/arvados/api.py
@@ -7,6 +7,7 @@ import types
 
 import apiclient
 import apiclient.discovery
+import apiclient.errors
 import config
 import errors
 import util
@@ -51,6 +52,15 @@ def _cast_objects_too(value, schema_type):
         return _cast_orig(value, schema_type)
 apiclient.discovery._cast = _cast_objects_too
 
+# Convert apiclient's HttpErrors into our own API error subclass for better
+# error reporting.
+# Reassigning apiclient.errors.HttpError is not sufficient because most of the
+# apiclient submodules import the class into their own namespace.
+def _new_http_error(cls, *args, **kwargs):
+    return super(apiclient.errors.HttpError, cls).__new__(
+        errors.ApiError, *args, **kwargs)
+apiclient.errors.HttpError.__new__ = staticmethod(_new_http_error)
+
 def http_cache(data_type):
     path = os.environ['HOME'] + '/.cache/arvados/' + data_type
     try:
diff --git a/sdk/python/arvados/errors.py b/sdk/python/arvados/errors.py
index 85472d8cdf..1d9c77851e 100644
--- a/sdk/python/arvados/errors.py
+++ b/sdk/python/arvados/errors.py
@@ -1,5 +1,16 @@
 # errors.py - Arvados-specific exceptions.
 
+import apiclient.errors
+import json
+
+class ApiError(apiclient.errors.HttpError):
+    def _get_reason(self):
+        try:
+            return '; '.join(json.loads(self.content)['errors'])
+        except (KeyError, TypeError, ValueError):
+            return super(ApiError, self)._get_reason()
+
+
 class ArgumentError(Exception):
     pass
 class SyntaxError(Exception):
diff --git a/sdk/python/tests/test_api.py b/sdk/python/tests/test_api.py
index cd7b1e9611..4c485712ec 100644
--- a/sdk/python/tests/test_api.py
+++ b/sdk/python/tests/test_api.py
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 
+import apiclient.errors
 import arvados
 import httplib2
 import json
@@ -13,6 +14,19 @@ if not mimetypes.inited:
     mimetypes.init()
 
 class ArvadosApiClientTest(unittest.TestCase):
+    @classmethod
+    def response_from_code(cls, code):
+        return httplib2.Response(
+            {'status': code,
+             'reason': HTTP_RESPONSES.get(code, "Unknown Response"),
+             'Content-Type': mimetypes.types_map['.json']})
+
+    @classmethod
+    def api_error_response(cls, code, *errors):
+        return (cls.response_from_code(code),
+                json.dumps({'errors': errors,
+                            'error_token': '1234567890+12345678'}))
+
     @classmethod
     def setUpClass(cls):
         # The apiclient library has support for mocking requests for
@@ -22,6 +36,9 @@ class ArvadosApiClientTest(unittest.TestCase):
         cls.orig_api_host = arvados.config.get('ARVADOS_API_HOST')
         arvados.config.settings()['ARVADOS_API_HOST'] = 'qr1hi.arvadosapi.com'
         mock_responses = {
+            'arvados.humans.delete': (cls.response_from_code(500), ""),
+            'arvados.humans.get': cls.api_error_response(
+                422, "Bad UUID format", "Bad output format"),
             'arvados.humans.list': (None, json.dumps(
                     {'items_available': 0, 'items': []})),
             }
@@ -42,6 +59,18 @@ class ArvadosApiClientTest(unittest.TestCase):
             filters=[['uuid', 'is', None]]).execute()
         self.assertEqual(answer['items_available'], len(answer['items']))
 
+    def test_exceptions_include_errors(self):
+        with self.assertRaises(apiclient.errors.HttpError) as err_ctx:
+            self.api.humans().get(uuid='xyz-xyz-abcdef').execute()
+        err_s = str(err_ctx.exception)
+        for msg in ["Bad UUID format", "Bad output format"]:
+            self.assertIn(msg, err_s)
+
+    def test_exceptions_without_errors_have_basic_info(self):
+        with self.assertRaises(apiclient.errors.HttpError) as err_ctx:
+            self.api.humans().delete(uuid='xyz-xyz-abcdef').execute()
+        self.assertIn("500", str(err_ctx.exception))
+
 
 if __name__ == '__main__':
     unittest.main()