Repository: googleapis/python-storage

Issue:
Handle data dict type in retry.is_etag_in_json(data)
`retry.is_etag_in_json(data)` is a helper method for conditional retries. It returns a boolean whether or not an `etag` is contained in the JSON body. 

There seem to be use cases in which a dictionary type of data is passed into [`retry.is_etag_in_json(data)`](https://github.com/googleapis/python-storage/blob/cb2a89b0b696defd56c4c1eee5a1cad67ae4cb43/google/cloud/storage/retry.py#L128). Currently the method is unable to handle dict type data correctly: a `JSONDecodeError` is caught before examining if an etag is contained within.

Discussion:

Patch:
diff --git a/docs/retry_timeout.rst b/docs/retry_timeout.rst
index b7fc4ff..7c3ad30 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 a9cdc9c..ce988fc 100644
--- a/google/cloud/storage/retry.py
+++ b/google/cloud/storage/retry.py
@@ -18,8 +18,6 @@ from google.api_core import exceptions as api_exceptions
 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(


Test patch:
diff --git a/tests/unit/test_retry.py b/tests/unit/test_retry.py
index 3111584..28b05f6 100644
--- a/tests/unit/test_retry.py
+++ b/tests/unit/test_retry.py
@@ -166,30 +166,24 @@ class Test_is_metageneration_specified(unittest.TestCase):
         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))