snowp review

* cleaner API for the max_vals argument of parseCookieValues
* get back to using std::string for the return value(s) of the parseCookie* functions

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

source/common/http/hash_policy.cc

@@ -88,7 +88,7 @@ class CookieHashMethod : public HashMethodImplBase {
                                     const HashPolicy::AddCookieCallback add_cookie,
                                     const StreamInfo::FilterStateSharedPtr) const override {
     absl::optional<uint64_t> hash;
-    absl::string_view value = Utility::parseCookieValue(headers, key_);
+    std::string value = Utility::parseCookieValue(headers, key_);
     if (value.empty() && ttl_.has_value()) {
       value = add_cookie(key_, path_, ttl_.value());
       hash = HashUtil::xxHash64(value);

source/common/http/utility.cc

@@ -288,14 +288,14 @@ forEachCookie(const HeaderMap& headers, const LowerCaseString& cookie_header,
   }
 }
 
-absl::string_view parseCookie(const HeaderMap& headers, const absl::string_view key,
-                              const LowerCaseString& cookie) {
-  absl::string_view value;
+static std::string parseCookie(const HeaderMap& headers, const absl::string_view key,
+                               const LowerCaseString& cookie) {
+  std::string value;
 
   // Iterate over each cookie & return if its value is not empty.
   forEachCookie(headers, cookie, [&key, &value](absl::string_view k, absl::string_view v) -> bool {
     if (key == k) {
-      value = v;
+      value = std::string{v};
       return false;
     }
 
@@ -329,17 +329,15 @@ Utility::parseCookies(const RequestHeaderMap& headers,
   return cookies;
 }
 
-absl::InlinedVector<absl::string_view, 2> Utility::parseCookieValues(const HeaderMap& headers,
-                                                                     const absl::string_view key,
-                                                                     const size_t max_vals) {
-  absl::InlinedVector<absl::string_view, 2> ret;
+absl::InlinedVector<std::string, 2> Utility::parseCookieValues(const HeaderMap& headers,
+                                                               const absl::string_view key,
+                                                               const size_t max_vals) {
+  absl::InlinedVector<std::string, 2> ret;
 
   forEachCookie(headers, Http::Headers::get().Cookie,
                 [&ret, &key, &max_vals](absl::string_view k, absl::string_view v) -> bool {
                   if (k == key) {
-                    ret.push_back(v);
-                    // max_vals == 0 => infinity, so the condition above will never be true
-                    // in this case.
+                    ret.emplace_back(v);
                     if (ret.size() == max_vals) {
                       return false;
                     }
@@ -506,13 +504,13 @@ std::string Utility::replaceQueryString(const HeaderString& path,
   return new_path;
 }
 
-absl::string_view Utility::parseCookieValue(const HeaderMap& headers, const absl::string_view key) {
+std::string Utility::parseCookieValue(const HeaderMap& headers, const absl::string_view key) {
   // TODO(wbpcode): Modify the headers parameter type to 'RequestHeaderMap'.
   return parseCookie(headers, key, Http::Headers::get().Cookie);
 }
 
-absl::string_view Utility::parseSetCookieValue(const Http::HeaderMap& headers,
-                                               const absl::string_view key) {
+std::string Utility::parseSetCookieValue(const Http::HeaderMap& headers,
+                                         const absl::string_view key) {
   return parseCookie(headers, key, Http::Headers::get().SetCookie);
 }
 

source/common/http/utility.h

@@ -278,7 +278,7 @@ std::string replaceQueryString(const HeaderString& path, const QueryParams& para
  * @return absl::string_view the parsed cookie value, or a default constructed absl::string_view if
  *         it doesn't exist.
  **/
-absl::string_view parseCookieValue(const HeaderMap& headers, const absl::string_view key);
+std::string parseCookieValue(const HeaderMap& headers, const absl::string_view key);
 
 /**
  * Parse cookies from header into a map.
@@ -301,9 +301,9 @@ absl::flat_hash_map<std::string, std::string> parseCookies(const RequestHeaderMa
  * Parse a particular value out of a set-cookie
  * @param headers supplies the headers to get the set-cookie from.
  * @param key the key for the particular set-cookie value to return
- * @return absl::string_view the parsed set-cookie value, or "" if none exists
+ * @return std::string the parsed set-cookie value, or "" if none exists
  **/
-absl::string_view parseSetCookieValue(const HeaderMap& headers, const absl::string_view key);
+std::string parseSetCookieValue(const HeaderMap& headers, const absl::string_view key);
 
 /**
  * Parse particular value(s) out of a cookie. The difference with
@@ -314,12 +314,13 @@ absl::string_view parseSetCookieValue(const HeaderMap& headers, const absl::stri
  *
  * @param headers supplies the headers to get the cookie from.
  * @param key the key for the particular cookie value to return.
- * @param max_vals the maximum number of values to return. 0 means all of them.
- * @return absl::InlinedVector<absl::string_view, 2> a vector of
- *         absl::string_view objects containing the extracted values.
+ * @param max_vals the maximum number of values to return.
+ * @return absl::InlinedVector<std::string, 2> a vector of
+ *         std::string objects containing the extracted values.
  **/
-absl::InlinedVector<absl::string_view, 2>
-parseCookieValues(const HeaderMap& headers, const absl::string_view key, const size_t max_vals = 0);
+absl::InlinedVector<std::string, 2>
+parseCookieValues(const HeaderMap& headers, const absl::string_view key,
+                  const size_t max_vals = std::numeric_limits<size_t>::max());
 
 /**
  * Produce the value for a Set-Cookie header with the given parameters.

source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc

@@ -33,9 +33,9 @@ absl::optional<std::string> HeaderValueSelector::extract(Http::HeaderMap& map) c
 
 // Extract the value of the key from the cookie header.
 absl::optional<std::string> CookieValueSelector::extract(Http::HeaderMap& map) const {
-  std::string value(Envoy::Http::Utility::parseCookieValue(map, cookie_));
+  std::string value = Envoy::Http::Utility::parseCookieValue(map, cookie_);
   if (!value.empty()) {
-    return absl::optional<std::string>(value);
+    return absl::optional<std::string>(std::move(value));
   }
   return absl::nullopt;
 }

source/extensions/http/stateful_session/cookie/cookie.h

@@ -53,7 +53,7 @@ class CookieBasedSessionStateFactory : public Envoy::Http::SessionStateFactory {
 
 private:
   absl::optional<std::string> parseAddress(const Envoy::Http::RequestHeaderMap& headers) const {
-    const absl::string_view cookie_value = Envoy::Http::Utility::parseCookieValue(headers, name_);
+    const std::string cookie_value = Envoy::Http::Utility::parseCookieValue(headers, name_);
     std::string address = Envoy::Base64::decode(cookie_value);
 
     return !address.empty() ? absl::make_optional(std::move(address)) : absl::nullopt;

test/common/http/matching/inputs_test.cc

@@ -71,7 +71,6 @@ TEST(HttpRequestCookiesDataInput, Idempotence) {
   data.onRequestHeaders(request_headers);
 
   EXPECT_EQ(input.get(data).data_, "foo,bar");
-  EXPECT_EQ(input.get(data).data_, "foo,bar");
 }
 
 TEST(MatchingData, HttpResponseHeadersDataInput) {

test/common/http/utility_test.cc

@@ -694,7 +694,7 @@ TEST(HttpUtility, TestParseCookie) {
       {"cookie", "key2=value2; key3=value3"}};
 
   absl::string_view key{"token"};
-  absl::string_view value = Utility::parseCookieValue(headers, key);
+  std::string value = Utility::parseCookieValue(headers, key);
   EXPECT_EQ(value, "abc123");
 }
 
@@ -716,7 +716,7 @@ TEST(HttpUtility, TestParseSetCookie) {
       {"set-cookie", "key2=value2; key3=value3"}};
 
   std::string key{"token"};
-  absl::string_view value = Utility::parseSetCookieValue(headers, key);
+  std::string value = Utility::parseSetCookieValue(headers, key);
   EXPECT_EQ(value, "abc123");
 }