Skip to content

support_delete_source_object#62

Draft
shubhangi-google wants to merge 1 commit into
mainfrom
support_delete_source_object
Draft

support_delete_source_object#62
shubhangi-google wants to merge 1 commit into
mainfrom
support_delete_source_object

Conversation

@shubhangi-google

Copy link
Copy Markdown
Owner

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
  • Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
  • Update code documentation if necessary.

closes: #<issue_number_goes_here>

@shubhangi-google shubhangi-google marked this pull request as draft June 12, 2026 05:37

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

Copy link
Copy Markdown

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 option to the compose method, allowing users to automatically delete source files after composing them into a new file. However, the reviewer correctly points out that the underlying Google Cloud Storage JSON API's ComposeRequest does not support this parameter, which would cause an ArgumentError at runtime. The feedback advises implementing this deletion client-side within the bucket layer instead, and provides detailed code suggestions to update the service layer, test helpers, and unit tests accordingly.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +2237 to +2238
user_project: user_project,
delete_source_objects: delete_source_objects

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since the Google Cloud Storage JSON API does not support deleting source objects during a compose operation, this feature must be implemented client-side. After a successful compose call, we should iterate over the source files and delete them.

                                      user_project: user_project
          if delete_source_objects
            sources.each do |source|
              source_name = source.respond_to?(:name) ? source.name : source
              service.delete_file name, source_name, user_project: user_project
            end
          end

if_generation_match: nil,
if_metageneration_match: nil,
user_project: nil,
delete_source_objects: nil,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The Google Cloud Storage JSON API's ComposeRequest does not support a delete_source_objects field. Passing this parameter to Google::Apis::StorageV1::ComposeRequest.new will result in an ArgumentError: unknown keyword: :delete_source_objects at runtime. Please remove this parameter from the service layer, as the deletion of source objects must be handled client-side in the bucket layer.

compose_req = Google::Apis::StorageV1::ComposeRequest.new source_objects: source_objects,
destination: destination_gapi
destination: destination_gapi,
delete_source_objects: delete_source_objects

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Remove delete_source_objects from the ComposeRequest initialization since it is not a supported field in the GCS API.

                                                                    destination: destination_gapi



def compose_request source_files, destination_gapi = nil, if_source_generation_match: nil
def compose_request source_files, destination_gapi = nil, if_source_generation_match: nil, delete_source_objects: nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Remove the delete_source_objects parameter from the helper method since it is not supported by ComposeRequest.

  def compose_request source_files, destination_gapi = nil, if_source_generation_match: nil

Comment on lines +643 to +644
source_objects: source_objects,
delete_source_objects: delete_source_objects

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Remove delete_source_objects from the ComposeRequest initialization in the test helper.

      source_objects: source_objects

Comment on lines +271 to +283
it "can compose a new file with delete_source_objects option" do
mock = Minitest::Mock.new
req = compose_request [file_gapi, file_2_gapi], delete_source_objects: true
mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})

bucket.service.mocked_service = mock

new_file = bucket.compose [file, file_2], file_3_name, delete_source_objects: true
_(new_file).must_be_kind_of Google::Cloud::Storage::File
_(new_file.name).must_equal file_3_name

mock.verify
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since delete_source_objects is now implemented client-side, the unit test needs to mock the delete_object calls on the service for each source file being deleted.

  it "can compose a new file with delete_source_objects option" do
    mock = Minitest::Mock.new
    req = compose_request [file_gapi, file_2_gapi]
    mock.expect :compose_object, file_3_gapi, [bucket.name, file_3_name, req], **compose_object_args(options: {retries: 0})
    mock.expect :delete_object, nil, [bucket.name, file.name], **compose_object_args(options: {})
    mock.expect :delete_object, nil, [bucket.name, file_2.name], **compose_object_args(options: {})

    bucket.service.mocked_service = mock

    new_file = bucket.compose [file, file_2], file_3_name, delete_source_objects: true
    _(new_file).must_be_kind_of Google::Cloud::Storage::File
    _(new_file.name).must_equal file_3_name

    mock.verify
  end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant