fix: revise and rename is_etag_in_json(data) (#483)

* fix: handle cases where data is a dict in is_etag_in_json(data)

* address comments and accept proposed changes

* revise is_etag_in_json to check dict data

* revise docstring wording

* revise docstrings

* rename conditional predicate to is_etag_in_data

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)
 
 

google/cloud/storage/retry.py

@@ -18,8 +18,6 @@
 from google.api_core import retry
 from google.auth import exceptions as auth_exceptions
 
-import json
-
 
 # ConnectionError is a built-in exception only in Python3 and not in Python2.
 try:
@@ -120,20 +118,21 @@ def is_metageneration_specified(query_params):
     return if_metageneration_match
 
 
+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 request JSON body. If not passed, returns False.
+    """
+    return data is not None and "etag" in 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."""
-    try:
-        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
+    """
+    ``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(

tests/unit/test_retry.py

@@ -166,30 +166,24 @@ 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)
 
-    @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))