feat(openapi): accept optional path params in the input schema - #1983
Conversation
The route only matches when the segment is present, so the generator documents the param as required instead of throwing when the schema marks it optional.
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/bun
@orpc/client
@orpc/cloudflare
@orpc/contract
@orpc/experimental-effect
@orpc/evlog
@orpc/hibernation
@orpc/json-schema
@orpc/experimental-msw
@orpc/nest
@orpc/next
@orpc/node
@orpc/openapi
@orpc/opentelemetry
@orpc/pinia-colada
@orpc/pino
@orpc/publisher
@orpc/ratelimit
@orpc/server
@orpc/shared
@orpc/swr
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/zod
commit: |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestion only — the change is correct, consistent with the runtime, and well tested.
Reviewed changes
This run reviewed the full PR: relaxing the OpenAPI generator so an optional path-param field in the input schema no longer throws, with the parameter documented as required: true.
renderPathParameters(packages/openapi/src/openapi-generator-operation.ts) — removed the "optional in the input schema"OpenAPIGeneratorError; the parameter was already emitted with a hardcodedrequired: true, matching both the runtime invariant (a{param}route only matches when the segment is present) and the OpenAPI 3.x mandate that path params be required.- Unit tests (
openapi-generator-operation.test.ts) — added compact and detailed cases asserting the optional param renders as required; removed the now-deadthrows whencase. Both new tests fail on the pre-change code, so they pin the behavior. - e2e test (
crud.test.ts) — added a zod.optional()path-param case and rewrote the aggregated-error test to keep exercising the "missing from the input schema" throw. - Docs (
routing.mdx) — dropped the "must be required" guidance in the path-parameters and prefixes sections.
I traced the surrounding surfaces to confirm consistency: the client codec (openapi-link-codec.ts) already rejects missing/empty path-param values at encode time ("Path param ... cannot be empty"), so the documented required: true matches actual runtime behavior; union inputs where the param is required only in some branches were previously rejected and now generate correctly; and no stale references to the removed error string remain in the repo. Tests pass and lint is clean.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…optional path params Exercises the handler, the link, and the generator for the pattern requested in middleapi#513.
There was a problem hiding this comment.
ℹ️ No issues in this delta — the new e2e test is well-constructed and genuinely pins the behavior, so no Fixing needed. Approval is withheld only because the one minor doc note from the prior review remains unaddressed.
Reviewed changes
This run covered the delta since the prior Pullfrog review (b0908b2 → 80880d3, one commit, one new file).
- Added
tests/openapi/reused-procedure-routes.test.ts— end-to-end coverage for #513: one procedure exposed under three routes where each route promotes a different optional field (tasting/user) to a path param. Exercises the handler (routing + coercion, e.g./users/7/comments→user: 7), the link (GET /tastings/42/comments,GET /users/me/comments?cursor=2, and the "Path param ... cannot be empty" rejection when the optional field is omitted), and the generator (each route documentspath:{param}!as required, correct per-route query set, tags merged['admin','tastings'],operationIdtastings.listComments).
The generator assertions fail on the pre-change code (which threw OpenAPIGeneratorError for optional path-param fields), so this test genuinely pins the fix rather than rubber-stamping it. Assertions match what the prior review confirmed about the runtime codec and the always-required rendering.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The one open thread from the prior review is now addressed, so the doc-only delta is good to merge.
Reviewed changes
This run reviewed the delta since the prior Pullfrog review (80880d3 → f759084, one commit).
- Added a docs note in
routing.mdx— "Path parameters are always documented as required, even when the schema field is optional, because the route only matches when the segment is present" — exactly the note the prior review suggested, so users writingz.string().optional()won't be surprised by their generated document. - Reworded the source comment in
openapi-generator-operation.ts:224to the single-line form; confirmed the removedentry[2]check leaves no dead/unused references (entry[1]still feeds the schema).
The documentation now matches the generator behavior (required: true emitted unconditionally) and the prior thread is resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
1 participant
The OpenAPI generator no longer throws when a dynamic path param is optional in the input schema. The router only reaches a procedure after the path matched, so the param is always present at runtime, and OpenAPI mandates
required: trueon path parameters anyway. The generator now emits that unconditionally instead of refusing to produce a document.This also removes an inconsistency: detailed mode already tolerated an optional
paramssection, and union inputs where only some branches carried the param were rejected even though the route physically requires the segment.Behavior
required: true.Testing
.optional()verifies the generated parameter.