Skip to content

Commit 69c280b

Browse files
v-pratapv-pratap
committed
refactor(storage): clarify Finalize() checksum logic to avoid apparent redundancy
1 parent d5b0ca2 commit 69c280b

3 files changed

Lines changed: 145 additions & 21 deletions

File tree

‎google/cloud/storage/examples/storage_async_samples.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -750,8 +750,8 @@ void CreateAndWriteAppendableObjectWithChecksum(
750750

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

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

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@
1313
// limitations under the License.
1414

1515
#include "google/cloud/storage/internal/async/writer_connection_impl.h"
16+
#include "google/cloud/storage/async/options.h"
17+
#include "google/cloud/storage/hashing_options.h"
1618
#include "google/cloud/storage/internal/async/handle_redirect_error.h"
1719
#include "google/cloud/storage/internal/async/partial_upload.h"
1820
#include "google/cloud/storage/internal/async/write_payload_impl.h"
1921
#include "google/cloud/storage/internal/grpc/ctype_cord_workaround.h"
2022
#include "google/cloud/storage/internal/grpc/object_metadata_parser.h"
2123
#include "google/cloud/storage/internal/grpc/object_request_parser.h"
22-
#include "google/cloud/storage/async/options.h"
23-
#include "google/cloud/storage/hashing_options.h"
2424
#include "google/cloud/internal/make_status.h"
2525

2626
namespace google {
@@ -143,31 +143,38 @@ AsyncWriterConnectionImpl::Finalize(storage::WritePayload payload) {
143143
auto size = p.size();
144144
auto is_append = request_.has_append_object_spec() ||
145145
request_.write_object_spec().appendable();
146-
auto current_options = google::cloud::internal::CurrentOptions();
146+
auto const& current_options = google::cloud::internal::CurrentOptions();
147147
auto merged = google::cloud::internal::MergeOptions(
148148
current_options, options_ ? *options_ : google::cloud::Options{});
149149

150-
// Default to letting the internal hash function compute and send the checksum.
150+
// Default to letting the internal hash function compute and send the
151+
// checksum.
151152
auto action = PartialUpload::kFinalizeWithChecksum;
152153

153-
if (is_append || current_options.has<google::cloud::storage::UseCrc32cValueOption>() ||
154-
current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
154+
if (is_append) {
155+
// For appendable uploads, the internal hash function only sees the chunks
156+
// uploaded in this stream, not the full object. We use `kFinalize` to avoid
157+
// sending this partial hash, which would otherwise fail validation.
155158
if (merged.has<google::cloud::storage::UseCrc32cValueOption>()) {
156159
write.mutable_object_checksums()->set_crc32c(
157160
merged.get<google::cloud::storage::UseCrc32cValueOption>());
158161
}
159-
if (merged.has<google::cloud::storage::UseMD5ValueOption>()) {
160-
auto as_proto = storage_internal::MD5ToProto(
161-
merged.get<google::cloud::storage::UseMD5ValueOption>());
162-
if (as_proto) {
163-
write.mutable_object_checksums()->set_md5_hash(*as_proto);
164-
}
162+
action = PartialUpload::kFinalize;
163+
} else if (current_options
164+
.has<google::cloud::storage::UseCrc32cValueOption>() ||
165+
current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
166+
// If the user specified a manual expected checksum dynamically at
167+
// Finalize() via current_options, we manually inject it and use `kFinalize`
168+
// so the internal hash function doesn't overwrite it with its own computed
169+
// hash.
170+
if (current_options.has<google::cloud::storage::UseCrc32cValueOption>()) {
171+
write.mutable_object_checksums()->set_crc32c(
172+
current_options.get<google::cloud::storage::UseCrc32cValueOption>());
173+
}
174+
if (current_options.has<google::cloud::storage::UseMD5ValueOption>()) {
175+
write.mutable_object_checksums()->set_md5_hash(
176+
current_options.get<google::cloud::storage::UseMD5ValueOption>());
165177
}
166-
// For appendable uploads, the internal hash function only sees the chunks uploaded
167-
// in this stream, not the full object. We use `kFinalize` to avoid sending this
168-
// partial hash, which would otherwise fail validation.
169-
// If the user specified a manual expected checksum in `current_options`, we must
170-
// also use `kFinalize` so the internal hash function doesn't overwrite it.
171178
action = PartialUpload::kFinalize;
172179
}
173180

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

Lines changed: 119 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -757,8 +757,7 @@ TEST(AsyncWriterConnectionTest, FinalizeAppendableWithExpectedChecksum) {
757757
EXPECT_EQ(request.common_object_request_params().encryption_algorithm(),
758758
"test-only-algo");
759759
EXPECT_TRUE(request.has_object_checksums());
760-
EXPECT_EQ(request.object_checksums().crc32c(),
761-
123456);
760+
EXPECT_EQ(request.object_checksums().crc32c(), 123456);
762761
return sequencer.PushBack("Write");
763762
});
764763
EXPECT_CALL(*mock, Read).WillOnce([&]() {
@@ -861,6 +860,124 @@ TEST(AsyncWriterConnectionTest,
861860
next.first.set_value(true);
862861
}
863862

863+
TEST(AsyncWriterConnectionTest,
864+
FinalizeNonAppendableWithExpectedChecksumFromCurrentOptions) {
865+
AsyncSequencer<bool> sequencer;
866+
auto mock = std::make_unique<MockStream>();
867+
EXPECT_CALL(*mock, Cancel).Times(1);
868+
EXPECT_CALL(*mock, Write)
869+
.WillOnce([&](Request const& request, grpc::WriteOptions wopt) {
870+
EXPECT_TRUE(request.finish_write());
871+
EXPECT_TRUE(wopt.is_last_message());
872+
EXPECT_EQ(request.common_object_request_params().encryption_algorithm(),
873+
"test-only-algo");
874+
EXPECT_TRUE(request.has_object_checksums());
875+
EXPECT_EQ(request.object_checksums().crc32c(), 654321);
876+
return sequencer.PushBack("Write");
877+
});
878+
EXPECT_CALL(*mock, Read).WillOnce([&]() {
879+
return sequencer.PushBack("Read").then([](auto f) {
880+
if (!f.get()) return absl::optional<Response>();
881+
return absl::make_optional(MakeTestResponse());
882+
});
883+
});
884+
EXPECT_CALL(*mock, Finish).WillOnce([&] {
885+
return sequencer.PushBack("Finish").then([](auto f) {
886+
if (f.get()) return Status{};
887+
return PermanentError();
888+
});
889+
});
890+
auto hash = std::make_shared<MockHashFunction>();
891+
EXPECT_CALL(*hash, Update(_, An<absl::Cord const&>(), _)).Times(1);
892+
// It shouldn't call Finish() because we use kFinalize!
893+
EXPECT_CALL(*hash, Finish).Times(0);
894+
895+
auto request = MakeRequest();
896+
// Ensure it's explicitly not appendable.
897+
request.mutable_write_object_spec()->set_appendable(false);
898+
899+
auto tested = std::make_unique<AsyncWriterConnectionImpl>(
900+
TestOptions(), std::move(request), std::move(mock), hash, 1024);
901+
902+
internal::OptionsSpan span(
903+
Options{}.set<storage::UseCrc32cValueOption>(654321));
904+
auto response = tested->Finalize(WritePayload{});
905+
906+
auto next = sequencer.PopFrontWithName();
907+
ASSERT_THAT(next.second, "Write");
908+
next.first.set_value(true);
909+
next = sequencer.PopFrontWithName();
910+
ASSERT_THAT(next.second, "Read");
911+
next.first.set_value(true);
912+
auto object = response.get();
913+
EXPECT_THAT(object, IsOkAndHolds(IsProtoEqual(MakeTestObject())))
914+
<< "=" << object->DebugString();
915+
916+
tested = {};
917+
next = sequencer.PopFrontWithName();
918+
ASSERT_THAT(next.second, "Finish");
919+
next.first.set_value(true);
920+
}
921+
922+
TEST(AsyncWriterConnectionTest,
923+
FinalizeNonAppendableWithExpectedMD5FromCurrentOptions) {
924+
AsyncSequencer<bool> sequencer;
925+
auto mock = std::make_unique<MockStream>();
926+
EXPECT_CALL(*mock, Cancel).Times(1);
927+
EXPECT_CALL(*mock, Write)
928+
.WillOnce([&](Request const& request, grpc::WriteOptions wopt) {
929+
EXPECT_TRUE(request.finish_write());
930+
EXPECT_TRUE(wopt.is_last_message());
931+
EXPECT_EQ(request.common_object_request_params().encryption_algorithm(),
932+
"test-only-algo");
933+
EXPECT_TRUE(request.has_object_checksums());
934+
EXPECT_EQ(request.object_checksums().md5_hash(), "test-md5");
935+
return sequencer.PushBack("Write");
936+
});
937+
EXPECT_CALL(*mock, Read).WillOnce([&]() {
938+
return sequencer.PushBack("Read").then([](auto f) {
939+
if (!f.get()) return absl::optional<Response>();
940+
return absl::make_optional(MakeTestResponse());
941+
});
942+
});
943+
EXPECT_CALL(*mock, Finish).WillOnce([&] {
944+
return sequencer.PushBack("Finish").then([](auto f) {
945+
if (f.get()) return Status{};
946+
return PermanentError();
947+
});
948+
});
949+
auto hash = std::make_shared<MockHashFunction>();
950+
EXPECT_CALL(*hash, Update(_, An<absl::Cord const&>(), _)).Times(1);
951+
// It shouldn't call Finish() because we use kFinalize!
952+
EXPECT_CALL(*hash, Finish).Times(0);
953+
954+
auto request = MakeRequest();
955+
// Ensure it's explicitly not appendable.
956+
request.mutable_write_object_spec()->set_appendable(false);
957+
958+
auto tested = std::make_unique<AsyncWriterConnectionImpl>(
959+
TestOptions(), std::move(request), std::move(mock), hash, 1024);
960+
961+
internal::OptionsSpan span(
962+
Options{}.set<storage::UseMD5ValueOption>("test-md5"));
963+
auto response = tested->Finalize(WritePayload{});
964+
965+
auto next = sequencer.PopFrontWithName();
966+
ASSERT_THAT(next.second, "Write");
967+
next.first.set_value(true);
968+
next = sequencer.PopFrontWithName();
969+
ASSERT_THAT(next.second, "Read");
970+
next.first.set_value(true);
971+
auto object = response.get();
972+
EXPECT_THAT(object, IsOkAndHolds(IsProtoEqual(MakeTestObject())))
973+
<< "=" << object->DebugString();
974+
975+
tested = {};
976+
next = sequencer.PopFrontWithName();
977+
ASSERT_THAT(next.second, "Finish");
978+
next.first.set_value(true);
979+
}
980+
864981
TEST(AsyncWriterConnectionTest, ResumeWithHandle) {
865982
AsyncSequencer<bool> sequencer;
866983
auto mock = std::make_unique<MockStream>();

0 commit comments

Comments
 (0)