Storage: Track deleted labels; make Bucket.patch() send them.

storage/google/cloud/storage/_helpers.py

@@ -142,6 +142,8 @@ def patch(self, client=None):
         # to work properly w/ 'noAcl'.
         update_properties = {key: self._properties[key]
                              for key in self._changes}
+
+        # Make the API call.
         api_response = client._connection.api_request(
             method='PATCH', path=self.path, data=update_properties,
             query_params={'projection': 'full'}, _target_object=self)

storage/google/cloud/storage/bucket.py

@@ -115,6 +115,7 @@ def __init__(self, client, name=None):
         self._client = client
         self._acl = BucketACL(self)
         self._default_object_acl = DefaultObjectACL(self)
+        self._label_removals = set()
 
     def __repr__(self):
         return '<Bucket: %s>' % (self.name,)
@@ -199,6 +200,47 @@ def create(self, client=None):
             data=properties, _target_object=self)
         self._set_properties(api_response)
 
+    def patch(self, client=None):
+        """Sends all changed properties in a PATCH request.
+
+        Updates the ``_properties`` with the response from the backend.
+
+        :type client: :class:`~google.cloud.storage.client.Client` or
+                      ``NoneType``
+        :param client: the client to use.  If not passed, falls back to the
+                       ``client`` stored on the current object.
+        """
+        # Special case: For buckets, it is possible that labels are being
+        # removed; this requires special handling.
+        if self._label_removals:
+            self._changes.add('labels')
+            self._properties.setdefault('labels', {})
+            for removed_label in self._label_removals:
+                self._properties['labels'][removed_label] = None
+
+        # Call the superclass method.
+        answer = super(Bucket, self).patch(client=client)
+
+        # Clear out the label removals.
+        # This comes after the superclass method to ensure that we hold on
+        # to the data in case of an error.
+        self._label_removals.clear()
+        return answer
+
+    def update(self, client=None):
+        """Sends all properties in a PUT request.
+
+        Updates the ``_properties`` with the response from the backend.
+
+        :type client: :class:`~google.cloud.storage.client.Client` or
+                      ``NoneType``
+        :param client: the client to use.  If not passed, falls back to the
+                       ``client`` stored on the current object.
+        """
+        answer = super(Bucket, self).update(client=client)
+        self._label_removals.clear()
+        return answer
+
     @property
     def acl(self):
         """Create our ACL on demand."""
@@ -619,6 +661,15 @@ def labels(self, mapping):
         :type mapping: :class:`dict`
         :param mapping: Name-value pairs (string->string) labelling the bucket.
         """
+        # If any labels have been expressly removed, we need to track this
+        # so that a future .patch() call can do the correct thing.
+        existing = set([k for k in self.labels.keys()])
+        incoming = set([k for k in mapping.keys()])
+        self._label_removals = self._label_removals.union(
+            existing.difference(incoming),
+        )
+
+        # Actually update the labels on the object.
         self._patch_property('labels', copy.deepcopy(mapping))
 
     @property

storage/tests/system.py

@@ -127,7 +127,7 @@ def test_bucket_update_labels(self):
 
         new_labels = {'another-label': 'another-value'}
         bucket.labels = new_labels
-        bucket.update()
+        bucket.patch()
         self.assertEqual(bucket.labels, new_labels)
 
         bucket.labels = {}

storage/tests/unit/test__helpers.py

@@ -115,6 +115,26 @@ def test_update(self):
         # Make sure changes get reset by patch().
         self.assertEqual(derived._changes, set())
 
+    def test_update(self):
+        connection = _Connection({'foo': 'Foo'})
+        client = _Client(connection)
+        derived = self._derivedClass('/path')()
+        # Make sure changes is non-empty, so we can observe a change.
+        BAR = object()
+        BAZ = object()
+        derived._properties = {'bar': BAR, 'baz': BAZ}
+        derived._changes = set(['bar'])  # Update sends 'baz' anyway.
+        derived.update(client=client)
+        self.assertEqual(derived._properties, {'foo': 'Foo'})
+        kw = connection._requested
+        self.assertEqual(len(kw), 1)
+        self.assertEqual(kw[0]['method'], 'PUT')
+        self.assertEqual(kw[0]['path'], '/path')
+        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+        self.assertEqual(kw[0]['data'], {'bar': BAR, 'baz': BAZ})
+        # Make sure changes get reset by patch().
+        self.assertEqual(derived._changes, set())
+
 
 class Test__scalar_property(unittest.TestCase):
 

storage/tests/unit/test_bucket.py

@@ -710,6 +710,43 @@ def test_labels_setter(self):
         self.assertIsNot(bucket._properties['labels'], LABELS)
         self.assertIn('labels', bucket._changes)
 
+        # Make sure that a update call correctly adds the labels.
+        client = mock.NonCallableMock(spec=('_connection',))
+        client._connection = mock.NonCallableMock(spec=('api_request',))
+        bucket.update(client=client)
+        client._connection.api_request.assert_called()
+        _, _, kwargs = client._connection.api_request.mock_calls[0]
+        self.assertEqual(len(kwargs['data']['labels']), 2)
+        self.assertEqual(kwargs['data']['labels']['color'], 'red')
+        self.assertEqual(kwargs['data']['labels']['flavor'], 'cherry')
+
+    def test_labels_setter_with_removal(self):
+        # Make sure the bucket labels look correct and follow the expected
+        # public structure.
+        bucket = self._make_one(name='name')
+        self.assertEqual(bucket.labels, {})
+        bucket.labels = {'color': 'red', 'flavor': 'cherry'}
+        self.assertEqual(bucket.labels, {'color': 'red', 'flavor': 'cherry'})
+        bucket.labels = {'color': 'red'}
+        self.assertEqual(bucket.labels, {'color': 'red'})
+
+        # Make sure that a patch call correctly removes the flavor label.
+        client = mock.NonCallableMock(spec=('_connection',))
+        client._connection = mock.NonCallableMock(spec=('api_request',))
+        bucket.patch(client=client)
+        client._connection.api_request.assert_called()
+        _, _, kwargs = client._connection.api_request.mock_calls[0]
+        self.assertEqual(len(kwargs['data']['labels']), 2)
+        self.assertEqual(kwargs['data']['labels']['color'], 'red')
+        self.assertIsNone(kwargs['data']['labels']['flavor'])
+
+        # A second patch call should be a no-op for labels.
+        client._connection.api_request.reset_mock()
+        bucket.patch(client=client)
+        client._connection.api_request.assert_called()
+        _, _, kwargs = client._connection.api_request.mock_calls[0]
+        self.assertNotIn('labels', kwargs['data'])
+
     def test_get_logging_w_prefix(self):
         NAME = 'name'
         LOG_BUCKET = 'logs'