feat(storage): migrate sync client to unified checksum options#16264
feat(storage): migrate sync client to unified checksum options#16264v-pratap wants to merge 9 commits into
Conversation
product-auto-label
Bot
added
the
api: storage
![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 introduces UploadChecksumValidationOption and DownloadChecksumValidationOption using a new ChecksumAlgorithm enum to replace several deprecated checksum options and classes. It updates internal hash functions, validators, and tests to support these new options. The review feedback highlights that using the deprecated types in hash_function.cc, hash_validator.cc, and object_requests.cc will trigger -Wdeprecated-declarations compiler warnings, suggesting the inclusion of deprecation warning disable headers. Additionally, the reviewer recommends refactoring duplicated fallback logic for resolving checksum settings into a shared helper function to align with the repository style guide.
| #include "google/cloud/options.h" | ||
| #include "google/cloud/storage/options.h" |
There was a problem hiding this comment.
Using the deprecated types DisableCrc32cChecksum and DisableMD5Hash in this file will trigger -Wdeprecated-declarations compiler warnings (which are treated as errors in builds with -Werror). Although hash_function.h disables deprecation warnings, it pops the diagnostics at the end of the header, meaning the warnings are re-enabled when compiling this .cc file. Please include disable_deprecation_warnings.inc at the top of this file and remember to add #include "google/cloud/internal/diagnostics_pop.inc" at the very end of the file.
#include "google/cloud/options.h"
#include "google/cloud/storage/options.h"
#include "google/cloud/internal/disable_deprecation_warnings.inc"| #include "google/cloud/options.h" | ||
| #include "google/cloud/storage/options.h" |
There was a problem hiding this comment.
Using the deprecated types DisableCrc32cChecksum and DisableMD5Hash in this file will trigger -Wdeprecated-declarations compiler warnings (which are treated as errors in builds with -Werror). Please include disable_deprecation_warnings.inc at the top of this file and remember to add #include "google/cloud/internal/diagnostics_pop.inc" at the very end of the file.
#include "google/cloud/options.h"
#include "google/cloud/storage/options.h"
#include "google/cloud/internal/disable_deprecation_warnings.inc"| #include "google/cloud/storage/options.h" | ||
| #include "google/cloud/options.h" |
There was a problem hiding this comment.
Using the deprecated types DisableCrc32cChecksum and DisableMD5Hash in this file will trigger -Wdeprecated-declarations compiler warnings (which are treated as errors in builds with -Werror). Although object_requests.h disables deprecation warnings, it pops the diagnostics at the end of the header, meaning the warnings are re-enabled when compiling this .cc file. Please include disable_deprecation_warnings.inc at the top of this file and remember to add #include "google/cloud/internal/diagnostics_pop.inc" at the very end of the file.
#include "google/cloud/storage/options.h"
#include "google/cloud/options.h"
#include "google/cloud/internal/disable_deprecation_warnings.inc"| bool disable_md5 = false; | ||
| bool disable_crc32c = false; | ||
| auto const& options = google::cloud::internal::CurrentOptions(); | ||
| if (options.has<DownloadChecksumValidationOption>()) { | ||
| auto const algo = options.get<DownloadChecksumValidationOption>(); | ||
| disable_md5 = (algo != ChecksumAlgorithm::kMD5); | ||
| disable_crc32c = (algo != ChecksumAlgorithm::kCrc32c); | ||
| } else { | ||
| disable_md5 = request.GetOption<DisableMD5Hash>().value_or(false); | ||
| disable_crc32c = request.GetOption<DisableCrc32cChecksum>().value_or(false); | ||
| } |
There was a problem hiding this comment.
This fallback logic for resolving download checksum settings is duplicated across multiple files (e.g., in hash_validator.cc for both download and upload requests). Per the repository style guide, we should prefer to factor out duplicated code if it appears 3 or more times in non-test files. Consider defining a shared helper function (e.g., GetDownloadChecksumSettings and GetUploadChecksumSettings overloads that accept a request and options) in a common internal header like google/cloud/storage/internal/hash_function.h to centralize this logic and reduce duplication.
References
- Prefer to factor out duplicated code if it appears 3 or more times in non-test files. (link)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16264 +/- ##
==========================================
- Coverage 92.27% 92.26% -0.01%
==========================================
Files 2214 2215 +1
Lines 206505 206582 +77
==========================================
+ Hits 190556 190609 +53
- Misses 15949 15973 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Migrates the Sync client 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 #16261 and includes its commits. It should be merged after #16261.