diff --git a/generator/integration_tests/golden/v1/internal/request_id_connection_impl.cc b/generator/integration_tests/golden/v1/internal/request_id_connection_impl.cc index 56869279ce201..ef9c362d669cc 100644 --- a/generator/integration_tests/golden/v1/internal/request_id_connection_impl.cc +++ b/generator/integration_tests/golden/v1/internal/request_id_connection_impl.cc @@ -66,20 +66,27 @@ RequestIdServiceConnectionImpl::RequestIdServiceConnectionImpl( StatusOr RequestIdServiceConnectionImpl::CreateFoo(google::test::requestid::v1::CreateFooRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } return google::cloud::internal::RetryLoop( retry_policy(*current), backoff_policy(*current), - idempotency_policy(*current)->CreateFoo(request), + idempotency_policy(*current)->CreateFoo(request_copy), [this](grpc::ClientContext& context, google::test::requestid::v1::CreateFooRequest const& request) { return stub_->CreateFoo(context, request); }, - request, __func__); + request_copy, __func__); } future> RequestIdServiceConnectionImpl::RenameFoo(google::test::requestid::v1::RenameFooRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } auto const idempotent = idempotency_policy(*current)->RenameFoo(request_copy); return google::cloud::internal::AsyncLongRunningOperation( @@ -138,6 +145,9 @@ future> RequestIdServiceConnectionImpl::AsyncCreateFoo(google::test::requestid::v1::CreateFooRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } auto const idempotent = idempotency_policy(*current)->CreateFoo(request_copy); return google::cloud::internal::AsyncRetryLoop( diff --git a/generator/integration_tests/golden/v1/internal/request_id_connection_impl.h b/generator/integration_tests/golden/v1/internal/request_id_connection_impl.h index 3f1e965feeb9f..44b0f333da6b5 100644 --- a/generator/integration_tests/golden/v1/internal/request_id_connection_impl.h +++ b/generator/integration_tests/golden/v1/internal/request_id_connection_impl.h @@ -27,6 +27,7 @@ #include "google/cloud/background_threads.h" #include "google/cloud/backoff_policy.h" #include "google/cloud/future.h" +#include "google/cloud/internal/invocation_id_generator.h" #include "google/cloud/options.h" #include "google/cloud/polling_policy.h" #include "google/cloud/status_or.h" @@ -68,6 +69,9 @@ class RequestIdServiceConnectionImpl std::unique_ptr background_; std::shared_ptr stub_; Options options_; + std::shared_ptr + invocation_id_generator_ = + std::make_shared(); }; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/generator/integration_tests/golden/v1/request_id_client.h b/generator/integration_tests/golden/v1/request_id_client.h index e5c3e31526b71..17674f9e848a1 100644 --- a/generator/integration_tests/golden/v1/request_id_client.h +++ b/generator/integration_tests/golden/v1/request_id_client.h @@ -101,8 +101,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L80} - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} + /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L79} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} /// // clang-format on StatusOr @@ -131,8 +131,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L80} - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} + /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L79} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} /// // clang-format on StatusOr @@ -165,8 +165,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} - /// [google.test.requestid.v1.RenameFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L100} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} + /// [google.test.requestid.v1.RenameFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L99} /// // clang-format on future> @@ -202,8 +202,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} - /// [google.test.requestid.v1.RenameFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L100} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} + /// [google.test.requestid.v1.RenameFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L99} /// // clang-format on future> @@ -236,8 +236,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} - /// [google.test.requestid.v1.ListFoosRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L135} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} + /// [google.test.requestid.v1.ListFoosRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L134} /// // clang-format on StreamRange @@ -275,8 +275,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} - /// [google.test.requestid.v1.ListFoosRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L135} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} + /// [google.test.requestid.v1.ListFoosRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L134} /// // clang-format on StreamRange @@ -301,8 +301,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L80} - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} + /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L79} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} /// // clang-format on future> @@ -331,8 +331,8 @@ class RequestIdServiceClient { /// [`future`]: @ref google::cloud::future /// [`StatusOr`]: @ref google::cloud::StatusOr /// [`Status`]: @ref google::cloud::Status - /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L80} - /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L66} + /// [google.test.requestid.v1.CreateFooRequest]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L79} + /// [google.test.requestid.v1.Foo]: @googleapis_reference_link{generator/integration_tests/test_request_id.proto#L65} /// // clang-format on future> diff --git a/generator/integration_tests/golden/v1/request_id_connection_idempotency_policy.cc b/generator/integration_tests/golden/v1/request_id_connection_idempotency_policy.cc index 9412d9297941a..5236ebcb3f724 100644 --- a/generator/integration_tests/golden/v1/request_id_connection_idempotency_policy.cc +++ b/generator/integration_tests/golden/v1/request_id_connection_idempotency_policy.cc @@ -33,11 +33,13 @@ RequestIdServiceConnectionIdempotencyPolicy::clone() const { return std::make_unique(*this); } -Idempotency RequestIdServiceConnectionIdempotencyPolicy::CreateFoo(google::test::requestid::v1::CreateFooRequest const&) { +Idempotency RequestIdServiceConnectionIdempotencyPolicy::CreateFoo(google::test::requestid::v1::CreateFooRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kNonIdempotent; } -Idempotency RequestIdServiceConnectionIdempotencyPolicy::RenameFoo(google::test::requestid::v1::RenameFooRequest const&) { +Idempotency RequestIdServiceConnectionIdempotencyPolicy::RenameFoo(google::test::requestid::v1::RenameFooRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kNonIdempotent; } diff --git a/generator/integration_tests/test_request_id.proto b/generator/integration_tests/test_request_id.proto index 8c229df6d4da2..004993b96a209 100644 --- a/generator/integration_tests/test_request_id.proto +++ b/generator/integration_tests/test_request_id.proto @@ -16,7 +16,6 @@ syntax = "proto3"; package google.test.requestid.v1; -import "google/api/annotations.proto"; import "google/api/client.proto"; import "google/api/field_behavior.proto"; import "google/api/field_info.proto"; diff --git a/generator/integration_tests/tests/request_id_connection_impl_test.cc b/generator/integration_tests/tests/request_id_connection_impl_test.cc index e81a1b0148bc8..f751e98e041ad 100644 --- a/generator/integration_tests/tests/request_id_connection_impl_test.cc +++ b/generator/integration_tests/tests/request_id_connection_impl_test.cc @@ -20,7 +20,6 @@ #include "google/cloud/testing_util/status_matchers.h" #include #include -#include #include #include @@ -97,10 +96,20 @@ auto WithRequestId(std::string expected) { template auto WithoutRequestId() { return ResultOf( - "request does not have request_id", + "request has empty request_id", [](Request const& request) { return request.request_id(); }, IsEmpty()); } +template +auto RequestIdIsUuidV4() { + using ::testing::ContainsRegex; + return ResultOf( + "request has request_id in UUIDV4 format", + [](Request const& request) { return request.request_id(); }, + ContainsRegex( + "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}")); +} + auto MakeTestConnection( std::shared_ptr stub) { auto options = golden_v1_internal::RequestIdServiceDefaultOptions({}); @@ -109,9 +118,14 @@ auto MakeTestConnection( std::move(background), std::move(stub), std::move(options)); } +Status TransientError() { + return Status(StatusCode::kUnavailable, "try-again"); +} + TEST(RequestIdTest, UnaryRpc) { auto mock = std::make_shared(); - EXPECT_CALL(*mock, CreateFoo(_, WithoutRequestId())) + EXPECT_CALL(*mock, CreateFoo(_, RequestIdIsUuidV4())) + .WillOnce(Return(TransientError())) .WillOnce(Return(Foo{})); auto connection = MakeTestConnection(mock); @@ -125,6 +139,7 @@ TEST(RequestIdTest, UnaryRpcExplicit) { auto mock = std::make_shared(); EXPECT_CALL(*mock, CreateFoo(_, WithRequestId("test-request-id"))) + .WillOnce(Return(TransientError())) .WillOnce(Return(Foo{})); auto connection = MakeTestConnection(mock); @@ -137,7 +152,10 @@ TEST(RequestIdTest, UnaryRpcExplicit) { TEST(RequestIdTest, AsyncUnaryRpc) { auto mock = std::make_shared(); - EXPECT_CALL(*mock, AsyncCreateFoo(_, _, WithoutRequestId())) + EXPECT_CALL(*mock, + AsyncCreateFoo(_, _, RequestIdIsUuidV4())) + .WillOnce( + Return(ByMove(make_ready_future(StatusOr(TransientError()))))) .WillOnce(Return(ByMove(make_ready_future(make_status_or(Foo{}))))); auto connection = MakeTestConnection(mock); @@ -152,6 +170,8 @@ TEST(RequestIdTest, AsyncUnaryRpcExplicit) { EXPECT_CALL( *mock, AsyncCreateFoo(_, _, WithRequestId("test-request-id"))) + .WillOnce( + Return(ByMove(make_ready_future(StatusOr(TransientError()))))) .WillOnce(Return(ByMove(make_ready_future(make_status_or(Foo{}))))); auto connection = MakeTestConnection(mock); @@ -165,7 +185,9 @@ TEST(RequestIdTest, AsyncUnaryRpcExplicit) { TEST(RequestIdTest, Lro) { auto mock = std::make_shared(); EXPECT_CALL(*mock, - AsyncRenameFoo(_, _, _, WithoutRequestId())) + AsyncRenameFoo(_, _, _, RequestIdIsUuidV4())) + .WillOnce(Return(ByMove(make_ready_future( + StatusOr(TransientError()))))) .WillOnce(Return(ByMove(make_ready_future( make_status_or(google::longrunning::Operation{}))))); EXPECT_CALL(*mock, AsyncGetOperation).WillOnce([] { @@ -187,6 +209,8 @@ TEST(RequestIdTest, LroExplicit) { EXPECT_CALL( *mock, AsyncRenameFoo(_, _, _, WithRequestId("test-request-id"))) + .WillOnce(Return(ByMove(make_ready_future( + StatusOr(TransientError()))))) .WillOnce(Return(ByMove(make_ready_future( make_status_or(google::longrunning::Operation{}))))); EXPECT_CALL(*mock, AsyncGetOperation).WillOnce([] { @@ -206,17 +230,17 @@ TEST(RequestIdTest, LroExplicit) { TEST(RequestIdTest, Pagination) { auto mock = std::make_shared(); - std::set sequence_ids; + std::vector sequence_ids; EXPECT_CALL(*mock, ListFoos(_, WithoutRequestId())) .WillOnce([&](auto&, auto const& request) { - sequence_ids.insert(request.request_id()); + sequence_ids.push_back(request.request_id()); ListFoosResponse response; response.add_foos()->set_name("name-0"); response.set_next_page_token("test-token-0"); return response; }) .WillOnce([&](auto&, auto const& request) { - sequence_ids.insert(request.request_id()); + sequence_ids.push_back(request.request_id()); ListFoosResponse response; response.add_foos()->set_name("name-1"); return response; @@ -234,26 +258,23 @@ TEST(RequestIdTest, Pagination) { }; EXPECT_THAT(results, ElementsAre(IsOkAndHolds(with_name("name-0")), IsOkAndHolds(with_name("name-1")))); - EXPECT_THAT(sequence_ids, ElementsAre(IsEmpty())); + EXPECT_THAT(sequence_ids, ElementsAre(IsEmpty(), IsEmpty())); } TEST(RequestIdTest, PaginationExplicit) { auto mock = std::make_shared(); - std::set sequence_ids; - ::testing::InSequence sequence; + std::vector sequence_ids; EXPECT_CALL(*mock, ListFoos(_, WithRequestId("test-request-id"))) .WillOnce([&](auto&, auto const& request) { - sequence_ids.insert(request.request_id()); + sequence_ids.push_back(request.request_id()); ListFoosResponse response; response.add_foos()->set_name("name-0"); response.set_next_page_token("test-token-0"); return response; - }); - EXPECT_CALL(*mock, - ListFoos(_, WithRequestId("test-request-id"))) + }) .WillOnce([&](auto&, auto const& request) { - sequence_ids.insert(request.request_id()); + sequence_ids.push_back(request.request_id()); ListFoosResponse response; response.add_foos()->set_name("name-1"); return response; @@ -272,7 +293,7 @@ TEST(RequestIdTest, PaginationExplicit) { }; EXPECT_THAT(results, ElementsAre(IsOkAndHolds(with_name("name-0")), IsOkAndHolds(with_name("name-1")))); - EXPECT_THAT(sequence_ids, ElementsAre("test-request-id")); + EXPECT_THAT(sequence_ids, ElementsAre("test-request-id", "test-request-id")); } } // namespace diff --git a/generator/integration_tests/tests/request_id_idempotency_test.cc b/generator/integration_tests/tests/request_id_idempotency_test.cc index fc596e2429791..d3d35e51fa6a0 100644 --- a/generator/integration_tests/tests/request_id_idempotency_test.cc +++ b/generator/integration_tests/tests/request_id_idempotency_test.cc @@ -30,18 +30,24 @@ TEST(RequestIdIdempotency, CreateFoo) { auto policy = MakeDefaultRequestIdServiceConnectionIdempotencyPolicy(); CreateFooRequest request; EXPECT_EQ(policy->CreateFoo(request), Idempotency::kNonIdempotent); + request.set_request_id("test-request-id"); + EXPECT_EQ(policy->CreateFoo(request), Idempotency::kIdempotent); } TEST(RequestIdIdempotency, ListFoos) { auto policy = MakeDefaultRequestIdServiceConnectionIdempotencyPolicy(); ListFoosRequest request; EXPECT_EQ(policy->ListFoos(request), Idempotency::kNonIdempotent); + request.set_request_id("test-request-id"); + EXPECT_EQ(policy->ListFoos(request), Idempotency::kNonIdempotent); } TEST(RequestIdIdempotency, RenameFoo) { auto policy = MakeDefaultRequestIdServiceConnectionIdempotencyPolicy(); RenameFooRequest request; EXPECT_EQ(policy->RenameFoo(request), Idempotency::kNonIdempotent); + request.set_request_id("test-request-id"); + EXPECT_EQ(policy->RenameFoo(request), Idempotency::kIdempotent); } } // namespace diff --git a/generator/internal/connection_impl_generator.cc b/generator/internal/connection_impl_generator.cc index da3b176f33d0c..3d733dcbfa68a 100644 --- a/generator/internal/connection_impl_generator.cc +++ b/generator/internal/connection_impl_generator.cc @@ -57,6 +57,7 @@ Status ConnectionImplGenerator::GenerateHeader() { : "", "google/cloud/background_threads.h", "google/cloud/backoff_policy.h", HasLongrunningMethod() ? "google/cloud/future.h" : "", + HasRequestId() ? "google/cloud/internal/invocation_id_generator.h" : "", "google/cloud/options.h", HasLongrunningMethod() ? "google/cloud/polling_policy.h" : "", "google/cloud/status_or.h", @@ -117,11 +118,17 @@ class $connection_class_name$Impl private: std::unique_ptr background_; std::shared_ptr<$product_internal_namespace$::$stub_class_name$> stub_; - Options options_; -)"""); + Options options_;)"""); + + if (HasRequestId()) { + HeaderPrint(R"""( + std::shared_ptr + invocation_id_generator_ = + std::make_shared();)"""); + } // This closes the *ConnectionImpl class definition. - HeaderPrint("};\n"); + HeaderPrint("\n};\n"); HeaderCloseNamespaces(); @@ -324,6 +331,9 @@ StreamRange<$response_type$> return {}; } + // We do not need to consider `HasRequestId()` in paginated APIs. The main + // motivation for `HasRequestId()` is to make the request idempotent, but + // paginated APIs are always read-only and therefore idempotent. if (IsPaginated(method)) { return R"""( StreamRange<$range_output_type$> @@ -363,6 +373,12 @@ StreamRange<$range_output_type$> ? R"""(future)""" : R"""(future>)"""; + auto const* request_id_fragment = HasRequestId(method) ? R"""( + if (request_copy.$request_id_field_name$().empty()) { + request_copy.set_$request_id_field_name$(invocation_id_generator_->MakeInvocationId()); + })""" + : ""; + // One of the variations is how to extract the value from the operation // result, some operations use the metadata, some the data. We need to // provide the right function to internal::AsyncLongRunningOperation. @@ -387,7 +403,8 @@ StreamRange<$range_output_type$> R"""( $connection_class_name$Impl::$method_name$($request_type$ const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); - auto request_copy = request; + auto request_copy = request;)""", + request_id_fragment, R"""( auto const idempotent = idempotency_policy(*current)->$method_name$(request_copy); return google::cloud::internal::AsyncLongRunningOperation<$longrunning_deduced_response_type$>( @@ -424,6 +441,26 @@ StreamRange<$range_output_type$> auto const* return_fragment = IsResponseTypeEmpty(method) ? R"""(Status)""" : R"""(StatusOr<$response_type$>)"""; + if (HasRequestId(method)) { + return absl::StrCat("\n", return_fragment, R"""( +$connection_class_name$Impl::$method_name$($request_type$ const& request) { + auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.$request_id_field_name$().empty()) { + request_copy.set_$request_id_field_name$(invocation_id_generator_->MakeInvocationId()); + } + return google::cloud::internal::RetryLoop( + retry_policy(*current), backoff_policy(*current), + idempotency_policy(*current)->$method_name$(request_copy), + [this](grpc::ClientContext& context, + $request_type$ const& request) { + return stub_->$method_name$(context, request); + }, + request_copy, __func__); +} +)"""); + } + return absl::StrCat("\n", return_fragment, R"""( $connection_class_name$Impl::$method_name$($request_type$ const& request) { @@ -445,12 +482,19 @@ std::string ConnectionImplGenerator::AsyncMethodDefinition( auto const* return_fragment = IsResponseTypeEmpty(method) ? R"""(future)""" : R"""(future>)"""; + auto const* request_id_fragment = HasRequestId(method) ? + R"""( + if (request_copy.$request_id_field_name$().empty()) { + request_copy.set_$request_id_field_name$(invocation_id_generator_->MakeInvocationId()); + })""" + : ""; return absl::StrCat("\n", return_fragment, R"""( $connection_class_name$Impl::Async$method_name$($request_type$ const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); - auto request_copy = request; + auto request_copy = request;)""", + request_id_fragment, R"""( auto const idempotent = idempotency_policy(*current)->$method_name$(request_copy); return google::cloud::internal::AsyncRetryLoop( diff --git a/generator/internal/connection_impl_generator.h b/generator/internal/connection_impl_generator.h index 5e07d459b0df1..0656f586b96a0 100644 --- a/generator/internal/connection_impl_generator.h +++ b/generator/internal/connection_impl_generator.h @@ -50,9 +50,9 @@ class ConnectionImplGenerator : public ServiceCodeGenerator { google::protobuf::MethodDescriptor const& method); static std::string AsyncMethodDeclaration( google::protobuf::MethodDescriptor const& method); - static std::string MethodDefinition( + std::string MethodDefinition( google::protobuf::MethodDescriptor const& method); - static std::string AsyncMethodDefinition( + std::string AsyncMethodDefinition( google::protobuf::MethodDescriptor const& method); }; diff --git a/generator/internal/idempotency_policy_generator.cc b/generator/internal/idempotency_policy_generator.cc index cecdf981ea460..164ba78816b87 100644 --- a/generator/internal/idempotency_policy_generator.cc +++ b/generator/internal/idempotency_policy_generator.cc @@ -18,6 +18,7 @@ #include "generator/internal/pagination.h" #include "generator/internal/predicate_utils.h" #include "generator/internal/printer.h" +#include "generator/internal/request_id.h" #include "google/cloud/internal/absl_str_cat_quiet.h" #include @@ -165,10 +166,29 @@ Idempotency $idempotency_class_name$::$method_name$( // TODO(#9982) - we should pass `$request_type$ const&` here if (IsPaginated(method)) { - CcPrintMethod(method, __FILE__, __LINE__, R"""( + if (HasRequestId(method)) { + CcPrintMethod(method, __FILE__, __LINE__, R"""( +Idempotency $idempotency_class_name$::$method_name$($request_type$ request) { // NOLINT + if (!request.$request_id_field_name$().empty()) return Idempotency::kIdempotent; + return Idempotency::$idempotency$; +} +)"""); + } else { + CcPrintMethod(method, __FILE__, __LINE__, R"""( Idempotency $idempotency_class_name$::$method_name$($request_type$) { // NOLINT return Idempotency::$idempotency$; } +)"""); + } + continue; + } + + if (HasRequestId(method)) { + CcPrintMethod(method, __FILE__, __LINE__, R"""( +Idempotency $idempotency_class_name$::$method_name$($request_type$ const& request) { + if (!request.$request_id_field_name$().empty()) return Idempotency::kIdempotent; + return Idempotency::$idempotency$; +} )"""); continue; } diff --git a/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.cc b/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.cc index ccf10401f75b3..32b8c49abf091 100644 --- a/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.cc +++ b/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.cc @@ -70,41 +70,53 @@ StatusOr StorageControlConnectionImpl::CreateFolder( google::storage::control::v2::CreateFolderRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } return google::cloud::internal::RetryLoop( retry_policy(*current), backoff_policy(*current), - idempotency_policy(*current)->CreateFolder(request), + idempotency_policy(*current)->CreateFolder(request_copy), [this](grpc::ClientContext& context, google::storage::control::v2::CreateFolderRequest const& request) { return stub_->CreateFolder(context, request); }, - request, __func__); + request_copy, __func__); } Status StorageControlConnectionImpl::DeleteFolder( google::storage::control::v2::DeleteFolderRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } return google::cloud::internal::RetryLoop( retry_policy(*current), backoff_policy(*current), - idempotency_policy(*current)->DeleteFolder(request), + idempotency_policy(*current)->DeleteFolder(request_copy), [this](grpc::ClientContext& context, google::storage::control::v2::DeleteFolderRequest const& request) { return stub_->DeleteFolder(context, request); }, - request, __func__); + request_copy, __func__); } StatusOr StorageControlConnectionImpl::GetFolder( google::storage::control::v2::GetFolderRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } return google::cloud::internal::RetryLoop( retry_policy(*current), backoff_policy(*current), - idempotency_policy(*current)->GetFolder(request), + idempotency_policy(*current)->GetFolder(request_copy), [this](grpc::ClientContext& context, google::storage::control::v2::GetFolderRequest const& request) { return stub_->GetFolder(context, request); }, - request, __func__); + request_copy, __func__); } StreamRange @@ -143,6 +155,9 @@ StorageControlConnectionImpl::RenameFolder( google::storage::control::v2::RenameFolderRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } auto const idempotent = idempotency_policy(*current)->RenameFolder(request_copy); return google::cloud::internal::AsyncLongRunningOperation< @@ -179,13 +194,17 @@ StatusOr StorageControlConnectionImpl::GetStorageLayout( google::storage::control::v2::GetStorageLayoutRequest const& request) { auto current = google::cloud::internal::SaveCurrentOptions(); + auto request_copy = request; + if (request_copy.request_id().empty()) { + request_copy.set_request_id(invocation_id_generator_->MakeInvocationId()); + } return google::cloud::internal::RetryLoop( retry_policy(*current), backoff_policy(*current), - idempotency_policy(*current)->GetStorageLayout(request), + idempotency_policy(*current)->GetStorageLayout(request_copy), [this](grpc::ClientContext& context, google::storage::control::v2::GetStorageLayoutRequest const& request) { return stub_->GetStorageLayout(context, request); }, - request, __func__); + request_copy, __func__); } GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.h b/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.h index 17da06b9b3b03..3f7904570ca3f 100644 --- a/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.h +++ b/google/cloud/storagecontrol/v2/internal/storage_control_connection_impl.h @@ -27,6 +27,7 @@ #include "google/cloud/background_threads.h" #include "google/cloud/backoff_policy.h" #include "google/cloud/future.h" +#include "google/cloud/internal/invocation_id_generator.h" #include "google/cloud/options.h" #include "google/cloud/polling_policy.h" #include "google/cloud/status_or.h" @@ -77,6 +78,9 @@ class StorageControlConnectionImpl std::unique_ptr background_; std::shared_ptr stub_; Options options_; + std::shared_ptr + invocation_id_generator_ = + std::make_shared(); }; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storagecontrol/v2/storage_control_connection_idempotency_policy.cc b/google/cloud/storagecontrol/v2/storage_control_connection_idempotency_policy.cc index 21784a4d88f4f..20f6e0a33d3ba 100644 --- a/google/cloud/storagecontrol/v2/storage_control_connection_idempotency_policy.cc +++ b/google/cloud/storagecontrol/v2/storage_control_connection_idempotency_policy.cc @@ -35,17 +35,20 @@ StorageControlConnectionIdempotencyPolicy::clone() const { } Idempotency StorageControlConnectionIdempotencyPolicy::CreateFolder( - google::storage::control::v2::CreateFolderRequest const&) { + google::storage::control::v2::CreateFolderRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kNonIdempotent; } Idempotency StorageControlConnectionIdempotencyPolicy::DeleteFolder( - google::storage::control::v2::DeleteFolderRequest const&) { + google::storage::control::v2::DeleteFolderRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kNonIdempotent; } Idempotency StorageControlConnectionIdempotencyPolicy::GetFolder( - google::storage::control::v2::GetFolderRequest const&) { + google::storage::control::v2::GetFolderRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kIdempotent; } @@ -55,12 +58,14 @@ Idempotency StorageControlConnectionIdempotencyPolicy::ListFolders( } Idempotency StorageControlConnectionIdempotencyPolicy::RenameFolder( - google::storage::control::v2::RenameFolderRequest const&) { + google::storage::control::v2::RenameFolderRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kNonIdempotent; } Idempotency StorageControlConnectionIdempotencyPolicy::GetStorageLayout( - google::storage::control::v2::GetStorageLayoutRequest const&) { + google::storage::control::v2::GetStorageLayoutRequest const& request) { + if (!request.request_id().empty()) return Idempotency::kIdempotent; return Idempotency::kIdempotent; }