fix: revise blob.compose query parameters if_generation_match (#454)

* 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 <[email protected]>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Author: 3 people

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,

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("-")