From 2ea6ec75a015257b592c3f2f5974600e8216489b Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 08:48:36 +0000 Subject: [PATCH 01/12] 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 | 53 ++++++--- .../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, 232 insertions(+), 93 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 e9b81bf9526be..952d6dbef46d2 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 b1622bd87a386..60e6d77879407 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 4b2658d83eb73..2fbb3a2f6e6df 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 14db6158db840..94cf30718a491 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,9 +138,8 @@ 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) @@ -132,6 +148,12 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{Options{} .set(false) .set(false), + std::nullopt, ""}, + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kMD5), + std::nullopt, kQuickFoxMD5Hash}, + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kNone), std::nullopt, ""})); INSTANTIATE_TEST_SUITE_P( @@ -139,26 +161,29 @@ 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)), std::nullopt, kQuickFoxMD5Hash}, HashTestCase{Options{} .set(false) .set(false), + std::nullopt, ""}, + HashTestCase{Options{}.set( + storage::ChecksumAlgorithm::kNone), std::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 4a331b3a34f65..ae7a53aa32010 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 68169ed48d8a6..5f93213d32bad 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 59c4dce471365..fb0dbdc95bc4b 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 e383aeee30f14..aae9bfc02f1f9 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 a5ea1880c962a47485f863343a406a1c8d4020da Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 14:14:38 +0000 Subject: [PATCH 02/12] 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 60e6d77879407..3f2c0cbbb295f 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 2473a8c9dcc123adbb09b89ba4d3dd0361dcc07e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 15:17:12 +0000 Subject: [PATCH 03/12] 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 3f2c0cbbb295f..2bc2c35251d37 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 ae7a53aa32010..9430880b8d966 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 fb0dbdc95bc4b..59c4dce471365 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 5228545802a123df131e23aa56d3f0c230fd4694 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 16:03:24 +0000 Subject: [PATCH 04/12] 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 2bc2c35251d37..d2f43c67169d9 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 14245280be4e027109fc35e6070ebab95b99e5b6 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 16:18:00 +0000 Subject: [PATCH 05/12] 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 d2f43c67169d9..f5e1a63cb07fb 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 2fbb3a2f6e6df..c07c382dd94ea 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 b76b819fb351b9bd67c4e8ce7584b3cb73fa6ba3 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Wed, 15 Jul 2026 18:57:32 +0000 Subject: [PATCH 06/12] 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 f5e1a63cb07fb..735f756eb77f0 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); std::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 39c4a25bc7e1b1180c07bc356a1260b55a70f097 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 16 Jul 2026 10:01:19 +0000 Subject: [PATCH 07/12] 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 735f756eb77f0..cc3de87521cab 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> From c0cf77bc7101265d592693654eb2d5c396e0edac Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Fri, 17 Jul 2026 09:16:49 +0000 Subject: [PATCH 08/12] fix(storage): resolve reviewer comments for read hash tests and upload checksum helper --- .../internal/async/connection_impl_read_hash_test.cc | 10 ---------- .../internal/async/connection_impl_upload_hash_test.cc | 10 ---------- .../storage/internal/async/default_options_test.cc | 10 ---------- .../internal/async/object_descriptor_impl_test.cc | 10 ---------- 4 files changed, 40 deletions(-) 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 c07c382dd94ea..ff49d65efe113 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 @@ -14,16 +14,6 @@ #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" 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 94cf30718a491..656374a389c82 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 @@ -14,16 +14,6 @@ #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" diff --git a/google/cloud/storage/internal/async/default_options_test.cc b/google/cloud/storage/internal/async/default_options_test.cc index 03ac3f40ca2fd..3c7a6a2a17862 100644 --- a/google/cloud/storage/internal/async/default_options_test.cc +++ b/google/cloud/storage/internal/async/default_options_test.cc @@ -14,16 +14,6 @@ #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_test.cc b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc index 5f93213d32bad..782d818a02a1a 100644 --- a/google/cloud/storage/internal/async/object_descriptor_impl_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc @@ -14,16 +14,6 @@ #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" From 23bb221f42f172c15c9dfcd5048d0af0345a19b5 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Fri, 17 Jul 2026 09:43:07 +0000 Subject: [PATCH 09/12] fix(storage): resolve clang format --- .../storage/internal/async/connection_impl_read_hash_test.cc | 1 - .../storage/internal/async/connection_impl_upload_hash_test.cc | 1 - google/cloud/storage/internal/async/default_options_test.cc | 1 - .../cloud/storage/internal/async/object_descriptor_impl_test.cc | 1 - 4 files changed, 4 deletions(-) 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 ff49d65efe113..7904fa2fc79ed 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,7 +13,6 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" - #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" 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 656374a389c82..ae8b3e6a2318e 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,7 +13,6 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" - #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" diff --git a/google/cloud/storage/internal/async/default_options_test.cc b/google/cloud/storage/internal/async/default_options_test.cc index 3c7a6a2a17862..dc7e4e8588be8 100644 --- a/google/cloud/storage/internal/async/default_options_test.cc +++ b/google/cloud/storage/internal/async/default_options_test.cc @@ -13,7 +13,6 @@ // limitations under the License. #include "google/cloud/storage/internal/async/default_options.h" - #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_test.cc b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc index 782d818a02a1a..68169ed48d8a6 100644 --- a/google/cloud/storage/internal/async/object_descriptor_impl_test.cc +++ b/google/cloud/storage/internal/async/object_descriptor_impl_test.cc @@ -13,7 +13,6 @@ // limitations under the License. #include "google/cloud/storage/internal/async/object_descriptor_impl.h" - #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" From e6d1487c8437a645a7d1427c478099724b579174 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Fri, 17 Jul 2026 09:55:43 +0000 Subject: [PATCH 10/12] fix(storage): add missing TODO username and disable deprecation warnings for tests --- .../storage/internal/async/connection_impl_read_hash_test.cc | 5 +++++ .../internal/async/connection_impl_upload_hash_test.cc | 5 +++++ google/cloud/storage/internal/async/default_options_test.cc | 5 +++++ .../storage/internal/async/object_descriptor_impl_test.cc | 5 +++++ 4 files changed, 20 insertions(+) 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 7904fa2fc79ed..35ad8dbc9dcdf 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,10 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" + +// TODO(v-pratap): Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#include "google/cloud/internal/disable_deprecation_warnings.inc" #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" @@ -496,3 +500,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" 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 ae8b3e6a2318e..18fd11a459adf 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,10 @@ // limitations under the License. #include "google/cloud/storage/async/options.h" + +// TODO(v-pratap): Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#include "google/cloud/internal/disable_deprecation_warnings.inc" #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" @@ -795,3 +799,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" diff --git a/google/cloud/storage/internal/async/default_options_test.cc b/google/cloud/storage/internal/async/default_options_test.cc index dc7e4e8588be8..7bdc79553a84b 100644 --- a/google/cloud/storage/internal/async/default_options_test.cc +++ b/google/cloud/storage/internal/async/default_options_test.cc @@ -13,6 +13,10 @@ // limitations under the License. #include "google/cloud/storage/internal/async/default_options.h" + +// TODO(v-pratap): Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#include "google/cloud/internal/disable_deprecation_warnings.inc" #include "google/cloud/storage/async/idempotency_policy.h" #include "google/cloud/storage/async/options.h" #include "google/cloud/storage/async/resume_policy.h" @@ -110,3 +114,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" 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 68169ed48d8a6..07830a3e63fdb 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,10 @@ // limitations under the License. #include "google/cloud/storage/internal/async/object_descriptor_impl.h" + +// TODO(v-pratap): Remove this when EnableMD5ValidationOption and +// EnableCrc32cValidationOption are removed. +#include "google/cloud/internal/disable_deprecation_warnings.inc" #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" @@ -2299,3 +2303,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal } // namespace cloud } // namespace google +#include "google/cloud/internal/diagnostics_pop.inc" From e2c354a48dabdf5a33a51630d95bf421399df656 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Fri, 17 Jul 2026 10:42:36 +0000 Subject: [PATCH 11/12] fix(storage): fix alphabetical order in google_cloud_cpp_storage_grpc.bzl --- google/cloud/storage/google_cloud_cpp_storage_grpc.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl b/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl index 6c1ad25a15247..ad210011ea06f 100644 --- a/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl +++ b/google/cloud/storage/google_cloud_cpp_storage_grpc.bzl @@ -37,8 +37,8 @@ google_cloud_cpp_storage_grpc_hdrs = [ "async/writer.h", "async/writer_connection.h", "grpc_plugin.h", - "internal/async/connection_fwd.h", "internal/async/checksum_helpers.h", + "internal/async/connection_fwd.h", "internal/async/connection_impl.h", "internal/async/connection_tracing.h", "internal/async/default_options.h", From 228b08383f49d29334497f79dfe53f075a5dca90 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Mon, 20 Jul 2026 07:31:58 +0000 Subject: [PATCH 12/12] test(storage): retain legacy checksum option test cases --- .../async/connection_impl_read_hash_test.cc | 27 +++++++++++++++++++ .../async/connection_impl_upload_hash_test.cc | 6 +++++ 2 files changed, 33 insertions(+) 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 35ad8dbc9dcdf..3c18df70bdb42 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 @@ -180,6 +180,22 @@ INSTANTIATE_TEST_SUITE_P( Options{}.set( storage::ChecksumAlgorithm::kNone), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + // Legacy options + HashTestCase{StatusCode::kOk, + Options{} + .set(true) + .set(false), + kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kOk, + Options{} + .set(false) + .set(true), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash}, + HashTestCase{StatusCode::kOk, + Options{} + .set(false) + .set(false), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, // In the next three cases we verify that validation works when the // returned values are not correct. HashTestCase{StatusCode::kInvalidArgument, @@ -203,6 +219,17 @@ INSTANTIATE_TEST_SUITE_P( Options{}.set( storage::ChecksumAlgorithm::kMD5), kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + // Legacy options + HashTestCase{StatusCode::kInvalidArgument, + Options{} + .set(false) + .set(true), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, + HashTestCase{StatusCode::kInvalidArgument, + Options{} + .set(true) + .set(false), + kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad}, // The application may know what the values should be. Verify the // validation works correctly when the application provides correct // values. 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 18fd11a459adf..37d2b3a71ffaa 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 @@ -134,6 +134,11 @@ INSTANTIATE_TEST_SUITE_P( HashTestCase{Options{}.set( storage::ChecksumAlgorithm::kCrc32c), kQuickFoxCrc32cChecksum, ""}, + // Legacy options + HashTestCase{Options{} + .set(true) + .set(false), + kQuickFoxCrc32cChecksum, ""}, HashTestCase{Options{} .set(false) .set(true), @@ -171,6 +176,7 @@ INSTANTIATE_TEST_SUITE_P( storage::ChecksumAlgorithm::kNone) .set(BinaryMD5(kQuickFoxMD5Hash)), std::nullopt, kQuickFoxMD5Hash}, + // Legacy options HashTestCase{Options{} .set(false) .set(false),