From 548f0f1cebf3f8894d468dffb939f570693d2506 Mon Sep 17 00:00:00 2001 From: Darren Bolduc Date: Fri, 1 Jul 2022 14:18:10 -0400 Subject: [PATCH] fix(generator): handle explicit routing params for nested fields (#9408) --- .../golden/golden_kitchen_sink_client.h | 84 ++++----- .../golden_kitchen_sink_metadata_decorator.cc | 6 +- ...en_kitchen_sink_metadata_decorator_test.cc | 162 +++++++++++++----- generator/integration_tests/test.proto | 13 ++ generator/internal/descriptor_utils.cc | 12 +- .../cloud/testing_util/validate_metadata.cc | 36 +++- 6 files changed, 223 insertions(+), 90 deletions(-) diff --git a/generator/integration_tests/golden/golden_kitchen_sink_client.h b/generator/integration_tests/golden/golden_kitchen_sink_client.h index 768f7e0d70600..af39310da0c67 100644 --- a/generator/integration_tests/golden/golden_kitchen_sink_client.h +++ b/generator/integration_tests/golden/golden_kitchen_sink_client.h @@ -110,10 +110,10 @@ class GoldenKitchenSinkClient { /// hour. /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenResponse,generator/integration_tests/test.proto#L992} + /// @return @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenResponse,generator/integration_tests/test.proto#L996} /// - /// [google.test.admin.database.v1.GenerateAccessTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L952} - /// [google.test.admin.database.v1.GenerateAccessTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L992} + /// [google.test.admin.database.v1.GenerateAccessTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L956} + /// [google.test.admin.database.v1.GenerateAccessTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L996} /// StatusOr GenerateAccessToken(std::string const& name, std::vector const& delegates, std::vector const& scope, google::protobuf::Duration const& lifetime, Options opts = {}); @@ -121,13 +121,13 @@ class GoldenKitchenSinkClient { /// /// Generates an OAuth 2.0 access token for a service account. /// - /// @param request @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenRequest,generator/integration_tests/test.proto#L952} + /// @param request @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenRequest,generator/integration_tests/test.proto#L956} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenResponse,generator/integration_tests/test.proto#L992} + /// @return @googleapis_link{google::test::admin::database::v1::GenerateAccessTokenResponse,generator/integration_tests/test.proto#L996} /// - /// [google.test.admin.database.v1.GenerateAccessTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L952} - /// [google.test.admin.database.v1.GenerateAccessTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L992} + /// [google.test.admin.database.v1.GenerateAccessTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L956} + /// [google.test.admin.database.v1.GenerateAccessTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L996} /// StatusOr GenerateAccessToken(google::test::admin::database::v1::GenerateAccessTokenRequest const& request, Options opts = {}); @@ -154,10 +154,10 @@ class GoldenKitchenSinkClient { /// token will contain `email` and `email_verified` claims. /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::GenerateIdTokenResponse,generator/integration_tests/test.proto#L1034} + /// @return @googleapis_link{google::test::admin::database::v1::GenerateIdTokenResponse,generator/integration_tests/test.proto#L1038} /// - /// [google.test.admin.database.v1.GenerateIdTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1001} - /// [google.test.admin.database.v1.GenerateIdTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1034} + /// [google.test.admin.database.v1.GenerateIdTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1005} + /// [google.test.admin.database.v1.GenerateIdTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1038} /// StatusOr GenerateIdToken(std::string const& name, std::vector const& delegates, std::string const& audience, bool include_email, Options opts = {}); @@ -165,13 +165,13 @@ class GoldenKitchenSinkClient { /// /// Generates an OpenID Connect ID token for a service account. /// - /// @param request @googleapis_link{google::test::admin::database::v1::GenerateIdTokenRequest,generator/integration_tests/test.proto#L1001} + /// @param request @googleapis_link{google::test::admin::database::v1::GenerateIdTokenRequest,generator/integration_tests/test.proto#L1005} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::GenerateIdTokenResponse,generator/integration_tests/test.proto#L1034} + /// @return @googleapis_link{google::test::admin::database::v1::GenerateIdTokenResponse,generator/integration_tests/test.proto#L1038} /// - /// [google.test.admin.database.v1.GenerateIdTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1001} - /// [google.test.admin.database.v1.GenerateIdTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1034} + /// [google.test.admin.database.v1.GenerateIdTokenRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1005} + /// [google.test.admin.database.v1.GenerateIdTokenResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1038} /// StatusOr GenerateIdToken(google::test::admin::database::v1::GenerateIdTokenRequest const& request, Options opts = {}); @@ -204,10 +204,10 @@ class GoldenKitchenSinkClient { /// See [LogEntry][google.logging.v2.LogEntry]. Test delimiter$ /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::WriteLogEntriesResponse,generator/integration_tests/test.proto#L1073} + /// @return @googleapis_link{google::test::admin::database::v1::WriteLogEntriesResponse,generator/integration_tests/test.proto#L1077} /// - /// [google.test.admin.database.v1.WriteLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1040} - /// [google.test.admin.database.v1.WriteLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1073} + /// [google.test.admin.database.v1.WriteLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1044} + /// [google.test.admin.database.v1.WriteLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1077} /// StatusOr WriteLogEntries(std::string const& log_name, std::map const& labels, Options opts = {}); @@ -221,13 +221,13 @@ class GoldenKitchenSinkClient { /// different resources (projects, organizations, billing accounts or /// folders) /// - /// @param request @googleapis_link{google::test::admin::database::v1::WriteLogEntriesRequest,generator/integration_tests/test.proto#L1040} + /// @param request @googleapis_link{google::test::admin::database::v1::WriteLogEntriesRequest,generator/integration_tests/test.proto#L1044} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::WriteLogEntriesResponse,generator/integration_tests/test.proto#L1073} + /// @return @googleapis_link{google::test::admin::database::v1::WriteLogEntriesResponse,generator/integration_tests/test.proto#L1077} /// - /// [google.test.admin.database.v1.WriteLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1040} - /// [google.test.admin.database.v1.WriteLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1073} + /// [google.test.admin.database.v1.WriteLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1044} + /// [google.test.admin.database.v1.WriteLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1077} /// StatusOr WriteLogEntries(google::test::admin::database::v1::WriteLogEntriesRequest const& request, Options opts = {}); @@ -245,7 +245,7 @@ class GoldenKitchenSinkClient { /// backoff policies. /// @return std::string /// - /// [google.test.admin.database.v1.ListLogsRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1076} + /// [google.test.admin.database.v1.ListLogsRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1080} /// StreamRange ListLogs(std::string const& parent, Options opts = {}); @@ -254,12 +254,12 @@ class GoldenKitchenSinkClient { /// Lists the logs in projects, organizations, folders, or billing accounts. /// Only logs that have entries are listed. /// - /// @param request @googleapis_link{google::test::admin::database::v1::ListLogsRequest,generator/integration_tests/test.proto#L1076} + /// @param request @googleapis_link{google::test::admin::database::v1::ListLogsRequest,generator/integration_tests/test.proto#L1080} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. /// @return std::string /// - /// [google.test.admin.database.v1.ListLogsRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1076} + /// [google.test.admin.database.v1.ListLogsRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1080} /// StreamRange ListLogs(google::test::admin::database::v1::ListLogsRequest request, Options opts = {}); @@ -280,10 +280,10 @@ class GoldenKitchenSinkClient { /// "folders/[FOLDER_ID]/locations/[LOCATION_ID]/buckets/[BUCKET_ID]/views/[VIEW_ID]" /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::TailLogEntriesResponse,generator/integration_tests/test.proto#L1334} + /// @return @googleapis_link{google::test::admin::database::v1::TailLogEntriesResponse,generator/integration_tests/test.proto#L1338} /// - /// [google.test.admin.database.v1.TailLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1302} - /// [google.test.admin.database.v1.TailLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1334} + /// [google.test.admin.database.v1.TailLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1306} + /// [google.test.admin.database.v1.TailLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1338} /// StreamRange TailLogEntries(std::vector const& resource_names, Options opts = {}); @@ -292,13 +292,13 @@ class GoldenKitchenSinkClient { /// Streaming read of log entries as they are ingested. Until the stream is /// terminated, it will continue reading logs. /// - /// @param request @googleapis_link{google::test::admin::database::v1::TailLogEntriesRequest,generator/integration_tests/test.proto#L1302} + /// @param request @googleapis_link{google::test::admin::database::v1::TailLogEntriesRequest,generator/integration_tests/test.proto#L1306} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::TailLogEntriesResponse,generator/integration_tests/test.proto#L1334} + /// @return @googleapis_link{google::test::admin::database::v1::TailLogEntriesResponse,generator/integration_tests/test.proto#L1338} /// - /// [google.test.admin.database.v1.TailLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1302} - /// [google.test.admin.database.v1.TailLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1334} + /// [google.test.admin.database.v1.TailLogEntriesRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1306} + /// [google.test.admin.database.v1.TailLogEntriesResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1338} /// StreamRange TailLogEntries(google::test::admin::database::v1::TailLogEntriesRequest const& request, Options opts = {}); @@ -316,10 +316,10 @@ class GoldenKitchenSinkClient { /// is provided, all keys are returned. /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysResponse,generator/integration_tests/test.proto#L1406} + /// @return @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysResponse,generator/integration_tests/test.proto#L1410} /// - /// [google.test.admin.database.v1.ListServiceAccountKeysRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1374} - /// [google.test.admin.database.v1.ListServiceAccountKeysResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1406} + /// [google.test.admin.database.v1.ListServiceAccountKeysRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1378} + /// [google.test.admin.database.v1.ListServiceAccountKeysResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1410} /// StatusOr ListServiceAccountKeys(std::string const& name, std::vector const& key_types, Options opts = {}); @@ -327,13 +327,13 @@ class GoldenKitchenSinkClient { /// /// Lists every [ServiceAccountKey][google.iam.admin.v1.ServiceAccountKey] for a service account. /// - /// @param request @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysRequest,generator/integration_tests/test.proto#L1374} + /// @param request @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysRequest,generator/integration_tests/test.proto#L1378} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. - /// @return @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysResponse,generator/integration_tests/test.proto#L1406} + /// @return @googleapis_link{google::test::admin::database::v1::ListServiceAccountKeysResponse,generator/integration_tests/test.proto#L1410} /// - /// [google.test.admin.database.v1.ListServiceAccountKeysRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1374} - /// [google.test.admin.database.v1.ListServiceAccountKeysResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1406} + /// [google.test.admin.database.v1.ListServiceAccountKeysRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1378} + /// [google.test.admin.database.v1.ListServiceAccountKeysResponse]: @googleapis_reference_link{generator/integration_tests/test.proto#L1410} /// StatusOr ListServiceAccountKeys(google::test::admin::database::v1::ListServiceAccountKeysRequest const& request, Options opts = {}); @@ -384,11 +384,11 @@ class GoldenKitchenSinkClient { /// x-goog-request-params: /// table_location=instances/instance_bar&routing_id=prof_qux /// - /// @param request @googleapis_link{google::test::admin::database::v1::ExplicitRoutingRequest,generator/integration_tests/test.proto#L1413} + /// @param request @googleapis_link{google::test::admin::database::v1::ExplicitRoutingRequest,generator/integration_tests/test.proto#L1417} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. /// - /// [google.test.admin.database.v1.ExplicitRoutingRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1413} + /// [google.test.admin.database.v1.ExplicitRoutingRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1417} /// Status ExplicitRouting1(google::test::admin::database::v1::ExplicitRoutingRequest const& request, Options opts = {}); @@ -397,11 +397,11 @@ class GoldenKitchenSinkClient { /// We use this RPC to verify the special case where a routing parameter key /// does not require a regex in order to match the correct value. /// - /// @param request @googleapis_link{google::test::admin::database::v1::ExplicitRoutingRequest,generator/integration_tests/test.proto#L1413} + /// @param request @googleapis_link{google::test::admin::database::v1::ExplicitRoutingRequest,generator/integration_tests/test.proto#L1417} /// @param opts Optional. Override the class-level options, such as retry and /// backoff policies. /// - /// [google.test.admin.database.v1.ExplicitRoutingRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1413} + /// [google.test.admin.database.v1.ExplicitRoutingRequest]: @googleapis_reference_link{generator/integration_tests/test.proto#L1417} /// Status ExplicitRouting2(google::test::admin::database::v1::ExplicitRoutingRequest const& request, Options opts = {}); diff --git a/generator/integration_tests/golden/internal/golden_kitchen_sink_metadata_decorator.cc b/generator/integration_tests/golden/internal/golden_kitchen_sink_metadata_decorator.cc index c9d849efad200..8b9e3ee8f654c 100644 --- a/generator/integration_tests/golden/internal/golden_kitchen_sink_metadata_decorator.cc +++ b/generator/integration_tests/golden/internal/golden_kitchen_sink_metadata_decorator.cc @@ -175,7 +175,7 @@ GoldenKitchenSinkMetadata::ExplicitRouting2( grpc::ClientContext& context, google::test::admin::database::v1::ExplicitRoutingRequest const& request) { std::vector params; - params.reserve(1); + params.reserve(2); if (!request.app_profile_id().empty()) { params.push_back("no_regex_needed=" + request.app_profile_id()); @@ -185,6 +185,10 @@ GoldenKitchenSinkMetadata::ExplicitRouting2( params.push_back("no_regex_needed=" + request.no_regex_needed()); } + if (!request.nested1().nested2().value().empty()) { + params.push_back("routing_id=" + request.nested1().nested2().value()); + } + if (params.empty()) { SetMetadata(context); } else { diff --git a/generator/integration_tests/golden/tests/golden_kitchen_sink_metadata_decorator_test.cc b/generator/integration_tests/golden/tests/golden_kitchen_sink_metadata_decorator_test.cc index adbe5d0dd11d2..24469edf63bb2 100644 --- a/generator/integration_tests/golden/tests/golden_kitchen_sink_metadata_decorator_test.cc +++ b/generator/integration_tests/golden/tests/golden_kitchen_sink_metadata_decorator_test.cc @@ -42,11 +42,11 @@ using ::google::test::admin::database::v1::TailLogEntriesResponse; using ::google::test::admin::database::v1::WriteObjectRequest; using ::google::test::admin::database::v1::WriteObjectResponse; using ::testing::_; +using ::testing::AnyOf; using ::testing::Contains; using ::testing::Not; using ::testing::Pair; using ::testing::Return; -using ::testing::UnorderedElementsAre; class MetadataDecoratorTest : public ::testing::Test { protected: @@ -320,81 +320,165 @@ TEST_F(MetadataDecoratorTest, AsyncWriteObject) { } TEST_F(MetadataDecoratorTest, ExplicitRouting) { + // In `test.proto` we define the `ExplicitRouting1` rpc to have the same + // routing parameters as Example 9 from the `google.api.routing` proto. + // + // In this test, we will use the request message provided in the + // `google.api.routing` examples: + // + // https://github.com/googleapis/googleapis/blob/f46dc249e1987a6bef1a70a371e8288ea4c17481/google/api/routing.proto#L57-L60 + google::test::admin::database::v1::ExplicitRoutingRequest request; + request.set_table_name( + "projects/proj_foo/instances/instance_bar/tables/table_baz"); + request.set_app_profile_id("profiles/prof_qux"); + + // We verify the routing metadata against the expectations provided in + // `google.api.routing` for Example 9: + // + // https://github.com/googleapis/googleapis/blob/f46dc249e1987a6bef1a70a371e8288ea4c17481/google/api/routing.proto#L387-L390 + std::string expected1 = "table_location=instances/instance_bar"; + std::string expected2 = "routing_id=prof_qux"; + EXPECT_CALL(*mock_, ExplicitRouting1) .WillOnce([this](grpc::ClientContext& context, google::test::admin::database::v1:: - ExplicitRoutingRequest const&) { - // Even though IsContextMDValid can do this work for us, let's spell out - // what we expect to find in the routing header. + ExplicitRoutingRequest const& request) { + IsContextMDValid( + context, + "google.test.admin.database.v1.GoldenKitchenSink.ExplicitRouting1", + request); + return Status(); + }) + .WillOnce([&, this](grpc::ClientContext& context, + google::test::admin::database::v1:: + ExplicitRoutingRequest const&) { auto headers = GetMetadata(context); - auto it = headers.find("x-goog-request-params"); - EXPECT_NE(it, headers.end()); - if (it == headers.end()) return Status(StatusCode::kAborted, "fail"); - auto pairs = absl::StrSplit(it->second, "&"); - // We verify the result against this expectation: - // https://github.com/googleapis/googleapis/blob/f46dc249e1987a6bef1a70a371e8288ea4c17481/google/api/routing.proto#L387-L390 - EXPECT_THAT( - pairs, UnorderedElementsAre("table_location=instances/instance_bar", - "routing_id=prof_qux")); + EXPECT_THAT(headers, + Contains(Pair("x-goog-request-params", + // We use `AnyOf` because it does not matter + // which order the parameters are added in. + AnyOf(expected1 + "&" + expected2, + expected2 + "&" + expected1)))); return Status(); }); GoldenKitchenSinkMetadata stub(mock_); - grpc::ClientContext context; - // Our request comes from the examples in the `google.api.routing` proto: - // https://github.com/googleapis/googleapis/blob/f46dc249e1987a6bef1a70a371e8288ea4c17481/google/api/routing.proto#L57-L60 - google::test::admin::database::v1::ExplicitRoutingRequest request; - request.set_table_name( - "projects/proj_foo/instances/instance_bar/tables/table_baz"); - request.set_app_profile_id("profiles/prof_qux"); - auto status = stub.ExplicitRouting1(context, request); - EXPECT_STATUS_OK(status); + grpc::ClientContext context1; + grpc::ClientContext context2; + // We make the same call twice. In the first call, we use `IsContextMDValid` + // to verify expectations. In the second call, we verify the routing + // parameters by hand. This gives us extra confidence in `IsContextMDValid` + // which is reasonably complex, but untested. (We cannot do them both in the + // same call, because the `grpc::ClientContext` is consumed in order to + // extract its metadata). + (void)stub.ExplicitRouting1(context1, request); + (void)stub.ExplicitRouting1(context2, request); } TEST_F(MetadataDecoratorTest, ExplicitRoutingDoesNotSendEmptyParams) { EXPECT_CALL(*mock_, ExplicitRouting1) + .WillOnce([this](grpc::ClientContext& context, + google::test::admin::database::v1:: + ExplicitRoutingRequest const& request) { + IsContextMDValid( + context, + "google.test.admin.database.v1.GoldenKitchenSink.ExplicitRouting1", + request); + return Status(); + }) .WillOnce([this](grpc::ClientContext& context, google::test::admin::database::v1:: ExplicitRoutingRequest const&) { - // Even though IsContextMDValid can do this work for us, let's spell out - // what we expect to find in the routing header. auto headers = GetMetadata(context); - auto it = headers.find("x-goog-request-params"); - EXPECT_EQ(it, headers.end()); + EXPECT_THAT(headers, Not(Contains(Pair("x-goog-request-params", _)))); return Status(); }); GoldenKitchenSinkMetadata stub(mock_); - grpc::ClientContext context; + grpc::ClientContext context1; + grpc::ClientContext context2; google::test::admin::database::v1::ExplicitRoutingRequest request; request.set_table_name("does-not-match"); - (void)stub.ExplicitRouting1(context, request); + // We make the same call twice. In the first call, we use `IsContextMDValid` + // to verify expectations. In the second call, we verify the routing + // parameters by hand. This gives us extra confidence in `IsContextMDValid` + // which is reasonably complex, but untested. (We cannot do them both in the + // same call, because the `grpc::ClientContext` is consumed in order to + // extract its metadata). + (void)stub.ExplicitRouting1(context1, request); + (void)stub.ExplicitRouting1(context2, request); } TEST_F(MetadataDecoratorTest, ExplicitRoutingNoRegexNeeded) { EXPECT_CALL(*mock_, ExplicitRouting2) + .WillOnce([this](grpc::ClientContext& context, + google::test::admin::database::v1:: + ExplicitRoutingRequest const& request) { + IsContextMDValid( + context, + "google.test.admin.database.v1.GoldenKitchenSink.ExplicitRouting2", + request); + return Status(); + }) .WillOnce([this](grpc::ClientContext& context, google::test::admin::database::v1:: ExplicitRoutingRequest const&) { - // Even though IsContextMDValid can do this work for us, let's spell out - // what we expect to find in the routing header. auto headers = GetMetadata(context); - auto it = headers.find("x-goog-request-params"); - EXPECT_NE(it, headers.end()); - if (it == headers.end()) return Status(StatusCode::kAborted, "fail"); - auto pairs = absl::StrSplit(it->second, "&"); - EXPECT_THAT(pairs, UnorderedElementsAre("no_regex_needed=used")); + EXPECT_THAT(headers, Contains(Pair("x-goog-request-params", + "no_regex_needed=used"))); return Status(); }); GoldenKitchenSinkMetadata stub(mock_); - grpc::ClientContext context; + grpc::ClientContext context1; + grpc::ClientContext context2; + // Note that the `app_profile_id` field is not set. google::test::admin::database::v1::ExplicitRoutingRequest request; request.set_table_name("used"); request.set_no_regex_needed("ignored"); - // Note that the `app_profile_id` field is not set. - auto status = stub.ExplicitRouting2(context, request); - EXPECT_STATUS_OK(status); + // We make the same call twice. In the first call, we use `IsContextMDValid` + // to verify expectations. In the second call, we verify the routing + // parameters by hand. This gives us extra confidence in `IsContextMDValid` + // which is reasonably complex, but untested. (We cannot do them both in the + // same call, because the `grpc::ClientContext` is consumed in order to + // extract its metadata). + (void)stub.ExplicitRouting2(context1, request); + (void)stub.ExplicitRouting2(context2, request); +} + +TEST_F(MetadataDecoratorTest, ExplicitRoutingNestedField) { + EXPECT_CALL(*mock_, ExplicitRouting2) + .WillOnce([this](grpc::ClientContext& context, + google::test::admin::database::v1:: + ExplicitRoutingRequest const& request) { + IsContextMDValid( + context, + "google.test.admin.database.v1.GoldenKitchenSink.ExplicitRouting2", + request); + return Status(); + }) + .WillOnce([this](grpc::ClientContext& context, + google::test::admin::database::v1:: + ExplicitRoutingRequest const&) { + auto headers = GetMetadata(context); + EXPECT_THAT(headers, Contains(Pair("x-goog-request-params", + "routing_id=value"))); + return Status(); + }); + + GoldenKitchenSinkMetadata stub(mock_); + grpc::ClientContext context1; + grpc::ClientContext context2; + google::test::admin::database::v1::ExplicitRoutingRequest request; + request.mutable_nested1()->mutable_nested2()->set_value("value"); + // We make the same call twice. In the first call, we use `IsContextMDValid` + // to verify expectations. In the second call, we verify the routing + // parameters by hand. This gives us extra confidence in `IsContextMDValid` + // which is reasonably complex, but untested. (We cannot do them both in the + // same call, because the `grpc::ClientContext` is consumed in order to + // extract its metadata). + (void)stub.ExplicitRouting2(context1, request); + (void)stub.ExplicitRouting2(context2, request); } } // namespace diff --git a/generator/integration_tests/test.proto b/generator/integration_tests/test.proto index 2506bf285a39a..f04ea9615c40b 100644 --- a/generator/integration_tests/test.proto +++ b/generator/integration_tests/test.proto @@ -929,6 +929,10 @@ service GoldenKitchenSink { field: "app_profile_id" path_template: "{no_regex_needed=**}" } + routing_parameters { + field: "nested1.nested2.value" + path_template: "{routing_id=**}" + } }; } } @@ -1427,4 +1431,13 @@ message ExplicitRoutingRequest { // We use this field to demonstrate the special case for routing parameter // keys which do not require a regex in order to match the correct value. string no_regex_needed = 3; + + // We use this field to test routing keys for nested types. + message Nested1 { + message Nested2 { + string value = 1; + } + optional Nested2 nested2 = 1; + } + Nested1 nested1 = 4; } diff --git a/generator/internal/descriptor_utils.cc b/generator/internal/descriptor_utils.cc index ae56a2a0f21fe..7f41a4f2d3f8b 100644 --- a/generator/internal/descriptor_utils.cc +++ b/generator/internal/descriptor_utils.cc @@ -556,12 +556,18 @@ ExplicitRoutingInfo ParseExplicitRoutingHeader( google::api::RoutingRule rule = method.options().GetExtension(google::api::routing); - auto const* input_type = method.input_type(); auto const& rps = rule.routing_parameters(); // We use reverse iterators so that "last wins" becomes "first wins". for (auto it = rps.rbegin(); it != rps.rend(); ++it) { - auto const* descriptor = input_type->FindFieldByName(it->field()); - auto field_name = FieldName(descriptor); + std::vector chunks; + auto const* input_type = method.input_type(); + for (auto const& sv : absl::StrSplit(it->field(), '.')) { + auto const chunk = std::string(sv); + auto const* chunk_descriptor = input_type->FindFieldByName(chunk); + chunks.push_back(FieldName(chunk_descriptor)); + input_type = chunk_descriptor->message_type(); + } + auto field_name = absl::StrJoin(chunks, "()."); auto const& path_template = it->path_template(); // When a path_template is not supplied, we use the field name as the // routing parameter key. The pattern matches the whole value of the field. diff --git a/google/cloud/testing_util/validate_metadata.cc b/google/cloud/testing_util/validate_metadata.cc index 88865044e7ead..7e9c08b10c835 100644 --- a/google/cloud/testing_util/validate_metadata.cc +++ b/google/cloud/testing_util/validate_metadata.cc @@ -17,6 +17,7 @@ #include "google/cloud/internal/absl_str_replace_quiet.h" #include "google/cloud/log.h" #include "google/cloud/status_or.h" +#include "absl/strings/str_split.h" #include #include #include @@ -26,9 +27,14 @@ #include #include #include +#include #include #include +// Undefine a Windows macro, which conflicts with +// `protobuf::Reflection::GetMessage` +#undef GetMessage + namespace google { namespace cloud { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN @@ -83,6 +89,22 @@ MATCHER_P(MatchesGlob, glob, "matches the glob: \"" + glob + "\"") { return std::regex_match(arg, regex); } +// This method is recursive because dbolduc could not figure out the iterative +// solution. +std::string GetField( // NOLINT(misc-no-recursion) + std::deque fields, + google::protobuf::Descriptor const* input_type, + google::protobuf::Message const& msg) { + if (fields.empty()) { + GCP_LOG(FATAL) << "Empty field name defined in RoutingRule."; + } + auto const* fd = input_type->FindFieldByName(fields.front()); + fields.pop_front(); + if (fields.empty()) return msg.GetReflection()->GetString(msg, fd); + auto const& sub_msg = msg.GetReflection()->GetMessage(msg, fd); + return GetField(fields, fd->message_type(), sub_msg); +} + /** * Parse the `RoutingRule` proto as described in the proto comments: * https://github.com/googleapis/googleapis/blob/master/google/api/routing.proto @@ -94,13 +116,18 @@ MATCHER_P(MatchesGlob, glob, "matches the glob: \"" + glob + "\"") { * overwrite the current value in the map, because the "last match wins". */ RoutingHeaders FromRoutingRule(google::api::RoutingRule const& routing, - google::protobuf::Descriptor const* input_type, + google::protobuf::MethodDescriptor const* method, google::protobuf::Message const& request) { RoutingHeaders headers; for (auto const& rp : routing.routing_parameters()) { auto const& path_template = rp.path_template(); - auto const* fd = input_type->FindFieldByName(rp.field()); - auto const& field = request.GetReflection()->GetString(request, fd); + // Some fields may look like: "nested1.nested2.value", where `nested1` and + // `nested2` are generic Messages, and `value` is the string field we are to + // match against. We must iterate over the nested messages to get to the + // string value. + std::deque fields = absl::StrSplit(rp.field(), "."); + auto const& field = GetField(fields, method->input_type(), request); + // We skip empty fields. if (field.empty()) continue; // If the path_template is empty, we use the field's name as the routing @@ -184,8 +211,7 @@ RoutingHeaders ExtractRoutingHeaders( auto options = method_desc->options(); if (options.HasExtension(google::api::routing)) { auto const& routing = options.GetExtension(google::api::routing); - auto const* input_type = method_desc->input_type(); - return FromRoutingRule(routing, input_type, request); + return FromRoutingRule(routing, method_desc, request); } if (options.HasExtension(google::api::http)) { auto const& http = options.GetExtension(google::api::http);