fix: validate zod schema before update operation is executed #1051
Conversation
This makes sure transformation rules like `@trim`, `@upper`, `@lower` are effective for updates.
WalkthroughWalkthroughThe recent updates focus on enhancing input validation logic, introducing new field validation directives, and refining data transformation rules across various operations. These changes aim to ensure data integrity and improve the robustness of create, update, and upsert operations with better handling of policy violations and permissions. Additionally, the introduction of directives like Changes
Assessment against linked issues
The changes align well with the objectives of enhancing data integrity and consistency through improved validation and transformation rules. However, there's an unclear indication of thorough testing for the update operation's transformation rules and a lack of documentation updates, which are critical for maintenance and future reference. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- packages/runtime/src/enhancements/policy/handler.ts (7 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/refactor.test.ts (14 hunks)
- tests/integration/tests/schema/refactor-pg.zmodel (3 hunks)
Additional comments: 19
tests/integration/tests/schema/refactor-pg.zmodel (3)
- 8-8: The addition of the
@lowerdirective to theUsermodel ensures that all email addresses are stored in lowercase, which is a good practice for consistency and avoiding case-sensitive issues during lookups or comparisons.- 55-55: Adding the
@trimdirective to thetitlefield in thePostmodel is a good practice to remove any leading or trailing whitespace. This can help in maintaining data consistency and avoiding unexpected behavior due to spaces.- 70-70: Similarly, the addition of the
@trimdirective to thecontentfield in theCommentmodel is beneficial for the same reasons mentioned for thePostmodel'stitlefield. It ensures that the content is stored without leading or trailing whitespace.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (5)
- 48-48: The addition of the
@lowerdirective to theslugfield in theTaskmodel, combined with the existing@regexdirective, ensures that slugs are stored in lowercase and adhere to the specified regex pattern. This is a good practice for URL slugs, enhancing consistency and predictability.- 511-526: The test case for creating
userDatawith the@trimand@lowerdirectives applied totext6and@upperdirective applied totext7correctly validates the transformation rules. It's good to see comprehensive testing of these directives to ensure they work as expected.- 528-537: The update operation test for
userDatathat checks the transformation oftext6andtext7with the@trim,@lower, and@upperdirectives also passes. This confirms the directives are correctly applied during update operations as well.- 579-593: The test for the upsert operation on the
tasksmodel, specifically testing the@lowerdirective on theslugfield, correctly demonstrates the directive's application in both creation and update scenarios. This is crucial for ensuring the PR objectives are met across different operation types.- 595-609: Similarly, the subsequent upsert operation test for updating the
slugfield in thetasksmodel validates the@lowerdirective's application. It's good to see thorough testing of these transformation rules in various scenarios.tests/integration/tests/enhancements/with-policy/refactor.test.ts (9)
- 216-229: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [208-226]
The test case for deeply nested creation success correctly trims the title of the post, aligning with the PR objectives to enforce Zod transformation rules. This test effectively validates the
@trimdirective's application in nested creation scenarios. However, ensure that similar transformation rules, such as@lowerfor email fields, are consistently applied and tested across all relevant scenarios to maintain data integrity and consistency.
- 409-418: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [401-415]
The test case for
createManywithskipDuplicatesoption demonstrates the correct application of the title trimming transformation rule. This aligns with the PR objectives and effectively tests the enforcement of Zod transformation rules in bulk creation scenarios. It's important to also verify that other transformation rules, if applicable to the data being tested, are enforced and tested in similar bulk operation scenarios.
- 427-438: The
createManytest case correctly applies the title trimming rule, validating the enforcement of Zod transformation rules in another bulk creation scenario. This consistency in applying and testing transformation rules across different operations is crucial for maintaining data integrity and consistency. Ensure that all transformation rules relevant to the data in question are similarly enforced and tested.- 583-586: The test case for updating nested posts includes both title trimming and updating other fields, demonstrating the correct application of transformation rules in nested update scenarios. This is in line with the PR objectives. To further enhance the test coverage, consider including scenarios where multiple transformation rules are applied simultaneously to different fields within the same update operation.
- 611-628: The test case for updating with create includes trimming of the comment content, aligning with the PR objectives to enforce Zod transformation rules. This test effectively validates the application of transformation rules in complex update scenarios involving nested creation. Ensure that the test suite comprehensively covers all relevant transformation rules across various nested and complex update scenarios to maintain consistency and data integrity.
- 660-668: The test case for updating with
createManycorrectly applies the title trimming rule, demonstrating the enforcement of Zod transformation rules in bulk update scenarios. This consistency in applying and testing transformation rules is crucial. To ensure comprehensive coverage, verify that all relevant transformation rules are enforced and tested in similar bulk operation scenarios, including updates and nested updates.- 748-754: The test case for updating a post's title with trimming demonstrates the correct application of transformation rules, aligning with the PR objectives. This test effectively validates the enforcement of Zod transformation rules in update scenarios. It's important to ensure that similar transformation rules are consistently applied and tested across all relevant update scenarios to maintain data integrity and consistency.
- 825-832: The test case for updating with upsert correctly applies the title trimming rule during the update operation, aligning with the PR objectives. This demonstrates the effective enforcement of Zod transformation rules in upsert scenarios. Ensure that the test suite comprehensively covers the application of all relevant transformation rules in upsert operations, both in the update and create paths, to maintain data integrity and consistency.
- 1120-1123: The test case for
updateManycorrectly applies the title trimming rule, aligning with the PR objectives to enforce Zod transformation rules in bulk update scenarios. This consistency in applying and testing transformation rules across different operations is crucial for maintaining data integrity. Ensure that all transformation rules relevant to the data in question are similarly enforced and tested in bulk update scenarios.packages/runtime/src/enhancements/policy/handler.ts (2)
- 792-798: The
validateUpdateInputSchemamethod is correctly used to validate update payloads. However, ensure that the method is capable of handling complex nested update scenarios where non-literal fields might be present.Consider adding tests or documentation examples that cover complex update scenarios, including nested updates and updates with non-literal fields (e.g.,
{ increment: 1 }).
- 1034-1062: The implementation of
validateUpdateInputSchemamethod is robust, correctly filtering out non-literal fields before validation. This approach is essential for update operations where Prisma's special update operations (e.g.,increment) are used. However, ensure that all potential non-literal fields are accounted for and that this method is consistently applied across all update operations.
| @@ -467,7 +467,7 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr | |||
| // Validates the given create payload against Zod schema if any | |||
| private validateCreateInputSchema(model: string, data: any) { | |||
| const schema = this.utils.getZodSchema(model, 'create'); | |||
| if (schema) { | |||
| if (schema && data) { | |||
There was a problem hiding this comment.
The validateCreateInputSchema method correctly applies Zod schema validation for create operations. However, consider adding error handling for cases where Zod schema is not defined but expected, to prevent silent failures.
if (schema && data) {
+ // Existing logic
} else {
+ throw new Error(`Zod schema for model '${model}' and operation 'create' not found.`);
}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.
| if (schema && data) { | |
| if (schema && data) { | |
| // Existing logic | |
| } else { | |
| throw new Error(`Zod schema for model '${model}' and operation 'create' not found.`); | |
| } |
| // go through create items, statically check input to determine if post-create | ||
| // check is needed, and also validate zod schema | ||
| let needPostCreateCheck = false; | ||
| for (const item of enumerate(args.data)) { | ||
| const validationResult = this.validateCreateInputSchema(this.model, item); | ||
| if (validationResult !== item) { | ||
| this.utils.replace(item, validationResult); | ||
| } | ||
|
|
||
| const inputCheck = this.utils.checkInputGuard(this.model, item, 'create'); | ||
| if (inputCheck === false) { | ||
| // unconditionally deny | ||
| throw this.utils.deniedByPolicy( | ||
| this.model, | ||
| 'create', | ||
| undefined, | ||
| CrudFailureReason.ACCESS_POLICY_VIOLATION | ||
| ); | ||
| } else if (inputCheck === true) { | ||
| const r = this.validateCreateInputSchema(this.model, item); | ||
| if (r !== item) { | ||
| this.utils.replace(item, r); | ||
| } | ||
| // unconditionally allow |
There was a problem hiding this comment.
The loop for validating and checking input guards in createMany method is well-implemented. However, consider extracting this logic into a separate method to improve readability and maintainability, especially since similar patterns are used in other parts of the class.
+ private validateAndCheckInputGuards(model: string, data: any[]) {
+ let needPostCreateCheck = false;
+ for (const item of data) {
+ // Existing validation and check logic here
+ }
+ return needPostCreateCheck;
+ }
- // Original loop logic hereCommittable 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.
| // go through create items, statically check input to determine if post-create | |
| // check is needed, and also validate zod schema | |
| let needPostCreateCheck = false; | |
| for (const item of enumerate(args.data)) { | |
| const validationResult = this.validateCreateInputSchema(this.model, item); | |
| if (validationResult !== item) { | |
| this.utils.replace(item, validationResult); | |
| } | |
| const inputCheck = this.utils.checkInputGuard(this.model, item, 'create'); | |
| if (inputCheck === false) { | |
| // unconditionally deny | |
| throw this.utils.deniedByPolicy( | |
| this.model, | |
| 'create', | |
| undefined, | |
| CrudFailureReason.ACCESS_POLICY_VIOLATION | |
| ); | |
| } else if (inputCheck === true) { | |
| const r = this.validateCreateInputSchema(this.model, item); | |
| if (r !== item) { | |
| this.utils.replace(item, r); | |
| } | |
| // unconditionally allow | |
| private validateAndCheckInputGuards(model: string, data: any[]) { | |
| let needPostCreateCheck = false; | |
| for (const item of data) { | |
| // Existing validation and check logic here | |
| } | |
| return needPostCreateCheck; | |
| } |
| args.data = this.validateUpdateInputSchema(this.model, args.data); | ||
|
|
There was a problem hiding this comment.
The use of validateUpdateInputSchema in updateMany method is appropriate. However, consider adding a comment explaining the rationale behind validating only literal fields, as this might not be immediately clear to other developers or future maintainers.
+ // Validate only literal fields to ensure compatibility with Prisma's special update operations
args.data = this.validateUpdateInputSchema(this.model, args.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.
| args.data = this.validateUpdateInputSchema(this.model, args.data); | |
| // Validate only literal fields to ensure compatibility with Prisma's special update operations | |
| args.data = this.validateUpdateInputSchema(this.model, args.data); |
| data: { id: 1, email: 'User1@zenstack.dev' }, | ||
| }) | ||
| ).rejects.toThrow(/not allowed to be read back/); | ||
|
|
||
| // success | ||
| await expect(user1Db.user.findUnique({ where: { id: 1 } })).toResolveTruthy(); | ||
| await expect(user1Db.user.findUnique({ where: { id: 1 } })).resolves.toMatchObject({ | ||
| // email to lower | ||
| email: '[email protected]', | ||
| }); |
There was a problem hiding this comment.
The test case for user creation correctly enforces email casing rules as per the PR objectives. However, it's crucial to ensure that the rejection message for the .rejects.toThrow assertion is specific enough to accurately reflect the policy violation related to email formatting or other validation rules. This specificity aids in understanding the test's intent and ensures that the test fails for the right reasons.
Consider enhancing the rejection message to explicitly mention the reason for the policy violation, such as invalid email format or other specific validation failures.
| adminDb.user.update({ where: { id: 1 }, data: { email: 'User1[email protected]' } }) | ||
| ).resolves.toMatchObject({ email: '[email protected]' }); |
There was a problem hiding this comment.
The test case for updating a single user's email demonstrates the correct application of the email casing rule, aligning with the PR objectives. However, it's essential to ensure that the test also covers scenarios where transformation rules might be violated, such as attempting to update with an improperly formatted email, to fully validate the robustness of the implementation.
Consider adding additional assertions or test cases to cover scenarios where updates might violate transformation rules, ensuring comprehensive testing of the enforcement mechanisms.
This makes sure transformation rules like
@trim,@upper,@lowerare effective for updates.Fixes #1005
Summary by CodeRabbit
New Features
@lowerdirective for field validation to enforce lowercasing.@trimdirective to fields for automatic trimming of whitespace.Refactor
Tests