Implement resumable upload status checking in the storage emulator#9047
Implement resumable upload status checking in the storage emulator#9047joehan merged 2 commits intofirebase:masterfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @Kazade, 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 enhances the functionality of the storage emulator by addressing several critical issues related to resumable uploads. The core purpose of these changes is to align the emulator's behavior more closely with the expected behavior of cloud storage services, particularly concerning how it handles partial uploads, status checks, and the completion of multi-part transfers. This ensures a more robust and accurate emulation environment for developers working with resumable and streaming upload patterns.
Highlights
- Resumable Upload Status Checking: The emulator now correctly returns a
308 Resume Incompletestatus code when checking the status of an incomplete resumable upload with aContent-Lengthof 0, instead of a400 Bad Request. - Multi-Chunk Resumable Uploads: Resumable uploads are no longer finalized after the first chunk, allowing for proper multi-chunk uploads. The emulator now waits for the final chunk as indicated by the
Content-Rangeheader before finalizing the upload. - Streaming Upload Support: The changes improve the handling of streaming uploads (uploads where the total size is initially unknown), ensuring they work correctly within the emulator.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces important fixes for resumable uploads in the storage emulator, such as handling status checks and multi-chunk uploads. My review focuses on ensuring the implementation aligns with the Google Cloud Storage resumable upload protocol. I've identified a couple of areas where the response status codes and headers deviate from the specification and have provided suggestions to correct them.
src/emulator/storage/apis/gcloud.ts
Outdated
| uploadService.continueResumableUpload(uploadId, await reqBodyToBuffer(req)); | ||
|
|
||
| const rangeHeader = req.headers["content-range"] ?? "bytes=0-0/*"; | ||
| const rangeParts = rangeHeader.substring(rangeHeader.indexOf("=")).split("/"); | ||
| const range = rangeParts[0]; | ||
|
|
||
| let rangeTotal = "*"; | ||
| if(rangeParts.length === 2) { | ||
| rangeTotal = rangeParts[1]; | ||
| } | ||
|
|
||
| const end = range.split("-")[1]; | ||
|
|
||
| if(rangeTotal !== "*" && parseInt(end) >= parseInt(rangeTotal) - 1) { | ||
| upload = uploadService.finalizeResumableUpload(uploadId); | ||
| } else { | ||
| return res.sendStatus(200); | ||
| } |
There was a problem hiding this comment.
There are a couple of issues with the handling of resumable upload chunks that deviate from the GCS specification:
- The
continueResumableUploadmethod returns the updated upload object containing the new total size. This return value is not being captured, so theuploadvariable holds stale data. - When a chunk is uploaded but the upload is not yet complete, the server should respond with
308 Resume Incomplete, not200 OK. - The
308response for an incomplete chunk upload should include aRangeheader indicating the total bytes received so far.
Here's a suggested change to address these points and align with the GCS documentation.
upload = uploadService.continueResumableUpload(uploadId, await reqBodyToBuffer(req));
const rangeHeader = req.headers["content-range"] ?? "bytes=0-0/*";
const rangeParts = rangeHeader.substring(rangeHeader.indexOf("=")).split("/");
const range = rangeParts[0];
let rangeTotal = "*";
if(rangeParts.length === 2) {
rangeTotal = rangeParts[1];
}
const end = range.split("-")[1];
if(rangeTotal !== "*" && parseInt(end) >= parseInt(rangeTotal) - 1) {
upload = uploadService.finalizeResumableUpload(uploadId);
} else {
if (upload.size > 0) {
res.header("Range", `bytes=0-${upload.size - 1}`);
}
return res.sendStatus(308);
}
src/emulator/storage/apis/gcloud.ts
Outdated
| // see https://cloud.google.com/storage/docs/performing-resumable-uploads#status-check | ||
| if(parseInt(req.headers["content-length"] ?? "0") === 0) { | ||
| if(upload.size === 0 || upload.status === UploadStatus.ACTIVE) { | ||
| return res.header("Range", `bytes=0-${upload.size}`).sendStatus(308); |
There was a problem hiding this comment.
The Range header for a status check response is slightly incorrect and could be improved:
- The range should be
bytes=0-${upload.size - 1}. For example, if 1 byte has been uploaded,upload.sizeis 1, and the range of received bytes is0-0. Your code would produceRange: bytes=0-1. - According to the GCS documentation, if no bytes have been uploaded (
upload.size === 0), theRangeheader should not be included in the response. Your code currently sendsRange: bytes=0-0.
Here is a suggested fix to address both points.
if (upload.size > 0) {
res.header("Range", `bytes=0-${upload.size - 1}`);
}
return res.sendStatus(308);|
Just to clarify in case it's not clear above; I resolved the issues highlighted by Gemini. |
016f9c8 to
9b0b0bc
Compare
|
Approved the workflow run, change LGTM to me! Thanks for the fix! 😃 |
|
I'm unsure what the linter is telling me to do here? |
|
Looks like its just whitespace issues - if you run |
This commit introduces a few fixes: * Checking the status of a resumable upload by sending a PUT with a Content-Length of 0 no longer returns a 400 Bad Request and correctly returns a 308 status code if the upload is incomplete. * Resumable uploads are no longer finalized on the first chunk allowing multiple chunks to be uploaded. * Streaming uploads (uploads of unknown size) should now work correctly
Description
This commit introduces a few fixes for resumable uploads in the storage emulator:
Scenarios Tested
This is a change to the storage emulator. I've tested uploading files in chunks and checking the resulting status codes and responses.
npm testis clean.