From 70d19e72831dee112bb07f38b50beef4890c1155 Mon Sep 17 00:00:00 2001 From: cojenco Date: Mon, 14 Jun 2021 15:31:15 -0700 Subject: [PATCH] fix: revise blob.compose query parameters `if_generation_match` (#454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * revise blob.compose logic to match API usage * update tests * update system test * address comments * 🦉 Updates from OwlBot * revise logic for backwards compatibility * add tests * revise docstring * fix test * revise to DeprecationWarning * address comments and revise docstrings Co-authored-by: Tres Seaver Co-authored-by: Owl Bot --- google/cloud/storage/blob.py | 114 +++++++++++------ tests/system/test_system.py | 45 ++++++- tests/unit/test_blob.py | 239 ++++++++++++++++++++++++++++++----- 3 files changed, 329 insertions(+), 69 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 597e63ca4..c22e6699c 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -3198,6 +3198,7 @@ def compose( timeout=_DEFAULT_TIMEOUT, if_generation_match=None, if_metageneration_match=None, + if_source_generation_match=None, retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, ): """Concatenate source blobs into this one. @@ -3218,73 +3219,98 @@ def compose( (Optional) The amount of time, in seconds, to wait for the server response. See: :ref:`configuring_timeouts` - :type if_generation_match: list of long + :type if_generation_match: long :param if_generation_match: - (Optional) Make the operation conditional on whether the blob's - current generation matches the given value. Setting to 0 makes the - operation succeed only if there are no live versions of the blob. - The list must match ``sources`` item-to-item. + (Optional) Makes the operation conditional on whether the + destination object's current generation matches the given value. + Setting to 0 makes the operation succeed only if there are no live + versions of the object. + + Note: In a previous version, this argument worked identically to the + ``if_source_generation_match`` argument. For backwards-compatibility reasons, + if a list is passed in, this argument will behave like ``if_source_generation_match`` + and also issue a DeprecationWarning. - :type if_metageneration_match: list of long + :type if_metageneration_match: long :param if_metageneration_match: - (Optional) Make the operation conditional on whether the blob's - current metageneration matches the given value. The list must match - ``sources`` item-to-item. + (Optional) Makes the operation conditional on whether the + destination object's current metageneration matches the given + value. + + If a list of long is passed in, no match operation will be performed. + (Deprecated: type(list of long) is supported for backwards-compatability reasons only.) + + :type if_source_generation_match: list of long + :param if_source_generation_match: + (Optional) Makes the operation conditional on whether the current generation + of each source blob matches the corresponding generation. + The list must match ``sources`` item-to-item. :type retry: google.api_core.retry.Retry or google.cloud.storage.retry.ConditionalRetryPolicy :param retry: (Optional) How to retry the RPC. See: :ref:`configuring_retries` Example: - Compose blobs using generation match preconditions. + Compose blobs using source generation match preconditions. >>> from google.cloud import storage >>> client = storage.Client() >>> bucket = client.bucket("bucket-name") >>> blobs = [bucket.blob("blob-name-1"), bucket.blob("blob-name-2")] - >>> if_generation_match = [None] * len(blobs) - >>> if_generation_match[0] = "123" # precondition for "blob-name-1" + >>> if_source_generation_match = [None] * len(blobs) + >>> if_source_generation_match[0] = "123" # precondition for "blob-name-1" >>> composed_blob = bucket.blob("composed-name") - >>> composed_blob.compose(blobs, if_generation_match) + >>> composed_blob.compose(blobs, if_source_generation_match=if_source_generation_match) """ sources_len = len(sources) - if if_generation_match is not None and len(if_generation_match) != sources_len: - raise ValueError( - "'if_generation_match' length must be the same as 'sources' length" + client = self._require_client(client) + query_params = {} + + if isinstance(if_generation_match, list): + warnings.warn( + "if_generation_match: type list is deprecated and supported for backwards-compatability reasons only." + "Use if_source_generation_match instead to match source objects generations.", + DeprecationWarning, + stacklevel=2, ) - if ( - if_metageneration_match is not None - and len(if_metageneration_match) != sources_len - ): - raise ValueError( - "'if_metageneration_match' length must be the same as 'sources' length" + if if_source_generation_match is not None: + raise ValueError( + "Use if_generation_match to match the generation of the destination object by passing in a generation number, instead of a list." + "Use if_source_generation_match to match source objects generations." + ) + + # if_generation_match: type list is deprecated. Instead use if_source_generation_match. + if_source_generation_match = if_generation_match + if_generation_match = None + + if isinstance(if_metageneration_match, list): + warnings.warn( + "if_metageneration_match: type list is deprecated and supported for backwards-compatability reasons only." + "Note that the metageneration to be matched is that of the destination blob." + "Please pass in a single value (type long).", + DeprecationWarning, + stacklevel=2, ) - client = self._require_client(client) - query_params = {} + if_metageneration_match = None - if self.user_project is not None: - query_params["userProject"] = self.user_project + if if_source_generation_match is None: + if_source_generation_match = [None] * sources_len + if len(if_source_generation_match) != sources_len: + raise ValueError( + "'if_source_generation_match' length must be the same as 'sources' length" + ) source_objects = [] - for index, source in enumerate(sources): - source_object = {"name": source.name} + for source, source_generation in zip(sources, if_source_generation_match): + source_object = {"name": source.name, "generation": source.generation} preconditions = {} - if ( - if_generation_match is not None - and if_generation_match[index] is not None - ): - preconditions["ifGenerationMatch"] = if_generation_match[index] - - if ( - if_metageneration_match is not None - and if_metageneration_match[index] is not None - ): - preconditions["ifMetagenerationMatch"] = if_metageneration_match[index] + if source_generation is not None: + preconditions["ifGenerationMatch"] = source_generation if preconditions: source_object["objectPreconditions"] = preconditions @@ -3295,6 +3321,16 @@ def compose( "sourceObjects": source_objects, "destination": self._properties.copy(), } + + if self.user_project is not None: + query_params["userProject"] = self.user_project + + _add_generation_match_parameters( + query_params, + if_generation_match=if_generation_match, + if_metageneration_match=if_metageneration_match, + ) + api_response = client._post_resource( "{}/compose".format(self.path), request, diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 6ec1c2a68..2a5cad487 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -1723,7 +1723,7 @@ def test_compose_replace_existing_blob(self): composed = original.download_as_bytes() self.assertEqual(composed, BEFORE + TO_APPEND) - def test_compose_with_generation_match(self): + def test_compose_with_generation_match_list(self): BEFORE = b"AAA\n" original = self.bucket.blob("original") original.content_type = "text/plain" @@ -1751,6 +1751,49 @@ def test_compose_with_generation_match(self): composed = original.download_as_bytes() self.assertEqual(composed, BEFORE + TO_APPEND) + def test_compose_with_generation_match_long(self): + BEFORE = b"AAA\n" + original = self.bucket.blob("original") + original.content_type = "text/plain" + original.upload_from_string(BEFORE) + self.case_blobs_to_delete.append(original) + + TO_APPEND = b"BBB\n" + to_append = self.bucket.blob("to_append") + to_append.upload_from_string(TO_APPEND) + self.case_blobs_to_delete.append(to_append) + + with self.assertRaises(google.api_core.exceptions.PreconditionFailed): + original.compose([original, to_append], if_generation_match=0) + + original.compose([original, to_append], if_generation_match=original.generation) + + composed = original.download_as_bytes() + self.assertEqual(composed, BEFORE + TO_APPEND) + + def test_compose_with_source_generation_match(self): + BEFORE = b"AAA\n" + original = self.bucket.blob("original") + original.content_type = "text/plain" + original.upload_from_string(BEFORE) + self.case_blobs_to_delete.append(original) + + TO_APPEND = b"BBB\n" + to_append = self.bucket.blob("to_append") + to_append.upload_from_string(TO_APPEND) + self.case_blobs_to_delete.append(to_append) + + with self.assertRaises(google.api_core.exceptions.PreconditionFailed): + original.compose([original, to_append], if_source_generation_match=[6, 7]) + + original.compose( + [original, to_append], + if_source_generation_match=[original.generation, to_append.generation], + ) + + composed = original.download_as_bytes() + self.assertEqual(composed, BEFORE + TO_APPEND) + @unittest.skipUnless(USER_PROJECT, "USER_PROJECT not set in environment.") def test_compose_with_user_project(self): new_bucket_name = "compose-user-project" + unique_resource_id("-") diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 46a130dc8..158109705 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -3754,7 +3754,10 @@ def test_compose_wo_content_type_set(self): expected_path = "/b/name/o/%s/compose" % destination_name expected_data = { - "sourceObjects": [{"name": source_1_name}, {"name": source_2_name}], + "sourceObjects": [ + {"name": source_1.name, "generation": source_1.generation}, + {"name": source_2.name, "generation": source_2.generation}, + ], "destination": {}, } expected_query_params = {} @@ -3788,7 +3791,10 @@ def test_compose_minimal_w_user_project_w_timeout(self): expected_path = "/b/name/o/%s/compose" % destination_name expected_data = { - "sourceObjects": [{"name": source_1_name}, {"name": source_2_name}], + "sourceObjects": [ + {"name": source_1.name, "generation": source_1.generation}, + {"name": source_2.name, "generation": source_2.generation}, + ], "destination": {"contentType": "text/plain"}, } expected_query_params = {"userProject": user_project} @@ -3823,7 +3829,10 @@ def test_compose_w_additional_property_changes_w_retry(self): expected_path = "/b/name/o/%s/compose" % destination_name expected_data = { - "sourceObjects": [{"name": source_1_name}, {"name": source_2_name}], + "sourceObjects": [ + {"name": source_1.name, "generation": source_1.generation}, + {"name": source_2.name, "generation": source_2.generation}, + ], "destination": { "contentType": "text/plain", "contentLanguage": "en-US", @@ -3840,13 +3849,12 @@ def test_compose_w_additional_property_changes_w_retry(self): _target_object=destination, ) - def test_compose_w_generation_match(self): + def test_compose_w_source_generation_match(self): source_1_name = "source-1" source_2_name = "source-2" destination_name = "destination" api_response = {} - generation_numbers = [6, 9] - metageneration_numbers = [7, 1] + source_generation_numbers = [6, 9] client = mock.Mock(spec=["_post_resource"]) client._post_resource.return_value = api_response @@ -3857,25 +3865,24 @@ def test_compose_w_generation_match(self): destination = self._make_one(destination_name, bucket=bucket) destination.compose( sources=[source_1, source_2], - if_generation_match=generation_numbers, - if_metageneration_match=metageneration_numbers, + if_source_generation_match=source_generation_numbers, ) expected_path = "/b/name/o/%s/compose" % destination_name expected_data = { "sourceObjects": [ { - "name": source_1_name, + "name": source_1.name, + "generation": source_1.generation, "objectPreconditions": { - "ifGenerationMatch": generation_numbers[0], - "ifMetagenerationMatch": metageneration_numbers[0], + "ifGenerationMatch": source_generation_numbers[0], }, }, { - "name": source_2_name, + "name": source_2.name, + "generation": source_2.generation, "objectPreconditions": { - "ifGenerationMatch": generation_numbers[1], - "ifMetagenerationMatch": metageneration_numbers[1], + "ifGenerationMatch": source_generation_numbers[1], }, }, ], @@ -3891,11 +3898,11 @@ def test_compose_w_generation_match(self): _target_object=destination, ) - def test_compose_w_generation_match_bad_length(self): + def test_compose_w_source_generation_match_bad_length(self): source_1_name = "source-1" source_2_name = "source-2" destination_name = "destination" - generation_numbers = [6] + source_generation_numbers = [6] client = mock.Mock(spec=["_post_resource"]) bucket = _Bucket(client=client) source_1 = self._make_one(source_1_name, bucket=bucket) @@ -3905,35 +3912,59 @@ def test_compose_w_generation_match_bad_length(self): with self.assertRaises(ValueError): destination.compose( - sources=[source_1, source_2], if_generation_match=generation_numbers + sources=[source_1, source_2], + if_source_generation_match=source_generation_numbers, ) client._post_resource.assert_not_called() - def test_compose_w_metageneration_match_bad_length(self): + def test_compose_w_source_generation_match_nones(self): source_1_name = "source-1" source_2_name = "source-2" destination_name = "destination" - metageneration_numbers = [7] + source_generation_numbers = [6, None] + api_response = {} client = mock.Mock(spec=["_post_resource"]) + client._post_resource.return_value = api_response bucket = _Bucket(client=client) source_1 = self._make_one(source_1_name, bucket=bucket) source_2 = self._make_one(source_2_name, bucket=bucket) destination = self._make_one(destination_name, bucket=bucket) - with self.assertRaises(ValueError): - destination.compose( - sources=[source_1, source_2], - if_metageneration_match=metageneration_numbers, - ) + destination.compose( + sources=[source_1, source_2], + if_source_generation_match=source_generation_numbers, + ) - client._post_resource.assert_not_called() + expected_path = "/b/name/o/%s/compose" % destination_name + expected_data = { + "sourceObjects": [ + { + "name": source_1.name, + "generation": source_1.generation, + "objectPreconditions": { + "ifGenerationMatch": source_generation_numbers[0], + }, + }, + {"name": source_2.name, "generation": source_2.generation}, + ], + "destination": {}, + } + expected_query_params = {} + client._post_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + _target_object=destination, + ) - def test_compose_w_generation_match_nones(self): + def test_compose_w_generation_match(self): source_1_name = "source-1" source_2_name = "source-2" destination_name = "destination" - generation_numbers = [6, None] + generation_number = 1 api_response = {} client = mock.Mock(spec=["_post_resource"]) client._post_resource.return_value = api_response @@ -3943,7 +3974,44 @@ def test_compose_w_generation_match_nones(self): destination = self._make_one(destination_name, bucket=bucket) destination.compose( - sources=[source_1, source_2], if_generation_match=generation_numbers + sources=[source_1, source_2], if_generation_match=generation_number, + ) + + expected_path = "/b/name/o/%s/compose" % destination_name + expected_data = { + "sourceObjects": [ + {"name": source_1.name, "generation": source_1.generation}, + {"name": source_2.name, "generation": source_2.generation}, + ], + "destination": {}, + } + expected_query_params = {"ifGenerationMatch": generation_number} + client._post_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + _target_object=destination, + ) + + @mock.patch("warnings.warn") + def test_compose_w_generation_match_w_warning(self, mock_warn): + source_1_name = "source-1" + source_2_name = "source-2" + destination_name = "destination" + api_response = {} + generation_numbers = [6, 9] + + client = mock.Mock(spec=["_post_resource"]) + client._post_resource.return_value = api_response + bucket = _Bucket(client=client) + source_1 = self._make_one(source_1_name, bucket=bucket) + source_2 = self._make_one(source_2_name, bucket=bucket) + + destination = self._make_one(destination_name, bucket=bucket) + destination.compose( + sources=[source_1, source_2], if_generation_match=generation_numbers, ) expected_path = "/b/name/o/%s/compose" % destination_name @@ -3951,11 +4019,18 @@ def test_compose_w_generation_match_nones(self): "sourceObjects": [ { "name": source_1_name, + "generation": None, "objectPreconditions": { "ifGenerationMatch": generation_numbers[0], }, }, - {"name": source_2_name}, + { + "name": source_2_name, + "generation": None, + "objectPreconditions": { + "ifGenerationMatch": generation_numbers[1], + }, + }, ], "destination": {}, } @@ -3969,6 +4044,112 @@ def test_compose_w_generation_match_nones(self): _target_object=destination, ) + mock_warn.assert_called_with( + "if_generation_match: type list is deprecated and supported for backwards-compatability reasons only." + "Use if_source_generation_match instead to match source objects generations.", + DeprecationWarning, + stacklevel=2, + ) + + def test_compose_invalid_generation_match(self): + source_1_name = "source-1" + source_2_name = "source-2" + destination_name = "destination" + source_generation_numbers = [6, 8] + client = mock.Mock(spec=["_post_resource"]) + bucket = _Bucket(client=client) + source_1 = self._make_one(source_1_name, bucket=bucket) + source_2 = self._make_one(source_2_name, bucket=bucket) + + destination = self._make_one(destination_name, bucket=bucket) + + with self.assertRaises(ValueError): + destination.compose( + sources=[source_1, source_2], + if_generation_match=source_generation_numbers, + if_source_generation_match=source_generation_numbers, + ) + + client._post_resource.assert_not_called() + + @mock.patch("warnings.warn") + def test_compose_w_metageneration_match_w_warning(self, mock_warn): + source_1_name = "source-1" + source_2_name = "source-2" + destination_name = "destination" + metageneration_number = [6] + client = mock.Mock(spec=["_post_resource"]) + bucket = _Bucket(client=client) + source_1 = self._make_one(source_1_name, bucket=bucket) + source_2 = self._make_one(source_2_name, bucket=bucket) + + destination = self._make_one(destination_name, bucket=bucket) + + destination.compose( + sources=[source_1, source_2], if_metageneration_match=metageneration_number, + ) + + expected_path = "/b/name/o/%s/compose" % destination_name + expected_data = { + "sourceObjects": [ + {"name": source_1_name, "generation": None}, + {"name": source_2_name, "generation": None}, + ], + "destination": {}, + } + expected_query_params = {} + client._post_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + _target_object=destination, + ) + + mock_warn.assert_called_with( + "if_metageneration_match: type list is deprecated and supported for backwards-compatability reasons only." + "Note that the metageneration to be matched is that of the destination blob." + "Please pass in a single value (type long).", + DeprecationWarning, + stacklevel=2, + ) + + def test_compose_w_metageneration_match(self): + source_1_name = "source-1" + source_2_name = "source-2" + destination_name = "destination" + metageneration_number = 1 + api_response = {} + client = mock.Mock(spec=["_post_resource"]) + client._post_resource.return_value = api_response + bucket = _Bucket(client=client) + source_1 = self._make_one(source_1_name, bucket=bucket) + source_2 = self._make_one(source_2_name, bucket=bucket) + destination = self._make_one(destination_name, bucket=bucket) + + destination.compose( + sources=[source_1, source_2], if_metageneration_match=metageneration_number, + ) + + expected_path = "/b/name/o/%s/compose" % destination_name + expected_data = { + "sourceObjects": [ + {"name": source_1.name, "generation": source_1.generation}, + {"name": source_2.name, "generation": source_2.generation}, + ], + "destination": {}, + } + expected_query_params = {"ifMetagenerationMatch": metageneration_number} + client._post_resource.assert_called_once_with( + expected_path, + expected_data, + query_params=expected_query_params, + timeout=self._get_default_timeout(), + retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED, + _target_object=destination, + ) + def test_rewrite_w_response_wo_resource(self): source_name = "source" dest_name = "dest"