From 5a8763f15a2c8a51e7b86b99a45302973bcbaa00 Mon Sep 17 00:00:00 2001
From: Andrew Gorcester <gorcester@google.com>
Date: Mon, 16 Nov 2020 12:31:02 -0800
Subject: [PATCH] Retry uploads only conditionally

---
 google/cloud/storage/blob.py | 17 ++++++++++++++---
 tests/unit/test_blob.py      |  6 ++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py
index e6b79dab5..e620ac1a9 100644
--- a/google/cloud/storage/blob.py
+++ b/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."
@@ -2058,6 +2060,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,
diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py
index 87c2a4878..bc9918627 100644
--- a/tests/unit/test_blob.py
+++ b/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: