From 8d192c793b6d8e8153b4a612ec476f9cf07e9d5a Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Thu, 12 Jun 2025 11:19:13 +0000 Subject: [PATCH 1/4] Add check for writeHandle before reansforming write_object_spec to append_object_spec --- .../storage/internal/async/connection_impl.cc | 6 +- .../internal/async/handle_redirect_error.cc | 23 ++++++-- .../internal/async/handle_redirect_error.h | 2 +- .../async/handle_redirect_error_test.cc | 56 +++++++++++++++++++ .../internal/async/writer_connection_impl.cc | 6 +- 5 files changed, 82 insertions(+), 11 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_impl.cc b/google/cloud/storage/internal/async/connection_impl.cc index 67ae6fae1ca11..63d1c78e68e9c 100644 --- a/google/cloud/storage/internal/async/connection_impl.cc +++ b/google/cloud/storage/internal/async/connection_impl.cc @@ -360,9 +360,11 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p) { open.reset(); auto response = f.get(); if (!response) { - EnsureFirstMessageAppendObjectSpec(request); + google::rpc::Status grpc_status = + ExtractGrpcStatus(response.status()); + EnsureFirstMessageAppendObjectSpec(request, grpc_status); ApplyWriteRedirectErrors(*request.mutable_append_object_spec(), - ExtractGrpcStatus(response.status())); + grpc_status); } return response; }); diff --git a/google/cloud/storage/internal/async/handle_redirect_error.cc b/google/cloud/storage/internal/async/handle_redirect_error.cc index 3481e15d706ca..a4a9afc1efe2c 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error.cc @@ -21,12 +21,23 @@ namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( - google::storage::v2::BidiWriteObjectRequest& request) { - if (request.has_write_object_spec()) { - auto spec = request.write_object_spec(); - auto& append_object_spec = *request.mutable_append_object_spec(); - append_object_spec.set_bucket(spec.resource().bucket()); - append_object_spec.set_object(spec.resource().name()); + google::storage::v2::BidiWriteObjectRequest& request, + google::rpc::Status const& rpc_status) { + for (auto const& any : rpc_status.details()) { + google::storage::v2::BidiWriteObjectRedirectedError error = + google::storage::v2::BidiWriteObjectRedirectedError{}; + if (!any.UnpackTo(&error)) continue; + if (!error.has_write_handle()) continue; + if (request.has_write_object_spec()) { + auto spec = request.write_object_spec(); + auto& append_object_spec = *request.mutable_append_object_spec(); + append_object_spec.set_bucket(spec.resource().bucket()); + append_object_spec.set_object(spec.resource().name()); + append_object_spec.set_if_metageneration_match( + spec.if_metageneration_match()); + append_object_spec.set_if_metageneration_not_match( + spec.if_metageneration_not_match()); + } } } diff --git a/google/cloud/storage/internal/async/handle_redirect_error.h b/google/cloud/storage/internal/async/handle_redirect_error.h index 94adb1679cf15..395822769c4b7 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.h +++ b/google/cloud/storage/internal/async/handle_redirect_error.h @@ -26,7 +26,7 @@ namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( - google::storage::v2::BidiWriteObjectRequest& request); + google::storage::v2::BidiWriteObjectRequest& request, google::rpc::Status const& rpc_status); google::rpc::Status ExtractGrpcStatus(Status const& status); diff --git a/google/cloud/storage/internal/async/handle_redirect_error_test.cc b/google/cloud/storage/internal/async/handle_redirect_error_test.cc index a8a09d51f4d6d..99c3486260f46 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error_test.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error_test.cc @@ -70,6 +70,62 @@ TEST(ApplyRedirectErrors, NoRedirect) { EXPECT_TRUE(spec.routing_token().empty()); } +TEST(EnsureFirstMessageAppendObjectSpec, Success) { + google::storage::v2::BidiWriteObjectRequest request; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + write_object_spec { + resource { bucket: "projects/_/buckets/b", name: "o" } + if_metageneration_match: 1 + if_metageneration_not_match: 1 + } + )pb", + &request)); + + google::rpc::Status rpc_status; + google::storage::v2::BidiWriteObjectRedirectedError redirect; + redirect.mutable_write_handle(); + rpc_status.add_details()->PackFrom(redirect); + + EnsureFirstMessageAppendObjectSpec(request, rpc_status); + + EXPECT_FALSE(request.has_write_object_spec()); + EXPECT_TRUE(request.has_append_object_spec()); + + auto const& append_spec = request.append_object_spec(); + EXPECT_EQ(append_spec.bucket(), "projects/_/buckets/b"); + EXPECT_EQ(append_spec.object(), "o"); + + EXPECT_FALSE(append_spec.has_write_handle()); + EXPECT_TRUE(append_spec.routing_token().empty()); + EXPECT_EQ(append_spec.if_metageneration_match(), 1); + EXPECT_EQ(append_spec.if_metageneration_not_match(), 1); + EXPECT_EQ(append_spec.generation(), 0); +} + +TEST(EnsureFirstMessageAppendObjectSpec, WriteHandleIsNotSet) { + google::storage::v2::BidiWriteObjectRequest request; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + write_object_spec { + resource { bucket: "projects/_/buckets/b", name: "o" } + } + )pb", + &request)); + + google::rpc::Status rpc_status; + google::storage::v2::BidiWriteObjectRedirectedError redirect; + redirect.set_generation(1234); + rpc_status.add_details()->PackFrom(redirect); + + EnsureFirstMessageAppendObjectSpec(request, rpc_status); + + EXPECT_TRUE(request.has_write_object_spec()); + EXPECT_FALSE(request.has_append_object_spec()); +} + + + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 53a0513ae62c0..018248391da15 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -234,9 +234,11 @@ future> AsyncWriterConnectionImpl::OnQuery( "Expected error in Finish() after non-ok Read()")) .then([this](auto g) { auto result = g.get(); - EnsureFirstMessageAppendObjectSpec(request_); + google::rpc::Status grpc_status = + ExtractGrpcStatus(result); + EnsureFirstMessageAppendObjectSpec(request_, grpc_status); ApplyWriteRedirectErrors(*request_.mutable_append_object_spec(), - ExtractGrpcStatus(result)); + grpc_status); return StatusOr(std::move(result)); }); } From c2980394de3424217c1a1529b62361d91887c16e Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Mon, 16 Jun 2025 04:12:42 +0000 Subject: [PATCH 2/4] Correct the formatting --- google/cloud/storage/internal/async/handle_redirect_error.h | 3 ++- .../storage/internal/async/handle_redirect_error_test.cc | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/internal/async/handle_redirect_error.h b/google/cloud/storage/internal/async/handle_redirect_error.h index 395822769c4b7..2026203fc8b4d 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.h +++ b/google/cloud/storage/internal/async/handle_redirect_error.h @@ -26,7 +26,8 @@ namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( - google::storage::v2::BidiWriteObjectRequest& request, google::rpc::Status const& rpc_status); + google::storage::v2::BidiWriteObjectRequest& request, + google::rpc::Status const& rpc_status); google::rpc::Status ExtractGrpcStatus(Status const& status); diff --git a/google/cloud/storage/internal/async/handle_redirect_error_test.cc b/google/cloud/storage/internal/async/handle_redirect_error_test.cc index 99c3486260f46..0324afd1fb069 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error_test.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error_test.cc @@ -75,7 +75,7 @@ TEST(EnsureFirstMessageAppendObjectSpec, Success) { ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( R"pb( write_object_spec { - resource { bucket: "projects/_/buckets/b", name: "o" } + resource { bucket: "projects/_/buckets/b", name: "o" } if_metageneration_match: 1 if_metageneration_not_match: 1 } @@ -124,8 +124,6 @@ TEST(EnsureFirstMessageAppendObjectSpec, WriteHandleIsNotSet) { EXPECT_FALSE(request.has_append_object_spec()); } - - } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_internal From 661dc930643701e2099b251e6bd7ce79d9e3a8b8 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Tue, 17 Jun 2025 04:07:11 +0000 Subject: [PATCH 3/4] rename the any variable to rpc_status_detail --- google/cloud/storage/internal/async/handle_redirect_error.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/async/handle_redirect_error.cc b/google/cloud/storage/internal/async/handle_redirect_error.cc index a4a9afc1efe2c..1444a8bda6f19 100644 --- a/google/cloud/storage/internal/async/handle_redirect_error.cc +++ b/google/cloud/storage/internal/async/handle_redirect_error.cc @@ -23,10 +23,10 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN void EnsureFirstMessageAppendObjectSpec( google::storage::v2::BidiWriteObjectRequest& request, google::rpc::Status const& rpc_status) { - for (auto const& any : rpc_status.details()) { + for (auto const& rpc_status_detail : rpc_status.details()) { google::storage::v2::BidiWriteObjectRedirectedError error = google::storage::v2::BidiWriteObjectRedirectedError{}; - if (!any.UnpackTo(&error)) continue; + if (!rpc_status_detail.UnpackTo(&error)) continue; if (!error.has_write_handle()) continue; if (request.has_write_object_spec()) { auto spec = request.write_object_spec(); From b1a89d1caa8e1fe454eb39cf4a22c1fd83c2dfe3 Mon Sep 17 00:00:00 2001 From: Vaibhav Pratap Date: Tue, 17 Jun 2025 05:27:47 +0000 Subject: [PATCH 4/4] Format write_connection_impl.cc file --- google/cloud/storage/internal/async/writer_connection_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/async/writer_connection_impl.cc b/google/cloud/storage/internal/async/writer_connection_impl.cc index 018248391da15..56fefa762fba0 100644 --- a/google/cloud/storage/internal/async/writer_connection_impl.cc +++ b/google/cloud/storage/internal/async/writer_connection_impl.cc @@ -234,8 +234,7 @@ future> AsyncWriterConnectionImpl::OnQuery( "Expected error in Finish() after non-ok Read()")) .then([this](auto g) { auto result = g.get(); - google::rpc::Status grpc_status = - ExtractGrpcStatus(result); + google::rpc::Status grpc_status = ExtractGrpcStatus(result); EnsureFirstMessageAppendObjectSpec(request_, grpc_status); ApplyWriteRedirectErrors(*request_.mutable_append_object_spec(), grpc_status);