fix(openapi-server): handle explode:true array query params in hono/express routers - #407
Merged
Merged
Conversation
…xpress routers The hono and express router emitters had no branch for explode:true array query params (type: array with the default explode). Such a param fell through to z.string() in the emitted Zod schema, and extraction read a single scalar value, while the generated service method expects an array type (number[] / boolean[] / string[]). This produced a TS2322 type mismatch in the generated output. The Fastify emitter already handled this correctly (#375/#348); this closes the same gap for hono and express. router.ts changes: - queryParamZodExpr: new isArray branch emits z.array(<itemExpr>), mirroring the Fastify emitter's item coercion (number -> z.coerce.number(), boolean -> z.boolean(), else z.string()). - Hono extraction: c.req.queries(name) collects every repeated key into string[] | undefined, then items are coerced (.map(Number) / .map(v => v === 'true')) to match the Zod validation. - Express extraction: qs normalizes a repeated key to string[], but a single occurrence stays a bare string and an absent key is undefined. A small shared _toQueryArray(...) helper (emitted once per file, only when needed) normalizes all three cases before the same item coercion. Adds a typechecked guard: compat-matrix.test.ts only asserts generation "does not throw", never that the output actually compiles, which is why this slipped through. array-query-typecheck.test.ts feeds the real generated service.ts + router.ts through the TypeScript compiler API for an explode:true integer array query param, across hono, express, and fastify (as a control), plus a sanity check proving the harness itself catches a genuine type mismatch. Requires real zod/hono/express/fastify type declarations on disk, so those are added as devDependencies here. hono-express-array-query.test.ts adds string-level regression tests for the emitted Zod expressions and extraction snippets across integer/ boolean/string array items and required/optional variants. Closes #377
📝 WalkthroughWalkthroughThis PR fixes explode:true array query parameter handling in generated Hono and Express routers, adding array-aware Zod schema generation and repeated-key extraction to match the existing Fastify behavior. It includes new TypeScript compile-based regression tests, test compilation helpers, formatting cleanups in shared.ts, and updated dev dependencies/config for test tooling. ChangesArray Query Param Fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HonoRouter
participant ExpressRouter
participant ZodSchema
participant Service
Client->>HonoRouter: GET /items?ids=1&ids=2
HonoRouter->>HonoRouter: honoArrayQueryExpr uses c.req.queries("ids")
HonoRouter->>ZodSchema: validate z.array(itemExpr)
ZodSchema-->>HonoRouter: number[] ids
HonoRouter->>Service: listItems({ ids })
Client->>ExpressRouter: GET /items?ids=1&ids=2
ExpressRouter->>ExpressRouter: expressArrayQueryExpr calls _toQueryArray(req.query["ids"])
ExpressRouter->>ZodSchema: validate z.array(itemExpr)
ZodSchema-->>ExpressRouter: number[] ids
ExpressRouter->>Service: listItems({ ids })
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Fallow audit reportFound 3 findings. Details
Generated by fallow. |
Contributor
Fallow audit report0 inline findings selected for GitHub review. |
![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
🧹 Nitpick comments (2)
packages/openapi-server/src/plugins/router.ts (1)
310-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between Hono/Express array extraction builders.
honoArrayQueryExprandexpressArrayQueryExprshare identical item-coercion suffix logic (?.map(Number)/?.map((v) => v === 'true')), differing only in the base expression. Could be factored into a shared helper taking the base expression string, reducing duplication if a third item type is added later.♻️ Optional refactor: extract shared coercion suffix
+function withArrayItemCoercion(base: string, itemsTsType: string | undefined): string { + if (itemsTsType === 'number') return `${base}?.map(Number)` + if (itemsTsType === 'boolean') return `${base}?.map((v) => v === 'true')` + return base +} + function honoArrayQueryExpr(q: QueryParam): string { - const base = `c.req.queries('${q.rawName}')` - if (q.itemsTsType === 'number') return `${base}?.map(Number)` - if (q.itemsTsType === 'boolean') return `${base}?.map((v) => v === 'true')` - return base + return withArrayItemCoercion(`c.req.queries('${q.rawName}')`, q.itemsTsType) } function expressArrayQueryExpr(q: QueryParam): string { - const base = `_toQueryArray(req.query['${q.rawName}'] as string | string[] | undefined)` - if (q.itemsTsType === 'number') return `${base}?.map(Number)` - if (q.itemsTsType === 'boolean') return `${base}?.map((v) => v === 'true')` - return base + return withArrayItemCoercion( + `_toQueryArray(req.query['${q.rawName}'] as string | string[] | undefined)`, + q.itemsTsType + ) }Correctness-wise this segment is solid: boolean items are pre-coerced to real booleans (matching
z.boolean(), not.coerce), and number items pre-coerced viaNumber()combined withz.coerce.number()on the Zod side correctly rejects invalid input (Zod rejectsNaNforz.number()/z.coerce.number()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openapi-server/src/plugins/router.ts` around lines 310 - 336, The Hono and Express array query extraction helpers duplicate the same item-coercion logic, so factor the shared `?.map(Number)` / `?.map((v) => v === 'true')` suffix into a common helper and keep `honoArrayQueryExpr` and `expressArrayQueryExpr` focused on only building their base query expression. Use the existing symbols `honoArrayQueryExpr`, `expressArrayQueryExpr`, and `QueryParam` to introduce a small shared helper that takes the base expression string plus `itemsTsType`, then have both functions delegate to it to reduce duplication and make future item-type additions easier.packages/openapi-server/src/__tests__/array-query-typecheck.test.ts (1)
48-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parameterizing the three near-identical
itblocks.Hono/Express/Fastify cases repeat the same generate → compile → assert pattern with only the generator functions differing. A small
it.eachtable would reduce duplication, though the current form is readable and low-risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openapi-server/src/__tests__/array-query-typecheck.test.ts` around lines 48 - 80, The three test cases in array-query-typecheck.test.ts duplicate the same generate-compile-assert flow for Hono, Express, and Fastify. Refactor the repeated `it` blocks into a parameterized table-driven test (for example, using a single shared helper or `it.each`) that calls the appropriate generators like `generateService`, `generateRouter`, `generateExpressRouter`, `generateFastifyTypedService`, and `generateFastifyRouter`, while preserving the existing compile and `assertNoTsDiagnostics` checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/openapi-server/src/__tests__/ts-compile-helpers.ts`:
- Around line 42-53: The TypeScript compiler options parsing in
ts-compile-helpers is ignoring the errors returned by
ts.convertCompilerOptionsFromJson, so invalid hardcoded options can be silently
accepted. Update the helper that builds compiler options to capture both options
and errors, and make it fail or report when errors are present instead of
proceeding with only options. Keep the change localized to the
ts.convertCompilerOptionsFromJson call site so any bad config in the test helper
surfaces immediately.
---
Nitpick comments:
In `@packages/openapi-server/src/__tests__/array-query-typecheck.test.ts`:
- Around line 48-80: The three test cases in array-query-typecheck.test.ts
duplicate the same generate-compile-assert flow for Hono, Express, and Fastify.
Refactor the repeated `it` blocks into a parameterized table-driven test (for
example, using a single shared helper or `it.each`) that calls the appropriate
generators like `generateService`, `generateRouter`, `generateExpressRouter`,
`generateFastifyTypedService`, and `generateFastifyRouter`, while preserving the
existing compile and `assertNoTsDiagnostics` checks.
In `@packages/openapi-server/src/plugins/router.ts`:
- Around line 310-336: The Hono and Express array query extraction helpers
duplicate the same item-coercion logic, so factor the shared `?.map(Number)` /
`?.map((v) => v === 'true')` suffix into a common helper and keep
`honoArrayQueryExpr` and `expressArrayQueryExpr` focused on only building their
base query expression. Use the existing symbols `honoArrayQueryExpr`,
`expressArrayQueryExpr`, and `QueryParam` to introduce a small shared helper
that takes the base expression string plus `itemsTsType`, then have both
functions delegate to it to reduce duplication and make future item-type
additions easier.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 997460c9-0840-4cd5-abbc-06e84b73b2d2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.fallowrc.json.gitignorepackages/openapi-server/package.jsonpackages/openapi-server/src/__tests__/array-query-typecheck.test.tspackages/openapi-server/src/__tests__/hono-express-array-query.test.tspackages/openapi-server/src/__tests__/ts-compile-helpers.tspackages/openapi-server/src/plugins/router.tspackages/openapi-server/src/plugins/shared.ts
Comment on lines
+42
to
+53
| const { options } = ts.convertCompilerOptionsFromJson( | ||
| { | ||
| strict: true, | ||
| target: 'ES2022', | ||
| module: 'ESNext', | ||
| moduleResolution: 'Bundler', | ||
| noEmit: true, | ||
| skipLibCheck: true, | ||
| lib: ['ES2022', 'DOM'], | ||
| }, | ||
| dir | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compiler-options parsing errors are silently discarded.
ts.convertCompilerOptionsFromJson returns { options, errors }; only options is destructured, so a typo or invalid option value in the hardcoded config would be swallowed instead of surfacing, potentially causing the compile to silently run with unintended defaults.
🛡️ Proposed fix
- const { options } = ts.convertCompilerOptionsFromJson(
+ const { options, errors } = ts.convertCompilerOptionsFromJson(
{
strict: true,
target: 'ES2022',
module: 'ESNext',
moduleResolution: 'Bundler',
noEmit: true,
skipLibCheck: true,
lib: ['ES2022', 'DOM'],
},
dir
)
+ if (errors.length > 0) {
+ throw new Error(`Invalid compiler options: ${errors.map((e) => e.messageText).join('\n')}`)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { options } = ts.convertCompilerOptionsFromJson( | |
| { | |
| strict: true, | |
| target: 'ES2022', | |
| module: 'ESNext', | |
| moduleResolution: 'Bundler', | |
| noEmit: true, | |
| skipLibCheck: true, | |
| lib: ['ES2022', 'DOM'], | |
| }, | |
| dir | |
| ) | |
| const { options, errors } = ts.convertCompilerOptionsFromJson( | |
| { | |
| strict: true, | |
| target: 'ES2022', | |
| module: 'ESNext', | |
| moduleResolution: 'Bundler', | |
| noEmit: true, | |
| skipLibCheck: true, | |
| lib: ['ES2022', 'DOM'], | |
| }, | |
| dir | |
| ) | |
| if (errors.length > 0) { | |
| throw new Error(`Invalid compiler options: ${errors.map((e) => e.messageText).join('\n')}`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/openapi-server/src/__tests__/ts-compile-helpers.ts` around lines 42
- 53, The TypeScript compiler options parsing in ts-compile-helpers is ignoring
the errors returned by ts.convertCompilerOptionsFromJson, so invalid hardcoded
options can be silently accepted. Update the helper that builds compiler options
to capture both options and errors, and make it fail or report when errors are
present instead of proceeding with only options. Keep the change localized to
the ts.convertCompilerOptionsFromJson call site so any bad config in the test
helper surfaces immediately.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #377.
The hono and express router emitters had no
explode:truearray branch for query params, so atype: arrayquery param fell through toz.string()in the generated Zod schema and was extracted as a single scalar, while the generated service method expects an array (number[]/boolean[]/string[]). Result: a TS2322 type mismatch in the generated output. #375 fixed only the Fastify target and left hono/express as a known gap (tracked here).The
QueryParamtype already carriedisArray/itemsTsType(the service side was already correct atnumber[]); the router emitters just weren't consuming them.Fix (
router.ts)Added an
isArraybranch toqueryParamZodExpr(mirrors Fastify'squeryParamBaseExprordering) plus aqueryParamItemZodExprhelper, and taught both extraction emitters to collect repeated keys:Hono —
c.req.queries("name")returns all values for a key:Express — qs yields
string[]for a repeated key but a barestringfor a single occurrence, so a small emitted normalizer arrays it first:Numbers are coerced in extraction and validated with
z.coerce.number()(harmless no-op, matches Fastify); booleans are coerced in extraction (=== 'true') and validated withz.boolean(). Delimited (explode:false) and deepObject paths are untouched (mutually exclusive withisArray).Typechecked guard
The issue's root cause was that
compat-matrix.test.tsonly asserts "generates without throwing", not that output typechecks. So this PR adds a guard that runs generatedservice.ts+router.tsthrough the real TypeScript compiler (ts.createProgram) for hono, express and fastify. Proven non-vacuous: stashing the fix reproduces exactly the TS2322 from the bug report for both hono and express.Notes for reviewers
hono,express,@types/express,zodadded toopenapi-serverdevDependencies (and to.fallowrc.jsonignoreDependencies, same bucket as the existingfastify-type-provider-zod) purely so the typecheck guard can resolve real framework type declarations on disk. They are not runtime deps and are only referenced inside generated string templates.shared.ts: the only intended change is a comment update (the old comment claimed hono/express don't handle array query params, now false). The surroundinggetBodyInforeformatting is the repo'spre-commitPrettier hook normalizing pre-existing lines, not a logic change.examples/or the sharedpetstorespec, to avoid a wide blast radius across the e2e/contract packages.Verification
pnpm --filter @codewithagents/openapi-server test→ 16 files, 644 passedtest:matrix(128-spec compat) → 384 passed, no regressionslint(tsc --noEmit) → cleantest:coverage→ 92.4 / 86.9 / 96.7 / 94.8, above the 85/75/88/85 floorspnpm fallow:audit→ clean for the changed filesSummary by CodeRabbit
New Features
Bug Fixes
explode: truequery arrays so they no longer fall back to scalar parsing.Chores