From 407341e25c3d51476d9b38a17a67a64b2e8c4d46 Mon Sep 17 00:00:00 2001 From: Matt Cowley Date: Thu, 15 Feb 2024 14:41:29 +0000 Subject: [PATCH] url: don't update URL immediately on update to URLSearchParams PR-URL: https://github.com/nodejs/node/pull/51520 Fixes: https://github.com/nodejs/node/issues/51518 Backport-PR-URL: https://github.com/nodejs/node/pull/51559 Reviewed-By: Antoine du Hamel Reviewed-By: Yagiz Nizipli Reviewed-By: Matteo Collina --- benchmark/url/url-searchparams-append.js | 19 ++++ benchmark/url/url-searchparams-update.js | 29 ++++++ lib/internal/url.js | 94 +++++++++++++++---- .../test-whatwg-url-custom-searchparams.js | 36 +++++++ test/parallel/test-whatwg-url-invalidthis.js | 17 +++- 5 files changed, 176 insertions(+), 19 deletions(-) create mode 100644 benchmark/url/url-searchparams-append.js create mode 100644 benchmark/url/url-searchparams-update.js diff --git a/benchmark/url/url-searchparams-append.js b/benchmark/url/url-searchparams-append.js new file mode 100644 index 00000000000000..cd8099b517c6f7 --- /dev/null +++ b/benchmark/url/url-searchparams-append.js @@ -0,0 +1,19 @@ +'use strict'; +const common = require('../common.js'); + +const bench = common.createBenchmark(main, { + type: ['URL', 'URLSearchParams'], + n: [1e3, 1e6], +}); + +function main({ type, n }) { + const params = type === 'URL' ? + new URL('https://nodejs.org').searchParams : + new URLSearchParams(); + + bench.start(); + for (let i = 0; i < n; i++) { + params.append('test', i); + } + bench.end(n); +} diff --git a/benchmark/url/url-searchparams-update.js b/benchmark/url/url-searchparams-update.js new file mode 100644 index 00000000000000..082d476a5d2250 --- /dev/null +++ b/benchmark/url/url-searchparams-update.js @@ -0,0 +1,29 @@ +'use strict'; +const common = require('../common.js'); +const assert = require('assert'); + +const bench = common.createBenchmark(main, { + searchParams: ['true', 'false'], + property: ['pathname', 'search', 'hash'], + n: [1e6], +}); + +function getMethod(url, property) { + if (property === 'pathname') return (x) => url.pathname = `/${x}`; + if (property === 'search') return (x) => url.search = `?${x}`; + if (property === 'hash') return (x) => url.hash = `#${x}`; + throw new Error(`Unsupported property "${property}"`); +} + +function main({ searchParams, property, n }) { + const url = new URL('https://nodejs.org'); + if (searchParams === 'true') assert(url.searchParams); + + const method = getMethod(url, property); + + bench.start(); + for (let i = 0; i < n; i++) { + method(i); + } + bench.end(n); +} diff --git a/lib/internal/url.js b/lib/internal/url.js index 38f97926064595..0e69ff52b5edef 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -206,6 +206,7 @@ class URLContext { } } +let setURLSearchParamsModified; let setURLSearchParamsContext; let getURLSearchParamsList; let setURLSearchParams; @@ -475,8 +476,9 @@ class URLSearchParams { name = StringPrototypeToWellFormed(`${name}`); value = StringPrototypeToWellFormed(`${value}`); ArrayPrototypePush(this.#searchParams, name, value); + if (this.#context) { - this.#context.search = this.toString(); + setURLSearchParamsModified(this.#context); } } @@ -509,8 +511,9 @@ class URLSearchParams { } } } + if (this.#context) { - this.#context.search = this.toString(); + setURLSearchParamsModified(this.#context); } } @@ -615,7 +618,7 @@ class URLSearchParams { } if (this.#context) { - this.#context.search = this.toString(); + setURLSearchParamsModified(this.#context); } } @@ -664,7 +667,7 @@ class URLSearchParams { } if (this.#context) { - this.#context.search = this.toString(); + setURLSearchParamsModified(this.#context); } } @@ -769,6 +772,20 @@ function isURL(self) { class URL { #context = new URLContext(); #searchParams; + #searchParamsModified; + + static { + setURLSearchParamsModified = (obj) => { + // When URLSearchParams changes, we lazily update URL on the next read/write for performance. + obj.#searchParamsModified = true; + + // If URL has an existing search, remove it without cascading back to URLSearchParams. + // Do this to avoid any internal confusion about whether URLSearchParams or URL is up-to-date. + if (obj.#context.hasSearch) { + obj.#updateContext(bindingUrl.update(obj.#context.href, updateActions.kSearch, '')); + } + }; + } constructor(input, base = undefined) { markTransferMode(this, false, false); @@ -814,7 +831,37 @@ class URL { return `${constructor.name} ${inspect(obj, opts)}`; } - #updateContext(href) { + #getSearchFromContext() { + if (!this.#context.hasSearch) return ''; + let endsAt = this.#context.href.length; + if (this.#context.hasHash) endsAt = this.#context.hash_start; + if (endsAt - this.#context.search_start <= 1) return ''; + return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt); + } + + #getSearchFromParams() { + if (!this.#searchParams?.size) return ''; + return `?${this.#searchParams}`; + } + + #ensureSearchParamsUpdated() { + // URL is updated lazily to greatly improve performance when URLSearchParams is updated repeatedly. + // If URLSearchParams has been modified, reflect that back into URL, without cascading back. + if (this.#searchParamsModified) { + this.#searchParamsModified = false; + this.#updateContext(bindingUrl.update(this.#context.href, updateActions.kSearch, this.#getSearchFromParams())); + } + } + + /** + * Update the internal context state for URL. + * @param {string} href New href string from `bindingUrl.update`. + * @param {boolean} [shouldUpdateSearchParams] If the update has potential to update search params (href/search). + */ + #updateContext(href, shouldUpdateSearchParams = false) { + const previousSearch = shouldUpdateSearchParams && this.#searchParams && + (this.#searchParamsModified ? this.#getSearchFromParams() : this.#getSearchFromContext()); + this.#context.href = href; const { @@ -840,19 +887,31 @@ class URL { this.#context.scheme_type = scheme_type; if (this.#searchParams) { - if (this.#context.hasSearch) { - setURLSearchParams(this.#searchParams, this.search); - } else { - setURLSearchParams(this.#searchParams, undefined); + // If the search string has updated, URL becomes the source of truth, and we update URLSearchParams. + // Only do this when we're expecting it to have changed, otherwise a change to hash etc. + // would incorrectly compare the URLSearchParams state to the empty URL search state. + if (shouldUpdateSearchParams) { + const currentSearch = this.#getSearchFromContext(); + if (previousSearch !== currentSearch) { + setURLSearchParams(this.#searchParams, currentSearch); + this.#searchParamsModified = false; + } } + + // If we have a URLSearchParams, ensure that URL is up-to-date with any modification to it. + this.#ensureSearchParamsUpdated(); } } toString() { + // Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync. + this.#ensureSearchParamsUpdated(); return this.#context.href; } get href() { + // Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync. + this.#ensureSearchParamsUpdated(); return this.#context.href; } @@ -860,7 +919,7 @@ class URL { value = `${value}`; const href = bindingUrl.update(this.#context.href, updateActions.kHref, value); if (!href) { throw new ERR_INVALID_URL(value); } - this.#updateContext(href); + this.#updateContext(href, true); } // readonly @@ -1002,17 +1061,15 @@ class URL { } get search() { - if (!this.#context.hasSearch) { return ''; } - let endsAt = this.#context.href.length; - if (this.#context.hasHash) { endsAt = this.#context.hash_start; } - if (endsAt - this.#context.search_start <= 1) { return ''; } - return StringPrototypeSlice(this.#context.href, this.#context.search_start, endsAt); + // Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync. + this.#ensureSearchParamsUpdated(); + return this.#getSearchFromContext(); } set search(value) { const href = bindingUrl.update(this.#context.href, updateActions.kSearch, StringPrototypeToWellFormed(`${value}`)); if (href) { - this.#updateContext(href); + this.#updateContext(href, true); } } @@ -1020,8 +1077,9 @@ class URL { get searchParams() { // Create URLSearchParams on demand to greatly improve the URL performance. if (this.#searchParams == null) { - this.#searchParams = new URLSearchParams(this.search); + this.#searchParams = new URLSearchParams(this.#getSearchFromContext()); setURLSearchParamsContext(this.#searchParams, this); + this.#searchParamsModified = false; } return this.#searchParams; } @@ -1041,6 +1099,8 @@ class URL { } toJSON() { + // Updates to URLSearchParams are lazily propagated to URL, so we need to check we're in sync. + this.#ensureSearchParamsUpdated(); return this.#context.href; } diff --git a/test/parallel/test-whatwg-url-custom-searchparams.js b/test/parallel/test-whatwg-url-custom-searchparams.js index 75fa1779bdeb45..faec86e017a2ec 100644 --- a/test/parallel/test-whatwg-url-custom-searchparams.js +++ b/test/parallel/test-whatwg-url-custom-searchparams.js @@ -43,6 +43,42 @@ assert.strictEqual(sp.toString(), serialized); assert.strictEqual(m.search, `?${serialized}`); +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +assert.strictEqual(m.href, `http://example.org/?${serialized}`); + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +assert.strictEqual(m.toString(), `http://example.org/?${serialized}`); + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +assert.strictEqual(m.toJSON(), `http://example.org/?${serialized}`); + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +m.href = 'http://example.org'; +assert.strictEqual(m.href, 'http://example.org/'); +assert.strictEqual(sp.size, 0); + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +m.search = ''; +assert.strictEqual(m.href, 'http://example.org/'); +assert.strictEqual(sp.size, 0); + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +m.pathname = '/test'; +assert.strictEqual(m.href, `http://example.org/test?${serialized}`); +m.pathname = ''; + +sp.delete('a'); +values.forEach((i) => sp.append('a', i)); +m.hash = '#test'; +assert.strictEqual(m.href, `http://example.org/?${serialized}#test`); +m.hash = ''; + assert.strictEqual(sp[Symbol.iterator], sp.entries); let key, val; diff --git a/test/parallel/test-whatwg-url-invalidthis.js b/test/parallel/test-whatwg-url-invalidthis.js index b46b5d8cceb8fa..f4d9f91ada0095 100644 --- a/test/parallel/test-whatwg-url-invalidthis.js +++ b/test/parallel/test-whatwg-url-invalidthis.js @@ -11,12 +11,26 @@ const assert = require('assert'); ].forEach((i) => { assert.throws(() => Reflect.apply(URL.prototype[i], [], {}), { name: 'TypeError', - message: /Cannot read private member/, + message: /Receiver must be an instance of class/, }); }); [ 'href', + 'search', +].forEach((i) => { + assert.throws(() => Reflect.get(URL.prototype, i, {}), { + name: 'TypeError', + message: /Receiver must be an instance of class/, + }); + + assert.throws(() => Reflect.set(URL.prototype, i, null, {}), { + name: 'TypeError', + message: /Cannot read private member/, + }); +}); + +[ 'protocol', 'username', 'password', @@ -24,7 +38,6 @@ const assert = require('assert'); 'hostname', 'port', 'pathname', - 'search', 'hash', ].forEach((i) => { assert.throws(() => Reflect.get(URL.prototype, i, {}), {