url: don't update URL immediately on update to URLSearchParams

PR-URL: #51520 Fixes: #51518 Backport-PR-URL: #51559 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Mar 25, 2024 · fdc2619 · fdc2619
1 parent 2b923f7
commit fdc2619
Show file tree

Hide file tree

Showing 5 changed files with 176 additions and 19 deletions.
diff --git a/benchmark/url/url-searchparams-append.js 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
@@ -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
@@ -202,6 +202,7 @@ class URLContext {
   }
 }
 
+let setURLSearchParamsModified;
 let setURLSearchParamsContext;
 let getURLSearchParamsList;
 let setURLSearchParams;
@@ -469,8 +470,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);
     }
   }
 
@@ -503,8 +505,9 @@ class URLSearchParams {
         }
       }
     }
+
     if (this.#context) {
-      this.#context.search = this.toString();
+      setURLSearchParamsModified(this.#context);
     }
   }
 
@@ -609,7 +612,7 @@ class URLSearchParams {
     }
 
     if (this.#context) {
-      this.#context.search = this.toString();
+      setURLSearchParamsModified(this.#context);
     }
   }
 
@@ -658,7 +661,7 @@ class URLSearchParams {
     }
 
     if (this.#context) {
-      this.#context.search = this.toString();
+      setURLSearchParamsModified(this.#context);
     }
   }
 
@@ -763,6 +766,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) {
     if (arguments.length === 0) {
@@ -806,7 +823,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 {
@@ -832,27 +879,39 @@ 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;
   }
 
   set href(value) {
     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
@@ -994,26 +1053,25 @@ 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);
     }
   }
 
   // readonly
   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;
   }
@@ -1033,6 +1091,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
@@ -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
@@ -11,20 +11,33 @@ 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',
   'host',
   'hostname',
   'port',
   'pathname',
-  'search',
   'hash',
 ].forEach((i) => {
   assert.throws(() => Reflect.get(URL.prototype, i, {}), {