-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: URL encode explicit routing values #12447
fix: URL encode explicit routing values #12447
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12447 +/- ##
==========================================
- Coverage 93.61% 93.60% -0.01%
==========================================
Files 2036 2036
Lines 180316 180340 +24
==========================================
+ Hits 168794 168810 +16
- Misses 11522 11530 +8
☔ View full report in Codecov by Sentry. |
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I was going to suggest using UrlEncode()
to construct these expected strings, but them I thought, "nah, it's fine putting exactly what is expected." But then I saw that you do use UrlEncode()
in other tests. Perhaps those latter cases are just because you have the unencoded string for other purposes? Anyway, perhaps worthy of a short ponder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so I didn't consider that. Now that I am considering it, I think I ever so slightly prefer the code the way it is.
We do not URL encode the =
, so it would look like:
auto expected1 = "table_location=" + internal::UrlEncode("instances/instance_bar");
or really...
auto expected1 = internal::UrlEncode("table_location") + "=" + internal::UrlEncode("instances/instance_bar");
.... meh
Part of the work for #12232
Update the explicit routing matchers to url encode the values in the routing pairs.
Update the part of
testing_util::ValidateMetadataFixture
that computes the expected routing headers.Then legacy bigtable also uses that testing helper, so we must update it in this PR. I separated the bigtable stuff into its own commit (hopefully) for clarity.
This change is