From 2eda89434dc90155ddbb9c5e38b4c3e0533d5460 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Tue, 1 Aug 2017 16:41:39 -0400 Subject: [PATCH 1/7] Add 'update' API wrapper for buckets/blobs. Turns out some properties (i.e., 'labels', see #3711) behave differently under 'patch semantics'[1], which makes 'update' useful. [1] https://cloud.google.com/storage/docs/json_api/v1/how-tos/performance#patch --- storage/google/cloud/storage/_helpers.py | 16 ++++++++++++++++ storage/tests/unit/test__helpers.py | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 56a75c684f4c..66c69186c0e9 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -163,6 +163,22 @@ def update(self, client=None): 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. diff --git a/storage/tests/unit/test__helpers.py b/storage/tests/unit/test__helpers.py index 90def4867268..366c2d5d51f9 100644 --- a/storage/tests/unit/test__helpers.py +++ b/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): From 2e29e208afb427ec17a05515357ff60b1153e2bc Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 7 Aug 2017 09:00:55 -0700 Subject: [PATCH 2/7] Correctly track label removals on Bucket.patch() --- storage/google/cloud/storage/_helpers.py | 2 + storage/google/cloud/storage/bucket.py | 51 ++++++++++++++++++++++++ storage/tests/unit/test_bucket.py | 37 +++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 66c69186c0e9..742838d24c94 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/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) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 06550b09ffcb..77c235ae5bcf 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/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 '' % (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 diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 0df94dc5db3d..9aeff186940d 100644 --- a/storage/tests/unit/test_bucket.py +++ b/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' From aec63b5a9652100ac30a60ff1628f27c04ba4e0b Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 7 Aug 2017 09:06:00 -0700 Subject: [PATCH 3/7] Have the labels system text exercise .patch --- storage/tests/system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/tests/system.py b/storage/tests/system.py index bc8169c356b3..e51cfcaeccb2 100644 --- a/storage/tests/system.py +++ b/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 = {} From a17be218fc20f18a4df23f1d89c4a1fd61ce7435 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 7 Aug 2017 10:57:27 -0700 Subject: [PATCH 4/7] Rebase merge goof fix. --- storage/google/cloud/storage/_helpers.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 742838d24c94..3a4eb2e232a2 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -165,22 +165,6 @@ def update(self, client=None): 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. From d623e2167e0518c7007c168e89312c60c70dc9c3 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 7 Aug 2017 12:58:12 -0700 Subject: [PATCH 5/7] Fix another merge/rebase goof. --- storage/tests/unit/test__helpers.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/storage/tests/unit/test__helpers.py b/storage/tests/unit/test__helpers.py index 366c2d5d51f9..90def4867268 100644 --- a/storage/tests/unit/test__helpers.py +++ b/storage/tests/unit/test__helpers.py @@ -115,26 +115,6 @@ 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): From 4fc8002b89555fd166ab6ac82167b464ca62f21e Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 7 Aug 2017 13:37:11 -0700 Subject: [PATCH 6/7] Refactor based on @tseaver feedback. --- storage/google/cloud/storage/_helpers.py | 2 ++ storage/google/cloud/storage/bucket.py | 22 +--------------------- storage/tests/unit/test_bucket.py | 10 ---------- 3 files changed, 3 insertions(+), 31 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 3a4eb2e232a2..9b72dddd79a0 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -126,6 +126,8 @@ def _set_properties(self, value): self._properties = value # If the values are reset, the changes must as well. self._changes = set() + if hasattr(self, '_label_removals'): + self._label_removals.clear() def patch(self, client=None): """Sends all changed properties in a PATCH request. diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 77c235ae5bcf..8fa0f6a53574 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -219,27 +219,7 @@ def patch(self, client=None): 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 + return super(Bucket, self).patch(client=client) @property def acl(self): diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 9aeff186940d..aac11fe51c7b 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -710,16 +710,6 @@ 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. From 413da52c5e4bd82651c9f38ae77ad0bb9e34282d Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Thu, 10 Aug 2017 09:38:20 -0700 Subject: [PATCH 7/7] Override _set_properties on Bucket. --- storage/google/cloud/storage/_helpers.py | 2 -- storage/google/cloud/storage/bucket.py | 9 +++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/storage/google/cloud/storage/_helpers.py b/storage/google/cloud/storage/_helpers.py index 9b72dddd79a0..3a4eb2e232a2 100644 --- a/storage/google/cloud/storage/_helpers.py +++ b/storage/google/cloud/storage/_helpers.py @@ -126,8 +126,6 @@ def _set_properties(self, value): self._properties = value # If the values are reset, the changes must as well. self._changes = set() - if hasattr(self, '_label_removals'): - self._label_removals.clear() def patch(self, client=None): """Sends all changed properties in a PATCH request. diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 8fa0f6a53574..076cda9c5d65 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -125,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.