Skip to content

chore: Error handling when upload id I'd incorrect for Resumable upload #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shubhangi-google wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: Error handling when upload id I'd incorrect for Resumable upload #6

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e73064f
handling missing upload_id in resumable upload
shubhangi-google Sep 7, 2025
e6884c0
adding test cases
shubhangi-google Sep 8, 2025
1c4c192
refactor
shubhangi-google Sep 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions google-apis-core/lib/google/apis/core/storage_upload.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ def initiate_resumable_upload(client)
# Reinitiating resumable upload
def reinitiate_resumable_upload(client)
logger.debug { sprintf('Restarting resumable upload command to %s', url) }
check_resumable_upload client
unless check_resumable_upload client
return false
end
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Sep 8, 2025

Choose a reason for hiding this comment

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

medium

For improved conciseness and readability, you can use a one-line guard clause here.

          return false unless check_resumable_upload client

upload_io.pos = @offset
end

Expand Down Expand Up @@ -224,6 +226,10 @@ def check_resumable_upload(client)
# Initiating call
response = client.put(@upload_url, "", request_header)
handle_resumable_upload_http_response_codes(response)
if response.status.to_i == 404
logger.debug { sprintf("Failed to fetch upload session. Response: #{response.status.to_i} - #{response.body}") }
false
end
Comment on lines 226 to +230
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Sep 8, 2025

Choose a reason for hiding this comment

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

critical

This change introduces a bug that breaks the resumable upload functionality. When the server responds with a 308 status (Resume Incomplete), handle_resumable_upload_http_response_codes correctly returns true. However, the if statement on line 229 is the last expression in the method, and when its condition is false, it evaluates to nil. This causes check_resumable_upload to return nil (a falsy value).

As a result, in reinitiate_resumable_upload, the unless check_resumable_upload client condition becomes true, causing the method to incorrectly return false instead of proceeding with the upload.

To fix this, check_resumable_upload must explicitly return the result from handle_resumable_upload_http_response_codes for non-404 cases.

          result = handle_resumable_upload_http_response_codes(response)
          if response.status.to_i == 404
            logger.debug { sprintf("Failed to fetch upload session. Response: #{response.status.to_i} - #{response.body}") }
            return false
          end
          result

Copy link
Copy Markdown
Collaborator

@gurusai-voleti gurusai-voleti Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

can you check this does handle_resumable_upload_http_response_codes is returning 404 specifically? @shubhangi-google

Copy link
Copy Markdown
Owner Author

@shubhangi-google shubhangi-google Sep 8, 2025

Choose a reason for hiding this comment

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

no handle_resumable_upload_http_response_codes will not return 404 it sets the value of variable which tells if the ubload is complete or incomplete
handle_resumable_upload_http_response_codes sets the value of variable so that further processing can be done in other methods

Copy link
Copy Markdown
Collaborator

@gurusai-voleti gurusai-voleti Sep 8, 2025

Choose a reason for hiding this comment

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

ok got it, the response object is filled in that method, and check 404 here, other than 404 it return true?

Copy link
Copy Markdown
Owner Author

@shubhangi-google shubhangi-google Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yes either it will return true or it will do further processing depending on the flow

end

# Cancel resumable upload
Expand All @@ -234,14 +240,13 @@ def cancel_resumable_upload(client)
# Initiating call
response = client.delete(@upload_url, nil, request_header)
handle_resumable_upload_http_response_codes(response)

if !@upload_incomplete && (400..499).include?(response.status.to_i)
if !@upload_incomplete && (400..499).include?(response.status.to_i) && response.status.to_i != 404
Copy link
Copy Markdown
Collaborator

@gurusai-voleti gurusai-voleti Sep 8, 2025

Choose a reason for hiding this comment

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

it seems 404 check should be with or condition?

Copy link
Copy Markdown
Owner Author

@shubhangi-google shubhangi-google Sep 8, 2025

Choose a reason for hiding this comment

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

yes it comes in this condition but we want to perform additional actions in case of 404

Copy link
Copy Markdown
Collaborator

@gurusai-voleti gurusai-voleti Sep 8, 2025

Choose a reason for hiding this comment

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

ok for 404 else case will work?

Copy link
Copy Markdown
Owner Author

@shubhangi-google shubhangi-google Sep 8, 2025

Choose a reason for hiding this comment

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

yes

@close_io_on_finish = true
true # method returns true if upload is successfully cancelled
else
logger.debug { sprintf("Failed to cancel upload session. Response: #{response.status.to_i} - #{response.body}") }
false
end

end

def handle_resumable_upload_http_response_codes(response)
Expand Down
36 changes: 36 additions & 0 deletions google-apis-core/spec/google/apis/core/service_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,24 @@
expect(a_request(:put, upload_url)).to have_been_made
end
end
context 'Should return false if wrong upload id is provided' do
let(:upload_id) { 'wrong_id' }
let(:command) do
service.send(
:restart_resumable_upload,
bucket_name, file, upload_id,
options: { upload_chunk_size: 11}
)
end
before(:example) do
stub_request(:put, upload_url)
.to_return(status: 404)
end
it 'should return false' do
expect(command).to be_falsey
end
end

end

context 'delete resumable upload with upload_id' do
Expand All @@ -312,6 +330,24 @@
expect(a_request(:delete, upload_url)).to have_been_made
expect(command).to be_truthy
end

context 'Should return false if wrong upload id is provided' do
let(:upload_id) { 'wrong_id' }
let(:command) do
service.send(
:delete_resumable_upload,
bucket_name, upload_id,
options: { upload_chunk_size: 11}
)
end
before(:example) do
stub_request(:delete, upload_url)
.to_return(status: 404)
end
it 'should return false' do
expect(command).to be_falsey
end
end
end

context 'with batch' do
Expand Down
17 changes: 17 additions & 0 deletions google-apis-core/spec/google/apis/core/storage_upload_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@
expect(a_request(:put, upload_url)
.with(body: 'Hello world')).to have_been_made
end

it 'should return false if wrong upload id is provided' do
stub_request(:put, upload_url)
.to_return(status: 404)
command.options.upload_chunk_size = 11
command.upload_id = "wrong_upload_id"
expect(command.execute(client)).to be_falsy
end
end

context('should not restart resumable upload if upload is completed') do
Expand Down Expand Up @@ -235,6 +243,15 @@
expect(a_request(:put, upload_url)
.with(body: 'Hello world')).to have_not_been_made
end

it 'should return false if wrong upload id is provided' do
stub_request(:delete, upload_url)
.to_return(status: 404)
command.options.upload_chunk_size = 11
command.upload_id = "wrong_upload_id"
command.delete_upload = true
expect(command.execute(client)).to be_falsy
end
end

context('with chunking disabled') do
Expand Down