Skip to content

Commit

Permalink
feat(generator): support request_id-like fields (#13615)
Browse files Browse the repository at this point in the history 
With this change the generator populates the `request_id`-like fields
and also uses the field to determine if a request is idempotent.
  • Loading branch information
@coryan
coryan authored Feb 15, 2024
1 parent 475dc1c commit 9ea3fb2
Show file tree
Hide file tree
Showing 13 changed files with 193 additions and 59 deletions.
14 changes: 12 additions & 2 deletions generator/integration_tests/golden/v1/internal/request_id_connection_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,20 +66,27 @@ RequestIdServiceConnectionImpl::RequestIdServiceConnectionImpl(
StatusOr<google::test::requestid::v1::Foo>
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<StatusOr<google::test::requestid::v1::Foo>>
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<google::test::requestid::v1::Foo>(
Expand Down Expand Up @@ -138,6 +145,9 @@ future<StatusOr<google::test::requestid::v1::Foo>>
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(
Expand Down
4 changes: 4 additions & 0 deletions generator/integration_tests/golden/v1/internal/request_id_connection_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -68,6 +69,9 @@ class RequestIdServiceConnectionImpl
std::unique_ptr<google::cloud::BackgroundThreads> background_;
std::shared_ptr<golden_v1_internal::RequestIdServiceStub> stub_;
Options options_;
std::shared_ptr<google::cloud::internal::InvocationIdGenerator>
invocation_id_generator_ =
std::make_shared<google::cloud::internal::InvocationIdGenerator>();
};

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down
32 changes: 16 additions & 16 deletions generator/integration_tests/golden/v1/request_id_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<google::test::requestid::v1::Foo>
Expand Down Expand Up @@ -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<google::test::requestid::v1::Foo>
Expand Down Expand Up @@ -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<StatusOr<google::test::requestid::v1::Foo>>
Expand Down Expand Up @@ -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<StatusOr<google::test::requestid::v1::Foo>>
Expand Down Expand Up @@ -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<google::test::requestid::v1::Foo>
Expand Down Expand Up @@ -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<google::test::requestid::v1::Foo>
Expand All @@ -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<StatusOr<google::test::requestid::v1::Foo>>
Expand Down Expand Up @@ -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<StatusOr<google::test::requestid::v1::Foo>>
Expand Down
6 changes: 4 additions & 2 deletions generator/integration_tests/golden/v1/request_id_connection_idempotency_policy.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ RequestIdServiceConnectionIdempotencyPolicy::clone() const {
return std::make_unique<RequestIdServiceConnectionIdempotencyPolicy>(*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;
}

Expand Down
1 change: 0 additions & 1 deletion generator/integration_tests/test_request_id.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
55 changes: 38 additions & 17 deletions generator/integration_tests/tests/request_id_connection_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "google/cloud/testing_util/status_matchers.h"
#include <gmock/gmock.h>
#include <memory>
#include <set>
#include <string>
#include <utility>

Expand Down Expand Up @@ -97,10 +96,20 @@ auto WithRequestId(std::string expected) {
template <typename Request>
auto WithoutRequestId() {
return ResultOf(
"request does not have request_id",
"request has empty request_id",
[](Request const& request) { return request.request_id(); }, IsEmpty());
}

template <typename Request>
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<golden_v1_internal::RequestIdServiceStub> stub) {
auto options = golden_v1_internal::RequestIdServiceDefaultOptions({});
Expand All @@ -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<MockRequestIdServiceStub>();
EXPECT_CALL(*mock, CreateFoo(_, WithoutRequestId<CreateFooRequest>()))
EXPECT_CALL(*mock, CreateFoo(_, RequestIdIsUuidV4<CreateFooRequest>()))
.WillOnce(Return(TransientError()))
.WillOnce(Return(Foo{}));

auto connection = MakeTestConnection(mock);
Expand All @@ -125,6 +139,7 @@ TEST(RequestIdTest, UnaryRpcExplicit) {
auto mock = std::make_shared<MockRequestIdServiceStub>();
EXPECT_CALL(*mock,
CreateFoo(_, WithRequestId<CreateFooRequest>("test-request-id")))
.WillOnce(Return(TransientError()))
.WillOnce(Return(Foo{}));

auto connection = MakeTestConnection(mock);
Expand All @@ -137,7 +152,10 @@ TEST(RequestIdTest, UnaryRpcExplicit) {

TEST(RequestIdTest, AsyncUnaryRpc) {
auto mock = std::make_shared<MockRequestIdServiceStub>();
EXPECT_CALL(*mock, AsyncCreateFoo(_, _, WithoutRequestId<CreateFooRequest>()))
EXPECT_CALL(*mock,
AsyncCreateFoo(_, _, RequestIdIsUuidV4<CreateFooRequest>()))
.WillOnce(
Return(ByMove(make_ready_future(StatusOr<Foo>(TransientError())))))
.WillOnce(Return(ByMove(make_ready_future(make_status_or(Foo{})))));

auto connection = MakeTestConnection(mock);
Expand All @@ -152,6 +170,8 @@ TEST(RequestIdTest, AsyncUnaryRpcExplicit) {
EXPECT_CALL(
*mock,
AsyncCreateFoo(_, _, WithRequestId<CreateFooRequest>("test-request-id")))
.WillOnce(
Return(ByMove(make_ready_future(StatusOr<Foo>(TransientError())))))
.WillOnce(Return(ByMove(make_ready_future(make_status_or(Foo{})))));

auto connection = MakeTestConnection(mock);
Expand All @@ -165,7 +185,9 @@ TEST(RequestIdTest, AsyncUnaryRpcExplicit) {
TEST(RequestIdTest, Lro) {
auto mock = std::make_shared<MockRequestIdServiceStub>();
EXPECT_CALL(*mock,
AsyncRenameFoo(_, _, _, WithoutRequestId<RenameFooRequest>()))
AsyncRenameFoo(_, _, _, RequestIdIsUuidV4<RenameFooRequest>()))
.WillOnce(Return(ByMove(make_ready_future(
StatusOr<google::longrunning::Operation>(TransientError())))))
.WillOnce(Return(ByMove(make_ready_future(
make_status_or(google::longrunning::Operation{})))));
EXPECT_CALL(*mock, AsyncGetOperation).WillOnce([] {
Expand All @@ -187,6 +209,8 @@ TEST(RequestIdTest, LroExplicit) {
EXPECT_CALL(
*mock, AsyncRenameFoo(_, _, _,
WithRequestId<RenameFooRequest>("test-request-id")))
.WillOnce(Return(ByMove(make_ready_future(
StatusOr<google::longrunning::Operation>(TransientError())))))
.WillOnce(Return(ByMove(make_ready_future(
make_status_or(google::longrunning::Operation{})))));
EXPECT_CALL(*mock, AsyncGetOperation).WillOnce([] {
Expand All @@ -206,17 +230,17 @@ TEST(RequestIdTest, LroExplicit) {

TEST(RequestIdTest, Pagination) {
auto mock = std::make_shared<MockRequestIdServiceStub>();
std::set<std::string> sequence_ids;
std::vector<std::string> sequence_ids;
EXPECT_CALL(*mock, ListFoos(_, WithoutRequestId<ListFoosRequest>()))
.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;
Expand All @@ -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<MockRequestIdServiceStub>();
std::set<std::string> sequence_ids;
::testing::InSequence sequence;
std::vector<std::string> sequence_ids;
EXPECT_CALL(*mock,
ListFoos(_, WithRequestId<ListFoosRequest>("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<ListFoosRequest>("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;
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions generator/integration_tests/tests/request_id_idempotency_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 9ea3fb2

Please sign in to comment.