impl(rest): improve http header representation#16058
impl(rest): improve http header representation#16058scotthart wants to merge 10 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 introduces the HttpHeaderName class to handle case-insensitive HTTP header names by storing them in lowercase and refactors HttpHeader, RestContext, and RestRequest to use this new type along with absl::flat_hash_map for header storage. Feedback identifies opportunities to improve efficiency by removing redundant lowercase conversions in HttpHeader comparison and key-checking methods, and suggests using explicit HttpHeader construction in tests to improve clarity.
scotthart
force-pushed
the
rab_prototype
branch
from
83fe4c2 to
d39ef3b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16058 +/- ##
=======================================
Coverage 92.69% 92.69%
=======================================
Files 2343 2343
Lines 216541 216598 +57
=======================================
+ Hits 200729 200783 +54
- Misses 15812 15815 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // 32-bit architecture | ||
|
|
||
| GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END | ||
| } // namespace rest_internal |
There was a problem hiding this comment.
Changing nesting levels within pre-processor conditionals is generally considered bad practice.
google/cloud/internal/http_header.h
Outdated
| std::vector<std::string> values_; | ||
| }; | ||
|
|
||
| // Abseil does not guarantee compatibility with 32-bit targets. |
There was a problem hiding this comment.
I'm not sure what that is supposed to mean. Consider something more descriptive.
There was a problem hiding this comment.
Updated. Additionally, despite modest efforts on my part, I could not get the m32-pr build to compile using flat_hash_map with a custom key type using the 32-bit version of abseil provided by fedora. Perhaps with additional work this could be made to work, but it's currently not a priority.
google/cloud/internal/http_header.h
Outdated
| #if defined(_WIN64) || defined(__LP64__) || defined(__x86_64__) || \ | ||
| defined(__ppc64__) |
There was a problem hiding this comment.
Rather than trying to enumerate 64-bit targets, you might consider something like #if UINTPTR_MAX == UINT64_MAX (i.e., is a pointer 64 bits).
There was a problem hiding this comment.
Thank you! I was trying to find a better conditional for this.
google/cloud/internal/http_header.h
Outdated
| #if defined(_WIN64) || defined(__LP64__) || defined(__x86_64__) || \ | ||
| defined(__ppc64__) |
There was a problem hiding this comment.
Use the same conditional here.
| // not test with. Support for such platforms is a community effort. Using | ||
| // std::unordered_map on 32-bit platforms reduces the likelihood of issues | ||
| // arising due to this arrangement. | ||
| #if UINTPTR_MAX == UINT64_MAX |
| } // namespace cloud | ||
| } // namespace google | ||
|
|
||
| #if UINTPTR_MAX != UINT64_MAX |
There was a problem hiding this comment.
Forgive my memory failings, but how does absl::flat_hash_map know how to hash a HttpHeaderName?
There was a problem hiding this comment.
The HttpHeaderName type has an implicit std::string conversion operator.
| #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_HTTP_HEADER_H | ||
|
|
||
| #include "google/cloud/version.h" | ||
| #if UINTPTR_MAX == UINT64_MAX |
There was a problem hiding this comment.
Better include <cstdint> before you use it.
| HttpHeaderName(char const* name) // NOLINT(google-explicit-constructor) | ||
| : HttpHeaderName(std::string{name}) {} | ||
|
|
||
| operator std::string() const { // NOLINT(google-explicit-constructor) |
There was a problem hiding this comment.
Does NOLINT(google-explicit-constructor) do anything on a non-constructor?
There was a problem hiding this comment.
Yes. There is not a different NOLINT for implicit conversion operators.
3 participants
Using plain
std::stringto represent HTTP headers, particularly the header names, requires all code locations to deal with the mismatch betweenstd::stringbeing case-sensitive and HTTP header names being case-insensitive. This PR extends the use of theHttpHeadertype and introduces theHttpHeaderNametype to consolidate such code. Additionally, it updates the definition of a collection ofHttpHeaderobjects inHttpHeaders.