From 89788fbfac5a799f3bf8d532be5c88fcde87235b Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Fri, 7 Nov 2014 12:59:46 -0500 Subject: [PATCH 1/2] Map HTTP error responses for storage onto specific exception classes. Fixes #164. --- gcloud/storage/bucket.py | 20 ++-- gcloud/storage/connection.py | 24 ++--- gcloud/storage/exceptions.py | 172 +++++++++++++++++++++++++++--- gcloud/storage/key.py | 8 +- gcloud/storage/test_acl.py | 4 +- gcloud/storage/test_bucket.py | 20 ++-- gcloud/storage/test_connection.py | 16 +-- gcloud/storage/test_exceptions.py | 76 +++++++++---- regression/storage.py | 4 +- 9 files changed, 263 insertions(+), 81 deletions(-) diff --git a/gcloud/storage/bucket.py b/gcloud/storage/bucket.py index 7dac8d91cbeb..397ce32e0a29 100644 --- a/gcloud/storage/bucket.py +++ b/gcloud/storage/bucket.py @@ -126,7 +126,7 @@ def get_key(self, key): try: response = self.connection.api_request(method='GET', path=key.path) return Key.from_dict(response, bucket=self) - except exceptions.NotFoundError: + except exceptions.NotFound: return None def get_all_keys(self): @@ -176,7 +176,7 @@ def delete(self, force=False): The bucket **must** be empty in order to delete it. If the bucket doesn't exist, this will raise a - :class:`gcloud.storage.exceptions.NotFoundError`. If the bucket + :class:`gcloud.storage.exceptions.NotFound`. If the bucket is not empty, this will raise an Exception. If you want to delete a non-empty bucket you can pass in a force @@ -186,9 +186,9 @@ def delete(self, force=False): :type force: bool :param full: If True, empties the bucket's objects then deletes it. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` if the + :raises: :class:`gcloud.storage.exceptions.NotFound` if the bucket does not exist, or - :class:`gcloud.storage.exceptions.ConnectionError` if the + :class:`gcloud.storage.exceptions.Conflict` if the bucket has keys and `force` is not passed. """ return self.connection.delete_bucket(self.name, force=force) @@ -197,7 +197,7 @@ def delete_key(self, key): """Deletes a key from the current bucket. If the key isn't found, - this will throw a :class:`gcloud.storage.exceptions.NotFoundError`. + this will throw a :class:`gcloud.storage.exceptions.NotFound`. For example:: @@ -210,7 +210,7 @@ def delete_key(self, key): >>> bucket.delete_key('my-file.txt') >>> try: ... bucket.delete_key('doesnt-exist') - ... except exceptions.NotFoundError: + ... except exceptions.NotFound: ... pass @@ -219,7 +219,7 @@ def delete_key(self, key): :rtype: :class:`gcloud.storage.key.Key` :returns: The key that was just deleted. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` (to suppress + :raises: :class:`gcloud.storage.exceptions.NotFound` (to suppress the exception, call ``delete_keys``, passing a no-op ``on_error`` callback, e.g.:: @@ -239,16 +239,16 @@ def delete_keys(self, keys, on_error=None): :type on_error: a callable taking (key) :param on_error: If not ``None``, called once for each key raising - :class:`gcloud.storage.exceptions.NotFoundError`; + :class:`gcloud.storage.exceptions.NotFound`; otherwise, the exception is propagated. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` (if + :raises: :class:`gcloud.storage.exceptions.NotFound` (if `on_error` is not passed). """ for key in keys: try: self.delete_key(key) - except exceptions.NotFoundError: + except exceptions.NotFound: if on_error is not None: on_error(key) else: diff --git a/gcloud/storage/connection.py b/gcloud/storage/connection.py index c1d62c194338..00eb63772cac 100644 --- a/gcloud/storage/connection.py +++ b/gcloud/storage/connection.py @@ -231,10 +231,8 @@ def api_request(self, method, path, query_params=None, response, content = self.make_request( method=method, url=url, data=data, content_type=content_type) - if response.status == 404: - raise exceptions.NotFoundError(response) - elif not 200 <= response.status < 300: - raise exceptions.ConnectionError(response, content) + if not 200 <= response.status < 300: + raise exceptions.make_exception(response, content) if content and expect_json: content_type = response.get('content-type', '') @@ -270,7 +268,7 @@ def get_bucket(self, bucket_name): """Get a bucket by name. If the bucket isn't found, this will raise a - :class:`gcloud.storage.exceptions.NotFoundError`. If you would + :class:`gcloud.storage.exceptions.NotFound`. If you would rather get a bucket by name, and return ``None`` if the bucket isn't found (like ``{}.get('...')``) then use :func:`Connection.lookup`. @@ -282,7 +280,7 @@ def get_bucket(self, bucket_name): >>> connection = storage.get_connection(project, email, key_path) >>> try: >>> bucket = connection.get_bucket('my-bucket') - >>> except exceptions.NotFoundError: + >>> except exceptions.NotFound: >>> print 'Sorry, that bucket does not exist!' :type bucket_name: string @@ -290,7 +288,7 @@ def get_bucket(self, bucket_name): :rtype: :class:`gcloud.storage.bucket.Bucket` :returns: The bucket matching the name provided. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` + :raises: :class:`gcloud.storage.exceptions.NotFound` """ bucket = self.new_bucket(bucket_name) response = self.api_request(method='GET', path=bucket.path) @@ -319,7 +317,7 @@ def lookup(self, bucket_name): """ try: return self.get_bucket(bucket_name) - except exceptions.NotFoundError: + except exceptions.NotFound: return None def create_bucket(self, bucket): @@ -338,7 +336,7 @@ def create_bucket(self, bucket): :rtype: :class:`gcloud.storage.bucket.Bucket` :returns: The newly created bucket. - :raises: :class:`gcloud.storage.exceptions.ConnectionError` if + :raises: :class:`gcloud.storage.exceptions.Conflict` if there is a confict (bucket already exists, invalid name, etc.) """ bucket = self.new_bucket(bucket) @@ -364,12 +362,12 @@ def delete_bucket(self, bucket, force=False): True If the bucket doesn't exist, this will raise a - :class:`gcloud.storage.exceptions.NotFoundError`:: + :class:`gcloud.storage.exceptions.NotFound`:: >>> from gcloud.storage import exceptions >>> try: >>> connection.delete_bucket('my-bucket') - >>> except exceptions.NotFoundError: + >>> except exceptions.NotFound: >>> print 'That bucket does not exist!' :type bucket: string or :class:`gcloud.storage.bucket.Bucket` @@ -380,9 +378,9 @@ def delete_bucket(self, bucket, force=False): :rtype: bool :returns: True if the bucket was deleted. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` if the + :raises: :class:`gcloud.storage.exceptions.NotFound` if the bucket doesn't exist, or - :class:`gcloud.storage.exceptions.ConnectionError` if the + :class:`gcloud.storage.exceptions.Conflict` if the bucket has keys and `force` is not passed. """ bucket = self.new_bucket(bucket) diff --git a/gcloud/storage/exceptions.py b/gcloud/storage/exceptions.py index 77fed212b354..a374e7c4ed1d 100644 --- a/gcloud/storage/exceptions.py +++ b/gcloud/storage/exceptions.py @@ -1,24 +1,170 @@ -"""Custom exceptions for gcloud.storage package.""" +"""Custom exceptions for gcloud.storage package. + +See: https://cloud.google.com/storage/docs/json_api/v1/status-codes +""" + +import json + +_HTTP_CODE_TO_EXCEPTION = {} # populated at end of module class StorageError(Exception): - """Base error class for gcloud errors.""" + """Base error class for gcloud errors (abstract). + Each subclass represents a single type of HTTP error response. + """ + code = None + """HTTP status code. Concrete subclasses *must* define. -class ConnectionError(StorageError): - """Exception corresponding to a bad HTTP/RPC connection.""" + See: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html + """ - def __init__(self, response, content): - message = str(response) + content - super(ConnectionError, self).__init__(message) + def __init__(self, message, errors=()): + super(StorageError, self).__init__() # suppress deprecation warning under 2.6.x self.message = message + self._errors = [error.copy() for error in errors] + def __str__(self): + return '%d %s' % (self.code, self.message) -class NotFoundError(StorageError): - """Exception corresponding to a 404 not found bad connection.""" + @property + def errors(self): + """Detailed error information. - def __init__(self, response): - super(NotFoundError, self).__init__('') - # suppress deprecation warning under 2.6.x - self.message = 'Request returned a 404. Headers: %s' % (response,) + :rtype: list(dict) + :returns: a list of mappings describing each error. + """ + return [error.copy() for error in self._errors] + + +class Redirection(StorageError): + """Base for 3xx responses + + This class is abstract. + """ + + +class MovedPermanently(Redirection): + """Exception mapping a '301 Moved Permanently' response.""" + code = 301 + + +class NotModified(Redirection): + """Exception mapping a '304 Not Modified' response.""" + code = 304 + + +class TemporaryRedirect(Redirection): + """Exception mapping a '307 Temporary Redirect' response.""" + code = 307 + + +class ResumeIncomplete(Redirection): + """Exception mapping a '308 Resume Incomplete' response.""" + code = 308 + + +class ClientError(StorageError): + """Base for 4xx responses + + This class is abstract + """ + + +class BadRequest(ClientError): + """Exception mapping a '400 Bad Request' response.""" + code = 400 + + +class Unauthorized(ClientError): + """Exception mapping a '401 Unauthorized' response.""" + code = 400 + + +class Forbidden(ClientError): + """Exception mapping a '403 Forbidden' response.""" + code = 400 + + +class NotFound(ClientError): + """Exception mapping a '404 Not Found' response.""" + code = 404 + + +class MethodNotAllowed(ClientError): + """Exception mapping a '405 Method Not Allowed' response.""" + code = 405 + + +class Conflict(ClientError): + """Exception mapping a '409 Conflict' response.""" + code = 409 + + +class LengthRequired(ClientError): + """Exception mapping a '411 Length Required' response.""" + code = 411 + + +class PreconditionFailed(ClientError): + """Exception mapping a '412 Precondition Failed' response.""" + code = 412 + + +class RequestRangeNotSatisfiable(ClientError): + """Exception mapping a '416 Request Range Not Satisfiable' response.""" + code = 416 + + +class TooManyRequests(ClientError): + """Exception mapping a '429 Too Many Requests' response.""" + code = 429 + + +class ServerError(StorageError): + """Base for 5xx responses: (abstract)""" + + +class InternalServerError(ServerError): + """Exception mapping a '500 Internal Server Error' response.""" + code = 500 + + +class NotImplemented(ServerError): + """Exception mapping a '501 Not Implemented' response.""" + code = 501 + + +class ServiceUnavailable(ServerError): + """Exception mapping a '503 Service Unavailable' response.""" + code = 503 + + +def make_exception(response, content): + """Factory: create exception based on HTTP response code. + + :rtype: instance of :class:`StorageError`, or a concrete subclass. + """ + + if isinstance(content, str): + content = json.loads(content) + + message = content.get('message') + error = content.get('error', {}) + errors = error.get('errors', ()) + + try: + klass = _HTTP_CODE_TO_EXCEPTION[response.status] + except KeyError: + error = StorageError(message, errors) + error.code = response.status + else: + error = klass(message, errors) + return error + + +for name, value in globals().items(): + code = getattr(value, 'code', None) + if code is not None: + _HTTP_CODE_TO_EXCEPTION[code] = value diff --git a/gcloud/storage/key.py b/gcloud/storage/key.py index 86d018e66b11..208d57de4467 100644 --- a/gcloud/storage/key.py +++ b/gcloud/storage/key.py @@ -203,7 +203,7 @@ def delete(self): :rtype: :class:`Key` :returns: The key that was just deleted. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` + :raises: :class:`gcloud.storage.exceptions.NotFound` (propagated from :meth:`gcloud.storage.bucket.Bucket.delete_key`). """ @@ -215,7 +215,7 @@ def download_to_file(self, file_obj): :type file_obj: file :param file_obj: A file handle to which to write the key's data. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` + :raises: :class:`gcloud.storage.exceptions.NotFound` """ for chunk in _KeyDataIterator(self): file_obj.write(chunk) @@ -229,7 +229,7 @@ def download_to_filename(self, filename): :type filename: string :param filename: A filename to be passed to ``open``. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` + :raises: :class:`gcloud.storage.exceptions.NotFound` """ with open(filename, 'wb') as file_obj: self.download_to_file(file_obj) @@ -242,7 +242,7 @@ def download_as_string(self): :rtype: string :returns: The data stored in this key. - :raises: :class:`gcloud.storage.exceptions.NotFoundError` + :raises: :class:`gcloud.storage.exceptions.NotFound` """ string_buffer = StringIO() self.download_to_file(string_buffer) diff --git a/gcloud/storage/test_acl.py b/gcloud/storage/test_acl.py index 67f208759368..7c771b86d8c2 100644 --- a/gcloud/storage/test_acl.py +++ b/gcloud/storage/test_acl.py @@ -697,12 +697,12 @@ def __init__(self, *responses): self._deleted = [] def api_request(self, **kw): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound self._requested.append(kw) try: response, self._responses = self._responses[0], self._responses[1:] except: # pragma: NO COVER - raise NotFoundError('miss') + raise NotFound('miss') else: return response diff --git a/gcloud/storage/test_bucket.py b/gcloud/storage/test_bucket.py index dc46016d033b..16ed2a2cd29d 100644 --- a/gcloud/storage/test_bucket.py +++ b/gcloud/storage/test_bucket.py @@ -190,11 +190,11 @@ def test_new_key_invalid(self): self.assertRaises(TypeError, bucket.new_key, object()) def test_delete_default_miss(self): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound NAME = 'name' connection = _Connection() bucket = self._makeOne(connection, NAME) - self.assertRaises(NotFoundError, bucket.delete) + self.assertRaises(NotFound, bucket.delete) self.assertEqual(connection._deleted, [(NAME, False)]) def test_delete_explicit_hit(self): @@ -206,12 +206,12 @@ def test_delete_explicit_hit(self): self.assertEqual(connection._deleted, [(NAME, True)]) def test_delete_key_miss(self): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound NAME = 'name' NONESUCH = 'nonesuch' connection = _Connection() bucket = self._makeOne(connection, NAME) - self.assertRaises(NotFoundError, bucket.delete_key, NONESUCH) + self.assertRaises(NotFound, bucket.delete_key, NONESUCH) kw, = connection._requested self.assertEqual(kw['method'], 'DELETE') self.assertEqual(kw['path'], '/b/%s/o/%s' % (NAME, NONESUCH)) @@ -247,13 +247,13 @@ def test_delete_keys_hit(self): self.assertEqual(kw[0]['path'], '/b/%s/o/%s' % (NAME, KEY)) def test_delete_keys_miss_no_on_error(self): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound NAME = 'name' KEY = 'key' NONESUCH = 'nonesuch' connection = _Connection({}) bucket = self._makeOne(connection, NAME) - self.assertRaises(NotFoundError, bucket.delete_keys, [KEY, NONESUCH]) + self.assertRaises(NotFound, bucket.delete_keys, [KEY, NONESUCH]) kw = connection._requested self.assertEqual(len(kw), 2) self.assertEqual(kw[0]['method'], 'DELETE') @@ -955,21 +955,21 @@ def __init__(self, *responses): self._deleted = [] def api_request(self, **kw): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound self._requested.append(kw) try: response, self._responses = self._responses[0], self._responses[1:] except: - raise NotFoundError('miss') + raise NotFound('miss') else: return response def delete_bucket(self, bucket, force=False): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound self._deleted.append((bucket, force)) if not self._delete_ok: - raise NotFoundError('miss') + raise NotFound('miss') return True diff --git a/gcloud/storage/test_connection.py b/gcloud/storage/test_connection.py index 036d8f36da00..7987cb40266c 100644 --- a/gcloud/storage/test_connection.py +++ b/gcloud/storage/test_connection.py @@ -312,24 +312,24 @@ def test_api_request_w_data(self): self.assertEqual(http._called_with['headers'], expected_headers) def test_api_request_w_404(self): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound PROJECT = 'project' conn = self._makeOne(PROJECT) conn._http = Http( {'status': '404', 'content-type': 'text/plain'}, - '', + '{}' ) - self.assertRaises(NotFoundError, conn.api_request, 'GET', '/') + self.assertRaises(NotFound, conn.api_request, 'GET', '/') def test_api_request_w_500(self): - from gcloud.storage.exceptions import ConnectionError + from gcloud.storage.exceptions import InternalServerError PROJECT = 'project' conn = self._makeOne(PROJECT) conn._http = Http( {'status': '500', 'content-type': 'text/plain'}, - '', + '{}', ) - self.assertRaises(ConnectionError, conn.api_request, 'GET', '/') + self.assertRaises(InternalServerError, conn.api_request, 'GET', '/') def test_get_all_buckets_empty(self): PROJECT = 'project' @@ -370,7 +370,7 @@ def test_get_all_buckets_non_empty(self): self.assertEqual(http._called_with['uri'], URI) def test_get_bucket_miss(self): - from gcloud.storage.exceptions import NotFoundError + from gcloud.storage.exceptions import NotFound PROJECT = 'project' NONESUCH = 'nonesuch' conn = self._makeOne(PROJECT) @@ -385,7 +385,7 @@ def test_get_bucket_miss(self): {'status': '404', 'content-type': 'application/json'}, '{}', ) - self.assertRaises(NotFoundError, conn.get_bucket, NONESUCH) + self.assertRaises(NotFound, conn.get_bucket, NONESUCH) self.assertEqual(http._called_with['method'], 'GET') self.assertEqual(http._called_with['uri'], URI) diff --git a/gcloud/storage/test_exceptions.py b/gcloud/storage/test_exceptions.py index 03d67a1ed968..eda720e3a2d1 100644 --- a/gcloud/storage/test_exceptions.py +++ b/gcloud/storage/test_exceptions.py @@ -1,31 +1,69 @@ import unittest2 -class TestConnectionError(unittest2.TestCase): +class Test_StorageError(unittest2.TestCase): def _getTargetClass(self): - from gcloud.storage.exceptions import ConnectionError - return ConnectionError + from gcloud.storage.exceptions import StorageError + return StorageError - def _makeOne(self, *args, **kw): - return self._getTargetClass()(*args, **kw) + def _makeOne(self, *args): + return self._getTargetClass()(*args) - def test_no_headers(self): - e = self._makeOne({}, '') - self.assertEqual(str(e), '{}') - self.assertEqual(e.message, '{}') + def test_ctor_defaults(self): + e = self._makeOne('Testing') + e.code = 600 + self.assertEqual(str(e), '600 Testing') + self.assertEqual(e.message, 'Testing') + self.assertEqual(list(e.errors), []) + def test_ctor_explicit(self): + ERROR = { + 'domain': 'global', + 'location': 'test', + 'locationType': 'testing', + 'message': 'Testing', + 'reason': 'test', + } + e = self._makeOne('Testing', [ERROR]) + e.code = 600 + self.assertEqual(str(e), '600 Testing') + self.assertEqual(e.message, 'Testing') + self.assertEqual(list(e.errors), [ERROR]) -class TestNotFoundError(unittest2.TestCase): - def _getTargetClass(self): - from gcloud.storage.exceptions import NotFoundError - return NotFoundError +class Test_make_exception(unittest2.TestCase): + + def _callFUT(self, response, content): + from gcloud.storage.exceptions import make_exception + return make_exception(response, content) + + def test_hit_w_content_as_str(self): + from gcloud.storage.exceptions import NotFound + response = _Response(404) + content = '{"message": "Not Found"}' + exception = self._callFUT(response, content) + self.assertTrue(isinstance(exception, NotFound)) + self.assertEqual(exception.message, 'Not Found') + self.assertEqual(list(exception.errors), []) + + def test_miss_w_content_as_dict(self): + from gcloud.storage.exceptions import StorageError + ERROR = { + 'domain': 'global', + 'location': 'test', + 'locationType': 'testing', + 'message': 'Testing', + 'reason': 'test', + } + response = _Response(600) + content = {"message": "Unknown Error", "error": {"errors": [ERROR]}} + exception = self._callFUT(response, content) + self.assertTrue(isinstance(exception, StorageError)) + self.assertEqual(exception.message, 'Unknown Error') + self.assertEqual(list(exception.errors), [ERROR]) - def _makeOne(self, *args, **kw): - return self._getTargetClass()(*args, **kw) - def test_no_headers(self): - e = self._makeOne({}) - self.assertEqual(str(e), '') - self.assertEqual(e.message, 'Request returned a 404. Headers: {}') +class _Response(object): + def __init__(self, status): + self.status = status diff --git a/regression/storage.py b/regression/storage.py index ff90e826d22a..f20c393c1e40 100644 --- a/regression/storage.py +++ b/regression/storage.py @@ -49,7 +49,7 @@ def tearDown(self): def test_create_bucket(self): new_bucket_name = 'a-new-bucket' - self.assertRaises(storage.exceptions.NotFoundError, + self.assertRaises(storage.exceptions.NotFound, self.connection.get_bucket, new_bucket_name) created = self.connection.create_bucket(new_bucket_name) self.case_buckets_to_delete.append(created) @@ -243,5 +243,5 @@ def test_create_signed_delete_url(self): self.assertEqual(content, '') # Check that the key has actually been deleted. - self.assertRaises(storage.exceptions.NotFoundError, + self.assertRaises(storage.exceptions.NotFound, key.reload_metadata) From e322609f07b55637638071d3777cd5d1c0483847 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Fri, 7 Nov 2014 16:36:20 -0500 Subject: [PATCH 2/2] Recursively walk subclass tree to build code->exception class mapping. --- gcloud/storage/exceptions.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/gcloud/storage/exceptions.py b/gcloud/storage/exceptions.py index a374e7c4ed1d..59c41381ddee 100644 --- a/gcloud/storage/exceptions.py +++ b/gcloud/storage/exceptions.py @@ -164,7 +164,16 @@ def make_exception(response, content): return error -for name, value in globals().items(): - code = getattr(value, 'code', None) +def _walk_subclasses(klass): + """Recursively walk subclass tree.""" + for sub in klass.__subclasses__(): + yield sub + for subsub in _walk_subclasses(sub): + yield subsub + + +# Build the code->exception class mapping. +for eklass in _walk_subclasses(StorageError): + code = getattr(eklass, 'code', None) if code is not None: - _HTTP_CODE_TO_EXCEPTION[code] = value + _HTTP_CODE_TO_EXCEPTION[code] = eklass