fix(storage): allow setting full checksum at finalize for appendable uploads#1
fix(storage): allow setting full checksum at finalize for appendable uploads#1v-pratap wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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;
}7e5ced2 to
982d3bf
Compare
aab03ae to
bf842c3
Compare
Fixes the checksum validation logic at the end of appendable uploads so users can provide a final expected CRC.