Repository navigation
Conversation
wcornell
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors REST path construction across generated stubs by replacing absl::StrCat with rest_internal::EncodePath and rest_internal::PathPiece. This change ensures that path segments are safely encoded and validated, rejecting invalid segments containing . or .. with an argument error. The generator, along with its unit and integration tests, has been updated to support and verify this new path encoding behavior. I have no further feedback to provide as there are no review comments to assess.
a52bd44 to
8534da2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16580 +/- ##
==========================================
+ Coverage 92.39% 92.42% +0.03%
==========================================
Files 2266 2268 +2
Lines 220060 220564 +504
==========================================
+ Hits 203317 203852 +535
+ Misses 16743 16712 -31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8534da2 to
8fe6ee9
Compare
8fe6ee9 to
d1cbe43
Compare
d1cbe43 to
9b076c5
Compare
9b076c5 to
4c7a4d9
Compare
| GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN | ||
| namespace { | ||
|
|
||
| bool IsInvalidSingleSegment(std::string_view value) { |
There was a problem hiding this comment.
Can we refactor this from IsInvalidSingleSegment to IsValidSingleSegment? This avoids having to reason about double negatives.
| } // namespace | ||
|
|
||
| StatusOr<std::string> EncodePathImpl(std::initializer_list<PathPiece> pieces) { | ||
| if (pieces.size() == 0) return std::string("/"); |
There was a problem hiding this comment.
It pains me that std::initializer_list::empty was not added until C++26.
| } | ||
|
|
||
| TEST(UrlEncode, HighBitBytes) { | ||
| absl::string_view constexpr kUnencodedString = "\x80\xC3\xA9\xFF"; |
There was a problem hiding this comment.
Let's make sure we use std::string_view instead of absl::string_view here and anywhere else in this PR.
We should probably update the AGENTS.md file to call this (and other absl types that we now have access to in C++17) out.
Generated REST stubs previously constructed request URI paths with
absl::StrCat, without percent-encoding path parameters or validating against./..traversal segments, unexpected/characters in single-segment fields, or malformed UTF-8.internal::UrlEncodeto strictly follow the RFC 3986 Section 2.3 unreserved character setrest_internal::EncodePath/rest_internal::PathPieceto validate and encode single-segment, multi-segment (=**), and custom-verb (:verb) path components (returningStatusCode::kInvalidArgumenton invalid inputs)EncodePathacross all generated REST stubs.The first commit contains all library, generator, golden, and test changes; the second commit contains only the regenerated Discovery
.protoand*_rest_stub.ccfiles.