Feat(Storage): Enable full object checksum validation on JSON path#9
Feat(Storage): Enable full object checksum validation on JSON path#9thiyaguk09 wants to merge 14 commits into
Conversation
Summary of ChangesHello @thiyaguk09, 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 enhances the storage client's object upload functionality by implementing comprehensive checksum validation. It ensures that both MD5 and CRC32c hashes are calculated for uploaded data and transmitted via the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
There was a problem hiding this comment.
Code Review
This pull request enables full object checksum validation by sending both CRC32c and MD5 hashes in the x-goog-hash header for uploads. The implementation correctly calculates and adds these hashes. My review highlights a significant performance issue where the data stream is read twice, which could be problematic for large files. I've also included suggestions for minor code cleanup in both the main logic and the tests. The added tests are comprehensive and effectively validate the new functionality.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces checksum validation for uploads by adding the X-Goog-Hash header, which is a valuable feature. The changes are implemented across multipart, resumable, and streamable uploaders, with comprehensive tests covering the new functionality. I've identified a couple of areas for improvement: a logic issue with merging custom headers in the multipart uploader, and an inconsistency in the casing of the X-Goog-Hash header. Addressing these points will enhance the robustness and maintainability of the code.
d61822d to
4091fcc
Compare
Refactor Resumable, Streamable, and Multipart uploaders to ensure integrity headers (X-Goog-Hash) are only attached to the request when an upload is being finalized. - In StreamableUploader, introduced `$isFinalRequest` to track intent before writeSize recalculations. - In ResumableUploader, added a boundary check to only attach the hash when the current range matches the total file size. - Aligns with GCS best practices for resumable upload integrity.
4091fcc to
2ac9aad
Compare
|
/gemini summary |
Summary of ChangesThis pull request introduces significant enhancements across two Google Cloud client libraries: Storage and BigQuery. For Google Cloud Storage, it implements comprehensive checksum validation for object uploads, ensuring that MD5 and CRC32c hashes are calculated and transmitted via the 'X-Goog-Hash' header, with careful handling for resumable and streamable uploads to send the header only on the final data chunk. Concurrently, for Google Cloud BigQuery, it integrates the stateless query API, allowing certain queries to bypass traditional job creation for improved efficiency. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
- Refactor Rest.php hash calculation to be more concise using array_filter. - Remove redundant array initializations in Rest.php by utilizing PHP autovivification. - Improve readability of X-Goog-Hash header generation.
Updates the minimum required version of cloud-core to ensure the new checksum validation logic is available in all environments, fixing the lowest-dependencies test failures.
Enhanced Checksum Validation & Header Logic
This PR implements comprehensive MD5 and CRC32c checksum validation for object uploads, ensuring data integrity via the
X-Goog-Hashheader, improving data integrity verification. It refactors the upload architecture to handle hashes dynamically across different upload strategies.Key Technical Changes
1. Core Library Enhancements (
google-cloud-core)ResumableUploader&StreamableUploader: Added type-safe logic(int)($rangeEnd + 1) === (int)$sizeto ensureX-Goog-Hashis transmitted only on the final chunk/request, preventing intermediate validation errors.MultipartUploader: Standardized header merging to ensure hashes calculated by the connection layer are always included in single-shot uploads.restOptionsmerging to ensure custom metadata and encryption headers are preserved alongside checksums.2. Storage Package Improvements (
google-cloud-storage)MD5orCRC32chashes when thevalidateoption is enabled.Bucket::upload()to honor user-provided checksums and prevent redundant re-calculation.BucketTestandRestTestto verify hash behavior in resumable, streamable, and multipart scenarios.Note
CI "Lowest Dependencies" Failure: This failure occurs because the CI environment pulls the tagged version of
google-cloud-corefrom Packagist instead of using the local changes in this PR. This will resolve once the Core changes are merged.