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

…apis#3737)

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,)
@@ -124,6 +125,15 @@ def client(self):
         """The client bound to this bucket."""
         return self._client
 
+    def _set_properties(self, value):
+        """Set the properties for the current object.
+
+        :type value: dict or :class:`google.cloud.storage.batch._FutureDict`
+        :param value: The properties to be set.
+        """
+        self._label_removals.clear()
+        return super(Bucket, self)._set_properties(value)
+
     def blob(self, blob_name, chunk_size=None, encryption_key=None):
         """Factory constructor for blob object.
 
@@ -199,6 +209,27 @@ 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.
+        return super(Bucket, self).patch(client=client)
+
     @property
     def acl(self):
         """Create our ACL on demand."""
@@ -624,6 +655,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_bucket.py

@@ -710,6 +710,33 @@ def test_labels_setter(self):
         self.assertIsNot(bucket._properties['labels'], LABELS)
         self.assertIn('labels', bucket._changes)
 
+    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'