From 139cd2c84475d2592cec2377d21361778ddb753d Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Wed, 23 Jun 2021 19:45:18 -0700 Subject: [PATCH 1/9] fix: revise upload operations preconditions to if_generation_match --- google/cloud/storage/blob.py | 21 ++++++++++----------- google/cloud/storage/fileio.py | 6 +++--- tests/unit/test_blob.py | 6 ++---- tests/unit/test_fileio.py | 4 ++-- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 60178aa2e..e195bb9f0 100644 --- a/google/cloud/storage/blob.py +++ b/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 @@ -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,7 +2396,7 @@ 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 @@ -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,7 +2557,7 @@ 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 @@ -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,7 +2686,7 @@ 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 @@ -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 diff --git a/google/cloud/storage/fileio.py b/google/cloud/storage/fileio.py index 6ac8e057f..e9b4c23cf 100644 --- a/google/cloud/storage/fileio.py +++ b/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. diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index a21385821..02aa22349 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -2955,7 +2955,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 +3062,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} ) diff --git a/tests/unit/test_fileio.py b/tests/unit/test_fileio.py index 6ce9b4990..067fdab00 100644 --- a/tests/unit/test_fileio.py +++ b/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=123456789, # Note: Assuming this is the blob generation ) # 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=123456789, ) upload.transmit_next_chunk.assert_called_with(transport) self.assertEqual(upload.transmit_next_chunk.call_count, 4) From b57b8635ebfe9a1eeebb876a56d2229254f1800b Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 24 Jun 2021 15:17:41 -0700 Subject: [PATCH 2/9] fix docstrings --- google/cloud/storage/blob.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index e195bb9f0..92af18d54 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -1702,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 @@ -1749,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 @@ -1876,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 @@ -1935,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 @@ -2069,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 @@ -2118,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 @@ -2203,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 @@ -2257,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 @@ -2399,7 +2399,7 @@ def upload_from_file( 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` @@ -2560,7 +2560,7 @@ def upload_from_filename( 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 @@ -2689,7 +2689,7 @@ def upload_from_string( 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` From 8c1ae3c40c7c9e9bb5f5f5f3cb0ce56b17a87232 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 24 Jun 2021 16:19:52 -0700 Subject: [PATCH 3/9] add retry configuration to blob.create_resumable_upload_session --- google/cloud/storage/blob.py | 58 ++++++++++++++++++++++++++++++++++++ tests/unit/test_blob.py | 49 +++++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 92af18d54..6d916ce24 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -2782,6 +2782,11 @@ def create_resumable_upload_session( client=None, timeout=_DEFAULT_TIMEOUT, checksum=None, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ): """Create a resumable upload session. @@ -2857,6 +2862,41 @@ def create_resumable_upload_session( delete the uploaded object automatically. Supported values are "md5", "crc32c" and None. The default is None. + :type if_generation_match: long + :param if_generation_match: + (Optional) See :ref:`using-if-generation-match` + + :type if_generation_not_match: long + :param if_generation_not_match: + (Optional) See :ref:`using-if-generation-not-match` + + :type if_metageneration_match: long + :param if_metageneration_match: + (Optional) See :ref:`using-if-metageneration-match` + + :type if_metageneration_not_match: long + :param if_metageneration_not_match: + (Optional) See :ref:`using-if-metageneration-not-match` + + :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy + :param retry: (Optional) How to retry the RPC. A None value will disable + retries. A google.api_core.retry.Retry value will enable retries, + and the object will define retriable response codes and errors and + configure backoff and timeout options. + A google.cloud.storage.retry.ConditionalRetryPolicy value wraps a + Retry object and activates it only if certain conditions are met. + 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. + See the retry.py source code and docstrings in this package + (google.cloud.storage.retry) for information on retry types and how + to configure them. + Media operations (downloads and uploads) do not support non-default + predicates in a Retry object. The default will always be used. Other + configuration changes for Retry objects such as delays and deadlines + are respected. + :rtype: str :returns: The resumable upload session URL. The upload can be completed by making an HTTP PUT request with the @@ -2865,6 +2905,19 @@ def create_resumable_upload_session( :raises: :class:`google.cloud.exceptions.GoogleCloudError` if the session creation response returns an error status. """ + + # Handle ConditionalRetryPolicy. + if isinstance(retry, ConditionalRetryPolicy): + # Conditional retries are designed for non-media calls, which change + # arguments into query_params dictionaries. Media operations work + # differently, so here we make a "fake" query_params to feed to the + # ConditionalRetryPolicy. + query_params = { + "ifGenerationMatch": if_generation_match, + "ifMetagenerationMatch": if_metageneration_match, + } + retry = retry.get_retry_policy_if_conditions_met(query_params=query_params) + extra_headers = {} if origin is not None: # This header is specifically for client-side uploads, it @@ -2883,10 +2936,15 @@ def create_resumable_upload_session( size, None, predefined_acl=None, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, extra_headers=extra_headers, chunk_size=self._CHUNK_SIZE_MULTIPLE, timeout=timeout, checksum=checksum, + retry=retry, ) return upload.resumable_url diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 02aa22349..cc9086b5f 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -3249,8 +3249,18 @@ def test_upload_from_string_w_text_w_num_retries(self): self._upload_from_string_helper(data, num_retries=2) def _create_resumable_upload_session_helper( - self, origin=None, side_effect=None, timeout=None + self, + origin=None, + side_effect=None, + timeout=None, + if_generation_match=None, + if_generation_not_match=None, + if_metageneration_match=None, + if_metageneration_not_match=None, + retry=None, ): + from six.moves.urllib.parse import urlencode + bucket = _Bucket(name="alex-trebek") blob = self._make_one("blob-name", bucket=bucket) chunk_size = 99 * blob._CHUNK_SIZE_MULTIPLE @@ -3276,11 +3286,19 @@ def _create_resumable_upload_session_helper( expected_timeout = timeout timeout_kwarg = {"timeout": timeout} + if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED: + retry = DEFAULT_RETRY if if_generation_match else None + new_url = blob.create_resumable_upload_session( content_type=content_type, size=size, origin=origin, client=client, + if_generation_match=if_generation_match, + if_generation_not_match=if_generation_not_match, + if_metageneration_match=if_metageneration_match, + if_metageneration_not_match=if_metageneration_not_match, + retry=retry, **timeout_kwarg ) @@ -3290,10 +3308,23 @@ def _create_resumable_upload_session_helper( # Check the mocks. upload_url = ( - "https://storage.googleapis.com/upload/storage/v1" - + bucket.path - + "/o?uploadType=resumable" + "https://storage.googleapis.com/upload/storage/v1" + bucket.path + "/o" ) + + qs_params = [("uploadType", "resumable")] + if if_generation_match is not None: + qs_params.append(("ifGenerationMatch", if_generation_match)) + + if if_generation_not_match is not None: + qs_params.append(("ifGenerationNotMatch", if_generation_not_match)) + + if if_metageneration_match is not None: + qs_params.append(("ifMetagenerationMatch", if_metageneration_match)) + + if if_metageneration_not_match is not None: + qs_params.append(("ifMetaGenerationNotMatch", if_metageneration_not_match)) + + upload_url += "?" + urlencode(qs_params) payload = b'{"name": "blob-name"}' expected_headers = { "content-type": "application/json; charset=UTF-8", @@ -3319,6 +3350,16 @@ def test_create_resumable_upload_session_with_custom_timeout(self): def test_create_resumable_upload_session_with_origin(self): self._create_resumable_upload_session_helper(origin="http://google.com") + def test_create_resumable_upload_session_with_conditional_retry_success(self): + self._create_resumable_upload_session_helper( + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=1 + ) + + def test_create_resumable_upload_session_with_conditional_retry_failure(self): + self._create_resumable_upload_session_helper( + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED + ) + def test_create_resumable_upload_session_with_failure(self): from google.resumable_media import InvalidResponse from google.cloud import exceptions From aec585bdd823b720c80f872126d65ded87e6c76e Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Thu, 24 Jun 2021 16:32:22 -0700 Subject: [PATCH 4/9] align var values in test --- tests/unit/test_blob.py | 2 +- tests/unit/test_fileio.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index cc9086b5f..85afc04fd 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -3352,7 +3352,7 @@ def test_create_resumable_upload_session_with_origin(self): def test_create_resumable_upload_session_with_conditional_retry_success(self): self._create_resumable_upload_session_helper( - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=1 + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456 ) def test_create_resumable_upload_session_with_conditional_retry_failure(self): diff --git a/tests/unit/test_fileio.py b/tests/unit/test_fileio.py index 067fdab00..9fadc967c 100644 --- a/tests/unit/test_fileio.py +++ b/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_generation_match=123456789, # Note: Assuming this is the blob generation + 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_generation_match=123456789, + if_generation_match=123456, ) upload.transmit_next_chunk.assert_called_with(transport) self.assertEqual(upload.transmit_next_chunk.call_count, 4) From e91916fa2f8a8c02de9784cb010f044676e28444 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Fri, 25 Jun 2021 13:01:14 -0700 Subject: [PATCH 5/9] test coverage --- tests/unit/test_blob.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 85afc04fd..592265fc2 100644 --- a/tests/unit/test_blob.py +++ b/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 @@ -3286,9 +3285,6 @@ def _create_resumable_upload_session_helper( expected_timeout = timeout timeout_kwarg = {"timeout": timeout} - if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED: - retry = DEFAULT_RETRY if if_generation_match else None - new_url = blob.create_resumable_upload_session( content_type=content_type, size=size, @@ -3350,6 +3346,16 @@ def test_create_resumable_upload_session_with_custom_timeout(self): def test_create_resumable_upload_session_with_origin(self): self._create_resumable_upload_session_helper(origin="http://google.com") + def test_create_resumable_upload_session_with_generation_match(self): + self._create_resumable_upload_session_helper( + if_generation_match=123456, if_metageneration_match=2 + ) + + def test_create_resumable_upload_session_with_generation_not_match(self): + self._create_resumable_upload_session_helper( + if_generation_not_match=0, if_metageneration_not_match=3 + ) + def test_create_resumable_upload_session_with_conditional_retry_success(self): self._create_resumable_upload_session_helper( retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456 From 6f7f3b6a012ad08218b08125113823360cb92be5 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 29 Jun 2021 12:50:20 -0700 Subject: [PATCH 6/9] Revert "test coverage" This reverts commit e91916fa2f8a8c02de9784cb010f044676e28444. --- tests/unit/test_blob.py | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 592265fc2..85afc04fd 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -29,6 +29,7 @@ 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(): @@ -2852,8 +2853,8 @@ def _do_upload_helper( **timeout_kwarg ) - if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED: - retry = DEFAULT_RETRY if if_generation_match else None + if retry is DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED: + retry = DEFAULT_RETRY if if_metageneration_match else None self.assertIs(created_json, mock.sentinel.json) response.json.assert_called_once_with() @@ -2924,11 +2925,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_GENERATION_SPECIFIED, if_generation_match=123456 + retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED, if_metageneration_match=1 ) def test__do_upload_with_conditional_retry_failure(self): - self._do_upload_helper(retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED) + self._do_upload_helper(retry=DEFAULT_RETRY_IF_METAGENERATION_SPECIFIED) def _upload_from_file_helper(self, side_effect=None, **kwargs): from google.cloud._helpers import UTC @@ -3285,6 +3286,9 @@ def _create_resumable_upload_session_helper( expected_timeout = timeout timeout_kwarg = {"timeout": timeout} + if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED: + retry = DEFAULT_RETRY if if_generation_match else None + new_url = blob.create_resumable_upload_session( content_type=content_type, size=size, @@ -3346,16 +3350,6 @@ def test_create_resumable_upload_session_with_custom_timeout(self): def test_create_resumable_upload_session_with_origin(self): self._create_resumable_upload_session_helper(origin="http://google.com") - def test_create_resumable_upload_session_with_generation_match(self): - self._create_resumable_upload_session_helper( - if_generation_match=123456, if_metageneration_match=2 - ) - - def test_create_resumable_upload_session_with_generation_not_match(self): - self._create_resumable_upload_session_helper( - if_generation_not_match=0, if_metageneration_not_match=3 - ) - def test_create_resumable_upload_session_with_conditional_retry_success(self): self._create_resumable_upload_session_helper( retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456 From cccc4f4f553f6cd4802da7476ed1f26468f7dbe3 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 29 Jun 2021 12:50:20 -0700 Subject: [PATCH 7/9] Revert "align var values in test" This reverts commit aec585bdd823b720c80f872126d65ded87e6c76e. --- tests/unit/test_blob.py | 2 +- tests/unit/test_fileio.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 85afc04fd..cc9086b5f 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -3352,7 +3352,7 @@ def test_create_resumable_upload_session_with_origin(self): def test_create_resumable_upload_session_with_conditional_retry_success(self): self._create_resumable_upload_session_helper( - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=123456 + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=1 ) def test_create_resumable_upload_session_with_conditional_retry_failure(self): diff --git a/tests/unit/test_fileio.py b/tests/unit/test_fileio.py index 9fadc967c..067fdab00 100644 --- a/tests/unit/test_fileio.py +++ b/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_generation_match=123456, + if_generation_match=123456789, # Note: Assuming this is the blob generation ) # 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_generation_match=123456, + if_generation_match=123456789, ) upload.transmit_next_chunk.assert_called_with(transport) self.assertEqual(upload.transmit_next_chunk.call_count, 4) From 18cdc86e755a7dc8a96e098048844c53f10fc44a Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 29 Jun 2021 12:52:42 -0700 Subject: [PATCH 8/9] Revert "add retry configuration to blob.create_resumable_upload_session" This reverts commit 8c1ae3c40c7c9e9bb5f5f5f3cb0ce56b17a87232. --- google/cloud/storage/blob.py | 58 ------------------------------------ tests/unit/test_blob.py | 49 +++--------------------------- 2 files changed, 4 insertions(+), 103 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 6d916ce24..92af18d54 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -2782,11 +2782,6 @@ def create_resumable_upload_session( client=None, timeout=_DEFAULT_TIMEOUT, checksum=None, - if_generation_match=None, - if_generation_not_match=None, - if_metageneration_match=None, - if_metageneration_not_match=None, - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ): """Create a resumable upload session. @@ -2862,41 +2857,6 @@ def create_resumable_upload_session( delete the uploaded object automatically. Supported values are "md5", "crc32c" and None. The default is None. - :type if_generation_match: long - :param if_generation_match: - (Optional) See :ref:`using-if-generation-match` - - :type if_generation_not_match: long - :param if_generation_not_match: - (Optional) See :ref:`using-if-generation-not-match` - - :type if_metageneration_match: long - :param if_metageneration_match: - (Optional) See :ref:`using-if-metageneration-match` - - :type if_metageneration_not_match: long - :param if_metageneration_not_match: - (Optional) See :ref:`using-if-metageneration-not-match` - - :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy - :param retry: (Optional) How to retry the RPC. A None value will disable - retries. A google.api_core.retry.Retry value will enable retries, - and the object will define retriable response codes and errors and - configure backoff and timeout options. - A google.cloud.storage.retry.ConditionalRetryPolicy value wraps a - Retry object and activates it only if certain conditions are met. - 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. - See the retry.py source code and docstrings in this package - (google.cloud.storage.retry) for information on retry types and how - to configure them. - Media operations (downloads and uploads) do not support non-default - predicates in a Retry object. The default will always be used. Other - configuration changes for Retry objects such as delays and deadlines - are respected. - :rtype: str :returns: The resumable upload session URL. The upload can be completed by making an HTTP PUT request with the @@ -2905,19 +2865,6 @@ def create_resumable_upload_session( :raises: :class:`google.cloud.exceptions.GoogleCloudError` if the session creation response returns an error status. """ - - # Handle ConditionalRetryPolicy. - if isinstance(retry, ConditionalRetryPolicy): - # Conditional retries are designed for non-media calls, which change - # arguments into query_params dictionaries. Media operations work - # differently, so here we make a "fake" query_params to feed to the - # ConditionalRetryPolicy. - query_params = { - "ifGenerationMatch": if_generation_match, - "ifMetagenerationMatch": if_metageneration_match, - } - retry = retry.get_retry_policy_if_conditions_met(query_params=query_params) - extra_headers = {} if origin is not None: # This header is specifically for client-side uploads, it @@ -2936,15 +2883,10 @@ def create_resumable_upload_session( size, None, predefined_acl=None, - if_generation_match=if_generation_match, - if_generation_not_match=if_generation_not_match, - if_metageneration_match=if_metageneration_match, - if_metageneration_not_match=if_metageneration_not_match, extra_headers=extra_headers, chunk_size=self._CHUNK_SIZE_MULTIPLE, timeout=timeout, checksum=checksum, - retry=retry, ) return upload.resumable_url diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index cc9086b5f..02aa22349 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -3249,18 +3249,8 @@ def test_upload_from_string_w_text_w_num_retries(self): self._upload_from_string_helper(data, num_retries=2) def _create_resumable_upload_session_helper( - self, - origin=None, - side_effect=None, - timeout=None, - if_generation_match=None, - if_generation_not_match=None, - if_metageneration_match=None, - if_metageneration_not_match=None, - retry=None, + self, origin=None, side_effect=None, timeout=None ): - from six.moves.urllib.parse import urlencode - bucket = _Bucket(name="alex-trebek") blob = self._make_one("blob-name", bucket=bucket) chunk_size = 99 * blob._CHUNK_SIZE_MULTIPLE @@ -3286,19 +3276,11 @@ def _create_resumable_upload_session_helper( expected_timeout = timeout timeout_kwarg = {"timeout": timeout} - if retry is DEFAULT_RETRY_IF_GENERATION_SPECIFIED: - retry = DEFAULT_RETRY if if_generation_match else None - new_url = blob.create_resumable_upload_session( content_type=content_type, size=size, origin=origin, client=client, - if_generation_match=if_generation_match, - if_generation_not_match=if_generation_not_match, - if_metageneration_match=if_metageneration_match, - if_metageneration_not_match=if_metageneration_not_match, - retry=retry, **timeout_kwarg ) @@ -3308,23 +3290,10 @@ def _create_resumable_upload_session_helper( # Check the mocks. upload_url = ( - "https://storage.googleapis.com/upload/storage/v1" + bucket.path + "/o" + "https://storage.googleapis.com/upload/storage/v1" + + bucket.path + + "/o?uploadType=resumable" ) - - qs_params = [("uploadType", "resumable")] - if if_generation_match is not None: - qs_params.append(("ifGenerationMatch", if_generation_match)) - - if if_generation_not_match is not None: - qs_params.append(("ifGenerationNotMatch", if_generation_not_match)) - - if if_metageneration_match is not None: - qs_params.append(("ifMetagenerationMatch", if_metageneration_match)) - - if if_metageneration_not_match is not None: - qs_params.append(("ifMetaGenerationNotMatch", if_metageneration_not_match)) - - upload_url += "?" + urlencode(qs_params) payload = b'{"name": "blob-name"}' expected_headers = { "content-type": "application/json; charset=UTF-8", @@ -3350,16 +3319,6 @@ def test_create_resumable_upload_session_with_custom_timeout(self): def test_create_resumable_upload_session_with_origin(self): self._create_resumable_upload_session_helper(origin="http://google.com") - def test_create_resumable_upload_session_with_conditional_retry_success(self): - self._create_resumable_upload_session_helper( - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, if_generation_match=1 - ) - - def test_create_resumable_upload_session_with_conditional_retry_failure(self): - self._create_resumable_upload_session_helper( - retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED - ) - def test_create_resumable_upload_session_with_failure(self): from google.resumable_media import InvalidResponse from google.cloud import exceptions From 63ec0835c03df6730f6c2055695247a23ba4d743 Mon Sep 17 00:00:00 2001 From: Cathy Ouyang Date: Tue, 29 Jun 2021 13:01:44 -0700 Subject: [PATCH 9/9] revise tests after reverting --- tests/unit/test_blob.py | 9 ++++----- tests/unit/test_fileio.py | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 02aa22349..8c4d9b955 100644 --- a/tests/unit/test_blob.py +++ b/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 diff --git a/tests/unit/test_fileio.py b/tests/unit/test_fileio.py index 067fdab00..9fadc967c 100644 --- a/tests/unit/test_fileio.py +++ b/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_generation_match=123456789, # Note: Assuming this is the blob generation + 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_generation_match=123456789, + if_generation_match=123456, ) upload.transmit_next_chunk.assert_called_with(transport) self.assertEqual(upload.transmit_next_chunk.call_count, 4)