From b91e57d6ca314ac4feaec30bf355fcf7ac4468c0 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Thu, 21 Jan 2021 15:41:48 -0800 Subject: [PATCH] fix: Amend default retry behavior for bucket operations on client (#358) * fix: Amend default retry behavior for bucket operations on client * Amend default for blob/bucket get and exist methods * fix tests for new behavior --- google/cloud/storage/blob.py | 2 +- google/cloud/storage/bucket.py | 4 +- google/cloud/storage/client.py | 7 ++-- tests/unit/test_blob.py | 8 ++-- tests/unit/test_bucket.py | 6 +-- tests/unit/test_client.py | 71 ++++++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 14 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 8f6ba13f6..8564f8e0d 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -597,7 +597,7 @@ def exists( if_generation_not_match=None, if_metageneration_match=None, if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + retry=DEFAULT_RETRY, ): """Determines whether or not this blob exists. diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index 3b51d9f82..3c8e6da9c 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -723,7 +723,7 @@ def exists( timeout=_DEFAULT_TIMEOUT, if_metageneration_match=None, if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + retry=DEFAULT_RETRY, ): """Determines whether or not this bucket exists. @@ -1108,7 +1108,7 @@ def get_blob( if_generation_not_match=None, if_metageneration_match=None, if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + retry=DEFAULT_RETRY, **kwargs ): """Get a blob object by name. diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index 42358ef68..8812dc32e 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -52,7 +52,6 @@ from google.cloud.storage.acl import DefaultObjectACL 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 _marker = object() @@ -320,7 +319,7 @@ def get_bucket( timeout=_DEFAULT_TIMEOUT, if_metageneration_match=None, if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + retry=DEFAULT_RETRY, ): """API call: retrieve a bucket via a GET request. @@ -407,7 +406,7 @@ def lookup_bucket( timeout=_DEFAULT_TIMEOUT, if_metageneration_match=None, if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + retry=DEFAULT_RETRY, ): """Get a bucket by name, returning None if not found. @@ -865,7 +864,7 @@ def list_blobs( path = bucket.path + "/o" api_request = functools.partial( - self._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY + self._connection.api_request, timeout=timeout, retry=retry ) iterator = page_iterator.HTTPIterator( client=self, diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 3d0d470f6..cd6ecafa0 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -669,7 +669,7 @@ def test_exists_miss(self): "query_params": {"fields": "name"}, "_target_object": None, "timeout": 42, - "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, }, ) @@ -692,7 +692,7 @@ def test_exists_hit_w_user_project(self): "query_params": {"fields": "name", "userProject": USER_PROJECT}, "_target_object": None, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, }, ) @@ -715,7 +715,7 @@ def test_exists_hit_w_generation(self): "query_params": {"fields": "name", "generation": GENERATION}, "_target_object": None, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, }, ) @@ -749,7 +749,7 @@ def test_exists_w_generation_match(self): }, "_target_object": None, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, }, ) diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index f3f2b4cd0..ae60c18e4 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -672,7 +672,7 @@ def api_request(cls, *args, **kwargs): "query_params": {"fields": "name"}, "_target_object": None, "timeout": 42, - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, } expected_cw = [((), expected_called_kwargs)] self.assertEqual(_FakeConnection._called_with, expected_cw) @@ -707,7 +707,7 @@ def api_request(cls, *args, **kwargs): }, "_target_object": None, "timeout": 42, - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, } expected_cw = [((), expected_called_kwargs)] self.assertEqual(_FakeConnection._called_with, expected_cw) @@ -735,7 +735,7 @@ def api_request(cls, *args, **kwargs): "query_params": {"fields": "name", "userProject": USER_PROJECT}, "_target_object": None, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + "retry": DEFAULT_RETRY, } expected_cw = [((), expected_called_kwargs)] self.assertEqual(_FakeConnection._called_with, expected_cw) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index ee0e387dd..a4c23d7cc 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -562,6 +562,54 @@ def test_get_bucket_with_object_hit(self): parms = dict(urlparse.parse_qsl(qs)) self.assertEqual(parms["projection"], "noAcl") + def test_get_bucket_default_retry(self): + from google.cloud.storage.bucket import Bucket + from google.cloud.storage._http import Connection + + PROJECT = "PROJECT" + CREDENTIALS = _make_credentials() + client = self._make_one(project=PROJECT, credentials=CREDENTIALS) + + bucket_name = "bucket-name" + bucket_obj = Bucket(client, bucket_name) + + with mock.patch.object(Connection, "api_request") as req: + client.get_bucket(bucket_obj) + + req.assert_called_once_with( + method="GET", + path=mock.ANY, + query_params=mock.ANY, + headers=mock.ANY, + _target_object=bucket_obj, + timeout=mock.ANY, + retry=DEFAULT_RETRY, + ) + + def test_get_bucket_respects_retry_override(self): + from google.cloud.storage.bucket import Bucket + from google.cloud.storage._http import Connection + + PROJECT = "PROJECT" + CREDENTIALS = _make_credentials() + client = self._make_one(project=PROJECT, credentials=CREDENTIALS) + + bucket_name = "bucket-name" + bucket_obj = Bucket(client, bucket_name) + + with mock.patch.object(Connection, "api_request") as req: + client.get_bucket(bucket_obj, retry=None) + + req.assert_called_once_with( + method="GET", + path=mock.ANY, + query_params=mock.ANY, + headers=mock.ANY, + _target_object=bucket_obj, + timeout=mock.ANY, + retry=None, + ) + def test_lookup_bucket_miss(self): PROJECT = "PROJECT" CREDENTIALS = _make_credentials() @@ -658,6 +706,29 @@ def test_lookup_bucket_with_metageneration_match(self): self.assertEqual(parms["projection"], "noAcl") self.assertEqual(parms["ifMetagenerationMatch"], str(METAGENERATION_NUMBER)) + def test_lookup_bucket_default_retry(self): + from google.cloud.storage.bucket import Bucket + from google.cloud.storage._http import Connection + + PROJECT = "PROJECT" + CREDENTIALS = _make_credentials() + client = self._make_one(project=PROJECT, credentials=CREDENTIALS) + + bucket_name = "bucket-name" + bucket_obj = Bucket(client, bucket_name) + + with mock.patch.object(Connection, "api_request") as req: + client.lookup_bucket(bucket_obj) + req.assert_called_once_with( + method="GET", + path=mock.ANY, + query_params=mock.ANY, + headers=mock.ANY, + _target_object=bucket_obj, + timeout=mock.ANY, + retry=DEFAULT_RETRY, + ) + def test_create_bucket_w_missing_client_project(self): credentials = _make_credentials() client = self._make_one(project=None, credentials=credentials)