Repository: googleapis/python-storage

Issue:
Uploads and if_metageneration_match and retries
Sorry for the ticket here, I've also put in a support case but I'm guessing you all have more domain expertise than them and can quickly yes/no this one.

When doing an upload of a new blob that shouldn't exist already, I would like retries to actually work. But the code makes it pretty clear that I should set an if_metageneration_match precondition if I want retries on uploads. So I have this code:

```
from google.cloud import storage
client = storage.Client()
bucket = client.bucket("whatever")
blob = client.blob("a_file_that_doesnt_yet_exist.txt")
blob.upload_from_string("Hello world", if_generation_match=0, if_metageneration_match=0)
```

But that fails all requests. I _think_ the right answer is just that the default retry policy is incorrect for fresh uploads and that if_generation_match=0 actually provides all the idemopotence safety I need? So the fix would be to use 1.39 and the new `retry=storage.retry.DEFAULT_RETRY` (or `storage.retry.DEFAULT_RETRY_IF_GENERATION_SPECIFIED` but that's functionally the same if I'm hardwiring in if_generation_match myself in the same call)? Maybe the default retry policy on uploads should be tweaked to "either if_metageneration_match is set at all or if_generation_match==0"? Thanks in advance!

Discussion:

Patch:
diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py
index 60178aa..92af18d 100644
--- a/google/cloud/storage/blob.py
+++ b/google/cloud/storage/blob.py
@@ -82,7 +82,6 @@ from google.cloud.storage.retry import ConditionalRetryPolicy
 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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
 
             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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
 
             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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
 
             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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
             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 @@ class Blob(_PropertyMixin):
         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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
             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 @@ class Blob(_PropertyMixin):
             # 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 @@ class Blob(_PropertyMixin):
         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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
             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 @@ class Blob(_PropertyMixin):
         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 @@ class Blob(_PropertyMixin):
         :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 @@ class Blob(_PropertyMixin):
             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
diff --git a/google/cloud/storage/fileio.py b/google/cloud/storage/fileio.py
index 6ac8e05..e9b4c23 100644
--- a/google/cloud/storage/fileio.py
+++ b/google/cloud/storage/fileio.py
@@ -18,7 +18,7 @@ import warnings
 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 @@ class BlobWriter(io.BufferedIOBase):
         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 @@ class BlobWriter(io.BufferedIOBase):
             # 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.


Test patch:
diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py
index a213858..8c4d9b9 100644
--- a/tests/unit/test_blob.py
+++ b/tests/unit/test_blob.py
@@ -29,7 +29,6 @@ from six.moves import http_client
 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 @@ class Test_Blob(unittest.TestCase):
             **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 @@ class Test_Blob(unittest.TestCase):
 
     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 @@ class Test_Blob(unittest.TestCase):
         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 @@ class Test_Blob(unittest.TestCase):
 
         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}
         )
diff --git a/tests/unit/test_fileio.py b/tests/unit/test_fileio.py
index 6ce9b49..9fadc96 100644
--- a/tests/unit/test_fileio.py
+++ b/tests/unit/test_fileio.py
@@ -395,7 +395,7 @@ class TestBlobWriterBinary(unittest.TestCase):
                 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 @@ class TestBlobWriterBinary(unittest.TestCase):
             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)