fix(storage): correctly merge options and handle MD5 for appendable uploads

Author: v-pratap

google/cloud/storage/examples/storage_async_samples.cc

@@ -750,6 +750,8 @@ void CreateAndWriteAppendableObjectWithChecksum(
 
     // Set the expected CRC32C checksum in the current options scope
     // just before calling Finalize().
+    // Note: 548262564U is the pre-computed CRC32C checksum for the string "Some data\n".
+    // If the data changes, this checksum must be updated to match.
     google::cloud::internal::OptionsSpan span(
         google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(548262564U));
     co_return (co_await writer.Finalize(std::move(token))).value();

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

@@ -144,37 +144,30 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) {
   auto is_append = request_.has_append_object_spec() ||
                    request_.write_object_spec().appendable();
   auto current_options = google::cloud::internal::CurrentOptions();
-  
+  auto merged = google::cloud::internal::MergeOptions(
+      current_options, options_ ? *options_ : google::cloud::Options{});
+
   // Default to letting the internal hash function compute and send the checksum.
   auto action = PartialUpload::kFinalizeWithChecksum;
 
-  if (current_options.has<google::cloud::storage::UseCrc32cValueOption>()) {
-    // The user provided a final CRC via OptionsSpan. We manually inject it into the
-    // request. We use `kFinalize` so the internal hash function doesn't overwrite it.
-    write.mutable_object_checksums()->set_crc32c(
-        current_options.get<google::cloud::storage::UseCrc32cValueOption>());
-    action = PartialUpload::kFinalize;
-  } else if (current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
-    auto as_proto = storage_internal::MD5ToProto(
-        current_options.get<google::cloud::storage::UseMD5ValueOption>());
-    if (as_proto) {
-      write.mutable_object_checksums()->set_md5_hash(*as_proto);
-      action = PartialUpload::kFinalize;
-    }
-  } else if (is_append) {
-    if (options_->has<google::cloud::storage::UseCrc32cValueOption>()) {
+  if (is_append || current_options.has<google::cloud::storage::UseCrc32cValueOption>() ||
+      current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
+    if (merged.has<google::cloud::storage::UseCrc32cValueOption>()) {
       write.mutable_object_checksums()->set_crc32c(
-          options_->get<google::cloud::storage::UseCrc32cValueOption>());
-    } else if (options_->has<google::cloud::storage::UseMD5ValueOption>()) {
+          merged.get<google::cloud::storage::UseCrc32cValueOption>());
+    }
+    if (merged.has<google::cloud::storage::UseMD5ValueOption>()) {
       auto as_proto = storage_internal::MD5ToProto(
-          options_->get<google::cloud::storage::UseMD5ValueOption>());
+          merged.get<google::cloud::storage::UseMD5ValueOption>());
       if (as_proto) {
         write.mutable_object_checksums()->set_md5_hash(*as_proto);
       }
     }
     // For appendable uploads, the internal hash function only sees the chunks uploaded
     // in this stream, not the full object. We use `kFinalize` to avoid sending this
     // partial hash, which would otherwise fail validation.
+    // If the user specified a manual expected checksum in `current_options`, we must
+    // also use `kFinalize` so the internal hash function doesn't overwrite it.
     action = PartialUpload::kFinalize;
   }
 

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

@@ -758,7 +758,7 @@ TEST(AsyncWriterConnectionTest, FinalizeAppendableWithExpectedChecksum) {
                   "test-only-algo");
         EXPECT_TRUE(request.has_object_checksums());
         EXPECT_EQ(request.object_checksums().crc32c(),
-                  123456);  // wait, it might be an int in proto
+                  123456);
         return sequencer.PushBack("Write");
       });
   EXPECT_CALL(*mock, Read).WillOnce([&]() {