fix: retry uploads only conditionally (#316)

* fix: retry uploads only conditionally

* update docstring for num_retries

Author: andrewsg

google/cloud/storage/blob.py

@@ -103,9 +103,11 @@
     "release. The default behavior (when `num_retries` is not specified) when "
     "a transient error (e.g. 429 Too Many Requests or 500 Internal Server "
     "Error) occurs will be as follows: upload requests will be automatically "
-    "retried. Subsequent retries will be sent after waiting 1, 2, 4, 8, etc. "
-    "seconds (exponential backoff) until 10 minutes of wait time have "
-    "elapsed. At that point, there will be no more attempts to retry."
+    "retried if and only if `if_metageneration_match` is specified (thus "
+    "making the upload idempotent). Subsequent retries will be sent after "
+    "waiting 1, 2, 4, 8, etc. seconds (exponential backoff) until 10 minutes "
+    "of wait time have elapsed. At that point, there will be no more attempts "
+    "to retry."
 )
 _READ_LESS_THAN_SIZE = (
     "Size {:d} was specified but the file-like object only had " "{:d} bytes remaining."
@@ -1550,8 +1552,14 @@ def _do_multipart_upload(
                      concluded once ``stream`` is exhausted (or :data:`None`).
 
         :type num_retries: int
-        :param num_retries: Number of upload retries. (Deprecated: This
-                            argument will be removed in a future release.)
+        :param num_retries: Number of upload retries. By default, only uploads
+                            with if_metageneration_match set will be retried, as
+                            uploads without the argument are not guaranteed to
+                            be idempotent. Setting num_retries will override
+                            this default behavior and guarantee retries even
+                            when if_metageneration_match is not set.
+                            (Deprecated: This argument will be removed in a
+                            future release.)
 
         :type predefined_acl: str
         :param predefined_acl: (Optional) Predefined access control list
@@ -1709,8 +1717,14 @@ def _initiate_resumable_upload(
         :param predefined_acl: (Optional) Predefined access control list
 
         :type num_retries: int
-        :param num_retries: Number of upload retries. (Deprecated: This
-                            argument will be removed in a future release.)
+        :param num_retries: Number of upload retries. By default, only uploads
+                            with if_metageneration_match set will be retried, as
+                            uploads without the argument are not guaranteed to
+                            be idempotent. Setting num_retries will override
+                            this default behavior and guarantee retries even
+                            when if_metageneration_match is not set.
+                            (Deprecated: This argument will be removed in a
+                            future release.)
 
         :type extra_headers: dict
         :param extra_headers: (Optional) Extra headers to add to standard
@@ -1887,8 +1901,14 @@ def _do_resumable_upload(
                      concluded once ``stream`` is exhausted (or :data:`None`).
 
         :type num_retries: int
-        :param num_retries: Number of upload retries. (Deprecated: This
-                            argument will be removed in a future release.)
+        :param num_retries: Number of upload retries. By default, only uploads
+                            with if_metageneration_match set will be retried, as
+                            uploads without the argument are not guaranteed to
+                            be idempotent. Setting num_retries will override
+                            this default behavior and guarantee retries even
+                            when if_metageneration_match is not set.
+                            (Deprecated: This argument will be removed in a
+                            future release.)
 
         :type predefined_acl: str
         :param predefined_acl: (Optional) Predefined access control list
@@ -2005,8 +2025,14 @@ def _do_upload(
                      concluded once ``stream`` is exhausted (or :data:`None`).
 
         :type num_retries: int
-        :param num_retries: Number of upload retries. (Deprecated: This
-                            argument will be removed in a future release.)
+        :param num_retries: Number of upload retries. By default, only uploads
+                            with if_metageneration_match set will be retried, as
+                            uploads without the argument are not guaranteed to
+                            be idempotent. Setting num_retries will override
+                            this default behavior and guarantee retries even
+                            when if_metageneration_match is not set.
+                            (Deprecated: This argument will be removed in a
+                            future release.)
 
         :type predefined_acl: str
         :param predefined_acl: (Optional) Predefined access control list
@@ -2058,6 +2084,15 @@ def _do_upload(
                   **only** response in the multipart case and it will be the
                   **final** response in the resumable case.
         """
+        if if_metageneration_match is None and num_retries is None:
+            # Uploads are only idempotent (safe to retry) if
+            # if_metageneration_match is set. If it is not set, the default
+            # num_retries should be 0. Note: Because retry logic for uploads is
+            # provided by the google-resumable-media-python package, it doesn't
+            # use the ConditionalRetryStrategy class used in other API calls in
+            # this library to solve this problem.
+            num_retries = 0
+
         if size is not None and size <= _MAX_MULTIPART_SIZE:
             response = self._do_multipart_upload(
                 client,
@@ -2161,8 +2196,14 @@ def upload_from_file(
         :param content_type: (Optional) Type of content being uploaded.
 
         :type num_retries: int
-        :param num_retries: Number of upload retries. (Deprecated: This
-                            argument will be removed in a future release.)
+        :param num_retries: Number of upload retries. By default, only uploads
+                            with if_metageneration_match set will be retried, as
+                            uploads without the argument are not guaranteed to
+                            be idempotent. Setting num_retries will override
+                            this default behavior and guarantee retries even
+                            when if_metageneration_match is not set.
+                            (Deprecated: This argument will be removed in a
+                            future release.)
 
         :type client: :class:`~google.cloud.storage.client.Client`
         :param client: (Optional) The client to use.  If not passed, falls back

tests/unit/test_blob.py

@@ -2577,6 +2577,12 @@ def _do_upload_helper(
             if_metageneration_not_match,
             **timeout_kwarg
         )
+
+        # Adjust num_retries expectations to reflect the conditional default in
+        # _do_upload()
+        if num_retries is None and if_metageneration_match is None:
+            num_retries = 0
+
         self.assertIs(created_json, mock.sentinel.json)
         response.json.assert_called_once_with()
         if size is not None and size <= _MAX_MULTIPART_SIZE: