feat(storage): Migrate async client to unified checksum options

google/cloud/storage/google_cloud_cpp_storage_grpc.bzl

@@ -37,6 +37,7 @@ google_cloud_cpp_storage_grpc_hdrs = [
     "async/writer.h",
     "async/writer_connection.h",
     "grpc_plugin.h",
+    "internal/async/checksum_helpers.h",
     "internal/async/connection_fwd.h",
     "internal/async/connection_impl.h",
     "internal/async/connection_tracing.h",

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

google/cloud/storage/internal/async/checksum_helpers.h

@@ -0,0 +1,59 @@
+// 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<storage::DownloadChecksumValidationOption>()) {
+    auto const algo = options.get<storage::DownloadChecksumValidationOption>();
+    return {algo == storage::ChecksumAlgorithm::kCrc32c,
+            algo == storage::ChecksumAlgorithm::kMD5};
+  }
+  return {options.get<storage::EnableCrc32cValidationOption>(),
+          options.get<storage::EnableMD5ValidationOption>()};
+}
+
+inline ChecksumSettings GetUploadChecksumSettings(Options const& options) {
+  if (options.has<storage::UploadChecksumValidationOption>()) {
+    auto const algo = options.get<storage::UploadChecksumValidationOption>();
+    return {algo == storage::ChecksumAlgorithm::kCrc32c,
+            algo == storage::ChecksumAlgorithm::kMD5};
+  }
+  return {options.get<storage::EnableCrc32cValidationOption>(),
+          options.get<storage::EnableMD5ValidationOption>()};
+}
+
+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

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"
@@ -80,14 +81,18 @@ inline std::unique_ptr<storage::AsyncIdempotencyPolicy> idempotency_policy(
 }
 
 std::unique_ptr<storage::internal::HashFunction> CreateHashFunction(
-    Options const& options) {
+    Options const& options,
+    storage_internal::ChecksumSettings const& settings) {
   auto crc32c = std::unique_ptr<storage::internal::HashFunction>();
+  auto const enable_crc32c = settings.enable_crc32c;
+  auto const enable_md5 = settings.enable_md5;
+
   if (options.has<storage::UseCrc32cValueOption>()) {
     crc32c = std::make_unique<storage::internal::PrecomputedHashFunction>(
         storage::internal::HashValues{
             Crc32cFromProto(options.get<storage::UseCrc32cValueOption>()),
             /*.md5=*/{}});
-  } else if (options.get<storage::EnableCrc32cValidationOption>()) {
+  } else if (enable_crc32c) {
     crc32c = std::make_unique<storage::internal::Crc32cHashFunction>();
   }
 
@@ -97,7 +102,7 @@ std::unique_ptr<storage::internal::HashFunction> CreateHashFunction(
         storage::internal::HashValues{
             /*.crc32=*/{},
             MD5FromProto(options.get<storage::UseMD5ValueOption>())});
-  } else if (options.get<storage::EnableMD5ValidationOption>()) {
+  } else if (enable_md5) {
     md5 = storage::internal::MD5HashFunction::Create();
   }
 
@@ -117,9 +122,9 @@ std::unique_ptr<storage::internal::HashValidator> CreateHashValidator(
       request.read_limit() != 0 || request.read_offset() != 0;
   if (is_ranged_read) return storage::internal::CreateNullHashValidator();
 
-  auto const enable_crc32c =
-      options.get<storage::EnableCrc32cValidationOption>();
-  auto const enable_md5 = options.get<storage::EnableMD5ValidationOption>();
+  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<storage::internal::CompositeValidator>(
@@ -167,7 +172,8 @@ future<StatusOr<google::storage::v2::Object>> AsyncConnectionImpl::InsertObject(
         options->get<storage::TransferStallMinimumRateOption>(),
         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);
@@ -271,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<storage::internal::Crc32cMessageHashFunction>(
-          CreateHashFunction(*current));
+          CreateHashFunction(*current, settings));
   auto hash_validator = CreateHashValidator(p.request, *current);
 
   std::optional<std::int64_t> requested_length;
@@ -323,7 +330,7 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) {
   auto current = internal::MakeImmutableOptions(std::move(p.options));
   auto request = p.request;
   std::shared_ptr<storage::internal::HashFunction> hash_function =
-      CreateHashFunction(*current);
+      CreateHashFunction(*current, GetUploadChecksumSettings(*current));
   auto retry =
       std::shared_ptr<storage::AsyncRetryPolicy>(retry_policy(*current));
   auto backoff =
@@ -703,7 +710,8 @@ AsyncConnectionImpl::StartUnbufferedUploadImpl(
         StatusOr<std::unique_ptr<storage::AsyncWriterConnection>>(
             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);
@@ -741,7 +749,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) {

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"
@@ -75,15 +79,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<storage::EnableCrc32cValidationOption>();
+     << std::boolalpha;                                      //
+  if (rhs.options.has<storage::DownloadChecksumValidationOption>()) {
+    os << ", download_checksum="
+       << static_cast<int>(
+              rhs.options.get<storage::DownloadChecksumValidationOption>());
+  } else {
+    os << ", enable_crc32c_validation="
+       << rhs.options.get<storage::EnableCrc32cValidationOption>();
+    os << ", enable_md5_validation="
+       << rhs.options.get<storage::EnableMD5ValidationOption>();
+  }
   if (rhs.options.has<storage::UseCrc32cValueOption>()) {
     os << ", use_crc32_value="
        << rhs.options.get<storage::UseCrc32cValueOption>();
   }
-  os << ", enable_md5_validation="
-     << rhs.options.get<storage::EnableMD5ValidationOption>();
   if (rhs.options.has<storage::UseMD5ValueOption>()) {
     os << ", use_md5_value=" << rhs.options.get<storage::UseMD5ValueOption>();
   }
@@ -129,16 +139,14 @@ 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<storage::EnableCrc32cValidationOption>(true)
-                .set<storage::EnableMD5ValidationOption>(false),
+            Options{}.set<storage::DownloadChecksumValidationOption>(
+                storage::ChecksumAlgorithm::kCrc32c),
             kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
         HashTestCase{
             // This is also common, the service does not return a MD5 value.
             StatusCode::kOk,
-            Options{}
-                .set<storage::EnableCrc32cValidationOption>(true)
-                .set<storage::EnableMD5ValidationOption>(false),
+            Options{}.set<storage::DownloadChecksumValidationOption>(
+                storage::ChecksumAlgorithm::kCrc32c),
             kQuickFoxCrc32cChecksum, ""},
         // Make sure things work when both hashes are validated too.
         HashTestCase{StatusCode::kOk,
@@ -148,6 +156,31 @@ INSTANTIATE_TEST_SUITE_P(
                      kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
         // In the next three cases we verify that disabling some validation
         // works.
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kCrc32c),
+                     kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kMD5),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash},
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kNone),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kCrc32c),
+                     kQuickFoxCrc32cChecksum, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kMD5),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5Hash},
+        HashTestCase{StatusCode::kOk,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kNone),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        // Legacy options
         HashTestCase{StatusCode::kOk,
                      Options{}
                          .set<storage::EnableCrc32cValidationOption>(true)
@@ -165,38 +198,55 @@ INSTANTIATE_TEST_SUITE_P(
                      kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
         // In the next three cases we verify that validation works when the
         // returned values are not correct.
+        HashTestCase{StatusCode::kInvalidArgument,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kMD5),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kInvalidArgument,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kCrc32c),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
         HashTestCase{StatusCode::kInvalidArgument,
                      Options{}
-                         .set<storage::EnableCrc32cValidationOption>(false)
+                         .set<storage::EnableCrc32cValidationOption>(true)
                          .set<storage::EnableMD5ValidationOption>(true),
                      kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kInvalidArgument,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kCrc32c),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        HashTestCase{StatusCode::kInvalidArgument,
+                     Options{}.set<storage::DownloadChecksumValidationOption>(
+                         storage::ChecksumAlgorithm::kMD5),
+                     kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
+        // Legacy options
         HashTestCase{StatusCode::kInvalidArgument,
                      Options{}
-                         .set<storage::EnableCrc32cValidationOption>(true)
-                         .set<storage::EnableMD5ValidationOption>(false),
+                         .set<storage::EnableCrc32cValidationOption>(false)
+                         .set<storage::EnableMD5ValidationOption>(true),
                      kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
         HashTestCase{StatusCode::kInvalidArgument,
                      Options{}
                          .set<storage::EnableCrc32cValidationOption>(true)
-                         .set<storage::EnableMD5ValidationOption>(true),
+                         .set<storage::EnableMD5ValidationOption>(false),
                      kQuickFoxCrc32cChecksumBad, kQuickFoxMD5HashBad},
         // The application may know what the values should be. Verify the
         // validation works correctly when the application provides correct
         // values.
         HashTestCase{
             StatusCode::kOk,
             Options{}
-                .set<storage::EnableCrc32cValidationOption>(true)
-                .set<storage::EnableMD5ValidationOption>(true)
+                .set<storage::DownloadChecksumValidationOption>(
+                    storage::ChecksumAlgorithm::kCrc32c)
                 .set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksum)
                 .set<storage::UseMD5ValueOption>(BinaryMD5(kQuickFoxMD5Hash)),
             kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
         // Verify bad values are detected
         HashTestCase{
             StatusCode::kInvalidArgument,
             Options{}
-                .set<storage::EnableCrc32cValidationOption>(true)
-                .set<storage::EnableMD5ValidationOption>(true)
+                .set<storage::DownloadChecksumValidationOption>(
+                    storage::ChecksumAlgorithm::kCrc32c)
                 .set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksumBad)
                 .set<storage::UseMD5ValueOption>(BinaryMD5(kQuickFoxMD5Hash)),
             kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash},
@@ -212,8 +262,8 @@ INSTANTIATE_TEST_SUITE_P(
         HashTestCase{
             StatusCode::kInvalidArgument,
             Options{}
-                .set<storage::EnableCrc32cValidationOption>(true)
-                .set<storage::EnableMD5ValidationOption>(true)
+                .set<storage::DownloadChecksumValidationOption>(
+                    storage::ChecksumAlgorithm::kCrc32c)
                 .set<storage::UseCrc32cValueOption>(kQuickFoxCrc32cChecksumBad)
                 .set<storage::UseMD5ValueOption>(
                     BinaryMD5(kQuickFoxMD5HashBad)),
@@ -477,3 +527,4 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage_internal
 }  // namespace cloud
 }  // namespace google
+#include "google/cloud/internal/diagnostics_pop.inc"