Skip to content

fix(storage): allow setting full checksum at finalize for appendable uploads #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
v-pratap wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(storage): allow setting full checksum at finalize for appendable uploads #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d78d42
chore(deps): update actions/checkout action to v7 (#16235)
renovate-bot Jul 10, 2026
982d3bf
fix(storage): allow setting full checksum at finalize for appendable …
v-pratap Jul 11, 2026
bf842c3
fix(storage): allow providing expected CRC32C to Finalize()
v-pratap Jul 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/codeql-analysis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:

steps:
- name: Checkout repository
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
with:
persist-credentials: false
- name: Checkout vcpkg
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/macos-bazel.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
targets:
- //google/cloud/storage/...
steps:
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
with:
ref: ${{ inputs.checkout-ref }}
persist-credentials: false
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/windows-bazel.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:
targets:
- //google/cloud/storage/...
steps:
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
with:
ref: ${{ inputs.checkout-ref }}
persist-credentials: false
Expand Down
42 changes: 42 additions & 0 deletions google/cloud/storage/examples/storage_async_samples.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,40 @@ void CreateAndWriteAppendableObject(google::cloud::storage::AsyncClient& client,
std::cout << "File successfully uploaded " << object.DebugString() << "\n";
}

void CreateAndWriteAppendableObjectWithChecksum(
google::cloud::storage::AsyncClient& client,
std::vector<std::string> const& argv) {
//! [create-and-write-appendable-object-with-checksum]
// [START storage_create_and_write_appendable_object_with_checksum]
namespace gcs = google::cloud::storage;
auto coro = [](gcs::AsyncClient& client, std::string bucket_name,
std::string object_name)
-> google::cloud::future<google::storage::v2::Object> {
auto [writer, token] =
(co_await client.StartAppendableObjectUpload(
gcs::BucketName(std::move(bucket_name)), std::move(object_name)))
.value();
std::cout << "Appendable upload started for object " << object_name << "\n";

token = (co_await writer.Write(std::move(token),
gcs::WritePayload("Some data\n")))
.value();
std::cout << "Wrote some data.\n";

// Set the expected CRC32C checksum in the current options scope
// just before calling Finalize().
google::cloud::internal::OptionsSpan span(
google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(1848151177U));
co_return (co_await writer.Finalize(std::move(token))).value();
};
// [END storage_create_and_write_appendable_object_with_checksum]
//! [create-and-write-appendable-object-with-checksum]
// The example is easier to test and run if we call the coroutine and block
// until it completes..
auto const object = coro(client, argv.at(0), argv.at(1)).get();
std::cout << "File successfully uploaded " << object.DebugString() << "\n";
}

void PauseAndResumeAppendableUpload(google::cloud::storage::AsyncClient& client,
std::vector<std::string> const& argv) {
//! [pause-and-resume-appendable-upload]
Expand Down Expand Up @@ -1035,6 +1069,12 @@ void CreateAndWriteAppendableObject(google::cloud::storage::AsyncClient&,
"coroutines\n";
}

void CreateAndWriteAppendableObjectWithChecksum(
google::cloud::storage::AsyncClient&, std::vector<std::string> const&) {
std::cerr << "AsyncClient::CreateAndWriteAppendableObjectWithChecksum() "
"example requires coroutines\n";
}

void PauseAndResumeAppendableUpload(google::cloud::storage::AsyncClient&,
std::vector<std::string> const&) {
std::cerr << "AsyncClient::PauseAndResumeAppendableUpload() example requires "
Expand Down Expand Up @@ -1505,6 +1545,8 @@ int main(int argc, char* argv[]) try {

make_entry("create-and-write-appendable-object", {},
CreateAndWriteAppendableObject),
make_entry("create-and-write-appendable-object-with-checksum", {},
CreateAndWriteAppendableObjectWithChecksum),
make_entry("pause-and-resume-appendable-upload", {},
PauseAndResumeAppendableUpload),
make_entry("finalize-appendable-object-upload", {},
Expand Down
26 changes: 22 additions & 4 deletions google/cloud/storage/internal/async/writer_connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include "google/cloud/storage/internal/async/write_payload_impl.h"
#include "google/cloud/storage/internal/grpc/ctype_cord_workaround.h"
#include "google/cloud/storage/internal/grpc/object_metadata_parser.h"
#include "google/cloud/storage/internal/grpc/object_request_parser.h"
#include "google/cloud/storage/async/options.h"
#include "google/cloud/internal/make_status.h"

namespace google {
Expand Down Expand Up @@ -138,10 +140,26 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) {

auto p = WritePayloadImpl::GetImpl(payload);
auto size = p.size();
auto action = request_.has_append_object_spec() ||
request_.write_object_spec().appendable()
? PartialUpload::kFinalize
: PartialUpload::kFinalizeWithChecksum;
auto is_append = request_.has_append_object_spec() ||
request_.write_object_spec().appendable();
auto current_options = google::cloud::internal::CurrentOptions();

// 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 (is_append && !options_->has<google::cloud::storage::UseCrc32cValueOption>()) {
// 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 CRC, which would otherwise fail validation.
action = PartialUpload::kFinalize;
}
Comment on lines +145 to +161

@gemini-code-assist gemini-code-assist Bot Jul 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is an inconsistency in how options are checked. In line 150, current_options is checked for UseCrc32cValueOption, but in line 156, options_ is checked instead. If the option is set at the connection level (options_) but not in the current thread-local options (current_options), the CRC value will not be injected, and the appendable upload check will also fail to set action = PartialUpload::kFinalize (since !options_->has... will be false). Additionally, options_ should be checked for nullptr before dereferencing to prevent potential crashes.

  auto current_options = google::cloud::internal::CurrentOptions();
  auto has_crc_option = current_options.has<google::cloud::storage::UseCrc32cValueOption>() ||
                        (options_ && options_->has<google::cloud::storage::UseCrc32cValueOption>());

  // Default to letting the internal hash function compute and send the checksum.
  auto action = PartialUpload::kFinalizeWithChecksum;

  if (has_crc_option) {
    // The user provided a final CRC. We manually inject it into the request.
    // We use `kFinalize` so the internal hash function doesn't overwrite it.
    auto crc = current_options.has<google::cloud::storage::UseCrc32cValueOption>()
                   ? current_options.get<google::cloud::storage::UseCrc32cValueOption>()
                   : options_->get<google::cloud::storage::UseCrc32cValueOption>();
    write.mutable_object_checksums()->set_crc32c(std::move(crc));
    action = PartialUpload::kFinalize;
  } else if (is_append) {
    // 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 CRC, which would otherwise fail validation.
    action = PartialUpload::kFinalize;
  }


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 {
Expand Down
119 changes: 119 additions & 0 deletions google/cloud/storage/internal/async/writer_connection_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@

#include "google/cloud/storage/internal/async/writer_connection_impl.h"
#include "google/cloud/mocks/mock_async_streaming_read_write_rpc.h"
#include "google/cloud/storage/async/options.h"
#include "google/cloud/storage/internal/async/write_object.h"
#include "google/cloud/storage/internal/crc32c.h"
#include "google/cloud/storage/internal/grpc/ctype_cord_workaround.h"
#include "google/cloud/storage/internal/grpc/object_metadata_parser.h"
#include "google/cloud/storage/internal/hash_function_impl.h"
#include "google/cloud/storage/options.h"
#include "google/cloud/storage/testing/canonical_errors.h"
Expand Down Expand Up @@ -744,6 +746,123 @@ TEST(AsyncWriterConnectionTest, FinalizeAppendableNoChecksum) {
next.first.set_value(true);
}

TEST(AsyncWriterConnectionTest, FinalizeAppendableWithExpectedChecksum) {
AsyncSequencer<bool> sequencer;
auto mock = std::make_unique<MockStream>();
EXPECT_CALL(*mock, Cancel).Times(1);
EXPECT_CALL(*mock, Write)
.WillOnce([&](Request const& request, grpc::WriteOptions wopt) {
EXPECT_TRUE(request.finish_write());
EXPECT_TRUE(wopt.is_last_message());
EXPECT_EQ(request.common_object_request_params().encryption_algorithm(),
"test-only-algo");
EXPECT_TRUE(request.has_object_checksums());
EXPECT_EQ(request.object_checksums().crc32c(),
123456); // wait, it might be an int in proto
return sequencer.PushBack("Write");
});
EXPECT_CALL(*mock, Read).WillOnce([&]() {
return sequencer.PushBack("Read").then([](auto f) {
if (!f.get()) return absl::optional<Response>();
return absl::make_optional(MakeTestResponse());
});
});
EXPECT_CALL(*mock, Finish).WillOnce([&] {
return sequencer.PushBack("Finish").then([](auto f) {
if (f.get()) return Status{};
return PermanentError();
});
});
auto hash = std::make_shared<MockHashFunction>();
EXPECT_CALL(*hash, Update(_, An<absl::Cord const&>(), _)).Times(1);
EXPECT_CALL(*hash, Finish)
.WillOnce(Return(storage::internal::HashValues{
storage_internal::Crc32cFromProto(123456), {}}));

auto request = MakeRequest();
request.mutable_write_object_spec()->set_appendable(true);

auto options = internal::MakeImmutableOptions(
Options{}.set<storage::UseCrc32cValueOption>(123456));

auto tested = std::make_unique<AsyncWriterConnectionImpl>(
options, std::move(request), std::move(mock), hash, 1024);
auto response = tested->Finalize(WritePayload{});

auto next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Write");
next.first.set_value(true);
next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Read");
next.first.set_value(true);
auto object = response.get();
EXPECT_THAT(object, IsOkAndHolds(IsProtoEqual(MakeTestObject())))
<< "=" << object->DebugString();

tested = {};
next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Finish");
next.first.set_value(true);
}

TEST(AsyncWriterConnectionTest,
FinalizeAppendableWithExpectedChecksumFromCurrentOptions) {
AsyncSequencer<bool> sequencer;
auto mock = std::make_unique<MockStream>();
EXPECT_CALL(*mock, Cancel).Times(1);
EXPECT_CALL(*mock, Write)
.WillOnce([&](Request const& request, grpc::WriteOptions wopt) {
EXPECT_TRUE(request.finish_write());
EXPECT_TRUE(wopt.is_last_message());
EXPECT_EQ(request.common_object_request_params().encryption_algorithm(),
"test-only-algo");
EXPECT_TRUE(request.has_object_checksums());
EXPECT_EQ(request.object_checksums().crc32c(), 654321);
return sequencer.PushBack("Write");
});
EXPECT_CALL(*mock, Read).WillOnce([&]() {
return sequencer.PushBack("Read").then([](auto f) {
if (!f.get()) return absl::optional<Response>();
return absl::make_optional(MakeTestResponse());
});
});
EXPECT_CALL(*mock, Finish).WillOnce([&] {
return sequencer.PushBack("Finish").then([](auto f) {
if (f.get()) return Status{};
return PermanentError();
});
});
auto hash = std::make_shared<MockHashFunction>();
EXPECT_CALL(*hash, Update(_, An<absl::Cord const&>(), _)).Times(1);
// It shouldn't call Finish() because we use kFinalize!
EXPECT_CALL(*hash, Finish).Times(0);

auto request = MakeRequest();
request.mutable_write_object_spec()->set_appendable(true);

auto tested = std::make_unique<AsyncWriterConnectionImpl>(
TestOptions(), std::move(request), std::move(mock), hash, 1024);

internal::OptionsSpan span(
Options{}.set<storage::UseCrc32cValueOption>(654321));
auto response = tested->Finalize(WritePayload{});

auto next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Write");
next.first.set_value(true);
next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Read");
next.first.set_value(true);
auto object = response.get();
EXPECT_THAT(object, IsOkAndHolds(IsProtoEqual(MakeTestObject())))
<< "=" << object->DebugString();

tested = {};
next = sequencer.PopFrontWithName();
ASSERT_THAT(next.second, "Finish");
next.first.set_value(true);
}

TEST(AsyncWriterConnectionTest, ResumeWithHandle) {
AsyncSequencer<bool> sequencer;
auto mock = std::make_unique<MockStream>();
Expand Down
Loading