fix skipConvexValidation - still do zod validation #874
Conversation
WalkthroughAdds a local Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
commit: |
Nicolapps
left a comment
Nicolapps
left a comment
There was a problem hiding this comment.
Thanks for the fix! Could you please make sure that the unit tests (at least in Zod 4) verify that validation works as intended?
![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 (2)
packages/convex-helpers/server/zod4.ts (1)
911-916: Consider improving error message clarity.The validation logic is correct, but the error message could be more explicit about why this combination is unsupported.
Apply this diff to clarify:
if (skipConvexValidation && Object.keys(inputArgs).length > 0) { throw new Error( - "If you're using a custom function with arguments for the input " + - "customization, you cannot skip convex validation.", + "Cannot skip Convex validation when the customization requires input arguments. " + + "The input arguments need Convex validation to be passed to the customization.", ); }packages/convex-helpers/server/zod3.ts (1)
407-412: Consider improving error message clarity.The validation logic is correct, but the error message could be more explicit, matching the suggestion for zod4.ts.
Apply this diff:
if (skipConvexValidation && Object.keys(inputArgs).length > 0) { throw new Error( - "If you're using a custom function with arguments for the input " + - "customization, you cannot skip convex validation.", + "Cannot skip Convex validation when the customization requires input arguments. " + + "The input arguments need Convex validation to be passed to the customization.", ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/convex-helpers/server/zod3.ts(6 hunks)packages/convex-helpers/server/zod4.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When modifying complex TypeScript types, run
npm run typecheckin the repository root to ensure types are correct, rather than runningtscmanually
Files:
packages/convex-helpers/server/zod3.tspackages/convex-helpers/server/zod4.ts
🧬 Code graph analysis (2)
packages/convex-helpers/server/zod3.ts (3)
packages/convex-helpers/server/zod4.ts (1)
zodOutputToConvex(514-546)packages/convex-helpers/server/zod.ts (1)
zodOutputToConvex(238-242)packages/convex-helpers/validators.ts (1)
addFieldsToValidator(373-396)
packages/convex-helpers/server/zod4.ts (3)
packages/convex-helpers/server/zod3.ts (1)
zodOutputToConvex(1332-1466)packages/convex-helpers/server/zod.ts (1)
zodOutputToConvex(238-242)packages/convex-helpers/validators.ts (1)
addFieldsToValidator(373-396)
🪛 GitHub Check: Test and lint
packages/convex-helpers/server/zod4.ts
[failure] 939-939: server/zod4.functions.test.ts > zCustomQuery, zCustomMutation, zCustomAction > transform > calling a function with asynchronous transforms in arguments and return values
TypeError: Cannot read properties of undefined (reading 'length')
❯ eval ../../node_modules/zod/v4/core/doc.js:33:16
❯ ../../node_modules/zod/v4/core/schemas.js:880:34
❯ Object.inst._zod.parse ../../node_modules/zod/v4/core/schemas.js:905:23
❯ Module. ../../node_modules/zod/v4/core/parse.js:33:32
❯ _.inst.safeParse ../../node_modules/zod/v4/classic/schemas.js:28:46
❯ parseOrThrow server/zod4.ts:939:28
❯ handler server/zod4.ts:895:30
❯ invokeFunction ../../node_modules/convex/src/server/impl/registration_impl.ts:80:14
❯ invokeQuery ../../node_modules/convex/src/server/impl/registration_impl.ts:272:18
🔇 Additional comments (9)
packages/convex-helpers/server/zod4.ts (5)
852-858: LGTM!The destructuring correctly extracts the
skipConvexValidationflag with a safe default offalse.
865-868: LGTM!The conditional return validator correctly implements the
skipConvexValidationbehavior by skipping Convex validator generation when the flag is true.
870-909: LGTM!The args handling correctly implements the dual validation strategy: skipping Convex validation when the flag is set while still performing Zod validation via
parseOrThrow.
917-934: LGTM!The no-args branch correctly mirrors the validation logic from the args branch, ensuring consistent behavior.
938-948: No async keyword is needed for this function—it is correctly implemented as synchronous.The
parseOrThrowfunction properly handles validation errors by throwing aConvexErrorand returns parsed data on success. No changes are required.packages/convex-helpers/server/zod3.ts (4)
348-354: LGTM!The destructuring is consistent with zod4.ts and correctly implements the feature.
356-364: LGTM!The return validator logic correctly implements the
skipConvexValidationbehavior, consistent with zod4.ts.
366-405: LGTM!The args handling correctly implements the dual validation strategy, consistent with zod4.ts.
434-447: LGTM!The
parseOrThrowimplementation is correct for Zod v3, properly declared asasyncand usingparsed.error.errors(Zod v3) instead ofparsed.error.issues(Zod v4).
ianmacartney
force-pushed
the
ian/skip-convex-validation
branch
from
9746c04 to
05e1939
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/convex-helpers/server/zod4.functions.test.ts (1)
451-464: Test could be clearer: missing requiredageparameter.The test passes
{ name: 123 }but omits the requiredageparameter. While the test checks for anametype error, it's unclear whether the validation fails due to the wrong type fornameor the missingage, or both. For clarity, include all required fields with only the specific field under test being invalid.Apply this diff:
await expect( t.query(testApi.testQueryWithSkipConvexValidation, { name: 123, + age: 30, } as any), ).rejects.toThrowError(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/convex-helpers/CHANGELOG.md(1 hunks)packages/convex-helpers/package.json(1 hunks)packages/convex-helpers/server/zod4.functions.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/convex-helpers/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When modifying complex TypeScript types, run
npm run typecheckin the repository root to ensure types are correct, rather than runningtscmanually
Files:
packages/convex-helpers/server/zod4.functions.test.ts
🧬 Code graph analysis (1)
packages/convex-helpers/server/zod4.functions.test.ts (2)
convex/zodFunctionsExample.ts (1)
zQuery(6-6)packages/convex-helpers/server/setup.test.ts (1)
modules(2-2)
🔇 Additional comments (2)
packages/convex-helpers/CHANGELOG.md (1)
6-6: LGTM!The changelog entry clearly documents the fix for
skipConvexValidationbehavior.packages/convex-helpers/server/zod4.functions.test.ts (1)
298-298: LGTM!The testApi type correctly includes the new test function.
| returnBadData: z.boolean().default(false), | ||
| }, | ||
| handler: async (_ctx, args) => { | ||
| assertType<{ name: string; age: number }>(args); |
There was a problem hiding this comment.
Incorrect type assertion: missing returnBadData field.
The assertType should include returnBadData: boolean in the type. According to Zod 4's .default() behavior, fields with defaults are always present in the parsed output type, even though they're optional in the input type.
Apply this diff:
- assertType<{ name: string; age: number }>(args);
+ assertType<{ name: string; age: number; returnBadData: boolean }>(args);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertType<{ name: string; age: number }>(args); | |
| assertType<{ name: string; age: number; returnBadData: boolean }>(args); |
🤖 Prompt for AI Agents
In packages/convex-helpers/server/zod4.functions.test.ts around line 192, the
assertType generic is missing the returnBadData field; update the asserted type
to include returnBadData: boolean (i.e., assertType<{ name: string; age: number;
returnBadData: boolean }>(args)) so the test reflects Zod 4’s .default()
behavior where fields with defaults are always present in the parsed output.
| describe("skipConvexValidation", () => { | ||
| test("zCustomQuery with skipConvexValidation", async () => { | ||
| const t = convexTest(schema, modules); | ||
| const args = { | ||
| name: "Alice", | ||
| age: 30, | ||
| // Should get dropped | ||
| extraArg: "extraArg", | ||
| }; | ||
| const response = await t.query( | ||
| testApi.testQueryWithSkipConvexValidation, | ||
| args, | ||
| ); | ||
| expect(response).toMatchObject({ | ||
| message: "Hello Alice, you are 30 years old", | ||
| doubledAge: 60, | ||
| }); | ||
| }); | ||
| test("throwing ConvexError on zod arg validation", async () => { | ||
| const t = convexTest(schema, modules); | ||
| await expect( | ||
| t.query(testApi.testQueryWithSkipConvexValidation, { | ||
| name: 123, | ||
| } as any), | ||
| ).rejects.toThrowError( | ||
| expect.objectContaining({ | ||
| data: expect.stringMatching( | ||
| /(?=.*"ZodError")(?=.*"name")(?=.*"invalid_type")(?=.*"expected")(?=.*"string")/s, | ||
| ), | ||
| }), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for return value validation.
The testQueryWithSkipConvexValidation handler includes logic to return invalid data when returnBadData: true (lines 197-202), but there's no test that exercises this path. This leaves return validation untested when skipConvexValidation is enabled.
Consider adding a test case like:
test("throwing ConvexError on zod return validation", async () => {
const t = convexTest(schema, modules);
await expect(
t.query(testApi.testQueryWithSkipConvexValidation, {
name: "Alice",
age: 30,
returnBadData: true,
}),
).rejects.toThrowError(
expect.objectContaining({
data: expect.stringMatching(
/(?=.*"ZodError")(?=.*"doubledAge")(?=.*"invalid_type")(?=.*"expected")(?=.*"number")/s,
),
}),
);
});🤖 Prompt for AI Agents
In packages/convex-helpers/server/zod4.functions.test.ts around lines 433 to
465, add a test that exercises the handler's returnBadData path so return value
zod validation is covered: create a convexTest instance and call
t.query(testApi.testQueryWithSkipConvexValidation, { name: "Alice", age: 30,
returnBadData: true }) and assert it rejects with a ConvexError whose data
contains a ZodError mentioning the doubledAge field and number type (use a regex
similar to
/(?=.*"ZodError")(?=.*"doubledAge")(?=.*"invalid_type")(?=.*"expected")(?=.*"number")/s).
Fixes #865
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.