From 4252aed8d7d1d00bc70565c93e67687431b7a8b9 Mon Sep 17 00:00:00 2001 From: ngDuyAnh <62583862+ngDuyAnh@users.noreply.github.com> Date: Mon, 8 Jun 2026 09:05:39 -0500 Subject: [PATCH] Preserve declared arg order when defaulted args precede a required arg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After #1756 surfaced `requestParamsOptional` so the extracted query argument gets marked optional whenever all its fields are optional, the existing "sort by optionality" step in procedure-call.ejs began hoisting the optional query argument *past* a required body argument. The signature flipped from `(query, body, params)` to `(body, query, params)` for endpoints with a required requestBody and a non-required query parameter, silently swapping callers' positional arguments. Fix: before sorting, promote any optional arg that has a defaultValue and is followed by a required arg back to "positionally required". argToTmpl already emits `name: T = default` (no `?:`) for defaulted args, which is TS-legal before a required arg, so the user-facing type stays the same while declaration order is preserved. Adds a regression test (tests/spec/optional-query-required-body) that fails on baseline and passes with the fix. Updates two existing snapshots that were capturing the bug — both `signRequest` cases now correctly read `(query: SignRequestParams = {}, body: Claims, params: RequestParams = {})`. --- templates/default/procedure-call.ejs | 14 +- templates/modular/procedure-call.ejs | 14 +- tests/__snapshots__/extended.test.ts.snap | 2 +- .../__snapshots__/basic.test.ts.snap | 2 +- .../__snapshots__/basic.test.ts.snap | 311 ++++++++++++++++++ .../basic.test.ts | 57 ++++ .../optional-query-required-body/schema.json | 37 +++ 7 files changed, 433 insertions(+), 4 deletions(-) create mode 100644 tests/spec/optional-query-required-body/__snapshots__/basic.test.ts.snap create mode 100644 tests/spec/optional-query-required-body/basic.test.ts create mode 100644 tests/spec/optional-query-required-body/schema.json diff --git a/templates/default/procedure-call.ejs b/templates/default/procedure-call.ejs index 5c116e01..6ccee0c1 100644 --- a/templates/default/procedure-call.ejs +++ b/templates/default/procedure-call.ejs @@ -46,9 +46,21 @@ const rawWrapperArgs = config.extractRequestParams ? requestConfigParam, ]) +// Before sorting, promote any optional arg that has a defaultValue and is +// followed by a required arg to "positionally required". The sort still pushes +// truly-optional (`?:`) args to the end, but defaultable args ahead of a +// required arg stay in declaration order, so callers' positional arguments +// don't silently shift when a previously-required query becomes optional. +const positionedWrapperArgs = rawWrapperArgs.map((arg, i) => { + if (arg.optional && arg.defaultValue && rawWrapperArgs.slice(i + 1).some(a => !a.optional)) { + return { ...arg, optional: false }; + } + return arg; +}) + const wrapperArgs = _ // Sort by optionality - .sortBy(rawWrapperArgs, [o => o.optional]) + .sortBy(positionedWrapperArgs, [o => o.optional]) .map(argToTmpl) .join(', ') diff --git a/templates/modular/procedure-call.ejs b/templates/modular/procedure-call.ejs index 83b3f179..0d3ad8ec 100644 --- a/templates/modular/procedure-call.ejs +++ b/templates/modular/procedure-call.ejs @@ -46,9 +46,21 @@ const rawWrapperArgs = config.extractRequestParams ? requestConfigParam, ]) +// Before sorting, promote any optional arg that has a defaultValue and is +// followed by a required arg to "positionally required". The sort still pushes +// truly-optional (`?:`) args to the end, but defaultable args ahead of a +// required arg stay in declaration order, so callers' positional arguments +// don't silently shift when a previously-required query becomes optional. +const positionedWrapperArgs = rawWrapperArgs.map((arg, i) => { + if (arg.optional && arg.defaultValue && rawWrapperArgs.slice(i + 1).some(a => !a.optional)) { + return { ...arg, optional: false }; + } + return arg; +}) + const wrapperArgs = _ // Sort by optionality - .sortBy(rawWrapperArgs, [o => o.optional]) + .sortBy(positionedWrapperArgs, [o => o.optional]) .map(argToTmpl) .join(', ') diff --git a/tests/__snapshots__/extended.test.ts.snap b/tests/__snapshots__/extended.test.ts.snap index a931f3cc..90de7fad 100644 --- a/tests/__snapshots__/extended.test.ts.snap +++ b/tests/__snapshots__/extended.test.ts.snap @@ -9545,8 +9545,8 @@ export class Api< * @request POST:/scope */ signRequest: ( - body: Claims, query: SignRequestParams = {}, + body: Claims, params: RequestParams = {}, ) => this.request({ diff --git a/tests/spec/extractRequestParams/__snapshots__/basic.test.ts.snap b/tests/spec/extractRequestParams/__snapshots__/basic.test.ts.snap index 40b6cb8d..328691f4 100644 --- a/tests/spec/extractRequestParams/__snapshots__/basic.test.ts.snap +++ b/tests/spec/extractRequestParams/__snapshots__/basic.test.ts.snap @@ -686,8 +686,8 @@ export class Api< * @request POST:/scope */ signRequest: ( - body: Claims, query: SignRequestParams = {}, + body: Claims, params: RequestParams = {}, ) => this.request< diff --git a/tests/spec/optional-query-required-body/__snapshots__/basic.test.ts.snap b/tests/spec/optional-query-required-body/__snapshots__/basic.test.ts.snap new file mode 100644 index 00000000..4ad7bb47 --- /dev/null +++ b/tests/spec/optional-query-required-body/__snapshots__/basic.test.ts.snap @@ -0,0 +1,311 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`optional-query-required-body > with --extract-request-params, query stays before body in generated signature 1`] = ` +"/* eslint-disable */ +/* tslint:disable */ +// @ts-nocheck +/* + * --------------------------------------------------------------- + * ## THIS FILE WAS GENERATED VIA SWAGGER-TYPESCRIPT-API ## + * ## ## + * ## AUTHOR: acacode ## + * ## SOURCE: https://github.com/acacode/swagger-typescript-api ## + * --------------------------------------------------------------- + */ + +export interface CheckImpactPayload { + staff_id: number; + action: string; +} + +export interface CheckImpactParams { + cadence?: "weekly" | "biweekly"; +} + +export type QueryParamsType = Record; +export type ResponseFormat = keyof Omit; + +export interface FullRequestParams extends Omit { + /** set parameter to \`true\` for call \`securityWorker\` for this request */ + secure?: boolean; + /** request path */ + path: string; + /** content type of request body */ + type?: ContentType; + /** query params */ + query?: QueryParamsType; + /** format of response (i.e. response.json() -> format: "json") */ + format?: ResponseFormat; + /** request body */ + body?: unknown; + /** base url */ + baseUrl?: string; + /** request cancellation token */ + cancelToken?: CancelToken; +} + +export type RequestParams = Omit< + FullRequestParams, + "body" | "method" | "query" | "path" +>; + +export interface ApiConfig { + baseUrl?: string; + baseApiParams?: Omit; + securityWorker?: ( + securityData: SecurityDataType | null, + ) => Promise | RequestParams | void; + customFetch?: typeof fetch; +} + +export interface HttpResponse + extends Response { + data: D; + error: E; +} + +type CancelToken = Symbol | string | number; + +export enum ContentType { + Json = "application/json", + JsonApi = "application/vnd.api+json", + FormData = "multipart/form-data", + UrlEncoded = "application/x-www-form-urlencoded", + Text = "text/plain", +} + +export class HttpClient { + public baseUrl: string = ""; + private securityData: SecurityDataType | null = null; + private securityWorker?: ApiConfig["securityWorker"]; + private abortControllers = new Map(); + private customFetch = (...fetchParams: Parameters) => + fetch(...fetchParams); + + private baseApiParams: RequestParams = { + credentials: "same-origin", + headers: {}, + redirect: "follow", + referrerPolicy: "no-referrer", + }; + + constructor(apiConfig: ApiConfig = {}) { + Object.assign(this, apiConfig); + } + + public setSecurityData = (data: SecurityDataType | null) => { + this.securityData = data; + }; + + protected encodeQueryParam(key: string, value: any) { + const encodedKey = encodeURIComponent(key); + return \`\${encodedKey}=\${encodeURIComponent(typeof value === "number" ? value : \`\${value}\`)}\`; + } + + protected addQueryParam(query: QueryParamsType, key: string) { + return this.encodeQueryParam(key, query[key]); + } + + protected addArrayQueryParam(query: QueryParamsType, key: string) { + const value = query[key]; + return value.map((v: any) => this.encodeQueryParam(key, v)).join("&"); + } + + protected toQueryString(rawQuery?: QueryParamsType): string { + const query = rawQuery || {}; + const keys = Object.keys(query).filter( + (key) => "undefined" !== typeof query[key], + ); + return keys + .map((key) => + Array.isArray(query[key]) + ? this.addArrayQueryParam(query, key) + : this.addQueryParam(query, key), + ) + .join("&"); + } + + protected addQueryParams(rawQuery?: QueryParamsType): string { + const queryString = this.toQueryString(rawQuery); + return queryString ? \`?\${queryString}\` : ""; + } + + private contentFormatters: Record any> = { + [ContentType.Json]: (input: any) => + input !== null && (typeof input === "object" || typeof input === "string") + ? JSON.stringify(input) + : input, + [ContentType.JsonApi]: (input: any) => + input !== null && (typeof input === "object" || typeof input === "string") + ? JSON.stringify(input) + : input, + [ContentType.Text]: (input: any) => + input !== null && typeof input !== "string" + ? JSON.stringify(input) + : input, + [ContentType.FormData]: (input: any) => { + if (input instanceof FormData) { + return input; + } + + return Object.keys(input || {}).reduce((formData, key) => { + const property = input[key]; + formData.append( + key, + property instanceof Blob + ? property + : typeof property === "object" && property !== null + ? JSON.stringify(property) + : \`\${property}\`, + ); + return formData; + }, new FormData()); + }, + [ContentType.UrlEncoded]: (input: any) => this.toQueryString(input), + }; + + protected mergeRequestParams( + params1: RequestParams, + params2?: RequestParams, + ): RequestParams { + return { + ...this.baseApiParams, + ...params1, + ...(params2 || {}), + headers: { + ...(this.baseApiParams.headers || {}), + ...(params1.headers || {}), + ...((params2 && params2.headers) || {}), + }, + }; + } + + protected createAbortSignal = ( + cancelToken: CancelToken, + ): AbortSignal | undefined => { + if (this.abortControllers.has(cancelToken)) { + const abortController = this.abortControllers.get(cancelToken); + if (abortController) { + return abortController.signal; + } + return void 0; + } + + const abortController = new AbortController(); + this.abortControllers.set(cancelToken, abortController); + return abortController.signal; + }; + + public abortRequest = (cancelToken: CancelToken) => { + const abortController = this.abortControllers.get(cancelToken); + + if (abortController) { + abortController.abort(); + this.abortControllers.delete(cancelToken); + } + }; + + public request = async ({ + body, + secure, + path, + type, + query, + format, + baseUrl, + cancelToken, + ...params + }: FullRequestParams): Promise> => { + const secureParams = + ((typeof secure === "boolean" ? secure : this.baseApiParams.secure) && + this.securityWorker && + (await this.securityWorker(this.securityData))) || + {}; + const requestParams = this.mergeRequestParams(params, secureParams); + const queryString = query && this.toQueryString(query); + const payloadFormatter = this.contentFormatters[type || ContentType.Json]; + const responseFormat = format || requestParams.format; + + return this.customFetch( + \`\${baseUrl || this.baseUrl || ""}\${path}\${queryString ? \`?\${queryString}\` : ""}\`, + { + ...requestParams, + headers: { + ...(requestParams.headers || {}), + ...(type && type !== ContentType.FormData + ? { "Content-Type": type } + : {}), + }, + signal: + (cancelToken + ? this.createAbortSignal(cancelToken) + : requestParams.signal) || null, + body: + typeof body === "undefined" || body === null + ? null + : payloadFormatter(body), + }, + ).then(async (response) => { + const r = response as HttpResponse; + r.data = null as unknown as T; + r.error = null as unknown as E; + + const responseToParse = responseFormat ? response.clone() : response; + const data = !responseFormat + ? r + : await responseToParse[responseFormat]() + .then((data) => { + if (r.ok) { + r.data = data; + } else { + r.error = data; + } + return r; + }) + .catch((e) => { + r.error = e; + return r; + }); + + if (cancelToken) { + this.abortControllers.delete(cancelToken); + } + + if (!response.ok) throw data; + return data; + }); + }; +} + +/** + * @title optional-query-required-body + * @version 1.0.0 + */ +export class Api< + SecurityDataType extends unknown, +> extends HttpClient { + checkImpact = { + /** + * No description + * + * @name CheckImpact + * @summary Reproduces issue #1755 follow-up: optional query + required body must keep (query, body) argument order in the generated wrapper. + * @request POST:/check-impact + */ + checkImpact: ( + query: CheckImpactParams = {}, + data: CheckImpactPayload, + params: RequestParams = {}, + ) => + this.request({ + path: \`/check-impact\`, + method: "POST", + query: query, + body: data, + type: ContentType.Json, + ...params, + }), + }; +} +" +`; diff --git a/tests/spec/optional-query-required-body/basic.test.ts b/tests/spec/optional-query-required-body/basic.test.ts new file mode 100644 index 00000000..6519dd4d --- /dev/null +++ b/tests/spec/optional-query-required-body/basic.test.ts @@ -0,0 +1,57 @@ +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { generateApi } from "../../../src/index.js"; + +describe("optional-query-required-body", async () => { + let tmpdir = ""; + + beforeAll(async () => { + tmpdir = await fs.mkdtemp(path.join(os.tmpdir(), "swagger-typescript-api")); + }); + + afterAll(async () => { + await fs.rm(tmpdir, { recursive: true }); + }); + + test("with --extract-request-params, query stays before body in generated signature", async () => { + await generateApi({ + fileName: "schema", + input: path.resolve(import.meta.dirname, "schema.json"), + output: tmpdir, + silent: true, + extractRequestParams: true, + extractRequestBody: true, + }); + + const content = await fs.readFile(path.join(tmpdir, "schema.ts"), { + encoding: "utf8", + }); + + // The signature must read (query, body, params) — NOT (body, query, params). + // The optional query arg has a defaultValue ({}) and must keep its + // declaration position even though it is sorted after required args. + // Default template nests operation under a namespace object, so the call + // arrow appears as `: (args) =>` rather than `= (args) =>`. + const sigMatch = content.match( + /checkImpact:\s*\(\s*([\s\S]*?)\s*\)\s*=>\s*this\.request/, + ); + expect(sigMatch, "checkImpact arrow not found in schema.ts").not.toBeNull(); + const signature = sigMatch![1]; + + const queryPos = signature.indexOf("query"); + const dataPos = signature.search(/\bdata\b/); + expect(queryPos).toBeGreaterThanOrEqual(0); + expect(dataPos).toBeGreaterThan(0); + expect(queryPos).toBeLessThan(dataPos); + + // The optional query arg must keep a defaultValue (`= {}`) so callers can + // omit it while it stays in its declared position before the required body. + expect(signature).toMatch(/query:\s*CheckImpactParams\s*=\s*\{\}/); + + // Lock the entire generated file so future template changes can't silently + // regress argument ordering. + expect(content).toMatchSnapshot(); + }); +}); diff --git a/tests/spec/optional-query-required-body/schema.json b/tests/spec/optional-query-required-body/schema.json new file mode 100644 index 00000000..4a525b6c --- /dev/null +++ b/tests/spec/optional-query-required-body/schema.json @@ -0,0 +1,37 @@ +{ + "openapi": "3.0.0", + "info": { "title": "optional-query-required-body", "version": "1.0.0" }, + "paths": { + "/check-impact": { + "post": { + "operationId": "checkImpact", + "summary": "Reproduces issue #1755 follow-up: optional query + required body must keep (query, body) argument order in the generated wrapper.", + "parameters": [ + { + "in": "query", + "name": "cadence", + "schema": { "type": "string", "enum": ["weekly", "biweekly"] } + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["staff_id", "action"], + "properties": { + "staff_id": { "type": "integer" }, + "action": { "type": "string" } + } + } + } + } + }, + "responses": { + "200": { "description": "OK" } + } + } + } + } +}