-
Notifications
You must be signed in to change notification settings - Fork 5.5k
move LowerCaseString to common for common use #16539
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
Closed
Closed
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8ff0501
mv lowercasestring to common for common use
0ecec1b
fix test
82139be
fix clang-tidy
468ad2b
Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…
7b04853
Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…
03db6bf
add comments to validated lowercasestring template
cfe8f3a
Merge branch 'main' of https://github.com/envoyproxy/envoy into case-…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "common/common/assert.h" | ||
|
|
||
| #include "absl/strings/ascii.h" | ||
| #include "absl/strings/string_view.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| /** | ||
| * Wrapper for case insensitive string to generally avoid needless case insensitive compares. The | ||
| * wrapper will uniformly convert the string content to lowercase. | ||
| */ | ||
| class LowerCaseStrBase { | ||
| public: | ||
| explicit LowerCaseStrBase(absl::string_view new_string) : string_(new_string) { lower(); } | ||
|
|
||
| LowerCaseStrBase(LowerCaseStrBase&&) = default; | ||
| LowerCaseStrBase& operator=(LowerCaseStrBase&&) = default; | ||
|
|
||
| LowerCaseStrBase(const LowerCaseStrBase&) = default; | ||
| LowerCaseStrBase& operator=(const LowerCaseStrBase&) = default; | ||
|
|
||
| const std::string& get() const { return string_; } | ||
| std::string& get() { return string_; } | ||
|
|
||
| bool operator==(const LowerCaseStrBase& rhs) const { return string_ == rhs.string_; } | ||
| bool operator!=(const LowerCaseStrBase& rhs) const { return string_ != rhs.string_; } | ||
| bool operator<(const LowerCaseStrBase& rhs) const { return string_.compare(rhs.string_) < 0; } | ||
|
|
||
| friend std::ostream& operator<<(std::ostream& os, const LowerCaseStrBase& lower_case_string) { | ||
| return os << lower_case_string.string_; | ||
| } | ||
|
|
||
| protected: | ||
| void lower() { | ||
| std::transform(string_.begin(), string_.end(), string_.begin(), absl::ascii_tolower); | ||
| } | ||
|
|
||
| std::string string_; | ||
| }; | ||
|
|
||
| template <bool (*V)(absl::string_view)> class ValidatedLowerCaseStr : public LowerCaseStrBase { | ||
| public: | ||
| ValidatedLowerCaseStr(LowerCaseStrBase&& rhs) noexcept : LowerCaseStrBase(std::move(rhs)) { | ||
| ASSERT(valid()); | ||
| } | ||
| ValidatedLowerCaseStr& operator=(LowerCaseStrBase&& rhs) noexcept { | ||
| string_ = std::move(rhs.get()); | ||
| ASSERT(valid()); | ||
| return *this; | ||
| } | ||
|
|
||
| ValidatedLowerCaseStr(const LowerCaseStrBase& rhs) : LowerCaseStrBase(rhs) { ASSERT(valid()); } | ||
| ValidatedLowerCaseStr& operator=(const LowerCaseStrBase& rhs) { | ||
| string_ = rhs.get(); | ||
| ASSERT(valid()); | ||
| return *this; | ||
| } | ||
|
|
||
| explicit ValidatedLowerCaseStr(absl::string_view new_string) : LowerCaseStrBase(new_string) { | ||
| ASSERT(valid()); | ||
| } | ||
|
|
||
| private: | ||
| bool valid() const { return V(string_); } | ||
| }; | ||
|
|
||
| } // namespace Envoy | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ class OpenTracingHTTPHeadersWriter : public opentracing::HTTPHeadersWriter { | |
| // opentracing::HTTPHeadersWriter | ||
| opentracing::expected<void> Set(opentracing::string_view key, | ||
| opentracing::string_view value) const override { | ||
| Http::LowerCaseString lowercase_key{key}; | ||
| Http::LowerCaseString lowercase_key{{key.data(), key.size()}}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be moved to a different PR? Doesn't seem like this should be needed in this one. |
||
| request_headers_.remove(lowercase_key); | ||
| request_headers_.addCopy(std::move(lowercase_key), {value.data(), value.size()}); | ||
| return {}; | ||
|
|
@@ -50,7 +50,7 @@ class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { | |
| // opentracing::HTTPHeadersReader | ||
| opentracing::expected<opentracing::string_view> | ||
| LookupKey(opentracing::string_view key) const override { | ||
| const auto entry = request_headers_.get(Http::LowerCaseString{key}); | ||
| const auto entry = request_headers_.get(Http::LowerCaseString{{key.data(), key.size()}}); | ||
| if (!entry.empty()) { | ||
| // This is an implicitly untrusted header, so only the first value is used. | ||
| return opentracing::string_view{entry[0]->value().getStringView().data(), | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure why we need this to be a template.
Uh oh!
There was an error while loading. Please reload this page.
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.
@lizan Because I want to get a protocol-independent lower case string and be able to set a different validator. The template is used so that the validator can be pluggable.
Here are some discussions about this question in original PR #16049.
@jmarantz #16049 (comment)
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.
OK I see, then this should be commented why it is template. and in Http, it should be named like validateHttpLowerCaseString.
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.
TBH my recollection from the.other PR is that there are a few jumbled concepts here and I want to just quickly mention my line of thinking.
IMO if you really need a validator, forget whether there's a compelling reason to prefer
But if we do use a function-pointer then in IMO it could be a std::function rather than a templatized function pointer.
Also I think once we are plugging in validation, there's no compelling reason to use LowerCaseString just for Tracing's sake, unless there's something about Tracing that wants strings to be lower-cased. IDK whether that's the case.
That said, I have no fundamental objection to moving LowerCaseString to common as long as it doesn't make anything slower for HTTP headers, which are performance critical.
I don't understand why LowerCaseString has to be paired with a validator though. Can't you just leave LowerCaseString exactly like it is, and move it out of http into common? That seems like a legitimate concept on its own. You could make another layer that adds validation if this is helpful in some way.
Uh oh!
There was an error while loading. Please reload this page.
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.
@lizan I will add some new comments to explain why we need this template. 👌
In Http, I kept the original LowerCaseString name so as not to affect the existing code as much as possible. After all, the name Http::LowerCaseString is already widely used. 🤔
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.
@jmarantz
LowerCaseStrBaseas a common implementation of LowerCaseString which with no any validation and can be used directly. Then I provideValidatedLowerCaseStrtemplate just as a helper for who wants to add validation to LowerCaseString, for example, Http.So that means I've got two layers as you think. One layer is the plain
LowerCaseString, and the second layer is theValidatedLowerCaseStrthat can mount a different validator.Uh oh!
There was an error while loading. Please reload this page.
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.
@jmarantz At the source code level, it is move. I just kept the previous name
LowerCaseStringinstead of renaming it toValidatedHttpLowerCaseStringor something like that to make PR clean.One of the key reasons Trace needs LowerCaseString is performance. But that can be discussed in the new PR.
This PR doesn't really have anything to do with Trace so far. Although the cause is Trace, for now this PR can be seen as a PR that simply strips LowerCaseString out of Http to make it more generic.
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.
OK...sorry, TBH I'm skeptical about the overall strategy and also concerned there may be overhead added to http headers.
But for the moment maybe it'd be better to focus on strategy. Do you want to describe the high level of what you are trying to achieve in a bug or a google doc? It might be easier to iterate there.
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks for you comments and patience. @jmarantz
In the original PR #16049, there is the high level design of the general tracing. And I remember I had a more detailed discussion with you in slack earlier.
For now, I think we can try to focus on this PR first. Because this PR don't involve the design of the new general tracing yet, just some pre-work. This is also the reason why I split the original PR into multiple PRs.
I think we can discuss the design of general tracing in detail in the new PR (One or two weeks after the completion of this PR).
For this PR I think the most important points are the following:
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.
I think PR is probably OK but I'll let Lizan review the details. It'd be better not to do it unless it's needed though.
I am not understanding why you need lower-case semantics at all for the new tracers. There may be a good reason but it's not clear. Iterating in a doc may be easier, rather than this PR.