-
-
Notifications
You must be signed in to change notification settings - Fork 623
fix(fetch): respect OpenAPI default explode: true for array query parameters (#3231) #3308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 7 commits
9ed5e47
6cb04e1
78de9b4
0b338b7
c197b9f
84dc056
d6a20ae
6d35e93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,83 @@ const getStatusCodeType = (key: string): string => { | |
| return key; | ||
| }; | ||
|
|
||
| // 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 => { | ||
| switch (parameterIn) { | ||
| case 'query': | ||
| case 'cookie': { | ||
| return 'form'; | ||
| } | ||
| case 'path': | ||
| case 'header': { | ||
| return 'simple'; | ||
| } | ||
| default: { | ||
| return 'form'; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // 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'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, what is the significance of extracting this variable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| const isArrayLikeSchema = ( | ||
| schemaObject: OpenApiSchemaObject, | ||
| context: GeneratorOptions['context'], | ||
| ): boolean => { | ||
| if (schemaObject.type === 'array') { | ||
| return true; | ||
| } | ||
|
|
||
| const variants: (keyof Pick< | ||
| OpenApiSchemaObject, | ||
| 'oneOf' | 'anyOf' | 'allOf' | ||
| >)[] = ['oneOf', 'anyOf', 'allOf']; | ||
|
|
||
| return variants.some((key) => { | ||
| const subSchemas = schemaObject[key] as | ||
| | (OpenApiSchemaObject | OpenApiReferenceObject)[] | ||
| | undefined; | ||
| if (!Array.isArray(subSchemas) || subSchemas.length === 0) { | ||
| return false; | ||
| } | ||
| return subSchemas.some( | ||
| (s) => resolveSchemaRef(s, context).schema.type === 'array', | ||
| ); | ||
| }); | ||
| }; | ||
|
|
||
| const shouldExplodeArrayQueryParameter = ( | ||
| parameterObject: OpenApiParameterObject, | ||
| context: GeneratorOptions['context'], | ||
| ): boolean => { | ||
| if (parameterObject.in !== 'query' || !parameterObject.schema) { | ||
| return false; | ||
| } | ||
|
|
||
| const { schema: schemaObject } = resolveSchemaRef( | ||
| parameterObject.schema, | ||
| context, | ||
| ); | ||
|
|
||
| if (!isArrayLikeSchema(schemaObject, context)) { | ||
| return false; | ||
| } | ||
|
|
||
| const style = parameterObject.style ?? getDefaultStyle(parameterObject.in); | ||
| // Only `form` style supports explode semantics in a way orval currently | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. / Only
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @soartec-lab |
||
| // emits (repeated `key=value` pairs). Other styles (spaceDelimited, | ||
| // pipeDelimited, deepObject) are intentionally not exploded here. | ||
| if (style !== 'form') { | ||
| return false; | ||
| } | ||
|
|
||
| return parameterObject.explode ?? getDefaultExplode(style); | ||
| }; | ||
|
|
||
| const FETCH_DEPENDENCIES: GeneratorDependency[] = [ | ||
| { | ||
| exports: [ | ||
|
|
@@ -103,38 +180,9 @@ export const generateRequestFunction = ( | |
| return schema as OpenApiParameterObject; | ||
| }); | ||
|
|
||
| const explodeParameters = parameterObjects.filter((parameterObject) => { | ||
| if (!parameterObject.schema) { | ||
| return false; | ||
| } | ||
|
|
||
| const { schema: schemaObject } = resolveSchemaRef( | ||
| parameterObject.schema, | ||
| context, | ||
| ); | ||
|
|
||
| const isArrayLike = | ||
| schemaObject.type === 'array' || | ||
| ( | ||
| (schemaObject.oneOf as | ||
| | (OpenApiSchemaObject | OpenApiReferenceObject)[] | ||
| | undefined) ?? [] | ||
| ).some((s) => resolveSchemaRef(s, context).schema.type === 'array') || | ||
| ( | ||
| (schemaObject.anyOf as | ||
| | (OpenApiSchemaObject | OpenApiReferenceObject)[] | ||
| | undefined) ?? [] | ||
| ).some((s) => resolveSchemaRef(s, context).schema.type === 'array') || | ||
| ( | ||
| (schemaObject.allOf as | ||
| | (OpenApiSchemaObject | OpenApiReferenceObject)[] | ||
| | undefined) ?? [] | ||
| ).some((s) => resolveSchemaRef(s, context).schema.type === 'array'); | ||
|
|
||
| return ( | ||
| parameterObject.in === 'query' && isArrayLike && parameterObject.explode | ||
| ); | ||
| }); | ||
| const explodeParameters = parameterObjects.filter((parameterObject) => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the significance of extracting this function? It just looks like the code has been moved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| shouldExplodeArrayQueryParameter(parameterObject, context), | ||
| ); | ||
|
|
||
| const explodeParametersNames = explodeParameters.map( | ||
| (parameter) => parameter.name, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unlikely that the default value will be reached. I thought a ternary operator would suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'.