From d78e84d519c4303d41fd6b4bc9529f06c831414c Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Wed, 23 Jun 2021 15:52:30 -0700 Subject: [PATCH 1/6] fix: handle cases where data is a dict in is_etag_in_json(data) --- google/cloud/storage/retry.py | 2 ++ tests/unit/test_retry.py | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index a9cdc9c0d..af0e14dfb 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -125,6 +125,8 @@ def is_etag_in_json(data): Indended for use on calls with relatively short JSON payloads.""" try: + if isinstance(data, dict) and data.get("etag"): + return True content = json.loads(data) if content.get("etag"): return True diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py index 3111584cb..596c75f95 100644 --- a/tests/unit/test_retry.py +++ b/tests/unit/test_retry.py @@ -193,6 +193,11 @@ def test_w_empty_data(self): self.assertFalse(self._call_fut(data)) + def test_w_etag_in_dict(self): + data = {"etag": "123"} + + self.assertTrue(self._call_fut(data)) + class Test_default_conditional_retry_policies(unittest.TestCase): def test_is_generation_specified_match_generation_match(self): From 73a9802948693a935704c0c08bae11faffed0810 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Fri, 2 Jul 2021 17:39:48 -0700 Subject: [PATCH 2/6] address comments and accept proposed changes --- google/cloud/storage/retry.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index af0e14dfb..ec678663a 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -19,6 +19,7 @@ from google.auth import exceptions as auth_exceptions import json +import six # ConnectionError is a built-in exception only in Python3 and not in Python2. @@ -123,19 +124,19 @@ def is_metageneration_specified(query_params): 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.""" - try: - if isinstance(data, dict) and data.get("etag"): - return True - content = json.loads(data) - if content.get("etag"): - return True - # 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 + :type data: str or dict + :param data: A string containing a JSON-encoded dict, or the dict itself. + + Intended for use on calls with relatively short JSON payloads.""" + if isinstance(data, six.text_type): + try: + data = json.loads(data) + # 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): + return False + return "etag" in data DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy( From 6e665ef4cb7a82aa6011913335634eac8288d448 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 8 Jul 2021 13:48:49 -0700 Subject: [PATCH 3/6] revise is_etag_in_json to check dict data --- google/cloud/storage/retry.py | 17 +++-------------- tests/unit/test_retry.py | 19 ++++--------------- 2 files changed, 7 insertions(+), 29 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index ec678663a..cffa10f97 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -18,9 +18,6 @@ from google.api_core import retry from google.auth import exceptions as auth_exceptions -import json -import six - # ConnectionError is a built-in exception only in Python3 and not in Python2. try: @@ -124,19 +121,11 @@ def is_metageneration_specified(query_params): def is_etag_in_json(data): """Return True if an etag is contained in the JSON body. - :type data: str or dict - :param data: A string containing a JSON-encoded dict, or the dict itself. + :type data: dict or None + :param data: A dict containing the JSON body. If not passed, returns False. Intended for use on calls with relatively short JSON payloads.""" - if isinstance(data, six.text_type): - try: - data = json.loads(data) - # 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): - return False - return "etag" in data + return data is not None and "etag" in data DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy( diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py index 596c75f95..5221d18ff 100644 --- a/tests/unit/test_retry.py +++ b/tests/unit/test_retry.py @@ -172,32 +172,21 @@ def _call_fut(self, data): return retry.is_etag_in_json(data) - @staticmethod - def _make_json_data(**kw): - import json - - return json.dumps(kw) - - def test_w_empty(self): - data = self._make_json_data() + def test_w_none(self): + data = None self.assertFalse(self._call_fut(data)) def test_w_etag_in_data(self): - data = self._make_json_data(etag="123") + data = {"etag": "123"} self.assertTrue(self._call_fut(data)) def test_w_empty_data(self): - data = "" + data = {} self.assertFalse(self._call_fut(data)) - def test_w_etag_in_dict(self): - data = {"etag": "123"} - - self.assertTrue(self._call_fut(data)) - class Test_default_conditional_retry_policies(unittest.TestCase): def test_is_generation_specified_match_generation_match(self): From da23d1325ba272f3dee4b18d714c07f635e5caf0 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Fri, 9 Jul 2021 11:16:34 -0700 Subject: [PATCH 4/6] revise docstring wording --- 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 cffa10f97..2ecd67faf 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -122,7 +122,7 @@ def is_etag_in_json(data): """Return True if an etag is contained in the JSON body. :type data: dict or None - :param data: A dict containing the JSON body. If not passed, returns False. + :param data: A dict representing the JSON body. If not passed, returns False. Intended for use on calls with relatively short JSON payloads.""" return data is not None and "etag" in data From ba3c16abd5a91e2d42d3e53b198ef03c547f6e46 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Fri, 9 Jul 2021 12:00:24 -0700 Subject: [PATCH 5/6] revise docstrings --- google/cloud/storage/retry.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 2ecd67faf..5435eef4a 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -123,8 +123,7 @@ def is_etag_in_json(data): :type data: dict or None :param data: A dict representing the JSON body. If not passed, returns False. - - Intended for use on calls with relatively short JSON payloads.""" + """ return data is not None and "etag" in data From af0abb3df3c9ae0d879e25188caa810ff8590187 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Fri, 9 Jul 2021 13:59:36 -0700 Subject: [PATCH 6/6] rename conditional predicate to is_etag_in_data --- docs/retry_timeout.rst | 4 ++-- google/cloud/storage/retry.py | 14 +++++++++++--- tests/unit/test_retry.py | 4 ++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/retry_timeout.rst b/docs/retry_timeout.rst index b7fc4ff41..7c3ad3084 100644 --- a/docs/retry_timeout.rst +++ b/docs/retry_timeout.rst @@ -133,14 +133,14 @@ explicit policy in your code. from google.api_core.retry import Retry from google.cloud.storage.retry import ConditionalRetryPolicy - from google.cloud.storage.retry import is_etag_in_json + from google.cloud.storage.retry import is_etag_in_data def is_retryable(exc): ... # as above my_retry_policy = Retry(predicate=is_retryable) my_cond_policy = ConditionalRetryPolicy( - my_retry_policy, conditional_predicate=is_etag_in_json) + my_retry_policy, conditional_predicate=is_etag_in_data) bucket = client.get_bucket(BUCKET_NAME, retry=my_cond_policy) diff --git a/google/cloud/storage/retry.py b/google/cloud/storage/retry.py index 5435eef4a..ce988fcc3 100644 --- a/google/cloud/storage/retry.py +++ b/google/cloud/storage/retry.py @@ -118,15 +118,23 @@ def is_metageneration_specified(query_params): return if_metageneration_match -def is_etag_in_json(data): - """Return True if an etag is contained in the JSON body. +def is_etag_in_data(data): + """Return True if an etag is contained in the request body. :type data: dict or None - :param data: A dict representing the JSON body. If not passed, returns False. + :param data: A dict representing the request JSON body. If not passed, returns False. """ return data is not None and "etag" in data +def is_etag_in_json(data): + """ + ``is_etag_in_json`` is supported for backwards-compatibility reasons only; + please use ``is_etag_in_data`` instead. + """ + return is_etag_in_data(data) + + DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy( DEFAULT_RETRY, is_generation_specified, ["query_params"] ) diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py index 5221d18ff..28b05f6ce 100644 --- a/tests/unit/test_retry.py +++ b/tests/unit/test_retry.py @@ -166,11 +166,11 @@ def test_w_if_metageneration_match(self): self.assertTrue(self._call_fut(query_params)) -class Test_is_etag_in_json(unittest.TestCase): +class Test_is_etag_in_data(unittest.TestCase): def _call_fut(self, data): from google.cloud.storage import retry - return retry.is_etag_in_json(data) + return retry.is_etag_in_data(data) def test_w_none(self): data = None