fix: add implicit routing in GAPICs #12544
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12544 +/- ##
=======================================
Coverage 93.63% 93.63%
=======================================
Files 2044 2044
Lines 181258 181278 +20
=======================================
+ Hits 169725 169748 +23
+ Misses 11533 11530 -3
☔ View full report in Codecov by Sentry. |
| auto matcher1 = absl::StrReplaceAll(glob, {{"*", "[^/]+"}}); | ||
| // Create a second matcher that replaces all "/" with "%2F" to match the | ||
| // character replacement in `internal::UrlEncode`. | ||
| auto decoded_glob = absl::StrReplaceAll(glob, {{"/", "%2F"}}); |
There was a problem hiding this comment.
It doesn't make sense to only decode one character. We want this test fixture to work, even if we provide it strings that have other characters that need escaping. We just happen not to do that at the moment.
| // character replacement in `internal::UrlEncode`. | ||
| auto decoded_glob = absl::StrReplaceAll(glob, {{"/", "%2F"}}); | ||
| auto matcher2 = absl::StrReplaceAll(decoded_glob, {{"*", "[^/]+"}}); | ||
| auto regex_string = absl::StrCat(matcher1, "|", matcher2); |
There was a problem hiding this comment.
Checking for either an escaped string or not seems to defeat the purpose. We should be verifying that every value is escaped properly.
I don't have a slick way to do it, but this seems like a recipe for false positives.
There was a problem hiding this comment.
So i think the reason we are checking for either is because of what the test is passing in.
Some tests pass in projects/*/resource/* while others are passing in projects%2F*%2Fresource%2F*%2F
It seems easier to understand/reason (as a human) if when we look at these tests we can see the / instead of the escaped characters.
There was a problem hiding this comment.
Some tests pass in projects//resource/ while others are passing in projects%2F*%2Fresource%2F*%2F
nit: Some tests pass in projects/*/resource/* while others are passing in projects%2Ffoo%2Fresource%2Fbar%2F. I think the implicit ones (with *s) do not encode, and the explicit ones (without *s) do.
We should find a way to verify that URL encoding works without false positives.
It seems easier to understand/reason (as a human) if when we look at these tests we can see the / instead of the escaped characters.
Yeah, I think I agree. That can be made to work by saying UrlEncode("foo/bar/baz") in the tests.
| @@ -89,7 +89,13 @@ RoutingHeaders ExtractMDFromHeader(std::string header) { | |||
| */ | |||
| MATCHER_P(MatchesGlob, glob, "matches the glob: \"" + glob + "\"") { | |||
There was a problem hiding this comment.
So part of the problem with this matcher is that we use it both for implicit and explicit routing cases. I think we (and by we I mean me) were too clever for our own good in the past.
With explicit routing we do not pass this thing any globs. (i.e. there are no '*' in the string). If we are able to inspect the contents of the protobuf messages to verify the exact value for explicit routing, we really should be able to do the same thing for the implicit case. Right? (I am genuinely asking. I am like 80% sure)
If so, we would not even need this MatchesGlob thing. We would just match on a string. That would be wayyy simpler. (assuming it is possible).
There was a problem hiding this comment.
To give a concrete example... If we did this with spanner's BatchCreateSessions:
Then FromHttpRule would return {{"database", "projects/*/instances/*/databases/*"}}
google-cloud-cpp/google/cloud/testing_util/validate_metadata.cc
Lines 221 to 224 in c91050b
But we could have that FromHttpRule accept the method and the request, the way FromRoutingRule does.
Disclaimer: I am not sure this approach is worth doing though. It could be a time trap.
Part of #12232
Previously was PR #12501
This change is