feat: add JSON body mode for engine API endpoints#294
Conversation
ce981cb to
63e0309
Compare
There was a problem hiding this comment.
Pull request overview
Fixes OpenAPIHono’s runtime JSON-body validation so it doesn’t eagerly parse/validate JSON when requests are sent with non-JSON Content-Type (e.g. NDJSON), while restoring strict JSON schemas in route definitions for accurate OpenAPI generation and typed c.req.valid('json').
Changes:
- Unwrap optional/nullable Zod wrappers when extracting header param content-type metadata and descriptions for spec generation.
- Introduce a content-type-guarded JSON validator middleware and use it for
application/jsonrequest bodies inOpenAPIHono. - Add tests covering JSON vs NDJSON vs missing
Content-Typebehavior and spec generation for multi-content request bodies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/hono-zod-openapi/src/index.ts | Adds guarded JSON validation and improves optional/nullable header schema introspection for OpenAPI/spec metadata. |
| packages/hono-zod-openapi/src/tests/json-content-header.test.ts | Adds regression tests for content-type-aware JSON body validation and multi-content OpenAPI output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function contentTypeGuardedJsonValidator( | ||
| schema: AnyZod, | ||
| hook?: DefaultHook | ||
| ): MiddlewareHandler { | ||
| const jsonValidator = zValidator('json', schema, hook as never) | ||
| return async (c, next) => { | ||
| if (c.req.header('content-type')?.includes('application/json')) { |
There was a problem hiding this comment.
Content-Type detection uses .includes('application/json'), which is case-sensitive and can produce false positives (e.g. application/json-seq would match and re-introduce the parsing/validation failure for streaming JSON types). Consider normalizing and parsing the media type (e.g. split on ;, trim, lowercase) and then doing an exact equality check against application/json.
| function contentTypeGuardedJsonValidator( | |
| schema: AnyZod, | |
| hook?: DefaultHook | |
| ): MiddlewareHandler { | |
| const jsonValidator = zValidator('json', schema, hook as never) | |
| return async (c, next) => { | |
| if (c.req.header('content-type')?.includes('application/json')) { | |
| function isApplicationJsonContentType(contentType?: string): boolean { | |
| const mediaType = contentType?.split(';', 1)[0]?.trim().toLowerCase() | |
| return mediaType === 'application/json' | |
| } | |
| function contentTypeGuardedJsonValidator( | |
| schema: AnyZod, | |
| hook?: DefaultHook | |
| ): MiddlewareHandler { | |
| const jsonValidator = zValidator('json', schema, hook as never) | |
| return async (c, next) => { | |
| if (isApplicationJsonContentType(c.req.header('content-type'))) { |
There was a problem hiding this comment.
Fixed — middleware now uses isApplicationJsonContentType() which normalizes (lowercase, strip params) and does exact === 'application/json' match.
| const ct = c.req.header('content-type') | ||
| if (ct?.includes('application/json')) { | ||
| const body = c.req.valid('json') | ||
| return c.json({ mode: 'json', pipeline: body.pipeline }, 200) |
There was a problem hiding this comment.
The test route handler uses ct?.includes('application/json'), which has the same false-positive/case-sensitivity issues as the middleware change. Align this with the production Content-Type parsing logic (exact media type match) and add a regression assertion for a streaming JSON-ish type like application/json-seq (should not trigger JSON body validation).
There was a problem hiding this comment.
Fixed — test handler now calls the same isApplicationJsonContentType() helper, and we have explicit regression tests for application/json-seq and Application/JSON; charset=utf-8.
| // Crucially, skip JSON body parsing entirely when the request's Content-Type | ||
| // is not application/json, so NDJSON/header-only requests aren't affected. | ||
| const jsonSchema = op.requestBody?.content?.['application/json']?.schema | ||
| if (jsonSchema instanceof Object && 'parse' in (jsonSchema as object)) { | ||
| middlewares.push(zValidator('json', jsonSchema as AnyZod, this._defaultHook as never)) | ||
| middlewares.push( | ||
| contentTypeGuardedJsonValidator(jsonSchema as AnyZod, this._defaultHook) | ||
| ) |
There was a problem hiding this comment.
Applying the content-type guard unconditionally changes behavior for JSON-only endpoints: requests with a missing/incorrect Content-Type will now skip validation entirely, but many handlers in this repo call c.req.valid('json') unconditionally (expecting the validator to have run). Consider only using the guarded validator when the route actually supports additional non-JSON content types, and otherwise keep strict JSON validation (or explicitly return 415/400 when a non-JSON Content-Type is sent to a JSON-only route).
There was a problem hiding this comment.
Fixed — we now split into strictJsonBodyValidator (JSON-only routes) vs contentTypeGuardedJsonValidator (mixed-content / header-alternative routes). The choice is made at route registration time based on hasNonJsonRequestBody || hasJsonHeaderAlternatives.
63e0309 to
ce981cb
Compare
Reintroduce application/json config bodies for the engine API while keeping legacy header and NDJSON flows working. Fix OpenAPIHono and the OpenAPI-based CLI generation so typed header schemas, streaming endpoints, and connector loading all continue to work with the new spec. Made-with: Cursor Committed-By-Agent: cursor
ce981cb to
29cb2c7
Compare
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Move JSON-vs-header requiredness into shared helpers so handlers stop merging two conditional code paths into nullable locals. Made-with: Cursor Committed-By-Agent: cursor
Keep JSON validation strict for JSON routes while avoiding false positives for NDJSON and JSON-header alternatives. Made-with: Cursor Committed-By-Agent: cursor
| const hookResult = await hook({ data: value, ...result, target: 'json' }, c) | ||
| if (hookResult) { | ||
| if (hookResult instanceof Response) return hookResult | ||
| if (typeof hookResult === 'object' && hookResult !== null && 'response' in hookResult) { |
There was a problem hiding this comment.
This is intentional — typeof null === 'object' in JS, so the !== null guard is necessary before the 'response' in hookResult check. Not a real type error.
Export isApplicationJsonContentType from hono-zod-openapi and reuse it in app.ts so handler and middleware agree on what counts as JSON body mode. Fixes mixed-case and json-seq false positives/negatives. Made-with: Cursor Committed-By-Agent: cursor
Made-with: Cursor Committed-By-Agent: cursor # Conflicts: # apps/engine/src/api/app.ts
Summary
application/jsonrequest mode for engine POST routes so callers can send typed config in the body (pipeline, optionalstate, optional inputbody, andsourcefor discover) while preserving the existing header and NDJSON flows.sync-hono-zod-openapito validate JSON bodies only for exactapplication/jsonrequests, keep JSON-only routes strict, and preserve correct OpenAPI output for JSON-content headers and mixed request modes.sync-ts-cliand regenerate the engine OpenAPI artifacts so header-based CLI usage keeps working when routes also expose JSON body alternatives.Test plan
pnpm --filter @stripe/sync-hono-zod-openapi buildpnpm --filter @stripe/sync-hono-zod-openapi testpnpm --filter sync-engine exec vitest run src/api/app.test.ts src/lib/remote-engine.test.ts