url: add value argument to has and delete methods

The change aims to add value argument to two methods of URLSearchParams class i.e the has method and the delete method. For has method, if value argument is provided, then use it to check for presence. For delete method, if value argument provided, use it to delete. Fixes: #47883 PR-URL: #47885 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
nodejs · May 14, 2023 · 9f3aacb · 9f3aacb
1 parent abb1c45
commit 9f3aacb
Show file tree

Hide file tree

Showing 15 changed files with 707 additions and 607 deletions.
diff --git a/benchmark/common.js b/benchmark/common.js
@@ -360,8 +360,9 @@ function getUrlData(withBase) {
   for (const item of data) {
     if (item.failure || !item.input) continue;
     if (withBase) {
-      result.push([item.input, item.base]);
-    } else if (item.base !== 'about:blank') {
+      // item.base might be null. It should be converted into `undefined`.
+      result.push([item.input, item.base ?? undefined]);
+    } else if (item.base !== null) {
       result.push(item.base);
     }
   }

diff --git a/doc/api/url.md b/doc/api/url.md
@@ -859,11 +859,22 @@ new URLSearchParams([
 
 Append a new name-value pair to the query string.
 
-#### `urlSearchParams.delete(name)`
+#### `urlSearchParams.delete(name[, value])`
+
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/47885
+    description: Add support for optional `value` argument.
+-->
 
 * `name` {string}
+* `value` {string}
+
+If `value` is provided, removes all name-value pairs
+where name is `name` and value is `value`..
 
-Remove all name-value pairs whose name is `name`.
+If `value` is not provided, removes all name-value pairs whose name is `name`.
 
 #### `urlSearchParams.entries()`
 
@@ -918,12 +929,27 @@ are no such pairs, `null` is returned.
 Returns the values of all name-value pairs whose name is `name`. If there are
 no such pairs, an empty array is returned.
 
-#### `urlSearchParams.has(name)`
+#### `urlSearchParams.has(name[, value])`
+
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/47885
+    description: Add support for optional `value` argument.
+-->
 
 * `name` {string}
+* `value` {string}
 * Returns: {boolean}
 
-Returns `true` if there is at least one name-value pair whose name is `name`.
+Checks if the `URLSearchParams` object contains key-value pair(s) based on
+`name` and an optional `value` argument.
+
+If `value` is provided, returns `true` when name-value pair with
+same `name` and `value` exists.
+
+If `value` is not provided, returns `true` if there is at least one name-value
+pair whose name is `name`.
 
 #### `urlSearchParams.keys()`
 

diff --git a/lib/internal/url.js b/lib/internal/url.js
@@ -435,7 +435,7 @@ class URLSearchParams {
     }
   }
 
-  delete(name) {
+  delete(name, value = undefined) {
     if (typeof this !== 'object' || this === null || !(#searchParams in this))
       throw new ERR_INVALID_THIS('URLSearchParams');
 
@@ -445,12 +445,23 @@ class URLSearchParams {
 
     const list = this.#searchParams;
     name = toUSVString(name);
-    for (let i = 0; i < list.length;) {
-      const cur = list[i];
-      if (cur === name) {
-        list.splice(i, 2);
-      } else {
-        i += 2;
+
+    if (value !== undefined) {
+      value = toUSVString(value);
+      for (let i = 0; i < list.length;) {
+        if (list[i] === name && list[i + 1] === value) {
+          list.splice(i, 2);
+        } else {
+          i += 2;
+        }
+      }
+    } else {
+      for (let i = 0; i < list.length;) {
+        if (list[i] === name) {
+          list.splice(i, 2);
+        } else {
+          i += 2;
+        }
       }
     }
     if (this.#context) {
@@ -495,7 +506,7 @@ class URLSearchParams {
     return values;
   }
 
-  has(name) {
+  has(name, value = undefined) {
     if (typeof this !== 'object' || this === null || !(#searchParams in this))
       throw new ERR_INVALID_THIS('URLSearchParams');
 
@@ -505,11 +516,19 @@ class URLSearchParams {
 
     const list = this.#searchParams;
     name = toUSVString(name);
+
+    if (value !== undefined) {
+      value = toUSVString(value);
+    }
+
     for (let i = 0; i < list.length; i += 2) {
       if (list[i] === name) {
-        return true;
+        if (value === undefined || list[i + 1] === value) {
+          return true;
+        }
       }
     }
+
     return false;
   }
 

diff --git a/test/fixtures/wpt/README.md b/test/fixtures/wpt/README.md
@@ -27,7 +27,7 @@ Last update:
 - resource-timing: https://github.com/web-platform-tests/wpt/tree/22d38586d0/resource-timing
 - resources: https://github.com/web-platform-tests/wpt/tree/919874f84f/resources
 - streams: https://github.com/web-platform-tests/wpt/tree/51750bc8d7/streams
-- url: https://github.com/web-platform-tests/wpt/tree/7c5c3cc125/url
+- url: https://github.com/web-platform-tests/wpt/tree/c4726447f3/url
 - user-timing: https://github.com/web-platform-tests/wpt/tree/df24fb604e/user-timing
 - wasm/jsapi: https://github.com/web-platform-tests/wpt/tree/cde25e7e3c/wasm/jsapi
 - wasm/webapi: https://github.com/web-platform-tests/wpt/tree/fd1b23eeaa/wasm/webapi

diff --git a/test/fixtures/wpt/url/README.md b/test/fixtures/wpt/url/README.md
@@ -1,31 +1,29 @@
 ## urltestdata.json
 
-These tests are for browsers, but the data for
-`a-element.html`, `url-constructor.html`, `a-element-xhtml.xhtml`, and `failure.html`
-is in `resources/urltestdata.json` and can be re-used by non-browser implementations.
-This file contains a JSON array of comments as strings and test cases as objects.
-The keys for each test case are:
-
-* `base`: an absolute URL as a string whose [parsing] without a base of its own must succeed.
-  This key is always present,
-  and may have a value like `"about:blank"` when `input` is an absolute URL.
-* `input`: an URL as a string to be [parsed][parsing] with `base` as its base URL.
-* Either:
-  * `failure` with the value `true`, indicating that parsing `input` should return failure,
-  * or `href`, `origin`, `protocol`, `username`, `password`, `host`, `hostname`, `port`,
-    `pathname`, `search`, and `hash` with string values;
-    indicating that parsing `input` should return an URL record
-    and that the getters of each corresponding attribute in that URL’s [API]
-    should return the corresponding value.
-
-    The `origin` key may be missing.
-    In that case, the API’s `origin` attribute is not tested.
-
-In addition to testing that parsing `input` against `base` gives the result, a test harness for the
-`URL` constructor (or similar APIs) should additionally test the following pattern: if `failure` is
-true, parsing `about:blank` against `input` must give failure. This tests that the logic for
-converting base URLs into strings properly fails the whole parsing algorithm if the base URL cannot
-be parsed.
+`resources/urltestdata.json` contains URL parsing tests suitable for any URL parser implementation.
+
+It's used as a source of tests by `a-element.html`, `failure.html`, `url-constructor.any.js`, and
+other test files in this directory.
+
+The format of `resources/urltestdata.json` is a JSON array of comments as strings and test cases as
+objects. The keys for each test case are:
+
+* `input`: a string to be parsed as URL.
+* `base`: null or a serialized URL (i.e., does not fail parsing).
+* Then either
+
+  * `failure` whose value is `true`, indicating that parsing `input` relative to `base` returns
+    failure
+  * `relativeTo` whose value is "`non-opaque-path-base`" (input does parse against a non-null base
+    URL without an opaque path) or "`any-base`" (input parses against any non-null base URL), or is
+    omitted in its entirety (input never parses successfully)
+
+  or `href`, `origin`, `protocol`, `username`, `password`, `host`, `hostname`, `port`,
+  `pathname`, `search`, and `hash` with string values; indicating that parsing `input` should return
+  an URL record and that the getters of each corresponding attribute in that URL’s [API] should
+  return the corresponding value.
+
+  The `origin` key may be missing. In that case, the API’s `origin` attribute is not tested.
 
 ## setters_tests.json
 

diff --git a/test/fixtures/wpt/url/failure.html b/test/fixtures/wpt/url/failure.html
@@ -9,14 +9,15 @@
 promise_test(() => fetch("resources/urltestdata.json").then(res => res.json()).then(runTests), "Loading data…")
 
 function runTests(testData) {
-  for(const test of testData) {
-    if (typeof test === "string" || !test.failure || test.base !== "about:blank") {
-      continue
+  for (const test of testData) {
+    if (typeof test === "string" || !test.failure || test.base !== null) {
+      continue;
     }
 
     const name = test.input + " should throw"
 
-    self.test(() => { // URL's constructor's first argument is tested by url-constructor.html
+    self.test(() => {
+      // URL's constructor's first argument is tested by url-constructor.html
       // If a URL fails to parse with any valid base, it must also fail to parse with no base, i.e.
       // when used as a base URL itself.
       assert_throws_js(TypeError, () => new URL("about:blank", test.input));
@@ -30,7 +31,7 @@
     // The following use cases resolve the URL input relative to the current
     // document's URL. If this test input could be construed as a valid URL
     // when resolved against a base URL, skip these cases.
-    if (!test.inputCanBeRelative) {
+    if (test.relativeTo === undefined) {
       self.test(() => {
         const client = new XMLHttpRequest()
         assert_throws_dom("SyntaxError", () => client.open("GET", test.input))

diff --git a/test/fixtures/wpt/url/historical.any.js b/test/fixtures/wpt/url/historical.any.js
@@ -36,4 +36,11 @@ test(() => {
   assert_throws_dom("DataCloneError", () => self.structuredClone(new URLSearchParams()));
 }, "URLSearchParams: no structured serialize/deserialize support");
 
+test(() => {
+  const url = new URL("about:blank");
+  url.toString = () => { throw 1 };
+  assert_throws_exactly(1, () => new URL(url), "url argument");
+  assert_throws_exactly(1, () => new URL("about:blank", url), "base argument");
+}, "Constructor only takes strings");
+
 done();
diff --git a/test/fixtures/wpt/url/resources/a-element-origin.js b/test/fixtures/wpt/url/resources/a-element-origin.js
@@ -5,23 +5,28 @@ function setBase(base) {
 }
 
 function bURL(url, base) {
-  base = base || "about:blank"
-  setBase(base)
-  var a = document.createElement("a")
-  a.setAttribute("href", url)
-  return a
+  setBase(base);
+  const a = document.createElement("a");
+  a.setAttribute("href", url);
+  return a;
 }
 
-function runURLTests(urltests) {
-  for(var i = 0, l = urltests.length; i < l; i++) {
-    var expected = urltests[i]
-    if (typeof expected === "string" || !("origin" in expected)) continue
-    // skip without base because you cannot unset the baseURL of a document
-    if (expected.base === null) continue;
+function runURLTests(urlTests) {
+  for (const expected of urlTests) {
+    // Skip comments and tests without "origin" expectation
+    if (typeof expected === "string" || !("origin" in expected))
+      continue;
+
+    // Fragments are relative against "about:blank" (this might always be redundant due to requiring "origin" in expected)
+    if (expected.base === null && expected.input.startsWith("#"))
+      continue;
+
+    // We cannot use a null base for HTML tests
+    const base = expected.base === null ? "about:blank" : expected.base;
 
     test(function() {
-      var url = bURL(expected.input, expected.base)
+      var url = bURL(expected.input, base)
       assert_equals(url.origin, expected.origin, "origin")
-    }, "Parsing origin: <" + expected.input + "> against <" + expected.base + ">")
+    }, "Parsing origin: <" + expected.input + "> against <" + base + ">")
   }
 }
diff --git a/test/fixtures/wpt/url/resources/a-element.js b/test/fixtures/wpt/url/resources/a-element.js
@@ -1,23 +1,28 @@
 promise_test(() => fetch("resources/urltestdata.json").then(res => res.json()).then(runURLTests), "Loading data…");
 
 function setBase(base) {
-  document.getElementById("base").href = base
+  document.getElementById("base").href = base;
 }
 
 function bURL(url, base) {
-  base = base || "about:blank"
-  setBase(base)
-  var a = document.createElement("a")
-  a.setAttribute("href", url)
-  return a
+  setBase(base);
+  const a = document.createElement("a");
+  a.setAttribute("href", url);
+  return a;
 }
 
-function runURLTests(urltests) {
-  for(var i = 0, l = urltests.length; i < l; i++) {
-    var expected = urltests[i]
-    if (typeof expected === "string") continue // skip comments
-    // skip without base because you cannot unset the baseURL of a document
-    if (expected.base === null) continue;
+function runURLTests(urlTests) {
+  for (const expected of urlTests) {
+    // Skip comments
+    if (typeof expected === "string")
+      continue;
+
+    // Fragments are relative against "about:blank"
+    if (expected.relativeTo === "any-base")
+      continue;
+
+    // We cannot use a null base for HTML tests
+    const base = expected.base === null ? "about:blank" : expected.base;
 
     function getKey(expected) {
       if (expected.protocol) {
@@ -30,7 +35,7 @@ function runURLTests(urltests) {
     }
 
     subsetTestByKey(getKey(expected), test, function() {
-      var url = bURL(expected.input, expected.base)
+      var url = bURL(expected.input, base)
       if(expected.failure) {
         if(url.protocol !== ':') {
           assert_unreached("Expected URL to fail parsing")
@@ -49,6 +54,6 @@ function runURLTests(urltests) {
       assert_equals(url.pathname, expected.pathname, "pathname")
       assert_equals(url.search, expected.search, "search")
       assert_equals(url.hash, expected.hash, "hash")
-    }, "Parsing: <" + expected.input + "> against <" + expected.base + ">")
+    }, "Parsing: <" + expected.input + "> against <" + base + ">")
   }
 }