Propagate ZodError as cause for PARSE_PATH_PARAMS
#15
Conversation
WalkthroughThe changes update the error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant PP as parsePathParams
participant ZO as ZodError
Caller->>PP: call parsePathParams(params)
PP->>PP: attempt to parse parameters
alt Parsing fails
PP->>ZO: capture issues from ZodError
PP->>Caller: throw Error("PARSE_PATH_PARAMS", {cause: issues})
else Parsing successful
PP->>Caller: return parsed parameters
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/path-params.ts (1)
14-14: Excellent improvement to error propagation!This change properly propagates the Zod validation issues through the Error's cause property, which addresses the PR objective of ensuring that
issuesare available when handling PARSE_PATH_PARAMS errors.Consider adding a null/type check before accessing
issuesto make the code more robust:- throw new Error("PARSE_PATH_PARAMS", { cause: (error as ZodError).issues }); + throw new Error("PARSE_PATH_PARAMS", { + cause: error instanceof Error && 'issues' in error ? error.issues : error + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/path-params.ts(1 hunks)
🔇 Additional comments (2)
src/core/path-params.ts (2)
2-2: Type import looks good.The ZodError import is correctly declared and is being used in the error handling logic.
7-15:❓ Verification inconclusive
Verify error handling consistency across the codebase.
This change looks good for the
parsePathParamsfunction, but you might want to ensure similar error handling is applied consistently across other parsing functions in the library.
🏁 Script executed:
#!/bin/bash # Description: Check other parsing functions to verify if they use similar error handling patterns. # Look for other functions that might need similar error handling rg -A 2 "throw new Error\(\"PARSE_" --type ts # Check if there are other usages of ZodError in the codebase that might need attention rg "as ZodError" --type tsLength of output: 1563
Action: Verify uniform error handling across parsing functions
The error handling in
src/core/path-params.tslooks solid and aligns with how most parsing functions handle errors. However, our search revealed a potential inconsistency insrc/core/body.tsfor the"PARSE_REQUEST_BODY"error: one instance throws an error usingresult.error?.issueswhile another uses(error as ZodError).issues. Please review this discrepancy to determine if it’s intentional or if it should be standardized across all parsing functions.
- Files to review:
src/core/path-params.ts– uses(error as ZodError).issuessrc/core/body.ts– bothPARSE_FORM_DATAand one occurrence ofPARSE_REQUEST_BODYuse(error as ZodError).issueswhile another occurrence usesresult.error?.issuessrc/core/search-params.ts– uses(error as ZodError).issues
| console.log((error as ZodError).issues); | ||
| } | ||
| throw new Error("PARSE_PATH_PARAMS"); | ||
| throw new Error("PARSE_PATH_PARAMS", { cause: (error as ZodError).issues }); |
There was a problem hiding this comment.
Technically in this case, ZodError will always be there since it's going to be the error type of safeParse.
In the future, perhaps we can structure this function in such a way that the error will always be a ZodError without relying on type assertions! 👍
|
Can you edit the commit message using conventional commits |
|
@omermecitoglu I fixed it, let me know if there's anything else that needs to be changed! |
|
Thanks for contributing 🥂 |
2 participants
Description
Hi there, thank you for making this library! It's very useful to generate OpenAPI schemas from Next.js's API route.
I noticed one thing that seems to be missing. Let's say for this code (in
app/api/[symbol]):This is the response, which I don't think is correct.
issuesshould not beundefinedsince from the code above,pathParamsare validated by the Zod schema.Expected Behavior
I think
issuesshould not beundefined. I noticed that forPARSE_PATH_PARAMS, we didn't put theZodErroras part of thecause. So, this PR should fix it.Future
In the future, would be nice to strategically structure the code in such a way that if we get one of these error types,
issuesshould never beundefinedsince these validations will always have to pass through the Zod schema, soZodErrorshall always be there.PARSE_FORM_DATAPARSE_REQUEST_BODYPARSE_SEARCH_PARAMSPARSE_PATH_PARAMSThank you!
Summary by CodeRabbit