HttpHeader::parse_url fixes and unit test

yahoo · Apr 27, 2020 · c008425 · c008425
1 parent b3d4c5b
commit c008425
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 17 deletions.
diff --git a/local/src/core/ProxyVerifier.cc b/local/src/core/ProxyVerifier.cc
@@ -1635,29 +1635,27 @@ HttpHeader::parse_url(TextView url)
   swoc::Errata errata;
   // Split out the path and scheme for http/2 required headers
   // See rfc3986 section-3.2.
-  // TODO: Break this out into a unit tested static function.
-  std::size_t end_scheme = _url.find("://");
+  std::size_t end_scheme = url.find("://");
   if (end_scheme == std::string::npos) {
     _path = url;
     // Scheme, authority, and the like will have to come from the corresponding YAML nodes.
     return errata;
   }
   std::size_t auth_start = end_scheme + 3; // "://" is 3 characters.
   std::size_t end_host = auth_start;
-  _scheme = this->localize(_url.substr(0, end_scheme));
+  _scheme = this->localize(url.substr(0, end_scheme));
   // Look for the ':' for the port.
-  end_host = _url.find(":", auth_start);
+  std::size_t next_colon = url.find(":", auth_start);
+  std::size_t next_slash = url.find("/", auth_start);
+  end_host = std::min(next_colon, next_slash);
   if (end_host == std::string::npos) {
-    // No ':': look for the next "/".
-    end_host = _url.find("/", auth_start);
-    if (end_host == std::string::npos) {
-      end_host = _url.length();
-    }
+    // No ':' nor '/'. Assume the rest of the string is the host.
+    end_host = url.length();
   }
-  _authority = this->localize(_url.substr(auth_start, end_host - auth_start));
-  std::size_t path_start = _url.find("/", end_host);
+  _authority = this->localize(url.substr(auth_start, end_host - auth_start));
+  std::size_t path_start = url.find("/", end_host);
   if (path_start != std::string::npos) {
-    _path = this->localize(_url.substr(path_start));
+    _path = this->localize(url.substr(path_start));
   }
   return errata;
 }

diff --git a/test/unit_tests/test_ProxyVerifier.cc b/test/unit_tests/test_ProxyVerifier.cc
@@ -69,18 +69,23 @@ TEST_CASE("RuleChecks of non-duplicate fields", "[RuleCheck]") {
 
 TEST_CASE("RuleChecks of duplicate fields", "[RuleCheck]") {
   swoc::TextView test_name("testName");
+  std::list<swoc::TextView> expected_values_arg{
+    "first_value",
+    "second_value",
+  };
   std::list<swoc::TextView> expected_values{
     "first_value",
     "second_value",
   };
   swoc::TextView empty_name;
+  std::list<swoc::TextView> empty_values_arg;
   std::list<swoc::TextView> empty_values;
 
   RuleCheck::options_init();
 
   SECTION("presence checks") {
     std::shared_ptr<RuleCheck> present_check =
-        RuleCheck::make_rule_check(test_name, expected_values, "present");
+        RuleCheck::make_rule_check(test_name, std::move(expected_values_arg), "present");
     REQUIRE(present_check);
 
     CHECK_FALSE(present_check->test(key, empty_name, empty_values));
@@ -92,7 +97,7 @@ TEST_CASE("RuleChecks of duplicate fields", "[RuleCheck]") {
 
   SECTION("absence checks") {
     std::shared_ptr<RuleCheck> absent_check =
-        RuleCheck::make_rule_check(test_name, expected_values, "absent");
+        RuleCheck::make_rule_check(test_name, std::move(expected_values_arg), "absent");
     REQUIRE(absent_check);
 
     CHECK(absent_check->test(key, empty_name, empty_values));
@@ -103,7 +108,7 @@ TEST_CASE("RuleChecks of duplicate fields", "[RuleCheck]") {
 
   SECTION("equal checks") {
     std::shared_ptr<RuleCheck> equal_check_not_blank =
-        RuleCheck::make_rule_check(test_name, expected_values, "equal");
+        RuleCheck::make_rule_check(test_name, std::move(expected_values_arg), "equal");
     REQUIRE(equal_check_not_blank);
 
     CHECK_FALSE(equal_check_not_blank->test(key, empty_name, empty_values));
@@ -127,7 +132,7 @@ TEST_CASE("RuleChecks of duplicate fields", "[RuleCheck]") {
 
   SECTION("equal checks with no values in the rule") {
     std::shared_ptr<RuleCheck> equal_check_blank =
-        RuleCheck::make_rule_check(test_name, empty_values, "equal");
+        RuleCheck::make_rule_check(test_name, std::move(empty_values_arg), "equal");
     REQUIRE(equal_check_blank);
 
     swoc::TextView non_empty_values = {"some", "values"};
@@ -137,3 +142,28 @@ TEST_CASE("RuleChecks of duplicate fields", "[RuleCheck]") {
     CHECK_FALSE(equal_check_blank->test(key, test_name, non_empty_values));
   }
 }
+
+TEST_CASE("Test path parsing", "[ParseUrl]") {
+  HttpHeader header;
+  SECTION("Verify a simple path can be parsed") {
+    std::string url = "/a/path";
+    header.parse_url(url);
+    CHECK(header._scheme == "");
+    CHECK(header._path == "/a/path");
+    CHECK(header._authority == "");
+  }
+  SECTION("Verify URL parsing") {
+    std::string url = "https://example-ab.candy.com/xy?zab=123456789:98765432";
+    header.parse_url(url);
+    CHECK(header._scheme == "https");
+    CHECK(header._path == "/xy?zab=123456789:98765432");
+    CHECK(header._authority == "example-ab.candy.com");
+  }
+  SECTION("Verify URL parsing with a port") {
+    std::string url = "http://example-ab.candy.com:8080/xy?zab=123456789:98765432";
+    header.parse_url(url);
+    CHECK(header._scheme == "http");
+    CHECK(header._path == "/xy?zab=123456789:98765432");
+    CHECK(header._authority == "example-ab.candy.com");
+  }
+}
diff --git a/test/unit_tests/unit_tests.part b/test/unit_tests/unit_tests.part
@@ -3,7 +3,7 @@ Import("*")
 PartName("tests")
 
 DependsOn([
-    Component("swoc++",requires=REQ.DEFAULT(internal=False)),
+    Component("libswoc.static"),
     Component("nghttp2",requires=REQ.DEFAULT(internal=False)),
     "proxy-verifier.core"
     ])