feat: enhance parameter parsing with POST body and dot notation support#582
Conversation
Introduces support for parsing parameters from the request body, prioritizing them over query parameters. Adds the ability to access nested object properties in both query and body parameters using dot notation (e.g., 'user.address.city'). This change is accompanied by an upgrade of the Node.js runtime to version 24 across development, testing, and CI/CD environments, along with various dependency updates. The test runner script has been updated to conditionally enable lcov reporting based on Node.js version.
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR upgrades Node.js versions across CI workflows, Travis config, and package.json engines to Node 24+, adds a custom test runner script with version-conditional coverage reporting, and introduces POST body parameter parsing with nested dot-notation support integrated into the Express request pipeline, validated by new fixture and test cases. ChangesNode.js Version Upgrade
POST body parameter parsing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressCallback as makeExpressCallback
participant Params as params.js
participant Controller
Client->>ExpressCallback: POST /v1/events with body { event: { startsAt, title } }
ExpressCallback->>Params: parsePost({ post: request.body, spec })
Params-->>ExpressCallback: parsedPost (nested values type-converted)
ExpressCallback->>Params: parseParams({ query, post: parsedPost, spec, mock })
Params->>Params: getNestedValue(post, name) preferred over query
Params->>Params: parseValue(schema, value)
Params-->>ExpressCallback: converted params
ExpressCallback->>Controller: createEvent(params)
Controller-->>ExpressCallback: response { startsAt as Date, title }
ExpressCallback-->>Client: 200 response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/params.js (1)
123-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't drop valid
0/falsevalues after body parsing.
parsePost()now converts body fields before they reachparseParams(), so valid inputs likecount: 0orenabled: falsehit the!paramNamechecks here and get replaced by defaults/examples or removed entirely.Suggested fix
export const parseParams = ({ query, post, spec, mock = false }) => spec .map((parameter) => { const { name, schema } = parameter const { default: defaultValue, example: exampleValue } = schema // Check post first, then query (post data takes precedence) // Support nested objects with dot notation const paramName = getNestedValue(post, name) ?? getNestedValue(query, name) + const hasValue = paramName !== undefined && paramName !== null - if (!paramName && defaultValue !== undefined) { + if (!hasValue && defaultValue !== undefined) { return { name, value: defaultValue } } - if (!paramName && mock && exampleValue !== undefined) { + if (!hasValue && mock && exampleValue !== undefined) { return { name, value: exampleValue } } - if (!paramName) { + if (!hasValue) { return undefined }🤖 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 `@src/params.js` around lines 123 - 147, parseParams is treating valid falsy body values as missing because it uses !paramName after getNestedValue(post, name) / getNestedValue(query, name), which drops 0 and false. Update the checks in parseParams to only fall back to defaultValue/exampleValue or return undefined when the resolved value is actually null or undefined, while preserving legitimate falsy values. Keep the precedence logic (post over query) and the parseValue step unchanged.
🧹 Nitpick comments (1)
scripts/run-tests.js (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
elsebranch is effectively dead code givenengines.node: ">= 24".Since
package.jsonnow requires Node>= 24,nodeMajorwill always satisfy the>= 24check in any environment that honors the engines constraint, making the fallback destination-only branch unreachable in practice. Harmless as defensive coding, but consider removing it (or documenting why it's kept, e.g. to support environments that ignoreengines) for clarity.♻️ Optional simplification if the fallback is confirmed unnecessary
-// The built-in lcov reporter is used on supported Node versions. -const reporterArgs = nodeMajor >= 24 - ? [ - '--test-reporter=lcov', - '--test-reporter-destination=stdout', - '--test-reporter-destination=./coverage/lcov.info' - ] - : ['--test-reporter-destination=stdout'] +const reporterArgs = [ + '--test-reporter=lcov', + '--test-reporter-destination=stdout', + '--test-reporter-destination=./coverage/lcov.info' +]🤖 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 `@scripts/run-tests.js` around lines 11 - 17, The fallback reporterArgs branch in run-tests.js is unreachable because the project already requires Node >= 24, so simplify the nodeMajor check in reporterArgs to remove the dead `: ['--test-reporter-destination=stdout']` path, or add a short comment if you intend to keep it for environments that ignore engines.
🤖 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 `@src/params.js`:
- Around line 123-133: The precedence logic in parseParams is currently applied
to the merged query object, which lets body values override path params as well.
Update parseParams in src/params.js to respect parameter.in so the post-vs-query
precedence only applies to query-like inputs, and adjust src/express-callback.js
so context.request?.params is passed separately instead of being merged into
query. Use parseParams and the request parameter mapping in express-callback as
the main points to change.
---
Outside diff comments:
In `@src/params.js`:
- Around line 123-147: parseParams is treating valid falsy body values as
missing because it uses !paramName after getNestedValue(post, name) /
getNestedValue(query, name), which drops 0 and false. Update the checks in
parseParams to only fall back to defaultValue/exampleValue or return undefined
when the resolved value is actually null or undefined, while preserving
legitimate falsy values. Keep the precedence logic (post over query) and the
parseValue step unchanged.
---
Nitpick comments:
In `@scripts/run-tests.js`:
- Around line 11-17: The fallback reporterArgs branch in run-tests.js is
unreachable because the project already requires Node >= 24, so simplify the
nodeMajor check in reporterArgs to remove the dead `:
['--test-reporter-destination=stdout']` path, or add a short comment if you
intend to keep it for environments that ignore engines.
🪄 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: b30b0c83-9edc-442d-bb06-6342dea80be4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/cpd.yml.github/workflows/lint.yml.github/workflows/sonarcloud.yml.github/workflows/test.yml.github/workflows/vulnerabilities.yml.travis.ymlpackage.jsonscripts/run-tests.jssrc/__fixtures__/spec.jsonsrc/express-callback.jssrc/params.jssrc/params.test.jssrc/server.test.js
| export const parseParams = ({ query, post, spec, mock = false }) => | ||
| spec | ||
| .map((parameter) => { | ||
| const { name, schema } = parameter | ||
| const { | ||
| type, | ||
| default: defaultValue, | ||
| example: exampleValue | ||
| } = schema | ||
| const Type = types[type] | ||
| const paramName = query?.[name] | ||
| // Check post first, then query (post data takes precedence) | ||
| // Support nested objects with dot notation | ||
| const paramName = getNestedValue(post, name) ?? getNestedValue(query, name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Scope body precedence to query params only.
This selection ignores parameter.in. Because src/express-callback.js passes a merged {...request.params, ...request.query} object as query, a body field can now override a path parameter too, which is broader than the stated “body beats query” contract.
Suggested direction
-export const parseParams = ({ query, post, spec, mock = false }) =>
+export const parseParams = ({ query, path, post, spec, mock = false }) =>
spec
.map((parameter) => {
const { name, schema } = parameter
+ const paramName =
+ parameter.in === 'path'
+ ? getNestedValue(path, name)
+ : getNestedValue(post, name) ?? getNestedValue(query, name)And in src/express-callback.js, pass context.request?.params separately instead of folding it into query.
🤖 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 `@src/params.js` around lines 123 - 133, The precedence logic in parseParams is
currently applied to the merged query object, which lets body values override
path params as well. Update parseParams in src/params.js to respect parameter.in
so the post-vs-query precedence only applies to query-like inputs, and adjust
src/express-callback.js so context.request?.params is passed separately instead
of being merged into query. Use parseParams and the request parameter mapping in
express-callback as the main points to change.
2 participants
Introduces support for parsing parameters from the request body, prioritizing them over query parameters. Adds the ability to access nested object properties in both query and body parameters using dot notation (e.g., 'user.address.city').
This change is accompanied by an upgrade of the Node.js runtime to version 24 across development, testing, and CI/CD environments, along with various dependency updates. The test runner script has been updated to conditionally enable lcov reporting based on Node.js version.
Summary by CodeRabbit
New Features
Bug Fixes