Setting default checksum new1 #50
Conversation
Clarify the default checksum behavior in documentation.
Summary of ChangesHello @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 introduces a significant enhancement to the Google Cloud Storage Ruby client by establishing CRC32c as the default checksum for file uploads. This change ensures that data integrity is automatically verified for uploads where a checksum isn't explicitly specified, thereby improving the reliability of data handling. Additionally, the underlying digest calculation methods have been streamlined through refactoring, leading to a more robust and maintainable codebase. The new behavior is thoroughly covered by an expanded suite of unit tests. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the change to use crc32c as the default checksum algorithm when uploading files. The refactoring in file/verifier.rb to remove duplicate code is a good improvement. The new and updated tests cover the changes well.
However, there are critical issues in the test helper methods (empty_file_gapi and set_crc32c_as_default) that need to be addressed. The helpers are modified to pass an invalid checksum parameter to a GAPI object constructor, which will cause tests to fail. Additionally, the new set_crc32c_as_default helper uses a hardcoded value, making the tests brittle. I've left detailed comments with suggestions on how to fix these issues.
I also noticed that the empty_file_gapi helper is duplicated across multiple test files. It would be a good opportunity to refactor this into a shared helper in test/helper.rb to improve maintainability.
google-cloud-storage/test/google/cloud/storage/bucket_encryption_test.rb
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request changes the default checksum behavior to use crc32c when no other checksum option is specified. The implementation looks correct, and the refactoring in file/verifier.rb to reduce code duplication is a nice improvement. The added tests provide good coverage for the new functionality. My main feedback is regarding a new test helper, set_crc32c_as_default, which is brittle due to a hardcoded value. I've suggested removing this helper and the logic that uses it to make the test suite more robust.
google-cloud-storage/test/google/cloud/storage/bucket_encryption_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-storage/test/google/cloud/storage/lazy/bucket_test.rb
Outdated
Show resolved
Hide resolved
shubhangi-google
requested review from
gurusai-voleti,
mahendra-google and
thiyaguk09
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements the change to make crc32c the default checksum for file uploads. The refactoring in verifier.rb to consolidate digest calculation logic is a good improvement for maintainability. However, I've identified a recurring bug in the new test cases that use StringIO, where the stream is consumed before being used, which could lead to tests passing incorrectly. I've also provided suggestions to make some of the other new tests more robust by avoiding hardcoded values and instead relying on the logic they are intended to test. Please see my detailed comments for suggestions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the default checksum behavior to use crc32c for file creation when no other checksum option is specified. The implementation is sound and includes a nice refactoring in file/verifier.rb to reduce code duplication. The accompanying tests are mostly well-updated, but I've identified a couple of instances where hardcoded checksum values in tests could lead to maintenance issues. I've provided suggestions to calculate these values dynamically. Overall, these are solid changes.
thiyaguk09
left a comment
thiyaguk09
left a comment
There was a problem hiding this comment.
Please update docs and commented messages for more user clarity
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request changes the default behavior for file uploads to use crc32c checksum for data integrity verification. The implementation correctly sets the default and also introduces boolean flags to enable/disable it. The code is also refactored to reduce duplication in checksum calculation logic.
My main concern is with the test helper logic, which will likely cause existing tests for empty file uploads to fail. I've left a detailed comment on this issue. Overall, the direction of the change is good, but the test suite updates need to be handled more carefully to avoid breaking existing functionality.
| event_based_hold: nil, checksum: nil | ||
|
|
||
| # Set crc32c if both md5 and crc32c are not provided | ||
| crc32c ||= set_crc32c_as_default md5, crc32c, checksum |
There was a problem hiding this comment.
This change, while intended to support default checksums in tests, will likely cause existing tests for empty file creation (e.g., it "creates an empty file") to fail. When empty_file_gapi is called with no arguments, set_crc32c_as_default is invoked and incorrectly adds a crc32c expectation to the mock. The production code doesn't send a checksum for empty files, so the mock verification will fail.
This logic needs to be more nuanced to avoid applying a default checksum for mocks of empty file uploads. You might need to modify empty_file_gapi to differentiate between uploads with and without content.
|
PR created for Google main- googleapis#32411 |
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:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>