Completely remove deprecated legacy checksum options

Author: v-pratap

google/cloud/storage/client_object_test.cc

@@ -234,119 +234,6 @@ TEST_F(ObjectTest, ReadObject) {
   EXPECT_EQ(actual.gcount(), 1024);
 }
 
-TEST_F(ObjectTest, ReadObjectChecksumPrecedence) {
-  EXPECT_CALL(*mock_, ReadObject)
-      .WillOnce([](internal::ReadObjectRangeRequest const& r) {
-        EXPECT_TRUE(r.HasOption<DisableMD5Hash>());
-        EXPECT_FALSE(r.GetOption<DisableMD5Hash>().value());
-
-        auto settings =
-            internal::GetDownloadChecksumSettings(r, CurrentOptions());
-        // Verify MD5 is enabled (disable_md5 = false) and CRC32C is disabled
-        // (disable_crc32c = true)
-        EXPECT_FALSE(settings.md5);
-        EXPECT_TRUE(settings.crc32c);
-
-        auto read_source = std::make_unique<testing::MockObjectReadSource>();
-        EXPECT_CALL(*read_source, IsOpen()).WillRepeatedly(Return(true));
-        EXPECT_CALL(*read_source, Read)
-            .WillOnce(Return(internal::ReadSourceResult{1024, {}}));
-        EXPECT_CALL(*read_source, Close).Times(1);
-        return StatusOr<std::unique_ptr<internal::ObjectReadSource>>(
-            std::move(read_source));
-      });
-  auto client = ClientForMock();
-  auto actual = client.ReadObject(
-      "test-bucket-name", "test-object-name", DisableMD5Hash(false),
-      Options{}.set<DownloadChecksumValidationOption>(
-          ChecksumAlgorithm::kNone));
-  ASSERT_STATUS_OK(actual.status());
-  std::vector<char> v(1024);
-  actual.read(v.data(), v.size());
-  EXPECT_EQ(actual.gcount(), 1024);
-}
-
-TEST_F(ObjectTest, ReadObjectChecksumPrecedenceDisableMD5) {
-  EXPECT_CALL(*mock_, ReadObject)
-      .WillOnce([](internal::ReadObjectRangeRequest const& r) {
-        EXPECT_TRUE(r.HasOption<DisableMD5Hash>());
-        EXPECT_TRUE(r.GetOption<DisableMD5Hash>().value());
-
-        auto settings =
-            internal::GetDownloadChecksumSettings(r, CurrentOptions());
-        // DisableMD5Hash(true) should override ChecksumAlgorithm::kMD5
-        EXPECT_TRUE(settings.md5);
-        EXPECT_TRUE(settings.crc32c);  // kMD5 disables crc32c
-
-        auto read_source = std::make_unique<testing::MockObjectReadSource>();
-        EXPECT_CALL(*read_source, IsOpen()).WillRepeatedly(Return(true));
-        EXPECT_CALL(*read_source, Read)
-            .WillOnce(Return(internal::ReadSourceResult{1024, {}}));
-        EXPECT_CALL(*read_source, Close).Times(1);
-        return StatusOr<std::unique_ptr<internal::ObjectReadSource>>(
-            std::move(read_source));
-      });
-  auto client = ClientForMock();
-  auto actual = client.ReadObject(
-      "test-bucket-name", "test-object-name", DisableMD5Hash(true),
-      Options{}.set<DownloadChecksumValidationOption>(ChecksumAlgorithm::kMD5));
-  ASSERT_STATUS_OK(actual.status());
-  std::vector<char> v(1024);
-  actual.read(v.data(), v.size());
-  EXPECT_EQ(actual.gcount(), 1024);
-}
-
-TEST_F(ObjectTest, InsertObjectChecksumPrecedence) {
-  EXPECT_CALL(*mock_, InsertObjectMedia)
-      .WillOnce([](internal::InsertObjectMediaRequest const& r) {
-        EXPECT_TRUE(r.HasOption<DisableCrc32cChecksum>());
-        EXPECT_TRUE(r.GetOption<DisableCrc32cChecksum>().value());
-
-        auto settings =
-            internal::GetUploadChecksumSettings(r, CurrentOptions());
-        // Verify CRC32C is disabled (disable_crc32c = true) and MD5 remains
-        // enabled (disable_md5 = false)
-        EXPECT_TRUE(settings.crc32c);
-        EXPECT_FALSE(settings.md5);
-
-        return make_status_or(
-            storage::internal::ObjectMetadataParser::FromString(
-                R"({"name": "test-object-name"})")
-                .value());
-      });
-  auto client = ClientForMock();
-  auto actual =
-      client.InsertObject("test-bucket-name", "test-object-name", "payload",
-                          DisableCrc32cChecksum(true),
-                          Options{}.set<UploadChecksumValidationOption>(
-                              ChecksumAlgorithm::kCrc32cAndMD5));
-  ASSERT_STATUS_OK(actual);
-}
-
-TEST_F(ObjectTest, InsertObjectChecksumPrecedenceEnableCrc32c) {
-  EXPECT_CALL(*mock_, InsertObjectMedia)
-      .WillOnce([](internal::InsertObjectMediaRequest const& r) {
-        EXPECT_TRUE(r.HasOption<DisableCrc32cChecksum>());
-        EXPECT_FALSE(r.GetOption<DisableCrc32cChecksum>().value());
-
-        auto settings =
-            internal::GetUploadChecksumSettings(r, CurrentOptions());
-        // DisableCrc32cChecksum(false) should override ChecksumAlgorithm::kNone
-        EXPECT_FALSE(settings.crc32c);
-        EXPECT_TRUE(settings.md5);  // kNone disables md5
-
-        return make_status_or(
-            storage::internal::ObjectMetadataParser::FromString(
-                R"({"name": "test-object-name"})")
-                .value());
-      });
-  auto client = ClientForMock();
-  auto actual = client.InsertObject(
-      "test-bucket-name", "test-object-name", "payload",
-      DisableCrc32cChecksum(false),
-      Options{}.set<UploadChecksumValidationOption>(ChecksumAlgorithm::kNone));
-  ASSERT_STATUS_OK(actual);
-}
 
 TEST_F(ObjectTest, WriteObject) {
   EXPECT_CALL(*mock_, CreateResumableUpload)

google/cloud/storage/hashing_options.h

@@ -62,40 +62,7 @@ inline std::string ComputeMD5Hash(char const* payload) {
 }
 
 /**
- * Disable or enable MD5 Hashing computations.
- *
- * By default MD5 hashes are disabled. To enable them use the
- * `EnableMD5Hash()` helper function.
- *
- * @warning MD5 hashes are disabled by default, as they are computationally
- *   expensive, and CRC32C checksums provide enough data integrity protection
- *   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 [[deprecated(
-    "Use UploadChecksumValidationOption and DownloadChecksumValidationOption "
-    "instead")]] DisableMD5Hash
-    : public internal::ComplexOption<DisableMD5Hash, bool> {
-  using ComplexOption<DisableMD5Hash, bool>::ComplexOption;
-  // GCC <= 7.0 does not use the inherited default constructor, redeclare it
-  // explicitly
-  DisableMD5Hash() = default;
-  static char const* name() { return "disable-md5-hash"; }
-};
-
-/**
- * Enable MD5 hashes in upload and download operations.
- *
- * Use this function where the option `DisableMD5Hash` is expected to enable MD5
- * hashes.
- */
-inline DisableMD5Hash EnableMD5Hash() { return DisableMD5Hash(false); }
 
-/**
  * Provide a pre-computed CRC32C checksum value.
  *
  * The application may be able to obtain a CRC32C checksum in some out-of-band
@@ -130,32 +97,6 @@ inline std::string ComputeCrc32cChecksum(char const* payload) {
                                                   : absl::string_view{payload});
 }
 
-/**
- * Disable CRC32C checksum computations.
- *
- * By default the GCS client library computes CRC32C checksums in all upload and
- * download operations. The application can use this option to disable the
- * checksum computation.
- *
- * @warning MD5 hashes are disabled by default, as they are computationally
- *   expensive, and CRC32C checksums provide enough data integrity protection
- *   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 [[deprecated(
-    "Use UploadChecksumValidationOption and DownloadChecksumValidationOption "
-    "instead")]] DisableCrc32cChecksum
-    : public internal::ComplexOption<DisableCrc32cChecksum, bool> {
-  using ComplexOption<DisableCrc32cChecksum, bool>::ComplexOption;
-  // GCC <= 7.0 does not use the inherited default constructor, redeclare it
-  // explicitly
-  DisableCrc32cChecksum() = default;
-  static char const* name() { return "disable-crc32c-checksum"; }
-};
 
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage

google/cloud/storage/internal/checksum_helpers.h

@@ -32,7 +32,7 @@ struct HashDisabled {
 };
 
 template <typename Request>
-HashDisabled GetDownloadChecksumSettings(Request const& request,
+HashDisabled GetDownloadChecksumSettings(Request const&,
                                          Options const& options) {
   bool disable_md5 = true;
   bool disable_crc32c = false;
@@ -44,19 +44,12 @@ HashDisabled GetDownloadChecksumSettings(Request const& request,
                       algo != ChecksumAlgorithm::kCrc32cAndMD5);
   }
 
-  auto const md5 = request.template GetOption<DisableMD5Hash>();
-  if (md5.has_value()) {
-    disable_md5 = md5.value();
-  }
-  auto const crc32c = request.template GetOption<DisableCrc32cChecksum>();
-  if (crc32c.has_value()) {
-    disable_crc32c = crc32c.value();
-  }
+
   return {disable_md5, disable_crc32c};
 }
 
 template <typename Request>
-HashDisabled GetUploadChecksumSettings(Request const& request,
+HashDisabled GetUploadChecksumSettings(Request const&,
                                        Options const& options) {
   bool disable_md5 = true;
   bool disable_crc32c = false;
@@ -68,14 +61,7 @@ HashDisabled GetUploadChecksumSettings(Request const& request,
                       algo != ChecksumAlgorithm::kCrc32cAndMD5);
   }
 
-  auto const md5 = request.template GetOption<DisableMD5Hash>();
-  if (md5.has_value()) {
-    disable_md5 = md5.value();
-  }
-  auto const crc32c = request.template GetOption<DisableCrc32cChecksum>();
-  if (crc32c.has_value()) {
-    disable_crc32c = crc32c.value();
-  }
+
   return {disable_md5, disable_crc32c};
 }
 

google/cloud/storage/internal/connection_impl.cc

@@ -814,7 +814,7 @@ This is often a problem because:
     preserve data integrity.
 
 Consider using UploadLimit option or Client::WriteObject(). You may also need to disable data
-integrity checks using the DisableMD5Hash() and DisableCrc32cChecksum() options.
+integrity checks using the UploadChecksumValidationOption() option.
 )""";
   } else {
     std::error_code size_err;

google/cloud/storage/internal/grpc/stub.cc

@@ -350,11 +350,9 @@ StatusOr<storage::ObjectMetadata> GrpcStub::InsertObjectMedia(
 
   auto const settings =
       storage::internal::GetUploadChecksumSettings(request, options);
-  auto disable_md5 = storage::DisableMD5Hash(settings.md5);
-  auto disable_crc32c = storage::DisableCrc32cChecksum(settings.crc32c);
   auto hash_function = storage::internal::CreateHashFunction(
-      request.GetOption<storage::Crc32cChecksumValue>(), disable_crc32c,
-      request.GetOption<storage::MD5HashValue>(), disable_md5);
+      request.GetOption<storage::Crc32cChecksumValue>(), settings.crc32c,
+      request.GetOption<storage::MD5HashValue>(), settings.md5);
 
   auto splitter = SplitObjectWriteData<ContentType>(request.payload());
   std::int64_t offset = 0;

google/cloud/storage/internal/hash_function.cc

@@ -29,15 +29,14 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace internal {
 
 std::unique_ptr<HashFunction> CreateHashFunction(
-    Crc32cChecksumValue const& crc32c_value,
-    DisableCrc32cChecksum const& crc32c_disabled, MD5HashValue const& md5_value,
-    DisableMD5Hash const& md5_disabled) {
+    Crc32cChecksumValue const& crc32c_value, bool disable_crc32c,
+    MD5HashValue const& md5_value, bool disable_md5) {
   auto crc32c = std::unique_ptr<HashFunction>();
   auto crc32c_v = crc32c_value.value_or("");
   if (!crc32c_v.empty()) {
     crc32c = std::make_unique<PrecomputedHashFunction>(
         HashValues{/*.crc32c=*/std::move(crc32c_v), /*md5=*/{}});
-  } else if (!crc32c_disabled.value_or(false)) {
+  } else if (!disable_crc32c) {
     crc32c = std::make_unique<Crc32cHashFunction>();
   }
 
@@ -46,7 +45,7 @@ std::unique_ptr<HashFunction> CreateHashFunction(
   if (!md5_v.empty()) {
     md5 = std::make_unique<PrecomputedHashFunction>(
         HashValues{/*.crc32c=*/{}, /*.md5=*/std::move(md5_v)});
-  } else if (!md5_disabled.value_or(false)) {
+  } else if (!disable_md5) {
     md5 = MD5HashFunction::Create();
   }
 
@@ -88,11 +87,9 @@ std::unique_ptr<HashFunction> CreateHashFunction(
 
   auto const settings = GetUploadChecksumSettings(
       request, google::cloud::internal::CurrentOptions());
-  auto disable_md5 = DisableMD5Hash(settings.md5);
-  auto disable_crc32c = DisableCrc32cChecksum(settings.crc32c);
   return CreateHashFunction(request.GetOption<Crc32cChecksumValue>(),
-                            disable_crc32c, request.GetOption<MD5HashValue>(),
-                            disable_md5);
+                            settings.crc32c, request.GetOption<MD5HashValue>(),
+                            settings.md5);
 }
 
 }  // namespace internal

google/cloud/storage/internal/hash_function.h

@@ -88,9 +88,8 @@ class HashFunction {
 
 /// Create a hash function configured by several options.
 std::unique_ptr<HashFunction> CreateHashFunction(
-    Crc32cChecksumValue const& crc32c_value,
-    DisableCrc32cChecksum const& crc32c_disabled, MD5HashValue const& md5_value,
-    DisableMD5Hash const& md5_disabled);
+    Crc32cChecksumValue const& crc32c_value, bool disable_crc32c,
+    MD5HashValue const& md5_value, bool disable_md5);
 
 /// Create a no-op hash function
 std::unique_ptr<HashFunction> CreateNullHashFunction();

google/cloud/storage/internal/hash_function_impl_test.cc

@@ -378,22 +378,19 @@ TEST(HashFunctionImplTest, CreateHashFunctionRead) {
   struct Test {
     std::string crc32c_expected;
     std::string md5_expected;
-    DisableCrc32cChecksum crc32_disabled;
-    DisableMD5Hash md5_disabled;
+    ChecksumAlgorithm validation_algo;
   } cases[]{
-      {"", "", DisableCrc32cChecksum(true), DisableMD5Hash(true)},
-      {"", kQuickFoxMD5Hash, DisableCrc32cChecksum(true),
-       DisableMD5Hash(false)},
-      {kQuickFoxCrc32cChecksum, "", DisableCrc32cChecksum(false),
-       DisableMD5Hash(true)},
-      {kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash, DisableCrc32cChecksum(false),
-       DisableMD5Hash(false)},
+      {"", "", ChecksumAlgorithm::kNone},
+      {"", kQuickFoxMD5Hash, ChecksumAlgorithm::kMD5},
+      {kQuickFoxCrc32cChecksum, "", ChecksumAlgorithm::kCrc32c},
+      {kQuickFoxCrc32cChecksum, kQuickFoxMD5Hash, ChecksumAlgorithm::kCrc32cAndMD5},
   };
 
   for (auto const& test : cases) {
+    google::cloud::internal::OptionsSpan span(
+        Options{}.set<DownloadChecksumValidationOption>(test.validation_algo));
     auto function = CreateHashFunction(
-        ReadObjectRangeRequest("test-bucket", "test-object")
-            .set_multiple_options(test.crc32_disabled, test.md5_disabled));
+        ReadObjectRangeRequest("test-bucket", "test-object"));
     function->Update(kQuickFox);
     auto const actual = std::move(*function).Finish();
     EXPECT_EQ(test.crc32c_expected, actual.crc32c);
@@ -404,20 +401,21 @@ TEST(HashFunctionImplTest, CreateHashFunctionRead) {
 struct UploadTest {
   std::string crc32c_expected;
   std::string md5_expected;
-  DisableCrc32cChecksum crc32_disabled;
+  bool disable_crc32c;
   Crc32cChecksumValue crc32_value;
-  DisableMD5Hash md5_disabled;
+  bool disable_md5;
   MD5HashValue md5_value;
 };
 
 TEST(HashFunctionImplTest, CreateHashFunctionUpload) {
   auto const upload_cases = testing::UploadHashCases();
 
   for (auto const& test : upload_cases) {
+    google::cloud::internal::OptionsSpan span(
+        Options{}.set<UploadChecksumValidationOption>(test.validation_algo));
     auto function = CreateHashFunction(
         ResumableUploadRequest("test-bucket", "test-object")
-            .set_multiple_options(test.crc32_disabled, test.crc32_value,
-                                  test.md5_disabled, test.md5_value));
+            .set_multiple_options(test.crc32_value, test.md5_value));
     function->Update(kQuickFox);
     auto const actual = std::move(*function).Finish();
     EXPECT_EQ(test.crc32c_expected, actual.crc32c);
@@ -428,9 +426,7 @@ TEST(HashFunctionImplTest, CreateHashFunctionUpload) {
 TEST(HashFunctionImplTest, CreateHashFunctionUploadResumedSession) {
   auto function = CreateHashFunction(
       ResumableUploadRequest("test-bucket", "test-object")
-          .set_multiple_options(UseResumableUploadSession("test-session-id"),
-                                DisableCrc32cChecksum(false),
-                                DisableMD5Hash(false)));
+          .set_multiple_options(UseResumableUploadSession("test-session-id")));
   function->Update(kQuickFox);
   auto const actual = std::move(*function).Finish();
   EXPECT_THAT(actual.crc32c, IsEmpty());
@@ -441,8 +437,9 @@ TEST(HashFunctionImplTest, CreateHashFunctionInsertObjectMedia) {
   auto const upload_cases = testing::UploadHashCases();
 
   for (auto const& test : upload_cases) {
-    auto function = CreateHashFunction(test.crc32_value, test.crc32_disabled,
-                                       test.md5_value, test.md5_disabled);
+    bool disable_crc32c = (test.validation_algo == ChecksumAlgorithm::kNone || test.validation_algo == ChecksumAlgorithm::kMD5);
+    bool disable_md5 = (test.validation_algo == ChecksumAlgorithm::kNone || test.validation_algo == ChecksumAlgorithm::kCrc32c);
+    auto function = CreateHashFunction(test.crc32_value, disable_crc32c, test.md5_value, disable_md5);
     ASSERT_STATUS_OK(function->Update(/*offset=*/0, kQuickFox));
     auto const actual = function->Finish();
     EXPECT_EQ(test.crc32c_expected, actual.crc32c);