fix: implement style:deepObject query parameter serialization#3193
fix: implement style:deepObject query parameter serialization#3193leanderpeter wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds support for OpenAPI Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Gen as GeneratedFunction
participant Params as URLSearchParams
participant Fetcher as Fetch
Caller->>Gen: invoke with params (may include deepObject)
Gen->>Gen: merge path+operation parameters, dedupe by "in:name"
Gen->>Gen: determine deepObject parameter names and date-time presence
Gen->>Gen: for each param key,value
alt key in deepObjectParameters and typeof value === 'object' && value !== null && !Array.isArray(value)
Gen->>Gen: for each [subKey, subValue] in Object.entries(value)
Gen->>Params: append `${encodeURIComponent(key)}[${encodeURIComponent(subKey)}]=${stringify(subValue)}`
else exploded array handling / scalar handling
Gen->>Params: append standard entries
end
Gen->>Gen: qs = join(deepObjectEntries, '&') + (&) + Params.toString()
Gen->>Fetcher: fetch(url + '?' + qs)
Fetcher-->>Caller: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
leanderpeter
force-pushed
the
fix/deep-object-query-param-serialization
branch
2 times, most recently
from
6ea680e to
138dd4d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fetch/src/index.ts`:
- Around line 154-161: The deepObject detection only checks parameterObjects
derived solely from the operation-level parameters, so path-item (route-level)
query params with style: 'deepObject' are missed; update the code that builds
parameterObjects (so that the array used by deepObjectParameters,
deepObjectParameterNames) to merge path-item parameters with operation-level
parameters (like how packages/solid-start handles merging) before filtering —
ensure you reference and merge both the route-level parameters array and the
operation-level parameters array into the variable consumed by
deepObjectParameters and deepObjectParameterNames so deepObject-style query
params are correctly detected and serialized.
- Line 234: The current one-liner that sets stringifiedParams rewrites %5B/%5D
across the whole query string (affecting values); change it to build
stringifiedParams by serializing normalizedParams and then rewriting %5B/%5D
only on the key side of each key=value pair: after getting
normalizedParams.toString(), split on '&', for each pair split at the first '='
into key and value, replace /%5B/g and /%5D/g on the key only (or leave value
untouched), then rejoin pairs with '=' and '&'; update the assignment where
stringifiedParams is created and keep the deepObjectParameters conditional so
replacements run only when deepObjectParameters.length > 0.
In `@packages/solid-start/src/index.ts`:
- Around line 366-367: The current replacement on queryString unescapes
percent-encoded brackets in both names and values, altering scalar values when
deepObjectParameters exist; instead, after building normalizedParams, iterate
its entries and rebuild queryString so only parameter names are decoded (replace
%5B/%5D in the key) while leaving values untouched—use normalizedParams (or
URLSearchParams) to get each key/value, transform only the key with
.replace(/%5B/g,'[').replace(/%5D/g,']') and join into the final queryString,
then set url = queryString ? `${route}?${queryString}` : `${route}`.
🪄 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: 608fbd10-b5d6-48f0-8b80-2b91b2532d75
📒 Files selected for processing (4)
packages/fetch/src/index.test.tspackages/fetch/src/index.tspackages/solid-start/src/index.test.tspackages/solid-start/src/index.ts
| const deepObjectParameters = parameterObjects.filter( | ||
| (parameterObject) => | ||
| parameterObject.in === 'query' && parameterObject.style === 'deepObject', | ||
| ); | ||
|
|
||
| const deepObjectParameterNames = deepObjectParameters.map( | ||
| (parameter) => parameter.name, | ||
| ); |
There was a problem hiding this comment.
Handle path-item deepObject params in fetch too.
This new filter only sees parameterObjects, and in this generator that list still comes from spec?.[verb]?.parameters only. A valid query param declared under paths.<route>.parameters will bypass the deepObject branch and still fall back to value.toString(), i.e. ...[object Object]. packages/solid-start/src/index.ts already merged path-item and operation-level parameters to avoid that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fetch/src/index.ts` around lines 154 - 161, The deepObject detection
only checks parameterObjects derived solely from the operation-level parameters,
so path-item (route-level) query params with style: 'deepObject' are missed;
update the code that builds parameterObjects (so that the array used by
deepObjectParameters, deepObjectParameterNames) to merge path-item parameters
with operation-level parameters (like how packages/solid-start handles merging)
before filtering — ensure you reference and merge both the route-level
parameters array and the operation-level parameters array into the variable
consumed by deepObjectParameters and deepObjectParameterNames so
deepObject-style query params are correctly detected and serialized.
✅ Addressed in commit 138dd4d
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fetch/src/index.ts (1)
87-92:⚠️ Potential issue | 🟠 MajorPath-item level parameters are not merged with operation-level parameters.
The
parametersarray is populated only fromspec?.[verb]?.parameters(operation-level). A query param declared atpaths.<route>.parameters(path-item level) withstyle: 'deepObject'will not be detected and will fall back tovalue.toString(), producing[object Object].The solid-start implementation (lines 181-200) correctly merges path-item and operation-level parameters. The same merge logic should be applied here.
🔧 Proposed fix
const spec = context.spec.paths?.[pathRoute]; - const parameters = spec?.[verb]?.parameters ?? []; + const pathItemParameters = spec?.parameters ?? []; + const operationParameters = spec?.[verb]?.parameters ?? []; + const mergedParameters = [ + ...pathItemParameters, + ...operationParameters, + ] as (OpenApiParameterObject | OpenApiReferenceObject)[]; + const byKey = new Map<string, OpenApiParameterObject | OpenApiReferenceObject>(); + for (const param of mergedParameters) { + const { schema } = resolveRef(param, context); + const paramObj = schema as OpenApiParameterObject; + byKey.set(`${paramObj.in}:${paramObj.name}`, param); + } + const parameters = [...byKey.values()]; const parameterObjects = parameters.map((parameter) => { const { schema } = resolveRef(parameter, context); return schema as OpenApiParameterObject; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fetch/src/index.ts` around lines 87 - 92, The code currently gathers only operation-level parameters into `parameters` (using `spec?.[verb]?.parameters`) so path-item level params under `spec.paths?.[pathRoute].parameters` are ignored; change the logic that builds `parameters` to merge path-item and operation-level arrays (e.g., combine `spec?.parameters` with `spec?.[verb]?.parameters`) before mapping to `parameterObjects`, then continue to call `resolveRef(parameter, context)` for each merged parameter so path-level query params (including style: 'deepObject') are detected and handled the same as in the solid-start merge approach.
♻️ Duplicate comments (2)
packages/solid-start/src/index.ts (1)
371-371:⚠️ Potential issue | 🟠 MajorThe
%5B/%5Dreplacement affects values, not just keys.Same issue as in the fetch package: this replaces encoded brackets in the entire query string, including values. Values legitimately containing
[or]will be incorrectly decoded.🔧 Safer fix: decode brackets only in keys
- const queryString = normalizedParams.toString()${deepObjectParameters.length > 0 ? ".replace(/%5B/g, '[').replace(/%5D/g, ']')" : ''}; + const queryString = ${deepObjectParameters.length > 0 ? `normalizedParams.toString().split('&').map((pair) => { + const eqIndex = pair.indexOf('='); + if (eqIndex === -1) return pair.replace(/%5B/g, '[').replace(/%5D/g, ']'); + return pair.slice(0, eqIndex).replace(/%5B/g, '[').replace(/%5D/g, ']') + '=' + pair.slice(eqIndex + 1); + }).join('&')` : 'normalizedParams.toString()'};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-start/src/index.ts` at line 371, The current construction of queryString uses normalizedParams.toString() with a global .replace that will decode %5B/%5D everywhere (including values); update the logic that builds queryString (the const queryString) to only decode bracket encodings in parameter names (keys) when deepObjectParameters is non-empty: iterate normalizedParams.entries() (or use normalizedParams.forEach) to build the encoded key=value pairs, decode %5B/%5D only on the encoded key portion, join pairs with '&', and assign that string to queryString so values remain untouched; reference normalizedParams, deepObjectParameters and the const queryString in your change.packages/fetch/src/index.ts (1)
234-234:⚠️ Potential issue | 🟠 MajorThe
%5B/%5Dreplacement affects values, not just keys.This post-processing replaces encoded brackets in the entire query string, including values. If any value legitimately contains
[or](e.g., a JSON string value), it will be incorrectly decoded when a deepObject param is present.🔧 Safer fix: decode brackets only in keys
- ${queryParams ? `const stringifiedParams = normalizedParams.toString()${deepObjectParameters.length > 0 ? ".replace(/%5B/g, '[').replace(/%5D/g, ']')" : ''};` : ``} + ${queryParams ? `const stringifiedParams = ${deepObjectParameters.length > 0 ? `normalizedParams.toString().split('&').map((pair) => { + const eqIndex = pair.indexOf('='); + if (eqIndex === -1) return pair.replace(/%5B/g, '[').replace(/%5D/g, ']'); + return pair.slice(0, eqIndex).replace(/%5B/g, '[').replace(/%5D/g, ']') + '=' + pair.slice(eqIndex + 1); + }).join('&')` : 'normalizedParams.toString()'};` : ``}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fetch/src/index.ts` at line 234, The current post-processing replaces "%5B"/"%5D" across the whole query string (variable stringifiedParams computed from normalizedParams), which wrongly decodes bracket characters inside values when deepObjectParameters is non-empty; change the logic to only decode brackets in parameter keys by iterating over normalizedParams (or URLSearchParams entries), transform each key by replacing %5B/%5D with "["/"]" while leaving the value untouched, then reassemble the query string into stringifiedParams; update the code paths that reference deepObjectParameters, normalizedParams, and stringifiedParams to use this key-only decoding approach.
🧹 Nitpick comments (1)
packages/fetch/src/index.test.ts (1)
233-265: Consider adding a test for values containing bracket characters.Once the
%5B/%5Dreplacement issue is fixed (to only affect keys), a test verifying that values like{ filter: { data: "[test]" } }preserve their encoded brackets would prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fetch/src/index.test.ts` around lines 233 - 265, Add a test that ensures bracket characters in parameter values are preserved (i.e., remain percent-encoded) while the existing replacement only targets keys: inside the existing describe('generateRequestFunction — deepObject query parameters') add an it(...) that uses makeContext/makeOptions/makeVerbOptions to generate an implementation for a deepObject param (like scope or filter) where the value contains brackets (e.g., { filter: { data: "[test]" } }), then assert the generated implementation still encodes the value brackets as '%5Btest%5D' (and does not replace them to '['/']') while keeping the existing expectations about key-bracket replacement behavior; target the same helpers used in the file (generateImplementation, makeContext, makeOptions, makeVerbOptions, STUB_QUERY_PARAMS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/fetch/src/index.ts`:
- Around line 87-92: The code currently gathers only operation-level parameters
into `parameters` (using `spec?.[verb]?.parameters`) so path-item level params
under `spec.paths?.[pathRoute].parameters` are ignored; change the logic that
builds `parameters` to merge path-item and operation-level arrays (e.g., combine
`spec?.parameters` with `spec?.[verb]?.parameters`) before mapping to
`parameterObjects`, then continue to call `resolveRef(parameter, context)` for
each merged parameter so path-level query params (including style: 'deepObject')
are detected and handled the same as in the solid-start merge approach.
---
Duplicate comments:
In `@packages/fetch/src/index.ts`:
- Line 234: The current post-processing replaces "%5B"/"%5D" across the whole
query string (variable stringifiedParams computed from normalizedParams), which
wrongly decodes bracket characters inside values when deepObjectParameters is
non-empty; change the logic to only decode brackets in parameter keys by
iterating over normalizedParams (or URLSearchParams entries), transform each key
by replacing %5B/%5D with "["/"]" while leaving the value untouched, then
reassemble the query string into stringifiedParams; update the code paths that
reference deepObjectParameters, normalizedParams, and stringifiedParams to use
this key-only decoding approach.
In `@packages/solid-start/src/index.ts`:
- Line 371: The current construction of queryString uses
normalizedParams.toString() with a global .replace that will decode %5B/%5D
everywhere (including values); update the logic that builds queryString (the
const queryString) to only decode bracket encodings in parameter names (keys)
when deepObjectParameters is non-empty: iterate normalizedParams.entries() (or
use normalizedParams.forEach) to build the encoded key=value pairs, decode
%5B/%5D only on the encoded key portion, join pairs with '&', and assign that
string to queryString so values remain untouched; reference normalizedParams,
deepObjectParameters and the const queryString in your change.
---
Nitpick comments:
In `@packages/fetch/src/index.test.ts`:
- Around line 233-265: Add a test that ensures bracket characters in parameter
values are preserved (i.e., remain percent-encoded) while the existing
replacement only targets keys: inside the existing
describe('generateRequestFunction — deepObject query parameters') add an it(...)
that uses makeContext/makeOptions/makeVerbOptions to generate an implementation
for a deepObject param (like scope or filter) where the value contains brackets
(e.g., { filter: { data: "[test]" } }), then assert the generated implementation
still encodes the value brackets as '%5Btest%5D' (and does not replace them to
'['/']') while keeping the existing expectations about key-bracket replacement
behavior; target the same helpers used in the file (generateImplementation,
makeContext, makeOptions, makeVerbOptions, STUB_QUERY_PARAMS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33746a2b-834d-4d40-a9db-eaa6bad8fd71
📒 Files selected for processing (4)
packages/fetch/src/index.test.tspackages/fetch/src/index.tspackages/solid-start/src/index.test.tspackages/solid-start/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/solid-start/src/index.test.ts
leanderpeter
force-pushed
the
fix/deep-object-query-param-serialization
branch
from
138dd4d to
19989a4
Compare
Orval did not implement the OpenAPI `style` field for query parameters. When a parameter declared `style: deepObject` with an object schema, the generated code called `.toString()` on the entire object, producing `key=[object+Object]` instead of the specified bracket notation `key[field]=value`. This adds a deepObject branch to the URL-building template in both `@orval/fetch` and `@orval/solid-start`, following the same pattern used for exploded array parameters. Bracket-encoded output from URLSearchParams is decoded so servers receive raw `[]` notation. Closes orval-labs#2589 Made-with: Cursor
leanderpeter
force-pushed
the
fix/deep-object-query-param-serialization
branch
from
19989a4 to
d1096ff
Compare
|
We will check them in order, so please wait a moment. |
|
|
||
| type OpenApiParameterLike = OpenApiParameterObject | OpenApiReferenceObject; | ||
|
|
||
| function makeOutput(useDates = false): ContextSpec['output'] { |
There was a problem hiding this comment.
Could this definition be simplified using the core generator? The same applies to other definitions.
There was a problem hiding this comment.
I mirrored the local helper setup from packages/solid-start/src/index.test.ts to keep the test style aligned with an existing package-level generator test
|
|
||
| const implementation = generateImplementation(verbOptions, options); | ||
|
|
||
| expect(implementation).toContain('const deepObjectParameters = ["scope"]'); |
There was a problem hiding this comment.
Would it be difficult to use the entire set of functions instead of toContain?
There was a problem hiding this comment.
I used toContain because that’s the dominant pattern in the existing solid-start generator tests, so I kept the fetch tests aligned with the current package-level style. If you’d prefer more behavioral assertions here, I can follow up by exercising the generated output more directly rather than checking structural snippets.
There was a problem hiding this comment.
Yes, it's unclear from this alone whether this is the correct behavior, so I'd like to change it.
| name: 'scope', | ||
| in: 'query', | ||
| style: 'deepObject', | ||
| explode: true, |
There was a problem hiding this comment.
What happens if the style is 'deepObject' but the explode method is not specified (false)?
There was a problem hiding this comment.
Current implementation treats style: 'deepObject' as sufficient to use deepObject serialization, even if explode is omitted or false.
I chose that because the practical intent behind this issue is to support bracket-style object query serialization, and the OpenAPI examples for deepObject are all shown with exploded object entries.
But the spec default for non-form styles makes the omitted/false case ambiguous. If you’d prefer strict handling here, I can change both generators to only apply this branch when style === 'deepObject' && explode === true, and add tests for the omitted/false cases (no strong opinion on this one)
There was a problem hiding this comment.
Yes, I want to strictly restrict it to only style === 'deepObject' && explode === true.
|
@leanderpeter |
3 participants
Summary
Implements
style: deepObjectquery parameter serialization for@orval/fetchand@orval/solid-start.When a query parameter declares
style: deepObjectwith an object schema, the generated URL-building code now emits bracket notation (key[field]=value) instead of calling.toString()on the object, which producedkey=[object+Object].Closes #2589
Changes
packages/fetch/src/index.ts— AddeddeepObjectparameter detection and a template branch that serializes object values askey[subKey]=value. Decoded%5B/%5DfromURLSearchParamsoutput so servers receive raw bracket notation. AddeduseDatessupport for deepObject sub-values withformat: date-time.packages/solid-start/src/index.ts— Same changes, following solid-start's existing conventions (path-level parameter merging, OpenAPI default explode semantics).packages/fetch/src/index.test.ts(new) — 9 tests covering deepObject serialization, bracket decoding, mixed param types, scalar fallback omission, anduseDateshandling.packages/solid-start/src/index.test.ts— 9 new tests in adeepObject query parametersdescribe block, plus bracket decoding anduseDatescoverage.Approach
Follows the existing pattern for
explode: truearray parameter handling:parameterObjectsforstyle === 'deepObject'query paramsObject.entries(params).forEach(...)loopnormalizedParams.append(\${key}[${subKey}]`, ...)` for each sub-key.replace(/%5B/g, '[').replace(/%5D/g, ']'))Scoped to
deepObjectonly —pipeDelimited,spaceDelimited, and other style values are not addressed.Test plan
bun run build— 12/12 packagesbun run typecheck— 12/12 packagesbun run test— 903 unit tests pass (9 new in fetch, 9 new in solid-start)bun run test:snapshots— 3,481 snapshot tests pass, no updates neededMade with Cursor
Summary by CodeRabbit
New Features
Tests