Fix http::cookie_value() implementation

This was not handling multiple cookies properly for a reason only known
to my past self.

Also add more unit tests while at it.

Fixes #126

lib/malloy/core/http/utils.hpp

@@ -73,26 +73,34 @@ namespace malloy::http
      * @param cookie_name The cookie name.
      * @return The cookie value (if any).
      */
+    // ToDo: This implementation could use some love. It's comparably inefficient as we're splitting first on each
+    //       cookie value separation ("; ") and then on each value-pair separator ("="). A more elegant implementation
+    //       would just continuously advance through the string.
     template<bool isReq, typename Fields>
     [[nodiscard]]
     std::optional<std::string_view>
     cookie_value(const boost::beast::http::header<isReq, Fields>& header, const std::string_view cookie_name)
     {
-        const auto& [begin, end] = header.equal_range(field::cookie);
-        for (auto it = begin; it != end; it++) {
-            const auto& str = it->value();
+        using namespace std::string_view_literals;
 
-            const auto& sep_pos = it->value().find('=');
-            if (sep_pos == std::string::npos)
-                continue;
+        // Get cookie field
+        const auto& it = header.find(field::cookie);
+        if (it == header.cend())
+            return std::nullopt;
 
-            const std::string_view key{ str.substr(0, sep_pos) };
-            const std::string_view value{ str.substr(sep_pos+1) };
+        // Split pairs
+        const auto& pairs = split_header_value(it->value());
 
-            if (key != cookie_name)
+        // Check each pair
+        for (const std::string_view& pair : pairs) {
+            // Split
+            const auto& parts = malloy::split(pair, "="sv);
+            if (parts.size() != 2)
                 continue;
 
-            return value;
+            // Check cookie name
+            if (parts[0] == cookie_name)
+                return parts[1];
         }
 
         return std::nullopt;

test/test_suites/components/utils_core_http.cpp

@@ -39,3 +39,70 @@ TEST_SUITE("components - utils - core - http")
         }
     }
 }
+
+TEST_SUITE("components - core - utils - http - cookies")
+{
+
+    TEST_CASE("cookie_value()")
+    {
+        using namespace std::string_view_literals;
+
+        // ToDo: Don't use req.insert() - Use malloy provided infrastructure to set cookies instead.
+
+        malloy::http::request req;
+
+        SUBCASE("none")
+        {
+            CHECK_EQ(cookie_value(req, "foo"), std::nullopt);
+        }
+
+        SUBCASE("one")
+        {
+            std::string_view cookie_str = "foo=bar"sv;
+
+            SUBCASE("exists")
+            {
+                req.insert(malloy::http::field::cookie, cookie_str);
+                auto o = cookie_value(req, "foo");
+                REQUIRE(o.has_value());
+                CHECK_EQ(*o, "bar");
+            }
+
+            SUBCASE("non-exist")
+            {
+                auto o = cookie_value(req, "nope");
+                CHECK_FALSE(o.has_value());
+
+                req.insert(malloy::http::field::cookie, cookie_str);
+
+                o = cookie_value(req, "nope");
+                CHECK_FALSE(o.has_value());
+            }
+        }
+
+        SUBCASE("multiple")
+        {
+            std::string_view cookie_str = "foo=bar; one=two; something=else";
+
+            SUBCASE("exists")
+            {
+                req.insert(malloy::http::field::cookie, cookie_str);
+                auto o = cookie_value(req, "foo");
+                REQUIRE(o.has_value());
+                CHECK_EQ(*o, "bar");
+            }
+
+            SUBCASE("non-exist")
+            {
+                auto o = cookie_value(req, "nope");
+                CHECK_FALSE(o.has_value());
+
+                req.insert(malloy::http::field::cookie, cookie_str);
+
+                o = cookie_value(req, "nope");
+                CHECK_FALSE(o.has_value());
+            }
+        }
+    }
+
+}