Storage: there is no way to remove labels from a bucket

[theacodes]

Attempting to remove a label from a bucket does not appear to work:

    # {'example': 'label'}
    labels = bucket.labels

    if 'example' in labels:
        del labels['example']

    # {}
    bucket.labels = labels

    bucket.patch()

    # Still {'example': 'label'}
    pprint.pprint(bucket.labels)

[tseaver]

After stopping inside the patch call with the empty labels:

(Pdb) pp update_properties
{'labels': {}}

stepping over the API call leaves an api_response:

(Pdb) pp api_response
{'acl': [{'bucket': 'test-bucket-patch-1501616216661',
          'entity': 'project-owners-455332057385',
          'etag': 'CAM=',
          'id': 'test-bucket-patch-1501616216661/project-owners-455332057385',
          'kind': 'storage#bucketAccessControl',
          'projectTeam': {'projectNumber': '455332057385', 'team': 'owners'},
          'role': 'OWNER',
          'selfLink': 'https://www.googleapis.com/storage/v1/b/test-bucket-patch-1501616216661/acl/project-owners-455332057385'},
         {'bucket': 'test-bucket-patch-1501616216661',
          'entity': 'project-editors-455332057385',
          'etag': 'CAM=',
          'id': 'test-bucket-patch-1501616216661/project-editors-455332057385',
          'kind': 'storage#bucketAccessControl',
          'projectTeam': {'projectNumber': '455332057385', 'team': 'editors'},
          'role': 'OWNER',
          'selfLink': 'https://www.googleapis.com/storage/v1/b/test-bucket-patch-1501616216661/acl/project-editors-455332057385'},
         {'bucket': 'test-bucket-patch-1501616216661',
          'entity': 'project-viewers-455332057385',
          'etag': 'CAM=',
          'id': 'test-bucket-patch-1501616216661/project-viewers-455332057385',
          'kind': 'storage#bucketAccessControl',
          'projectTeam': {'projectNumber': '455332057385', 'team': 'viewers'},
          'role': 'READER',
          'selfLink': 'https://www.googleapis.com/storage/v1/b/test-bucket-patch-1501616216661/acl/project-viewers-455332057385'}],
 'defaultObjectAcl': [{'entity': 'project-owners-455332057385',
                       'etag': 'CAM=',
                       'kind': 'storage#objectAccessControl',
                       'projectTeam': {'projectNumber': '455332057385',
                                       'team': 'owners'},
                       'role': 'OWNER'},
                      {'entity': 'project-editors-455332057385',
                       'etag': 'CAM=',
                       'kind': 'storage#objectAccessControl',
                       'projectTeam': {'projectNumber': '455332057385',
                                       'team': 'editors'},
                       'role': 'OWNER'},
                      {'entity': 'project-viewers-455332057385',
                       'etag': 'CAM=',
                       'kind': 'storage#objectAccessControl',
                       'projectTeam': {'projectNumber': '455332057385',
                                       'team': 'viewers'},
                       'role': 'READER'}],
 'etag': 'CAM=',
 'id': 'test-bucket-patch-1501616216661',
 'kind': 'storage#bucket',
 'labels': {'test-label': 'label-value'},
 'location': 'US',
 'metageneration': '3',
 'name': 'test-bucket-patch-1501616216661',
 'owner': {'entity': 'project-owners-455332057385'},
 'projectNumber': '455332057385',
 'selfLink': 'https://www.googleapis.com/storage/v1/b/test-bucket-patch-1501616216661',
 'storageClass': 'STANDARD',
 'timeCreated': '2017-08-01T19:37:45.519Z',
 'updated': '2017-08-01T19:38:34.487Z'}

[theacodes]

@tseaver I'm not sure how to process your last comment. Are you saying we're sending the right stuff to the server?

[tseaver]

If I tweak the code to assign a different, non-empty mapping:

bucket.labels = {'another-label': 'another-value'}
bucket.patch()

It passes the new value:

(Pdb) p update_properties
{'labels': {'another-label': 'another-value'}}

But the back-end returns the union of the two:

(Pdb) pp api_response['labels']
{'another-label': 'another-value', 'test-label': 'label-value'}

[tseaver]

@jonparrott Yes, we are: we send along {'labels': {}} as the payload for the patch API.

[theacodes]

@frankyn can you figure out who to talk to about the backend? :)

[tseaver]

This is the (failing) system test I'm using to check out the payload (by adding pdb).

[tseaver]

@dhermes Maybe this is the usecase which finally justifies wrapping the buckets.update API. ;)

[tseaver]

What a hoot! My guess was right! If I hack in an update method:

diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py
index 88f9b8d..56a75c6 100644
--- a/storage/google/cloud/storage/_helpers.py
+++ b/storage/google/cloud/storage/_helpers.py
@@ -147,6 +147,22 @@ class _PropertyMixin(object):
             query_params={'projection': 'full'}, _target_object=self)
         self._set_properties(api_response)
 
+    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.
+        """
+        client = self._require_client(client)
+        api_response = client._connection.api_request(
+            method='PUT', path=self.path, data=self._properties,
+            query_params={'projection': 'full'}, _target_object=self)
+        self._set_properties(api_response)
+
 
 def _scalar_property(fieldname):
     """Create a property descriptor around the :class:`_PropertyMixin` helpers.

and tweak the system test to use it:

diff --git a/storage/tests/system.py b/storage/tests/system.py
index dfe35e3..934f761 100644
--- a/storage/tests/system.py
+++ b/storage/tests/system.py
@@ -127,11 +127,13 @@ class TestStorageBuckets(unittest.TestCase):
 
         new_labels = {'another-label': 'another-value'}
         bucket.labels = new_labels
-        bucket.patch()
+        #bucket.patch()
+        bucket.update()
         self.assertEqual(bucket.labels, new_labels)
 
         bucket.labels = {}
-        bucket.patch()
+        #bucket.patch()
+        bucket.update()
         self.assertEqual(bucket.labels, {})

it passes!

[tseaver]

Maybe something in the patch semantics docs would explain this?

[theacodes]

I think it's definitely because of patch semantics. Adding bucket.update and using that in the sample instead of patch seems like a reasonable way to solve this.

[frankyn]

After reviewing patch semantics, the issue I see here is the patch request is using the following:

{
"labels": {}
}

When it should be using:

{
"labels": null
}

To remove the data in that property. It's recommended that patch be used over update because it reduces the amount of data that's requested and sent. However, the python client library does a full projection of Bucket properties so it would be the same thing as an update..?

[tseaver]

@jonparrott #3714 adds the update method for both Blob and Bucket.

[tseaver]

@frankyn The back-end also merges existing labels with new labels under PATCH: is that expected behavior? If we wanted to clear an individual label using patch, would we need to pass it's value as null ? E.g.:

{
"labels": {"keep-me": "some-value", "delete-me": null}
}

[frankyn]

Ah I see, I don't believe so given the following comment:

Note about arrays: Patch requests that contain arrays replace the existing array with the one you provide. You cannot modify, add, or delete items in an array in a piecemeal fashion.

I'll follow-up with the POC for Labels about this.

Thanks!

[frankyn]

Disregard my last comment.. Object != Array, but I'll follow-up anyway to verify expected result.

[frankyn]

@tseaver I got a prompt response. It's an expected result for this operation. The payload you presented above is also the expected way to remove a label from the object.

Delete all labels

{
"labels": null
}

Delete a specific label

{
"labels": {"keep-me": "some-value", "delete-me": null}
}

Add a new label

{
"labels": {"new-label": "merged-label"}
}

[tseaver]

@jonparrott supporting those semantics (rather than "make arbitrary changes to the local copy and then call update", as illustrated in #3715) will need one of two strategies:

• Add label-specific methods (add_label, remove_label, clear_labels), each of which makes a separate API call. Maybe that isn't too big a downside, given how low-volume such changes are likely to be.
• Use some batch-like context manager which exposes the same kind of methods, and synthesizes from them a single "right" mapping to send in a single API call in its __exit__. More complex, higher bug potential, but more performant.

[theacodes]

By update do you mean patch? I'm fine with not suggesting patch for label usage or going with (1).

[tseaver]

By update do you mean patch?

Nope. The status quo (once #3714 / #3715 land) is to call update after re-assigning bucket.labels (because patch won't work).

[theacodes]

Oh okay, sorry, I just misread.

I'm okay with the status quo of having to use update over patch for now. In terms of which behavior seems most appropriate it seems like (2) is the right long-term approach if we want to keep patch, but (1) is the right approach if we don't. @lukesneeringer should also chime in.

[lukesneeringer]

Slight preference for (2), which I can knock out once #3714 and #3715 land.

[lukesneeringer]

There is a third option: The setter for bucket.labels can track the diff and do the right thing on a .patch() call.