Merge pull request #651 from capnproto/harris/round-trip-eq-sign-in-url-query

Distinguish between empty- and null-valued query parameters

c++/src/kj/compat/url-test.c++

@@ -30,6 +30,9 @@ Url parseAndCheck(kj::StringPtr originalText, kj::StringPtr expectedRestringifie
   if (expectedRestringified == nullptr) expectedRestringified = originalText;
   auto url = Url::parse(originalText);
   KJ_EXPECT(kj::str(url) == expectedRestringified, url, originalText, expectedRestringified);
+  // Make sure clones also restringify to the expected string.
+  auto clone = url.clone();
+  KJ_EXPECT(kj::str(clone) == expectedRestringified, clone, originalText, expectedRestringified);
   return url;
 }
 
@@ -104,6 +107,36 @@ KJ_TEST("parse / stringify URL") {
     KJ_EXPECT(KJ_ASSERT_NONNULL(url.fragment) == "garply");
   }
 
+  {
+    auto url = parseAndCheck("https://capnproto.org/foo/bar?baz=qux&corge=#garply");
+    KJ_EXPECT(url.scheme == "https");
+    KJ_EXPECT(url.userInfo == nullptr);
+    KJ_EXPECT(url.host == "capnproto.org");
+    KJ_EXPECT(url.path.asPtr() == kj::ArrayPtr<const StringPtr>({"foo", "bar"}));
+    KJ_EXPECT(!url.hasTrailingSlash);
+    KJ_ASSERT(url.query.size() == 2);
+    KJ_EXPECT(url.query[0].name == "baz");
+    KJ_EXPECT(url.query[0].value == "qux");
+    KJ_EXPECT(url.query[1].name == "corge");
+    KJ_EXPECT(url.query[1].value == nullptr);
+    KJ_EXPECT(KJ_ASSERT_NONNULL(url.fragment) == "garply");
+  }
+
+  {
+    auto url = parseAndCheck("https://capnproto.org/foo/bar?baz=&corge=grault#garply");
+    KJ_EXPECT(url.scheme == "https");
+    KJ_EXPECT(url.userInfo == nullptr);
+    KJ_EXPECT(url.host == "capnproto.org");
+    KJ_EXPECT(url.path.asPtr() == kj::ArrayPtr<const StringPtr>({"foo", "bar"}));
+    KJ_EXPECT(!url.hasTrailingSlash);
+    KJ_ASSERT(url.query.size() == 2);
+    KJ_EXPECT(url.query[0].name == "baz");
+    KJ_EXPECT(url.query[0].value == "");
+    KJ_EXPECT(url.query[1].name == "corge");
+    KJ_EXPECT(url.query[1].value == "grault");
+    KJ_EXPECT(KJ_ASSERT_NONNULL(url.fragment) == "garply");
+  }
+
   {
     auto url = parseAndCheck("https://capnproto.org/foo/bar/?baz=qux&corge=grault#garply");
     KJ_EXPECT(url.scheme == "https");
@@ -353,15 +386,26 @@ KJ_TEST("parse URL failure") {
 void parseAndCheckRelative(kj::StringPtr base, kj::StringPtr relative, kj::StringPtr expected) {
   auto parsed = Url::parse(base).parseRelative(relative);
   KJ_EXPECT(kj::str(parsed) == expected, parsed, expected);
+  auto clone = parsed.clone();
+  KJ_EXPECT(kj::str(clone) == expected, clone, expected);
 }
 
 KJ_TEST("parse relative URL") {
   parseAndCheckRelative("https://capnproto.org/foo/bar?baz=qux#corge",
                         "#grault",
                         "https://capnproto.org/foo/bar?baz=qux#grault");
+  parseAndCheckRelative("https://capnproto.org/foo/bar?baz#corge",
+                        "#grault",
+                        "https://capnproto.org/foo/bar?baz#grault");
+  parseAndCheckRelative("https://capnproto.org/foo/bar?baz=#corge",
+                        "#grault",
+                        "https://capnproto.org/foo/bar?baz=#grault");
   parseAndCheckRelative("https://capnproto.org/foo/bar?baz=qux#corge",
                         "?grault",
                         "https://capnproto.org/foo/bar?grault");
+  parseAndCheckRelative("https://capnproto.org/foo/bar?baz=qux#corge",
+                        "?grault=",
+                        "https://capnproto.org/foo/bar?grault=");
   parseAndCheckRelative("https://capnproto.org/foo/bar?baz=qux#corge",
                         "?grault+garply=waldo",
                         "https://capnproto.org/foo/bar?grault+garply=waldo");

c++/src/kj/compat/url.c++

@@ -111,7 +111,9 @@ Url Url::clone() const {
     KJ_MAP(part, path) { return kj::str(part); },
     hasTrailingSlash,
     KJ_MAP(param, query) -> QueryParam {
-      return { kj::str(param.name), kj::str(param.value) };
+      // Preserve the "allocated-ness" of `param.value` with this careful copy.
+      return { kj::str(param.name), param.value.begin() == nullptr ? kj::String()
+                                                                   : kj::str(param.value) };
     },
     fragment.map([](const String& s) { return kj::str(s); })
   };
@@ -349,8 +351,10 @@ Maybe<Url> Url::tryParseRelative(StringPtr text) const {
     } while (text.startsWith("&"));
   } else if (!hadNewAuthority && !hadNewPath) {
     // copy query
-    result.query = KJ_MAP(param, this->query) {
-      return QueryParam { kj::str(param.name), kj::str(param.value) };
+    result.query = KJ_MAP(param, this->query) -> QueryParam {
+      // Preserve the "allocated-ness" of `param.value` with this careful copy.
+      return { kj::str(param.name), param.value.begin() == nullptr ? kj::String()
+                                                                   : kj::str(param.value) };
     };
   }
 
@@ -418,7 +422,7 @@ String Url::toString(Context context) const {
     chars.add(first ? '?' : '&');
     first = false;
     chars.addAll(encodeWwwForm(param.name));
-    if (param.value.size() > 0) {
+    if (param.value.begin() != nullptr) {
       chars.add('=');
       chars.addAll(encodeWwwForm(param.value));
     }

c++/src/kj/compat/url.h

@@ -63,7 +63,14 @@ struct Url {
   };
   Vector<QueryParam> query;
   // Query, e.g. from "?key=value&key2=value2". If a component of the query contains no '=' sign,
-  // it will be parsed as a key with an empty value.
+  // it will be parsed as a key with a null value, and later serialized with no '=' sign if you call
+  // Url::toString().
+  //
+  // To distinguish between null-valued and empty-valued query parameters, we test whether
+  // QueryParam::value is an allocated or unallocated string. For example:
+  //
+  //     QueryParam { kj::str("name"), nullptr }      // Null-valued; will not have an '=' sign.
+  //     QueryParam { kj::str("name"), kj::str("") }  // Empty-valued; WILL have an '=' sign.
 
   Maybe<String> fragment;
   // The stuff after the '#' character (not including the '#' character itself), if present.