From 0b03f60bdf16311c48f63068c29ce781c958496c Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Mon, 28 Sep 2026 09:44:11 +0000 Subject: [PATCH 1/3] fix(storage): preserve metadata in ReadAll() for 0-byte objects `ReadPayloadImpl::Accumulate()` replaced the accumulated payload with the next one whenever the accumulated payload had no data, ignoring whether it carried object metadata. For a 0-byte object the first payload has empty contents but valid metadata, and the synthetic end-of-stream `ReadPayload{}` then overwrote it, so `ReadAll()` and `AsyncClient::ReadObjectRange()` returned `metadata() == std::nullopt`. Only take the replace fast path when the accumulated payload has neither data nor metadata. Add unit tests for `Accumulate()`, `ReadAll()`, and an end-to-end `ReadObjectRange()` test with a mocked 0-byte gRPC stream. --- google/cloud/storage/async/read_all_test.cc | 53 +++++++++++++++ .../async/connection_impl_read_test.cc | 61 ++++++++++++++++++ .../internal/async/read_payload_impl.h | 5 +- .../internal/async/read_payload_impl_test.cc | 64 +++++++++++++++++++ 4 files changed, 182 insertions(+), 1 deletion(-) diff --git a/google/cloud/storage/async/read_all_test.cc b/google/cloud/storage/async/read_all_test.cc index edbec521b69a7..dd0dc1d03a662 100644 --- a/google/cloud/storage/async/read_all_test.cc +++ b/google/cloud/storage/async/read_all_test.cc @@ -30,6 +30,7 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN using ::google::cloud::storage::testing::canonical_errors::PermanentError; using ::google::cloud::storage_mocks::MockAsyncReaderConnection; +using ::google::cloud::testing_util::IsOk; using ::google::cloud::testing_util::IsProtoEqual; using ::google::cloud::testing_util::StatusIs; using ::testing::ElementsAre; @@ -149,6 +150,58 @@ TEST(ReadAll, Empty) { EXPECT_THAT(payload->contents(), IsEmpty()); } +namespace { +google::storage::v2::Object MakeZeroByteTestObject() { + google::storage::v2::Object object; + object.set_name("test-only-name"); + object.set_generation(123456789); + object.set_size(0); + return object; +} +} // namespace + +// b/565852217: reading a 0-byte object must preserve the object metadata. +TEST(ReadAll, ZeroBytePayloadPreservesMetadata) { + auto mock = std::make_unique(); + EXPECT_CALL(*mock, Read) + .WillOnce([] { + return make_ready_future(ReadResponse( + ReadPayload(std::string{}).set_metadata(MakeZeroByteTestObject()))); + }) + .WillOnce([] { return make_ready_future(ReadResponse(Status{})); }); + + AsyncToken token = storage_internal::MakeAsyncToken(mock.get()); + StatusOr payload = + ReadAll(AsyncReader(std::move(mock)), std::move(token)).get(); + ASSERT_THAT(payload, IsOk()); + EXPECT_THAT(payload->contents(), IsEmpty()); + EXPECT_THAT(payload->metadata(), + Optional(IsProtoEqual(MakeZeroByteTestObject()))); +} + +// b/565852217: an empty first payload with metadata, followed by data, must +// preserve both the metadata and the data. +TEST(ReadAll, EmptyPayloadWithMetadataThenData) { + auto mock = std::make_unique(); + EXPECT_CALL(*mock, Read) + .WillOnce([] { + return make_ready_future(ReadResponse( + ReadPayload(std::string{}).set_metadata(MakeZeroByteTestObject()))); + }) + .WillOnce([] { + return make_ready_future(ReadResponse(ReadPayload("test-message-1"))); + }) + .WillOnce([] { return make_ready_future(ReadResponse(Status{})); }); + + AsyncToken token = storage_internal::MakeAsyncToken(mock.get()); + StatusOr payload = + ReadAll(AsyncReader(std::move(mock)), std::move(token)).get(); + ASSERT_THAT(payload, IsOk()); + EXPECT_THAT(payload->contents(), ElementsAre("test-message-1")); + EXPECT_THAT(payload->metadata(), + Optional(IsProtoEqual(MakeZeroByteTestObject()))); +} + TEST(ReadAll, Error) { auto mock = std::make_unique(); EXPECT_CALL(*mock, Read) diff --git a/google/cloud/storage/internal/async/connection_impl_read_test.cc b/google/cloud/storage/internal/async/connection_impl_read_test.cc index 1f7531d6510ca..13a5ad59a8196 100644 --- a/google/cloud/storage/internal/async/connection_impl_read_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_read_test.cc @@ -60,6 +60,7 @@ using ::testing::AllOf; using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::IsEmpty; +using ::testing::Optional; using ::testing::ResultOf; using ::testing::Return; using ::testing::VariantWith; @@ -626,6 +627,66 @@ TEST_F(AsyncConnectionImplTest, ReadObjectRangePermanentError) { EXPECT_THAT(pending.get(), StatusIs(PermanentError().code())); } +// b/565852217: `ReadObjectRange()` on a 0-byte object must return the object +// metadata sent by the service, even though there is no data. +TEST_F(AsyncConnectionImplTest, ReadObjectRangeZeroByteObjectKeepsMetadata) { + auto constexpr kMetadata = R"pb( + bucket: "projects/_/buckets/test-bucket" + name: "test-object" + generation: 123456789 + metageneration: 1 + size: 0 + )pb"; + google::storage::v2::Object expected; + ASSERT_TRUE(TextFormat::ParseFromString(kMetadata, &expected)); + + AsyncSequencer sequencer; + auto mock = std::make_shared(); + EXPECT_CALL(*mock, AsyncReadObject).WillOnce([&] { + auto stream = std::make_unique(); + EXPECT_CALL(*stream, Start).WillOnce([&] { + return sequencer.PushBack("Start"); + }); + EXPECT_CALL(*stream, Read) + .WillOnce([&] { + return sequencer.PushBack("Read").then([&](auto) { + // A 0-byte object: metadata, but no `checksummed_data`. + google::storage::v2::ReadObjectResponse response; + *response.mutable_metadata() = expected; + return std::make_optional(response); + }); + }) + .WillOnce([&] { + return sequencer.PushBack("Read").then([](auto) { + return std::optional(); + }); + }); + EXPECT_CALL(*stream, Finish).WillOnce([&] { + return sequencer.PushBack("Finish").then([](auto) { return Status{}; }); + }); + return std::unique_ptr(std::move(stream)); + }); + + internal::AutomaticallyCreatedBackgroundThreads pool(1); + auto connection = + MakeTestConnection(pool.cq(), mock, + Options{}.set( + std::chrono::seconds(0))); + future> pending = connection->ReadObjectRange( + {google::storage::v2::ReadObjectRequest{}, connection->options()}); + + for (auto const* name : {"Start", "Read", "Read", "Finish"}) { + auto next = sequencer.PopFrontWithName(); + EXPECT_EQ(next.second, name); + next.first.set_value(true); + } + + StatusOr payload = pending.get(); + ASSERT_THAT(payload, IsOk()); + EXPECT_THAT(payload->contents(), IsEmpty()); + EXPECT_THAT(payload->metadata(), Optional(IsProtoEqual(expected))); +} + TEST_F(AsyncConnectionImplTest, ReadObjectDetectBadMessageChecksum) { AsyncSequencer sequencer; auto make_bad_checksum_stream = [&](AsyncSequencer& sequencer) { diff --git a/google/cloud/storage/internal/async/read_payload_impl.h b/google/cloud/storage/internal/async/read_payload_impl.h index b49614546f245..acecb0fd56027 100644 --- a/google/cloud/storage/internal/async/read_payload_impl.h +++ b/google/cloud/storage/internal/async/read_payload_impl.h @@ -52,7 +52,10 @@ struct ReadPayloadImpl { /// Append the data from @p rhs to @p lhs. static void Accumulate(storage::ReadPayload& lhs, storage::ReadPayload&& rhs) { - if (lhs.impl_.empty()) { + // Only replace `lhs` if it holds no data *and* no metadata. A 0-byte + // object (or range) produces a payload with empty contents but valid + // metadata, which must not be discarded by later (e.g. EOF) payloads. + if (lhs.impl_.empty() && !lhs.metadata_.has_value()) { lhs = std::move(rhs); return; } diff --git a/google/cloud/storage/internal/async/read_payload_impl_test.cc b/google/cloud/storage/internal/async/read_payload_impl_test.cc index e206e2185abd0..925d73ce4cd84 100644 --- a/google/cloud/storage/internal/async/read_payload_impl_test.cc +++ b/google/cloud/storage/internal/async/read_payload_impl_test.cc @@ -25,7 +25,9 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { using ::google::cloud::testing_util::IsProtoEqual; +using ::testing::AllOf; using ::testing::ElementsAre; +using ::testing::Field; using ::testing::IsEmpty; using ::testing::Optional; using ::testing::Pair; @@ -90,6 +92,68 @@ TEST(ReadPayload, Reset) { EXPECT_THAT(actual.headers(), IsEmpty()); } +TEST(ReadPayload, AccumulateIntoDefault) { + google::storage::v2::Object const resource = MakeTestObject(); + storage::ReadPayload actual; + ReadPayloadImpl::Accumulate(actual, ReadPayloadImpl::Make(absl::Cord(kQuick)) + .set_metadata(resource) + .set_offset(1024)); + EXPECT_THAT(actual.contents(), ElementsAre(absl::string_view(kQuick))); + EXPECT_THAT(actual.metadata(), Optional(IsProtoEqual(resource))); + EXPECT_EQ(actual.offset(), 1024); +} + +TEST(ReadPayload, AccumulateAppendsData) { + google::storage::v2::Object const resource = MakeTestObject(); + storage::ReadPayload actual = ReadPayloadImpl::Make(absl::Cord(kQuick)) + .set_metadata(resource) + .set_offset(1024); + ReadPayloadImpl::Accumulate( + actual, ReadPayloadImpl::Make(absl::Cord(kQuick)).set_offset(2048)); + EXPECT_THAT(actual.contents(), ElementsAre(absl::string_view(kQuick), + absl::string_view(kQuick))); + EXPECT_THAT(actual.metadata(), Optional(IsProtoEqual(resource))); + EXPECT_EQ(actual.offset(), 1024); +} + +// b/565852217: a 0-byte payload with metadata must survive the EOF payload. +TEST(ReadPayload, AccumulateEmptyWithMetadataKeepsMetadata) { + google::storage::v2::Object const resource = MakeTestObject(); + storage::ReadPayload actual = + ReadPayloadImpl::Make(absl::Cord()).set_metadata(resource); + ReadPayloadImpl::Accumulate(actual, storage::ReadPayload{}); + EXPECT_THAT(actual.contents(), IsEmpty()); + EXPECT_EQ(actual.size(), 0); + EXPECT_THAT(actual.metadata(), Optional(IsProtoEqual(resource))); +} + +// b/565852217: a 0-byte payload with metadata followed by data keeps the +// metadata, offset, and object hashes from the first payload. +TEST(ReadPayload, AccumulateEmptyWithMetadataThenData) { + google::storage::v2::Object const resource = MakeTestObject(); + storage::ReadPayload actual = ReadPayloadImpl::Make(absl::Cord()) + .set_metadata(resource) + .set_offset(1024); + ReadPayloadImpl::SetObjectHashes( + actual, storage::internal::HashValues{"test-crc32c", "test-md5"}); + ReadPayloadImpl::Accumulate( + actual, ReadPayloadImpl::Make(absl::Cord(kQuick)).set_offset(1024)); + EXPECT_THAT(actual.contents(), ElementsAre(absl::string_view(kQuick))); + EXPECT_THAT(actual.metadata(), Optional(IsProtoEqual(resource))); + EXPECT_EQ(actual.offset(), 1024); + EXPECT_THAT(ReadPayloadImpl::GetObjectHashes(actual), + Optional(AllOf( + Field(&storage::internal::HashValues::crc32c, "test-crc32c"), + Field(&storage::internal::HashValues::md5, "test-md5")))); +} + +TEST(ReadPayload, AccumulateEmptyIntoEmpty) { + storage::ReadPayload actual; + ReadPayloadImpl::Accumulate(actual, storage::ReadPayload{}); + EXPECT_THAT(actual.contents(), IsEmpty()); + EXPECT_FALSE(actual.metadata().has_value()); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal From ed6d478d60eb27d26ffe185f63e7f0fa9c7eea4a Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Mon, 28 Sep 2026 09:51:15 +0000 Subject: [PATCH 2/3] Address review: only replace a fully default ReadPayload in Accumulate() --- .../internal/async/read_payload_impl.h | 18 ++++++++++++---- .../internal/async/read_payload_impl_test.cc | 21 +++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/internal/async/read_payload_impl.h b/google/cloud/storage/internal/async/read_payload_impl.h index acecb0fd56027..f8b6fe79e3458 100644 --- a/google/cloud/storage/internal/async/read_payload_impl.h +++ b/google/cloud/storage/internal/async/read_payload_impl.h @@ -52,10 +52,11 @@ struct ReadPayloadImpl { /// Append the data from @p rhs to @p lhs. static void Accumulate(storage::ReadPayload& lhs, storage::ReadPayload&& rhs) { - // Only replace `lhs` if it holds no data *and* no metadata. A 0-byte - // object (or range) produces a payload with empty contents but valid - // metadata, which must not be discarded by later (e.g. EOF) payloads. - if (lhs.impl_.empty() && !lhs.metadata_.has_value()) { + // Only replace `lhs` if it is in its default-constructed state. A 0-byte + // object (or range) produces a payload with empty contents, but it may + // carry metadata, headers, an offset, or object hashes. Those must not be + // discarded by later (e.g. end-of-stream) payloads. + if (IsDefault(lhs)) { lhs = std::move(rhs); return; } @@ -78,6 +79,15 @@ struct ReadPayloadImpl { storage::ReadPayload new_data) { payload.impl_.Append(std::move(new_data.impl_)); } + + private: + /// Returns true if @p p is equivalent to a default-constructed payload. + static bool IsDefault(storage::ReadPayload const& p) { + // Note: read `object_hash_values_` directly, `GetObjectHashes()` moves + // the value out of the payload. + return p.impl_.empty() && p.offset_ == 0 && !p.metadata_.has_value() && + p.headers_.empty() && !p.object_hash_values_.has_value(); + } }; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/async/read_payload_impl_test.cc b/google/cloud/storage/internal/async/read_payload_impl_test.cc index 925d73ce4cd84..97cf6d6768583 100644 --- a/google/cloud/storage/internal/async/read_payload_impl_test.cc +++ b/google/cloud/storage/internal/async/read_payload_impl_test.cc @@ -147,6 +147,27 @@ TEST(ReadPayload, AccumulateEmptyWithMetadataThenData) { Field(&storage::internal::HashValues::md5, "test-md5")))); } +// An empty payload without metadata may still carry headers, an offset, or +// object hashes. None of these should be discarded by later payloads. +TEST(ReadPayload, AccumulateEmptyWithoutMetadataKeepsOtherFields) { + storage::ReadPayload actual = ReadPayloadImpl::Make(absl::Cord()) + .set_headers({{"k1", "v1"}}) + .set_offset(1024); + ReadPayloadImpl::SetObjectHashes( + actual, storage::internal::HashValues{"test-crc32c", "test-md5"}); + ReadPayloadImpl::Accumulate( + actual, ReadPayloadImpl::Make(absl::Cord(kQuick)).set_offset(1024)); + ReadPayloadImpl::Accumulate(actual, storage::ReadPayload{}); + EXPECT_THAT(actual.contents(), ElementsAre(absl::string_view(kQuick))); + EXPECT_FALSE(actual.metadata().has_value()); + EXPECT_THAT(actual.headers(), UnorderedElementsAre(Pair("k1", "v1"))); + EXPECT_EQ(actual.offset(), 1024); + EXPECT_THAT(ReadPayloadImpl::GetObjectHashes(actual), + Optional(AllOf( + Field(&storage::internal::HashValues::crc32c, "test-crc32c"), + Field(&storage::internal::HashValues::md5, "test-md5")))); +} + TEST(ReadPayload, AccumulateEmptyIntoEmpty) { storage::ReadPayload actual; ReadPayloadImpl::Accumulate(actual, storage::ReadPayload{}); From d951f55935f442421c46d0f9384ff8983b5a1699 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 1 Oct 2026 08:54:45 +0000 Subject: [PATCH 3/3] Address review: drop bug IDs from test comments, remove stale comment --- google/cloud/storage/async/read_all_test.cc | 6 +++--- .../storage/internal/async/connection_impl_read_test.cc | 6 ++---- .../cloud/storage/internal/async/read_payload_impl_test.cc | 6 +++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/google/cloud/storage/async/read_all_test.cc b/google/cloud/storage/async/read_all_test.cc index dd0dc1d03a662..65d6e0757bb1a 100644 --- a/google/cloud/storage/async/read_all_test.cc +++ b/google/cloud/storage/async/read_all_test.cc @@ -160,7 +160,7 @@ google::storage::v2::Object MakeZeroByteTestObject() { } } // namespace -// b/565852217: reading a 0-byte object must preserve the object metadata. +// Reading a 0-byte object must preserve the object metadata. TEST(ReadAll, ZeroBytePayloadPreservesMetadata) { auto mock = std::make_unique(); EXPECT_CALL(*mock, Read) @@ -179,8 +179,8 @@ TEST(ReadAll, ZeroBytePayloadPreservesMetadata) { Optional(IsProtoEqual(MakeZeroByteTestObject()))); } -// b/565852217: an empty first payload with metadata, followed by data, must -// preserve both the metadata and the data. +// An empty first payload with metadata, followed by data, must preserve both +// the metadata and the data. TEST(ReadAll, EmptyPayloadWithMetadataThenData) { auto mock = std::make_unique(); EXPECT_CALL(*mock, Read) diff --git a/google/cloud/storage/internal/async/connection_impl_read_test.cc b/google/cloud/storage/internal/async/connection_impl_read_test.cc index 13a5ad59a8196..8dec0635ee755 100644 --- a/google/cloud/storage/internal/async/connection_impl_read_test.cc +++ b/google/cloud/storage/internal/async/connection_impl_read_test.cc @@ -602,8 +602,6 @@ TEST_F(AsyncConnectionImplTest, ReadObjectSilentWhenRetriesAreDisabled) { EXPECT_THAT(RetryRecords(log), IsEmpty()); } -// Only one test for ReadObjectRange(). The tests for `ReadAll()` and -// `ReadObject()` cover most other cases. TEST_F(AsyncConnectionImplTest, ReadObjectRangePermanentError) { AsyncSequencer sequencer; auto mock = std::make_shared(); @@ -627,8 +625,8 @@ TEST_F(AsyncConnectionImplTest, ReadObjectRangePermanentError) { EXPECT_THAT(pending.get(), StatusIs(PermanentError().code())); } -// b/565852217: `ReadObjectRange()` on a 0-byte object must return the object -// metadata sent by the service, even though there is no data. +// `ReadObjectRange()` on a 0-byte object must return the object metadata sent +// by the service, even though there is no data. TEST_F(AsyncConnectionImplTest, ReadObjectRangeZeroByteObjectKeepsMetadata) { auto constexpr kMetadata = R"pb( bucket: "projects/_/buckets/test-bucket" diff --git a/google/cloud/storage/internal/async/read_payload_impl_test.cc b/google/cloud/storage/internal/async/read_payload_impl_test.cc index 97cf6d6768583..197f7a062b5b5 100644 --- a/google/cloud/storage/internal/async/read_payload_impl_test.cc +++ b/google/cloud/storage/internal/async/read_payload_impl_test.cc @@ -116,7 +116,7 @@ TEST(ReadPayload, AccumulateAppendsData) { EXPECT_EQ(actual.offset(), 1024); } -// b/565852217: a 0-byte payload with metadata must survive the EOF payload. +// A 0-byte payload with metadata must survive the EOF payload. TEST(ReadPayload, AccumulateEmptyWithMetadataKeepsMetadata) { google::storage::v2::Object const resource = MakeTestObject(); storage::ReadPayload actual = @@ -127,8 +127,8 @@ TEST(ReadPayload, AccumulateEmptyWithMetadataKeepsMetadata) { EXPECT_THAT(actual.metadata(), Optional(IsProtoEqual(resource))); } -// b/565852217: a 0-byte payload with metadata followed by data keeps the -// metadata, offset, and object hashes from the first payload. +// A 0-byte payload with metadata followed by data keeps the metadata, offset, +// and object hashes from the first payload. TEST(ReadPayload, AccumulateEmptyWithMetadataThenData) { google::storage::v2::Object const resource = MakeTestObject(); storage::ReadPayload actual = ReadPayloadImpl::Make(absl::Cord())