fix(zod): "strip" mode causes create payload fields to be accidentally dropped #1747
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new utility file Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (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: 6
🧹 Outside diff range and nitpick comments (9)
packages/schema/src/plugins/zod/types.ts (1)
30-30: Well-definedObjectModetype.The
ObjectModetype is a great addition that provides type safety and clarity for specifying the transformer's behavior. It directly addresses the PR's focus on the "strip" mode while also offering flexibility with 'strict' and 'passthrough' options.Consider adding a brief comment above the type definition to explain the purpose of each mode. This would enhance code readability and maintainability. For example:
/** * Defines the mode of operation for object transformation: * - 'strict': Throws an error on unknown properties * - 'strip': Removes unknown properties * - 'passthrough': Allows unknown properties */ export type ObjectMode = 'strict' | 'strip' | 'passthrough';packages/schema/src/plugins/zod/generator.ts (1)
44-56: LGTM: Constructor updated with mode validation and initialization.The constructor has been updated to include validation for the
modeoption and initialization of themodeproperty. The changes look good and improve the robustness of the class.Consider extracting the allowed mode values into a constant array for better maintainability. For example:
const ALLOWED_MODES = ['strip', 'strict', 'passthrough'] as const; // ... if ( this.options.mode && (typeof this.options.mode !== 'string' || !ALLOWED_MODES.includes(this.options.mode as any)) ) { throw new PluginError( name, `Invalid mode option: "${this.options.mode}". Must be one of ${ALLOWED_MODES.join(', ')}.` ); }This approach would make it easier to update the allowed modes in the future and ensure consistency throughout the code.
packages/runtime/src/zod-utils.ts (2)
13-52: Evaluate performance impact of multiple schema evaluationsThe
smartUnionfunction evaluates multiple candidate schemas to determine the one with the fewest unrecognized keys. While this approach improves accuracy in schema selection, it may impact performance, especially with a large number of candidates or complex schemas.Consider profiling the performance and, if necessary, optimizing the candidate selection process to balance accuracy and efficiency.
54-117: Enhance error handling for unexpected inputsFunctions like
smartArrayUnion,smartObjectUnion, andcountUnrecognizedKeysmay encounter unexpected inputs or edge cases. Improving error handling and input validation can make the code more robust and easier to debug.tests/regression/tests/issue-1746.test.ts (5)
62-65: Consider adding assertions for thecontentfield.In this test case, you assert the values of
userId,submissionId, and verify thatunknownis undefined. To ensure comprehensive test coverage, consider adding an assertion forcontentto confirm it is correctly parsed.
76-79: Add assertion for thecontentfield in the checked scenario.Similar to the previous test, including an assertion for
contentwill enhance the test's completeness by verifying all expected fields.
99-101: Include assertion for thecontentfield in nested create schema tests.In this nested create schema test, you assert
userIdand check thatunknownis undefined. Adding an assertion forcontentwill provide a more thorough verification of the parsed data.
115-118: Assert all elements in the unchecked array test.Currently, assertions are made only for the first element in the array. To ensure all items are correctly parsed, consider adding assertions for the second element as well.
135-138: Extend assertions to all array elements in the checked array test.Similar to the previous suggestion, including assertions for both elements in the array will enhance the test's robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/runtime/package.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
- packages/runtime/src/zod-utils.ts (1 hunks)
- packages/schema/src/plugins/zod/generator.ts (5 hunks)
- packages/schema/src/plugins/zod/transformer.ts (11 hunks)
- packages/schema/src/plugins/zod/types.ts (2 hunks)
- tests/regression/tests/issue-1746.test.ts (1 hunks)
🔇 Additional comments (15)
packages/schema/src/plugins/zod/types.ts (2)
17-17: Excellent addition to address the "strip" mode issue.The inclusion of the
modeproperty inTransformerParamsis a crucial change that directly addresses the PR's objective. This addition will allow for better control over how object properties are handled, particularly in "strip" mode, which should resolve the issue of accidentally dropped fields.
Line range hint
1-30: Verify implementation in related files.The changes in this file provide a solid foundation for addressing the "strip" mode issue. To ensure full resolution of the problem:
Please run the following script to check the usage of the new
ObjectModetype andmodeproperty in other relevant files:This will help ensure that the changes are properly implemented throughout the Zod plugin.
packages/schema/src/plugins/zod/generator.ts (5)
26-26: LGTM: Import statement for ObjectMode added.The import statement for
ObjectModefrom './types' has been correctly added. This import is necessary for the newmodeproperty in theZodSchemaGeneratorclass.
32-32: LGTM: Mode property added to ZodSchemaGenerator class.The
modeproperty has been correctly added as a private readonly property of typeObjectMode. This addition aligns with the class's purpose and maintains proper encapsulation.
223-223: LGTM: Mode property passed to Transformer in generateEnumSchemas.The
modeproperty has been correctly added to theTransformerconstructor call in thegenerateEnumSchemasmethod. This ensures consistent behavior across the schema generation process.
252-252: LGTM: Mode property passed to Transformer in generateObjectSchemas.The
modeproperty has been correctly added to theTransformerconstructor call in thegenerateObjectSchemasmethod. This change is consistent with the modification ingenerateEnumSchemasand ensures proper mode handling during object schema generation.
Line range hint
1-653: Overall assessment: Changes effectively address the PR objectives.The modifications to the
ZodSchemaGeneratorclass, including the addition of themodeproperty and its usage throughout the class, effectively address the issue mentioned in the PR objectives. The "strip" mode problem with create payload fields being accidentally dropped should now be resolved.These changes provide more flexibility in schema generation based on the specified mode, allowing for better control over how Zod schemas are generated and validated. The consistent application of the
modeproperty across different schema generation methods ensures a cohesive approach to handling the various modes (strip, strict, passthrough).The code changes are well-implemented, maintain good coding practices, and improve the overall functionality of the Zod plugin.
tests/regression/tests/issue-1746.test.ts (1)
54-148: Tests effectively validate schema behavior with 'strip' mode.The test cases comprehensively cover various scenarios, ensuring that unknown fields are stripped and that required fields are properly validated in both checked and unchecked modes.
packages/schema/src/plugins/zod/transformer.ts (7)
10-10: Import statement updated correctly.The required types
AggregateOperationSupport,ObjectMode, andTransformerParamsare correctly imported from'./types'.
31-31: 'mode' property added and initialized correctly.The
'mode'property is added to theTransformerclass and properly initialized in the constructor usingparams.mode.Also applies to: 44-44
78-83: Conditional import of 'smartUnion' implemented correctly.The
generateImportZodStatementmethod now conditionally imports thesmartUnionhelper whenmodeis set to'strip', ensuring necessary functionality is available.
220-232: Updated union generation logic to handle 'Unchecked' types appropriately.The logic correctly utilizes
wrapWithSmartUnionwhenalternativesinclude'Unchecked'types, ensuring that the best candidate is parsed at runtime.
435-441: 'wrapWithSmartUnion' method implemented correctly.The
wrapWithSmartUnionmethod abstracts union creation, usingsmartUnionwhen in'strip'mode and defaulting toz.unionotherwise.
482-482: Import statements updated appropriately.The
generateImportZodStatementmethod is correctly included in theimportsarray, ensuring necessary imports are generated.
Line range hint
541-548: Data schema generation updated to use 'wrapWithSmartUnion'.The data schemas for create, update, and upsert operations now use
wrapWithSmartUnionwhengenerateUncheckedistrue, correctly combining checked and unchecked input types.Also applies to: 589-592, 610-613, 633-636, 639-642
| @@ -0,0 +1,121 @@ | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid disabling ESLint rules and eliminate any types
Disabling @typescript-eslint/no-explicit-any might hide potential type safety issues. It's recommended to avoid using any and instead provide explicit type definitions to ensure full type safety and maintainability.
| const schema = candidates.find((candidate) => candidate.element === itemSchema); | ||
| return schema!.safeParse(data); |
There was a problem hiding this comment.
Handle undefined schema to prevent runtime errors
In smartArrayUnion, the schema obtained from .find() may be undefined if no matching candidate is found. Using schema! assumes it's always defined, which can lead to runtime errors.
Add a check to ensure schema is defined before proceeding:
const schema = candidates.find((candidate) => candidate.element === itemSchema);
+if (!schema) {
+ return { data: undefined, success: false };
+}
return schema.safeParse(data);📝 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.
| const schema = candidates.find((candidate) => candidate.element === itemSchema); | |
| return schema!.safeParse(data); | |
| const schema = candidates.find((candidate) => candidate.element === itemSchema); | |
| if (!schema) { | |
| return { data: undefined, success: false }; | |
| } | |
| return schema.safeParse(data); |
| const strictResults = candidates.map((candidate) => { | ||
| // make sure to strip `z.lazy` before parsing | ||
| const unwrapped = unwrapLazy(z, candidate); | ||
| return { | ||
| schema: candidate, | ||
| // force object schema to run in strict mode to capture unrecognized keys | ||
| result: unwrapped.strict().safeParse(data), | ||
| }; | ||
| }); | ||
|
|
||
| // find the schema with the fewest unrecognized keys | ||
| const { schema } = strictResults.sort((a, b) => { | ||
| const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []); | ||
| const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []); | ||
| return aCount - bCount; | ||
| })[0]; | ||
| return schema; | ||
| } |
There was a problem hiding this comment.
Ensure consistency between unwrapped and returned schemas
In identifyCandidate, you unwrapped candidate to unwrapped for parsing but return the original candidate in schema, which may still be a ZodLazy. This inconsistency can cause issues if the returned schema is used without unwrapping.
Modify the return object to include the unwrapped schema:
return {
- schema: candidate,
+ schema: unwrapped,
result: unwrapped.strict().safeParse(data),
};This change ensures that the schema used for parsing and the one returned are the same, preventing potential errors when the schema is used later.
📝 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.
| const strictResults = candidates.map((candidate) => { | |
| // make sure to strip `z.lazy` before parsing | |
| const unwrapped = unwrapLazy(z, candidate); | |
| return { | |
| schema: candidate, | |
| // force object schema to run in strict mode to capture unrecognized keys | |
| result: unwrapped.strict().safeParse(data), | |
| }; | |
| }); | |
| // find the schema with the fewest unrecognized keys | |
| const { schema } = strictResults.sort((a, b) => { | |
| const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []); | |
| const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []); | |
| return aCount - bCount; | |
| })[0]; | |
| return schema; | |
| } | |
| const strictResults = candidates.map((candidate) => { | |
| // make sure to strip `z.lazy` before parsing | |
| const unwrapped = unwrapLazy(z, candidate); | |
| return { | |
| schema: unwrapped, | |
| // force object schema to run in strict mode to capture unrecognized keys | |
| result: unwrapped.strict().safeParse(data), | |
| }; | |
| }); | |
| // find the schema with the fewest unrecognized keys | |
| const { schema } = strictResults.sort((a, b) => { | |
| const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []); | |
| const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []); | |
| return aCount - bCount; | |
| })[0]; | |
| return schema; | |
| } |
| let resultData: any; | ||
|
|
||
| return z | ||
| .custom((data) => { | ||
| if (Array.isArray(data)) { | ||
| const { data: result, success } = smartArrayUnion( | ||
| z, | ||
| processedCandidates.filter((c) => c instanceof z.ZodArray), | ||
| data | ||
| ); | ||
| if (success) { | ||
| resultData = result; | ||
| } | ||
| return success; | ||
| } else { | ||
| const { data: result, success } = smartObjectUnion( | ||
| z, | ||
| processedCandidates.filter((c) => c instanceof z.ZodObject), | ||
| data | ||
| ); | ||
| if (success) { | ||
| resultData = result; | ||
| } | ||
| return success; | ||
| } | ||
| }) | ||
| .transform(() => { | ||
| // return the parsed data | ||
| return resultData; | ||
| }); |
There was a problem hiding this comment.
Prevent mutating external state within Zod schemas
The use of an external variable resultData that's modified inside the .custom() method and then accessed in .transform() can lead to unexpected behaviors, especially in concurrent scenarios. Modifying external state inside schema parsing functions may cause race conditions or data inconsistency.
Consider refactoring to eliminate external state mutation by directly returning the parsed data in the .transform() method.
Here's a suggested refactor:
export function smartUnion(z: typeof Z, candidates: Z.ZodSchema[]) {
// ... [code omitted for brevity]
- let resultData: any;
return z
.custom((data) => {
- if (Array.isArray(data)) {
- const { data: result, success } = smartArrayUnion(
+ let parseResult;
+ if (Array.isArray(data)) {
+ const result = smartArrayUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodArray),
data
);
- if (success) {
- resultData = result;
+ if (result.success) {
+ parseResult = result.data;
return true;
}
return false;
} else {
- const { data: result, success } = smartObjectUnion(
+ const result = smartObjectUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodObject),
data
);
- if (success) {
- resultData = result;
+ if (result.success) {
+ parseResult = result.data;
return true;
}
return false;
}
})
.transform(() => {
- // return the parsed data
- return resultData;
+ return parseResult;
});
}📝 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.
| let resultData: any; | |
| return z | |
| .custom((data) => { | |
| if (Array.isArray(data)) { | |
| const { data: result, success } = smartArrayUnion( | |
| z, | |
| processedCandidates.filter((c) => c instanceof z.ZodArray), | |
| data | |
| ); | |
| if (success) { | |
| resultData = result; | |
| } | |
| return success; | |
| } else { | |
| const { data: result, success } = smartObjectUnion( | |
| z, | |
| processedCandidates.filter((c) => c instanceof z.ZodObject), | |
| data | |
| ); | |
| if (success) { | |
| resultData = result; | |
| } | |
| return success; | |
| } | |
| }) | |
| .transform(() => { | |
| // return the parsed data | |
| return resultData; | |
| }); | |
| return z | |
| .custom((data) => { | |
| let parseResult; | |
| if (Array.isArray(data)) { | |
| const result = smartArrayUnion( | |
| z, | |
| processedCandidates.filter((c) => c instanceof z.ZodArray), | |
| data | |
| ); | |
| if (result.success) { | |
| parseResult = result.data; | |
| return true; | |
| } | |
| return false; | |
| } else { | |
| const result = smartObjectUnion( | |
| z, | |
| processedCandidates.filter((c) => c instanceof z.ZodObject), | |
| data | |
| ); | |
| if (result.success) { | |
| parseResult = result.data; | |
| return true; | |
| } | |
| return false; | |
| } | |
| }) | |
| .transform(() => { | |
| return parseResult; | |
| }); |
| unknown: 'unknown', | ||
| }, | ||
| }); | ||
| expect(parsed.success).toBe(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the error details when parsing fails.
Since parsed.success is expected to be false, consider asserting the specific error messages to ensure that the failure is due to mixing checked and unchecked fields as intended.
| { userId: '1', content: 'Content2', unknown: 'unknown2' }, | ||
| ], | ||
| }); | ||
| expect(parsed.success).toBe(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check error details for mixed input parsing failure.
When parsing fails for mixed inputs, adding assertions for the error messages will help confirm that the schema correctly identifies the inconsistency between checked and unchecked fields.
Fixes #1746