From 85c64a359da6954be8b4bf582a98cfc01e9cc85f Mon Sep 17 00:00:00 2001 From: Danny Hermes Date: Tue, 3 Feb 2015 09:50:06 -0800 Subject: [PATCH] Making Bucket.delete() work in face on eventual consistency. Fixes #564. --- gcloud/storage/bucket.py | 46 +++++++++++++++++------- gcloud/storage/connection.py | 38 ++++++++------------ gcloud/storage/test_bucket.py | 60 +++++++++++++++++++++++++++---- gcloud/storage/test_connection.py | 46 +----------------------- regression/storage.py | 13 +------ 5 files changed, 103 insertions(+), 100 deletions(-) diff --git a/gcloud/storage/bucket.py b/gcloud/storage/bucket.py index d1c2e95f8c97..edb0e7e4b577 100644 --- a/gcloud/storage/bucket.py +++ b/gcloud/storage/bucket.py @@ -64,6 +64,9 @@ class Bucket(_PropertyMixin): """ _iterator_class = _BlobIterator + _MAX_OBJECTS_FOR_BUCKET_DELETE = 256 + """Maximum number of existing objects allowed in Bucket.delete().""" + CUSTOM_PROPERTY_ACCESSORS = { 'acl': 'acl', 'cors': 'get_cors()', @@ -237,24 +240,41 @@ def new_blob(self, blob): def delete(self, force=False): """Delete this bucket. - The bucket **must** be empty in order to delete it. If the - bucket doesn't exist, this will raise a - :class:`gcloud.exceptions.NotFound`. If the bucket is - not empty, this will raise an Exception. + The bucket **must** be empty in order to submit a delete request. If + ``force=True`` is passed, this will first attempt to delete all the + objects / blobs in the bucket (i.e. try to empty the bucket). + + If the bucket doesn't exist, this will raise + :class:`gcloud.exceptions.NotFound`. If the bucket is not empty + (and ``force=False``), will raise :class:`gcloud.exceptions.Conflict`. - If you want to delete a non-empty bucket you can pass in a force - parameter set to ``True``. This will iterate through and delete the - bucket's objects, before deleting the bucket. + If ``force=True`` and the bucket contains more than 256 objects / blobs + this will cowardly refuse to delete the objects (or the bucket). This + is to prevent accidental bucket deletion and to prevent extremely long + runtime of this method. :type force: boolean :param force: If True, empties the bucket's objects then deletes it. - :raises: :class:`gcloud.exceptions.NotFound` if the - bucket does not exist, or - :class:`gcloud.exceptions.Conflict` if the - bucket has blobs and `force` is not passed. + :raises: :class:`ValueError` if ``force`` is ``True`` and the bucket + contains more than 256 objects / blobs. """ - return self.connection.delete_bucket(self.name, force=force) + if force: + blobs = list(self.iterator( + max_results=self._MAX_OBJECTS_FOR_BUCKET_DELETE + 1)) + if len(blobs) > self._MAX_OBJECTS_FOR_BUCKET_DELETE: + message = ( + 'Refusing to delete bucket with more than ' + '%d objects. If you actually want to delete ' + 'this bucket, please delete the objects ' + 'yourself before calling Bucket.delete().' + ) % (self._MAX_OBJECTS_FOR_BUCKET_DELETE,) + raise ValueError(message) + + # Ignore 404 errors on delete. + self.delete_blobs(blobs, on_error=lambda blob: None) + + self.connection.delete_bucket(self.name) def delete_blob(self, blob): """Deletes a blob from the current bucket. @@ -286,7 +306,7 @@ def delete_blob(self, blob): the exception, call ``delete_blobs``, passing a no-op ``on_error`` callback, e.g.:: - >>> bucket.delete_blobs([blob], on_error=lambda blob: pass) + >>> bucket.delete_blobs([blob], on_error=lambda blob: None) """ blob = self.new_blob(blob) self.connection.api_request(method='DELETE', path=blob.path) diff --git a/gcloud/storage/connection.py b/gcloud/storage/connection.py index f473e5ebca43..e0dcc9ac000d 100644 --- a/gcloud/storage/connection.py +++ b/gcloud/storage/connection.py @@ -357,7 +357,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.NotFound`. If you would + :class:`gcloud.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`. @@ -377,7 +377,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.NotFound` + :raises: :class:`gcloud.exceptions.NotFound` """ bucket = self.new_bucket(bucket_name) response = self.api_request(method='GET', path=bucket.path) @@ -425,7 +425,7 @@ def create_bucket(self, bucket): :rtype: :class:`gcloud.storage.bucket.Bucket` :returns: The newly created bucket. - :raises: :class:`gcloud.storage.exceptions.Conflict` if + :raises: :class:`gcloud.exceptions.Conflict` if there is a confict (bucket already exists, invalid name, etc.) """ bucket = self.new_bucket(bucket) @@ -433,7 +433,7 @@ def create_bucket(self, bucket): data={'name': bucket.name}) return Bucket(properties=response, connection=self) - def delete_bucket(self, bucket, force=False): + def delete_bucket(self, bucket): """Delete a bucket. You can use this method to delete a bucket by name, or to delete @@ -442,16 +442,14 @@ def delete_bucket(self, bucket, force=False): >>> from gcloud import storage >>> connection = storage.get_connection(project) >>> connection.delete_bucket('my-bucket') - True You can also delete pass in the bucket object:: >>> bucket = connection.get_bucket('other-bucket') >>> connection.delete_bucket(bucket) - True If the bucket doesn't exist, this will raise a - :class:`gcloud.storage.exceptions.NotFound`:: + :class:`gcloud.exceptions.NotFound`:: >>> from gcloud.exceptions import NotFound >>> try: @@ -459,28 +457,20 @@ def delete_bucket(self, bucket, force=False): >>> except NotFound: >>> print 'That bucket does not exist!' - :type bucket: string or :class:`gcloud.storage.bucket.Bucket` - :param bucket: The bucket name (or bucket object) to create. + If the bucket still has objects in it, this will raise a + :class:`gcloud.exceptions.Conflict`:: - :type force: boolean - :param full: If True, empties the bucket's objects then deletes it. + >>> from gcloud.exceptions import Conflict + >>> try: + >>> connection.delete_bucket('my-bucket') + >>> except Conflict: + >>> print 'That bucket is not empty!' - :rtype: boolean - :returns: True if the bucket was deleted. - :raises: :class:`gcloud.storage.exceptions.NotFound` if the - bucket doesn't exist, or - :class:`gcloud.storage.exceptions.Conflict` if the - bucket has blobs and `force` is not passed. + :type bucket: string or :class:`gcloud.storage.bucket.Bucket` + :param bucket: The bucket name (or bucket object) to delete. """ bucket = self.new_bucket(bucket) - - # This force delete operation is slow. - if force: - for blob in bucket: - blob.delete() - self.api_request(method='DELETE', path=bucket.path) - return True def new_bucket(self, bucket): """Factory method for creating a new (unsaved) bucket object. diff --git a/gcloud/storage/test_bucket.py b/gcloud/storage/test_bucket.py index 886aa6d5b4eb..89ae97462d82 100644 --- a/gcloud/storage/test_bucket.py +++ b/gcloud/storage/test_bucket.py @@ -282,15 +282,63 @@ def test_delete_default_miss(self): connection = _Connection() bucket = self._makeOne(connection, NAME) self.assertRaises(NotFound, bucket.delete) - self.assertEqual(connection._deleted, [(NAME, False)]) + self.assertEqual(connection._deleted, [NAME]) def test_delete_explicit_hit(self): NAME = 'name' - connection = _Connection() + GET_BLOBS_RESP = {'items': []} + connection = _Connection(GET_BLOBS_RESP) + connection._delete_ok = True + bucket = self._makeOne(connection, NAME) + self.assertEqual(bucket.delete(force=True), None) + self.assertEqual(connection._deleted, [NAME]) + + def test_delete_explicit_force_delete_blobs(self): + NAME = 'name' + BLOB_NAME1 = 'blob-name1' + BLOB_NAME2 = 'blob-name2' + GET_BLOBS_RESP = { + 'items': [ + {'name': BLOB_NAME1}, + {'name': BLOB_NAME2}, + ], + } + DELETE_BLOB1_RESP = DELETE_BLOB2_RESP = {} + connection = _Connection(GET_BLOBS_RESP, DELETE_BLOB1_RESP, + DELETE_BLOB2_RESP) + connection._delete_ok = True + bucket = self._makeOne(connection, NAME) + self.assertEqual(bucket.delete(force=True), None) + self.assertEqual(connection._deleted, [NAME]) + + def test_delete_explicit_force_miss_blobs(self): + NAME = 'name' + BLOB_NAME = 'blob-name1' + GET_BLOBS_RESP = {'items': [{'name': BLOB_NAME}]} + # Note the connection does not have a response for the blob. + connection = _Connection(GET_BLOBS_RESP) + connection._delete_ok = True + bucket = self._makeOne(connection, NAME) + self.assertEqual(bucket.delete(force=True), None) + self.assertEqual(connection._deleted, [NAME]) + + def test_delete_explicit_too_many(self): + NAME = 'name' + BLOB_NAME1 = 'blob-name1' + BLOB_NAME2 = 'blob-name2' + GET_BLOBS_RESP = { + 'items': [ + {'name': BLOB_NAME1}, + {'name': BLOB_NAME2}, + ], + } + connection = _Connection(GET_BLOBS_RESP) connection._delete_ok = True bucket = self._makeOne(connection, NAME) - self.assertTrue(bucket.delete(True)) - self.assertEqual(connection._deleted, [(NAME, True)]) + + # Make the Bucket refuse to delete with 2 objects. + bucket._MAX_OBJECTS_FOR_BUCKET_DELETE = 1 + self.assertRaises(ValueError, bucket.delete, force=True) def test_delete_blob_miss(self): from gcloud.exceptions import NotFound @@ -992,9 +1040,9 @@ def api_request(self, **kw): else: return response - def delete_bucket(self, bucket, force=False): + def delete_bucket(self, bucket): from gcloud.exceptions import NotFound - self._deleted.append((bucket, force)) + self._deleted.append(bucket) if not self._delete_ok: raise NotFound('miss') return True diff --git a/gcloud/storage/test_connection.py b/gcloud/storage/test_connection.py index 1e10f1928cd4..cd2e59872a5e 100644 --- a/gcloud/storage/test_connection.py +++ b/gcloud/storage/test_connection.py @@ -530,55 +530,11 @@ def _new_bucket(name): return _Bucket(name) conn.new_bucket = _new_bucket - self.assertEqual(conn.delete_bucket(BLOB_NAME), True) + self.assertEqual(conn.delete_bucket(BLOB_NAME), None) self.assertEqual(_deleted_blobs, []) self.assertEqual(http._called_with['method'], 'DELETE') self.assertEqual(http._called_with['uri'], URI) - def test_delete_bucket_force_True(self): - _deleted_blobs = [] - - class _Blob(object): - - def __init__(self, name): - self._name = name - - def delete(self): - _deleted_blobs.append(self._name) - - class _Bucket(object): - - def __init__(self, name): - self._name = name - self.path = '/b/' + name - - def __iter__(self): - return iter([_Blob(x) for x in ('foo', 'bar')]) - - PROJECT = 'project' - BLOB_NAME = 'blob-name' - conn = self._makeOne(PROJECT) - URI = '/'.join([ - conn.API_BASE_URL, - 'storage', - conn.API_VERSION, - 'b', - '%s?project=%s' % (BLOB_NAME, PROJECT), - ]) - http = conn._http = Http( - {'status': '200', 'content-type': 'application/json'}, - '{}', - ) - - def _new_bucket(name): - return _Bucket(name) - - conn.new_bucket = _new_bucket - self.assertEqual(conn.delete_bucket(BLOB_NAME, True), True) - self.assertEqual(_deleted_blobs, ['foo', 'bar']) - self.assertEqual(http._called_with['method'], 'DELETE') - self.assertEqual(http._called_with['uri'], URI) - def test_new_bucket_w_existing(self): from gcloud.storage.bucket import Bucket PROJECT = 'project' diff --git a/regression/storage.py b/regression/storage.py index 4327522c7e61..f7a657452e9c 100644 --- a/regression/storage.py +++ b/regression/storage.py @@ -42,20 +42,9 @@ def setUpModule(): SHARED_BUCKETS['test_bucket'] = CONNECTION.create_bucket(bucket_name) -def safe_delete(bucket): - for blob in bucket: - try: - blob.delete() - except exceptions.NotFound: - print('Delete failed with 404: %r' % (blob,)) - - # Passing force=False does not try to delete the contained files. - bucket.delete(force=False) - - def tearDownModule(): for bucket in SHARED_BUCKETS.values(): - safe_delete(bucket) + bucket.delete(force=True) class TestStorageBuckets(unittest2.TestCase):