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 eb09664d29e55a497c21efe84db94749fb3f24da..8a71b90c0dda661ea0ac8bfbcd17e47cb1ee36b9 100644 (file)
--- 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 85472d8cdf0bcb729933709aa7aee4d45a8df18e..1d9c77851eacde7f1f9571e697361c1a75a8849b 100644 (file)
--- 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 cd7b1e9611f763ef06c3dedca920202a6478c5a7..4c485712ec44b38e2a0009bb01f84ed17853b743 100644 (file)
--- 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()