Fix #230: API action adapter should pass schema params as positional args#232
Fix #230: API action adapter should pass schema params as positional args#232
Conversation
…lose #230 The createActionRoute function was passing the entire request body object as a single argument to Server Actions. This broke actions that expect positional parameters (e.g., `getUserStatistics(timeRange)` receiving `{ timeRange: "hour" }` instead of `"hour"`). The fix extracts parameters from the request body based on the schema keys and passes them as positional arguments to match the action function signatures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the OpenAPI action adapter that caused Server Actions to receive malformed arguments from API requests. By implementing a more sophisticated parameter extraction mechanism based on the defined request schema, the adapter now correctly translates incoming JSON bodies into the expected positional arguments for Server Actions. This fix resolves type mismatch errors, such as passing an object when a string was expected, thereby enhancing the reliability and correctness of API integrations with Server Actions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔒 Security Scan Results✅ No security vulnerabilities detected This PR has been scanned against OWASP Top 10, CWE Top 25, and common security anti-patterns. No security issues were identified in the code changes. 📋 OWASP Top 10 Coverage
Additional Security Checks
🛡️ Security PostureStrong - The change modifies parameter passing logic but maintains existing security controls:
🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where API action parameters were passed as a single object instead of positional arguments. The change correctly extracts parameters from the request body based on the Zod schema and passes them as individual arguments to the server action.
My review includes two main points:
- A critical issue regarding how the request body is obtained. The current implementation bypasses the framework's validation, which can lead to runtime errors. I've recommended using
c.req.valid('json')for safe and validated data access. - A high-severity issue that identifies a regression for actions that expect the full request body but have no parameters defined in their schema. I've suggested a fix to maintain backward compatibility for these cases.
| if (requestSchema instanceof z.ZodObject) { | ||
| const schemaShape = requestSchema.shape; | ||
| const keys = Object.keys(schemaShape); | ||
| if (keys.length === 0) { | ||
| // 没有参数 | ||
| args = []; | ||
| } else if (keys.length === 1) { | ||
| // 单个参数，直接传递值 | ||
| args = [body[keys[0] as keyof typeof body]]; | ||
| } else { | ||
| // 多个参数，按 schema key 顺序传递 | ||
| args = keys.map((key) => body[key as keyof typeof body]); | ||
| } | ||
| } else { | ||
| // 非对象 schema，直接传递整个 body | ||
| args = [body]; | ||
| } |
There was a problem hiding this comment.
While this new logic correctly extracts positional arguments, it relies on the body variable, which is obtained unsafely on line 224. This makes the new code susceptible to runtime errors and bypasses the framework's validation features.
The Problem:
The current use of const body = await c.req.json().catch(() => ({})) on line 224 has two major flaws in a hono/zod-openapi application:
- Bypasses Schema Validation: The framework is designed to validate the request body against the
requestSchemabefore this handler code is executed. The correct way to access this validated, parsed, and typed data is viac.req.valid('json'). By callingc.req.json()again, you are re-parsing the body and completely ignoring the validation that has already occurred. - Hides Errors: If a client sends malformed JSON, the
.catch(() => ({}))will causebodyto become an empty object. Your new logic (e.g., on lines 241 and 244) will then attempt to access properties on this empty object, resulting inundefinedarguments being passed to your action. This can lead to subtle bugs or crashes that are hard to debug.
Recommendation:
To ensure correctness and robustness, line 224 should be changed to:
const body = c.req.valid("json");Although this line is outside the current diff, this change is critical for the stability of the logic you've introduced. It will guarantee that body is a type-safe, validated object that conforms to your Zod schema, making your new argument extraction code safe and reliable.
| // 没有参数 | ||
| args = []; |
There was a problem hiding this comment.
When the schema is an object with no defined keys (e.g., z.object({}).passthrough()), this logic calls the action with no arguments. This is a regression from the previous behavior, which passed the entire body object.
This will break actions that expect the whole request body as a single object but don't have properties formally defined in the schema (relying on passthrough for dynamic keys).
To maintain backward compatibility for this scenario, you should pass the body as a single argument.
| // 没有参数 | |
| args = []; | |
| // 没有参数，但可能是 passthrough，传递整个 body 对象以保持兼容 | |
| args = [body]; |
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR aims to fix issue #230 where API actions were receiving the entire body object instead of extracted parameters. While the fix correctly handles single-parameter actions, it introduces a critical regression for multi-parameter actions.
🔍 Issues Found
- Critical (🔴): 1 issue
- High (🟠): 0 issues
- Medium (🟡): 0 issues
- Low (🟢): 0 issues
🎯 Priority Actions
- CRITICAL: The multi-parameter branch (lines 243-246) breaks actions like
editUser(userId, data),editKey(keyId, data), andeditProvider(providerId, data)which expect(id, dataObject)not(id, prop1, prop2, ...)
💡 General Observations
The root cause is that there are two distinct action signature patterns in the codebase:
- Single primitive parameter:
getUserStatistics(timeRange),removeUser(userId)- ✅ Fixed by this PR - Single object parameter:
addKey(data: {...}),addUser(data: {...})- ✅ Was already working, still works - ID + data object:
editUser(userId, data),editKey(keyId, data)- ❌ BROKEN by this PR
The schema cannot distinguish between these patterns. A safer approach would be to keep the multi-parameter case passing the full body object (reverting to original behavior for that case).
🤖 Automated review by Claude AI - focused on identifying issues for improvement
| } | ||
| } else { | ||
| // 非对象 schema，直接传递整个 body | ||
| args = [body]; |
There was a problem hiding this comment.
🔴 Critical: This fix breaks multi-parameter actions like editUser(userId, data), editKey(keyId, data), and editProvider(providerId, data).
Why this is a problem: The new logic extracts all keys from the schema and passes them as individual positional arguments. However, actions like editUser expect (userId: number, data: {...}) - two arguments where the second is an object containing remaining properties.
For example, with schema { userId, name, note, rpm, ... }:
- Current code produces:
editUser(body.userId, body.name, body.note, body.rpm, ...) - Expected by action:
editUser(body.userId, { name, note, rpm, ... })
This will cause data parameter in editUser to receive just body.name (a string) instead of the expected object, breaking all edit operations.
Suggested fix:
// 如果 requestSchema 是对象类型，提取 keys 作为参数顺序
let args: unknown[];
if (requestSchema instanceof z.ZodObject) {
const schemaShape = requestSchema.shape;
const keys = Object.keys(schemaShape);
if (keys.length === 0) {
// 没有参数
args = [];
} else if (keys.length === 1) {
// 单个参数，直接传递值
args = [body[keys[0] as keyof typeof body]];
} else {
// 多个参数场景 - 保持原有行为传递整个 body 对象
// 因为存在两种 action 签名：
// 1. action(arg1, arg2, ...) - 多个独立参数
// 2. action(data: {...}) - 单个对象参数包含多个属性
// 无法从 schema 区分这两种情况，传递整个 body 更安全
args = [body];
}
} else {
// 非对象 schema，直接传递整个 body
args = [body];
}
- Change multi-parameter case to pass entire body object - Fixes #230 for single-param actions (e.g., getUserStatistics) - Preserves compatibility for editUser/editKey/editProvider signatures
| // 因为存在 editUser(userId, data) 这类签名，无法从 schema 区分 | ||
| args = [body]; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🔴 Critical: Multi-parameter actions like editKey, editUser, editProvider will break
Why this is a problem: The fallback for keys.length > 1 passes the entire body object as a single argument. However, actions like editKey(keyId, data), editUser(userId, data), and editProvider(providerId, data) expect two separate positional arguments: an ID and a data object.
For example, when calling editKey with body { keyId: 1, name: "test" }:
- Expected:
editKey(1, { name: "test" }) - Actual (this PR):
editKey({ keyId: 1, name: "test" })← First param receives the whole object, second param isundefined
This is a regression that will break all edit operations.
Suggested fix: The fix needs to happen at the route definition level. Each multi-param action route should use a wrapper:
// In route.ts for editKey:
const { route: editKeyRoute, handler: editKeyHandler } = createActionRoute(
"keys",
"editKey",
// Wrapper to extract positional args from body
async (body: { keyId: number; name: string; /* ... */ }) => {
const { keyId, ...data } = body;
return keyActions.editKey(keyId, data);
},
{ requestSchema: z.object({ keyId: z.number().int().positive(), ... }) }
);Alternatively, add explicit parameter extraction config to ActionRouteOptions to specify which keys should be extracted as separate positional args.
🔒 Security Scan Results✅ No NEW security vulnerabilities introduced by this PR This PR has been scanned against OWASP Top 10, CWE Top 25, and common security anti-patterns. The code changes in this PR do not introduce new security vulnerabilities. Analysis SummaryThe PR modifies Change Analysis:
This is a functional fix (addressing issue #230) that corrects parameter passing behavior. The change does not introduce new attack vectors. Scanned Categories
Pre-existing Observation (Not Introduced by This PR)
🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR fixes the body parsing issue for single-parameter Server Actions by extracting the parameter value from the body object instead of passing the entire object. The fix correctly addresses issue #230 for single-param actions like getUserStatistics(timeRange).
🔍 Issues Found
- Critical (🔴): 0 issues (my previous comment was inaccurate - see clarification below)
- High (🟠): 0 issues
- Medium (🟡): 1 issue (documentation/comment accuracy)
- Low (🟢): 0 issues
🎯 Priority Actions
-
Clarification on my previous comment: After deeper analysis, I realized the multi-param issue I flagged is not a regression - the existing code (
action(body)) already didn't work foreditUser(userId, data)style signatures. The PR correctly maintains this existing behavior while fixing single-param cases. The code comment accurately documents this limitation. -
Minor suggestion: The comment on line 227 claims compatibility with
action(arg1, arg2, ...)but the implementation only supportsaction(singleArg)andaction({...})patterns. Consider updating the comment to be more precise:// 这样可以兼容 action(singleValue) 和 action({ arg1, arg2, ... }) 两种签名 // 注意：不支持 action(arg1, arg2, ...) 多个位置参数的签名
💡 General Observations
- The fix is correctly scoped to address the specific issue reported (#230)
- The single-param case extraction logic is sound
- For multi-param positional-args signatures like
editUser(userId, data), the current API routes already pass the whole body object, which means these routes were already broken before this PR. This is a pre-existing architectural limitation, not something introduced by this change. - Future consideration: If multi-param positional actions need API support, wrapper functions should be added at the route definition level in
route.ts
🤖 Automated review by Claude AI - focused on identifying issues for improvement
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR fixes the body parsing issue for single-parameter Server Actions by extracting the parameter value from the body object instead of passing the entire object. The fix correctly addresses issue #230 for single-param actions like getUserStatistics(timeRange).
🔍 Issues Found
- Critical (🔴): 0 issues (my previous comment was inaccurate - see clarification below)
- High (🟠): 0 issues
- Medium (🟡): 1 issue (documentation/comment accuracy)
- Low (🟢): 0 issues
🎯 Priority Actions
-
Clarification on my previous comment: After deeper analysis, I realized the multi-param issue I flagged is not a regression - the existing code (
action(body)) already did not work foreditUser(userId, data)style signatures. The PR correctly maintains this existing behavior while fixing single-param cases. The code comment accurately documents this limitation. -
Minor suggestion (🟡 Medium): The comment on line 227 claims compatibility with
action(arg1, arg2, ...)but the implementation only supportsaction(singleArg)andaction({...})patterns. Consider updating the comment to be more precise:// 这样可以兼容 action(singleValue) 和 action({ arg1, arg2, ... }) 两种签名 // 注意：不支持 action(arg1, arg2, ...) 多个位置参数的签名
💡 General Observations
- The fix is correctly scoped to address the specific issue reported (#230)
- The single-param case extraction logic is sound
- For multi-param positional-args signatures like
editUser(userId, data), the current API routes already pass the whole body object, which means these routes were already not working before this PR. This is a pre-existing architectural limitation, not something introduced by this change. - Future consideration: If multi-param positional actions need API support, wrapper functions should be added at the route definition level in
route.ts
🤖 Automated review by Claude AI - focused on identifying issues for improvement
1 participant
Close #230
Summary
createActionRoutefunction in the OpenAPI action adapter to properly extract and pass request body parameters as positional arguments to Server ActionsgetUserStatistics(timeRange)to receive{ timeRange: "hour" }instead of the expected"hour"stringRoot Cause
The error
Invalid time range: [object Object]occurred because:POST /api/actions/statistics/getUserStatisticswith body{ "timeRange": "hour", "userId": 1 }action(body)which passed the entire object as the first parametergetUserStatistics(timeRange)expected a string but received the whole objectChanges
src/lib/api/action-adapter-openapi.tsto extract parameters from the request body based on the schema keys and pass them as positional argumentsTest plan
POST /api/actions/statistics/getUserStatisticswith{ "timeRange": "hour" }returns valid statistics🤖 Generated with Claude Code