Skip to content
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.

Sign up for GitHub

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

http: Adding flexible remove function to HeaderMap #3186

Merged
merged 4 commits into from
Apr 24, 2018
Merged

http: Adding flexible remove function to HeaderMap #3186

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
acad34a
http: Adding flexible remove function to HeaderMap
alyssawilk Apr 24, 2018
31e05c0
Fix typo
alyssawilk Apr 24, 2018
d9dd886
-> remove_if, fix for inline removal
alyssawilk Apr 24, 2018
219472d
regex -> prefix
alyssawilk Apr 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/envoy/http/header_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <algorithm>
#include <cstdint>
#include <memory>
#include <regex>
#include <string>

#include "envoy/common/pure.h"
Expand Down Expand Up @@ -465,6 +466,14 @@ class HeaderMap {
*/
virtual void remove(const LowerCaseString& key) PURE;

/**
* Remove all instances of headers with key matching the supplied regex.
* @param key_matcher supplies the regex to match header keys against. Note
* that this will be matching against LowerCaseString, so should be using
* lowercase characters.
*/
virtual void remove(const std::regex& key_matcher) PURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider doing a prefix version? I suspect the regex version is going to be slower, even if heavily tuned and basically a prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me. I'd be inclined to then swap regex for prefix as I have 3 confirmed users of prefixes and 0 that need generic regex :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got inline fixed - one more push and I'll do this swap as well


/**
* @return the number of headers in the map.
*/
Expand Down
11 changes: 11 additions & 0 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,17 @@ void HeaderMapImpl::remove(const LowerCaseString& key) {
}
}

void HeaderMapImpl::remove(const std::regex& regex) {
for (auto i = headers_.begin(); i != headers_.end();) {
absl::string_view key = i->key().getStringView();
if (std::regex_search(key.begin(), key.end(), regex)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider std::list::remove_if ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah - just converted over and got Matt's comment about inline headers. I think the additional complexity to remove the constant time references may make this version preferable. Will look though!

i = headers_.erase(i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not going to work correctly if this is a match of an inline header. You are probably going to need to do some extra work to make it work... Can you add a test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And my intuition of "I should wait an hour and see what Matt thinks" pans out once again :-D

} else {
++i;
}
}
}

HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl** entry,
const LowerCaseString& key) {
if (*entry) {
Expand Down
1 change: 1 addition & 0 deletions source/common/http/header_map_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class HeaderMapImpl : public HeaderMap {
void iterateReverse(ConstIterateCb cb, void* context) const override;
Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override;
void remove(const LowerCaseString& key) override;
void remove(const std::regex& regex) override;
size_t size() const override { return headers_.size(); }

protected:
Expand Down
38 changes: 38 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,44 @@ TEST(HeaderMapImplTest, Remove) {
EXPECT_EQ(0UL, headers.size());
}

TEST(HeaderMapImplTest, RemoveRegex) {
// These will match.
LowerCaseString key1 = LowerCaseString("X-prefix-foo");
LowerCaseString key3 = LowerCaseString("X-Prefix-");
LowerCaseString key5 = LowerCaseString("x-prefix-eep");
// These will not.
LowerCaseString key2 = LowerCaseString(" x-prefix-foo");
LowerCaseString key4 = LowerCaseString("y-x-prefix-foo");

HeaderMapImpl headers;
headers.addReference(key1, "value");
headers.addReference(key2, "value");
headers.addReference(key3, "value");
headers.addReference(key4, "value");
headers.addReference(key5, "value");

// Trying to remove upper case strings from LowerCaseStrings will not work.
headers.remove(std::regex("^X-Prefix-"));
EXPECT_NE(nullptr, headers.get(key1));
EXPECT_NE(nullptr, headers.get(key2));
EXPECT_NE(nullptr, headers.get(key3));
EXPECT_NE(nullptr, headers.get(key4));
EXPECT_NE(nullptr, headers.get(key5));

// Test removing the first header, middle headers, and the end header.
headers.remove(std::regex("^x-prefix-"));
EXPECT_EQ(nullptr, headers.get(key1));
EXPECT_NE(nullptr, headers.get(key2));
EXPECT_EQ(nullptr, headers.get(key3));
EXPECT_NE(nullptr, headers.get(key4));
EXPECT_EQ(nullptr, headers.get(key5));

// Remove all headers.
headers.remove(std::regex(".*"));
EXPECT_EQ(nullptr, headers.get(key2));
EXPECT_EQ(nullptr, headers.get(key4));
}

TEST(HeaderMapImplTest, SetRemovesAllValues) {
HeaderMapImpl headers;

Expand Down