chore: Error handling when upload id I'd incorrect for Resumable upload #6
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @shubhangi-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly improves the robustness of resumable upload operations by adding explicit error handling for scenarios where an upload session ID is missing or invalid. It ensures that the system correctly identifies and responds to 404 (Not Found) errors during reinitiation and cancellation of uploads, preventing unintended behavior and providing clearer feedback.
Highlights
- Improved Resumable Upload Error Handling: Enhanced the
reinitiate_resumable_uploadandcheck_resumable_uploadmethods to explicitly handle 404 (Not Found) responses, ensuring that operations correctly fail when an upload session ID is missing or invalid. - Refined Cancellation Logic: Modified
cancel_resumable_uploadto prevent it from incorrectly reporting success or proceeding with cancellation if the upload session is not found (404 status code). - Comprehensive Test Coverage: Introduced new test cases across
service_spec.rbandstorage_upload_spec.rbto validate the new 404 error handling for both restarting and deleting resumable uploads.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
shubhangi-google
requested review from
gurusai-voleti,
mahendra-google and
thiyaguk09
There was a problem hiding this comment.
Code Review
This pull request aims to improve the handling of resumable uploads when a provided upload ID is missing or invalid, which typically results in a 404 Not Found error. The changes correctly adjust the logic for canceling uploads to treat a 404 as a failure and add relevant test cases for both restarting and deleting uploads with invalid IDs.
However, I've identified a critical bug in the implementation for re-initiating an upload. The current change inadvertently breaks the functionality for correctly resuming an incomplete upload (i.e., when a 308 Resume Incomplete is received). I have provided a detailed comment and a code suggestion to fix this issue. I've also included a minor suggestion to improve code style.
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
can you check this does handle_resumable_upload_http_response_codes is returning 404 specifically? @shubhangi-google
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ok got it, the response object is filled in that method, and check 404 here, other than 404 it return true?
There was a problem hiding this comment.
yes either it will return true or it will do further processing depending on the flow
| unless check_resumable_upload client | ||
| return false | ||
| end |
There was a problem hiding this comment.
For improved conciseness and readability, you can use a one-line guard clause here.
return false unless check_resumable_upload client| 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 |
There was a problem hiding this comment.
it seems 404 check should be with or condition?
There was a problem hiding this comment.
yes it comes in this condition but we want to perform additional actions in case of 404
There was a problem hiding this comment.
ok for 404 else case will work?
shubhangi-google
changed the title
Updated code to explicitly handle 404 (Not Found) responses, ensuring that operations correctly fail when an upload session ID is invalid.
Error handling done for code implemented in - googleapis#21896, googleapis#23376
Implementation linked with- googleapis/google-cloud-ruby#30882