More rtrim fix

Signed-off-by: Yuchen Dai <[email protected]>
envoyproxy · Jul 28, 2020 · b0132a1 · b0132a1
1 parent dd4c6bb
commit b0132a1
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 16 deletions.
diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc
@@ -151,22 +151,11 @@ void HeaderString::append(const char* data, uint32_t size) {
 }
 
 void HeaderString::rtrim() {
-  switch (type_) {
-  case Type::Reference: {
-    RELEASE_ASSERT("false", "rtrim does not support header string in reference type.");
-  }
-  case Type::Dynamic: {
-    // rtrim only manipulate length_. Share the same code with Inline.
-    FALLTHRU;
-  }
-  case Type::Inline: {
-    absl::string_view original = getStringView();
-    absl::string_view rtrimmed = StringUtil::rtrim(original);
-    if (original.size() != rtrimmed.size()) {
-      string_length_ = rtrimmed.size();
-    }
-    break;
-  }
+  ASSERT(type() == Type::Inline || type() == Type::Dynamic);
+  absl::string_view original = getStringView();
+  absl::string_view rtrimmed = StringUtil::rtrim(original);
+  if (original.size() != rtrimmed.size()) {
+    string_length_ = rtrimmed.size();
   }
 }
 

diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc
@@ -107,17 +107,35 @@ TEST(HeaderStringTest, All) {
 
   // Inline rtrim removes trailing whitespace only.
   {
+    // This header string is short enough to fit into Inline type.
     const std::string data_with_leading_lws = " \t\f\v  data";
     const std::string data_with_leading_and_trailing_lws = data_with_leading_lws + " \t\f\v";
     HeaderString string;
     string.append(data_with_leading_and_trailing_lws.data(),
                   data_with_leading_and_trailing_lws.size());
     EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView());
+    EXPECT_EQ(string.type(), HeaderString::Type::Inline);
     string.rtrim();
     EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView());
     EXPECT_EQ(data_with_leading_lws, string.getStringView());
   }
 
+  // Dynamic rtrim removes trailing whitespace only.
+  {
+    // Making this string longer than Inline can fit.
+    const std::string padding_data_with_leading_lws = " \t\f\v  data" + std::string(128, 'a');
+    const std::string data_with_leading_and_trailing_lws =
+        padding_data_with_leading_lws + " \t\f\v";
+    HeaderString string;
+    string.append(data_with_leading_and_trailing_lws.data(),
+                  data_with_leading_and_trailing_lws.size());
+    EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView());
+    EXPECT_EQ(string.type(), HeaderString::Type::Dynamic);
+    string.rtrim();
+    EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView());
+    EXPECT_EQ(padding_data_with_leading_lws, string.getStringView());
+  }
+
   // Static clear() does nothing.
   {
     std::string static_string("HELLO");