Skip to content

Commit

Permalink
url: add value argument to has and delete methods
Browse files Browse the repository at this point in the history
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 <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
  • Loading branch information
sankalp1999 authored and ruyadorno committed Sep 6, 2023
1 parent 3be5335 commit c59ae86
Show file tree
Hide file tree
Showing 15 changed files with 709 additions and 609 deletions.
5 changes: 3 additions & 2 deletions benchmark/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
34 changes: 30 additions & 4 deletions doc/api/url.md
Original file line number Diff line number Diff line change
Expand Up @@ -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()`

Expand Down Expand Up @@ -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()`

Expand Down
41 changes: 30 additions & 11 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,8 @@ class URLSearchParams {
}
}

delete(name) {
if (!isURLSearchParams(this))
delete(name, value = undefined) {
if (typeof this !== 'object' || this === null || !isURLSearchParams(this))
throw new ERR_INVALID_THIS('URLSearchParams');

if (arguments.length < 1) {
Expand All @@ -354,12 +354,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]) {
Expand Down Expand Up @@ -404,8 +415,8 @@ class URLSearchParams {
return values;
}

has(name) {
if (!isURLSearchParams(this))
has(name, value = undefined) {
if (typeof this !== 'object' || this === null || !isURLSearchParams(this))
throw new ERR_INVALID_THIS('URLSearchParams');

if (arguments.length < 1) {
Expand All @@ -414,11 +425,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;
}

Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/wpt/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Last update:
- performance-timeline: https://github.com/web-platform-tests/wpt/tree/17ebc3aea0/performance-timeline
- resources: https://github.com/web-platform-tests/wpt/tree/fbf1e7d247/resources
- streams: https://github.com/web-platform-tests/wpt/tree/9e5ef42bd3/streams
- url: https://github.com/web-platform-tests/wpt/tree/84782d9315/url
- url: https://github.com/web-platform-tests/wpt/tree/c4726447f3/url
- user-timing: https://github.com/web-platform-tests/wpt/tree/5ae85bf826/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
Expand Down
50 changes: 24 additions & 26 deletions test/fixtures/wpt/url/README.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
11 changes: 6 additions & 5 deletions test/fixtures/wpt/url/failure.html
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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))
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/wpt/url/historical.any.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
31 changes: 18 additions & 13 deletions test/fixtures/wpt/url/resources/a-element-origin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 + ">")
}
}
33 changes: 19 additions & 14 deletions test/fixtures/wpt/url/resources/a-element.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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")
Expand All @@ -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 + ">")
}
}
Loading

0 comments on commit c59ae86

Please sign in to comment.