fix(fetch): respect OpenAPI default explode: true for array query parameters (#3231)#3308
fix(fetch): respect OpenAPI default explode: true for array query parameters (#3231)#3308zeriong wants to merge 8 commits into
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:
📝 WalkthroughWalkthroughArray-like query parameter explode logic was refactored into ChangesQuery Parameter Explosion Default Compliance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
In v3.1, the default value is |
|
According to the Swagger spec, for query parameters the default is, in fact, Here's a typical example produced by FastAPI: {
"/api/v1/interests": {
"get": {
"summary": "List all interests",
"description": "Lists all available interests with localized names.",
"operationId": "list_interests_endpoint",
"parameters": [
{
"name": "l",
"in": "query",
"required": false,
"schema": {
"anyOf": [
{
"type": "array",
"items": {
"type": "string"
}
},
{
"type": "null"
}
],
"description": "Language codes to prioritize for text",
"title": "L"
},
"description": "Language codes to prioritize for text"
}
],
}
}
} |
|
@idiodoneo-chris |
Use nullish coalescing (parameterObject.explode ?? true) so that undefined defaults to true per the OpenAPI 3.0/3.1 spec instead of being treated as falsy. Closes orval-labs#3231 Signed-off-by: zeriong <jaeryong95@gmail.com>
Signed-off-by: zeriong <jaeryong95@gmail.com>
97ac3f4 to
6cb04e1
Compare
|
@soartec-lab const style = parameterObject.style ?? 'form';
return (
parameterObject.in === 'query' &&
isArrayLike &&
style === 'form' &&
(parameterObject.explode ?? true)
);Snapshots unchanged. |
| | undefined) ?? [] | ||
| ).some((s) => resolveSchemaRef(s, context).schema.type === 'array'); | ||
|
|
||
| const style = parameterObject.style ?? 'form'; |
There was a problem hiding this comment.
Note that the default value may change depending on the type of parameter.
Describes how the parameter value will be serialized depending on the type of the parameter value. Default values (based on value of in): for "query" - "form"; for "path" - "simple"; for "header" - "simple"; for "cookie" - "form".
There was a problem hiding this comment.
@soartec-lab
Thanks for check, please one more review!
Address review feedback on orval-labs#3308: the OpenAPI spec defines and defaults per value (query/cookie -> form/true, path/header -> simple/false). Extract , , , and so the spec mapping is explicit and array-like schema detection guards against malformed oneOf/anyOf/allOf. No behavior change; snapshots unchanged. Signed-off-by: zeriong <jaeryong95@gmail.com>
Address review feedback on orval-labs#3308: the OpenAPI spec defines style and explode defaults per "in" value (query/cookie -> form/true, path/header -> simple/false). Extract getDefaultStyle, getDefaultExplode, isArrayLikeSchema, and shouldExplodeArrayQueryParameter so the spec mapping is explicit and array-like schema detection guards against malformed oneOf/anyOf/allOf. No behavior change; snapshots unchanged. Signed-off-by: zeriong <jaeryong95@gmail.com>
6812509 to
0b338b7
Compare
|
@zeriong https://github.com/orval-labs/orval/blob/master/CONTRIBUTING.md#a-note-about-ai |
|
Apologies — I think I misinterpreted your earlier note about per- Shall I revert to the 2nd commit (6cb04e1) with just I'll also be more careful in reviewing AI-assisted output before pushing. Thanks for your patience. |
| parameterObject.in === 'query' && isArrayLike && parameterObject.explode | ||
| ); | ||
| }); | ||
| const explodeParameters = parameterObjects.filter((parameterObject) => |
There was a problem hiding this comment.
What is the significance of extracting this function? It just looks like the code has been moved.
There was a problem hiding this comment.
Fair point — it's a single-use extraction that just relocates the original inline filter without reducing complexity. I'll fold the logic back into the .filter() callback.
| } | ||
|
|
||
| const style = parameterObject.style ?? getDefaultStyle(parameterObject.in); | ||
| // Only `form` style supports explode semantics in a way orval currently |
There was a problem hiding this comment.
/ Only form style supports explode semantics in a way orval currently
// emits (repeated key=value pairs). Other styles (spaceDelimited,
// pipeDelimited, deepObject) are intentionally not exploded here.
There was a problem hiding this comment.
@soartec-lab
I think the annotation is simply copied, is there any other meaning?
|
|
||
| // Per OpenAPI 3.0/3.1, the default `explode` is `true` when `style` is `form`, | ||
| // otherwise `false`. https://swagger.io/specification/ | ||
| const getDefaultExplode = (style: string): boolean => style === 'form'; |
There was a problem hiding this comment.
Similarly, what is the significance of extracting this variable?
There was a problem hiding this comment.
Agreed. By the time this is reached, style === 'form' is already guaranteed by the guard above, so getDefaultExplode(style) always returns true. The extraction adds nothing — I'll inline it to parameterObject.explode ?? true.
| // Per OpenAPI 3.0/3.1, the default `style` depends on `in`: | ||
| // query, cookie -> "form"; path, header -> "simple". | ||
| // https://swagger.io/specification/ | ||
| const getDefaultStyle = (parameterIn: string | undefined): string => { |
There was a problem hiding this comment.
It seems unlikely that the default value will be reached. I thought a ternary operator would suffice.
There was a problem hiding this comment.
You're right. getDefaultStyle is only ever called from shouldExplodeArrayQueryParameter, which already returns early when in !== 'query'. So it's always called with 'query' and always returns 'form' — the cookie/path/header/default branches are unreachable here. I'll drop the switch entirely and just default style to 'form'.
|
@soartec-lab |
Summary
parameterObject.explode ?? true) so thatundefineddefaults totrueper the OpenAPI 3.0/3.1 spec instead of being treated asfalsy.
explodefield were serialized as comma-separated (?ids=1,2,3) instead of repeated key-value pairs(
?ids=1&ids=2&ids=3).Breaking Change
Users relying on the implicit comma-separated behavior without setting
explode: falsewill see a change in serialization output. To preserve the old behavior,explicitly set
explode: falseon the parameter.Test plan
bun run build— 12 successfulbun vitest run— 2303 passed (3 pre-existing failures inresolve-version.test.ts, unrelated)bun run test:snapshots— 3746 passed; 2 snapshot files updated (fetch/parameters,fetch/dateParams)solid-startpackage already handled this correctly, no other clients affectedCloses #3231
Summary by CodeRabbit