revert: revert previous appendable upload checksum validation implementation

Author: v-pratap

google/cloud/storage/internal/async/connection_impl_appendable_upload_test.cc

@@ -631,262 +631,6 @@ TEST_F(AsyncConnectionImplAppendableTest, AppendableUploadRedirectNoHandle) {
   next.first.set_value(true);
 }
 
-TEST_F(AsyncConnectionImplAppendableTest,
-       StartAppendableObjectUploadWithChecksum) {
-  auto constexpr kRequestText = R"pb(
-    write_object_spec {
-      resource {
-        bucket: "projects/_/buckets/test-bucket"
-        name: "test-object"
-        content_type: "text/plain"
-      }
-    }
-  )pb";
-  AsyncSequencer<bool> sequencer;
-  auto mock = std::make_shared<storage::testing::MockStorageStub>();
-
-  google::storage::v2::Object initial_resource;
-  initial_resource.set_bucket("projects/_/buckets/test-bucket");
-  initial_resource.set_name("test-object");
-  initial_resource.set_size(1024);
-  initial_resource.mutable_checksums()->set_crc32c(12345);  // Some dummy CRC
-
-  auto stream = std::make_unique<MockAsyncBidiWriteObjectStream>();
-  EXPECT_CALL(*stream, Start).WillOnce([&] {
-    return sequencer.PushBack("Start");
-  });
-
-  EXPECT_CALL(*stream, Read)
-      .WillOnce([&, initial_resource] {
-        return sequencer.PushBack("Read(Takeover)")
-            .then([initial_resource](auto) {
-              auto response = google::storage::v2::BidiWriteObjectResponse{};
-              *response.mutable_resource() = initial_resource;
-              return absl::make_optional(std::move(response));
-            });
-      })
-      .WillOnce([&, initial_resource] {
-        return sequencer.PushBack("Read(FinalObject)")
-            .then([initial_resource](auto) {
-              auto response = google::storage::v2::BidiWriteObjectResponse{};
-              *response.mutable_resource() = initial_resource;
-              response.mutable_resource()->set_size(
-                  initial_resource.size() + 9);  // "some data" size is 9
-              return absl::make_optional(std::move(response));
-            });
-      });
-
-  EXPECT_CALL(*stream, Cancel).Times(1);
-  EXPECT_CALL(*stream, Finish).WillOnce([&] {
-    return sequencer.PushBack("Finish").then([](auto) { return Status{}; });
-  });
-
-  EXPECT_CALL(*stream, Write)
-      .WillOnce([&](google::storage::v2::BidiWriteObjectRequest const& request,
-                    grpc::WriteOptions wopt) {
-        EXPECT_TRUE(request.state_lookup());
-        EXPECT_FALSE(wopt.is_last_message());
-        return sequencer.PushBack("Write(StateLookup)");
-      })
-      .WillOnce(
-          [&](google::storage::v2::BidiWriteObjectRequest const& /*request*/,
-              grpc::WriteOptions wopt) {
-            EXPECT_FALSE(wopt.is_last_message());
-            return sequencer.PushBack("Write(data)");
-          })
-      .WillOnce([&](google::storage::v2::BidiWriteObjectRequest const& request,
-                    grpc::WriteOptions wopt) {
-        EXPECT_TRUE(request.finish_write());
-        EXPECT_TRUE(wopt.is_last_message());
-        // Here we expect full checksums to be set because we had the resource
-        // in takeover.
-        EXPECT_TRUE(request.has_object_checksums());
-        auto expected_crc =
-            google::cloud::storage_internal::ExtendCrc32c(12345, "some data");
-        EXPECT_EQ(request.object_checksums().crc32c(), expected_crc);
-        return sequencer.PushBack("Write(Finalize)");
-      });
-
-  EXPECT_CALL(*mock, AsyncBidiWriteObject).WillOnce([&] {
-    return std::unique_ptr<AsyncBidiWriteObjectStream>(std::move(stream));
-  });
-
-  internal::AutomaticallyCreatedBackgroundThreads pool(1);
-  // Enable CRC32C validation in options
-  auto options = TestOptions().set<storage::EnableCrc32cValidationOption>(true);
-  auto connection = MakeTestConnection(pool.cq(), mock, options);
-
-  auto request = google::storage::v2::BidiWriteObjectRequest{};
-  ASSERT_TRUE(TextFormat::ParseFromString(kRequestText, &request));
-  request.mutable_write_object_spec()->set_appendable(true);
-
-  auto pending = connection->StartAppendableObjectUpload(
-      {std::move(request), connection->options()});
-
-  auto next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Start");
-  next.first.set_value(true);
-
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(StateLookup)");
-  next.first.set_value(true);
-
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Read(Takeover)");
-  next.first.set_value(true);
-
-  auto r = pending.get();
-  ASSERT_STATUS_OK(r);
-  auto writer = *std::move(r);
-
-  // Write some data.
-  auto w1 = writer->Write(storage::WritePayload("some data"));
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(data)");
-  next.first.set_value(true);
-  EXPECT_STATUS_OK(w1.get());
-
-  // Finalize the upload.
-  auto w2 = writer->Finalize({});
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(Finalize)");
-  next.first.set_value(true);
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Read(FinalObject)");
-  next.first.set_value(true);
-
-  auto response = w2.get();
-  ASSERT_STATUS_OK(response);
-
-  writer.reset();
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Finish");
-  next.first.set_value(true);
-}
-
-// TODO(#16174): Figure out why this test fails to compile in MSVC.
-#ifndef _WIN32
-
-TEST_F(AsyncConnectionImplAppendableTest,
-       ResumeAppendableObjectUploadWithChecksum) {
-  auto constexpr kRequestText = R"pb(
-    append_object_spec { object: "test-object" }
-  )pb";
-  AsyncSequencer<bool> sequencer;
-  auto mock = std::make_shared<storage::testing::MockStorageStub>();
-
-  constexpr std::int64_t kPersistedSize = 16384;
-  constexpr std::uint32_t kPersistedCrc = 12345;
-
-  auto stream = std::make_unique<MockAsyncBidiWriteObjectStream>();
-  EXPECT_CALL(*stream, Start).WillOnce([&] {
-    return sequencer.PushBack("Start");
-  });
-
-  EXPECT_CALL(*stream, Read)
-      .WillOnce([&] {
-        return sequencer.PushBack("Read(PersistedSize)").then([](auto) {
-          auto response = google::storage::v2::BidiWriteObjectResponse{};
-          response.set_persisted_size(kPersistedSize);
-          response.mutable_persisted_data_checksums()->set_crc32c(
-              kPersistedCrc);
-          return absl::make_optional(std::move(response));
-        });
-      })
-      .WillOnce([&] {
-        return sequencer.PushBack("Read(FinalObject)").then([](auto) {
-          auto response = google::storage::v2::BidiWriteObjectResponse{};
-          auto object = google::storage::v2::Object{};
-          object.set_bucket("projects/_/buckets/test-bucket");
-          object.set_name("test-object");
-          object.set_size(kPersistedSize + 9);
-          *response.mutable_resource() = std::move(object);
-          return absl::make_optional(std::move(response));
-        });
-      });
-
-  EXPECT_CALL(*stream, Cancel).Times(1);
-  EXPECT_CALL(*stream, Finish).WillOnce([&] {
-    return sequencer.PushBack("Finish").then([](auto) { return Status{}; });
-  });
-
-  EXPECT_CALL(*stream, Write)
-      .WillOnce([&](google::storage::v2::BidiWriteObjectRequest const& request,
-                    grpc::WriteOptions wopt) {
-        EXPECT_TRUE(request.state_lookup());
-        EXPECT_FALSE(wopt.is_last_message());
-        return sequencer.PushBack("Write(StateLookup)");
-      })
-      .WillOnce(
-          [&](google::storage::v2::BidiWriteObjectRequest const& /*request*/,
-              grpc::WriteOptions wopt) {
-            EXPECT_FALSE(wopt.is_last_message());
-            return sequencer.PushBack("Write(data)");
-          })
-      .WillOnce([&](google::storage::v2::BidiWriteObjectRequest const& request,
-                    grpc::WriteOptions wopt) {
-        EXPECT_TRUE(request.finish_write());
-        EXPECT_TRUE(wopt.is_last_message());
-        EXPECT_TRUE(request.has_object_checksums());
-        EXPECT_EQ(request.object_checksums().crc32c(), 2901820631);
-        return sequencer.PushBack("Write(Finalize)");
-      });
-
-  EXPECT_CALL(*mock, AsyncBidiWriteObject)
-      .WillOnce([&](auto const&, auto, auto) {
-        return std::unique_ptr<AsyncBidiWriteObjectStream>(std::move(stream));
-      });
-
-  internal::AutomaticallyCreatedBackgroundThreads pool(1);
-  auto options = TestOptions().set<storage::EnableCrc32cValidationOption>(true);
-  auto connection = MakeTestConnection(pool.cq(), mock, options);
-
-  auto request = google::storage::v2::BidiWriteObjectRequest{};
-  ASSERT_TRUE(TextFormat::ParseFromString(kRequestText, &request));
-  auto pending = connection->ResumeAppendableObjectUpload(
-      {std::move(request), connection->options()});
-
-  auto next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Start");
-  next.first.set_value(true);
-
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(StateLookup)");
-  next.first.set_value(true);
-
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Read(PersistedSize)");
-  next.first.set_value(true);
-
-  auto r = pending.get();
-  ASSERT_STATUS_OK(r);
-  auto writer = *std::move(r);
-
-  auto w1 = writer->Write(storage::WritePayload("some data"));
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(data)");
-  next.first.set_value(true);
-  EXPECT_STATUS_OK(w1.get());
-
-  auto w2 = writer->Finalize({});
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Write(Finalize)");
-  next.first.set_value(true);
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Read(FinalObject)");
-  next.first.set_value(true);
-
-  auto response = w2.get();
-  ASSERT_STATUS_OK(response);
-
-  writer.reset();
-  next = sequencer.PopFrontWithName();
-  EXPECT_EQ(next.second, "Finish");
-  next.first.set_value(true);
-}
-
-#endif
-
 }  // namespace
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage_internal

google/cloud/storage/internal/async/writer_connection_impl.cc

@@ -146,15 +146,10 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) {
 
   auto p = WritePayloadImpl::GetImpl(payload);
   auto size = p.size();
-  auto action = PartialUpload::kFinalizeWithChecksum;
-  if (request_.has_append_object_spec() ||
-      request_.write_object_spec().appendable()) {
-    if (!absl::holds_alternative<google::storage::v2::Object>(
-            persisted_state_) &&
-        !persisted_data_checksums_.has_value()) {
-      action = PartialUpload::kFinalize;
-    }
-  }
+  auto action = request_.has_append_object_spec() ||
+                        request_.write_object_spec().appendable()
+                    ? PartialUpload::kFinalize
+                    : PartialUpload::kFinalizeWithChecksum;
   auto coro = PartialUpload::Call(impl_, hash_function_, std::move(write),
                                   std::move(p), std::move(action));
   return coro->Start().then([coro, size, this](auto f) mutable {

google/cloud/storage/internal/async/writer_connection_resumed.cc

@@ -508,15 +508,8 @@ class AsyncWriterConnectionResumedState
     }
 
     // Recreate the underlying stream if still active.
-    auto hash = hash_function_;
-    if (checksums && checksums->has_crc32c()) {
-      hash = std::make_shared<
-          ::google::cloud::storage::internal::Crc32cHashFunction>(
-          checksums->crc32c(), persisted_offset);
-    }
-
     impl_ = std::make_unique<AsyncWriterConnectionImpl>(
-        options_, initial_request_, std::move(res->stream), std::move(hash),
+        options_, initial_request_, std::move(res->stream), hash_function_,
         persisted_offset, false, checksums);
     // OnQuery will restart the WriteLoop if necessary.
     OnQuery(std::move(lk), persisted_offset);