fix: replace default retry for upload operations (#480)

* fix: revise upload operations preconditions to if_generation_match

* fix docstrings

* add retry configuration to blob.create_resumable_upload_session

* align var values in test

* test coverage

* Revert "test coverage"

This reverts commit e91916f.

* Revert "align var values in test"

This reverts commit aec585b.

* Revert "add retry configuration to blob.create_resumable_upload_session"

This reverts commit 8c1ae3c.

* revise tests after reverting

Author: cojenco

google/cloud/storage/blob.py

@@ -82,7 +82,6 @@
 from google.cloud.storage.retry import DEFAULT_RETRY
 from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON
 from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
-from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED
 from google.cloud.storage.fileio import BlobReader
 from google.cloud.storage.fileio import BlobWriter
 
@@ -1703,10 +1702,10 @@ def _do_multipart_upload(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type predefined_acl: str
@@ -1750,7 +1749,7 @@ def _do_multipart_upload(
 
             This private method does not accept ConditionalRetryPolicy values
             because the information necessary to evaluate the policy is instead
-            evaluated in client.download_blob_to_file().
+            evaluated in blob._do_upload().
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -1877,10 +1876,10 @@ def _initiate_resumable_upload(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type extra_headers: dict
@@ -1936,7 +1935,7 @@ def _initiate_resumable_upload(
 
             This private method does not accept ConditionalRetryPolicy values
             because the information necessary to evaluate the policy is instead
-            evaluated in client.download_blob_to_file().
+            evaluated in blob._do_upload().
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -2070,10 +2069,10 @@ def _do_resumable_upload(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type predefined_acl: str
@@ -2119,7 +2118,7 @@ def _do_resumable_upload(
 
             This private method does not accept ConditionalRetryPolicy values
             because the information necessary to evaluate the policy is instead
-            evaluated in client.download_blob_to_file().
+            evaluated in blob._do_upload().
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -2204,10 +2203,10 @@ def _do_upload(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type predefined_acl: str
@@ -2258,7 +2257,7 @@ def _do_upload(
             This class exists to provide safe defaults for RPC calls that are
             not technically safe to retry normally (due to potential data
             duplication or other side-effects) but become safe to retry if a
-            condition such as if_metageneration_match is set.
+            condition such as if_generation_match is set.
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -2337,7 +2336,7 @@ def upload_from_file(
         if_metageneration_not_match=None,
         timeout=_DEFAULT_TIMEOUT,
         checksum=None,
-        retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+        retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
     ):
         """Upload the contents of this blob from a file-like object.
 
@@ -2397,10 +2396,10 @@ def upload_from_file(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type client: :class:`~google.cloud.storage.client.Client`
@@ -2456,7 +2455,7 @@ def upload_from_file(
             This class exists to provide safe defaults for RPC calls that are
             not technically safe to retry normally (due to potential data
             duplication or other side-effects) but become safe to retry if a
-            condition such as if_metageneration_match is set.
+            condition such as if_generation_match is set.
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -2479,7 +2478,7 @@ def upload_from_file(
             # num_retries and retry are mutually exclusive. If num_retries is
             # set and retry is exactly the default, then nullify retry for
             # backwards compatibility.
-            if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
+            if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
                 retry = None
 
         _maybe_rewind(file_obj, rewind=rewind)
@@ -2518,7 +2517,7 @@ def upload_from_filename(
         if_metageneration_not_match=None,
         timeout=_DEFAULT_TIMEOUT,
         checksum=None,
-        retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+        retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
     ):
         """Upload this blob's contents from the content of a named file.
 
@@ -2558,10 +2557,10 @@ def upload_from_filename(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type predefined_acl: str
@@ -2612,7 +2611,7 @@ def upload_from_filename(
             This class exists to provide safe defaults for RPC calls that are
             not technically safe to retry normally (due to potential data
             duplication or other side-effects) but become safe to retry if a
-            condition such as if_metageneration_match is set.
+            condition such as if_generation_match is set.
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how
@@ -2656,7 +2655,7 @@ def upload_from_string(
         if_metageneration_not_match=None,
         timeout=_DEFAULT_TIMEOUT,
         checksum=None,
-        retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+        retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
     ):
         """Upload contents of this blob from the provided string.
 
@@ -2687,10 +2686,10 @@ def upload_from_string(
         :type num_retries: int
         :param num_retries:
             Number of upload retries. By default, only uploads with
-            if_metageneration_match set will be retried, as uploads without the
+            if_generation_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
+            if_generation_match is not set.  (Deprecated: This argument
             will be removed in a future release.)
 
         :type client: :class:`~google.cloud.storage.client.Client`
@@ -2746,7 +2745,7 @@ def upload_from_string(
             This class exists to provide safe defaults for RPC calls that are
             not technically safe to retry normally (due to potential data
             duplication or other side-effects) but become safe to retry if a
-            condition such as if_metageneration_match is set.
+            condition such as if_generation_match is set.
 
             See the retry.py source code and docstrings in this package
             (google.cloud.storage.retry) for information on retry types and how

google/cloud/storage/fileio.py

@@ -18,7 +18,7 @@
 from google.api_core.exceptions import RequestRangeNotSatisfiable
 from google.cloud.storage._helpers import _NUM_RETRIES_MESSAGE
 from google.cloud.storage.retry import DEFAULT_RETRY
-from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED
+from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
 from google.cloud.storage.retry import ConditionalRetryPolicy
 
 
@@ -278,7 +278,7 @@ def __init__(
         blob,
         chunk_size=None,
         text_mode=False,
-        retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED,
+        retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
         **upload_kwargs
     ):
         for kwarg in upload_kwargs:
@@ -346,7 +346,7 @@ def _initiate_upload(self):
             # num_retries and retry are mutually exclusive. If num_retries is
             # set and retry is exactly the default, then nullify retry for
             # backwards compatibility.
-            if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
+            if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
                 retry = None
 
         # Handle ConditionalRetryPolicy.

tests/unit/test_blob.py

@@ -29,7 +29,6 @@
 from google.cloud.storage.retry import DEFAULT_RETRY
 from google.cloud.storage.retry import DEFAULT_RETRY_IF_ETAG_IN_JSON
 from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED
-from google.cloud.storage.retry import DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED
 
 
 def _make_credentials():
@@ -2853,8 +2852,8 @@ def _do_upload_helper(
             **timeout_kwarg
         )
 
-        if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED:
-            retry = DEFAULT_RETRY if if_metageneration_match else None
+        if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED:
+            retry = DEFAULT_RETRY if if_generation_match else None
 
         self.assertIs(created_json, mock.sentinel.json)
         response.json.assert_called_once_with()
@@ -2925,11 +2924,11 @@ def test__do_upload_with_num_retries(self):
 
     def test__do_upload_with_conditional_retry_success(self):
         self._do_upload_helper(
-            retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, if_metageneration_match=1
+            retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456
         )
 
     def test__do_upload_with_conditional_retry_failure(self):
-        self._do_upload_helper(retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED)
+        self._do_upload_helper(retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED)
 
     def _upload_from_file_helper(self, side_effect=None, **kwargs):
         from google.cloud._helpers import UTC
@@ -2955,7 +2954,7 @@ def _upload_from_file_helper(self, side_effect=None, **kwargs):
         if_metageneration_not_match = kwargs.get("if_metageneration_not_match", None)
         num_retries = kwargs.get("num_retries", None)
         default_retry = (
-            DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED if not num_retries else None
+            DEFAULT_RETRY_IF_GENERATION_SPECIFIED if not num_retries else None
         )
         retry = kwargs.get("retry", default_retry)
         ret_val = blob.upload_from_file(
@@ -3062,9 +3061,7 @@ def _do_upload_mock_call_helper(
 
         expected_timeout = self._get_default_timeout() if timeout is None else timeout
         if not retry:
-            retry = (
-                DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED if not num_retries else None
-            )
+            retry = DEFAULT_RETRY_IF_GENERATION_SPECIFIED if not num_retries else None
         self.assertEqual(
             kwargs, {"timeout": expected_timeout, "checksum": None, "retry": retry}
         )

tests/unit/test_fileio.py

@@ -395,7 +395,7 @@ def test_conditional_retry_pass(self):
                 blob,
                 chunk_size=chunk_size,
                 content_type=PLAIN_CONTENT_TYPE,
-                if_metageneration_match=1,
+                if_generation_match=123456,
             )
 
         # The transmit_next_chunk method must actually consume bytes from the
@@ -421,7 +421,7 @@ def test_conditional_retry_pass(self):
             None,  # num_retries
             chunk_size=chunk_size,
             retry=DEFAULT_RETRY,
-            if_metageneration_match=1,
+            if_generation_match=123456,
         )
         upload.transmit_next_chunk.assert_called_with(transport)
         self.assertEqual(upload.transmit_next_chunk.call_count, 4)