Skip to content

feat(storage): support deleteSourceObjects option in compose sample#14348

Closed
nidhiii-27 wants to merge 1 commit into
mainfrom
compose-delete-sources
Closed

feat(storage): support deleteSourceObjects option in compose sample#14348
nidhiii-27 wants to merge 1 commit into
mainfrom
compose-delete-sources

Conversation

@nidhiii-27

Copy link
Copy Markdown
Contributor

Add deleteSourceObjects parameter to compose sample.
Updated test to assert both delete behavior scenarios.

Add deleteSourceObjects parameter to compose sample.
Updated test to assert both delete behavior scenarios.

[Generated-by: AI]

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a delete_source_objects parameter to the compose_file snippet, allowing source blobs to be deleted after composition, and adds corresponding unit tests. The feedback suggests refactoring the print statements in storage_compose_file.py to reduce string duplication, and moving the destination blob cleanup inside the with block in the tests to improve scoping and clarity.

Comment on lines +60 to 71
if delete_source_objects:
print(
"New composite object {} in the bucket {} was created by combining {} and {}. Source objects were deleted.".format(
destination_blob_name, bucket_name, first_blob_name, second_blob_name
)
)
else:
print(
"New composite object {} in the bucket {} was created by combining {} and {}".format(
destination_blob_name, bucket_name, first_blob_name, second_blob_name
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The print statements for delete_source_objects contain duplicated text. We can simplify this by constructing the base message first and conditionally appending the deletion status. This reduces duplication and improves maintainability.

    message = "New composite object {} in the bucket {} was created by combining {} and {}".format(
        destination_blob_name, bucket_name, first_blob_name, second_blob_name
    )
    if delete_source_objects:
        message += " Source objects were deleted."
    print(message)

Comment on lines +694 to +709
with tempfile.NamedTemporaryFile() as dest_file:
destination = storage_compose_file.compose_file(
test_bucket.name,
source_files[0],
source_files[1],
dest_file.name,
delete_source_objects=True,
)
composed = destination.download_as_bytes()

assert composed.decode("utf-8") == source_files[0] + source_files[1]
assert not test_bucket.blob(source_files[0]).exists()
assert not test_bucket.blob(source_files[1]).exists()

# Clean up destination file
destination.delete()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the previous test block, the destination blob cleanup should be moved inside the with block to keep the scope localized, and the comment updated to refer to 'destination blob' instead of 'destination file'.

Suggested change
with tempfile.NamedTemporaryFile() as dest_file:
destination = storage_compose_file.compose_file(
test_bucket.name,
source_files[0],
source_files[1],
dest_file.name,
delete_source_objects=True,
)
composed = destination.download_as_bytes()
assert composed.decode("utf-8") == source_files[0] + source_files[1]
assert not test_bucket.blob(source_files[0]).exists()
assert not test_bucket.blob(source_files[1]).exists()
# Clean up destination file
destination.delete()
with tempfile.NamedTemporaryFile() as dest_file:
destination = storage_compose_file.compose_file(
test_bucket.name,
source_files[0],
source_files[1],
dest_file.name,
delete_source_objects=True,
)
composed = destination.download_as_bytes()
assert composed.decode("utf-8") == source_files[0] + source_files[1]
assert not test_bucket.blob(source_files[0]).exists()
assert not test_bucket.blob(source_files[1]).exists()
# Clean up destination blob
destination.delete()

@nidhiii-27 nidhiii-27 closed this Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant