fix: handle Union with objects in query array params#1830
fix: handle Union with objects in query array params#1830eL1fe wants to merge 2 commits intoelysiajs:mainfrom
Conversation
When using t.Union([t.Object({a}), t.Object({b})]) inside a query
array, the second object variant always failed validation.
Two issues:
1. ArrayQuery decode used naive split(',') which produced string items
that TypeBox couldn't match against Object union variants. Now tries
JSON.parse('[...]') first to properly parse objects.
2. Multi-param format (data=1&data={"b":"world"}) created a string
array that failed Check before Decode could run. Now joins
multi-param values as comma-separated string so ArrayQuery
Decode handles both formats uniformly.
Fixes elysiajs#1829
WalkthroughArray query parsing was tightened: bracketed values now attempt JSON.parse with a safe fallback; comma-delimited single-value arrays are parsed with nesting- and quote-aware splitting and per-item JSON parsing to better handle objects/arrays in query strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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)
src/parse-query.ts (1)
93-111:⚠️ Potential issue | 🟠 MajorStop leaving garbage mixed arrays, you dummy ♡~~
Your code has a nasty inconsistency that's gonna bite you~
When
finalValuestarts with[, you parse it into a real array (line 95). Then at line 106, ifcurrentValueis already a string, you dounshift()and create a disgusting mixed-type array like["{\"a\":\"x\"}",{"b":"y"}].But in the else branch (line 111), you just concatenate strings normally. This means the same parameter appearing in different orders produces completely different shapes—sometimes actual arrays, sometimes stringified messes. And when you stringify that mixed array? Haha, the objects turn into
[object Object]strings. How embarrassing~ (¬‿¬)Just normalize everything to comma-separated strings from the start, like you're already doing in the non-bracket branch. Don't store both arrays AND strings for the same parameter, you're making this harder than it needs to be ♡
The suggested fix below keeps it uniform—always strings, always consistent:
Here's how to stop being so messy~
+ const normalizeArrayValue = (value: string) => { + if (value.charCodeAt(0) !== 91) return value + + try { + return (JSON.parse(value) as unknown[]) + .map((item) => JSON.stringify(item)) + .join(',') + } catch { + return value.slice(1, -1) + } + } + if (array && array?.[finalKey]) { - if (finalValue.charCodeAt(0) === 91) { - try { - finalValue = JSON.parse(finalValue) as any - } catch { - finalValue = finalValue.slice(1, -1).split(',') as any - } - - if (currentValue === undefined) result[finalKey] = finalValue - else if (Array.isArray(currentValue)) - currentValue.push(...finalValue) - else { - result[finalKey] = finalValue - result[finalKey].unshift(currentValue) - } - } else { - // join multi-param values as comma-separated so ArrayQuery - // Decode handles them uniformly (same as comma format) - if (currentValue === undefined) result[finalKey] = finalValue - else result[finalKey] = currentValue + ',' + finalValue - } + const normalized = normalizeArrayValue(finalValue) + + if (currentValue === undefined) result[finalKey] = normalized as any + else result[finalKey] = `${currentValue},${normalized}` as any } else { result[finalKey] = finalValue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parse-query.ts` around lines 93 - 111, The code in parse-query.ts mixes real arrays and strings for the same query param (variables finalValue, currentValue, finalKey, and result) when finalValue starts with '['; to fix, always normalize finalValue to a comma-separated string before assigning/merging: if JSON.parse succeeds and returns an array, convert it to a string via array.join(','); if JSON.parse fails and you split, join the split pieces into a single comma string; then use the same merge logic used in the non-bracket branch (if currentValue undefined set result[finalKey] to that string, else set result[finalKey] = currentValue + ',' + normalizedFinalValue) and remove branches that create mixed-type arrays (e.g., the Array.isArray(currentValue) unshift/push paths) so result values are always strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/type-system/index.ts`:
- Around line 437-445: In the decode function, the fallback that currently does
value.split(',') must be replaced so it splits only at top-level commas
(respecting {}, [], and quoted strings) and then applies the existing
tryParseItem logic to each token; update the block inside the catch of
compiler.Decode(JSON.parse(`[${value}]`)) to call a top-level-aware splitter
(e.g., splitTopLevel) on value, map tryParseItem over the resulting tokens, and
pass that array into compiler.Decode so structured values and quoted strings are
preserved (references: function decode, helper tryParseItem, and add/use
splitTopLevel for top-level splitting).
---
Outside diff comments:
In `@src/parse-query.ts`:
- Around line 93-111: The code in parse-query.ts mixes real arrays and strings
for the same query param (variables finalValue, currentValue, finalKey, and
result) when finalValue starts with '['; to fix, always normalize finalValue to
a comma-separated string before assigning/merging: if JSON.parse succeeds and
returns an array, convert it to a string via array.join(','); if JSON.parse
fails and you split, join the split pieces into a single comma string; then use
the same merge logic used in the non-bracket branch (if currentValue undefined
set result[finalKey] to that string, else set result[finalKey] = currentValue +
',' + normalizedFinalValue) and remove branches that create mixed-type arrays
(e.g., the Array.isArray(currentValue) unshift/push paths) so result values are
always strings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b48ec7ff-e8e2-4c8b-afbe-3cb93b14a342
📒 Files selected for processing (2)
src/parse-query.tssrc/type-system/index.ts
The naive split(',') fallback sliced through JSON objects with multiple
keys like {"b":"world","c":1}. Added splitTopLevel that respects
{} [] nesting and quoted strings, and applies tryParseItem to each
token so structured values survive the fallback path.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/type-system/index.ts (1)
446-450: Escaped backslash edge case in quote detection, dummy~ (´∀`)♡Your escape check only looks at the single preceding character, but what if someone has
\\\"(escaped backslash followed by a quote)? The\\is the escape sequence, so the"should actually END the string, not be treated as escaped~Example:
{"a":"test\\"}- the trailing"would be incorrectly considered escaped because it's preceded by\, but that\is part of\\(escaped backslash) ξ・∀・ξBuuut~ this is such a rare edge case for query params that I'll let it slide, you lucky baka ♡
✨ More robust escape handling (if you want to be a good developer for once~)
if (inStr) { - if (ch === inStr && value.charCodeAt(i - 1) !== 92) - inStr = 0 + if (ch === inStr) { + // Count consecutive backslashes before quote + let backslashes = 0 + for (let j = i - 1; j >= 0 && value.charCodeAt(j) === 92; j--) { + backslashes++ + } + // Even number of backslashes means quote is NOT escaped + if (backslashes % 2 === 0) inStr = 0 + } continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/type-system/index.ts` around lines 446 - 450, The quote-termination check incorrectly treats any single preceding backslash as an escape (using value.charCodeAt(i - 1) !== 92), which fails for sequences like \\\"; update the logic around variables inStr, ch, value and index i so you count the number of consecutive backslashes immediately before the quote and only treat the quote as escaped when that count is odd; replace the single-char check with a loop or backward scan that computes backslashCount and then end the string (set inStr = 0) only if backslashCount % 2 === 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/type-system/index.ts`:
- Around line 446-450: The quote-termination check incorrectly treats any single
preceding backslash as an escape (using value.charCodeAt(i - 1) !== 92), which
fails for sequences like \\\"; update the logic around variables inStr, ch,
value and index i so you count the number of consecutive backslashes immediately
before the quote and only treat the quote as escaped when that count is odd;
replace the single-char check with a loop or backward scan that computes
backslashCount and then end the string (set inStr = 0) only if backslashCount %
2 === 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebad7910-2125-42da-b780-37fcecdf5e36
📒 Files selected for processing (1)
src/type-system/index.ts
1 participant
Summary
When using
t.Union([t.Object({a}), t.Object({b})])inside a query array schema, only the first object variant worked. The second always failed with a validation error.Problem
Two root causes:
ArrayQuerydecode usedvalue.split(',')which produced string items like'{"b":"world"}'that TypeBox couldn't match against Object union variantsdata=1&data={"b":"world"}) built a string array that failedCheckbeforeDecodecould runChanges
src/type-system/index.ts: ArrayQuery decode now triesJSON.parse('[' + value + ']')before falling back to naive split, and parses JSON-like single items viatryParseItemsrc/parse-query.ts: multi-param ArrayQuery values are joined as comma-separated string instead of building a string array, so Decode handles both formats uniformlyTest plan
Fixes #1829
Summary by CodeRabbit