From b182c45ef26090b0f83381ca682d7686b0a6c83a Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 8 Sep 2021 12:28:12 -0700 Subject: [PATCH 1/8] chore: Update CODEOWNERS for @googleapis/cloud-storage-dpe (#583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-storage/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes # 🦕 --- .github/CODEOWNERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 6b8ab74e1..fe940a679 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -5,6 +5,6 @@ # https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax -# The storage-dpe team is the default owner for anything not +# The cloud-storage-dpe team is the default owner for anything not # explicitly taken by someone else. -* @googleapis/storage-dpe @googleapis/yoshi-python +* @googleapis/cloud-storage-dpe @googleapis/yoshi-python From a0f05a6a7176e531af1768ffad12415dbef589b2 Mon Sep 17 00:00:00 2001 From: cojenco Date: Wed, 8 Sep 2021 13:19:10 -0700 Subject: [PATCH 2/8] tests: add retry conformance test cases (#580) * add s4 never idempotent methods to json schema * add project based method test cases * add hmacKey.update method test case * add ACL methods test cases * add instruction cases and update test names * fix typo --- .../conformance/retry_strategy_test_data.json | 39 ++- tests/conformance/test_conformance.py | 222 +++++++++++++++++- 2 files changed, 246 insertions(+), 15 deletions(-) diff --git a/tests/conformance/retry_strategy_test_data.json b/tests/conformance/retry_strategy_test_data.json index f37d309e9..9ed6cbdcd 100644 --- a/tests/conformance/retry_strategy_test_data.json +++ b/tests/conformance/retry_strategy_test_data.json @@ -6,6 +6,12 @@ "cases": [ { "instructions": ["return-503", "return-503"] + }, + { + "instructions": ["return-reset-connection", "return-reset-connection"] + }, + { + "instructions": ["return-reset-connection", "return-503"] } ], "methods": [ @@ -41,6 +47,12 @@ "cases": [ { "instructions": ["return-503", "return-503"] + }, + { + "instructions": ["return-reset-connection", "return-reset-connection"] + }, + { + "instructions": ["return-reset-connection", "return-503"] } ], "methods": [ @@ -65,6 +77,9 @@ "cases": [ { "instructions": ["return-503"] + }, + { + "instructions": ["return-reset-connection"] } ], "methods": [ @@ -85,13 +100,31 @@ }, { "id": 4, - "description": "non idempotent", + "description": "non_idempotent", "cases": [ { - "instructions": [] + "instructions": ["return-503"] + }, + { + "instructions": ["return-reset-connection"] } ], - "methods": [], + "methods": [ + {"name": "storage.bucket_acl.delete", "resources": ["BUCKET"]}, + {"name": "storage.bucket_acl.insert", "resources": ["BUCKET"]}, + {"name": "storage.bucket_acl.patch", "resources": ["BUCKET"]}, + {"name": "storage.bucket_acl.update", "resources": ["BUCKET"]}, + {"name": "storage.default_object_acl.delete", "resources": ["BUCKET"]}, + {"name": "storage.default_object_acl.insert", "resources": ["BUCKET"]}, + {"name": "storage.default_object_acl.patch", "resources": ["BUCKET"]}, + {"name": "storage.default_object_acl.update", "resources": ["BUCKET"]}, + {"name": "storage.hmacKey.create", "resources": []}, + {"name": "storage.notifications.insert", "resources": ["BUCKET"]}, + {"name": "storage.object_acl.delete", "resources": ["BUCKET", "OBJECT"]}, + {"name": "storage.object_acl.insert", "resources": ["BUCKET", "OBJECT"]}, + {"name": "storage.object_acl.patch", "resources": ["BUCKET", "OBJECT"]}, + {"name": "storage.object_acl.update", "resources": ["BUCKET", "OBJECT"]} + ], "preconditionProvided": false, "expectSuccess": false } diff --git a/tests/conformance/test_conformance.py b/tests/conformance/test_conformance.py index 468895835..9ae48455f 100644 --- a/tests/conformance/test_conformance.py +++ b/tests/conformance/test_conformance.py @@ -24,6 +24,7 @@ from google.cloud import storage from google.auth.credentials import AnonymousCredentials +from google.cloud.storage.hmac_key import HMACKeyMetadata from . import _read_local_json @@ -42,6 +43,9 @@ _STRING_CONTENT = "hello world" _BYTE_CONTENT = b"12345678" +_BUCKET_ACL_PATCH_MSG = "BucketACL patch operations call storage.buckets.patch, but are never idempotent; Preconditions are irrelevant." +_DEFAULT_OBJECT_ACL_PATCH_MSG = "DefaultObjectACL patch operations call storage.buckets.patch, but are never idempotent; Preconditions are irrelevant." +_OBJECT_ACL_PATCH_MSG = "ObjectACL patch operations call storage.objects.patch, but are never idempotent; Preconditions are irrelevant." ######################################################################################################################################## @@ -171,6 +175,10 @@ def bucket_lock_retention_policy(client, _preconditions, **resources): bucket.lock_retention_policy() +def client_get_service_account_email(client, _preconditions, **_): + client.get_service_account_email() + + def notification_create(client, _preconditions, **resources): bucket = client.get_bucket(resources.get("bucket").name) notification = bucket.notification() @@ -217,8 +225,44 @@ def client_list_hmac_keys(client, _preconditions, **_): pass -def client_get_service_account_email(client, _preconditions, **_): - client.get_service_account_email() +def client_get_hmac_key_metadata(client, _preconditions, **resources): + access_id = resources.get("hmac_key").access_id + client.get_hmac_key_metadata(access_id=access_id) + + +def hmac_key_exists(client, _preconditions, **resources): + access_id = resources.get("hmac_key").access_id + hmac_key = HMACKeyMetadata(client, access_id=access_id) + hmac_key.exists() + + +def hmac_key_reload(client, _preconditions, **resources): + access_id = resources.get("hmac_key").access_id + hmac_key = HMACKeyMetadata(client, access_id=access_id) + hmac_key.reload() + + +def hmac_key_delete(client, _preconditions, **resources): + access_id = resources.get("hmac_key").access_id + hmac_key = HMACKeyMetadata(client, access_id=access_id) + hmac_key.state = "INACTIVE" + hmac_key.update() + hmac_key.delete() + + +def client_create_hmac_key(client, _preconditions, **_): + client.create_hmac_key(service_account_email=_CONF_TEST_SERVICE_ACCOUNT_EMAIL) + + +def hmac_key_update(client, _preconditions, **resources): + access_id = resources.get("hmac_key").access_id + etag = resources.get("hmac_key").etag + hmac_key = HMACKeyMetadata(client, access_id=access_id) + if _preconditions: + pytest.skip("Etag is not yet supported") + hmac_key.etag = etag + hmac_key.state = "INACTIVE" + hmac_key.update() def bucket_patch(client, _preconditions, **resources): @@ -423,6 +467,131 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): blob.create_resumable_upload_session() +def blob_make_private(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_OBJECT_ACL_PATCH_MSG) + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.make_private() + + +def blob_make_public(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_OBJECT_ACL_PATCH_MSG) + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.make_public() + + +def bucket_make_private(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_BUCKET_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.make_private() + + +def bucket_make_public(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_BUCKET_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.make_public() + + +def bucket_acl_reload(client, _preconditions, **resources): + bucket = client.bucket(resources.get("bucket").name) + bucket.acl.reload() + + +def bucket_acl_save(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_BUCKET_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.acl.reload() + bucket.acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() + bucket.acl.save() + + +def bucket_acl_save_predefined(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_BUCKET_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.acl.save_predefined("bucketOwnerFullControl") + + +def bucket_acl_clear(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_BUCKET_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.acl.clear() + + +def default_object_acl_reload(client, _preconditions, **resources): + bucket = client.bucket(resources.get("bucket").name) + print(bucket.default_object_acl) + bucket.default_object_acl.reload() + + +def default_object_acl_save(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.default_object_acl.reload() + bucket.default_object_acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() + bucket.default_object_acl.save() + + +def default_object_acl_save_predefined(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.default_object_acl.save_predefined("bucketOwnerFullControl") + + +def default_object_acl_clear(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) + bucket = client.bucket(resources.get("bucket").name) + bucket.default_object_acl.clear() + + +def object_acl_reload(client, _preconditions, **resources): + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.acl.reload() + + +def object_acl_save(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_OBJECT_ACL_PATCH_MSG) + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.acl.reload() + blob.acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() + blob.acl.save() + + +def object_acl_save_predefined(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_OBJECT_ACL_PATCH_MSG) + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.acl.save_predefined("bucketOwnerFullControl") + + +def object_acl_clear(client, _preconditions, **resources): + if _preconditions: + pytest.skip(_OBJECT_ACL_PATCH_MSG) + bucket = resources.get("bucket") + object = resources.get("object") + blob = client.bucket(bucket.name).blob(object.name) + blob.acl.clear() + + ######################################################################################################################################## ### Method Invocation Mapping ########################################################################################################## ######################################################################################################################################## @@ -434,7 +603,8 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): # read or just a metadata get). method_mapping = { - "storage.buckets.delete": [bucket_delete], # S1 start + "storage.bucket_acl.list": [bucket_acl_reload], # S1 start + "storage.buckets.delete": [bucket_delete], "storage.buckets.get": [ client_get_bucket, bucket_reload, @@ -446,6 +616,14 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): "storage.buckets.list": [client_list_buckets], "storage.buckets.lockRetentionPolicy": [bucket_lock_retention_policy], "storage.buckets.testIamPermissions": [bucket_test_iam_permissions], + "storage.default_object_acl.list": [default_object_acl_reload], + "storage.hmacKey.delete": [hmac_key_delete], + "storage.hmacKey.get": [ + client_get_hmac_key_metadata, + hmac_key_exists, + hmac_key_reload, + ], + "storage.hmacKey.list": [client_list_hmac_keys], "storage.notifications.delete": [notification_delete], "storage.notifications.get": [ bucket_get_notification, @@ -453,6 +631,7 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): notification_reload, ], "storage.notifications.list": [bucket_list_notifications], + "storage.object_acl.list": [object_acl_reload], "storage.objects.get": [ bucket_get_blob, blob_exists, @@ -462,14 +641,22 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): blob_download_as_text, blobreader_read, ], - "storage.objects.list": [ - client_list_blobs, - bucket_list_blobs, - bucket_delete, - ], # S1 end - "storage.buckets.patch": [bucket_patch], # S2/S3 start + "storage.objects.list": [client_list_blobs, bucket_list_blobs, bucket_delete], + "storage.serviceaccount.get": [client_get_service_account_email], # S1 end + "storage.buckets.patch": [ + bucket_patch, + bucket_make_public, + bucket_make_private, + bucket_acl_save, + bucket_acl_save_predefined, + bucket_acl_clear, + default_object_acl_save, + default_object_acl_save_predefined, + default_object_acl_clear, + ], # S2/S3 start "storage.buckets.setIamPolicy": [bucket_set_iam_policy], "storage.buckets.update": [bucket_update], + "storage.hmacKey.update": [hmac_key_update], "storage.objects.compose": [blob_compose], "storage.objects.copy": [bucket_copy_blob, bucket_rename_blob], "storage.objects.delete": [ @@ -485,9 +672,18 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): blobwriter_write, blob_create_resumable_upload_session, ], - "storage.objects.patch": [blob_patch], + "storage.objects.patch": [ + blob_patch, + object_acl_save, + object_acl_save_predefined, + object_acl_clear, + blob_make_private, + blob_make_public, + ], "storage.objects.rewrite": [blob_rewrite, blob_update_storage_class], "storage.objects.update": [blob_update], # S2/S3 end + "storage.hmacKey.create": [client_create_hmac_key], # S4 start + "storage.notifications.insert": [notification_create], } @@ -715,7 +911,7 @@ def run_test_case( id = scenario["id"] methods = scenario["methods"] cases = scenario["cases"] - for c in cases: + for i, c in enumerate(cases): for m in methods: method_name = m["name"] if method_name not in method_mapping: @@ -723,7 +919,9 @@ def run_test_case( continue for lib_func in method_mapping[method_name]: - test_name = "test-S{}-{}-{}".format(id, method_name, lib_func.__name__) + test_name = "test-S{}-{}-{}-{}".format( + id, method_name, lib_func.__name__, i + ) globals()[test_name] = functools.partial( run_test_case, id, m, c, lib_func, host ) From 3d43049cc94f65f71932b210e6edc5f195d408c4 Mon Sep 17 00:00:00 2001 From: cojenco Date: Fri, 10 Sep 2021 15:59:35 -0700 Subject: [PATCH 3/8] tests: add retry conformance tests to run in presubmit (#585) * use subprocess to pull and run testbench docker image * remove env var check. testbench default host is set and launched in the conf tests --- noxfile.py | 8 --- tests/conformance/test_conformance.py | 71 ++++++++++++++++----------- 2 files changed, 42 insertions(+), 37 deletions(-) diff --git a/noxfile.py b/noxfile.py index d57287b73..3e6a7ceaa 100644 --- a/noxfile.py +++ b/noxfile.py @@ -156,14 +156,6 @@ def conftest_retry(session): """Run the retry conformance test suite.""" conformance_test_path = os.path.join("tests", "conformance.py") conformance_test_folder_path = os.path.join("tests", "conformance") - - # Environment check: Only run tests if the STORAGE_EMULATOR_HOST is set. - if ( - os.environ.get("STORAGE_EMULATOR_HOST", _DEFAULT_STORAGE_HOST) - == _DEFAULT_STORAGE_HOST - ): - session.skip("Set STORAGE_EMULATOR_HOST to run, skipping") - conformance_test_exists = os.path.exists(conformance_test_path) conformance_test_folder_exists = os.path.exists(conformance_test_folder_path) # Environment check: only run tests if found. diff --git a/tests/conformance/test_conformance.py b/tests/conformance/test_conformance.py index 9ae48455f..517b4fe7b 100644 --- a/tests/conformance/test_conformance.py +++ b/tests/conformance/test_conformance.py @@ -21,6 +21,10 @@ import logging import functools import pytest +import subprocess +import time + +from six.moves.urllib import parse as urlparse from google.cloud import storage from google.auth.credentials import AnonymousCredentials @@ -33,8 +37,16 @@ "retryStrategyTests" ] -_STORAGE_EMULATOR_ENV_VAR = "STORAGE_EMULATOR_HOST" -"""Environment variable defining host for Storage testbench emulator.""" +"""Environment variable or default host for Storage testbench emulator.""" +_HOST = os.environ.get("STORAGE_EMULATOR_HOST", "http://localhost:9000") +_PORT = urlparse.urlsplit(_HOST).port + +"""The storage testbench docker image info and commands.""" +_DEFAULT_IMAGE_NAME = "gcr.io/cloud-devrel-public-resources/storage-testbench" +_DEFAULT_IMAGE_TAG = "latest" +_DOCKER_IMAGE = "{}:{}".format(_DEFAULT_IMAGE_NAME, _DEFAULT_IMAGE_TAG) +_PULL_CMD = ["docker", "pull", _DOCKER_IMAGE] +_RUN_CMD = ["docker", "run", "--rm", "-d", "-p", "{}:9000".format(_PORT), _DOCKER_IMAGE] _CONF_TEST_PROJECT_ID = "my-project-id" _CONF_TEST_SERVICE_ACCOUNT_EMAIL = ( @@ -694,11 +706,10 @@ def object_acl_clear(client, _preconditions, **resources): @pytest.fixture def client(): - host = os.environ.get(_STORAGE_EMULATOR_ENV_VAR) client = storage.Client( project=_CONF_TEST_PROJECT_ID, credentials=AnonymousCredentials(), - client_options={"api_endpoint": host}, + client_options={"api_endpoint": _HOST}, ) return client @@ -900,28 +911,30 @@ def run_test_case( ### Run Conformance Tests for Retry Strategy ########################################################################################### ######################################################################################################################################## -for scenario in _CONFORMANCE_TESTS: - host = os.environ.get(_STORAGE_EMULATOR_ENV_VAR) - if host is None: - logging.error( - "This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run." - ) - break - - id = scenario["id"] - methods = scenario["methods"] - cases = scenario["cases"] - for i, c in enumerate(cases): - for m in methods: - method_name = m["name"] - if method_name not in method_mapping: - logging.info("No tests for operation {}".format(method_name)) - continue - - for lib_func in method_mapping[method_name]: - test_name = "test-S{}-{}-{}-{}".format( - id, method_name, lib_func.__name__, i - ) - globals()[test_name] = functools.partial( - run_test_case, id, m, c, lib_func, host - ) +# Pull storage-testbench docker image +subprocess.run(_PULL_CMD) +time.sleep(5) + +# Run docker image to start storage-testbench +with subprocess.Popen(_RUN_CMD) as proc: + # Run retry conformance tests + for scenario in _CONFORMANCE_TESTS: + id = scenario["id"] + methods = scenario["methods"] + cases = scenario["cases"] + for i, c in enumerate(cases): + for m in methods: + method_name = m["name"] + if method_name not in method_mapping: + logging.info("No tests for operation {}".format(method_name)) + continue + + for lib_func in method_mapping[method_name]: + test_name = "test-S{}-{}-{}-{}".format( + id, method_name, lib_func.__name__, i + ) + globals()[test_name] = functools.partial( + run_test_case, id, m, c, lib_func, _HOST + ) + time.sleep(5) + proc.kill() From 4333caf3674d78b3dfbc161a796abac604d57953 Mon Sep 17 00:00:00 2001 From: cojenco Date: Tue, 14 Sep 2021 20:02:28 -0700 Subject: [PATCH 4/8] fix: add preconditions and retry config support to ACL patch operationss (#586) * add preconditions and retry config support to ACL patch operations * update existing unit tests * add unit tests * add preconditions and retry config to bucket make public/private * add preconditions and retry config to blob make public/private * update docstrings * add system tests acl with metegeneration match * revise to use permitted group email --- google/cloud/storage/acl.py | 175 +++++++++++++++++++++++++++++++-- google/cloud/storage/blob.py | 82 ++++++++++++++- google/cloud/storage/bucket.py | 76 ++++++++++++-- tests/system/test_blob.py | 35 +++++++ tests/system/test_bucket.py | 36 +++++++ tests/unit/test_acl.py | 128 ++++++++++++++++++++++-- tests/unit/test_blob.py | 71 ++++++++++++- tests/unit/test_bucket.py | 80 +++++++++++++-- 8 files changed, 639 insertions(+), 44 deletions(-) diff --git a/google/cloud/storage/acl.py b/google/cloud/storage/acl.py index a17e4f09e..b3b77766f 100644 --- a/google/cloud/storage/acl.py +++ b/google/cloud/storage/acl.py @@ -84,8 +84,10 @@ when sending metadata for ACLs to the API. """ +from google.cloud.storage._helpers import _add_generation_match_parameters from google.cloud.storage.constants import _DEFAULT_TIMEOUT from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED class _ACLEntity(object): @@ -465,7 +467,18 @@ def reload(self, client=None, timeout=_DEFAULT_TIMEOUT, retry=DEFAULT_RETRY): for entry in found.get("items", ()): self.add_entity(self.entity_from_dict(entry)) - def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): + def _save( + self, + acl, + predefined, + client, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + timeout=_DEFAULT_TIMEOUT, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Helper for :meth:`save` and :meth:`save_predefined`. :type acl: :class:`google.cloud.storage.acl.ACL`, or a compatible list. @@ -481,12 +494,28 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): :param client: (Optional) The client to use. If not passed, falls back to the ``client`` stored on the ACL's parent. + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + :type timeout: float or tuple :param timeout: (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` - :type retry: :class:`~google.api_core.retry.Retry` + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy :param retry: (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ @@ -500,6 +529,14 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): if self.user_project is not None: query_params["userProject"] = self.user_project + _add_generation_match_parameters( + query_params, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + ) + path = self.save_path result = client._patch_resource( @@ -507,7 +544,7 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): {self._URL_PATH_ELEM: list(acl)}, query_params=query_params, timeout=timeout, - retry=None, + retry=retry, ) self.entities.clear() @@ -517,7 +554,17 @@ def _save(self, acl, predefined, client, timeout=_DEFAULT_TIMEOUT): self.loaded = True - def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): + def save( + self, + acl=None, + client=None, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + timeout=_DEFAULT_TIMEOUT, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Save this ACL for the current bucket. If :attr:`user_project` is set, bills the API request to that project. @@ -531,10 +578,30 @@ def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): :param client: (Optional) The client to use. If not passed, falls back to the ``client`` stored on the ACL's parent. + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + :type timeout: float or tuple :param timeout: (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ if acl is None: acl = self @@ -543,9 +610,29 @@ def save(self, acl=None, client=None, timeout=_DEFAULT_TIMEOUT): save_to_backend = True if save_to_backend: - self._save(acl, None, client, timeout=timeout) - - def save_predefined(self, predefined, client=None, timeout=_DEFAULT_TIMEOUT): + self._save( + acl, + None, + client, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + timeout=timeout, + retry=retry, + ) + + def save_predefined( + self, + predefined, + client=None, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + timeout=_DEFAULT_TIMEOUT, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Save this ACL for the current bucket using a predefined ACL. If :attr:`user_project` is set, bills the API request to that project. @@ -562,15 +649,54 @@ def save_predefined(self, predefined, client=None, timeout=_DEFAULT_TIMEOUT): :param client: (Optional) The client to use. If not passed, falls back to the ``client`` stored on the ACL's parent. + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + :type timeout: float or tuple :param timeout: (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ predefined = self.validate_predefined(predefined) - self._save(None, predefined, client, timeout=timeout) + self._save( + None, + predefined, + client, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + timeout=timeout, + retry=retry, + ) - def clear(self, client=None, timeout=_DEFAULT_TIMEOUT): + def clear( + self, + client=None, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + timeout=_DEFAULT_TIMEOUT, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Remove all ACL entries. If :attr:`user_project` is set, bills the API request to that project. @@ -585,12 +711,41 @@ def clear(self, client=None, timeout=_DEFAULT_TIMEOUT): :param client: (Optional) The client to use. If not passed, falls back to the ``client`` stored on the ACL's parent. + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + :type timeout: float or tuple :param timeout: (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ - self.save([], client=client, timeout=timeout) + self.save( + [], + client=client, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + timeout=timeout, + retry=retry, + ) class BucketACL(ACL): diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 781d6e0a0..737afbe31 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -83,6 +83,7 @@ from google.cloud.storage.retry import DEFAULT_RETRY from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED from google.cloud.storage.fileio import BlobReader from google.cloud.storage.fileio import BlobWriter @@ -3226,7 +3227,16 @@ def test_iam_permissions( return resp.get("permissions", []) - def make_public(self, client=None, timeout=_DEFAULT_TIMEOUT): + def make_public( + self, + client=None, + timeout=_DEFAULT_TIMEOUT, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Update blob's ACL, granting read access to anonymous users. :type client: :class:`~google.cloud.storage.client.Client` or @@ -3239,11 +3249,47 @@ def make_public(self, client=None, timeout=_DEFAULT_TIMEOUT): (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ self.acl.all().grant_read() - self.acl.save(client=client, timeout=timeout) + self.acl.save( + client=client, + timeout=timeout, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) - def make_private(self, client=None, timeout=_DEFAULT_TIMEOUT): + def make_private( + self, + client=None, + timeout=_DEFAULT_TIMEOUT, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ): """Update blob's ACL, revoking read access for anonymous users. :type client: :class:`~google.cloud.storage.client.Client` or @@ -3255,9 +3301,37 @@ def make_private(self, client=None, timeout=_DEFAULT_TIMEOUT): :param timeout: (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` """ self.acl.all().revoke_read() - self.acl.save(client=client, timeout=timeout) + self.acl.save( + client=client, + timeout=timeout, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) def compose( self, diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index d2af37ac7..8da6e09a8 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -2839,7 +2839,14 @@ def test_iam_permissions( return resp.get("permissions", []) def make_public( - self, recursive=False, future=False, client=None, timeout=_DEFAULT_TIMEOUT, + self, + recursive=False, + future=False, + client=None, + timeout=_DEFAULT_TIMEOUT, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ): """Update bucket's ACL, granting read access to anonymous users. @@ -2860,6 +2867,18 @@ def make_public( (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + :type if_metageneration_match: long + :param if_metageneration_match: (Optional) Make the operation conditional on whether the + blob's current metageneration matches the given value. + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: (Optional) Make the operation conditional on whether the + blob's current metageneration does not match the given value. + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` + :raises ValueError: If ``recursive`` is True, and the bucket contains more than 256 blobs. This is to prevent extremely long runtime of this @@ -2869,14 +2888,26 @@ def make_public( for each blob. """ self.acl.all().grant_read() - self.acl.save(client=client, timeout=timeout) + self.acl.save( + client=client, + timeout=timeout, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) if future: doa = self.default_object_acl if not doa.loaded: doa.reload(client=client, timeout=timeout) doa.all().grant_read() - doa.save(client=client, timeout=timeout) + doa.save( + client=client, + timeout=timeout, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) if recursive: blobs = list( @@ -2899,10 +2930,19 @@ def make_public( for blob in blobs: blob.acl.all().grant_read() - blob.acl.save(client=client, timeout=timeout) + blob.acl.save( + client=client, timeout=timeout, + ) def make_private( - self, recursive=False, future=False, client=None, timeout=_DEFAULT_TIMEOUT, + self, + recursive=False, + future=False, + client=None, + timeout=_DEFAULT_TIMEOUT, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ): """Update bucket's ACL, revoking read access for anonymous users. @@ -2924,6 +2964,16 @@ def make_private( (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` + :type if_metageneration_match: long + :param if_metageneration_match: (Optional) Make the operation conditional on whether the + blob's current metageneration matches the given value. + :type if_metageneration_not_match: long + :param if_metageneration_not_match: (Optional) Make the operation conditional on whether the + blob's current metageneration does not match the given value. + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: + (Optional) How to retry the RPC. See: :ref:`configuring_retries` + :raises ValueError: If ``recursive`` is True, and the bucket contains more than 256 blobs. This is to prevent extremely long runtime of this @@ -2933,14 +2983,26 @@ def make_private( for each blob. """ self.acl.all().revoke_read() - self.acl.save(client=client, timeout=timeout) + self.acl.save( + client=client, + timeout=timeout, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) if future: doa = self.default_object_acl if not doa.loaded: doa.reload(client=client, timeout=timeout) doa.all().revoke_read() - doa.save(client=client, timeout=timeout) + doa.save( + client=client, + timeout=timeout, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, + ) if recursive: blobs = list( diff --git a/tests/system/test_blob.py b/tests/system/test_blob.py index 5d654f648..c530b58ee 100644 --- a/tests/system/test_blob.py +++ b/tests/system/test_blob.py @@ -373,6 +373,41 @@ def test_blob_acl_w_user_project( assert not acl.has_entity("allUsers") +def test_blob_acl_w_metageneration_match( + shared_bucket, blobs_to_delete, file_data, service_account, +): + wrong_metageneration_number = 9 + wrong_generation_number = 6 + + blob = shared_bucket.blob("FilePatchACL") + info = file_data["simple"] + blob.upload_from_filename(info["path"]) + blobs_to_delete.append(blob) + + # Exercise blob ACL with metageneration/generation match + acl = blob.acl + blob.reload() + + with pytest.raises(exceptions.PreconditionFailed): + acl.save_predefined( + "publicRead", if_metageneration_match=wrong_metageneration_number + ) + assert "READER" not in acl.all().get_roles() + + acl.save_predefined("publicRead", if_metageneration_match=blob.metageneration) + assert "READER" in acl.all().get_roles() + + blob.reload() + del acl.entities["allUsers"] + + with pytest.raises(exceptions.PreconditionFailed): + acl.save(if_generation_match=wrong_generation_number) + assert acl.has_entity("allUsers") + + acl.save(if_generation_match=blob.generation) + assert not acl.has_entity("allUsers") + + def test_blob_acl_upload_predefined( shared_bucket, blobs_to_delete, file_data, service_account, ): diff --git a/tests/system/test_bucket.py b/tests/system/test_bucket.py index 55ea09057..9a5c76931 100644 --- a/tests/system/test_bucket.py +++ b/tests/system/test_bucket.py @@ -246,6 +246,42 @@ def test_bucket_acls_iam_w_user_project( with_user_project.set_iam_policy(policy) +def test_bucket_acls_w_metageneration_match(storage_client, buckets_to_delete): + wrong_metageneration_number = 9 + bucket_name = _helpers.unique_name("acl-w-metageneration-match") + bucket = _helpers.retry_429_503(storage_client.create_bucket)(bucket_name) + buckets_to_delete.append(bucket) + + # Exercise bucket ACL with metageneration match + acl = bucket.acl + acl.group("cloud-developer-relations@google.com").grant_read() + bucket.reload() + + with pytest.raises(exceptions.PreconditionFailed): + acl.save(if_metageneration_match=wrong_metageneration_number) + assert ( + "READER" + not in acl.group("cloud-developer-relations@google.com").get_roles() + ) + + acl.save(if_metageneration_match=bucket.metageneration) + assert "READER" in acl.group("cloud-developer-relations@google.com").get_roles() + + # Exercise default object ACL w/ metageneration match + doa = bucket.default_object_acl + doa.group("cloud-developer-relations@google.com").grant_owner() + bucket.reload() + + with pytest.raises(exceptions.PreconditionFailed): + doa.save(if_metageneration_match=wrong_metageneration_number) + assert ( + "OWNER" not in doa.group("cloud-developer-relations@google.com").get_roles() + ) + + doa.save(if_metageneration_match=bucket.metageneration) + assert "OWNER" in doa.group("cloud-developer-relations@google.com").get_roles() + + def test_bucket_copy_blob( storage_client, buckets_to_delete, blobs_to_delete, user_project, ): diff --git a/tests/unit/test_acl.py b/tests/unit/test_acl.py index aad44809e..6083ef1e1 100644 --- a/tests/unit/test_acl.py +++ b/tests/unit/test_acl.py @@ -16,7 +16,10 @@ import mock -from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import ( + DEFAULT_RETRY, + DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, +) class Test_ACLEntity(unittest.TestCase): @@ -646,7 +649,7 @@ class Derived(self._get_target_class()): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_save_no_acl_w_timeout(self): @@ -673,7 +676,7 @@ def test_save_no_acl_w_timeout(self): expected_data, query_params=expected_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_save_w_acl_w_user_project(self): @@ -705,7 +708,46 @@ def test_save_w_acl_w_user_project(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_save_w_acl_w_preconditions(self): + save_path = "/testing" + role1 = "role1" + role2 = "role2" + sticky = {"entity": "allUsers", "role": role2} + new_acl = [{"entity": "allUsers", "role": role1}] + api_response = {"acl": [sticky] + new_acl} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + acl = self._make_one() + acl.save_path = save_path + acl.loaded = True + + acl.save( + new_acl, + client=client, + if_metageneration_match=2, + if_metageneration_not_match=1, + ) + + entries = list(acl) + self.assertEqual(len(entries), 2) + self.assertTrue(sticky in entries) + self.assertTrue(new_acl[0] in entries) + + expected_data = {"acl": new_acl} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + save_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_save_prefefined_invalid(self): @@ -749,7 +791,7 @@ class Derived(self._get_target_class()): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_save_predefined_w_XML_alias_w_timeout(self): @@ -779,7 +821,7 @@ def test_save_predefined_w_XML_alias_w_timeout(self): expected_data, query_params=expected_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_save_predefined_w_alternate_query_param(self): @@ -809,7 +851,42 @@ def test_save_predefined_w_alternate_query_param(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_save_predefined_w_preconditions(self): + save_path = "/testing" + predefined = "private" + api_response = {"acl": []} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + acl = self._make_one() + acl.save_path = save_path + acl.loaded = True + + acl.save_predefined( + predefined, + client=client, + if_metageneration_match=2, + if_metageneration_not_match=1, + ) + + entries = list(acl) + self.assertEqual(len(entries), 0) + + expected_data = {"acl": []} + expected_query_params = { + "projection": "full", + "predefinedAcl": predefined, + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + save_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_clear_w_defaults(self): @@ -842,7 +919,7 @@ class Derived(self._get_target_class()): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_clear_w_explicit_client_w_timeout(self): @@ -872,7 +949,40 @@ def test_clear_w_explicit_client_w_timeout(self): expected_data, query_params=expected_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_clear_w_explicit_client_w_preconditions(self): + save_path = "/testing" + role1 = "role1" + role2 = "role2" + sticky = {"entity": "allUsers", "role": role2} + api_response = {"acl": [sticky]} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + acl = self._make_one() + acl.save_path = save_path + acl.loaded = True + acl.entity("allUsers", role1) + + acl.clear( + client=client, if_metageneration_match=2, if_metageneration_not_match=1 + ) + + self.assertEqual(list(acl), [sticky]) + + expected_data = {"acl": []} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + save_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index f74e6e111..db5212466 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -27,7 +27,10 @@ from six.moves import http_client from six.moves.urllib.parse import urlencode -from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import ( + DEFAULT_RETRY, + DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, +) from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED @@ -3936,7 +3939,7 @@ def test_make_public_w_defaults(self): expected_patch_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_make_public_w_timeout(self): @@ -3963,7 +3966,37 @@ def test_make_public_w_timeout(self): expected_patch_data, query_params=expected_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_make_public_w_preconditions(self): + from google.cloud.storage.acl import _ACLEntity + + blob_name = "blob-name" + permissive = [{"entity": "allUsers", "role": _ACLEntity.READER_ROLE}] + api_response = {"acl": permissive} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + bucket = _Bucket(client=client) + blob = self._make_one(blob_name, bucket=bucket) + blob.acl.loaded = True + + blob.make_public(if_metageneration_match=2, if_metageneration_not_match=1) + + self.assertEqual(list(blob.acl), permissive) + + expected_patch_data = {"acl": permissive} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + blob.path, + expected_patch_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_make_private_w_defaults(self): @@ -3987,7 +4020,7 @@ def test_make_private_w_defaults(self): expected_patch_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_make_private_w_timeout(self): @@ -4012,7 +4045,35 @@ def test_make_private_w_timeout(self): expected_patch_data, query_params=expected_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_make_private_w_preconditions(self): + blob_name = "blob-name" + no_permissions = [] + api_response = {"acl": no_permissions} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + bucket = _Bucket(client=client) + blob = self._make_one(blob_name, bucket=bucket) + blob.acl.loaded = True + + blob.make_private(if_metageneration_match=2, if_metageneration_not_match=1) + + self.assertEqual(list(blob.acl), no_permissions) + + expected_patch_data = {"acl": no_permissions} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + blob.path, + expected_patch_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_compose_wo_content_type_set(self): diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index cc2466ac8..a63b7fca3 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -1995,7 +1995,7 @@ def test_copy_blob_w_preserve_acl_false_w_explicit_client(self): expected_patch_data, query_params=expected_patch_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def test_copy_blob_w_name_and_user_project(self): @@ -3199,7 +3199,39 @@ def test_make_public_defaults(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_make_public_w_preconditions(self): + from google.cloud.storage.acl import _ACLEntity + + name = "name" + permissive = [{"entity": "allUsers", "role": _ACLEntity.READER_ROLE}] + api_response = {"acl": permissive, "defaultObjectAcl": []} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + bucket = self._make_one(client=client, name=name) + bucket.acl.loaded = True + bucket.default_object_acl.loaded = True + + bucket.make_public(if_metageneration_match=2, if_metageneration_not_match=1) + + self.assertEqual(list(bucket.acl), permissive) + self.assertEqual(list(bucket.default_object_acl), []) + + expected_path = bucket.path + expected_data = {"acl": permissive} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def _make_public_w_future_helper(self, default_object_acl_loaded=True): @@ -3232,7 +3264,7 @@ def _make_public_w_future_helper(self, default_object_acl_loaded=True): expected_kw = { "query_params": {"projection": "full"}, "timeout": self._get_default_timeout(), - "retry": None, + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, } client._patch_resource.assert_has_calls( [ @@ -3317,7 +3349,7 @@ def save(self, client=None, timeout=None): expected_patch_data, query_params=expected_patch_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) client.list_blobs.assert_called_once() @@ -3352,7 +3384,7 @@ def test_make_public_recursive_too_many(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) client.list_blobs.assert_called_once() @@ -3380,7 +3412,37 @@ def test_make_private_defaults(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + ) + + def test_make_private_w_preconditions(self): + name = "name" + no_permissions = [] + api_response = {"acl": no_permissions, "defaultObjectAcl": []} + client = mock.Mock(spec=["_patch_resource"]) + client._patch_resource.return_value = api_response + bucket = self._make_one(client=client, name=name) + bucket.acl.loaded = True + bucket.default_object_acl.loaded = True + + bucket.make_private(if_metageneration_match=2, if_metageneration_not_match=1) + + self.assertEqual(list(bucket.acl), no_permissions) + self.assertEqual(list(bucket.default_object_acl), []) + + expected_path = bucket.path + expected_data = {"acl": no_permissions} + expected_query_params = { + "projection": "full", + "ifMetagenerationMatch": 2, + "ifMetagenerationNotMatch": 1, + } + client._patch_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def _make_private_w_future_helper(self, default_object_acl_loaded=True): @@ -3414,7 +3476,7 @@ def _make_private_w_future_helper(self, default_object_acl_loaded=True): expected_kw = { "query_params": {"projection": "full"}, "timeout": self._get_default_timeout(), - "retry": None, + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, } client._patch_resource.assert_has_calls( [ @@ -3497,7 +3559,7 @@ def save(self, client=None, timeout=None): expected_patch_data, query_params=expected_patch_query_params, timeout=timeout, - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) client.list_blobs.assert_called_once() @@ -3531,7 +3593,7 @@ def test_make_private_recursive_too_many(self): expected_data, query_params=expected_query_params, timeout=self._get_default_timeout(), - retry=None, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) client.list_blobs.assert_called_once() From 9ca97bf9139c71cd033c78af73da904b27d8ff50 Mon Sep 17 00:00:00 2001 From: Dan Lee <71398022+dandhlee@users.noreply.github.com> Date: Thu, 16 Sep 2021 12:27:45 -0400 Subject: [PATCH 5/8] fix: pin six as a required dependency (#589) * fix: pin six as a required dependency * chore: pin protobuf for older Python 2.7 --- setup.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.py b/setup.py index 3f28dc981..191af3be2 100644 --- a/setup.py +++ b/setup.py @@ -37,7 +37,9 @@ "google-resumable-media >= 1.3.0, < 2.0dev; python_version<'3.0'", "google-resumable-media >= 1.3.0, < 3.0dev; python_version>='3.6'", "requests >= 2.18.0, < 3.0.0dev", + "protobuf < 3.18.0; python_version<'3.0'", "googleapis-common-protos < 1.53.0; python_version<'3.0'", + "six", ] extras = {} From 53f7ad0204ad425011da9162d1a78f8276c837eb Mon Sep 17 00:00:00 2001 From: cojenco Date: Thu, 16 Sep 2021 11:02:40 -0700 Subject: [PATCH 6/8] fix: add unpinned protobuf for python3 (#592) --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index 191af3be2..d679c2727 100644 --- a/setup.py +++ b/setup.py @@ -38,6 +38,7 @@ "google-resumable-media >= 1.3.0, < 3.0dev; python_version>='3.6'", "requests >= 2.18.0, < 3.0.0dev", "protobuf < 3.18.0; python_version<'3.0'", + "protobuf; python_version>='3.6'", "googleapis-common-protos < 1.53.0; python_version<'3.0'", "six", ] From 258c791d2401aa01316466fbebae589398ac9fac Mon Sep 17 00:00:00 2001 From: cojenco Date: Thu, 16 Sep 2021 12:13:17 -0700 Subject: [PATCH 7/8] tests: update retry conformance tests ACL related methods (#590) * update acl patch methods with preconditions * address comments Co-authored-by: Tres Seaver --- tests/conformance/test_conformance.py | 139 +++++++++++++++----------- 1 file changed, 80 insertions(+), 59 deletions(-) diff --git a/tests/conformance/test_conformance.py b/tests/conformance/test_conformance.py index 517b4fe7b..35b788795 100644 --- a/tests/conformance/test_conformance.py +++ b/tests/conformance/test_conformance.py @@ -26,8 +26,9 @@ from six.moves.urllib import parse as urlparse -from google.cloud import storage from google.auth.credentials import AnonymousCredentials +from google.cloud import storage +from google.cloud.exceptions import NotFound from google.cloud.storage.hmac_key import HMACKeyMetadata from . import _read_local_json @@ -55,9 +56,6 @@ _STRING_CONTENT = "hello world" _BYTE_CONTENT = b"12345678" -_BUCKET_ACL_PATCH_MSG = "BucketACL patch operations call storage.buckets.patch, but are never idempotent; Preconditions are irrelevant." -_DEFAULT_OBJECT_ACL_PATCH_MSG = "DefaultObjectACL patch operations call storage.buckets.patch, but are never idempotent; Preconditions are irrelevant." -_OBJECT_ACL_PATCH_MSG = "ObjectACL patch operations call storage.objects.patch, but are never idempotent; Preconditions are irrelevant." ######################################################################################################################################## @@ -192,7 +190,7 @@ def client_get_service_account_email(client, _preconditions, **_): def notification_create(client, _preconditions, **resources): - bucket = client.get_bucket(resources.get("bucket").name) + bucket = client.bucket(resources.get("bucket").name) notification = bucket.notification() notification.create() @@ -278,8 +276,8 @@ def hmac_key_update(client, _preconditions, **resources): def bucket_patch(client, _preconditions, **resources): - bucket = client.get_bucket(resources.get("bucket").name) - metageneration = bucket.metageneration + bucket = client.bucket(resources.get("bucket").name) + metageneration = resources.get("bucket").metageneration bucket.storage_class = "COLDLINE" if _preconditions: bucket.patch(if_metageneration_match=metageneration) @@ -288,8 +286,8 @@ def bucket_patch(client, _preconditions, **resources): def bucket_update(client, _preconditions, **resources): - bucket = client.get_bucket(resources.get("bucket").name) - metageneration = bucket.metageneration + bucket = client.bucket(resources.get("bucket").name) + metageneration = resources.get("bucket").metageneration bucket._properties = {"storageClass": "STANDARD"} if _preconditions: bucket.update(if_metageneration_match=metageneration) @@ -298,7 +296,7 @@ def bucket_update(client, _preconditions, **resources): def bucket_set_iam_policy(client, _preconditions, **resources): - bucket = client.get_bucket(resources.get("bucket").name) + bucket = client.bucket(resources.get("bucket").name) role = "roles/storage.objectViewer" member = _CONF_TEST_SERVICE_ACCOUNT_EMAIL @@ -480,35 +478,43 @@ def blob_create_resumable_upload_session(client, _preconditions, **resources): def blob_make_private(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_OBJECT_ACL_PATCH_MSG) bucket = resources.get("bucket") object = resources.get("object") blob = client.bucket(bucket.name).blob(object.name) - blob.make_private() + if _preconditions: + blob.make_private(if_metageneration_match=object.metageneration) + else: + blob.make_private() def blob_make_public(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_OBJECT_ACL_PATCH_MSG) bucket = resources.get("bucket") object = resources.get("object") blob = client.bucket(bucket.name).blob(object.name) - blob.make_public() + if _preconditions: + blob.make_public(if_metageneration_match=object.metageneration) + else: + blob.make_public() def bucket_make_private(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_BUCKET_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.make_private() + if _preconditions: + bucket.make_private( + if_metageneration_match=resources.get("bucket").metageneration + ) + else: + bucket.make_private() def bucket_make_public(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_BUCKET_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.make_public() + if _preconditions: + bucket.make_public( + if_metageneration_match=resources.get("bucket").metageneration + ) + else: + bucket.make_public() def bucket_acl_reload(client, _preconditions, **resources): @@ -517,55 +523,68 @@ def bucket_acl_reload(client, _preconditions, **resources): def bucket_acl_save(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_BUCKET_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.acl.reload() bucket.acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() - bucket.acl.save() + if _preconditions: + bucket.acl.save(if_metageneration_match=resources.get("bucket").metageneration) + else: + bucket.acl.save() def bucket_acl_save_predefined(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_BUCKET_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.acl.save_predefined("bucketOwnerFullControl") + if _preconditions: + bucket.acl.save_predefined( + "bucketOwnerFullControl", + if_metageneration_match=resources.get("bucket").metageneration, + ) + else: + bucket.acl.save_predefined("bucketOwnerFullControl") def bucket_acl_clear(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_BUCKET_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.acl.clear() + if _preconditions: + bucket.acl.clear(if_metageneration_match=resources.get("bucket").metageneration) + else: + bucket.acl.clear() def default_object_acl_reload(client, _preconditions, **resources): bucket = client.bucket(resources.get("bucket").name) - print(bucket.default_object_acl) bucket.default_object_acl.reload() def default_object_acl_save(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.default_object_acl.reload() bucket.default_object_acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() - bucket.default_object_acl.save() + if _preconditions: + bucket.default_object_acl.save( + if_metageneration_match=resources.get("bucket").metageneration + ) + else: + bucket.default_object_acl.save() def default_object_acl_save_predefined(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.default_object_acl.save_predefined("bucketOwnerFullControl") + if _preconditions: + bucket.default_object_acl.save_predefined( + "bucketOwnerFullControl", + if_metageneration_match=resources.get("bucket").metageneration, + ) + else: + bucket.default_object_acl.save_predefined("bucketOwnerFullControl") def default_object_acl_clear(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_DEFAULT_OBJECT_ACL_PATCH_MSG) bucket = client.bucket(resources.get("bucket").name) - bucket.default_object_acl.clear() + if _preconditions: + bucket.default_object_acl.clear( + if_metageneration_match=resources.get("bucket").metageneration + ) + else: + bucket.default_object_acl.clear() def object_acl_reload(client, _preconditions, **resources): @@ -576,32 +595,36 @@ def object_acl_reload(client, _preconditions, **resources): def object_acl_save(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_OBJECT_ACL_PATCH_MSG) bucket = resources.get("bucket") object = resources.get("object") blob = client.bucket(bucket.name).blob(object.name) - blob.acl.reload() blob.acl.user(_CONF_TEST_SERVICE_ACCOUNT_EMAIL).grant_owner() - blob.acl.save() + if _preconditions: + blob.acl.save(if_metageneration_match=object.metageneration) + else: + blob.acl.save() def object_acl_save_predefined(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_OBJECT_ACL_PATCH_MSG) bucket = resources.get("bucket") object = resources.get("object") blob = client.bucket(bucket.name).blob(object.name) - blob.acl.save_predefined("bucketOwnerFullControl") + if _preconditions: + blob.acl.save_predefined( + "bucketOwnerFullControl", if_metageneration_match=object.metageneration + ) + else: + blob.acl.save_predefined("bucketOwnerFullControl") def object_acl_clear(client, _preconditions, **resources): - if _preconditions: - pytest.skip(_OBJECT_ACL_PATCH_MSG) bucket = resources.get("bucket") object = resources.get("object") blob = client.bucket(bucket.name).blob(object.name) - blob.acl.clear() + if _preconditions: + blob.acl.clear(if_metageneration_match=object.metageneration) + else: + blob.acl.clear() ######################################################################################################################################## @@ -721,9 +744,7 @@ def bucket(client): yield bucket try: bucket.delete(force=True) - except Exception: - # in cases where resources are deleted within the test - # TODO(cathyo@): narrow except to NotFound once the emulator response issue is resolved + except NotFound: # in cases where bucket is deleted within the test pass @@ -735,7 +756,7 @@ def object(client, bucket): yield blob try: blob.delete() - except Exception: # in cases where resources are deleted within the test + except NotFound: # in cases where object is deleted within the test pass @@ -747,7 +768,7 @@ def notification(client, bucket): yield notification try: notification.delete() - except Exception: # in cases where resources are deleted within the test + except NotFound: # in cases where notification is deleted within the test pass @@ -762,7 +783,7 @@ def hmac_key(client): hmac_key.state = "INACTIVE" hmac_key.update() hmac_key.delete() - except Exception: # in cases where resources are deleted within the test + except NotFound: # in cases where hmac_key is deleted within the test pass From 29d85ca83a8756b800041a9e05efa258ed8ef0d6 Mon Sep 17 00:00:00 2001 From: "release-please[bot]" <55107282+release-please[bot]@users.noreply.github.com> Date: Thu, 16 Sep 2021 13:56:40 -0700 Subject: [PATCH 8/8] chore: release 1.42.2 (#587) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: cojenco --- CHANGELOG.md | 9 +++++++++ google/cloud/storage/version.py | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e98af1181..e88e144f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ [1]: https://pypi.org/project/google-cloud-storage/#history +### [1.42.2](https://www.github.com/googleapis/python-storage/compare/v1.42.1...v1.42.2) (2021-09-16) + + +### Bug Fixes + +* add preconditions and retry config support to ACL patch operationss ([#586](https://www.github.com/googleapis/python-storage/issues/586)) ([4333caf](https://www.github.com/googleapis/python-storage/commit/4333caf3674d78b3dfbc161a796abac604d57953)) +* add unpinned protobuf for python3 ([#592](https://www.github.com/googleapis/python-storage/issues/592)) ([53f7ad0](https://www.github.com/googleapis/python-storage/commit/53f7ad0204ad425011da9162d1a78f8276c837eb)) +* pin six as a required dependency ([#589](https://www.github.com/googleapis/python-storage/issues/589)) ([9ca97bf](https://www.github.com/googleapis/python-storage/commit/9ca97bf9139c71cd033c78af73da904b27d8ff50)) + ### [1.42.1](https://www.github.com/googleapis/python-storage/compare/v1.42.0...v1.42.1) (2021-09-07) diff --git a/google/cloud/storage/version.py b/google/cloud/storage/version.py index 31244e9a8..ccb2fa8f6 100644 --- a/google/cloud/storage/version.py +++ b/google/cloud/storage/version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = "1.42.1" +__version__ = "1.42.2"