feat: pass zod schema to custom fetch response implementation#3226
feat: pass zod schema to custom fetch response implementation#3226Tarvion wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors how request options are constructed: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Gen as Code Generator
participant Caller as Generated Call Site
participant Mut as Mutator/CustomFetch
participant Val as Optional Validator
rect rgba(200,200,255,0.5)
Gen->>Caller: create rawFetchFnOptions (url getter, merged global opts, method, headers, body)
Gen->>Caller: compute schemaValueRef (if includeZodSchemaInArguments && schema not void)
Gen->>Caller: derive validateFetchFnOptions (append schema if present)
end
Caller->>Mut: call(mutator/customFetch, validateFetchFnOptions)
alt Mutator performs validation
Mut->>Val: validate(response, schema)
Val-->>Mut: validation result
end
Mut-->>Caller: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 |
![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
🧹 Nitpick comments (1)
packages/fetch/src/index.ts (1)
432-432: Be aware: Zod validation throwsZodErroron failure.The
.parse()method on line 432 will throw aZodErrorif validation fails. The current implementation doesn't wrap this in a try-catch, so validation errors will propagate up as thrown errors rather than being handled like fetch errors. This might be the intended behavior, but ensure downstream code handlesZodErrorappropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fetch/src/index.ts` at line 432, The assignment using ${schemaValueRef}.parse(parsedBody) can throw a ZodError; update the code in the fetch response handling to either use ${schemaValueRef}.safeParse(parsedBody) and handle the returned success/error path (convert validation errors into the same error shape used for fetch failures), or wrap the .parse call in a try/catch that catches ZodError and transforms/logs it into a controlled error before rethrowing/returning; locate the usage of ${schemaValueRef} and the data assignment to implement one of these approaches so validation failures are handled consistently with other fetch errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fetch/src/index.ts`:
- Around line 396-399: The template string validateFetchFnOptions incorrectly
always appends "schema: ${schemaValueRef}" regardless of isValidateResponse,
causing syntax errors and passing schema when validation is disabled; fix by
building validateFetchFnOptions conditionally—use rawFetchFnOptions alone when
isValidateResponse is false, and only append ", schema: ${schemaValueRef}" (or
include the schema property) when isValidateResponse is true, or alternatively
choose between fetchFnOptions and validateFetchFnOptions at call sites (the
places using validateFetchFnOptions) so that schemaValueRef is only present when
isValidateResponse is true.
---
Nitpick comments:
In `@packages/fetch/src/index.ts`:
- Line 432: The assignment using ${schemaValueRef}.parse(parsedBody) can throw a
ZodError; update the code in the fetch response handling to either use
${schemaValueRef}.safeParse(parsedBody) and handle the returned success/error
path (convert validation errors into the same error shape used for fetch
failures), or wrap the .parse call in a try/catch that catches ZodError and
transforms/logs it into a controlled error before rethrowing/returning; locate
the usage of ${schemaValueRef} and the data assignment to implement one of these
approaches so validation failures are handled consistently with other fetch
errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d7578c5-75b8-46b6-ae98-2c98f86228d3
📒 Files selected for processing (1)
packages/fetch/src/index.ts
|
looks like build is failing |
soartec-lab
left a comment
soartec-lab
left a comment
There was a problem hiding this comment.
Let's make this the following option: override.mutator.includeZodSchemaInArgments: boolean
This argument will only be added if this flag is true and schema.type is zod.
|
This would be a great improvement |
That sounds like a good idea. Will do that |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fetch/src/index.ts`:
- Around line 427-432: The code unconditionally injects schema:
${schemaValueRef} into validateFetchFnOptions and always uses
validateFetchFnOptions at mutator call sites (validateFetchFnOptions symbol),
which breaks users unless they opt in and only works for zod-backed schemas;
update the logic to only include the schema when isValidateResponse &&
override?.mutator?.includeZodSchemaInArgments === true && isZodOutput is true
(use the existing isZodOutput, isValidateResponse,
override.mutator.includeZodSchemaInArgments and schemaValueRef symbols), create
a new mutatorFetchFnOptions string that falls back to fetchFnOptions when the
opt-in/schema conditions are not met, and replace uses of validateFetchFnOptions
at the mutator call sites with mutatorFetchFnOptions so no schema/key is emitted
unless the opt-in and zod-output checks pass.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 976b8d52-169d-4a34-830e-eee8cb18f9c0
📒 Files selected for processing (1)
packages/fetch/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/types.ts`:
- Line 171: The public interface OverrideOutput is missing the
includeZodSchemaInArguments?: boolean flag that was added to
NormalizedOverrideOutput; add includeZodSchemaInArguments?: boolean to the
OverrideOutput definition so callers can provide it in configs, and ensure the
config normalization code (the normalization routine that builds a
NormalizedOverrideOutput in utils/options.ts) continues to accept and spread
this property from OverrideOutput into NormalizedOverrideOutput (verify the same
property name is used where overrides are merged).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a855fe8d-4bd1-4bfb-bf75-d302a1bd2640
📒 Files selected for processing (2)
packages/core/src/types.tspackages/fetch/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fetch/src/index.ts
please check, and you should update normalize process |
|
Looks like Snapshots are failing? |
soartec-lab
left a comment
soartec-lab
left a comment
There was a problem hiding this comment.
This is a fantastic feature. I've left a few comments, so please check those out. I look forward to your response.
| * @default false | ||
| */ | ||
| useNullForOptional?: boolean; | ||
| includeZodSchemaInArguments?: boolean; |
There was a problem hiding this comment.
Please normalize and specify the initial value for the boolean.
https://github.com/orval-labs/orval/blob/master/packages/orval/src/utils/options.ts
| includeZodSchemaInArguments?: boolean; | |
| includeZodSchemaInArguments: boolean; |
| : `body: JSON.stringify(${requestBodyParams})` | ||
| : ''; | ||
| const fetchFnOptions = `${getUrlFnName}(${getUrlFnProperties}), | ||
| const rawFetchFnOptions = `${getUrlFnName}(${getUrlFnProperties}), |
There was a problem hiding this comment.
I didn't think it was necessary to relinquish the variable name and rearrange the values here.
| schemaValueRef !== 'void' && | ||
| typeof context.output.schemas === 'object' && | ||
| context.output.schemas.type === 'zod'; | ||
| const validateFetchFnOptions = `${rawFetchFnOptions}${includeZodSchema ? ',' : ''} |
There was a problem hiding this comment.
I think it's best to include this process within the function that creates the fetch options.
4 participants
Motivation
Currently, when using a custom mutator together with Zod runtime validation, there is no straightforward way to verify the returned data. This makes it harder to ensure that responses conform to the expected shape.
This PR proposes passing the corresponding Zod schema to the custom mutator when runtime validation is enabled, allowing proper validation of the response data.
Currently generated Code:
The same Code with the new change:
Changes
Summary by CodeRabbit
New Features
Refactor