impl(rest): update AuthenticationHeader method to return multiple headers#16061
impl(rest): update AuthenticationHeader method to return multiple headers#16061scotthart wants to merge 12 commits intogoogleapis:mainfrom
Conversation
![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 HTTP header handling by introducing the HttpHeaderName class for case-insensitive keys and a dedicated HttpHeader class, replacing previous implementations that used raw maps and string pairs. The authentication logic was updated to support multiple headers through the new AuthenticationHeaders and AuthenticationHeadersJoined functions. Feedback indicates that joining multiple headers with a semicolon in AuthenticationHeadersJoined is non-standard and may result in invalid HTTP fields; it is recommended to use \r\n or add assertions if only one header is expected. Additionally, a comment in http_header.h regarding the preservation of header case is now outdated and should be corrected.
| return absl::StrJoin(*headers, ";", | ||
| [](std::string* s, rest_internal::HttpHeader const& h) { | ||
| *s += std::string{h}; | ||
| }); |
There was a problem hiding this comment.
Joining multiple HTTP headers with a semicolon is not a standard practice for representing multiple headers. This will produce a string like header1: value1;header2: value2, which is not a valid HTTP header field. HTTP headers should be on separate lines, separated by \r\n.
If callers of this function might receive multiple headers, this implementation could cause issues. If this function is only intended to handle a single header, it would be safer to add an assertion (e.g., GCP_ASSERT(headers->size() <= 1);) to make this assumption explicit and prevent future problems.
References
- Prefer defensive code, such as explicit ok() checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.
google/cloud/internal/http_header.h
Outdated
| // While the RFCs indicate that header keys are case-insensitive, no attempt | ||
| // to convert them to all lowercase is made. Header keys are printed in the |
There was a problem hiding this comment.
This comment is now outdated. With the introduction of HttpHeaderName, header keys are converted to lowercase, and the original case is lost. When printed, they will be in lowercase. This comment should be removed or updated to avoid confusion.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16061 +/- ##
==========================================
- Coverage 92.69% 92.68% -0.02%
==========================================
Files 2343 2343
Lines 216541 216577 +36
==========================================
+ Hits 200729 200740 +11
- Misses 15812 15837 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scotthart
force-pushed
the
rab_prototype_2
branch
from
21728d8 to
ae7fe69
Compare
1 participant
No description provided.