Skip to content

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

Open
v-pratap wants to merge 3 commits into
mainfrom
append-full-object-checksum
Open

fix(storage): allow setting full checksum at finalize for appendable uploads#1
v-pratap wants to merge 3 commits into
mainfrom
append-full-object-checksum

Conversation

@v-pratap

Copy link
Copy Markdown
Owner

Fixes the checksum validation logic at the end of appendable uploads so users can provide a final expected CRC.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates AsyncWriterConnectionImpl::Finalize to support manually injecting a user-provided CRC32C value via options. The review feedback highlights an inconsistency where current_options is checked in one condition while options_ is checked in another, which could lead to incorrect behavior. Additionally, options_ should be checked for nullptr before dereferencing to prevent potential crashes. A refactored code block is suggested to address these issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +145 to +161
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;
}

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;
  }

@v-pratap v-pratap force-pushed the append-full-object-checksum branch from aab03ae to bf842c3 Compare July 11, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants