From 99cf26117d4fe4ec6b08702423389badae04ed45 Mon Sep 17 00:00:00 2001 From: Thomas Schultz Date: Fri, 23 Sep 2016 16:27:06 -0400 Subject: [PATCH 1/3] Preserve ACL when copying or renaming blob. Closes #2389 --- storage/google/cloud/storage/bucket.py | 17 +++++++++++--- storage/unit_tests/test_bucket.py | 31 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 086e697f1a92..87aabda27af5 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -440,7 +440,7 @@ def delete_blobs(self, blobs, on_error=None, client=None): raise def copy_blob(self, blob, destination_bucket, new_name=None, - client=None): + client=None, preserve_acl=False): """Copy the given blob to the given bucket, optionally with a new name. :type blob: :class:`google.cloud.storage.blob.Blob` @@ -458,6 +458,10 @@ def copy_blob(self, blob, destination_bucket, new_name=None, :param client: Optional. The client to use. If not passed, falls back to the ``client`` stored on the current bucket. + :type preserve_acl: bool + :param preserve_acl: Optional. Copies ACL from old blob to new blob. + Default: False. + :rtype: :class:`google.cloud.storage.blob.Blob` :returns: The new Blob. """ @@ -469,9 +473,11 @@ def copy_blob(self, blob, destination_bucket, new_name=None, copy_result = client.connection.api_request( method='POST', path=api_path, _target_object=new_blob) new_blob._set_properties(copy_result) + if preserve_acl: + new_blob.acl.save(blob.acl) return new_blob - def rename_blob(self, blob, new_name, client=None): + def rename_blob(self, blob, new_name, client=None, preserve_acl=False): """Rename the given blob using copy and delete operations. Effectively, copies blob to the same bucket with a new name, then @@ -494,10 +500,15 @@ def rename_blob(self, blob, new_name, client=None): :param client: Optional. The client to use. If not passed, falls back to the ``client`` stored on the current bucket. + :type preserve_acl: bool + :param preserve_acl: Optional. Copies ACL from old blob to renamed + blob. Default: False. + :rtype: :class:`Blob` :returns: The newly-renamed blob. """ - new_blob = self.copy_blob(blob, self, new_name, client=client) + new_blob = self.copy_blob(blob, self, new_name, client=client, + preserve_acl=preserve_acl) blob.delete(client=client) return new_blob diff --git a/storage/unit_tests/test_bucket.py b/storage/unit_tests/test_bucket.py index 8f378befd744..7ddacfa8c4d3 100644 --- a/storage/unit_tests/test_bucket.py +++ b/storage/unit_tests/test_bucket.py @@ -534,6 +534,37 @@ class _Blob(object): self.assertEqual(kw['method'], 'POST') self.assertEqual(kw['path'], COPY_PATH) + def test_copy_blobs_preserve_acl(self): + from google.cloud._testing import _Monkey + from google.cloud.storage.acl import ACL + from google.cloud.storage.acl import ObjectACL + SOURCE = 'source' + DEST = 'dest' + BLOB_NAME = 'blob-name' + NEW_NAME = 'new_name' + + def save(self, client): # pylint: disable=unused-argument + return + + class _Blob(object): + name = BLOB_NAME + path = '/b/%s/o/%s' % (SOURCE, BLOB_NAME) + + def __init__(self): + self.acl = ACL() + + connection = _Connection({}) + client = _Client(connection) + source = self._makeOne(client=client, name=SOURCE) + dest = self._makeOne(client=client, name=DEST) + blob = _Blob() + with _Monkey(ACL, save=save): + new_blob = source.copy_blob(blob, dest, NEW_NAME, client=client, + preserve_acl=True) + self.assertIs(new_blob.bucket, dest) + self.assertEqual(new_blob.name, NEW_NAME) + self.assertIsInstance(new_blob.acl, ObjectACL) + def test_copy_blobs_w_name(self): SOURCE = 'source' DEST = 'dest' From 4ce1fccc8f83c2c93a3d8d6f0f25ef1b0cfd599b Mon Sep 17 00:00:00 2001 From: Thomas Schultz Date: Sun, 25 Sep 2016 21:29:33 -0400 Subject: [PATCH 2/3] Switch preserve_acl default to True. --- storage/google/cloud/storage/bucket.py | 18 +++++++----------- storage/unit_tests/test_bucket.py | 8 ++++++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 87aabda27af5..699b2e70794c 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -440,7 +440,7 @@ def delete_blobs(self, blobs, on_error=None, client=None): raise def copy_blob(self, blob, destination_bucket, new_name=None, - client=None, preserve_acl=False): + client=None, preserve_acl=True): """Copy the given blob to the given bucket, optionally with a new name. :type blob: :class:`google.cloud.storage.blob.Blob` @@ -460,7 +460,7 @@ def copy_blob(self, blob, destination_bucket, new_name=None, :type preserve_acl: bool :param preserve_acl: Optional. Copies ACL from old blob to new blob. - Default: False. + Default: True. :rtype: :class:`google.cloud.storage.blob.Blob` :returns: The new Blob. @@ -470,14 +470,15 @@ def copy_blob(self, blob, destination_bucket, new_name=None, new_name = blob.name new_blob = Blob(bucket=destination_bucket, name=new_name) api_path = blob.path + '/copyTo' + new_blob.path + if not preserve_acl: + new_blob.acl.reset() + new_blob.acl.save() copy_result = client.connection.api_request( method='POST', path=api_path, _target_object=new_blob) new_blob._set_properties(copy_result) - if preserve_acl: - new_blob.acl.save(blob.acl) return new_blob - def rename_blob(self, blob, new_name, client=None, preserve_acl=False): + def rename_blob(self, blob, new_name, client=None): """Rename the given blob using copy and delete operations. Effectively, copies blob to the same bucket with a new name, then @@ -500,15 +501,10 @@ def rename_blob(self, blob, new_name, client=None, preserve_acl=False): :param client: Optional. The client to use. If not passed, falls back to the ``client`` stored on the current bucket. - :type preserve_acl: bool - :param preserve_acl: Optional. Copies ACL from old blob to renamed - blob. Default: False. - :rtype: :class:`Blob` :returns: The newly-renamed blob. """ - new_blob = self.copy_blob(blob, self, new_name, client=client, - preserve_acl=preserve_acl) + new_blob = self.copy_blob(blob, self, new_name, client=client) blob.delete(client=client) return new_blob diff --git a/storage/unit_tests/test_bucket.py b/storage/unit_tests/test_bucket.py index 7ddacfa8c4d3..e8c0cf25ddfd 100644 --- a/storage/unit_tests/test_bucket.py +++ b/storage/unit_tests/test_bucket.py @@ -543,7 +543,7 @@ def test_copy_blobs_preserve_acl(self): BLOB_NAME = 'blob-name' NEW_NAME = 'new_name' - def save(self, client): # pylint: disable=unused-argument + def save(self): # pylint: disable=unused-argument return class _Blob(object): @@ -552,6 +552,8 @@ class _Blob(object): def __init__(self): self.acl = ACL() + self.acl.loaded = True + self.acl.entity('type', 'id') connection = _Connection({}) client = _Client(connection) @@ -560,10 +562,12 @@ def __init__(self): blob = _Blob() with _Monkey(ACL, save=save): new_blob = source.copy_blob(blob, dest, NEW_NAME, client=client, - preserve_acl=True) + preserve_acl=False) self.assertIs(new_blob.bucket, dest) self.assertEqual(new_blob.name, NEW_NAME) self.assertIsInstance(new_blob.acl, ObjectACL) + self.assertFalse(new_blob.acl.loaded) + self.assertEqual(new_blob.acl.entities, {}) def test_copy_blobs_w_name(self): SOURCE = 'source' From 04ea4f730f817562e57432cc80ca96711289e4ef Mon Sep 17 00:00:00 2001 From: Thomas Schultz Date: Tue, 27 Sep 2016 17:52:44 -0400 Subject: [PATCH 3/3] Correct order of operations. Add more testing. --- storage/google/cloud/storage/bucket.py | 5 ++-- storage/unit_tests/test_bucket.py | 37 ++++++++++++-------------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 699b2e70794c..d1b83f8ce5a2 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -470,11 +470,10 @@ def copy_blob(self, blob, destination_bucket, new_name=None, new_name = blob.name new_blob = Blob(bucket=destination_bucket, name=new_name) api_path = blob.path + '/copyTo' + new_blob.path - if not preserve_acl: - new_blob.acl.reset() - new_blob.acl.save() copy_result = client.connection.api_request( method='POST', path=api_path, _target_object=new_blob) + if not preserve_acl: + new_blob.acl.save(acl={}, client=client) new_blob._set_properties(copy_result) return new_blob diff --git a/storage/unit_tests/test_bucket.py b/storage/unit_tests/test_bucket.py index e8c0cf25ddfd..c8e598d0a27d 100644 --- a/storage/unit_tests/test_bucket.py +++ b/storage/unit_tests/test_bucket.py @@ -535,39 +535,36 @@ class _Blob(object): self.assertEqual(kw['path'], COPY_PATH) def test_copy_blobs_preserve_acl(self): - from google.cloud._testing import _Monkey - from google.cloud.storage.acl import ACL from google.cloud.storage.acl import ObjectACL SOURCE = 'source' DEST = 'dest' BLOB_NAME = 'blob-name' NEW_NAME = 'new_name' - - def save(self): # pylint: disable=unused-argument - return + BLOB_PATH = '/b/%s/o/%s' % (SOURCE, BLOB_NAME) + NEW_BLOB_PATH = '/b/%s/o/%s' % (DEST, NEW_NAME) + COPY_PATH = '/b/%s/o/%s/copyTo/b/%s/o/%s' % (SOURCE, BLOB_NAME, + DEST, NEW_NAME) class _Blob(object): name = BLOB_NAME - path = '/b/%s/o/%s' % (SOURCE, BLOB_NAME) - - def __init__(self): - self.acl = ACL() - self.acl.loaded = True - self.acl.entity('type', 'id') + path = BLOB_PATH - connection = _Connection({}) + connection = _Connection({}, {}) client = _Client(connection) source = self._makeOne(client=client, name=SOURCE) dest = self._makeOne(client=client, name=DEST) blob = _Blob() - with _Monkey(ACL, save=save): - new_blob = source.copy_blob(blob, dest, NEW_NAME, client=client, - preserve_acl=False) - self.assertIs(new_blob.bucket, dest) - self.assertEqual(new_blob.name, NEW_NAME) - self.assertIsInstance(new_blob.acl, ObjectACL) - self.assertFalse(new_blob.acl.loaded) - self.assertEqual(new_blob.acl.entities, {}) + new_blob = source.copy_blob(blob, dest, NEW_NAME, client=client, + preserve_acl=False) + self.assertIs(new_blob.bucket, dest) + self.assertEqual(new_blob.name, NEW_NAME) + self.assertIsInstance(new_blob.acl, ObjectACL) + kw = connection._requested + self.assertEqual(len(kw), 2) + self.assertEqual(kw[0]['method'], 'POST') + self.assertEqual(kw[0]['path'], COPY_PATH) + self.assertEqual(kw[1]['method'], 'PATCH') + self.assertEqual(kw[1]['path'], NEW_BLOB_PATH) def test_copy_blobs_w_name(self): SOURCE = 'source'