Merge pull request #581 from dhermes/storage-fix-564

Making Bucket.delete() work in face of eventual consistency.

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)

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,15 +425,15 @@ 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)
         response = self.api_request(method='POST', path='/b',
                                     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,45 +442,35 @@ 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:
           >>>   connection.delete_bucket('my-bucket')
           >>> 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.

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

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'

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):