From b81847ef28d78e313fdf3722e2dcf30893dae1d2 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 09:13:42 +0000 Subject: [PATCH 1/7] Make MD5HashValue and Crc32cChecksumValue compatible with Options By simply adding `using Type = std::string;` to the legacy ComplexOption structs `MD5HashValue` and `Crc32cChecksumValue`, we instantly make them compatible with the modern `google::cloud::Options` API. This enables the exact same type to be used as both a variadic option for the synchronous client (e.g. `InsertObject(..., MD5HashValue("hash"))`) and as a modern option for the asynchronous client (e.g. `options.set("hash")`). This avoids needing any deprecations, any new types, or any duplicate upload/download namespace variants. It is perfectly backward-compatible. --- google/cloud/storage/client.cc | 10 ++++++++-- google/cloud/storage/hashing_options.h | 2 ++ google/cloud/storage/internal/hash_function.cc | 11 +++++++++-- google/cloud/storage/internal/rest/stub.cc | 8 +++++++- google/cloud/storage/options.h | 1 + google/cloud/storage/parallel_upload.cc | 14 ++++++++++++-- 6 files changed, 39 insertions(+), 7 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 9b854bdde37f5..92c6a22980cc3 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -141,8 +141,14 @@ ObjectWriteStream Client::WriteObjectImpl( response->committed_size, std::move(response->metadata), buffer_size, internal::CreateHashFunction(request), internal::HashValues{ - request.GetOption().value_or(""), - request.GetOption().value_or(""), + request.GetOption().value_or( + current.has() + ? current.get() + : ""), + request.GetOption().value_or( + current.has() + ? current.get() + : ""), }, internal::CreateHashValidator(request), request.GetOption().value_or( diff --git a/google/cloud/storage/hashing_options.h b/google/cloud/storage/hashing_options.h index 8b286d6587be0..4b3741f1093f2 100644 --- a/google/cloud/storage/hashing_options.h +++ b/google/cloud/storage/hashing_options.h @@ -41,6 +41,7 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN struct MD5HashValue : public internal::ComplexOption { using ComplexOption::ComplexOption; + using Type = std::string; // GCC <= 7.0 does not use the inherited default constructor, redeclare it // explicitly MD5HashValue() = default; @@ -110,6 +111,7 @@ inline DisableMD5Hash EnableMD5Hash() { return DisableMD5Hash(false); } struct Crc32cChecksumValue : public internal::ComplexOption { using ComplexOption::ComplexOption; + using Type = std::string; // GCC <= 7.0 does not use the inherited default constructor, redeclare it // explicitly Crc32cChecksumValue() = default; diff --git a/google/cloud/storage/internal/hash_function.cc b/google/cloud/storage/internal/hash_function.cc index fdaf3f1782a27..fc134f694943e 100644 --- a/google/cloud/storage/internal/hash_function.cc +++ b/google/cloud/storage/internal/hash_function.cc @@ -32,8 +32,12 @@ std::unique_ptr CreateHashFunction( Crc32cChecksumValue const& crc32c_value, DisableCrc32cChecksum const& crc32c_disabled, MD5HashValue const& md5_value, DisableMD5Hash const& md5_disabled) { + auto const& options = google::cloud::internal::CurrentOptions(); auto crc32c = std::unique_ptr(); - auto crc32c_v = crc32c_value.value_or(""); + auto crc32c_v = crc32c_value.value_or( + options.has() + ? options.get() + : ""); if (!crc32c_v.empty()) { crc32c = std::make_unique( HashValues{/*.crc32c=*/std::move(crc32c_v), /*md5=*/{}}); @@ -42,7 +46,10 @@ std::unique_ptr CreateHashFunction( } auto md5 = std::unique_ptr(); - auto md5_v = md5_value.value_or(""); + auto md5_v = md5_value.value_or( + options.has() + ? options.get() + : ""); if (!md5_v.empty()) { md5 = std::make_unique( HashValues{/*.crc32c=*/{}, /*.md5=*/std::move(md5_v)}); diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index 24ad632f144df..3eba50c168187 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -463,7 +463,9 @@ StatusOr RestStub::InsertObjectMedia( auto const settings = storage::internal::GetUploadChecksumSettings(request, options); if (!settings.md5 || !settings.crc32c || request.HasOption() || - request.HasOption()) { + request.HasOption() || + options.has() || + options.has()) { return InsertObjectMediaMultipart(context, options, request); } @@ -713,9 +715,13 @@ StatusOr RestStub::CreateResumableUpload( } if (request.HasOption()) { resource["crc32c"] = request.GetOption().value(); + } else if (options.has()) { + resource["crc32c"] = options.get(); } if (request.HasOption()) { resource["md5Hash"] = request.GetOption().value(); + } else if (options.has()) { + resource["md5Hash"] = options.get(); } if (resource.empty()) { diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index ea687c9d090b8..88672b8b9e4fe 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_OPTIONS_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_OPTIONS_H +#include "google/cloud/storage/hashing_options.h" #include "google/cloud/storage/idempotency_policy.h" #include "google/cloud/storage/retry_policy.h" #include "google/cloud/storage/version.h" diff --git a/google/cloud/storage/parallel_upload.cc b/google/cloud/storage/parallel_upload.cc index 7ab199f77472b..de13776676f2f 100644 --- a/google/cloud/storage/parallel_upload.cc +++ b/google/cloud/storage/parallel_upload.cc @@ -44,8 +44,18 @@ class ParallelObjectWriteStreambuf : public ObjectWriteStreambuf { committed_size, std::move(metadata), max_buffer_size, CreateHashFunction(request), internal::HashValues{ - request.GetOption().value_or(""), - request.GetOption().value_or(""), + request.GetOption().value_or( + google::cloud::internal::CurrentOptions() + .has() + ? google::cloud::internal::CurrentOptions() + .get() + : ""), + request.GetOption().value_or( + google::cloud::internal::CurrentOptions() + .has() + ? google::cloud::internal::CurrentOptions() + .get() + : ""), }, CreateHashValidator(request), AutoFinalizeConfig::kEnabled), state_(std::move(state)), From 607d7d8ea3d594362467dc302b3c17525f6d618e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 09:45:37 +0000 Subject: [PATCH 2/7] feat(storage): Add PrecomputedChecksumsOption Introduces a map-based option that allows users to provide both CRC32C and MD5 hashes simultaneously for an upload or download. --- .../cloud/storage/internal/hash_function.cc | 26 +++++++++++++------ google/cloud/storage/internal/rest/stub.cc | 8 ++++++ google/cloud/storage/options.h | 16 ++++++++++++ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/internal/hash_function.cc b/google/cloud/storage/internal/hash_function.cc index fc134f694943e..57bb1c8cec40f 100644 --- a/google/cloud/storage/internal/hash_function.cc +++ b/google/cloud/storage/internal/hash_function.cc @@ -34,10 +34,15 @@ std::unique_ptr CreateHashFunction( DisableMD5Hash const& md5_disabled) { auto const& options = google::cloud::internal::CurrentOptions(); auto crc32c = std::unique_ptr(); - auto crc32c_v = crc32c_value.value_or( - options.has() - ? options.get() - : ""); + auto crc32c_v = crc32c_value.value_or(""); + if (crc32c_v.empty() && options.has()) { + crc32c_v = options.get(); + } + if (crc32c_v.empty() && options.has()) { + auto m = options.get(); + auto it = m.find(ChecksumAlgorithm::kCrc32c); + if (it != m.end()) crc32c_v = it->second; + } if (!crc32c_v.empty()) { crc32c = std::make_unique( HashValues{/*.crc32c=*/std::move(crc32c_v), /*md5=*/{}}); @@ -46,10 +51,15 @@ std::unique_ptr CreateHashFunction( } auto md5 = std::unique_ptr(); - auto md5_v = md5_value.value_or( - options.has() - ? options.get() - : ""); + auto md5_v = md5_value.value_or(""); + if (md5_v.empty() && options.has()) { + md5_v = options.get(); + } + if (md5_v.empty() && options.has()) { + auto m = options.get(); + auto it = m.find(ChecksumAlgorithm::kMD5); + if (it != m.end()) md5_v = it->second; + } if (!md5_v.empty()) { md5 = std::make_unique( HashValues{/*.crc32c=*/{}, /*.md5=*/std::move(md5_v)}); diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index 3eba50c168187..68795df4412b3 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -717,11 +717,19 @@ StatusOr RestStub::CreateResumableUpload( resource["crc32c"] = request.GetOption().value(); } else if (options.has()) { resource["crc32c"] = options.get(); + } else if (options.has()) { + auto m = options.get(); + auto it = m.find(ChecksumAlgorithm::kCrc32c); + if (it != m.end()) resource["crc32c"] = it->second; } if (request.HasOption()) { resource["md5Hash"] = request.GetOption().value(); } else if (options.has()) { resource["md5Hash"] = options.get(); + } else if (options.has()) { + auto m = options.get(); + auto it = m.find(ChecksumAlgorithm::kMD5); + if (it != m.end()) resource["md5Hash"] = it->second; } if (resource.empty()) { diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index 88672b8b9e4fe..6d66fdb5039eb 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -25,6 +25,7 @@ #include "google/cloud/options.h" #include #include +#include #include #include @@ -122,6 +123,20 @@ struct DownloadChecksumValidationOption { using Type = ChecksumAlgorithm; }; +/** + * Provide precomputed hashes for uploads and downloads. + * + * If set, the client will use these precomputed hashes instead of computing + * them locally. This is useful when the application has already computed the + * hash and wants to avoid recomputing it. + * + * @ingroup storage-options + */ +struct PrecomputedChecksumsOption { + using Type = std::map; +}; + + /** * Configure the REST endpoint for the GCS client library. * @@ -378,6 +393,7 @@ using ClientOptionList = ::google::cloud::OptionList< TransferStallTimeoutOption, RetryPolicyOption, BackoffPolicyOption, IdempotencyPolicyOption, CARootsFilePathOption, UploadChecksumValidationOption, DownloadChecksumValidationOption, + PrecomputedChecksumsOption, storage_experimental::HttpVersionOption, storage_experimental::OTelSpanEnrichmentOption>; From e72622a94fdae3eceaf5a68722bacea79192ad88 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 09:49:51 +0000 Subject: [PATCH 3/7] refactor(storage): use IIFE in parallel upload initializer list --- google/cloud/storage/parallel_upload.cc | 36 +++++++++++++++---------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/google/cloud/storage/parallel_upload.cc b/google/cloud/storage/parallel_upload.cc index de13776676f2f..2be141592e5c4 100644 --- a/google/cloud/storage/parallel_upload.cc +++ b/google/cloud/storage/parallel_upload.cc @@ -43,20 +43,28 @@ class ParallelObjectWriteStreambuf : public ObjectWriteStreambuf { std::move(connection), request, std::move(upload_id), committed_size, std::move(metadata), max_buffer_size, CreateHashFunction(request), - internal::HashValues{ - request.GetOption().value_or( - google::cloud::internal::CurrentOptions() - .has() - ? google::cloud::internal::CurrentOptions() - .get() - : ""), - request.GetOption().value_or( - google::cloud::internal::CurrentOptions() - .has() - ? google::cloud::internal::CurrentOptions() - .get() - : ""), - }, + [&request]() { + auto const& current = google::cloud::internal::CurrentOptions(); + auto crc = request.GetOption().value_or(""); + if (crc.empty() && current.has()) { + crc = current.get(); + } + if (crc.empty() && current.has()) { + auto m = current.get(); + auto it = m.find(ChecksumAlgorithm::kCrc32c); + if (it != m.end()) crc = it->second; + } + auto md5 = request.GetOption().value_or(""); + if (md5.empty() && current.has()) { + md5 = current.get(); + } + if (md5.empty() && current.has()) { + auto m = current.get(); + auto it = m.find(ChecksumAlgorithm::kMD5); + if (it != m.end()) md5 = it->second; + } + return internal::HashValues{std::move(crc), std::move(md5)}; + }(), CreateHashValidator(request), AutoFinalizeConfig::kEnabled), state_(std::move(state)), stream_idx_(stream_idx) {} From f33924a281b64deab75f64a1e14489436a657a2f Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 10:02:24 +0000 Subject: [PATCH 4/7] Revert Crc32cChecksumValue and MD5HashValue from Options --- google/cloud/storage/client.cc | 25 +++++++++++-------- google/cloud/storage/hashing_options.h | 2 -- .../cloud/storage/internal/hash_function.cc | 6 ----- google/cloud/storage/internal/rest/stub.cc | 4 --- google/cloud/storage/parallel_upload.cc | 6 ----- 5 files changed, 15 insertions(+), 28 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 92c6a22980cc3..0ecc6cbf0c276 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -140,16 +140,21 @@ ObjectWriteStream Client::WriteObjectImpl( connection_, request, std::move(response->upload_id), response->committed_size, std::move(response->metadata), buffer_size, internal::CreateHashFunction(request), - internal::HashValues{ - request.GetOption().value_or( - current.has() - ? current.get() - : ""), - request.GetOption().value_or( - current.has() - ? current.get() - : ""), - }, + [&]() { + auto crc = request.GetOption().value_or(""); + if (crc.empty() && current.has()) { + auto m = current.get(); + auto it = m.find(ChecksumAlgorithm::kCrc32c); + if (it != m.end()) crc = it->second; + } + auto md5 = request.GetOption().value_or(""); + if (md5.empty() && current.has()) { + auto m = current.get(); + auto it = m.find(ChecksumAlgorithm::kMD5); + if (it != m.end()) md5 = it->second; + } + return internal::HashValues{std::move(crc), std::move(md5)}; + }(), internal::CreateHashValidator(request), request.GetOption().value_or( AutoFinalizeConfig::kEnabled))); diff --git a/google/cloud/storage/hashing_options.h b/google/cloud/storage/hashing_options.h index 4b3741f1093f2..8b286d6587be0 100644 --- a/google/cloud/storage/hashing_options.h +++ b/google/cloud/storage/hashing_options.h @@ -41,7 +41,6 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN struct MD5HashValue : public internal::ComplexOption { using ComplexOption::ComplexOption; - using Type = std::string; // GCC <= 7.0 does not use the inherited default constructor, redeclare it // explicitly MD5HashValue() = default; @@ -111,7 +110,6 @@ inline DisableMD5Hash EnableMD5Hash() { return DisableMD5Hash(false); } struct Crc32cChecksumValue : public internal::ComplexOption { using ComplexOption::ComplexOption; - using Type = std::string; // GCC <= 7.0 does not use the inherited default constructor, redeclare it // explicitly Crc32cChecksumValue() = default; diff --git a/google/cloud/storage/internal/hash_function.cc b/google/cloud/storage/internal/hash_function.cc index 57bb1c8cec40f..b2870dbdca816 100644 --- a/google/cloud/storage/internal/hash_function.cc +++ b/google/cloud/storage/internal/hash_function.cc @@ -35,9 +35,6 @@ std::unique_ptr CreateHashFunction( auto const& options = google::cloud::internal::CurrentOptions(); auto crc32c = std::unique_ptr(); auto crc32c_v = crc32c_value.value_or(""); - if (crc32c_v.empty() && options.has()) { - crc32c_v = options.get(); - } if (crc32c_v.empty() && options.has()) { auto m = options.get(); auto it = m.find(ChecksumAlgorithm::kCrc32c); @@ -52,9 +49,6 @@ std::unique_ptr CreateHashFunction( auto md5 = std::unique_ptr(); auto md5_v = md5_value.value_or(""); - if (md5_v.empty() && options.has()) { - md5_v = options.get(); - } if (md5_v.empty() && options.has()) { auto m = options.get(); auto it = m.find(ChecksumAlgorithm::kMD5); diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index 68795df4412b3..ad01ba6495864 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -715,8 +715,6 @@ StatusOr RestStub::CreateResumableUpload( } if (request.HasOption()) { resource["crc32c"] = request.GetOption().value(); - } else if (options.has()) { - resource["crc32c"] = options.get(); } else if (options.has()) { auto m = options.get(); auto it = m.find(ChecksumAlgorithm::kCrc32c); @@ -724,8 +722,6 @@ StatusOr RestStub::CreateResumableUpload( } if (request.HasOption()) { resource["md5Hash"] = request.GetOption().value(); - } else if (options.has()) { - resource["md5Hash"] = options.get(); } else if (options.has()) { auto m = options.get(); auto it = m.find(ChecksumAlgorithm::kMD5); diff --git a/google/cloud/storage/parallel_upload.cc b/google/cloud/storage/parallel_upload.cc index 2be141592e5c4..064bc72631526 100644 --- a/google/cloud/storage/parallel_upload.cc +++ b/google/cloud/storage/parallel_upload.cc @@ -46,18 +46,12 @@ class ParallelObjectWriteStreambuf : public ObjectWriteStreambuf { [&request]() { auto const& current = google::cloud::internal::CurrentOptions(); auto crc = request.GetOption().value_or(""); - if (crc.empty() && current.has()) { - crc = current.get(); - } if (crc.empty() && current.has()) { auto m = current.get(); auto it = m.find(ChecksumAlgorithm::kCrc32c); if (it != m.end()) crc = it->second; } auto md5 = request.GetOption().value_or(""); - if (md5.empty() && current.has()) { - md5 = current.get(); - } if (md5.empty() && current.has()) { auto m = current.get(); auto it = m.find(ChecksumAlgorithm::kMD5); From 0512aaad56dd618ba945b33f25a1245cfb64c153 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 10:16:09 +0000 Subject: [PATCH 5/7] Fix CI checker format errors and missed MD5 check in InsertObjectMedia --- google/cloud/storage/internal/rest/stub.cc | 3 +-- google/cloud/storage/options.h | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index ad01ba6495864..be8eecf02e759 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -464,8 +464,7 @@ StatusOr RestStub::InsertObjectMedia( storage::internal::GetUploadChecksumSettings(request, options); if (!settings.md5 || !settings.crc32c || request.HasOption() || request.HasOption() || - options.has() || - options.has()) { + options.has()) { return InsertObjectMediaMultipart(context, options, request); } diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index 6d66fdb5039eb..a44c5cd3def9e 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -136,7 +136,6 @@ struct PrecomputedChecksumsOption { using Type = std::map; }; - /** * Configure the REST endpoint for the GCS client library. * @@ -393,8 +392,7 @@ using ClientOptionList = ::google::cloud::OptionList< TransferStallTimeoutOption, RetryPolicyOption, BackoffPolicyOption, IdempotencyPolicyOption, CARootsFilePathOption, UploadChecksumValidationOption, DownloadChecksumValidationOption, - PrecomputedChecksumsOption, - storage_experimental::HttpVersionOption, + PrecomputedChecksumsOption, storage_experimental::HttpVersionOption, storage_experimental::OTelSpanEnrichmentOption>; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END From a0361284407542da7ecbdc0fbb004568fa3c9305 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 12:34:12 +0000 Subject: [PATCH 6/7] fix(storage): refactor PrecomputedChecksumsOption to use a flat struct for performance Refactored PrecomputedChecksumsOption to use a flat PrecomputedChecksums struct containing crc32c and md5 fields rather than allocating a std::map dynamically on the hot path for every object insertion. Added unit tests to internal/hash_function_impl_test.cc to verify its behavior and precedence against explicitly provided variadic checksum options. --- google/cloud/storage/client.cc | 8 +--- google/cloud/storage/hashing_options.h | 11 +++++ .../cloud/storage/internal/hash_function.cc | 8 +--- .../internal/hash_function_impl_test.cc | 41 ++++++++++++++++++- google/cloud/storage/internal/rest/stub.cc | 14 +++---- google/cloud/storage/options.h | 2 +- google/cloud/storage/parallel_upload.cc | 8 +--- 7 files changed, 64 insertions(+), 28 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 0ecc6cbf0c276..cf056ff6d2990 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -143,15 +143,11 @@ ObjectWriteStream Client::WriteObjectImpl( [&]() { auto crc = request.GetOption().value_or(""); if (crc.empty() && current.has()) { - auto m = current.get(); - auto it = m.find(ChecksumAlgorithm::kCrc32c); - if (it != m.end()) crc = it->second; + crc = current.get().crc32c; } auto md5 = request.GetOption().value_or(""); if (md5.empty() && current.has()) { - auto m = current.get(); - auto it = m.find(ChecksumAlgorithm::kMD5); - if (it != m.end()) md5 = it->second; + md5 = current.get().md5; } return internal::HashValues{std::move(crc), std::move(md5)}; }(), diff --git a/google/cloud/storage/hashing_options.h b/google/cloud/storage/hashing_options.h index 8b286d6587be0..9373d6575ca8f 100644 --- a/google/cloud/storage/hashing_options.h +++ b/google/cloud/storage/hashing_options.h @@ -38,6 +38,17 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN * @see * https://sigops.org/s/conferences/hotos/2021/papers/hotos21-s01-hochschild.pdf */ + +/** + * A structure to hold precomputed hashes. + * + * @ingroup storage-options + */ +struct PrecomputedChecksums { + std::string crc32c; + std::string md5; +}; + struct MD5HashValue : public internal::ComplexOption { using ComplexOption::ComplexOption; diff --git a/google/cloud/storage/internal/hash_function.cc b/google/cloud/storage/internal/hash_function.cc index b2870dbdca816..7938571b69048 100644 --- a/google/cloud/storage/internal/hash_function.cc +++ b/google/cloud/storage/internal/hash_function.cc @@ -36,9 +36,7 @@ std::unique_ptr CreateHashFunction( auto crc32c = std::unique_ptr(); auto crc32c_v = crc32c_value.value_or(""); if (crc32c_v.empty() && options.has()) { - auto m = options.get(); - auto it = m.find(ChecksumAlgorithm::kCrc32c); - if (it != m.end()) crc32c_v = it->second; + crc32c_v = options.get().crc32c; } if (!crc32c_v.empty()) { crc32c = std::make_unique( @@ -50,9 +48,7 @@ std::unique_ptr CreateHashFunction( auto md5 = std::unique_ptr(); auto md5_v = md5_value.value_or(""); if (md5_v.empty() && options.has()) { - auto m = options.get(); - auto it = m.find(ChecksumAlgorithm::kMD5); - if (it != m.end()) md5_v = it->second; + md5_v = options.get().md5; } if (!md5_v.empty()) { md5 = std::make_unique( diff --git a/google/cloud/storage/internal/hash_function_impl_test.cc b/google/cloud/storage/internal/hash_function_impl_test.cc index 0d4949019be27..00ba439513f93 100644 --- a/google/cloud/storage/internal/hash_function_impl_test.cc +++ b/google/cloud/storage/internal/hash_function_impl_test.cc @@ -18,6 +18,7 @@ #include "google/cloud/storage/internal/object_requests.h" #include "google/cloud/storage/testing/mock_hash_function.h" #include "google/cloud/storage/testing/upload_hash_cases.h" +#include "google/cloud/options.h" #include "google/cloud/testing_util/status_matchers.h" #include #include @@ -450,10 +451,48 @@ TEST(HashFunctionImplTest, CreateHashFunctionInsertObjectMedia) { } } +TEST(HashFunctionImplTest, CreateHashFunctionPrecomputedChecksumsOption) { + google::cloud::internal::OptionsSpan span( + google::cloud::Options{}.set( + PrecomputedChecksums{"crc-from-options", "md5-from-options"})); + + ResumableUploadRequest request("bucket", "object"); + auto function = CreateHashFunction(request); + EXPECT_EQ(function->Finish().crc32c, "crc-from-options"); + EXPECT_EQ(function->Finish().md5, "md5-from-options"); +} + +TEST(HashFunctionImplTest, + CreateHashFunctionPrecomputedChecksumsOptionPartial) { + google::cloud::internal::OptionsSpan span( + google::cloud::Options{}.set( + PrecomputedChecksums{"crc-from-options", ""})); + + ResumableUploadRequest request("bucket", "object"); + auto function = CreateHashFunction(request); + EXPECT_EQ(function->Finish().crc32c, "crc-from-options"); + // MD5 is disabled by default for uploads, so it will be empty + EXPECT_EQ(function->Finish().md5, ""); +} + +TEST(HashFunctionImplTest, CreateHashFunctionPrecedence) { + google::cloud::internal::OptionsSpan span( + google::cloud::Options{}.set( + PrecomputedChecksums{"crc-from-options", "md5-from-options"})); + + ResumableUploadRequest request("bucket", "object"); + request.set_multiple_options(Crc32cChecksumValue("crc-from-request"), + MD5HashValue("md5-from-request")); + auto function = CreateHashFunction(request); + // The variadic options provided directly to the request should take + // precedence + EXPECT_EQ(function->Finish().crc32c, "crc-from-request"); + EXPECT_EQ(function->Finish().md5, "md5-from-request"); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage } // namespace cloud } // namespace google -#include "google/cloud/internal/diagnostics_pop.inc" diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index be8eecf02e759..563981490efdc 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -714,17 +714,15 @@ StatusOr RestStub::CreateResumableUpload( } if (request.HasOption()) { resource["crc32c"] = request.GetOption().value(); - } else if (options.has()) { - auto m = options.get(); - auto it = m.find(ChecksumAlgorithm::kCrc32c); - if (it != m.end()) resource["crc32c"] = it->second; + } else if (options.has() && + !options.get().crc32c.empty()) { + resource["crc32c"] = options.get().crc32c; } if (request.HasOption()) { resource["md5Hash"] = request.GetOption().value(); - } else if (options.has()) { - auto m = options.get(); - auto it = m.find(ChecksumAlgorithm::kMD5); - if (it != m.end()) resource["md5Hash"] = it->second; + } else if (options.has() && + !options.get().md5.empty()) { + resource["md5Hash"] = options.get().md5; } if (resource.empty()) { diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index a44c5cd3def9e..56651f99b19c3 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -133,7 +133,7 @@ struct DownloadChecksumValidationOption { * @ingroup storage-options */ struct PrecomputedChecksumsOption { - using Type = std::map; + using Type = PrecomputedChecksums; }; /** diff --git a/google/cloud/storage/parallel_upload.cc b/google/cloud/storage/parallel_upload.cc index 064bc72631526..f6dcc0e5391f0 100644 --- a/google/cloud/storage/parallel_upload.cc +++ b/google/cloud/storage/parallel_upload.cc @@ -47,15 +47,11 @@ class ParallelObjectWriteStreambuf : public ObjectWriteStreambuf { auto const& current = google::cloud::internal::CurrentOptions(); auto crc = request.GetOption().value_or(""); if (crc.empty() && current.has()) { - auto m = current.get(); - auto it = m.find(ChecksumAlgorithm::kCrc32c); - if (it != m.end()) crc = it->second; + crc = current.get().crc32c; } auto md5 = request.GetOption().value_or(""); if (md5.empty() && current.has()) { - auto m = current.get(); - auto it = m.find(ChecksumAlgorithm::kMD5); - if (it != m.end()) md5 = it->second; + md5 = current.get().md5; } return internal::HashValues{std::move(crc), std::move(md5)}; }(), From 1eed8f2c3c18ff88712ce4f0ef12cf325907579e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 30 Jul 2026 13:17:33 +0000 Subject: [PATCH 7/7] refactor(storage): address PR review comments - Removed unused include from options.h - Extracted PrecomputedChecksumsOption lookups into local variables to avoid redundant get() calls in parallel_upload.cc, client.cc, hash_function.cc, and rest/stub.cc. --- google/cloud/storage/client.cc | 10 +++++----- google/cloud/storage/internal/hash_function.cc | 16 ++++++++-------- google/cloud/storage/internal/rest/stub.cc | 17 +++++++++++------ google/cloud/storage/options.h | 1 - google/cloud/storage/parallel_upload.cc | 11 ++++++----- 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index cf056ff6d2990..c12d8d623b5f1 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -142,12 +142,12 @@ ObjectWriteStream Client::WriteObjectImpl( internal::CreateHashFunction(request), [&]() { auto crc = request.GetOption().value_or(""); - if (crc.empty() && current.has()) { - crc = current.get().crc32c; - } auto md5 = request.GetOption().value_or(""); - if (md5.empty() && current.has()) { - md5 = current.get().md5; + if ((crc.empty() || md5.empty()) && + current.has()) { + auto const& checksums = current.get(); + if (crc.empty()) crc = checksums.crc32c; + if (md5.empty()) md5 = checksums.md5; } return internal::HashValues{std::move(crc), std::move(md5)}; }(), diff --git a/google/cloud/storage/internal/hash_function.cc b/google/cloud/storage/internal/hash_function.cc index 7938571b69048..c7bc081e98270 100644 --- a/google/cloud/storage/internal/hash_function.cc +++ b/google/cloud/storage/internal/hash_function.cc @@ -35,8 +35,14 @@ std::unique_ptr CreateHashFunction( auto const& options = google::cloud::internal::CurrentOptions(); auto crc32c = std::unique_ptr(); auto crc32c_v = crc32c_value.value_or(""); - if (crc32c_v.empty() && options.has()) { - crc32c_v = options.get().crc32c; + auto md5 = std::unique_ptr(); + auto md5_v = md5_value.value_or(""); + + if ((crc32c_v.empty() || md5_v.empty()) && + options.has()) { + auto const& checksums = options.get(); + if (crc32c_v.empty()) crc32c_v = checksums.crc32c; + if (md5_v.empty()) md5_v = checksums.md5; } if (!crc32c_v.empty()) { crc32c = std::make_unique( @@ -44,12 +50,6 @@ std::unique_ptr CreateHashFunction( } else if (!crc32c_disabled.value_or(false)) { crc32c = std::make_unique(); } - - auto md5 = std::unique_ptr(); - auto md5_v = md5_value.value_or(""); - if (md5_v.empty() && options.has()) { - md5_v = options.get().md5; - } if (!md5_v.empty()) { md5 = std::make_unique( HashValues{/*.crc32c=*/{}, /*.md5=*/std::move(md5_v)}); diff --git a/google/cloud/storage/internal/rest/stub.cc b/google/cloud/storage/internal/rest/stub.cc index 563981490efdc..16323ca3d959d 100644 --- a/google/cloud/storage/internal/rest/stub.cc +++ b/google/cloud/storage/internal/rest/stub.cc @@ -714,15 +714,20 @@ StatusOr RestStub::CreateResumableUpload( } if (request.HasOption()) { resource["crc32c"] = request.GetOption().value(); - } else if (options.has() && - !options.get().crc32c.empty()) { - resource["crc32c"] = options.get().crc32c; } if (request.HasOption()) { resource["md5Hash"] = request.GetOption().value(); - } else if (options.has() && - !options.get().md5.empty()) { - resource["md5Hash"] = options.get().md5; + } + + if (options.has()) { + auto const& checksums = options.get(); + if (!request.HasOption() && + !checksums.crc32c.empty()) { + resource["crc32c"] = checksums.crc32c; + } + if (!request.HasOption() && !checksums.md5.empty()) { + resource["md5Hash"] = checksums.md5; + } } if (resource.empty()) { diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index 56651f99b19c3..2f76f7a215738 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -25,7 +25,6 @@ #include "google/cloud/options.h" #include #include -#include #include #include diff --git a/google/cloud/storage/parallel_upload.cc b/google/cloud/storage/parallel_upload.cc index f6dcc0e5391f0..702f3193c34c4 100644 --- a/google/cloud/storage/parallel_upload.cc +++ b/google/cloud/storage/parallel_upload.cc @@ -46,12 +46,13 @@ class ParallelObjectWriteStreambuf : public ObjectWriteStreambuf { [&request]() { auto const& current = google::cloud::internal::CurrentOptions(); auto crc = request.GetOption().value_or(""); - if (crc.empty() && current.has()) { - crc = current.get().crc32c; - } auto md5 = request.GetOption().value_or(""); - if (md5.empty() && current.has()) { - md5 = current.get().md5; + if ((crc.empty() || md5.empty()) && + current.has()) { + auto const& checksums = + current.get(); + if (crc.empty()) crc = checksums.crc32c; + if (md5.empty()) md5 = checksums.md5; } return internal::HashValues{std::move(crc), std::move(md5)}; }(),