fix: URLSearchParams construction and iteration spec compliance#1970
Merged
NathanWalker merged 5 commits intoJun 12, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughURLSearchParams constructor dispatch, live iterator implementation/wiring, and query method semantics were changed to match the spec; tests were expanded for iterator protocol, initialization shapes, coercion, and value-filtered has/delete behavior. ChangesURLSearchParams Spec Compliance
Sequence DiagramsequenceDiagram
participant Client
participant URLSearchParams_prototype
participant EntriesTemplate
participant LiveIterator
participant InternalStorage
Client->>URLSearchParams_prototype: call entries()/keys()/values()/@@iterator
URLSearchParams_prototype->>EntriesTemplate: create/return live iterator (ReturnLiveIterator)
EntriesTemplate->>LiveIterator: instantiate with hidden state
LiveIterator->>InternalStorage: read current pair on next()
LiveIterator->>InternalStorage: advance index
Client->>LiveIterator: next()
LiveIterator->>Client: return {value, done}
Note over Client,InternalStorage: mutation to URLSearchParams between next() calls
Client->>LiveIterator: next()
LiveIterator->>InternalStorage: reflect updated pairs in result
alt source iterator yields malformed pair
LiveIterator->>EntriesTemplate: close source iterator
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp (1)
523-544:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCoerce the
nameargument the same way you now coercevalue.
get(),has(), anddelete()still downcastargs[0]straight tov8::String, so non-string names bypass the WebIDL/USVString conversion this PR is otherwise trying to implement. That leaves calls likeparams.get(1),params.has(true, 2),params.delete({ toString() { return "a"; } }), or an omitted first argument on the wrong path, andSymbolnames will not fail in the spec-defined way.Illustrative fix
void URLSearchParamsImpl::Delete(const v8::FunctionCallbackInfo<v8::Value> &args) { URLSearchParamsImpl *ptr = GetPointer(args.This()); if (ptr == nullptr) { return; } - auto key = ArgConverter::ConvertToString(args[0].As<v8::String>()); + std::string key; + if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[0], key)) { + return; + } if (args.Length() > 1 && !args[1]->IsUndefined()) { std::string value; if (!ValueToString(args.GetIsolate()->GetCurrentContext(), args[1], value)) { return; } - ptr->GetURLSearchParams()->remove(key, value); + ptr->GetURLSearchParams()->remove(key, value); } else { - ptr->GetURLSearchParams()->remove(key.c_str()); + ptr->GetURLSearchParams()->remove(key.c_str()); } }Apply the same
ValueToString(...)pattern toGetandHas.Also applies to: 586-603, 623-648
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp` around lines 523 - 544, Coerce the first (name) argument with the same WebIDL/USVString path you already use for value: replace direct downcasts like ArgConverter::ConvertToString(args[0].As<v8::String()) with a ValueToString(context, args[0], std::string name) call (using args.GetIsolate()->GetCurrentContext()) in URLSearchParamsImpl::Delete and do the same fixes in URLSearchParamsImpl::Get and URLSearchParamsImpl::Has; if ValueToString returns false, return early (preserve exception), and retain the existing special-case logic for an explicit undefined second argument in Delete so only a present non-undefined second arg triggers the value-filtered removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp`:
- Around line 125-174: IteratorNext currently ignores the actual receiver and
reads iterator state from args.Data(), allowing
illegal-invocation/use-after-detach; add a brand check: ensure args.This() is an
object and that its kStateSource/kStateIndex/kStateKind properties match/are
present (i.e., the same iterator state object) before proceeding, otherwise
throw a TypeError (e.g., "Illegal invocation"); update IteratorNext to read
state from args.This() (or verify args.This() === args.Data()) and perform the
same validation in the other iterator next implementations referenced (the ones
around the other line ranges), keeping the existing use of kStateSource,
kStateIndex, kStateKind, IteratorNext and URLSearchParamsImpl::GetPointer to
locate and validate the iterator state.
---
Outside diff comments:
In `@test-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp`:
- Around line 523-544: Coerce the first (name) argument with the same
WebIDL/USVString path you already use for value: replace direct downcasts like
ArgConverter::ConvertToString(args[0].As<v8::String()) with a
ValueToString(context, args[0], std::string name) call (using
args.GetIsolate()->GetCurrentContext()) in URLSearchParamsImpl::Delete and do
the same fixes in URLSearchParamsImpl::Get and URLSearchParamsImpl::Has; if
ValueToString returns false, return early (preserve exception), and retain the
existing special-case logic for an explicit undefined second argument in Delete
so only a present non-undefined second arg triggers the value-filtered removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30e744b1-a1f0-48d8-8fd8-4ebb8ee7aba7
📒 Files selected for processing (2)
test-app/app/src/main/assets/app/tests/testURLSearchParamsImpl.jstest-app/runtime/src/main/cpp/URLSearchParamsImpl.cpp
Contributor
Author
|
On the outside-diff comment about coercing the name argument: agreed. The IDL declares every name and value as USVString, so get, getAll, has, delete, append and set all run their arguments through the same ValueToString conversion (1ddf6d9, 65bf3bf). A number/boolean/object argument coerces via toString, a Symbol or a throwing toString raises a TypeError, and for append/set a failed coercion appends or replaces nothing. Covered by new tests; the full suite passes on an emulator (521 tests, 0 failures, URLSearchParams group at 57). |
The constructor previously handled only the string init form; any object init was stringified to "[object Object]", collapsing every query parameter into a single bogus key. It now follows the WHATWG/WebIDL init dispatch (https://url.spec.whatwg.org/#interface-urlsearchparams): record, sequence (any iterable of pairs: Array, Map, Set, another URLSearchParams, generators - with IteratorClose on abrupt completion), other primitives (USVString coercion; Symbol throws), and null/undefined (empty). entries()/keys()/values() returned plain v8::Arrays and the type exposed no @@iterator, so for..of, spread, and the copy-constructor form did not work; the array path also returned the first value for every occurrence of a repeated key. They now return genuine live ES iterators (reflecting mutations mid-iteration, per spec), and @@iterator aliases entries(). Also aligned to spec: get() returns null (not undefined) for a missing name; delete(name, value) and has(name, value) honor the optional value argument (coerced to USVString; an explicit undefined is treated as omitted, matching WPT).
Review follow-up:
- get()/getAll()/has()/delete() now coerce the name argument through
the same USVString conversion as the optional value argument instead of
casting it to a string blindly, so a number/boolean/object name works
and a Symbol (or a throwing toString) raises a TypeError.
- entries()/keys()/values() and the live iterator's next() brand-check
their receivers and throw "Illegal invocation" for foreign objects,
per the WebIDL iterable<> semantics. GetPointer also refuses to read an
internal field from an object that has none (e.g. entries.call({})),
which was an out-of-bounds read.
Completes the review follow-up: append() and set() now run both arguments through the same USVString coercion as the rest of the binding, instead of casting them to strings blindly. A number/boolean/object argument coerces via toString; a Symbol (or a throwing toString) raises a TypeError before anything is appended or replaced.
…bIDL - new URLSearchParams(null) parses the string "null" (so "null="). The IDL default "" applies only to undefined/missing; null goes through the union conversion, which has no null special case (the union is not nullable and a record is not a dictionary), so it lands in the USVString branch. Matches the whatwg-url reference implementation; Node treats null as empty and is the outlier, and WPT does not cover the case. - An own enumerable Symbol key in a record init throws a TypeError instead of being silently skipped: WebIDL record conversion takes the keys from [[OwnPropertyKeys]] filtered by enumerability only, and converting a Symbol key to a USVString throws. - The remaining members (get/getAll/has/append/set/delete/forEach/ sort/toString/size) brand-check their receiver and throw an "Illegal invocation" TypeError for a foreign object, consistent with entries()/keys()/values().
adrian-niculescu
force-pushed
the
fix/urlsearchparams-construction-live-iterators
branch
from
eae9cc4 to
dfcce8c
Compare
Comment-only. The previous wording ("on either abrupt completion")
could be read as closing the iterator on any failure, including a
throw from next() itself. Spelled out the actual rule: conversion
failures and wrong-length pairs close the source iterator, while a
throw from the stepping (next(), or reading the result's done/value)
does not, because the ES iterator protocol marks the iterator done on
such a throw and IteratorClose is skipped for a done iterator.
NathanWalker
approved these changes
Jun 12, 2026
adrian-niculescu
deleted the
fix/urlsearchparams-construction-live-iterators
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While chasing a failing backend request in our app we noticed it was hitting a URL that looked like this:
The culprit was an innocent
new URLSearchParams({ limit: "50", cursor: "abc123" }). The binding only handles the string init form, so any object passed to the constructor gets stringified to"[object Object]"and the whole query collapses into one bogus key. The same code works in browsers and Node, which made it a fun one to track down.This reworks the constructor to follow the WebIDL init dispatch from the URL spec: a plain object is read as a record (own enumerable keys in order; a Symbol key throws), an iterable (Array, Map, Set, another URLSearchParams, generators) is read as a sequence of pairs (with IteratorClose on abrupt completion, and a TypeError for a pair whose length isn't 2), other primitives and null coerce to a USVString and get parsed (a Symbol throws; null parses as the string "null", matching the whatwg-url reference implementation), and undefined produces empty params (the IDL default).
Writing tests for that surfaced a few more spec gaps, also fixed here:
entries()/keys()/values()returned plain arrays, and there was no@@iterator, sofor..of, spread, and thenew URLSearchParams(other)copy form didn't work. They now return live ES iterators per spec, with@@iteratoraliasingentries. This also fixes a duplicate-key bug: the oldget_keys()+get()walk returned the first value for every occurrence of a repeated key.get()returnsnullfor a missing name instead ofundefined(one existing test assertedundefinedand was updated to match the spec).delete(name, value)andhas(name, value)honor the optional value argument, which ada already exposes. An explicitundefinedvalue is treated as omitted, matching WPT.The URLSearchParams test group now has 49 specs covering all of the above. Full suite run on an emulator via
runtests: 513 tests, 0 failures.iOS counterpart: NativeScript/ios#388.
Update after review: every name and value argument across get/getAll/has/delete/append/set now goes through the same USVString coercion (a Symbol throws), entries()/keys()/values() and the iterator's next() brand-check their receivers, and GetPointer no longer reads an internal field from objects that have none (entries.call({}) used to be an out-of-bounds read). A second pass against the IDL and the whatwg-url reference implementation aligned three more corners: new URLSearchParams(null) parses as the string "null" instead of being treated as empty (the union conversion has no null special case; WPT does not cover it and Node is the outlier), an enumerable Symbol key in a record init throws, and the remaining members (get/getAll/has/append/set/delete/forEach/sort/toString/size) brand-check their receiver like the iterator methods.
Summary by CodeRabbit
Bug Fixes
entries/keys/values/default) now behave per spec: live, have correct identity/branding, preserve duplicates, close on errors, and throw on illegal invocation;get()returns null for missing keys.Improvements
?;delete()/has()support value-based matching with proper coercion and explicit-undefined handling.Tests