feat(v29): Supporting QUERY method#3479
Merged
Merged
Conversation
|
coverage: 100.0%. remained the same — add-query-method into make-v29 |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds HTTP QUERY method support (RFC 10008) with comprehensive type system integration and a runtime guard in route registration. Concurrently removes async patterns from server lifecycle: ChangesHTTP QUERY Method Support
Synchronous Server and Integration Lifecycle
Documentation Constructor Refactor and OpenAPI 3.2 Migration
Migration Rule Updates for v29 Breaking Changes
Infrastructure, Workflows, Packaging, and Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RobinTail
changed the title
QUERY method supportQUERY method
Owner
Author
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ One minor issue — import convention fix needed.
Reviewed changes — adds HTTP QUERY method support (RFC 10008), making "query" a first-class method on EndpointsFactory, with appropriate default input sources and full test coverage.
- Add
"query"toFamiliarMethod,methods, andclientMethods— forces the method name into the type system despite Express types not yet including it, with@todomarkers for future cleanup. - Add
"query"todefaultInputSources—["query", "body", "params"]with query params at lowest priority, body in the middle, and route params highest. - Add runtime safety check in
initRouting— castsappto include thequerymethod and throwsRoutingErrorif the Express runtime doesn't support it. - Change
RoutingError.causefromMethodtoCORSMethod— needed becauseoptionscan also flow through routing paths. - Update tests, snapshots, and client generation — comprehensive test coverage for method registration, input sources, type assertions, and environment checks.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@express-zod-api/src/routing.ts`:
- Line 13: The import statement for RoutingError from "./errors.ts" includes a
`.ts` extension in the relative import path, which violates the compiled code
guideline that requires imports without file extensions. Remove the `.ts`
extension from the import path so it reads "./errors" instead, ensuring
compliance with the coding guidelines for compiled workspace code.
🪄 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
Run ID: 9da551c1-8068-49a0-be96-3bdf80c81944
⛔ Files ignored due to path filters (3)
express-zod-api/tests/__snapshots__/common-helpers.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/documentation.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/integration.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
CHANGELOG.mdexample/example.client.tsexpress-zod-api/src/common-helpers.tsexpress-zod-api/src/errors.tsexpress-zod-api/src/method.tsexpress-zod-api/src/routing.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/documentation.spec.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/env.spec.tsexpress-zod-api/tests/express-mock.tsexpress-zod-api/tests/integration.spec.tsexpress-zod-api/tests/method.spec.tsexpress-zod-api/tests/routing.spec.ts
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review, excluded "all" from FamiliarMethod so Express's app.all() is correctly filtered out of the supported HTTP method type union.
- Exclude
"all"fromFamiliarMethod—IRouter["all"]registers handlers for all HTTP methods, not a real method itself, so it doesn't belong inFamiliarMethod.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review, the example listUsersEndpoint was converted from GET to QUERY to demonstrate the new method, and all generated artifacts (client types, OpenAPI docs, test) were updated accordingly.
- Switch example endpoint to QUERY —
listUsersEndpointnow usesmethod: "query"instead of"get", with the client types,example.documentation.yaml, and example test regenerated to match.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Owner
Author
|
RobinTail
changed the title
QUERY methodQUERY method
RobinTail
added a commit
that referenced
this pull request
Jun 20, 2026
RobinTail
changed the title
QUERY methodQUERY method
RobinTail
changed the title
QUERY methodQUERY method
RobinTail
force-pushed
the
add-query-method
branch
from
67fce61 to
81204da
Compare
Owner
Author
|
rebased against #3469 |
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — adds HTTP QUERY method support (RFC 10008) across the type system, routing, input resolution, integration, and documentation layers, with comprehensive test coverage.
- Add
"query"toFamiliarMethodand method arrays — with@todomarkers for cleanup when Express types catch up, and exclusion of"all"from the familiar set. - Add
"query"todefaultInputSources—["query", "body", "params"]giving body priority over query params for QUERY requests. - Broaden
RoutingError.causefromMethodtoCORSMethod— needed becauseoptionscan flow through routing. - Add runtime safety check in
initRouting— castsappand throwsRoutingErrorif Express runtime lacksquerysupport, using.bind(app)for correctthiscontext. - Convert example
listUsersEndpointfrom GET to QUERY — regenerated client types, OpenAPI docs, and tests. - Update tests and snapshots — method type tests, routing registration tests, environment check for
http.METHODS, input source resolution, and integration/documentation snapshot matches.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
express-zod-api/src/server.ts (1)
83-93:⚠️ Potential issue | 🟠 MajorGuard against Promise-returning lifecycle hooks.
Lines 83 and 93 currently ignore returned values. In TypeScript,
asynccallbacks can still be assignable to void-return hook types, so rejected Promises can become unhandled and hook side effects can race startup/routing.💡 Proposed fix
+const assertSyncHook = ( + hook: "beforeRouting" | "afterRouting", + result: unknown, +) => { + if (result && typeof (result as PromiseLike<unknown>).then === "function") { + throw new Error( + `${hook} must be synchronous in v29. Move async initialization before createServer().`, + ); + } +}; + export const createServer = (config: ServerConfig, routing: Routing) => { @@ - config.beforeRouting?.({ app, getLogger }); + assertSyncHook("beforeRouting", config.beforeRouting?.({ app, getLogger })); @@ - config.afterRouting?.({ app, getLogger }); + assertSyncHook("afterRouting", config.afterRouting?.({ app, getLogger }));🤖 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 `@express-zod-api/src/server.ts` around lines 83 - 93, The lifecycle hooks config.beforeRouting and config.afterRouting at lines 83 and 93 may return Promises, but their return values are currently ignored, which can lead to unhandled Promise rejections and race conditions with side effects. Capture the return values from both config.beforeRouting and config.afterRouting hook invocations and await them to ensure they complete before proceeding with the next operations, using Promise.resolve to safely handle both synchronous and asynchronous return values.
🧹 Nitpick comments (1)
express-zod-api/src/security.ts (1)
77-80: ⚡ Quick winUse an interface for the new object-shaped flow type
Line 77 introduces an object-shaped type alias. Please switch it to an interface to match the TypeScript guideline used in this repo.
Proposed fix
-type DeviceAuthFlow<S extends string> = DeviceAuthUrl & - TokenUrl & - RefreshUrl & - Scopes<S>; +interface DeviceAuthFlow<S extends string> + extends DeviceAuthUrl, + TokenUrl, + RefreshUrl, + Scopes<S> {}As per coding guidelines,
**/*.ts: “Declare object-based types as interfaces, not type aliases.”🤖 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 `@express-zod-api/src/security.ts` around lines 77 - 80, The DeviceAuthFlow type alias on lines 77-80 uses a type alias to define an object-shaped type, but the repository guidelines require using interfaces instead of type aliases for object-based types. Convert the DeviceAuthFlow type alias to an interface by replacing the type keyword and equals sign with the interface keyword, and adjust the body to list the constituent types as interface extensions using the extends keyword instead of the intersection operator.Source: Coding guidelines
🤖 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 @.github/workflows/oas.yml:
- Around line 3-6: The workflow trigger configuration in the push and
pull_request sections is missing the v29 branch. Add v29 to the branches array
in both the push trigger block and the pull_request trigger block so that the
workflow executes for pushes and pull requests targeting the new v29 branch in
addition to the existing master, v25, v26, and v27 branches.
- Around line 16-19: Replace the mutable version tags in both the
actions/checkout and swaggerexpert/apidom-validate actions with specific commit
SHAs for improved supply chain security. For the actions/checkout action, also
add the persist-credentials parameter set to false since this workflow is
read-only and does not require writable credentials. Look up the latest stable
commit SHAs for each action and substitute them in place of the version tag
references.
In `@example/index.spec.ts`:
- Around line 609-610: The OpenAPI Documentation test suite (describe.skip block
at line 609-610 in example/index.spec.ts) is disabled locally with the
expectation that validation is handled by the .github/workflows/oas.yml
workflow. However, that workflow currently does not run on the make-v29 branch,
leaving the branch without OpenAPI validation coverage. Before keeping the
describe.skip directive in place, update the .github/workflows/oas.yml file to
ensure it runs on the make-v29 branch by adding make-v29 to the branch
configuration or trigger conditions in the workflow file.
In `@express-zod-api/src/documentation.ts`:
- Around line 67-71: Make the `server` property optional in the type definition
by adding a question mark (making it `server?:`) around line 67, and then add a
guard check in the code that registers metadata (around line 179) to verify that
`server` exists before attempting to use it. This ensures the Documentation
class can be instantiated without providing a `server` property and prevents
runtime errors when `addServer` or similar metadata registration methods receive
undefined values from JavaScript consumers.
In `@express-zod-api/src/security.ts`:
- Around line 87-91: The optional OAuth2 fields oauth2MetadataUrl and
deviceAuthorization are missing the required `@default` JSDoc tag in their
documentation blocks. Add `@default` tags to the JSDoc comments for both
oauth2MetadataUrl? (around line 87-91) and deviceAuthorization? (around line
101-105) to indicate their default values, following the coding guidelines that
require all public properties in express-zod-api/src/*.ts files to have `@desc`,
`@default` for optional properties, and `@example` directives.
In `@migration/index.ts`:
- Around line 124-147: The documentationConfig handler silently drops title and
version properties when an info property already exists in the config. When the
info property is encountered, infoItems is set to undefined, and then when title
or version properties are subsequently processed, the optional chaining
infoItems?.push() silently does nothing without adding these properties to
parts. To fix this, modify the logic for handling title and version properties
to check whether infoItems is undefined (indicating info already exists) and
either add them to parts to preserve the legacy properties in the output, or
throw an error or log a warning to explicitly inform the user that both info and
legacy title/version properties cannot coexist in the same config.
---
Outside diff comments:
In `@express-zod-api/src/server.ts`:
- Around line 83-93: The lifecycle hooks config.beforeRouting and
config.afterRouting at lines 83 and 93 may return Promises, but their return
values are currently ignored, which can lead to unhandled Promise rejections and
race conditions with side effects. Capture the return values from both
config.beforeRouting and config.afterRouting hook invocations and await them to
ensure they complete before proceeding with the next operations, using
Promise.resolve to safely handle both synchronous and asynchronous return
values.
---
Nitpick comments:
In `@express-zod-api/src/security.ts`:
- Around line 77-80: The DeviceAuthFlow type alias on lines 77-80 uses a type
alias to define an object-shaped type, but the repository guidelines require
using interfaces instead of type aliases for object-based types. Convert the
DeviceAuthFlow type alias to an interface by replacing the type keyword and
equals sign with the interface keyword, and adjust the body to list the
constituent types as interface extensions using the extends keyword instead of
the intersection operator.
🪄 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
Run ID: c8a35ca1-dea2-46e5-810d-9e8031382c7c
⛔ Files ignored due to path filters (6)
express-zod-api/tests/__snapshots__/common-helpers.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/documentation-helpers.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/documentation.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/integration.spec.ts.snapis excluded by!**/*.snapmigration/__snapshots__/index.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.github/workflows/codeql-analysis.yml.github/workflows/node.js.yml.github/workflows/oas.yml.pnpmfile.mjsCHANGELOG.mdREADME.mdcompat-test/eslint.config.jscompat-test/migration.spec.tscompat-test/package.jsoneslint.config.jsexample/endpoints/list-users.tsexample/example.client.tsexample/example.documentation.yamlexample/generate-documentation.tsexample/index.spec.tsexample/index.tsexpress-zod-api/package.jsonexpress-zod-api/src/common-helpers.tsexpress-zod-api/src/config-type.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/documentation.tsexpress-zod-api/src/errors.tsexpress-zod-api/src/integration.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/method.tsexpress-zod-api/src/routing.tsexpress-zod-api/src/security.tsexpress-zod-api/src/server.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/documentation-helpers.spec.tsexpress-zod-api/tests/documentation.spec.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/env.spec.tsexpress-zod-api/tests/express-mock.tsexpress-zod-api/tests/integration.spec.tsexpress-zod-api/tests/method.spec.tsexpress-zod-api/tests/routing.spec.tsexpress-zod-api/tests/server.spec.tsexpress-zod-api/tests/system.spec.tsissue952-test/tags.tsmigration/index.spec.tsmigration/index.tsmigration/package.jsonzod-plugin/package.json
💤 Files with no reviewable changes (2)
- .pnpmfile.mjs
- express-zod-api/src/integration.ts
✅ Files skipped from review due to trivial changes (8)
- example/index.ts
- compat-test/migration.spec.ts
- zod-plugin/package.json
- express-zod-api/src/json-schema-helpers.ts
- express-zod-api/tests/express-mock.ts
- README.md
- express-zod-api/tests/endpoint.spec.ts
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (10)
- express-zod-api/src/common-helpers.ts
- express-zod-api/src/errors.ts
- example/endpoints/list-users.ts
- express-zod-api/tests/env.spec.ts
- express-zod-api/tests/common-helpers.spec.ts
- express-zod-api/tests/routing.spec.ts
- express-zod-api/src/method.ts
- express-zod-api/tests/method.spec.ts
- express-zod-api/src/routing.ts
- example/example.client.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
express-zod-api/src/server.ts (1)
83-93:⚠️ Potential issue | 🟠 MajorGuard against Promise-returning lifecycle hooks.
Lines 83 and 93 currently ignore returned values. In TypeScript,
asynccallbacks can still be assignable to void-return hook types, so rejected Promises can become unhandled and hook side effects can race startup/routing.💡 Proposed fix
+const assertSyncHook = ( + hook: "beforeRouting" | "afterRouting", + result: unknown, +) => { + if (result && typeof (result as PromiseLike<unknown>).then === "function") { + throw new Error( + `${hook} must be synchronous in v29. Move async initialization before createServer().`, + ); + } +}; + export const createServer = (config: ServerConfig, routing: Routing) => { @@ - config.beforeRouting?.({ app, getLogger }); + assertSyncHook("beforeRouting", config.beforeRouting?.({ app, getLogger })); @@ - config.afterRouting?.({ app, getLogger }); + assertSyncHook("afterRouting", config.afterRouting?.({ app, getLogger }));🤖 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 `@express-zod-api/src/server.ts` around lines 83 - 93, The lifecycle hooks config.beforeRouting and config.afterRouting at lines 83 and 93 may return Promises, but their return values are currently ignored, which can lead to unhandled Promise rejections and race conditions with side effects. Capture the return values from both config.beforeRouting and config.afterRouting hook invocations and await them to ensure they complete before proceeding with the next operations, using Promise.resolve to safely handle both synchronous and asynchronous return values.
🧹 Nitpick comments (1)
express-zod-api/src/security.ts (1)
77-80: ⚡ Quick winUse an interface for the new object-shaped flow type
Line 77 introduces an object-shaped type alias. Please switch it to an interface to match the TypeScript guideline used in this repo.
Proposed fix
-type DeviceAuthFlow<S extends string> = DeviceAuthUrl & - TokenUrl & - RefreshUrl & - Scopes<S>; +interface DeviceAuthFlow<S extends string> + extends DeviceAuthUrl, + TokenUrl, + RefreshUrl, + Scopes<S> {}As per coding guidelines,
**/*.ts: “Declare object-based types as interfaces, not type aliases.”🤖 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 `@express-zod-api/src/security.ts` around lines 77 - 80, The DeviceAuthFlow type alias on lines 77-80 uses a type alias to define an object-shaped type, but the repository guidelines require using interfaces instead of type aliases for object-based types. Convert the DeviceAuthFlow type alias to an interface by replacing the type keyword and equals sign with the interface keyword, and adjust the body to list the constituent types as interface extensions using the extends keyword instead of the intersection operator.Source: Coding guidelines
🤖 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 @.github/workflows/oas.yml:
- Around line 3-6: The workflow trigger configuration in the push and
pull_request sections is missing the v29 branch. Add v29 to the branches array
in both the push trigger block and the pull_request trigger block so that the
workflow executes for pushes and pull requests targeting the new v29 branch in
addition to the existing master, v25, v26, and v27 branches.
- Around line 16-19: Replace the mutable version tags in both the
actions/checkout and swaggerexpert/apidom-validate actions with specific commit
SHAs for improved supply chain security. For the actions/checkout action, also
add the persist-credentials parameter set to false since this workflow is
read-only and does not require writable credentials. Look up the latest stable
commit SHAs for each action and substitute them in place of the version tag
references.
In `@example/index.spec.ts`:
- Around line 609-610: The OpenAPI Documentation test suite (describe.skip block
at line 609-610 in example/index.spec.ts) is disabled locally with the
expectation that validation is handled by the .github/workflows/oas.yml
workflow. However, that workflow currently does not run on the make-v29 branch,
leaving the branch without OpenAPI validation coverage. Before keeping the
describe.skip directive in place, update the .github/workflows/oas.yml file to
ensure it runs on the make-v29 branch by adding make-v29 to the branch
configuration or trigger conditions in the workflow file.
In `@express-zod-api/src/documentation.ts`:
- Around line 67-71: Make the `server` property optional in the type definition
by adding a question mark (making it `server?:`) around line 67, and then add a
guard check in the code that registers metadata (around line 179) to verify that
`server` exists before attempting to use it. This ensures the Documentation
class can be instantiated without providing a `server` property and prevents
runtime errors when `addServer` or similar metadata registration methods receive
undefined values from JavaScript consumers.
In `@express-zod-api/src/security.ts`:
- Around line 87-91: The optional OAuth2 fields oauth2MetadataUrl and
deviceAuthorization are missing the required `@default` JSDoc tag in their
documentation blocks. Add `@default` tags to the JSDoc comments for both
oauth2MetadataUrl? (around line 87-91) and deviceAuthorization? (around line
101-105) to indicate their default values, following the coding guidelines that
require all public properties in express-zod-api/src/*.ts files to have `@desc`,
`@default` for optional properties, and `@example` directives.
In `@migration/index.ts`:
- Around line 124-147: The documentationConfig handler silently drops title and
version properties when an info property already exists in the config. When the
info property is encountered, infoItems is set to undefined, and then when title
or version properties are subsequently processed, the optional chaining
infoItems?.push() silently does nothing without adding these properties to
parts. To fix this, modify the logic for handling title and version properties
to check whether infoItems is undefined (indicating info already exists) and
either add them to parts to preserve the legacy properties in the output, or
throw an error or log a warning to explicitly inform the user that both info and
legacy title/version properties cannot coexist in the same config.
---
Outside diff comments:
In `@express-zod-api/src/server.ts`:
- Around line 83-93: The lifecycle hooks config.beforeRouting and
config.afterRouting at lines 83 and 93 may return Promises, but their return
values are currently ignored, which can lead to unhandled Promise rejections and
race conditions with side effects. Capture the return values from both
config.beforeRouting and config.afterRouting hook invocations and await them to
ensure they complete before proceeding with the next operations, using
Promise.resolve to safely handle both synchronous and asynchronous return
values.
---
Nitpick comments:
In `@express-zod-api/src/security.ts`:
- Around line 77-80: The DeviceAuthFlow type alias on lines 77-80 uses a type
alias to define an object-shaped type, but the repository guidelines require
using interfaces instead of type aliases for object-based types. Convert the
DeviceAuthFlow type alias to an interface by replacing the type keyword and
equals sign with the interface keyword, and adjust the body to list the
constituent types as interface extensions using the extends keyword instead of
the intersection operator.
🪄 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
Run ID: c8a35ca1-dea2-46e5-810d-9e8031382c7c
⛔ Files ignored due to path filters (6)
express-zod-api/tests/__snapshots__/common-helpers.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/documentation-helpers.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/documentation.spec.ts.snapis excluded by!**/*.snapexpress-zod-api/tests/__snapshots__/integration.spec.ts.snapis excluded by!**/*.snapmigration/__snapshots__/index.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.github/workflows/codeql-analysis.yml.github/workflows/node.js.yml.github/workflows/oas.yml.pnpmfile.mjsCHANGELOG.mdREADME.mdcompat-test/eslint.config.jscompat-test/migration.spec.tscompat-test/package.jsoneslint.config.jsexample/endpoints/list-users.tsexample/example.client.tsexample/example.documentation.yamlexample/generate-documentation.tsexample/index.spec.tsexample/index.tsexpress-zod-api/package.jsonexpress-zod-api/src/common-helpers.tsexpress-zod-api/src/config-type.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/documentation.tsexpress-zod-api/src/errors.tsexpress-zod-api/src/integration.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/method.tsexpress-zod-api/src/routing.tsexpress-zod-api/src/security.tsexpress-zod-api/src/server.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/documentation-helpers.spec.tsexpress-zod-api/tests/documentation.spec.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/env.spec.tsexpress-zod-api/tests/express-mock.tsexpress-zod-api/tests/integration.spec.tsexpress-zod-api/tests/method.spec.tsexpress-zod-api/tests/routing.spec.tsexpress-zod-api/tests/server.spec.tsexpress-zod-api/tests/system.spec.tsissue952-test/tags.tsmigration/index.spec.tsmigration/index.tsmigration/package.jsonzod-plugin/package.json
💤 Files with no reviewable changes (2)
- .pnpmfile.mjs
- express-zod-api/src/integration.ts
✅ Files skipped from review due to trivial changes (8)
- example/index.ts
- compat-test/migration.spec.ts
- zod-plugin/package.json
- express-zod-api/src/json-schema-helpers.ts
- express-zod-api/tests/express-mock.ts
- README.md
- express-zod-api/tests/endpoint.spec.ts
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (10)
- express-zod-api/src/common-helpers.ts
- express-zod-api/src/errors.ts
- example/endpoints/list-users.ts
- express-zod-api/tests/env.spec.ts
- express-zod-api/tests/common-helpers.spec.ts
- express-zod-api/tests/routing.spec.ts
- express-zod-api/src/method.ts
- express-zod-api/tests/method.spec.ts
- express-zod-api/src/routing.ts
- example/example.client.ts
🛑 Comments failed to post (6)
.github/workflows/oas.yml (2)
3-6:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the v29 branch in the workflow trigger.
This workflow still only runs for
master/v25/v26/v27, so it will miss pushes and PRs for the new v29 line this PR is meant to validate.🤖 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 @.github/workflows/oas.yml around lines 3 - 6, The workflow trigger configuration in the push and pull_request sections is missing the v29 branch. Add v29 to the branches array in both the push trigger block and the pull_request trigger block so that the workflow executes for pushes and pull requests targeting the new v29 branch in addition to the existing master, v25, v26, and v27 branches.
16-19:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/oas.ymlRepository: RobinTail/express-zod-api
Length of output: 664
🏁 Script executed:
find .github/workflows -name "*.yml" -o -name "*.yaml" | head -20Repository: RobinTail/express-zod-api
Length of output: 285
🏁 Script executed:
head -30 .github/workflows/bump.ymlRepository: RobinTail/express-zod-api
Length of output: 655
🏁 Script executed:
head -30 .github/workflows/node.js.ymlRepository: RobinTail/express-zod-api
Length of output: 973
🏁 Script executed:
head -30 .github/workflows/npm-publish.ymlRepository: RobinTail/express-zod-api
Length of output: 738
🏁 Script executed:
grep -r "uses:" .github/workflows/*.yml | head -30Repository: RobinTail/express-zod-api
Length of output: 1785
🏁 Script executed:
grep -r "persist-credentials" .github/workflows/Repository: RobinTail/express-zod-api
Length of output: 51
🏁 Script executed:
cat .github/workflows/codeql-analysis.yml | grep -A 5 "checkout"Repository: RobinTail/express-zod-api
Length of output: 234
🏁 Script executed:
cat .github/workflows/npm-publish.yml | grep -A 5 "permissions"Repository: RobinTail/express-zod-api
Length of output: 224
Pin the actions to commit SHAs and disable persisted checkout credentials.
actions/checkout@v6andswaggerexpert/apidom-validate@v1use mutable version tags; pin both to commit SHAs for supply chain security. Also setpersist-credentials: falseon checkout since this job is read-only and does not need writable credentials.🧰 Tools
🪛 zizmor (1.25.2)
[warning] 16-17: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 17-17: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/oas.yml around lines 16 - 19, Replace the mutable version tags in both the actions/checkout and swaggerexpert/apidom-validate actions with specific commit SHAs for improved supply chain security. For the actions/checkout action, also add the persist-credentials parameter set to false since this workflow is read-only and does not require writable credentials. Look up the latest stable commit SHAs for each action and substitute them in place of the version tag references.Source: Linters/SAST tools
example/index.spec.ts (1)
609-610:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore OpenAPI validation coverage for
make-v29before skipping this suite.Line 609 disables the local docs test, but the replacement workflow (
.github/workflows/oas.yml) currently does not run on
make-v29. That leaves this branch without OpenAPI validation.Suggested fix (`.github/workflows/oas.yml`)
on: push: - branches: [ master, v25, v26, v27 ] + branches: [ master, v25, v26, v27, make-v29 ] pull_request: - branches: [ master, v25, v26, v27 ] + branches: [ master, v25, v26, v27, make-v29 ]🤖 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 `@example/index.spec.ts` around lines 609 - 610, The OpenAPI Documentation test suite (describe.skip block at line 609-610 in example/index.spec.ts) is disabled locally with the expectation that validation is handled by the .github/workflows/oas.yml workflow. However, that workflow currently does not run on the make-v29 branch, leaving the branch without OpenAPI validation coverage. Before keeping the describe.skip directive in place, update the .github/workflows/oas.yml file to ensure it runs on the make-v29 branch by adding make-v29 to the branch configuration or trigger conditions in the workflow file.express-zod-api/src/documentation.ts (1)
67-71:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
serveroptional and guard metadata registration.Line 67 currently requires
server, and Line 179 assumes it always exists. That conflicts with the migrated constructor shape that may omitserver(new Documentation({ info, routing, config })), and can passundefinedtoaddServerat runtime in JS consumers.Suggested fix
interface DocumentationParams { /** `@desc` At least title and version properties are required */ info: InfoObject; /** `@desc` Server URL(s) or their complete definitions */ - server: + server?: | string | [string, ...string[]] | ServerObject | [ServerObject, ...ServerObject[]]; @@ `#addMetadata`({ tags, info, server }: DocumentationParams) { this.addInfo(info); if (tags) this.rootDoc.tags = depictTags(tags); - for (const one of Array.isArray(server) ? server : [server]) - this.addServer(typeof one === "string" ? { url: one } : one); + if (server) { + for (const one of Array.isArray(server) ? server : [server]) { + this.addServer(typeof one === "string" ? { url: one } : one); + } + } }Also applies to: 176-180
🤖 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 `@express-zod-api/src/documentation.ts` around lines 67 - 71, Make the `server` property optional in the type definition by adding a question mark (making it `server?:`) around line 67, and then add a guard check in the code that registers metadata (around line 179) to verify that `server` exists before attempting to use it. This ensures the Documentation class can be instantiated without providing a `server` property and prevents runtime errors when `addServer` or similar metadata registration methods receive undefined values from JavaScript consumers.express-zod-api/src/security.ts (1)
87-91:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing
@defaulttags for optional OAuth2 fieldsLine 91 (
oauth2MetadataUrl?) and Line 105 (deviceAuthorization?) are optional, but their JSDoc blocks miss@default, which is required for public properties inexpress-zod-api/src/*.ts.Proposed fix
/** * `@desc` URL to OAuth 2.0 Authorization Server metadata document + * `@default` undefined * `@link` https://www.rfc-editor.org/rfc/rfc8414 * */ oauth2MetadataUrl?: string; @@ /** * `@desc` Device Authorization flow (OAuth 2.0 Device Authorization Grant) + * `@default` undefined * `@link` https://oauth.net/2/device-flow/ * */ deviceAuthorization?: DeviceAuthFlow<S>;As per coding guidelines,
express-zod-api/src/*.ts: “All properties of publicly available entities (exported via index.ts) must have JSDoc documentation with@desc,@default(for optional properties), and@example(for literal types) directives.”Also applies to: 101-105
🤖 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 `@express-zod-api/src/security.ts` around lines 87 - 91, The optional OAuth2 fields oauth2MetadataUrl and deviceAuthorization are missing the required `@default` JSDoc tag in their documentation blocks. Add `@default` tags to the JSDoc comments for both oauth2MetadataUrl? (around line 87-91) and deviceAuthorization? (around line 101-105) to indicate their default values, following the coding guidelines that require all public properties in express-zod-api/src/*.ts files to have `@desc`, `@default` for optional properties, and `@example` directives.Source: Coding guidelines
migration/index.ts (1)
124-147:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEdge case:
title/versionprops are silently dropped wheninfoalready exists.If a user's config contains both an
infoproperty and legacytitle/versionproperties (e.g., partially migrated code), this handler will:
- Add the
infoprop topartsand setinfoItems = undefined(line 137)- Skip adding
title/versionto eitherpartsorinfoItems(line 140 does nothing via optional chaining)- Still report a change because
changeloghas an entryResult: The
title/versionproperties are silently lost in the output.Consider either preserving the legacy props in parts when
infoalready exists, or explicitly warning when both forms are present.Suggested fix
} else if (propName === "title" || propName === "version") { - changelog["title, version"] = infoItems ? "info" : ""; - infoItems?.push(ctx.sourceCode.getText(prop)); + if (infoItems) { + changelog["title, version"] = "info"; + infoItems.push(ctx.sourceCode.getText(prop)); + } else { + // info already exists; preserve legacy props to avoid data loss + parts.push(ctx.sourceCode.getText(prop)); + }🤖 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 `@migration/index.ts` around lines 124 - 147, The documentationConfig handler silently drops title and version properties when an info property already exists in the config. When the info property is encountered, infoItems is set to undefined, and then when title or version properties are subsequently processed, the optional chaining infoItems?.push() silently does nothing without adding these properties to parts. To fix this, modify the logic for handling title and version properties to check whether infoItems is undefined (indicating info already exists) and either add them to parts to preserve the legacy properties in the output, or throw an error or log a warning to explicitly inform the user that both info and legacy title/version properties cannot coexist in the same config.
RobinTail
commented
Jun 20, 2026
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review, the CHANGELOG entry for QUERY was moved into the v29 section, and the routing registration was refactored from .bind(app)(...) to .call(app, ...) with a simpler type annotation.
- Move QUERY CHANGELOG entry to v29 — relocated from v28.8.0 section to sit alongside other v29 features.
- Refactor routing registration from
bindtocall— replaced.bind(app)(path, ...handlers)with.call(app, path, ...handlers)and simplified theregistertype fromIRouterMatcher<IRouter>to a plain function type, avoiding an unnecessary intermediate bound function.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review, removed the runtime safety check for the QUERY method and switched to .call(app, ...) for more direct invocation.
- Removed QUERY runtime safety check — the
enginesrestriction (^22.19.0) ensures Node.js 22.19+ which includes QUERY inhttp.METHODS, so the guard is dead code. This also leterrors.tsrevert to its pre-PR state, removing the unnecessaryCORSMethodbroadening. - Switched
.bind(app)to.call(app, path, ...handlers)— invokes the register function directly with the correctthiscontext in one shot, avoiding an intermediate bound function.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Owner
Author
|
@pullfrog let me know if I forget something |
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — adds HTTP QUERY method (RFC 10008) support, a safe+idempotent method with body semantics, integrated across the full framework surface.
- Add
"query"toFamiliarMethodtype andmethodsarray — also excludes"all"(not a real HTTP method) from the type pool. - Wire routing for QUERY — uses a type assertion +
.call()bridge to work around Express types not yet listing"query"onIRouter. - Define default input sources for QUERY —
["query", "body", "params"]giving body higher priority than query params. - Update example, tests, and snapshots —
list-usersendpoint switches from GET to QUERY; integration, documentation, routing, method, endpoint, and common-helpers tests all cover the new method. - Update
IntegrationandDocumentationgenerators — client code types useQueryprefix, generatedMethodunion includes"query",hasBodycorrectly evaluates totruefor QUERY. - Environment check —
env.spec.tsasserts Node.jshttp.METHODSincludesQUERY.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review, QUERY method endpoints now correctly keep isQueryEnabled input properties in the request body, since the body IS the semantic query for the QUERY method.
- Exclude
"query"from query-param rendering indepictRequestParams—getLocationnow skipsisQueryEnabledinputs whenmethod === "query", so those properties stay in the request body rather than being redundantly shown as URL query parameters.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
RobinTail
added a commit
that referenced
this pull request
Jul 2, 2026
1. Related to #3479 2. Restores #1733 3. Partially reverts #1741 to support both JSON and more conventional URL-encoded body types for QUERY method 4. Splits CORS to maintain handling for the issue #2706 5. Changes the type of the `cors` config option (breaking) to support RequestHandler, such as a middleware provided by the well-known `cors` library 6. Featuring `beforeParsers` hook, while `beforeRouting` remains exactly before calling `initRouting()` --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
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.
Supporting HTTP QUERY Method in express-zod-api
Background: What Is HTTP QUERY?
The HTTP QUERY method is defined in RFC 10008 (published June 2026, Standards Track). In essence, QUERY is like GET but with a body. It is a safe and idempotent request method that can carry request content — a combination of properties that no existing standard HTTP method fully satisfies.
QUERY requests that the target resource process the enclosed content (the query) and respond with the result, without changing resource state. The server uses the
Content-Typeto determine how to interpret the body, and clients can discover QUERY support via theAccept-Queryresponse header.Key properties:
Why Use QUERY When GET and POST Already Exist?
Problems with GET for queries
GET encodes the entire query in the URI. This has several drawbacks:
Problems with POST for queries
POST is neither safe nor idempotent. Using it for queries means:
How QUERY bridges the gap
QUERY is semantically a read operation (safe, idempotent, cacheable) but with body support like POST. This makes it the natural choice for:
Content-TypeWhat Would It Take to Add QUERY Support?
Current state
QUERYinhttp.METHODS— the parser accepts it.@types/express-serve-static-core) do not listqueryonIRouter. This is a deliberate policy: Express adds type entries only when the IETF method reaches sufficient maturity. (For reference, Express also does not type"query"despite it being the?key=valpart of a URL — the name collision is coincidental.)Summary by CodeRabbit
value→dataValue), and adjusted documentation constructor inputs toinfo+server.Integration.create()factory.