fix(zod): enhance schema conversion logic for zod4 preprocess #1318
Conversation
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ 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 |
Summary of ChangesHello @unnoq, 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 refines the schema conversion logic within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
There was a problem hiding this comment.
Code Review
This pull request correctly enhances the Zod schema conversion logic to handle preprocess functions, which are implemented as ZodPipe with a transform on the input side. The fix ensures that for an input strategy, the out schema is used, which is the desired behavior for preprocess. The change is well-supported by a new test case that validates this scenario. The implementation is solid, and I have one minor suggestion to improve code readability for future maintenance.
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-pino
@orpc/experimental-publisher
@orpc/experimental-publisher-durable-object
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/zod/src/zod4/converter.ts (1)
522-530: Logic correctly handlesz.preprocess()for OpenAPI generation.The conditional logic properly addresses issue #1317 by detecting when the input schema is a transform (as created by
z.preprocess()) and using the output schema instead. This ensures OpenAPI input schemas describe the validated type rather than the preprocessing transformation.For clarity, consider refining the comment to be more specific:
- // prefer out schema when in schema is preprocess/transform + // For input strategy: use in schema unless it's a transform (e.g., z.preprocess), + // in which case use out schema to represent the validated type after preprocessingpackages/zod/src/zod4/converter.processed.test.ts (1)
21-26: Test validates the fix, but consider expanding coverage.The test correctly demonstrates that
z.preprocess()now produces consistent string schemas for both input and output strategies, resolving issue #1317.However, given that this fixes a critical OpenAPI generation blocker, consider adding tests for:
- Non-trivial preprocessing (e.g.,
z.preprocess(x => String(x).toUpperCase(), z.string()))- Complex schemas (objects, arrays with preprocess in nested fields)
- Interaction with other Zod features (nullable, optional, default with preprocess)
🔎 Example additional test cases
+ { + name: 'object with preprocess field', + schema: z.object({ + name: z.preprocess(x => String(x).trim(), z.string()), + }), + input: [true, { type: 'object', properties: { name: { type: 'string' } }, required: ['name'] }], + output: [true, { type: 'object', properties: { name: { type: 'string' } }, required: ['name'] }], + }, + { + name: 'preprocess with optional', + schema: z.preprocess(x => x, z.string()).optional(), + input: [false, { type: 'string' }], + output: [false, { type: 'string' }], + },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/zod/src/zod4/converter.processed.test.tspackages/zod/src/zod4/converter.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/zod/src/zod4/converter.ts (1)
524-524: Verify handling of edge cases and complex preprocessing scenarios.The condition
pipe._zod.def.in._zod.def.type !== 'transform'correctly identifies preprocessing, but ensure this handles:
- Nested or chained preprocessing (e.g.,
z.preprocess(f1, z.preprocess(f2, z.string())))- Preprocessing with complex schemas (objects with multiple fields using preprocess)
- Combinations with other transformations
Consider adding tests for these scenarios:
// Test: Nested preprocess z.preprocess(x => x, z.preprocess(y => y, z.string())) // Test: Preprocess with object schema z.object({ field: z.preprocess(x => x?.toUpperCase(), z.string()) }) // Test: Preprocess then transform z.preprocess(x => x?.trim(), z.string()).transform(s => s.length)Would you like me to generate these additional test cases?
Deploying orpc with
|
| Latest commit: |
691e0b3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ec7bf25e.orpc-1qh.pages.dev |
| Branch Preview URL: | https://unnoq-issue1317.orpc-1qh.pages.dev |
Fixes #1317
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.