From 8d3ff6a4ebc1af6ea782b7d14686c7db75a12006 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 2 Oct 2020 09:44:31 -0700 Subject: [PATCH 01/13] unit tests pass --- google/cloud/storage/_helpers.py | 6 ++ google/cloud/storage/_http.py | 15 +++++ google/cloud/storage/blob.py | 5 ++ google/cloud/storage/bucket.py | 17 ++++-- google/cloud/storage/client.py | 10 +-- google/cloud/storage/hmac_key.py | 5 +- google/cloud/storage/notification.py | 6 +- google/cloud/storage/retry.py | 91 ++++++++++++++++++++++++++++ tests/unit/test__helpers.py | 14 +++++ tests/unit/test_blob.py | 7 +++ tests/unit/test_bucket.py | 31 ++++++++++ tests/unit/test_client.py | 38 ++++++++++++ tests/unit/test_hmac_key.py | 6 ++ tests/unit/test_notification.py | 10 ++- tests/unit/test_retry.py | 66 ++++++++++++++++++++ 15 files changed, 314 insertions(+), 13 deletions(-) create mode 100644 google/cloud/storage/retry.py create mode 100644 tests/unit/test_retry.py diff --git a/google/cloud/storage/_helpers.py b/google/cloud/storage/_helpers.py index a1075eac7..88a0941b3 100644 --- a/google/cloud/storage/_helpers.py +++ b/google/cloud/storage/_helpers.py @@ -24,6 +24,8 @@ from six.moves.urllib.parse import urlsplit 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 STORAGE_EMULATOR_ENV_VAR = "STORAGE_EMULATOR_HOST" @@ -205,6 +207,7 @@ def reload( headers=self._encryption_headers(), _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY ) self._set_properties(api_response) @@ -306,6 +309,7 @@ def patch( query_params=query_params, _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) self._set_properties(api_response) @@ -368,6 +372,7 @@ def update( if_metageneration_match=if_metageneration_match, if_metageneration_not_match=if_metageneration_not_match, ) + api_response = client._connection.api_request( method="PUT", path=self.path, @@ -375,6 +380,7 @@ def update( query_params=query_params, _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) self._set_properties(api_response) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index 032f70e02..e39704a43 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -14,9 +14,12 @@ """Create / interact with Google Cloud Storage connections.""" +import functools + from google.cloud import _http from google.cloud.storage import __version__ +from google.cloud.storage import retry class Connection(_http.JSONConnection): @@ -46,3 +49,15 @@ def __init__(self, client, client_info=None, api_endpoint=DEFAULT_API_ENDPOINT): API_URL_TEMPLATE = "{api_base_url}/storage/{api_version}{path}" """A template for the URL of a particular API call.""" + + def api_request(self, *args, retry=None, **kwargs): + call = functools.partial(super(Connection, self).api_request, *args, **kwargs) + if retry: + # If this is a ConditionalRetryPolicy, check conditions. + try: + retry = retry.get_retry_policy_if_conditions_met(kwargs) + except AttributeError: + pass + call = retry(call) + return call() + diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index b1e13788d..54ee3869e 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -74,6 +74,9 @@ from google.cloud.storage.constants import NEARLINE_STORAGE_CLASS from google.cloud.storage.constants import REGIONAL_LEGACY_STORAGE_CLASS from google.cloud.storage.constants import STANDARD_STORAGE_CLASS +from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED _API_ACCESS_ENDPOINT = "https://storage.googleapis.com" @@ -2856,6 +2859,7 @@ def compose( data=request, _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ) self._set_properties(api_response) @@ -3000,6 +3004,7 @@ def rewrite( headers=headers, _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ) rewritten = int(api_response["totalBytesRewritten"]) size = int(api_response["objectSize"]) diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index adf37d398..92eac34c5 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -57,7 +57,9 @@ from google.cloud.storage.constants import STANDARD_STORAGE_CLASS from google.cloud.storage.notification import BucketNotification from google.cloud.storage.notification import NONE_PAYLOAD_FORMAT - +from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON _UBLA_BPO_ENABLED_MESSAGE = ( "Pass only one of 'uniform_bucket_level_access_enabled' / " @@ -1244,7 +1246,7 @@ def list_blobs( client = self._require_client(client) path = self.path + "/o" - api_request = functools.partial(client._connection.api_request, timeout=timeout) + api_request = functools.partial(client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) iterator = page_iterator.HTTPIterator( client=client, api_request=api_request, @@ -1283,7 +1285,7 @@ def list_notifications(self, client=None, timeout=_DEFAULT_TIMEOUT): """ client = self._require_client(client) path = self.path + "/notificationConfigs" - api_request = functools.partial(client._connection.api_request, timeout=timeout) + api_request = functools.partial(client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) iterator = page_iterator.HTTPIterator( client=client, api_request=api_request, @@ -1424,6 +1426,7 @@ def delete( query_params=query_params, _target_object=None, timeout=timeout, + retry=DEFAULT_RETRY ) def delete_blob( @@ -1521,6 +1524,7 @@ def delete_blob( query_params=query_params, _target_object=None, timeout=timeout, + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ) def delete_blobs( @@ -1795,6 +1799,7 @@ def copy_blob( query_params=query_params, _target_object=new_blob, timeout=timeout, + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ) if not preserve_acl: @@ -2644,6 +2649,7 @@ def get_iam_policy( query_params=query_params, _target_object=None, timeout=timeout, + retry=DEFAULT_RETRY, ) return Policy.from_api_repr(info) @@ -2689,6 +2695,7 @@ def set_iam_policy(self, policy, client=None, timeout=_DEFAULT_TIMEOUT): data=resource, _target_object=None, timeout=timeout, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, ) return Policy.from_api_repr(info) @@ -2727,7 +2734,8 @@ def test_iam_permissions(self, permissions, client=None, timeout=_DEFAULT_TIMEOU path = "%s/iam/testPermissions" % (self.path,) resp = client._connection.api_request( - method="GET", path=path, query_params=query_params, timeout=timeout + method="GET", path=path, query_params=query_params, timeout=timeout, + retry=DEFAULT_RETRY, ) return resp.get("permissions", []) @@ -2967,6 +2975,7 @@ def lock_retention_policy(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params=query_params, _target_object=self, timeout=timeout, + retry=DEFAULT_RETRY, ) self._set_properties(api_response) diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index fd29abe9c..d6f0d5633 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -45,6 +45,7 @@ from google.cloud.storage.acl import BucketACL from google.cloud.storage.acl import DefaultObjectACL from google.cloud.storage.constants import _DEFAULT_TIMEOUT +from google.cloud.storage.retry import DEFAULT_RETRY _marker = object() @@ -255,7 +256,7 @@ def get_service_account_email(self, project=None, timeout=_DEFAULT_TIMEOUT): project = self.project path = "/projects/%s/serviceAccount" % (project,) api_response = self._base_connection.api_request( - method="GET", path=path, timeout=timeout + method="GET", path=path, timeout=timeout, retry=DEFAULT_RETRY, ) return api_response["email_address"] @@ -531,6 +532,7 @@ def create_bucket( data=properties, _target_object=bucket, timeout=timeout, + retry=DEFAULT_RETRY, ) bucket._set_properties(api_response) @@ -777,7 +779,7 @@ def list_buckets( if fields is not None: extra_params["fields"] = fields - api_request = functools.partial(self._connection.api_request, timeout=timeout) + api_request = functools.partial(self._connection.api_request, retry=DEFAULT_RETRY, timeout=timeout) return page_iterator.HTTPIterator( client=self, @@ -829,7 +831,7 @@ def create_hmac_key( qs_params["userProject"] = user_project api_response = self._connection.api_request( - method="POST", path=path, query_params=qs_params, timeout=timeout + method="POST", path=path, query_params=qs_params, timeout=timeout, retry=None ) metadata = HMACKeyMetadata(self) metadata._properties = api_response["metadata"] @@ -893,7 +895,7 @@ def list_hmac_keys( if user_project is not None: extra_params["userProject"] = user_project - api_request = functools.partial(self._connection.api_request, timeout=timeout) + api_request = functools.partial(self._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) return page_iterator.HTTPIterator( client=self, diff --git a/google/cloud/storage/hmac_key.py b/google/cloud/storage/hmac_key.py index d9c451c68..82e75c811 100644 --- a/google/cloud/storage/hmac_key.py +++ b/google/cloud/storage/hmac_key.py @@ -16,6 +16,8 @@ from google.cloud._helpers import _rfc3339_to_datetime 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 HMACKeyMetadata(object): @@ -260,6 +262,7 @@ def update(self, timeout=_DEFAULT_TIMEOUT): data=payload, query_params=qs_params, timeout=timeout, + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, ) def delete(self, timeout=_DEFAULT_TIMEOUT): @@ -283,5 +286,5 @@ def delete(self, timeout=_DEFAULT_TIMEOUT): qs_params["userProject"] = self.user_project self._client._connection.api_request( - method="DELETE", path=self.path, query_params=qs_params, timeout=timeout + method="DELETE", path=self.path, query_params=qs_params, timeout=timeout, retry=DEFAULT_RETRY ) diff --git a/google/cloud/storage/notification.py b/google/cloud/storage/notification.py index 434a44dd1..a63d4513a 100644 --- a/google/cloud/storage/notification.py +++ b/google/cloud/storage/notification.py @@ -19,6 +19,7 @@ from google.api_core.exceptions import NotFound from google.cloud.storage.constants import _DEFAULT_TIMEOUT +from google.cloud.storage.retry import DEFAULT_RETRY OBJECT_FINALIZE_EVENT_TYPE = "OBJECT_FINALIZE" @@ -271,6 +272,7 @@ def create(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params=query_params, data=properties, timeout=timeout, + retry=None, ) def exists(self, client=None, timeout=_DEFAULT_TIMEOUT): @@ -347,7 +349,7 @@ def reload(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params["userProject"] = self.bucket.user_project response = client._connection.api_request( - method="GET", path=self.path, query_params=query_params, timeout=timeout + method="GET", path=self.path, query_params=query_params, timeout=timeout, retry=DEFAULT_RETRY ) self._set_properties(response) @@ -385,7 +387,7 @@ def delete(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params["userProject"] = self.bucket.user_project client._connection.api_request( - method="DELETE", path=self.path, query_params=query_params, timeout=timeout + method="DELETE", path=self.path, query_params=query_params, timeout=timeout, retry=DEFAULT_RETRY ) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py new file mode 100644 index 000000000..d815d1cb2 --- /dev/null +++ b/google/cloud/storage/retry.py @@ -0,0 +1,91 @@ +# Copyright 2020 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from google.api_core import exceptions +from google.api_core import retry + +import json + + +_RETRYABLE_REASONS = frozenset( + ["rateLimitExceeded", "backendError", "internalError", "badGateway"] +) + +_UNSTRUCTURED_RETRYABLE_TYPES = ( + exceptions.TooManyRequests, + exceptions.InternalServerError, + exceptions.BadGateway, +) + +# FIXME: needs to be brought in line with doc outlining all retriable error codes +# FIXME: add tests +def _should_retry(exc): + """Predicate for determining when to retry.""" + if not hasattr(exc, "errors"): + return False + + if len(exc.errors) == 0: + # Check for unstructured error returns, e.g. from GFE + return isinstance(exc, _UNSTRUCTURED_RETRYABLE_TYPES) + + reason = exc.errors[0]["reason"] + return reason in _RETRYABLE_REASONS + + +DEFAULT_RETRY = retry.Retry(predicate=_should_retry) +"""The default retry object. + +To modify the default retry behavior, call a ``with_XXX`` method +on ``DEFAULT_RETRY``. For example, to change the deadline to 30 seconds, +pass ``retry=DEFAULT_RETRY.with_deadline(30)``. +""" + +class ConditionalRetryPolicy(object): + def __init__(self, retry_policy, conditional_predicate, required_kwargs): + self.retry_policy = retry_policy + self.conditional_predicate = conditional_predicate + self.required_kwargs = required_kwargs + + def get_retry_policy_if_conditions_met(self, **kwargs): + if self.conditional_predicate(*[kwargs[key] for key in self.required_kwargs]): + return self.retry_policy + return None + +def is_generation_specified(query_params): + """Return True if generation or if_generation_match is specified.""" + generation = query_params.get("generation") is not None + if_generation_match = query_params.get("if_generation_match") is not None + return generation or if_generation_match + +def is_metageneration_specified(query_params): + """Return True if if_metageneration_match is specified.""" + if_metageneration_match = query_params.get("if_metageneration_match") is not None + return if_metageneration_match + +def is_metageneration_specified_or_etag_in_json(query_params, data): + """Return True if if_metageneration_match is specified.""" + if query_params.get("if_metageneration_match") is not None: + return True + try: + content = json.loads(data) + if content.get("etag"): + return True + except (json.decoder.JSONDecodeError, TypeError): + pass + return False + + +DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy(DEFAULT_RETRY, is_generation_specified, ["query_params"]) +DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED = ConditionalRetryPolicy(DEFAULT_RETRY, is_metageneration_specified, ["query_params"]) +DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON = ConditionalRetryPolicy(DEFAULT_RETRY, is_metageneration_specified_or_etag_in_json, ["query_params", "data"]) diff --git a/tests/unit/test__helpers.py b/tests/unit/test__helpers.py index e295cbefc..372fea145 100644 --- a/tests/unit/test__helpers.py +++ b/tests/unit/test__helpers.py @@ -16,6 +16,10 @@ import mock +from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + class Test__get_storage_host(unittest.TestCase): @staticmethod @@ -122,6 +126,7 @@ def test_reload(self): "headers": {}, "_target_object": derived, "timeout": 42, + "retry": DEFAULT_RETRY, }, ) self.assertEqual(derived._changes, set()) @@ -158,6 +163,7 @@ def test_reload_with_generation_match(self): "headers": {}, "_target_object": derived, "timeout": 42, + "retry": DEFAULT_RETRY, }, ) self.assertEqual(derived._changes, set()) @@ -183,6 +189,7 @@ def test_reload_w_user_project(self): "headers": {}, "_target_object": derived, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, }, ) self.assertEqual(derived._changes, set()) @@ -207,6 +214,7 @@ def test_reload_w_projection(self): "headers": {}, "_target_object": derived, "timeout": 42, + "retry": DEFAULT_RETRY, }, ) self.assertEqual(derived._changes, set()) @@ -246,6 +254,7 @@ def test_patch(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": 42, + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED }, ) # Make sure changes get reset by patch(). @@ -286,6 +295,7 @@ def test_patch_with_metageneration_match(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": 42, + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED }, ) # Make sure changes get reset by patch(). @@ -315,6 +325,7 @@ def test_patch_w_user_project(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED }, ) # Make sure changes get reset by patch(). @@ -338,6 +349,7 @@ def test_update(self): self.assertEqual(kw[0]["query_params"], {"projection": "full"}) self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ}) self.assertEqual(kw[0]["timeout"], 42) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED) # Make sure changes get reset by patch(). self.assertEqual(derived._changes, set()) @@ -366,6 +378,7 @@ def test_update_with_metageneration_not_match(self): ) self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ}) self.assertEqual(kw[0]["timeout"], 42) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED) # Make sure changes get reset by patch(). self.assertEqual(derived._changes, set()) @@ -390,6 +403,7 @@ def test_update_w_user_project(self): ) self.assertEqual(kw[0]["data"], {"bar": BAR, "baz": BAZ}) self.assertEqual(kw[0]["timeout"], self._get_default_timeout()) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED) # Make sure changes get reset by patch(). self.assertEqual(derived._changes, set()) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index f67b6501e..f713861bd 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -26,6 +26,8 @@ import six from six.moves import http_client +from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED + def _make_credentials(): import google.auth.credentials @@ -3218,6 +3220,7 @@ def test_compose_wo_content_type_set(self): }, "_target_object": destination, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, }, ) @@ -3254,6 +3257,7 @@ def test_compose_minimal_w_user_project(self): }, "_target_object": destination, "timeout": 42, + "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, }, ) @@ -3295,6 +3299,7 @@ def test_compose_w_additional_property_changes(self): }, "_target_object": destination, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, }, ) @@ -3349,6 +3354,7 @@ def test_compose_w_generation_match(self): }, "_target_object": destination, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, }, ) @@ -3418,6 +3424,7 @@ def test_compose_w_generation_match_nones(self): }, "_target_object": destination, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_GENERATION_SPECIFIED, }, ) diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index 38a358da4..668db2d6d 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -18,6 +18,10 @@ import mock import pytest +from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + def _make_connection(*responses): import google.cloud.storage._http @@ -1021,6 +1025,7 @@ def test_delete_miss(self): "query_params": {}, "_target_object": None, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, } ] self.assertEqual(connection._deleted_buckets, expected_cw) @@ -1042,6 +1047,7 @@ def test_delete_hit_with_user_project(self): "_target_object": None, "query_params": {"userProject": USER_PROJECT}, "timeout": 42, + "retry": DEFAULT_RETRY, } ] self.assertEqual(connection._deleted_buckets, expected_cw) @@ -1065,6 +1071,7 @@ def test_delete_force_delete_blobs(self): "query_params": {}, "_target_object": None, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, } ] self.assertEqual(connection._deleted_buckets, expected_cw) @@ -1090,6 +1097,7 @@ def test_delete_with_metageneration_match(self): "query_params": {"ifMetagenerationMatch": METAGENERATION_NUMBER}, "_target_object": None, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, } ] self.assertEqual(connection._deleted_buckets, expected_cw) @@ -1112,6 +1120,7 @@ def test_delete_force_miss_blobs(self): "query_params": {}, "_target_object": None, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, } ] self.assertEqual(connection._deleted_buckets, expected_cw) @@ -1160,6 +1169,7 @@ def test_delete_blob_hit_with_user_project(self): self.assertEqual(kw["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME)) self.assertEqual(kw["query_params"], {"userProject": USER_PROJECT}) self.assertEqual(kw["timeout"], 42) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blob_hit_with_generation(self): NAME = "name" @@ -1175,6 +1185,7 @@ def test_delete_blob_hit_with_generation(self): self.assertEqual(kw["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME)) self.assertEqual(kw["query_params"], {"generation": GENERATION}) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blob_with_generation_match(self): NAME = "name" @@ -1200,6 +1211,7 @@ def test_delete_blob_with_generation_match(self): {"ifGenerationMatch": GENERATION, "ifMetagenerationMatch": METAGENERATION}, ) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blobs_empty(self): NAME = "name" @@ -1223,6 +1235,7 @@ def test_delete_blobs_hit_w_user_project(self): self.assertEqual(kw[0]["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME)) self.assertEqual(kw[0]["query_params"], {"userProject": USER_PROJECT}) self.assertEqual(kw[0]["timeout"], 42) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blobs_w_generation_match(self): NAME = "name" @@ -1248,12 +1261,14 @@ def test_delete_blobs_w_generation_match(self): self.assertEqual( kw[0]["query_params"], {"ifGenerationMatch": GENERATION_NUMBER} ) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) self.assertEqual(kw[1]["method"], "DELETE") self.assertEqual(kw[1]["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME2)) self.assertEqual(kw[1]["timeout"], 42) self.assertEqual( kw[1]["query_params"], {"ifGenerationMatch": GENERATION_NUMBER2} ) + self.assertEqual(kw[1]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blobs_w_generation_match_wrong_len(self): NAME = "name" @@ -1295,10 +1310,12 @@ def test_delete_blobs_w_generation_match_none(self): self.assertEqual( kw[0]["query_params"], {"ifGenerationMatch": GENERATION_NUMBER} ) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) self.assertEqual(kw[1]["method"], "DELETE") self.assertEqual(kw[1]["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME2)) self.assertEqual(kw[1]["timeout"], 42) self.assertEqual(kw[1]["query_params"], {}) + self.assertEqual(kw[1]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blobs_miss_no_on_error(self): from google.cloud.exceptions import NotFound @@ -1315,9 +1332,11 @@ def test_delete_blobs_miss_no_on_error(self): self.assertEqual(kw[0]["method"], "DELETE") self.assertEqual(kw[0]["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME)) self.assertEqual(kw[0]["timeout"], self._get_default_timeout()) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) self.assertEqual(kw[1]["method"], "DELETE") self.assertEqual(kw[1]["path"], "/b/%s/o/%s" % (NAME, NONESUCH)) self.assertEqual(kw[1]["timeout"], self._get_default_timeout()) + self.assertEqual(kw[1]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_delete_blobs_miss_w_on_error(self): NAME = "name" @@ -1334,9 +1353,11 @@ def test_delete_blobs_miss_w_on_error(self): self.assertEqual(kw[0]["method"], "DELETE") self.assertEqual(kw[0]["path"], "/b/%s/o/%s" % (NAME, BLOB_NAME)) self.assertEqual(kw[0]["timeout"], self._get_default_timeout()) + self.assertEqual(kw[0]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) self.assertEqual(kw[1]["method"], "DELETE") self.assertEqual(kw[1]["path"], "/b/%s/o/%s" % (NAME, NONESUCH)) self.assertEqual(kw[1]["timeout"], self._get_default_timeout()) + self.assertEqual(kw[1]["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_reload_bucket_w_metageneration_match(self): NAME = "name" @@ -1385,6 +1406,7 @@ def test_update_bucket_w_metageneration_match(self): req["query_params"], {"projection": "full", "ifMetagenerationMatch": METAGENERATION_NUMBER}, ) + self.assertEqual(req["retry"], DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED) def test_update_bucket_w_generation_match(self): connection = _Connection({}) @@ -1426,6 +1448,7 @@ def test_copy_blobs_wo_name(self): self.assertEqual(kw["path"], COPY_PATH) self.assertEqual(kw["query_params"], {}) self.assertEqual(kw["timeout"], 42) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_copy_blobs_source_generation(self): SOURCE = "source" @@ -1452,6 +1475,7 @@ def test_copy_blobs_source_generation(self): self.assertEqual(kw["path"], COPY_PATH) self.assertEqual(kw["query_params"], {"sourceGeneration": GENERATION}) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_copy_blobs_w_generation_match(self): SOURCE = "source" @@ -1489,6 +1513,7 @@ def test_copy_blobs_w_generation_match(self): }, ) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_copy_blobs_preserve_acl(self): from google.cloud.storage.acl import ObjectACL @@ -1522,6 +1547,7 @@ def test_copy_blobs_preserve_acl(self): self.assertEqual(kw1["path"], COPY_PATH) self.assertEqual(kw1["query_params"], {}) self.assertEqual(kw1["timeout"], self._get_default_timeout()) + self.assertEqual(kw1["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) self.assertEqual(kw2["method"], "PATCH") self.assertEqual(kw2["path"], NEW_BLOB_PATH) @@ -1553,6 +1579,7 @@ def test_copy_blobs_w_name_and_user_project(self): self.assertEqual(kw["path"], COPY_PATH) self.assertEqual(kw["query_params"], {"userProject": USER_PROJECT}) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) def test_rename_blob(self): BUCKET_NAME = "BUCKET_NAME" @@ -1579,6 +1606,7 @@ def test_rename_blob(self): self.assertEqual(kw["path"], COPY_PATH) self.assertEqual(kw["query_params"], {}) self.assertEqual(kw["timeout"], 42) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) blob.delete.assert_called_once_with( client=client, @@ -1628,6 +1656,7 @@ def test_rename_blob_with_generation_match(self): }, ) self.assertEqual(kw["timeout"], 42) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) blob.delete.assert_called_once_with( client=client, @@ -1660,6 +1689,7 @@ def test_rename_blob_to_itself(self): self.assertEqual(kw["path"], COPY_PATH) self.assertEqual(kw["query_params"], {}) self.assertEqual(kw["timeout"], self._get_default_timeout()) + self.assertEqual(kw["retry"], DEFAULT_RETRY_IF_GENERATION_SPECIFIED) blob.delete.assert_not_called() @@ -2272,6 +2302,7 @@ def test_create_deprecated(self, mock_warn): data=DATA, _target_object=bucket, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) mock_warn.assert_called_with( diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 600e11943..da824d95f 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -22,9 +22,14 @@ import unittest from six.moves import http_client +from google.api_core import exceptions + from google.oauth2.service_account import Credentials from . import _read_local_json +from google.cloud.storage.retry import DEFAULT_RETRY + + _SERVICE_ACCOUNT_JSON = _read_local_json("url_signer_v4_test_account.json") _CONFORMANCE_TESTS = _read_local_json("url_signer_v4_test_data.json")[ "postPolicyV4Tests" @@ -696,6 +701,7 @@ def test_create_bucket_w_conflict(self): data=data, _target_object=mock.ANY, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) @mock.patch("warnings.warn") @@ -765,6 +771,7 @@ def test_create_bucket_w_predefined_acl_valid(self): data=data, _target_object=bucket, timeout=42, + retry=DEFAULT_RETRY, ) def test_create_bucket_w_predefined_default_object_acl_invalid(self): @@ -800,6 +807,7 @@ def test_create_bucket_w_predefined_default_object_acl_valid(self): data=data, _target_object=bucket, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_create_bucket_w_explicit_location(self): @@ -825,6 +833,7 @@ def test_create_bucket_w_explicit_location(self): _target_object=bucket, query_params={"project": project}, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) self.assertEqual(bucket.location, location) @@ -848,6 +857,7 @@ def test_create_bucket_w_explicit_project(self): data=DATA, _target_object=bucket, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_create_w_extra_properties(self): @@ -899,6 +909,7 @@ def test_create_w_extra_properties(self): data=DATA, _target_object=bucket, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_create_hit(self): @@ -920,6 +931,7 @@ def test_create_hit(self): data=DATA, _target_object=bucket, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_create_bucket_w_string_success(self): @@ -1055,6 +1067,7 @@ def test_list_blobs(self): path="/b/%s/o" % BUCKET_NAME, query_params={"projection": "noAcl"}, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_list_blobs_w_all_arguments_and_user_project(self): @@ -1119,6 +1132,7 @@ def test_list_blobs_w_all_arguments_and_user_project(self): path="/b/%s/o" % BUCKET_NAME, query_params=EXPECTED, timeout=42, + retry=DEFAULT_RETRY, ) def test_list_buckets_wo_project(self): @@ -1835,6 +1849,30 @@ def test_get_signed_policy_v4_with_access_token(self): self.assertEqual(fields["x-goog-signature"], EXPECTED_SIGN) self.assertEqual(fields["policy"], EXPECTED_POLICY) + def test_list_buckets_retries_error(self): + PROJECT = "PROJECT" + CREDENTIALS = _make_credentials() + client = self._make_one(project=PROJECT, credentials=CREDENTIALS) + + BUCKET_NAME = "bucket-name" + + data = {"items": [{"name": BUCKET_NAME}]} + http = _make_requests_session([exceptions.InternalServerError("mock error"), _make_json_response(data)]) + client._http_internal = http + + buckets = list(client.list_buckets()) + + self.assertEqual(len(buckets), 1) + self.assertEqual(buckets[0].name, BUCKET_NAME) + + call = mock.call( + method="GET", + url=mock.ANY, + data=mock.ANY, + headers=mock.ANY, + timeout=self._get_default_timeout(), + ) + http.request.assert_has_calls([call, call]) @pytest.mark.parametrize("test_data", _POST_POLICY_TESTS) def test_conformance_post_policy(test_data): diff --git a/tests/unit/test_hmac_key.py b/tests/unit/test_hmac_key.py index a142939d5..37517db43 100644 --- a/tests/unit/test_hmac_key.py +++ b/tests/unit/test_hmac_key.py @@ -16,6 +16,8 @@ import mock +from google.cloud.storage.retry import DEFAULT_RETRY +from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED class TestHMACKeyMetadata(unittest.TestCase): @staticmethod @@ -343,6 +345,7 @@ def test_update_miss_no_project_set(self): "data": {"state": "INACTIVE"}, "query_params": {}, "timeout": 42, + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, } connection.api_request.assert_called_once_with(**expected_kwargs) @@ -376,6 +379,7 @@ def test_update_hit_w_project_set(self): "data": {"state": "ACTIVE"}, "query_params": {"userProject": user_project}, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, } connection.api_request.assert_called_once_with(**expected_kwargs) @@ -409,6 +413,7 @@ def test_delete_miss_no_project_set(self): "path": expected_path, "query_params": {}, "timeout": 42, + "retry": DEFAULT_RETRY, } connection.api_request.assert_called_once_with(**expected_kwargs) @@ -432,6 +437,7 @@ def test_delete_hit_w_project_set(self): "path": expected_path, "query_params": {"userProject": user_project}, "timeout": self._get_default_timeout(), + "retry": DEFAULT_RETRY, } connection.api_request.assert_called_once_with(**expected_kwargs) diff --git a/tests/unit/test_notification.py b/tests/unit/test_notification.py index f056701e3..4ed25eb73 100644 --- a/tests/unit/test_notification.py +++ b/tests/unit/test_notification.py @@ -16,6 +16,8 @@ import mock +from google.cloud.storage.retry import DEFAULT_RETRY + class TestBucketNotification(unittest.TestCase): @@ -269,6 +271,7 @@ def test_create_w_defaults(self): query_params={}, data=data, timeout=self._get_default_timeout(), + retry=None, ) def test_create_w_explicit_client(self): @@ -320,6 +323,7 @@ def test_create_w_explicit_client(self): query_params={"userProject": USER_PROJECT}, data=data, timeout=42, + retry=None, ) def test_exists_wo_notification_id(self): @@ -391,7 +395,7 @@ def test_reload_miss(self): notification.reload(timeout=42) api_request.assert_called_once_with( - method="GET", path=self.NOTIFICATION_PATH, query_params={}, timeout=42 + method="GET", path=self.NOTIFICATION_PATH, query_params={}, timeout=42, retry=DEFAULT_RETRY ) def test_reload_hit(self): @@ -425,6 +429,7 @@ def test_reload_hit(self): path=self.NOTIFICATION_PATH, query_params={"userProject": USER_PROJECT}, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY, ) def test_delete_wo_notification_id(self): @@ -449,7 +454,7 @@ def test_delete_miss(self): notification.delete(timeout=42) api_request.assert_called_once_with( - method="DELETE", path=self.NOTIFICATION_PATH, query_params={}, timeout=42 + method="DELETE", path=self.NOTIFICATION_PATH, query_params={}, timeout=42, retry=DEFAULT_RETRY ) def test_delete_hit(self): @@ -468,6 +473,7 @@ def test_delete_hit(self): path=self.NOTIFICATION_PATH, query_params={"userProject": USER_PROJECT}, timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY ) diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py new file mode 100644 index 000000000..623cc4b13 --- /dev/null +++ b/tests/unit/test_retry.py @@ -0,0 +1,66 @@ +# Copyright 2020 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +from google.cloud.storage.retry import DEFAULT_RETRY +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.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + +class TestConditionalRetryPolicy(unittest.TestCase): + def test_is_generation_specified_match_metageneration(self): + conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}) + self.assertEqual(policy, DEFAULT_RETRY) + + def test_is_generation_specified_match_generation(self): + conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"generation": 1}) + self.assertEqual(policy, DEFAULT_RETRY) + + def test_is_generation_specified_mismatch(self): + conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}) + self.assertEqual(policy, None) + + def test_is_metageneration_specified_match(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}) + self.assertEqual(policy, DEFAULT_RETRY) + + def test_is_metageneration_specified_mismatch(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}) + self.assertEqual(policy, None) + + def test_is_meta_or_etag_in_json_meta_match(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}, data="{}") + self.assertEqual(policy, DEFAULT_RETRY) + + def test_is_meta_or_etag_in_json_etag_match(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data='{"etag": "12345678"}') + self.assertEqual(policy, DEFAULT_RETRY) + + def test_is_meta_or_etag_in_json_mismatch(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data="{}") + self.assertEqual(policy, None) + + def test_is_meta_or_etag_in_json_invalid(self): + conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data="I am invalid JSON!") + self.assertEqual(policy, None) From 0c483989b38c513e71343422cd19e87efcf55d89 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 2 Oct 2020 10:10:33 -0700 Subject: [PATCH 02/13] _http tests --- google/cloud/storage/_http.py | 5 ++- tests/unit/test__http.py | 85 +++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index e39704a43..fa221f467 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -55,9 +55,10 @@ def api_request(self, *args, retry=None, **kwargs): if retry: # If this is a ConditionalRetryPolicy, check conditions. try: - retry = retry.get_retry_policy_if_conditions_met(kwargs) + retry = retry.get_retry_policy_if_conditions_met(**kwargs) except AttributeError: pass - call = retry(call) + if retry: + call = retry(call) return call() diff --git a/tests/unit/test__http.py b/tests/unit/test__http.py index 021698eb9..d57b31f8c 100644 --- a/tests/unit/test__http.py +++ b/tests/unit/test__http.py @@ -81,3 +81,88 @@ def test_build_api_url_w_extra_query_params(self): self.assertEqual(path, "/".join(["", "storage", conn.API_VERSION, "foo"])) parms = dict(parse_qsl(qs)) self.assertEqual(parms["bar"], "baz") + + def test_api_request_no_retry(self): + import requests + http = mock.create_autospec(requests.Session, instance=True) + client = mock.Mock(_http=http, spec=["_http"]) + + conn = self._make_one(client) + response = requests.Response() + response.status_code = 200 + data = b"brent-spiner" + response._content = data + http.request.return_value = response + + req_data = "hey-yoooouuuuu-guuuuuyyssss" + result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False) + http.request.assert_called_once() + + def test_api_request_basic_retry(self): + # For this test, the "retry" function will just short-circuit. + FAKE_RESPONSE_STRING = "fake_response" + retry = lambda _: lambda: FAKE_RESPONSE_STRING + + import requests + http = mock.create_autospec(requests.Session, instance=True) + client = mock.Mock(_http=http, spec=["_http"]) + + # Some of this is unnecessary if the test succeeds, but we'll leave it + # to ensure a failure produces a less confusing error message. + conn = self._make_one(client) + response = requests.Response() + response.status_code = 200 + data = b"brent-spiner" + response._content = data + http.request.return_value = response + + req_data = "hey-yoooouuuuu-guuuuuyyssss" + result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=retry) + http.request.assert_not_called() + self.assertEqual(result, FAKE_RESPONSE_STRING) + + def test_api_request_conditional_retry(self): + # For this test, the "retry" function will short-circuit. + FAKE_RESPONSE_STRING = "fake_response" + retry = lambda _: lambda: FAKE_RESPONSE_STRING + conditional_retry_mock = mock.MagicMock() + conditional_retry_mock.get_retry_policy_if_conditions_met.return_value = retry + + import requests + http = mock.create_autospec(requests.Session, instance=True) + client = mock.Mock(_http=http, spec=["_http"]) + + # Some of this is unnecessary if the test succeeds, but we'll leave it + # to ensure a failure produces a less confusing error message. + conn = self._make_one(client) + response = requests.Response() + response.status_code = 200 + data = b"brent-spiner" + response._content = data + http.request.return_value = response + + req_data = "hey-yoooouuuuu-guuuuuyyssss" + result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=conditional_retry_mock) + http.request.assert_not_called() + self.assertEqual(result, FAKE_RESPONSE_STRING) + + def test_api_request_conditional_retry_failed(self): + conditional_retry_mock = mock.MagicMock() + conditional_retry_mock.get_retry_policy_if_conditions_met.return_value = None + + import requests + http = mock.create_autospec(requests.Session, instance=True) + client = mock.Mock(_http=http, spec=["_http"]) + + # Some of this is unnecessary if the test succeeds, but we'll leave it + # to ensure a failure produces a less confusing error message. + conn = self._make_one(client) + response = requests.Response() + response.status_code = 200 + data = b"brent-spiner" + response._content = data + http.request.return_value = response + + req_data = "hey-yoooouuuuu-guuuuuyyssss" + result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=conditional_retry_mock) + http.request.assert_called_once() From 054710dfce40964c3fc488b3880f8963e4a107be Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 2 Oct 2020 10:24:55 -0700 Subject: [PATCH 03/13] blacken --- google/cloud/storage/_helpers.py | 2 +- google/cloud/storage/_http.py | 1 - google/cloud/storage/bucket.py | 19 +++++++++---- google/cloud/storage/client.py | 14 ++++++++-- google/cloud/storage/hmac_key.py | 6 +++- google/cloud/storage/notification.py | 12 ++++++-- google/cloud/storage/retry.py | 16 +++++++++-- tests/unit/test__helpers.py | 6 ++-- tests/unit/test__http.py | 24 ++++++++++++++-- tests/unit/test_client.py | 5 +++- tests/unit/test_hmac_key.py | 1 + tests/unit/test_notification.py | 14 ++++++++-- tests/unit/test_retry.py | 41 +++++++++++++++++++++------- 13 files changed, 125 insertions(+), 36 deletions(-) diff --git a/google/cloud/storage/_helpers.py b/google/cloud/storage/_helpers.py index 88a0941b3..ba59f8fa9 100644 --- a/google/cloud/storage/_helpers.py +++ b/google/cloud/storage/_helpers.py @@ -207,7 +207,7 @@ def reload( headers=self._encryption_headers(), _target_object=self, timeout=timeout, - retry=DEFAULT_RETRY + retry=DEFAULT_RETRY, ) self._set_properties(api_response) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index fa221f467..2ebf50fe7 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -61,4 +61,3 @@ def api_request(self, *args, retry=None, **kwargs): if retry: call = retry(call) return call() - diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index 92eac34c5..650ea8783 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -59,7 +59,9 @@ from google.cloud.storage.notification import NONE_PAYLOAD_FORMAT from google.cloud.storage.retry import DEFAULT_RETRY from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED -from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON +from google.cloud.storage.retry import ( + DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, +) _UBLA_BPO_ENABLED_MESSAGE = ( "Pass only one of 'uniform_bucket_level_access_enabled' / " @@ -1246,7 +1248,9 @@ def list_blobs( client = self._require_client(client) path = self.path + "/o" - api_request = functools.partial(client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) + api_request = functools.partial( + client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY + ) iterator = page_iterator.HTTPIterator( client=client, api_request=api_request, @@ -1285,7 +1289,9 @@ def list_notifications(self, client=None, timeout=_DEFAULT_TIMEOUT): """ client = self._require_client(client) path = self.path + "/notificationConfigs" - api_request = functools.partial(client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) + api_request = functools.partial( + client._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY + ) iterator = page_iterator.HTTPIterator( client=client, api_request=api_request, @@ -1426,7 +1432,7 @@ def delete( query_params=query_params, _target_object=None, timeout=timeout, - retry=DEFAULT_RETRY + retry=DEFAULT_RETRY, ) def delete_blob( @@ -2734,7 +2740,10 @@ def test_iam_permissions(self, permissions, client=None, timeout=_DEFAULT_TIMEOU path = "%s/iam/testPermissions" % (self.path,) resp = client._connection.api_request( - method="GET", path=path, query_params=query_params, timeout=timeout, + method="GET", + path=path, + query_params=query_params, + timeout=timeout, retry=DEFAULT_RETRY, ) return resp.get("permissions", []) diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index d6f0d5633..2ff8c6e9d 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -779,7 +779,9 @@ def list_buckets( if fields is not None: extra_params["fields"] = fields - api_request = functools.partial(self._connection.api_request, retry=DEFAULT_RETRY, timeout=timeout) + api_request = functools.partial( + self._connection.api_request, retry=DEFAULT_RETRY, timeout=timeout + ) return page_iterator.HTTPIterator( client=self, @@ -831,7 +833,11 @@ def create_hmac_key( qs_params["userProject"] = user_project api_response = self._connection.api_request( - method="POST", path=path, query_params=qs_params, timeout=timeout, retry=None + method="POST", + path=path, + query_params=qs_params, + timeout=timeout, + retry=None, ) metadata = HMACKeyMetadata(self) metadata._properties = api_response["metadata"] @@ -895,7 +901,9 @@ def list_hmac_keys( if user_project is not None: extra_params["userProject"] = user_project - api_request = functools.partial(self._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY) + api_request = functools.partial( + self._connection.api_request, timeout=timeout, retry=DEFAULT_RETRY + ) return page_iterator.HTTPIterator( client=self, diff --git a/google/cloud/storage/hmac_key.py b/google/cloud/storage/hmac_key.py index 82e75c811..c0b956f3f 100644 --- a/google/cloud/storage/hmac_key.py +++ b/google/cloud/storage/hmac_key.py @@ -286,5 +286,9 @@ def delete(self, timeout=_DEFAULT_TIMEOUT): qs_params["userProject"] = self.user_project self._client._connection.api_request( - method="DELETE", path=self.path, query_params=qs_params, timeout=timeout, retry=DEFAULT_RETRY + method="DELETE", + path=self.path, + query_params=qs_params, + timeout=timeout, + retry=DEFAULT_RETRY, ) diff --git a/google/cloud/storage/notification.py b/google/cloud/storage/notification.py index a63d4513a..07333e6e7 100644 --- a/google/cloud/storage/notification.py +++ b/google/cloud/storage/notification.py @@ -349,7 +349,11 @@ def reload(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params["userProject"] = self.bucket.user_project response = client._connection.api_request( - method="GET", path=self.path, query_params=query_params, timeout=timeout, retry=DEFAULT_RETRY + method="GET", + path=self.path, + query_params=query_params, + timeout=timeout, + retry=DEFAULT_RETRY, ) self._set_properties(response) @@ -387,7 +391,11 @@ def delete(self, client=None, timeout=_DEFAULT_TIMEOUT): query_params["userProject"] = self.bucket.user_project client._connection.api_request( - method="DELETE", path=self.path, query_params=query_params, timeout=timeout, retry=DEFAULT_RETRY + method="DELETE", + path=self.path, + query_params=query_params, + timeout=timeout, + retry=DEFAULT_RETRY, ) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index d815d1cb2..093f8756f 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -51,6 +51,7 @@ def _should_retry(exc): pass ``retry=DEFAULT_RETRY.with_deadline(30)``. """ + class ConditionalRetryPolicy(object): def __init__(self, retry_policy, conditional_predicate, required_kwargs): self.retry_policy = retry_policy @@ -62,17 +63,20 @@ def get_retry_policy_if_conditions_met(self, **kwargs): return self.retry_policy return None + def is_generation_specified(query_params): """Return True if generation or if_generation_match is specified.""" generation = query_params.get("generation") is not None if_generation_match = query_params.get("if_generation_match") is not None return generation or if_generation_match + def is_metageneration_specified(query_params): """Return True if if_metageneration_match is specified.""" if_metageneration_match = query_params.get("if_metageneration_match") is not None return if_metageneration_match + def is_metageneration_specified_or_etag_in_json(query_params, data): """Return True if if_metageneration_match is specified.""" if query_params.get("if_metageneration_match") is not None: @@ -86,6 +90,12 @@ def is_metageneration_specified_or_etag_in_json(query_params, data): return False -DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy(DEFAULT_RETRY, is_generation_specified, ["query_params"]) -DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED = ConditionalRetryPolicy(DEFAULT_RETRY, is_metageneration_specified, ["query_params"]) -DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON = ConditionalRetryPolicy(DEFAULT_RETRY, is_metageneration_specified_or_etag_in_json, ["query_params", "data"]) +DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy( + DEFAULT_RETRY, is_generation_specified, ["query_params"] +) +DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED = ConditionalRetryPolicy( + DEFAULT_RETRY, is_metageneration_specified, ["query_params"] +) +DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON = ConditionalRetryPolicy( + DEFAULT_RETRY, is_metageneration_specified_or_etag_in_json, ["query_params", "data"] +) diff --git a/tests/unit/test__helpers.py b/tests/unit/test__helpers.py index 372fea145..8d2cda9e6 100644 --- a/tests/unit/test__helpers.py +++ b/tests/unit/test__helpers.py @@ -254,7 +254,7 @@ def test_patch(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": 42, - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, }, ) # Make sure changes get reset by patch(). @@ -295,7 +295,7 @@ def test_patch_with_metageneration_match(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": 42, - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, }, ) # Make sure changes get reset by patch(). @@ -325,7 +325,7 @@ def test_patch_w_user_project(self): "data": {"bar": BAR}, "_target_object": derived, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, }, ) # Make sure changes get reset by patch(). diff --git a/tests/unit/test__http.py b/tests/unit/test__http.py index d57b31f8c..12eb2fca4 100644 --- a/tests/unit/test__http.py +++ b/tests/unit/test__http.py @@ -84,6 +84,7 @@ def test_build_api_url_w_extra_query_params(self): def test_api_request_no_retry(self): import requests + http = mock.create_autospec(requests.Session, instance=True) client = mock.Mock(_http=http, spec=["_http"]) @@ -104,6 +105,7 @@ def test_api_request_basic_retry(self): retry = lambda _: lambda: FAKE_RESPONSE_STRING import requests + http = mock.create_autospec(requests.Session, instance=True) client = mock.Mock(_http=http, spec=["_http"]) @@ -117,7 +119,9 @@ def test_api_request_basic_retry(self): http.request.return_value = response req_data = "hey-yoooouuuuu-guuuuuyyssss" - result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=retry) + result = conn.api_request( + "GET", "/rainbow", data=req_data, expect_json=False, retry=retry + ) http.request.assert_not_called() self.assertEqual(result, FAKE_RESPONSE_STRING) @@ -129,6 +133,7 @@ def test_api_request_conditional_retry(self): conditional_retry_mock.get_retry_policy_if_conditions_met.return_value = retry import requests + http = mock.create_autospec(requests.Session, instance=True) client = mock.Mock(_http=http, spec=["_http"]) @@ -142,7 +147,13 @@ def test_api_request_conditional_retry(self): http.request.return_value = response req_data = "hey-yoooouuuuu-guuuuuyyssss" - result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=conditional_retry_mock) + result = conn.api_request( + "GET", + "/rainbow", + data=req_data, + expect_json=False, + retry=conditional_retry_mock, + ) http.request.assert_not_called() self.assertEqual(result, FAKE_RESPONSE_STRING) @@ -151,6 +162,7 @@ def test_api_request_conditional_retry_failed(self): conditional_retry_mock.get_retry_policy_if_conditions_met.return_value = None import requests + http = mock.create_autospec(requests.Session, instance=True) client = mock.Mock(_http=http, spec=["_http"]) @@ -164,5 +176,11 @@ def test_api_request_conditional_retry_failed(self): http.request.return_value = response req_data = "hey-yoooouuuuu-guuuuuyyssss" - result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False, retry=conditional_retry_mock) + result = conn.api_request( + "GET", + "/rainbow", + data=req_data, + expect_json=False, + retry=conditional_retry_mock, + ) http.request.assert_called_once() diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index da824d95f..3290ab9b0 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -1857,7 +1857,9 @@ def test_list_buckets_retries_error(self): BUCKET_NAME = "bucket-name" data = {"items": [{"name": BUCKET_NAME}]} - http = _make_requests_session([exceptions.InternalServerError("mock error"), _make_json_response(data)]) + http = _make_requests_session( + [exceptions.InternalServerError("mock error"), _make_json_response(data)] + ) client._http_internal = http buckets = list(client.list_buckets()) @@ -1874,6 +1876,7 @@ def test_list_buckets_retries_error(self): ) http.request.assert_has_calls([call, call]) + @pytest.mark.parametrize("test_data", _POST_POLICY_TESTS) def test_conformance_post_policy(test_data): import datetime diff --git a/tests/unit/test_hmac_key.py b/tests/unit/test_hmac_key.py index 37517db43..18e74b485 100644 --- a/tests/unit/test_hmac_key.py +++ b/tests/unit/test_hmac_key.py @@ -19,6 +19,7 @@ from google.cloud.storage.retry import DEFAULT_RETRY from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED + class TestHMACKeyMetadata(unittest.TestCase): @staticmethod def _get_default_timeout(): diff --git a/tests/unit/test_notification.py b/tests/unit/test_notification.py index 4ed25eb73..e49e80138 100644 --- a/tests/unit/test_notification.py +++ b/tests/unit/test_notification.py @@ -395,7 +395,11 @@ def test_reload_miss(self): notification.reload(timeout=42) api_request.assert_called_once_with( - method="GET", path=self.NOTIFICATION_PATH, query_params={}, timeout=42, retry=DEFAULT_RETRY + method="GET", + path=self.NOTIFICATION_PATH, + query_params={}, + timeout=42, + retry=DEFAULT_RETRY, ) def test_reload_hit(self): @@ -454,7 +458,11 @@ def test_delete_miss(self): notification.delete(timeout=42) api_request.assert_called_once_with( - method="DELETE", path=self.NOTIFICATION_PATH, query_params={}, timeout=42, retry=DEFAULT_RETRY + method="DELETE", + path=self.NOTIFICATION_PATH, + query_params={}, + timeout=42, + retry=DEFAULT_RETRY, ) def test_delete_hit(self): @@ -473,7 +481,7 @@ def test_delete_hit(self): path=self.NOTIFICATION_PATH, query_params={"userProject": USER_PROJECT}, timeout=self._get_default_timeout(), - retry=DEFAULT_RETRY + retry=DEFAULT_RETRY, ) diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py index 623cc4b13..5a3d049ac 100644 --- a/tests/unit/test_retry.py +++ b/tests/unit/test_retry.py @@ -17,50 +17,71 @@ from google.cloud.storage.retry import DEFAULT_RETRY 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.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON +from google.cloud.storage.retry import ( + DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, +) + class TestConditionalRetryPolicy(unittest.TestCase): def test_is_generation_specified_match_metageneration(self): conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}) + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_generation_match": 1} + ) self.assertEqual(policy, DEFAULT_RETRY) def test_is_generation_specified_match_generation(self): conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"generation": 1}) + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"generation": 1} + ) self.assertEqual(policy, DEFAULT_RETRY) def test_is_generation_specified_mismatch(self): conditional_policy = DEFAULT_RETRY_IF_GENERATION_SPECIFIED - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}) + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_metageneration_match": 1} + ) self.assertEqual(policy, None) def test_is_metageneration_specified_match(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}) + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_metageneration_match": 1} + ) self.assertEqual(policy, DEFAULT_RETRY) def test_is_metageneration_specified_mismatch(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}) + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_generation_match": 1} + ) self.assertEqual(policy, None) def test_is_meta_or_etag_in_json_meta_match(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_metageneration_match": 1}, data="{}") + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_metageneration_match": 1}, data="{}" + ) self.assertEqual(policy, DEFAULT_RETRY) def test_is_meta_or_etag_in_json_etag_match(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data='{"etag": "12345678"}') + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_generation_match": 1}, data='{"etag": "12345678"}' + ) self.assertEqual(policy, DEFAULT_RETRY) def test_is_meta_or_etag_in_json_mismatch(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data="{}") + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_generation_match": 1}, data="{}" + ) self.assertEqual(policy, None) def test_is_meta_or_etag_in_json_invalid(self): conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON - policy = conditional_policy.get_retry_policy_if_conditions_met(query_params={"if_generation_match": 1}, data="I am invalid JSON!") + policy = conditional_policy.get_retry_policy_if_conditions_met( + query_params={"if_generation_match": 1}, data="I am invalid JSON!" + ) self.assertEqual(policy, None) From 186362fa87019fbcb7b98a14b395ba1cecbba48d Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 2 Oct 2020 10:30:21 -0700 Subject: [PATCH 04/13] lint --- google/cloud/storage/_http.py | 1 - google/cloud/storage/blob.py | 2 -- google/cloud/storage/retry.py | 1 + tests/unit/test__helpers.py | 1 - tests/unit/test__http.py | 17 +++++++++++++---- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index 2ebf50fe7..96c089630 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -19,7 +19,6 @@ from google.cloud import _http from google.cloud.storage import __version__ -from google.cloud.storage import retry class Connection(_http.JSONConnection): diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 54ee3869e..f63303a37 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -74,9 +74,7 @@ from google.cloud.storage.constants import NEARLINE_STORAGE_CLASS from google.cloud.storage.constants import REGIONAL_LEGACY_STORAGE_CLASS from google.cloud.storage.constants import STANDARD_STORAGE_CLASS -from google.cloud.storage.retry import DEFAULT_RETRY from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED -from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED _API_ACCESS_ENDPOINT = "https://storage.googleapis.com" diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 093f8756f..db42c8b1b 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -28,6 +28,7 @@ exceptions.BadGateway, ) + # FIXME: needs to be brought in line with doc outlining all retriable error codes # FIXME: add tests def _should_retry(exc): diff --git a/tests/unit/test__helpers.py b/tests/unit/test__helpers.py index 8d2cda9e6..fa989f96e 100644 --- a/tests/unit/test__helpers.py +++ b/tests/unit/test__helpers.py @@ -17,7 +17,6 @@ import mock from google.cloud.storage.retry import DEFAULT_RETRY -from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED diff --git a/tests/unit/test__http.py b/tests/unit/test__http.py index 12eb2fca4..eb77a83eb 100644 --- a/tests/unit/test__http.py +++ b/tests/unit/test__http.py @@ -96,13 +96,17 @@ def test_api_request_no_retry(self): http.request.return_value = response req_data = "hey-yoooouuuuu-guuuuuyyssss" - result = conn.api_request("GET", "/rainbow", data=req_data, expect_json=False) + conn.api_request("GET", "/rainbow", data=req_data, expect_json=False) http.request.assert_called_once() def test_api_request_basic_retry(self): # For this test, the "retry" function will just short-circuit. FAKE_RESPONSE_STRING = "fake_response" - retry = lambda _: lambda: FAKE_RESPONSE_STRING + + def retry(_): + def fake_response(): + return FAKE_RESPONSE_STRING + return fake_response import requests @@ -128,7 +132,12 @@ def test_api_request_basic_retry(self): def test_api_request_conditional_retry(self): # For this test, the "retry" function will short-circuit. FAKE_RESPONSE_STRING = "fake_response" - retry = lambda _: lambda: FAKE_RESPONSE_STRING + + def retry(_): + def fake_response(): + return FAKE_RESPONSE_STRING + return fake_response + conditional_retry_mock = mock.MagicMock() conditional_retry_mock.get_retry_policy_if_conditions_met.return_value = retry @@ -176,7 +185,7 @@ def test_api_request_conditional_retry_failed(self): http.request.return_value = response req_data = "hey-yoooouuuuu-guuuuuyyssss" - result = conn.api_request( + conn.api_request( "GET", "/rainbow", data=req_data, From bbf10d73765c3e9f06aefce18ef22f032152f5b6 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 2 Oct 2020 10:31:45 -0700 Subject: [PATCH 05/13] notes --- google/cloud/storage/retry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index db42c8b1b..3a700bcc2 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -30,7 +30,7 @@ # FIXME: needs to be brought in line with doc outlining all retriable error codes -# FIXME: add tests +# FIXME: add tests once above is done def _should_retry(exc): """Predicate for determining when to retry.""" if not hasattr(exc, "errors"): From 741aff03ebc239544f620223f0edab351e7e8e68 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 9 Oct 2020 11:15:56 -0700 Subject: [PATCH 06/13] change etag behavior --- google/cloud/storage/_http.py | 2 +- google/cloud/storage/bucket.py | 6 ++---- google/cloud/storage/hmac_key.py | 4 ++-- google/cloud/storage/retry.py | 10 ++++------ tests/unit/test_hmac_key.py | 6 +++--- tests/unit/test_retry.py | 21 ++++++--------------- 6 files changed, 18 insertions(+), 31 deletions(-) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index 96c089630..fecda1921 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -49,7 +49,7 @@ def __init__(self, client, client_info=None, api_endpoint=DEFAULT_API_ENDPOINT): API_URL_TEMPLATE = "{api_base_url}/storage/{api_version}{path}" """A template for the URL of a particular API call.""" - def api_request(self, *args, retry=None, **kwargs): + def api_request(self, retry=None, *args, **kwargs): call = functools.partial(super(Connection, self).api_request, *args, **kwargs) if retry: # If this is a ConditionalRetryPolicy, check conditions. diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index 650ea8783..7ab9a13ef 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -59,9 +59,7 @@ from google.cloud.storage.notification import NONE_PAYLOAD_FORMAT from google.cloud.storage.retry import DEFAULT_RETRY from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED -from google.cloud.storage.retry import ( - DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, -) +from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON _UBLA_BPO_ENABLED_MESSAGE = ( "Pass only one of 'uniform_bucket_level_access_enabled' / " @@ -2701,7 +2699,7 @@ def set_iam_policy(self, policy, client=None, timeout=_DEFAULT_TIMEOUT): data=resource, _target_object=None, timeout=timeout, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, + retry=DEFAULT_RETRY_IF_ETAG_IN_JSON, ) return Policy.from_api_repr(info) diff --git a/google/cloud/storage/hmac_key.py b/google/cloud/storage/hmac_key.py index c0b956f3f..796aeeedb 100644 --- a/google/cloud/storage/hmac_key.py +++ b/google/cloud/storage/hmac_key.py @@ -17,7 +17,7 @@ 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 +from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON class HMACKeyMetadata(object): @@ -262,7 +262,7 @@ def update(self, timeout=_DEFAULT_TIMEOUT): data=payload, query_params=qs_params, timeout=timeout, - retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + retry=DEFAULT_RETRY_IF_ETAG_IN_JSON, ) def delete(self, timeout=_DEFAULT_TIMEOUT): diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 3a700bcc2..4b171c7af 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -78,10 +78,8 @@ def is_metageneration_specified(query_params): return if_metageneration_match -def is_metageneration_specified_or_etag_in_json(query_params, data): - """Return True if if_metageneration_match is specified.""" - if query_params.get("if_metageneration_match") is not None: - return True +def is_etag_in_json(query_params, data): + """Return True if an etag is contained in the JSON body.""" try: content = json.loads(data) if content.get("etag"): @@ -97,6 +95,6 @@ def is_metageneration_specified_or_etag_in_json(query_params, data): DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED = ConditionalRetryPolicy( DEFAULT_RETRY, is_metageneration_specified, ["query_params"] ) -DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON = ConditionalRetryPolicy( - DEFAULT_RETRY, is_metageneration_specified_or_etag_in_json, ["query_params", "data"] +DEFAULT_RETRY_IF_ETAG_IN_JSON = ConditionalRetryPolicy( + DEFAULT_RETRY, is_etag_in_json, ["query_params", "data"] ) diff --git a/tests/unit/test_hmac_key.py b/tests/unit/test_hmac_key.py index 18e74b485..d4ac933cf 100644 --- a/tests/unit/test_hmac_key.py +++ b/tests/unit/test_hmac_key.py @@ -17,7 +17,7 @@ import mock from google.cloud.storage.retry import DEFAULT_RETRY -from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED +from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON class TestHMACKeyMetadata(unittest.TestCase): @@ -346,7 +346,7 @@ def test_update_miss_no_project_set(self): "data": {"state": "INACTIVE"}, "query_params": {}, "timeout": 42, - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + "retry": DEFAULT_RETRY_IF_ETAG_IN_JSON, } connection.api_request.assert_called_once_with(**expected_kwargs) @@ -380,7 +380,7 @@ def test_update_hit_w_project_set(self): "data": {"state": "ACTIVE"}, "query_params": {"userProject": user_project}, "timeout": self._get_default_timeout(), - "retry": DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, + "retry": DEFAULT_RETRY_IF_ETAG_IN_JSON, } connection.api_request.assert_called_once_with(**expected_kwargs) diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py index 5a3d049ac..7c5a6ba1e 100644 --- a/tests/unit/test_retry.py +++ b/tests/unit/test_retry.py @@ -17,9 +17,7 @@ from google.cloud.storage.retry import DEFAULT_RETRY 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.retry import ( - DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON, -) +from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON class TestConditionalRetryPolicy(unittest.TestCase): @@ -58,29 +56,22 @@ def test_is_metageneration_specified_mismatch(self): ) self.assertEqual(policy, None) - def test_is_meta_or_etag_in_json_meta_match(self): - conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON - policy = conditional_policy.get_retry_policy_if_conditions_met( - query_params={"if_metageneration_match": 1}, data="{}" - ) - self.assertEqual(policy, DEFAULT_RETRY) - - def test_is_meta_or_etag_in_json_etag_match(self): - conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + def test_is_etag_in_json_etag_match(self): + conditional_policy = DEFAULT_RETRY_IF_ETAG_IN_JSON policy = conditional_policy.get_retry_policy_if_conditions_met( query_params={"if_generation_match": 1}, data='{"etag": "12345678"}' ) self.assertEqual(policy, DEFAULT_RETRY) - def test_is_meta_or_etag_in_json_mismatch(self): - conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + def test_is_etag_in_json_mismatch(self): + conditional_policy = DEFAULT_RETRY_IF_ETAG_IN_JSON policy = conditional_policy.get_retry_policy_if_conditions_met( query_params={"if_generation_match": 1}, data="{}" ) self.assertEqual(policy, None) def test_is_meta_or_etag_in_json_invalid(self): - conditional_policy = DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED_OR_ETAG_IN_JSON + conditional_policy = DEFAULT_RETRY_IF_ETAG_IN_JSON policy = conditional_policy.get_retry_policy_if_conditions_met( query_params={"if_generation_match": 1}, data="I am invalid JSON!" ) From 95293c1664dd16a52cc41a293167f39986ee3285 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 9 Oct 2020 11:22:01 -0700 Subject: [PATCH 07/13] py2 compat --- google/cloud/storage/_http.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index fecda1921..a1ab644ed 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -49,7 +49,8 @@ def __init__(self, client, client_info=None, api_endpoint=DEFAULT_API_ENDPOINT): API_URL_TEMPLATE = "{api_base_url}/storage/{api_version}{path}" """A template for the URL of a particular API call.""" - def api_request(self, retry=None, *args, **kwargs): + def api_request(self, *args, **kwargs): + retry = kwargs.pop("retry", None) call = functools.partial(super(Connection, self).api_request, *args, **kwargs) if retry: # If this is a ConditionalRetryPolicy, check conditions. From 73d37cf89cbd121f6c581996b163dcfa3d3a638d Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 9 Oct 2020 11:29:21 -0700 Subject: [PATCH 08/13] comments --- google/cloud/storage/_http.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/_http.py b/google/cloud/storage/_http.py index a1ab644ed..6e175196c 100644 --- a/google/cloud/storage/_http.py +++ b/google/cloud/storage/_http.py @@ -56,7 +56,7 @@ def api_request(self, *args, **kwargs): # If this is a ConditionalRetryPolicy, check conditions. try: retry = retry.get_retry_policy_if_conditions_met(**kwargs) - except AttributeError: + except AttributeError: # This is not a ConditionalRetryPolicy. pass if retry: call = retry(call) From 46693d7993eca7556bb6122fc8b8ae1cb380a249 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Fri, 9 Oct 2020 17:49:02 -0700 Subject: [PATCH 09/13] Support expanded error types for retry; retry ConnectionError; lint --- google/cloud/storage/retry.py | 36 +++++++++++++++-------------------- tests/unit/test__http.py | 2 ++ 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 4b171c7af..b308ce36a 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -12,36 +12,27 @@ # See the License for the specific language governing permissions and # limitations under the License. +import requests + from google.api_core import exceptions from google.api_core import retry import json -_RETRYABLE_REASONS = frozenset( - ["rateLimitExceeded", "backendError", "internalError", "badGateway"] -) - -_UNSTRUCTURED_RETRYABLE_TYPES = ( - exceptions.TooManyRequests, - exceptions.InternalServerError, - exceptions.BadGateway, +_RETRYABLE_TYPES = ( + exceptions.TooManyRequests, # 429 + exceptions.InternalServerError, # 500 + exceptions.BadGateway, # 502 + exceptions.ServiceUnavailable, # 503 + exceptions.GatewayTimeout, # 504 + requests.ConnectionError, ) -# FIXME: needs to be brought in line with doc outlining all retriable error codes -# FIXME: add tests once above is done def _should_retry(exc): """Predicate for determining when to retry.""" - if not hasattr(exc, "errors"): - return False - - if len(exc.errors) == 0: - # Check for unstructured error returns, e.g. from GFE - return isinstance(exc, _UNSTRUCTURED_RETRYABLE_TYPES) - - reason = exc.errors[0]["reason"] - return reason in _RETRYABLE_REASONS + return isinstance(exc, _RETRYABLE_TYPES) DEFAULT_RETRY = retry.Retry(predicate=_should_retry) @@ -49,7 +40,8 @@ def _should_retry(exc): To modify the default retry behavior, call a ``with_XXX`` method on ``DEFAULT_RETRY``. For example, to change the deadline to 30 seconds, -pass ``retry=DEFAULT_RETRY.with_deadline(30)``. +pass ``retry=DEFAULT_RETRY.with_deadline(30)``. See google-api-core reference +(https://googleapis.dev/python/google-api-core/latest/retry.html) for details. """ @@ -79,7 +71,9 @@ def is_metageneration_specified(query_params): def is_etag_in_json(query_params, data): - """Return True if an etag is contained in the JSON body.""" + """Return True if an etag is contained in the JSON body. + + Indended for use on calls with relatively short JSON payloads.""" try: content = json.loads(data) if content.get("etag"): diff --git a/tests/unit/test__http.py b/tests/unit/test__http.py index ddd3cabef..00cb4d34e 100644 --- a/tests/unit/test__http.py +++ b/tests/unit/test__http.py @@ -124,6 +124,7 @@ def test_api_request_basic_retry(self): def retry(_): def fake_response(): return FAKE_RESPONSE_STRING + return fake_response import requests @@ -154,6 +155,7 @@ def test_api_request_conditional_retry(self): def retry(_): def fake_response(): return FAKE_RESPONSE_STRING + return fake_response conditional_retry_mock = mock.MagicMock() From 7052b12a36bd8764dd85876823ff3747ff5d08a5 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Mon, 12 Oct 2020 09:24:42 -0700 Subject: [PATCH 10/13] py2 compat --- google/cloud/storage/retry.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index b308ce36a..f4244b577 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -70,7 +70,7 @@ def is_metageneration_specified(query_params): return if_metageneration_match -def is_etag_in_json(query_params, data): +def is_etag_in_json(data): """Return True if an etag is contained in the JSON body. Indended for use on calls with relatively short JSON payloads.""" @@ -78,7 +78,10 @@ def is_etag_in_json(query_params, data): content = json.loads(data) if content.get("etag"): return True - except (json.decoder.JSONDecodeError, TypeError): + # Though this method should only be called when a JSON body is expected, + # the retry policy should be robust to unexpected payloads. + # In Python 3 a JSONDecodeError is possible, but it is a subclass of ValueError. + except (ValueError, TypeError): pass return False @@ -90,5 +93,5 @@ def is_etag_in_json(query_params, data): DEFAULT_RETRY, is_metageneration_specified, ["query_params"] ) DEFAULT_RETRY_IF_ETAG_IN_JSON = ConditionalRetryPolicy( - DEFAULT_RETRY, is_etag_in_json, ["query_params", "data"] + DEFAULT_RETRY, is_etag_in_json, ["data"] ) From 5f0f473e952105171e32adef400f6c68f0e8e7fe Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Mon, 12 Oct 2020 14:25:46 -0700 Subject: [PATCH 11/13] add support for 408 exceptions --- google/cloud/storage/retry.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index f4244b577..228ec7e88 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -29,10 +29,20 @@ requests.ConnectionError, ) +# Some retriable errors don't have their own custom exception in api_core. +_ADDITIONAL_RETRYABLE_STATUS_CODES = ( + 408, +) + def _should_retry(exc): """Predicate for determining when to retry.""" - return isinstance(exc, _RETRYABLE_TYPES) + if isinstance(exc, _RETRYABLE_TYPES): + return True + elif isinstance(exc, exceptions.GoogleAPICallError): + return exc.code in _ADDITIONAL_RETRYABLE_STATUS_CODES + else: + return False DEFAULT_RETRY = retry.Retry(predicate=_should_retry) From 8ba68c4a7b1e356c527cc8d84173a65d46e5d3ba Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Mon, 12 Oct 2020 16:03:28 -0700 Subject: [PATCH 12/13] lint --- google/cloud/storage/retry.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 228ec7e88..8b9f4a567 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -30,9 +30,7 @@ ) # Some retriable errors don't have their own custom exception in api_core. -_ADDITIONAL_RETRYABLE_STATUS_CODES = ( - 408, -) +_ADDITIONAL_RETRYABLE_STATUS_CODES = (408,) def _should_retry(exc): From ffa652786d445e65ff2958e616c45d2223aa6e48 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Thu, 15 Oct 2020 10:16:55 -0700 Subject: [PATCH 13/13] documentation --- google/cloud/storage/retry.py | 37 ++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 8b9f4a567..c1f1ad10d 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -46,14 +46,45 @@ def _should_retry(exc): DEFAULT_RETRY = retry.Retry(predicate=_should_retry) """The default retry object. -To modify the default retry behavior, call a ``with_XXX`` method -on ``DEFAULT_RETRY``. For example, to change the deadline to 30 seconds, -pass ``retry=DEFAULT_RETRY.with_deadline(30)``. See google-api-core reference +This retry setting will retry all _RETRYABLE_TYPES and any status codes from +_ADDITIONAL_RETRYABLE_STATUS_CODES. + +To modify the default retry behavior, create a new retry object modeled after +this one by calling it a ``with_XXX`` method. For example, to create a copy of +DEFAULT_RETRY with a deadline of 30 seconds, pass +``retry=DEFAULT_RETRY.with_deadline(30)``. See google-api-core reference (https://googleapis.dev/python/google-api-core/latest/retry.html) for details. """ class ConditionalRetryPolicy(object): + """A class for use when an API call is only conditionally safe to retry. + + This class is intended for use in inspecting the API call parameters of an + API call to verify that any flags necessary to make the API call idempotent + (such as specifying an ``if_generation_match`` or related flag) are present. + + It can be used in place of a ``retry.Retry`` object, in which case + ``_http.Connection.api_request`` will pass the requested api call keyword + arguments into the ``conditional_predicate`` and return the ``retry_policy`` + if the conditions are met. + + :type retry_policy: class:`google.api_core.retry.Retry` + :param retry_policy: A retry object defining timeouts, persistence and which + exceptions to retry. + + :type conditional_predicate: callable + :param conditional_predicate: A callable that accepts exactly the number of + arguments in ``required_kwargs``, in order, and returns True if the + arguments have sufficient data to determine that the call is safe to + retry (idempotent). + + :type required_kwargs: list(str) + :param required_kwargs: + A list of keyword argument keys that will be extracted from the API call + and passed into the ``conditional predicate`` in order. + """ + def __init__(self, retry_policy, conditional_predicate, required_kwargs): self.retry_policy = retry_policy self.conditional_predicate = conditional_predicate