diff --git a/generator/integration_tests/tests/golden_kitchen_sink_metadata_decorator_test.cc b/generator/integration_tests/tests/golden_kitchen_sink_metadata_decorator_test.cc index 6e02c61d59d49..a24588030ff3a 100644 --- a/generator/integration_tests/tests/golden_kitchen_sink_metadata_decorator_test.cc +++ b/generator/integration_tests/tests/golden_kitchen_sink_metadata_decorator_test.cc @@ -322,7 +322,7 @@ TEST_F(MetadataDecoratorTest, ExplicitRouting) { // `google.api.routing` for Example 9: // // https://github.com/googleapis/googleapis/blob/70147caca58ebf4c8cd7b96f5d569a72723e11c1/google/api/routing.proto#L387-L390 - std::string expected1 = "table_location=instances/instance_bar"; + std::string expected1 = "table_location=instances%2Finstance_bar"; std::string expected2 = "routing_id=prof_qux"; EXPECT_CALL(*mock_, ExplicitRouting1) diff --git a/generator/integration_tests/tests/golden_kitchen_sink_rest_metadata_decorator_test.cc b/generator/integration_tests/tests/golden_kitchen_sink_rest_metadata_decorator_test.cc index 65eab28a5332c..908d91928dd05 100644 --- a/generator/integration_tests/tests/golden_kitchen_sink_rest_metadata_decorator_test.cc +++ b/generator/integration_tests/tests/golden_kitchen_sink_rest_metadata_decorator_test.cc @@ -246,10 +246,10 @@ TEST(KitchenSinkRestMetadataDecoratorTest, ExplicitRouting) { EXPECT_THAT(context.GetHeader("x-goog-quota-user"), IsEmpty()); EXPECT_THAT(context.GetHeader("x-server-timeout"), IsEmpty()); EXPECT_THAT(context.GetHeader("x-goog-request-params"), - AnyOf(Contains("table_location=instances/" + AnyOf(Contains("table_location=instances%2F" "instance_bar&routing_id=prof_qux"), Contains("routing_id=prof_qux&table_location=" - "instances/instance_bar"))); + "instances%2Finstance_bar"))); return TransientError(); }); diff --git a/generator/integration_tests/tests/golden_thing_admin_rest_metadata_decorator_test.cc b/generator/integration_tests/tests/golden_thing_admin_rest_metadata_decorator_test.cc index 938b3dafee3d6..b0bae150e09ca 100644 --- a/generator/integration_tests/tests/golden_thing_admin_rest_metadata_decorator_test.cc +++ b/generator/integration_tests/tests/golden_thing_admin_rest_metadata_decorator_test.cc @@ -216,9 +216,10 @@ TEST(ThingAdminRestMetadataDecoratorTest, DropDatabaseExplicitRoutingMatch) { EXPECT_THAT( context.GetHeader("x-goog-request-params")[0], AllOf( - HasSubstr(std::string("project=projects/my_project")), - HasSubstr(std::string("instance=instances/my_instance")), - HasSubstr(std::string("database=databases/my_database")))); + HasSubstr(std::string("project=projects%2Fmy_project")), + HasSubstr(std::string("instance=instances%2Fmy_instance")), + HasSubstr( + std::string("database=databases%2Fmy_database")))); return TransientError(); }); diff --git a/google/cloud/bigtable/internal/legacy_row_reader_test.cc b/google/cloud/bigtable/internal/legacy_row_reader_test.cc index dbc97993f656e..76d93eb9cd48e 100644 --- a/google/cloud/bigtable/internal/legacy_row_reader_test.cc +++ b/google/cloud/bigtable/internal/legacy_row_reader_test.cc @@ -127,8 +127,7 @@ class LegacyRowReaderTest : public bigtable::testing::TableTestFixture { : TableTestFixture(CompletionQueue{}), retry_policy_(std::make_unique>()), backoff_policy_(std::make_unique>()), - metadata_update_policy_(kTableName, - bigtable::MetadataParamTypes::TABLE_NAME), + metadata_update_policy_(MakeMetadataUpdatePolicy(kTableName, "")), parser_factory_( std::make_unique>()) {} diff --git a/google/cloud/bigtable/metadata_update_policy.cc b/google/cloud/bigtable/metadata_update_policy.cc index b3724ae4e12d0..690350bc76cff 100644 --- a/google/cloud/bigtable/metadata_update_policy.cc +++ b/google/cloud/bigtable/metadata_update_policy.cc @@ -15,6 +15,7 @@ #include "google/cloud/bigtable/metadata_update_policy.h" #include "google/cloud/bigtable/version.h" #include "google/cloud/internal/api_client_header.h" +#include "google/cloud/internal/url_encode.h" namespace google { namespace cloud { @@ -58,8 +59,10 @@ bigtable::MetadataUpdatePolicy MakeMetadataUpdatePolicy( // The rule is the same for all RPCs in the Data API. We always include the // table name. We append an app profile id only if one was provided. return bigtable::MetadataUpdatePolicy( - table_name + - (app_profile_id.empty() ? "" : "&app_profile_id=" + app_profile_id), + internal::UrlEncode(table_name) + + (app_profile_id.empty() + ? "" + : "&app_profile_id=" + internal::UrlEncode(app_profile_id)), bigtable::MetadataParamTypes::TABLE_NAME); } diff --git a/google/cloud/bigtable/metadata_update_policy_test.cc b/google/cloud/bigtable/metadata_update_policy_test.cc index be21f959252d0..2d3a58675c9e9 100644 --- a/google/cloud/bigtable/metadata_update_policy_test.cc +++ b/google/cloud/bigtable/metadata_update_policy_test.cc @@ -16,6 +16,7 @@ #include "google/cloud/bigtable/admin_client.h" #include "google/cloud/bigtable/table.h" #include "google/cloud/bigtable/testing/embedded_server_test_fixture.h" +#include "google/cloud/internal/url_encode.h" #include #include #include @@ -32,7 +33,8 @@ class MetadataUpdatePolicyTest : public testing::EmbeddedServerTestFixture {}; using ::testing::HasSubstr; TEST_F(MetadataUpdatePolicyTest, TableMetadata) { - grpc::string expected = "table_name=" + std::string(kTableName); + grpc::string expected = + "table_name=" + google::cloud::internal::UrlEncode(kTableName); auto reader = table_->ReadRows(RowSet("row1"), 1, Filter::PassAllFilter()); // lets make the RPC call to send metadata reader.begin(); @@ -51,8 +53,8 @@ TEST_F(MetadataUpdatePolicyTest, TableWithNewTargetMetadata) { other_table_id); grpc::string expected = - "table_name=" + - TableName(other_project_id, other_instance_id, other_table_id); + "table_name=" + google::cloud::internal::UrlEncode(TableName( + other_project_id, other_instance_id, other_table_id)); auto reader = other_table.ReadRows(RowSet("row1"), 1, Filter::PassAllFilter()); // lets make the RPC call to send metadata @@ -78,7 +80,8 @@ TEST_F(MetadataUpdatePolicyTest, SimpleDefault) { TEST_F(MetadataUpdatePolicyTest, TableAppProfileMetadata) { auto table = Table(data_client_, "profile", kTableId); grpc::string expected = - "table_name=" + std::string(kTableName) + "&app_profile_id=profile"; + "table_name=" + google::cloud::internal::UrlEncode(kTableName) + + "&app_profile_id=profile"; auto reader = table.ReadRows(RowSet("row1"), 1, Filter::PassAllFilter()); // lets make the RPC call to send metadata reader.begin(); @@ -94,7 +97,8 @@ TEST_F(MetadataUpdatePolicyTest, TableWithNewTargetAppProfileMetadata) { auto table = table_->WithNewTarget(kProjectId, kInstanceId, "profile", kTableId); grpc::string expected = - "table_name=" + std::string(kTableName) + "&app_profile_id=profile"; + "table_name=" + google::cloud::internal::UrlEncode(kTableName) + + "&app_profile_id=profile"; auto reader = table.ReadRows(RowSet("row1"), 1, Filter::PassAllFilter()); // lets make the RPC call to send metadata reader.begin(); diff --git a/google/cloud/internal/routing_matcher.h b/google/cloud/internal/routing_matcher.h index f89a9f07666e9..efcea9f8b26f1 100644 --- a/google/cloud/internal/routing_matcher.h +++ b/google/cloud/internal/routing_matcher.h @@ -15,6 +15,8 @@ #ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ROUTING_MATCHER_H #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_ROUTING_MATCHER_H +#include "google/cloud/internal/absl_str_cat_quiet.h" +#include "google/cloud/internal/url_encode.h" #include "google/cloud/version.h" #include "absl/types/optional.h" #include @@ -52,11 +54,11 @@ struct RoutingMatcher { // When the optional regex is not engaged, it is implied that we should // match the whole field. if (!pattern.re) { - params.push_back(routing_key + field); + params.push_back(absl::StrCat(routing_key, UrlEncode(field))); return; } if (std::regex_match(field, match, *pattern.re)) { - params.push_back(routing_key + match[1].str()); + params.push_back(absl::StrCat(routing_key, UrlEncode(match[1].str()))); return; } } diff --git a/google/cloud/internal/routing_matcher_test.cc b/google/cloud/internal/routing_matcher_test.cc index d6f3a0489df9d..9072130e29f38 100644 --- a/google/cloud/internal/routing_matcher_test.cc +++ b/google/cloud/internal/routing_matcher_test.cc @@ -59,9 +59,9 @@ TEST(RoutingMatcher, MatchesAll) { }}; std::vector params = {"previous"}; - auto request = TestRequest{"foo/foo", "bar/bar"}; + auto request = TestRequest{"foo", "bar"}; matcher.AppendParam(request, params); - EXPECT_THAT(params, UnorderedElementsAre("previous", "routing_id=foo/foo")); + EXPECT_THAT(params, UnorderedElementsAre("previous", "routing_id=foo")); } TEST(RoutingMatcher, EmptyFieldIsSkipped) { @@ -104,6 +104,38 @@ TEST(RoutingMatcher, FirstNonEmptyMatchIsUsed) { EXPECT_THAT(params, UnorderedElementsAre("previous", "routing_id=foo")); } +TEST(RoutingMatcher, UrlEncodesMatchAll) { + auto matcher = RoutingMatcher{ + "routing_id=", + { + {[](TestRequest const& request) -> std::string const& { + return request.foo(); + }, + absl::nullopt}, + }}; + + std::vector params = {"previous"}; + auto request = TestRequest{"foo/foo", "bar/bar"}; + matcher.AppendParam(request, params); + EXPECT_THAT(params, UnorderedElementsAre("previous", "routing_id=foo%2Ffoo")); +} + +TEST(RoutingMatcher, UrlEncodesRegex) { + auto matcher = RoutingMatcher{ + "routing_id=", + { + {[](TestRequest const& request) -> std::string const& { + return request.foo(); + }, + std::regex{"(.*)"}}, + }}; + + std::vector params = {"previous"}; + auto request = TestRequest{"foo/foo", "bar/bar"}; + matcher.AppendParam(request, params); + EXPECT_THAT(params, UnorderedElementsAre("previous", "routing_id=foo%2Ffoo")); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/testing_util/validate_metadata.cc b/google/cloud/testing_util/validate_metadata.cc index c908ab45244d8..6cdd0edd8af14 100644 --- a/google/cloud/testing_util/validate_metadata.cc +++ b/google/cloud/testing_util/validate_metadata.cc @@ -15,6 +15,7 @@ #include "google/cloud/testing_util/validate_metadata.h" #include "google/cloud/internal/absl_str_cat_quiet.h" #include "google/cloud/internal/absl_str_replace_quiet.h" +#include "google/cloud/internal/url_encode.h" #include "google/cloud/log.h" #include "google/cloud/status_or.h" #include "absl/strings/str_split.h" @@ -136,7 +137,7 @@ RoutingHeaders FromRoutingRule(google::api::RoutingRule const& routing, // If the path_template is empty, we use the field's name as the routing // param key, and we match the entire value of the field. if (path_template.empty()) { - headers[rp.field()] = field; + headers[rp.field()] = internal::UrlEncode(field); continue; } // First we parse the path_template field to extract the routing param key @@ -154,7 +155,7 @@ RoutingHeaders FromRoutingRule(google::api::RoutingRule const& routing, // Then we parse the field in the given request to see if it matches the // pattern we expect. if (std::regex_match(field, match, std::regex{pattern})) { - headers[std::move(param)] = match[1].str(); + headers[std::move(param)] = internal::UrlEncode(match[1].str()); } } return headers;