feat(storage): Migrate async client to unified checksum options#16261
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces UploadChecksumValidationOption and DownloadChecksumValidationOption to configure checksum algorithms for uploads and downloads, deprecating the older EnableCrc32cValidationOption and EnableMD5ValidationOption. It updates the internal connection and reader implementations to respect these new options with fallback support, adds corresponding unit and integration tests, and provides a code sample. The reviewer feedback highlights that the fallback logic for download checksum validation is duplicated three times across non-test files and suggests refactoring it into a shared helper function in accordance with the repository style guide.
67305d6 to
8bd3544
Compare
8bd3544 to
1268c7f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16261 +/- ##
==========================================
- Coverage 92.27% 92.27% -0.01%
==========================================
Files 2217 2218 +1
Lines 206628 206654 +26
==========================================
+ Hits 190672 190687 +15
- Misses 15956 15967 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dc5fbb5 to
39c4a25
Compare
There was a problem hiding this comment.
Looks like you'll need to modify some more tests which are using the deprecated options:
Step #2 - "build.sh": /workspace/google/cloud/storage/internal/object_write_streambuf_test.cc:690:22: warning: 'DisableCrc32cChecksum' is deprecated: Use UploadChecksumValidationOption and DownloadChecksumValidationOption instead [-Wdeprecated-declarations]
Step #2 - "build.sh": 690 | request.set_option(DisableCrc32cChecksum(false));
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/hashing_options.h:149:10: note: 'DisableCrc32cChecksum' has been explicitly marked deprecated here
Step #2 - "build.sh": 149 | struct [[deprecated(
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/internal/object_write_streambuf_test.cc:691:22: warning: 'DisableMD5Hash' is deprecated: Use UploadChecksumValidationOption and DownloadChecksumValidationOption instead [-Wdeprecated-declarations]
Step #2 - "build.sh": 691 | request.set_option(DisableMD5Hash(false));
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/hashing_options.h:79:10: note: 'DisableMD5Hash' has been explicitly marked deprecated here
Step #2 - "build.sh": 79 | struct [[deprecated(
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/internal/object_write_streambuf_test.cc:695:49: warning: 'DisableCrc32cChecksum' is deprecated: Use UploadChecksumValidationOption and DownloadChecksumValidationOption instead [-Wdeprecated-declarations]
Step #2 - "build.sh": 695 | CreateHashFunction(Crc32cChecksumValue(), DisableCrc32cChecksum(false),
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/hashing_options.h:149:10: note: 'DisableCrc32cChecksum' has been explicitly marked deprecated here
Step #2 - "build.sh": 149 | struct [[deprecated(
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/internal/object_write_streambuf_test.cc:696:42: warning: 'DisableMD5Hash' is deprecated: Use UploadChecksumValidationOption and DownloadChecksumValidationOption instead [-Wdeprecated-declarations]
Step #2 - "build.sh": 696 | MD5HashValue(), DisableMD5Hash(false)),
Step #2 - "build.sh": | ^
Step #2 - "build.sh": /workspace/google/cloud/storage/hashing_options.h:79:10: note: 'DisableMD5Hash' has been explicitly marked deprecated here
Step #2 - "build.sh": 79 | struct [[deprecated(
…d checksum helper
59e1bf1 to
c0cf77b
Compare
Migrates the Async client (
connection_impl.cc,object_descriptor_impl.cc) to use the newly introduced unified checksum options. Also includes the necessary fallback logic to support users who are still using the deprecated options.Note: This PR is stacked on top of #16260 and includes its commit. It should be merged after #16260.