fix: properly handle missing fields when evaluating @@validate model-level rules
#1097
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…l-level rules Fixes #1078
WalkthroughWalkthroughThe recent updates focus on enhancing validation capabilities within the schema generation and transformation framework. A notable addition is the Changes
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 (
|
|
Fixes #1078 |
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/schema/src/plugins/zod/utils/schema-gen.ts (2 hunks)
- packages/schema/src/utils/typescript-expression-transformer.ts (5 hunks)
- packages/testtools/src/schema.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1 hunks)
- tests/integration/tests/regression/issue-1078.test.ts (1 hunks)
Additional comments: 7
tests/integration/tests/regression/issue-1078.test.ts (1)
- 32-51: The 'read' test case appears to focus on testing permissions (
@allowattribute) rather than the handling of missing fields in model-level validations. While it's important to ensure comprehensive testing, this test case might be slightly off-topic for the PR's main objective. Consider the following:
- Clarify the relevance of this test case to the PR objectives if it indirectly relates to the handling of missing fields in validations.
- If the test case is primarily focused on permissions rather than validation, it might be beneficial to document its purpose clearly or move it to a more relevant test suite focused on permissions.
packages/testtools/src/schema.ts (1)
- 108-108: Uncommenting the
preserveTsFilessetting and setting it totruein thezodplugin configuration is a positive change for enhancing testing and debugging capabilities. This allows for the preservation of TypeScript files, which can be beneficial for more detailed analysis and troubleshooting.
- Ensure that this change is consistently applied across all relevant test configurations to maintain uniformity in testing practices.
- Consider documenting the implications of this setting for developers, especially regarding how it affects the build and testing process.
packages/schema/src/utils/typescript-expression-transformer.ts (4)
- 20-20: The addition of
isDataModelFieldReferenceto the import list from@zenstackhq/sdkis crucial for the new logic introduced in this file to handle missing fields correctly during model-level validation. This utility function will be used to check if a given expression references a data model field, which is essential for the changes made in theensureBoolean,unary, andtransformBinarymethods.- 261-267: The modification to the
ensureBooleanmethod to handleExpressionContext.ValidationRuledifferently is a significant improvement. By treatingundefinedastruein a validation context, it aligns with the principle that missing fields should not automatically cause a validation rule to fail. This change ensures that validation rules are applied more intuitively, especially in scenarios where optional fields are involved.- 309-320: The update to the
unarymethod to handle specific cases based on context and operand, particularly for the!operator inExpressionContext.ValidationRule, is well thought out. By treating unary operations involvingundefinedastruein a validation context, it further enhances the handling of missing fields. This adjustment ensures that negation operations in validation rules behave as expected when fields are missing.- 336-346: The adjustments made in the
transformBinarymethod to handle binary expressions differently based onExpressionContext.ValidationRuleare crucial for correctly evaluating model-level validation rules. By treating binary operations involvingundefinedastruein a validation context, it ensures that comparisons and logical operations in validation rules are interpreted correctly when fields are missing. This change is essential for achieving the desired behavior in model-level validations.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (1)
- 613-765: The addition of comprehensive tests for model-level validation in the context of the With Policy feature is a significant enhancement. These tests cover a wide range of scenarios, including validation rules involving optional fields, boolean values, comparisons, complex expressions, and negation. By ensuring that these scenarios are thoroughly tested, the changes contribute to the robustness and reliability of the validation mechanism. The tests are well-structured and clearly demonstrate the intended behavior of the validation rules under various conditions.
Comment on lines
+3
to
+30
| describe('issue 1078', () => { | ||
| it('regression', async () => { | ||
| const { prisma, enhance } = await loadSchema( | ||
| ` | ||
| model Counter { | ||
| id String @id | ||
|
|
||
| name String | ||
| value Int | ||
|
|
||
| @@validate(value >= 0) | ||
| @@allow('all', true) | ||
| } | ||
| ` | ||
| ); | ||
|
|
||
| const db = enhance(); | ||
|
|
||
| const counter = await db.counter.create({ | ||
| data: { id: '1', name: 'It should create', value: 1 }, | ||
| }); | ||
|
|
||
| //! This query fails validation | ||
| const updated = await db.counter.update({ | ||
| where: { id: '1' }, | ||
| data: { name: 'It should update' }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The 'regression' test case effectively demonstrates the handling of missing fields during model-level validation, specifically testing the scenario where the 'value' field is omitted in an update operation. This aligns well with the PR objectives of ensuring that missing fields are correctly handled in validation rules.
- Ensure that the expected behavior (e.g., passing or failing validation) is explicitly asserted in the test. Currently, the test performs operations but does not assert the outcome. Adding assertions will make the test more robust and clear about the expected behavior.
- Consider adding negative test cases where the validation should fail, to ensure that the validation logic is not overly permissive.
Comment on lines
+209
to
+218
| let expr = new TypeScriptExpressionTransformer({ | ||
| context: ExpressionContext.ValidationRule, | ||
| fieldReferenceContext: 'value', | ||
| }).transform(valueArg); | ||
|
|
||
| if (isDataModelFieldReference(valueArg)) { | ||
| // if the expression is a simple field reference, treat undefined | ||
| // as true since the all fields are optional in validation context | ||
| expr = `${expr} ?? true`; | ||
| } |
There was a problem hiding this comment.
The update to the makeValidationRefinements function introduces a check for simple field references using isDataModelFieldReference. This logic treats undefined fields as true in the validation context, which aligns with the PR objectives of correctly handling missing fields in model-level validations.
- Ensure that this change does not inadvertently affect other validation scenarios where
undefinedshould not be treated astrue. It might be beneficial to add more tests covering various validation scenarios to ensure the change's correctness and prevent regressions. - Consider documenting this behavior in the code comments or the project's documentation to make it clear to other developers how
undefinedfields are treated in validation rules.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/integration/tests/cli/plugins.test.ts (1 hunks)
Additional comments: 1
tests/integration/tests/cli/plugins.test.ts (1)
- 119-119: The addition of
skipLibCheck: trueto the TypeScript compiler options can significantly speed up the compilation process by skipping type checks on declaration files from external libraries. This is particularly beneficial in test environments where the focus is more on the project's code rather than external dependencies. However, it's important to ensure that this does not inadvertently mask potential type-related issues with dependencies.
- Ensure that this change aligns with the project's overall TypeScript configuration strategy and does not conflict with the need for thorough type checking in other contexts.
- Consider periodically reviewing the necessity of this option, especially if significant changes are made to the project's dependencies or TypeScript version.
This was referenced Mar 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
undefinedvalues.zodplugin configuration for better debugging and tooling support.