From 6c92722111757f48370d89c9615306da57c24522 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 08:36:40 +0000 Subject: [PATCH 1/8] feat(storage): introduce unified checksum options and deprecate old options --- google/cloud/storage/async/options.h | 14 ++++++- google/cloud/storage/hashing_options.h | 18 ++++++++- google/cloud/storage/hashing_options_test.cc | 19 ++++++++++ google/cloud/storage/internal/hash_function.h | 2 + .../cloud/storage/internal/object_requests.h | 2 + google/cloud/storage/options.h | 38 +++++++++++++++++++ google/cloud/storage/parallel_upload.h | 2 + 7 files changed, 91 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/async/options.h b/google/cloud/storage/async/options.h index d979730dbac24..2637be6bab192 100644 --- a/google/cloud/storage/async/options.h +++ b/google/cloud/storage/async/options.h @@ -29,8 +29,13 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN * checksum of an object during uploads and downloads. * * The option has no effect if the service does not return a CRC32C hash. + * + * @deprecated Use `UploadChecksumValidationOption` and + * `DownloadChecksumValidationOption` instead. */ -struct EnableCrc32cValidationOption { +struct [[deprecated( + "Use UploadChecksumValidationOption and DownloadChecksumValidationOption " + "instead")]] EnableCrc32cValidationOption { using Type = bool; }; @@ -54,8 +59,13 @@ struct UseCrc32cValueOption { * * The option has no effect for partial downloads or any other circumstance * where the service does not return a MD5 hash. + * + * @deprecated Use `UploadChecksumValidationOption` and + * `DownloadChecksumValidationOption` instead. */ -struct EnableMD5ValidationOption { +struct [[deprecated( + "Use UploadChecksumValidationOption and DownloadChecksumValidationOption " + "instead")]] EnableMD5ValidationOption { using Type = bool; }; diff --git a/google/cloud/storage/hashing_options.h b/google/cloud/storage/hashing_options.h index 3fcac86aea5d4..d9c26edbc6b8f 100644 --- a/google/cloud/storage/hashing_options.h +++ b/google/cloud/storage/hashing_options.h @@ -15,8 +15,10 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_HASHING_OPTIONS_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_HASHING_OPTIONS_H +#include "google/cloud/internal/disable_deprecation_warnings.inc" #include "google/cloud/storage/internal/complex_option.h" #include "google/cloud/storage/version.h" +#include "google/cloud/version.h" #include "absl/strings/string_view.h" #include @@ -70,8 +72,14 @@ inline std::string ComputeMD5Hash(char const* payload) { * for most applications. Disabling CRC32C checksums while MD5 hashes remain * disabled exposes your application to data corruption. We recommend that all * uploads to GCS and downloads from GCS use CRC32C checksums. + * + * @deprecated Use `UploadChecksumValidationOption` and + * `DownloadChecksumValidationOption` instead. */ -struct DisableMD5Hash : public internal::ComplexOption { +struct [[deprecated( + "Use UploadChecksumValidationOption and DownloadChecksumValidationOption " + "instead")]] DisableMD5Hash + : public internal::ComplexOption { using ComplexOption::ComplexOption; // GCC <= 7.0 does not use the inherited default constructor, redeclare it // explicitly @@ -134,8 +142,13 @@ inline std::string ComputeCrc32cChecksum(char const* payload) { * for most applications. Disabling CRC32C checksums while MD5 hashes remain * disabled exposes your application to data corruption. We recommend that all * uploads to GCS and downloads from GCS use CRC32C checksums. + * + * @deprecated Use `UploadChecksumValidationOption` and + * `DownloadChecksumValidationOption` instead. */ -struct DisableCrc32cChecksum +struct [[deprecated( + "Use UploadChecksumValidationOption and DownloadChecksumValidationOption " + "instead")]] DisableCrc32cChecksum : public internal::ComplexOption { using ComplexOption::ComplexOption; // GCC <= 7.0 does not use the inherited default constructor, redeclare it @@ -149,4 +162,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" #endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_HASHING_OPTIONS_H diff --git a/google/cloud/storage/hashing_options_test.cc b/google/cloud/storage/hashing_options_test.cc index 047731d655c6b..8bd3ec50dc388 100644 --- a/google/cloud/storage/hashing_options_test.cc +++ b/google/cloud/storage/hashing_options_test.cc @@ -13,6 +13,8 @@ // limitations under the License. #include "google/cloud/storage/hashing_options.h" +#include "google/cloud/storage/options.h" +#include "google/cloud/options.h" #include #include @@ -54,6 +56,23 @@ TEST(ComputeCrc32cChecksumTest, Simple) { EXPECT_EQ("ImIEBA==", actual); } +TEST(ChecksumOptionsTest, SetAndGet) { + Options options; + EXPECT_FALSE(options.has()); + EXPECT_FALSE(options.has()); + + options.set(ChecksumAlgorithm::kMD5); + options.set(ChecksumAlgorithm::kCrc32c); + + EXPECT_TRUE(options.has()); + EXPECT_EQ(ChecksumAlgorithm::kMD5, + options.get()); + + EXPECT_TRUE(options.has()); + EXPECT_EQ(ChecksumAlgorithm::kCrc32c, + options.get()); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage diff --git a/google/cloud/storage/internal/hash_function.h b/google/cloud/storage/internal/hash_function.h index da7699353739f..0e17442acce05 100644 --- a/google/cloud/storage/internal/hash_function.h +++ b/google/cloud/storage/internal/hash_function.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_HASH_FUNCTION_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_HASH_FUNCTION_H +#include "google/cloud/internal/disable_deprecation_warnings.inc" #include "google/cloud/storage/hashing_options.h" #include "google/cloud/storage/internal/hash_values.h" #include "google/cloud/storage/version.h" @@ -108,4 +109,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" #endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_HASH_FUNCTION_H diff --git a/google/cloud/storage/internal/object_requests.h b/google/cloud/storage/internal/object_requests.h index 6fd14a86b841b..ee5598d5d49c1 100644 --- a/google/cloud/storage/internal/object_requests.h +++ b/google/cloud/storage/internal/object_requests.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_OBJECT_REQUESTS_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_OBJECT_REQUESTS_H +#include "google/cloud/internal/disable_deprecation_warnings.inc" #include "google/cloud/storage/auto_finalize.h" #include "google/cloud/storage/download_options.h" #include "google/cloud/storage/hashing_options.h" @@ -645,4 +646,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" #endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_OBJECT_REQUESTS_H diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index f64f5d1f28e51..649c717e2c45e 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -70,6 +70,43 @@ struct CAPathOption { } // namespace internal +/** + * Supported checksum algorithms. + * + * @ingroup storage-options + */ +enum class ChecksumAlgorithm { + kNone, ///< Disable checksum validation + kCrc32c, ///< Use CRC32C for checksum validation + kMD5, ///< Use MD5 for checksum validation +}; + +/** + * Configure the checksum algorithm used for uploads. + * + * If set, the client computes (if necessary) and validates the checksum of + * an object during uploads. Set to `ChecksumAlgorithm::kNone` to disable + * checksum validation. + * + * @ingroup storage-options + */ +struct UploadChecksumValidationOption { + using Type = ChecksumAlgorithm; +}; + +/** + * Configure the checksum algorithm used for downloads. + * + * If set, the client computes (if necessary) and validates the checksum of + * an object during downloads. Set to `ChecksumAlgorithm::kNone` to disable + * checksum validation. + * + * @ingroup storage-options + */ +struct DownloadChecksumValidationOption { + using Type = ChecksumAlgorithm; +}; + /** * Configure the REST endpoint for the GCS client library. * @@ -325,6 +362,7 @@ using ClientOptionList = ::google::cloud::OptionList< MaximumCurlSocketRecvSizeOption, MaximumCurlSocketSendSizeOption, TransferStallTimeoutOption, RetryPolicyOption, BackoffPolicyOption, IdempotencyPolicyOption, CARootsFilePathOption, + UploadChecksumValidationOption, DownloadChecksumValidationOption, storage_experimental::HttpVersionOption>; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/parallel_upload.h b/google/cloud/storage/parallel_upload.h index 0d0e08db1b0ac..ae06bd1bbb6fe 100644 --- a/google/cloud/storage/parallel_upload.h +++ b/google/cloud/storage/parallel_upload.h @@ -15,6 +15,7 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_PARALLEL_UPLOAD_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_PARALLEL_UPLOAD_H +#include "google/cloud/internal/disable_deprecation_warnings.inc" #include "google/cloud/storage/client.h" #include "google/cloud/storage/internal/tuple_filter.h" #include "google/cloud/storage/object_stream.h" @@ -1217,4 +1218,5 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" #endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_PARALLEL_UPLOAD_H From 1268c7f0bae2b1854e725aefe756b5d5b20b1e6c Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 08:48:36 +0000 Subject: [PATCH 2/8] feat(storage): migrate async client to unified checksum options --- .../storage/google_cloud_cpp_storage_grpc.bzl | 1 + .../google_cloud_cpp_storage_grpc.cmake | 1 + .../storage/internal/async/checksum_helpers.h | 49 ++++++++ .../storage/internal/async/connection_impl.cc | 42 ++++--- .../async/connection_impl_read_hash_test.cc | 108 +++++++++++------- .../async/connection_impl_upload_hash_test.cc | 59 ++++++---- .../internal/async/default_options_test.cc | 11 ++ .../internal/async/object_descriptor_impl.cc | 12 +- .../async/object_descriptor_impl_test.cc | 11 ++ .../internal/async/writer_connection_impl.cc | 20 ++-- .../tests/async_client_integration_test.cc | 17 +-- 11 files changed, 229 insertions(+), 102 deletions(-) create mode 100644 google/cloud/storage/internal/async/checksum_helpers.h diff --git a/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl b/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl index 4113b1e4cab04..6c1ad25a15247 100644 --- a/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl +++ b/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl @@ -38,6 +38,7 @@ google_cloud_cpp_storage_grpc_hdrs = [ "async/writer_connection.h", "grpc_plugin.h", "internal/async/connection_fwd.h", + "internal/async/checksum_helpers.h", "internal/async/connection_impl.h", "internal/async/connection_tracing.h", "internal/async/default_options.h", diff --git a/google/cloud/storage/google_cloud_cpp_storage_grpc.cmake b/google/cloud/storage/google_cloud_cpp_storage_grpc.cmake index 35f458503ec73..0064cd7e9e78e 100644 --- a/google/cloud/storage/google_cloud_cpp_storage_grpc.cmake +++ b/google/cloud/storage/google_cloud_cpp_storage_grpc.cmake @@ -99,6 +99,7 @@ add_library( async/writer_connection.h grpc_plugin.cc grpc_plugin.h + internal/async/checksum_helpers.h internal/async/connection_fwd.h internal/async/connection_impl.cc internal/async/connection_impl.h diff --git a/google/cloud/storage/internal/async/checksum_helpers.h b/google/cloud/storage/internal/async/checksum_helpers.h new file mode 100644 index 0000000000000..eb8bd14cc3bfa --- /dev/null +++ b/google/cloud/storage/internal/async/checksum_helpers.h @@ -0,0 +1,49 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H +#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H + +#include "google/cloud/internal/disable_deprecation_warnings.inc" +#include "google/cloud/storage/async/options.h" +#include "google/cloud/storage/options.h" +#include "google/cloud/options.h" + +namespace google { +namespace cloud { +namespace storage_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +struct ChecksumSettings { + bool enable_crc32c; + bool enable_md5; +}; + +inline ChecksumSettings GetDownloadChecksumSettings(Options const& options) { + if (options.has()) { + auto const algo = options.get(); + return {algo == storage::ChecksumAlgorithm::kCrc32c, + algo == storage::ChecksumAlgorithm::kMD5}; + } + return {options.get(), + options.get()}; +} + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace storage_internal +} // namespace cloud +} // namespace google + +#include "google/cloud/internal/diagnostics_pop.inc" +#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_CHECKSUM_HELPERS_H diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 976fea52f2337..09bb6368cc8cd 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -19,6 +19,7 @@ #include "google/cloud/storage/async/reader.h" #include "google/cloud/storage/async/resume_policy.h" #include "google/cloud/storage/async/retry_policy.h" +#include "google/cloud/storage/internal/async/checksum_helpers.h" #include "google/cloud/storage/internal/async/default_options.h" #include "google/cloud/storage/internal/async/handle_redirect_error.h" #include "google/cloud/storage/internal/async/insert_object.h" @@ -82,12 +83,24 @@ inline std::unique_ptr idempotency_policy( std::unique_ptr CreateHashFunction( Options const& options) { auto crc32c = std::unique_ptr(); + bool enable_crc32c = false; + bool enable_md5 = false; + + if (options.has()) { + auto const algo = options.get(); + enable_crc32c = (algo == storage::ChecksumAlgorithm::kCrc32c); + enable_md5 = (algo == storage::ChecksumAlgorithm::kMD5); + } else { + enable_crc32c = options.get(); + enable_md5 = options.get(); + } + if (options.has()) { crc32c = std::make_unique( storage::internal::HashValues{ Crc32cFromProto(options.get()), /*.md5=*/{}}); - } else if (options.get()) { + } else if (enable_crc32c) { crc32c = std::make_unique(); } @@ -97,7 +110,7 @@ std::unique_ptr CreateHashFunction( storage::internal::HashValues{ /*.crc32=*/{}, MD5FromProto(options.get())}); - } else if (options.get()) { + } else if (enable_md5) { md5 = storage::internal::MD5HashFunction::Create(); } @@ -117,9 +130,7 @@ std::unique_ptr CreateHashValidator( request.read_limit() != 0 || request.read_offset() != 0; if (is_ranged_read) return storage::internal::CreateNullHashValidator(); - auto const enable_crc32c = - options.get(); - auto const enable_md5 = options.get(); + auto const [enable_crc32c, enable_md5] = GetDownloadChecksumSettings(options); if (enable_crc32c && enable_md5) { return std::make_unique( @@ -412,7 +423,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -467,7 +478,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -501,14 +512,15 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then([current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then( + [current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future> diff --git a/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc b/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc index 5519f9bdbb2d1..868b5530eccdd 100644 --- a/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc @@ -13,6 +13,17 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" + +// TODO: Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#elif defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +#endif + #include "google/cloud/storage/internal/async/connection_impl.h" #include "google/cloud/storage/internal/async/default_options.h" #include "google/cloud/storage/internal/crc32c.h" @@ -75,15 +86,21 @@ auto GeneratedObjectChecksums(HashTestCase const& tc) { std::ostream& operator<<(std::ostream& os, HashTestCase const& rhs) { os << "HashTestCase={options={"; os << "expected_status_code=" << rhs.expected_status_code // - << std::boolalpha // - << ", enable_crc32c_validation=" - << rhs.options.get(); + << std::boolalpha; // + if (rhs.options.has()) { + os << ", download_checksum=" + << static_cast( + rhs.options.get()); + } else { + os << ", enable_crc32c_validation=" + << rhs.options.get(); + os << ", enable_md5_validation=" + << rhs.options.get(); + } if (rhs.options.has()) { os << ", use_crc32_value=" << rhs.options.get(); } - os << ", enable_md5_validation=" - << rhs.options.get(); if (rhs.options.has()) { os << ", use_md5_value=" << rhs.options.get(); } @@ -129,56 +146,67 @@ INSTANTIATE_TEST_SUITE_P( // This is the common case. Only CRC32C is enabled by default. The // service returns both CRC32C and MD5 values. StatusCode::kOk, - Options{} - .set(true) - .set(false), + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, HashTestCase{ // This is also common, the service does not return a MD5 value. StatusCode::kOk, - Options{} - .set(true) - .set(false), + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksum, ""}, // Make sure things work when both hashes are validated too. HashTestCase{StatusCode::kOk, - Options{} - .set(true) - .set(true), + Options{}.set( + storage::ChecksumAlgorithm::kNone), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, // In the next three cases we verify that disabling some validation // works. HashTestCase{StatusCode::kOk, - Options{} - .set(true) - .set(false), + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad}, HashTestCase{StatusCode::kOk, - Options{} - .set(false) - .set(true), + Options{}.set( + storage::ChecksumAlgorithm::kMD5), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash}, HashTestCase{StatusCode::kOk, - Options{} - .set(false) - .set(false), + Options{}.set( + storage::ChecksumAlgorithm::kNone), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kOk, + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), + kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kOk, + Options{}.set( + storage::ChecksumAlgorithm::kMD5), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash}, + HashTestCase{StatusCode::kOk, + Options{}.set( + storage::ChecksumAlgorithm::kNone), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, // In the next three cases we verify that validation works when the // returned values are not correct. HashTestCase{StatusCode::kInvalidArgument, - Options{} - .set(false) - .set(true), + Options{}.set( + storage::ChecksumAlgorithm::kMD5), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kInvalidArgument, + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kInvalidArgument, + Options{}.set( + storage::ChecksumAlgorithm::kNone), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, HashTestCase{StatusCode::kInvalidArgument, - Options{} - .set(true) - .set(false), + Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, HashTestCase{StatusCode::kInvalidArgument, - Options{} - .set(true) - .set(true), + Options{}.set( + storage::ChecksumAlgorithm::kMD5), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, // The application may know what the values should be. Verify the // validation works correctly when the application provides correct @@ -186,8 +214,8 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{ StatusCode::kOk, Options{} - .set(true) - .set(true) + .set( + storage::ChecksumAlgorithm::kCrc32c) .set(kQuickFoxCrc32cChecksum) .set(BinaryMD5(kQuickFoxMD5Hash)), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, @@ -195,16 +223,16 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{ StatusCode::kInvalidArgument, Options{} - .set(true) - .set(true) + .set( + storage::ChecksumAlgorithm::kCrc32c) .set(kQuickFoxCrc32cChecksumBad) .set(BinaryMD5(kQuickFoxMD5Hash)), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, HashTestCase{ StatusCode::kInvalidArgument, Options{} - .set(true) - .set(true) + .set( + storage::ChecksumAlgorithm::kCrc32c) .set(kQuickFoxCrc32cChecksum) .set( BinaryMD5(kQuickFoxMD5HashBad)), @@ -212,8 +240,8 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{ StatusCode::kInvalidArgument, Options{} - .set(true) - .set(true) + .set( + storage::ChecksumAlgorithm::kCrc32c) .set(kQuickFoxCrc32cChecksumBad) .set( BinaryMD5(kQuickFoxMD5HashBad)), diff --git a/google/cloud/storage/internal/async/connection_impl_upload_hash_test.cc b/google/cloud/storage/internal/async/connection_impl_upload_hash_test.cc index 9106e0fd3ba1b..1e444cdbb24cd 100644 --- a/google/cloud/storage/internal/async/connection_impl_upload_hash_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_upload_hash_test.cc @@ -13,6 +13,17 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" + +// TODO: Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#elif defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +#endif + #include "google/cloud/storage/async/writer_connection.h" #include "google/cloud/storage/internal/async/connection_impl.h" #include "google/cloud/storage/internal/async/default_options.h" @@ -69,15 +80,21 @@ auto ExpectedObjectChecksums(HashTestCase const& tc) { std::ostream& operator<<(std::ostream& os, HashTestCase const& rhs) { os << "HashTestCase={options={"; - os << std::boolalpha // - << "enable_crc32c_validation=" - << rhs.options.get(); + os << std::boolalpha; // + if (rhs.options.has()) { + os << "upload_checksum=" + << static_cast( + rhs.options.get()); + } else { + os << "enable_crc32c_validation=" + << rhs.options.get(); + os << ", enable_md5_validation=" + << rhs.options.get(); + } if (rhs.options.has()) { os << ", use_crc32_value=" << rhs.options.get(); } - os << ", enable_md5_validation=" - << rhs.options.get(); if (rhs.options.has()) { os << ", use_md5_value=" << rhs.options.get(); } @@ -121,17 +138,14 @@ INSTANTIATE_TEST_SUITE_P( .set(true) .set(true), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, - HashTestCase{Options{} - .set(true) - .set(false), + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksum, ""}, - HashTestCase{Options{} - .set(false) - .set(true), + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kMD5), absl::nullopt, kQuickFoxMD5Hash}, - HashTestCase{Options{} - .set(false) - .set(false), + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kNone), absl::nullopt, ""})); INSTANTIATE_TEST_SUITE_P( @@ -139,26 +153,25 @@ INSTANTIATE_TEST_SUITE_P( ::testing::Values( HashTestCase{ Options{} - .set(false) - .set(false) + .set( + storage::ChecksumAlgorithm::kNone) .set(kQuickFoxCrc32cChecksum) .set(BinaryMD5(kQuickFoxMD5Hash)), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, HashTestCase{ Options{} - .set(false) - .set(false) + .set( + storage::ChecksumAlgorithm::kNone) .set(kQuickFoxCrc32cChecksum), kQuickFoxCrc32cChecksum, ""}, HashTestCase{ Options{} - .set(false) - .set(false) + .set( + storage::ChecksumAlgorithm::kNone) .set(BinaryMD5(kQuickFoxMD5Hash)), absl::nullopt, kQuickFoxMD5Hash}, - HashTestCase{Options{} - .set(false) - .set(false), + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kNone), absl::nullopt, ""})); TEST_P(AsyncConnectionImplUploadHashTest, StartUnbuffered) { diff --git a/google/cloud/storage/internal/async/default_options_test.cc b/google/cloud/storage/internal/async/default_options_test.cc index dc7e4e8588be8..03ac3f40ca2fd 100644 --- a/google/cloud/storage/internal/async/default_options_test.cc +++ b/google/cloud/storage/internal/async/default_options_test.cc @@ -13,6 +13,17 @@ // limitations under the License. #include "google/cloud/storage/internal/async/default_options.h" + +// TODO: Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#elif defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +#endif + #include "google/cloud/storage/async/idempotency_policy.h" #include "google/cloud/storage/async/options.h" #include "google/cloud/storage/async/resume_policy.h" diff --git a/google/cloud/storage/internal/async/object_descriptor_impl.cc b/google/cloud/storage/internal/async/object_descriptor_impl.cc index 4f145986521ad..64838ec30e358 100644 --- a/google/cloud/storage/internal/async/object_descriptor_impl.cc +++ b/google/cloud/storage/internal/async/object_descriptor_impl.cc @@ -14,6 +14,7 @@ #include "google/cloud/storage/internal/async/object_descriptor_impl.h" #include "google/cloud/storage/async/options.h" +#include "google/cloud/storage/internal/async/checksum_helpers.h" #include "google/cloud/storage/internal/async/handle_redirect_error.h" #include "google/cloud/storage/internal/async/multi_stream_manager.h" #include "google/cloud/storage/internal/async/object_descriptor_reader_tracing.h" @@ -23,6 +24,7 @@ #include "google/cloud/storage/internal/hash_validator.h" #include "google/cloud/storage/internal/hash_validator_impl.h" #include "google/cloud/storage/internal/hash_values.h" +#include "google/cloud/storage/options.h" #include "google/cloud/grpc_error_delegate.h" #include "google/cloud/internal/opentelemetry.h" #include "google/rpc/status.pb.h" @@ -221,9 +223,8 @@ std::unique_ptr ObjectDescriptorImpl::Read( std::shared_ptr ObjectDescriptorImpl::CreateHashFunction(bool is_full_read) const { - auto const enable_crc32c = - options_.get(); - auto const enable_md5 = options_.get(); + auto const [enable_crc32c, enable_md5] = + GetDownloadChecksumSettings(options_); if (enable_crc32c) { std::unique_ptr child; @@ -255,9 +256,8 @@ ObjectDescriptorImpl::CreateHashValidator(bool is_full_read) const { return storage::internal::CreateNullHashValidator(); } - auto const enable_crc32c = - options_.get(); - auto const enable_md5 = options_.get(); + auto const [enable_crc32c, enable_md5] = + GetDownloadChecksumSettings(options_); std::unique_ptr hash_validator; if (enable_crc32c && enable_md5) { diff --git a/google/cloud/storage/internal/async/object_descriptor_impl_test.cc b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc index 5423423b4a08d..821f8745a34f4 100644 --- a/google/cloud/storage/internal/async/object_descriptor_impl_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc @@ -13,6 +13,17 @@ // limitations under the License. #include "google/cloud/storage/internal/async/object_descriptor_impl.h" + +// TODO: Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#elif defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +#endif + #include "google/cloud/mocks/mock_async_streaming_read_write_rpc.h" #include "google/cloud/storage/async/options.h" #include "google/cloud/storage/async/resume_policy.h" diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 88dd17fc9cf74..649a13e928874 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -160,20 +160,18 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) { merged.get()); } action = PartialUpload::kFinalize; - } else if (current_options - .has() || - current_options.has()) { - // If the user specified a manual expected checksum dynamically at - // Finalize() via current_options, we manually inject it and use `kFinalize` - // so the internal hash function doesn't overwrite it with its own computed - // hash. - if (current_options.has()) { + } else if (merged.has() || + merged.has()) { + // If the user specified a manual expected checksum, we manually inject it + // and use `kFinalize` so the internal hash function doesn't overwrite it + // with its own computed hash. + if (merged.has()) { write.mutable_object_checksums()->set_crc32c( - current_options.get()); + merged.get()); } - if (current_options.has()) { + if (merged.has()) { write.mutable_object_checksums()->set_md5_hash( - current_options.get()); + merged.get()); } action = PartialUpload::kFinalize; } diff --git a/google/cloud/storage/tests/async_client_integration_test.cc b/google/cloud/storage/tests/async_client_integration_test.cc index 6827ec4ec8f57..71a54f181b229 100644 --- a/google/cloud/storage/tests/async_client_integration_test.cc +++ b/google/cloud/storage/tests/async_client_integration_test.cc @@ -1006,11 +1006,12 @@ TEST_F(AsyncClientIntegrationTest, Open) { TEST_F(AsyncClientIntegrationTest, OpenExceedMaximumRange) { if (!UsingEmulator()) GTEST_SKIP(); - auto async = - AsyncClient(TestOptions() - .set(1024) - .set(false) - .set(false)); + auto async = AsyncClient(TestOptions() + .set(1024) + .set( + storage::ChecksumAlgorithm::kNone) + .set( + storage::ChecksumAlgorithm::kNone)); auto client = MakeIntegrationTestClient(true, TestOptions()); auto object_name = MakeRandomObjectName(); @@ -1065,8 +1066,10 @@ TEST_F(AsyncClientIntegrationTest, OpenExceedMaximumRange) { TEST_F(AsyncClientIntegrationTest, OpenWithChecksumValidation) { if (!UsingEmulator()) GTEST_SKIP(); auto async = AsyncClient(TestOptions() - .set(true) - .set(true)); + .set( + storage::ChecksumAlgorithm::kCrc32c) + .set( + storage::ChecksumAlgorithm::kCrc32c)); auto client = MakeIntegrationTestClient(true, TestOptions()); auto object_name = MakeRandomObjectName(); From fb53b3e981160d82d25708312f6f121d4d19db66 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 14:14:38 +0000 Subject: [PATCH 3/8] fix(storage): address reviewer comments and fix format --- .../storage/internal/async/connection_impl.cc | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 09bb6368cc8cd..cec73569dff71 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -423,7 +423,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -478,7 +478,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -512,15 +512,14 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then( - [current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then([current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future> From 407b341ade292d02c61fbec2b104be4b0e347614 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 15:17:12 +0000 Subject: [PATCH 4/8] fix: resolve review comments and unit tests failures --- .../storage/internal/async/connection_impl.cc | 25 +++++++++++-------- .../internal/async/object_descriptor_impl.cc | 10 +++++--- .../internal/async/writer_connection_impl.cc | 20 ++++++++------- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index cec73569dff71..61d24323e3fdf 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -130,7 +130,9 @@ std::unique_ptr CreateHashValidator( request.read_limit() != 0 || request.read_offset() != 0; if (is_ranged_read) return storage::internal::CreateNullHashValidator(); - auto const [enable_crc32c, enable_md5] = GetDownloadChecksumSettings(options); + auto const settings = GetDownloadChecksumSettings(options); + auto const enable_crc32c = settings.enable_crc32c; + auto const enable_md5 = settings.enable_md5; if (enable_crc32c && enable_md5) { return std::make_unique( @@ -423,7 +425,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -478,7 +480,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -512,14 +514,15 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then([current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then( + [current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future> diff --git a/google/cloud/storage/internal/async/object_descriptor_impl.cc b/google/cloud/storage/internal/async/object_descriptor_impl.cc index 64838ec30e358..8d4117cefa0bf 100644 --- a/google/cloud/storage/internal/async/object_descriptor_impl.cc +++ b/google/cloud/storage/internal/async/object_descriptor_impl.cc @@ -223,8 +223,9 @@ std::unique_ptr ObjectDescriptorImpl::Read( std::shared_ptr ObjectDescriptorImpl::CreateHashFunction(bool is_full_read) const { - auto const [enable_crc32c, enable_md5] = - GetDownloadChecksumSettings(options_); + auto const settings = GetDownloadChecksumSettings(options_); + auto const enable_crc32c = settings.enable_crc32c; + auto const enable_md5 = settings.enable_md5; if (enable_crc32c) { std::unique_ptr child; @@ -256,8 +257,9 @@ ObjectDescriptorImpl::CreateHashValidator(bool is_full_read) const { return storage::internal::CreateNullHashValidator(); } - auto const [enable_crc32c, enable_md5] = - GetDownloadChecksumSettings(options_); + auto const settings = GetDownloadChecksumSettings(options_); + auto const enable_crc32c = settings.enable_crc32c; + auto const enable_md5 = settings.enable_md5; std::unique_ptr hash_validator; if (enable_crc32c && enable_md5) { diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 649a13e928874..88dd17fc9cf74 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -160,18 +160,20 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) { merged.get()); } action = PartialUpload::kFinalize; - } else if (merged.has() || - merged.has()) { - // If the user specified a manual expected checksum, we manually inject it - // and use `kFinalize` so the internal hash function doesn't overwrite it - // with its own computed hash. - if (merged.has()) { + } else if (current_options + .has() || + current_options.has()) { + // If the user specified a manual expected checksum dynamically at + // Finalize() via current_options, we manually inject it and use `kFinalize` + // so the internal hash function doesn't overwrite it with its own computed + // hash. + if (current_options.has()) { write.mutable_object_checksums()->set_crc32c( - merged.get()); + current_options.get()); } - if (merged.has()) { + if (current_options.has()) { write.mutable_object_checksums()->set_md5_hash( - merged.get()); + current_options.get()); } action = PartialUpload::kFinalize; } From 2701d5d4ce4c958084f7502542f79b41b899f2d0 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 16:03:24 +0000 Subject: [PATCH 5/8] fix(storage): clang-format style issues in connection_impl.cc --- .../storage/internal/async/connection_impl.cc | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 61d24323e3fdf..2dc45211a1095 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -425,7 +425,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -480,7 +480,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -514,15 +514,14 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then( - [current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then([current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future> From 81d61088fb974c2f434717ce5077e6cf95a3253f Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 16:18:00 +0000 Subject: [PATCH 6/8] fix: address PR review comments for read hash tests and upload checksum helper --- .../storage/internal/async/checksum_helpers.h | 10 ++++++++++ .../storage/internal/async/connection_impl.cc | 14 +++----------- .../async/connection_impl_read_hash_test.cc | 14 ++++++++------ 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/google/cloud/storage/internal/async/checksum_helpers.h b/google/cloud/storage/internal/async/checksum_helpers.h index eb8bd14cc3bfa..9937c75945236 100644 --- a/google/cloud/storage/internal/async/checksum_helpers.h +++ b/google/cloud/storage/internal/async/checksum_helpers.h @@ -40,6 +40,16 @@ inline ChecksumSettings GetDownloadChecksumSettings(Options const& options) { options.get()}; } +inline ChecksumSettings GetUploadChecksumSettings(Options const& options) { + if (options.has()) { + auto const algo = options.get(); + return {algo == storage::ChecksumAlgorithm::kCrc32c, + algo == storage::ChecksumAlgorithm::kMD5}; + } + return {options.get(), + options.get()}; +} + GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 2dc45211a1095..05a3e80b7e07d 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -83,17 +83,9 @@ inline std::unique_ptr idempotency_policy( std::unique_ptr CreateHashFunction( Options const& options) { auto crc32c = std::unique_ptr(); - bool enable_crc32c = false; - bool enable_md5 = false; - - if (options.has()) { - auto const algo = options.get(); - enable_crc32c = (algo == storage::ChecksumAlgorithm::kCrc32c); - enable_md5 = (algo == storage::ChecksumAlgorithm::kMD5); - } else { - enable_crc32c = options.get(); - enable_md5 = options.get(); - } + auto const settings = GetUploadChecksumSettings(options); + auto const enable_crc32c = settings.enable_crc32c; + auto const enable_md5 = settings.enable_md5; if (options.has()) { crc32c = std::make_unique( diff --git a/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc b/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc index 868b5530eccdd..d7e9db7a578ad 100644 --- a/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_read_hash_test.cc @@ -157,8 +157,9 @@ INSTANTIATE_TEST_SUITE_P( kQuickFoxCrc32cChecksum, ""}, // Make sure things work when both hashes are validated too. HashTestCase{StatusCode::kOk, - Options{}.set( - storage::ChecksumAlgorithm::kNone), + Options{} + .set(true) + .set(true), kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash}, // In the next three cases we verify that disabling some validation // works. @@ -197,8 +198,9 @@ INSTANTIATE_TEST_SUITE_P( storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, HashTestCase{StatusCode::kInvalidArgument, - Options{}.set( - storage::ChecksumAlgorithm::kNone), + Options{} + .set(true) + .set(true), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, HashTestCase{StatusCode::kInvalidArgument, Options{}.set( @@ -231,8 +233,8 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{ StatusCode::kInvalidArgument, Options{} - .set( - storage::ChecksumAlgorithm::kCrc32c) + .set(true) + .set(true) .set(kQuickFoxCrc32cChecksum) .set( BinaryMD5(kQuickFoxMD5HashBad)), From 16ac1155de47de279f05742d681149116831eb98 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 18:57:32 +0000 Subject: [PATCH 7/8] fix(storage): resolve read hash tests failing due to incorrect checksum settings --- .../storage/internal/async/connection_impl.cc | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 05a3e80b7e07d..6ca0e3c7ae06c 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -81,9 +81,9 @@ inline std::unique_ptr idempotency_policy( } std::unique_ptr CreateHashFunction( - Options const& options) { + Options const& options, + storage_internal::ChecksumSettings const& settings) { auto crc32c = std::unique_ptr(); - auto const settings = GetUploadChecksumSettings(options); auto const enable_crc32c = settings.enable_crc32c; auto const enable_md5 = settings.enable_md5; @@ -172,7 +172,8 @@ future> AsyncConnectionImpl::InsertObject( options->get(), google::storage::v2::ServiceConstants::MAX_WRITE_CHUNK_BYTES); - auto hash_function = CreateHashFunction(*options); + auto hash_function = + CreateHashFunction(*options, GetUploadChecksumSettings(*options)); ApplyRoutingHeaders(*context, request.write_object_spec()); AddIdempotencyToken(*context, id); auto rpc = stub->AsyncWriteObject(cq, std::move(context), options); @@ -276,9 +277,10 @@ AsyncConnectionImpl::ReadObject(ReadObjectParams p) { // Create the hash function and validator based on the original request. Note // that p.request will be moved-from, so we have to do it relatively early in // this function. + auto const settings = GetDownloadChecksumSettings(*current); auto hash_function = std::make_shared( - CreateHashFunction(*current)); + CreateHashFunction(*current, settings)); auto hash_validator = CreateHashValidator(p.request, *current); absl::optional requested_length; @@ -328,7 +330,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { auto current = internal::MakeImmutableOptions(std::move(p.options)); auto request = p.request; std::shared_ptr hash_function = - CreateHashFunction(*current); + CreateHashFunction(*current, GetUploadChecksumSettings(*current)); auto retry = std::shared_ptr(retry_policy(*current)); auto backoff = @@ -417,7 +419,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -472,7 +474,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -506,14 +508,15 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then([current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then( + [current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future> @@ -708,7 +711,8 @@ AsyncConnectionImpl::StartUnbufferedUploadImpl( StatusOr>( std::move(response).status())); } - auto hash_function = CreateHashFunction(*current); + auto hash_function = + CreateHashFunction(*current, GetUploadChecksumSettings(*current)); auto configure = [current, upload = response->upload_id()](grpc::ClientContext& context) { ApplyResumableUploadRoutingHeader(context, upload); @@ -746,7 +750,8 @@ AsyncConnectionImpl::ResumeUnbufferedUploadImpl( // the upload resumes from the beginning of the file. auto hash_function = storage::internal::CreateNullHashFunction(); if (response->persisted_size() == 0) { - hash_function = CreateHashFunction(*current); + hash_function = + CreateHashFunction(*current, GetUploadChecksumSettings(*current)); } auto configure = [current, upload_id = query.upload_id()](grpc::ClientContext& context) { From dc5fbb57acd827eb8a664b12bcef9e99609ed12e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 16 Jul 2026 10:01:19 +0000 Subject: [PATCH 8/8] fix(storage): resolve clang-format style issues --- .../storage/internal/async/connection_impl.cc | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 6ca0e3c7ae06c..2ef264992a79b 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -419,7 +419,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { return pending.then( [current, request = std::move(p.request), hash = std::move(hash_function), fa = std::move(factory)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto rpc = f.get(); if (!rpc) return std::move(rpc).status(); std::unique_ptr impl; @@ -474,7 +474,7 @@ AsyncConnectionImpl::StartBufferedUpload(UploadParams p) { return StartUnbufferedUpload(std::move(p)) .then([current = std::move(current), async_write_object = std::move(async_write_object)](auto f) mutable - -> StatusOr> { + -> StatusOr> { auto w = f.get(); if (!w) return std::move(w).status(); auto factory = [upload_id = (*w)->UploadId(), @@ -508,15 +508,14 @@ AsyncConnectionImpl::ResumeBufferedUpload(ResumeUploadParams p) { }; auto f = make_unbuffered(); - return f.then( - [current = std::move(current), - make_unbuffered = std::move(make_unbuffered)](auto f) mutable - -> StatusOr> { - auto w = f.get(); - if (!w) return std::move(w).status(); - return MakeWriterConnectionBuffered(std::move(make_unbuffered), - *std::move(w), *current); - }); + return f.then([current = std::move(current), + make_unbuffered = std::move(make_unbuffered)](auto f) mutable + -> StatusOr> { + auto w = f.get(); + if (!w) return std::move(w).status(); + return MakeWriterConnectionBuffered(std::move(make_unbuffered), + *std::move(w), *current); + }); } future>