fix: @@validate should ignore fields that are not present
#1104
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
WalkthroughWalkthroughThese updates encompass enhancements and refactoring across schema validation and expression evaluation functionalities. The focus is on improving optional field handling, model field references, and boolean expressions in validation contexts. New utility methods and tests aim to strengthen validation logic, especially concerning optional chaining, array functions, and scalar functions. The changes strive to enhance the validation process for better clarity and accuracy, addressing the intricacies of data modeling and validation expressions. 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 (
|
![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: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- packages/schema/src/language-server/validator/attribute-application-validator.ts (2 hunks)
- packages/schema/src/language-server/validator/expression-validator.ts (4 hunks)
- packages/schema/src/utils/ast-utils.ts (2 hunks)
- packages/schema/src/utils/typescript-expression-transformer.ts (8 hunks)
- packages/schema/tests/generator/expression-writer.test.ts (9 hunks)
- packages/schema/tests/schema/validation/attribute-validation.test.ts (4 hunks)
- tests/integration/tests/enhancements/with-policy/field-validation.test.ts (4 hunks)
Additional comments: 27
packages/schema/src/utils/ast-utils.ts (1)
- 171-183: The implementation of
findUpAstfunction is correct and follows best practices for traversing AST nodes. It's a useful utility for scenarios where upward traversal is needed to find a node that meets specific criteria. This addition enhances the AST utilities by providing a flexible way to navigate the AST structure.One minor suggestion for improvement is to add more detailed documentation or examples on how to use this function effectively, especially in complex AST scenarios. This could help developers understand its utility and potential use cases more clearly.
packages/schema/src/language-server/validator/expression-validator.ts (4)
- 127-136: The logic within the
isInValidationContextcheck for handling null comparisons against data model fields is sound and aligns with the PR's objective to improve handling of optional fields. This change ensures that fields can be compared against null without causing validation errors in contexts where fields are optional.- 147-164: The refactoring of model field comparison logic to exclude comparisons between fields of different models unless specific conditions are met (e.g., not being a model field expression) is a significant improvement. It helps prevent unintended validation behavior and ensures that comparisons are made within the expected context.
- 226-228: The addition of the
isInValidationContextmethod is a strategic enhancement that enables the validator to determine if the current validation is occurring within a context where the@@validateattribute is applied. This method improves the flexibility and accuracy of expression validation by considering the context in which validation rules are applied.- 231-233: The
isNotModelFieldExprmethod is a valuable addition for identifying expressions that do not represent model field references. This method enhances the validator's ability to handle various expression types and supports the overall goal of improving validation logic, especially in complex scenarios involving optional fields and boolean expressions.packages/schema/src/language-server/validator/attribute-application-validator.ts (1)
- 154-165: The addition of the
_checkValidatemethod is a crucial enhancement to the attribute validation logic. It correctly identifies and prevents the use of relation fields in the@@validatecondition, aligning with the PR's objective to refine validation logic. This change ensures that validation conditions are applied correctly and only to appropriate fields, enhancing the system's reliability and integrity.One suggestion for improvement is to consider adding more detailed error messages or examples in the documentation to help developers understand the constraints around using relation fields in validation conditions.
packages/schema/src/utils/typescript-expression-transformer.ts (6)
- 172-178: The update to the
_lengthfunction to useensureBooleanTernaryfor handling boolean expressions related to field lengths is a thoughtful improvement. It ensures that optional fields are correctly handled in validation contexts, aligning with the PR's objectives. This change enhances the system's ability to accurately validate expressions involving field lengths, especially for optional fields.- 212-212: The update to the
_regexfunction to useensureBooleanfor handling regex match results is appropriate and enhances the expression transformation logic. This change ensures that regex validations are correctly interpreted as boolean expressions, improving the accuracy and reliability of validation rules involving regex patterns.- 218-218: The update to the
_emailfunction to useensureBooleanTernaryfor handling email validation expressions is a significant improvement. It correctly handles optional fields in validation contexts, ensuring that email validations are accurately performed even when fields are not provided. This change aligns with the PR's objectives to improve the handling of optional fields in validation scenarios.- 270-271: The implementation of the
ensureBooleanmethod to treat undefined values astrueorfalsebased on the validation context is a strategic enhancement. It ensures that boolean expressions are correctly interpreted in validation scenarios, especially when dealing with optional fields. This method enhances the flexibility and accuracy of the expression transformation process in validation contexts.- 274-280: The introduction of the
ensureBooleanTernarymethod is a key enhancement to the expression transformation logic. It provides a flexible way to handle boolean expressions involving optional fields in validation contexts, ensuring that undefined values are correctly interpreted astrueorfalse. This method significantly improves the system's ability to accurately process and validate expressions, especially in complex scenarios involving optional fields.- 411-423: The addition of the
extractNullComparisonmethod is a valuable enhancement for managing comparisons involvingnullorundefined. This method simplifies the handling of null comparisons in validation contexts, ensuring that expressions are accurately evaluated. It aligns with the PR's objectives to improve the handling of optional fields and complex validation scenarios.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (6)
- 2-2: The addition of
createPostgresDbanddropPostgresDbfunctions to the import statement aligns with the PR's objective to enhance the validation logic and testing capabilities. This change suggests that some tests may involve database operations requiring setup and teardown, which these functions facilitate.- 688-688: Renaming the test from
'optionality with comparison'to'optionality with binary'clarifies the test's intent, making it more understandable that the test is specifically about binary operations rather than general comparisons. This change improves the readability and maintainability of the test suite.- 708-723: The introduction of tests for optionality with the
inoperator for both left-hand side and right-hand side comparisons is a valuable addition. These tests ensure that the validation logic correctly handles scenarios where fields may or may not be present and are being checked against a set of values using theinoperator. This aligns with the PR's objective to enhance the system's validation capabilities, especially in relation to optional fields.- 816-870: The addition of tests for optionality with scalar functions like
length,contains,startsWith,endsWith,regex,url, anddatetimeis commendable. These tests comprehensively cover the validation logic's behavior with various scalar functions, ensuring that the system accurately validates expressions involving optional fields. This thorough testing approach significantly contributes to the robustness and reliability of the schema management system's validation capabilities.- 873-907: The inclusion of tests for optionality with array functions like
has,hasEvery,hasSome, and checking for non-empty arrays is an excellent addition to the test suite. These tests ensure that the validation logic accurately handles scenarios involving optional array fields and various array-related functions. This addition enhances the system's validation capabilities, particularly in relation to complex validation scenarios involving arrays.- 914-938: The addition of tests for null comparisons in model fields is a critical enhancement to the test suite. These tests verify that the validation logic correctly handles scenarios where fields may be null, ensuring that the system's behavior aligns with the intended functionality of the
@@validateattribute in relation to nullability. This contributes to the system's ability to handle complex validation scenarios involving null values, thereby improving the accuracy and reliability of the schema management system.packages/schema/tests/generator/expression-writer.test.ts (9)
- 1181-1181: The addition of parentheses around the conditional expression involving
Role.ADMINanduser?.roles?.includesis a good practice for clarity. However, ensure that the logic correctly reflects the intended behavior, especially in the context of optional chaining and logical operations.- 1208-1208: Adding parentheses to improve the readability of the conditional expression involving
user?.email?.includes('test')is beneficial. It's crucial to verify that these changes do not alter the original logic, particularly when dealing with optional chaining and string operations.- 1221-1221: The modification to include parentheses and explicitly handle case insensitivity in the expression
user?.email?.toLowerCase().includes('test'?.toLowerCase())enhances readability and correctness. Ensure that this change accurately reflects the intended case-insensitive comparison without introducing any side effects.- 1234-1234: The addition of parentheses in the expression
user?.email?.startsWith('test')is a good practice. It's important to confirm that this change does not inadvertently modify the intended logic, especially considering the nuances of string manipulation and optional chaining.- 1247-1247: Incorporating parentheses to clarify the expression
user?.email?.endsWith('test')is beneficial. Verify that this adjustment accurately captures the intended logic of checking the end of a string, taking into account optional chaining and the specificities of string operations.- 1260-1260: The use of parentheses in the expression
user?.roles?.includes(Role.ADMIN)improves clarity. It's essential to ensure that this modification does not change the original logic, particularly in the context of array operations and optional chaining.- 1273-1273: Adding parentheses to the expression involving
user?.roles,Role.ADMIN, andRole.USERfor checking every role is a good practice. Confirm that this change correctly implements the intended logic of array operations and optional chaining without introducing errors.- 1286-1286: The modification to include parentheses in the expression checking if
user?.rolescontains any of the specified roles enhances readability. Ensure that this adjustment accurately reflects the intended array operation logic, considering optional chaining.- 1299-1299: Incorporating parentheses to clarify the expression checking if
user?.rolesis empty is beneficial. It's crucial to verify that this change does not alter the original logic, especially in the context of array operations and optional chaining.
Comment on lines
+994
to
+998
| x Int | ||
| @@validate(has(x, 1)) | ||
| } | ||
| `) | ||
| ).toContain('left operand of "in" must be of scalar type'); | ||
| ).toContain('argument is not assignable to parameter'); |
There was a problem hiding this comment.
The use of @@validate(has(x, 1)) on line 995 raises a concern because the has function is intended for use with arrays or sets, not scalar values like x which is of type Int. This misuse could lead to confusion or runtime errors.
- @@validate(has(x, 1))
+ // Consider revising the validation logic to correctly apply to the intended data type.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.
Suggested change
| x Int | |
| @@validate(has(x, 1)) | |
| } | |
| `) | |
| ).toContain('left operand of "in" must be of scalar type'); | |
| ).toContain('argument is not assignable to parameter'); | |
| x Int | |
| // Consider revising the validation logic to correctly apply to the intended data type. | |
| } | |
| `) | |
| ).toContain('argument is not assignable to parameter'); |
Comment on lines
+1005
to
+1006
| x Int[] | ||
| @@validate(hasSome(x, 1)) |
There was a problem hiding this comment.
Similar to the previous comment, the use of @@validate(hasSome(x, 1)) on line 1006 is incorrect because hasSome expects its first argument to be an array or a set, and its second argument to be an array. If x is intended to be an array, ensure that the second argument is also an array for correct usage.
- @@validate(hasSome(x, 1))
+ @@validate(hasSome(x, [1]))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.
Suggested change
| x Int[] | |
| @@validate(hasSome(x, 1)) | |
| x Int[] | |
| @@validate(hasSome(x, [1])) |
Comment on lines
+1016
to
+1027
| n N? | ||
| @@validate(n.value > 0) | ||
| } | ||
|
|
||
| model N { | ||
| id String @id | ||
| value Int | ||
| m M @relation(fields: [mId], references: [id]) | ||
| mId String @unique | ||
| } | ||
| `) | ||
| ).toContain('argument is not assignable to parameter'); | ||
| ).toContain('`@@validate` condition cannot use relation fields'); |
There was a problem hiding this comment.
The validation rule @@validate(n.value > 0) on lines 1017-1018 attempts to use a relation field n in a direct comparison, which is not supported. This is because relation fields represent links between models rather than scalar values that can be directly compared. This usage could lead to confusion or runtime errors.
- @@validate(n.value > 0)
+ // Consider revising the validation logic to avoid direct comparisons on relation fields.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.
Suggested change
| n N? | |
| @@validate(n.value > 0) | |
| } | |
| model N { | |
| id String @id | |
| value Int | |
| m M @relation(fields: [mId], references: [id]) | |
| mId String @unique | |
| } | |
| `) | |
| ).toContain('argument is not assignable to parameter'); | |
| ).toContain('`@@validate` condition cannot use relation fields'); | |
| n N? | |
| // Consider revising the validation logic to avoid direct comparisons on relation fields. | |
| } | |
| model N { | |
| id String @id | |
| value Int | |
| m M @relation(fields: [mId], references: [id]) | |
| mId String @unique | |
| } | |
| `) | |
| ).toContain('`@@validate` condition cannot use relation fields'); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/src/utils/typescript-expression-transformer.ts (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/utils/typescript-expression-transformer.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/src/utils/typescript-expression-transformer.ts (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/utils/typescript-expression-transformer.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/schema/src/utils/typescript-expression-transformer.ts (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/schema/src/utils/typescript-expression-transformer.ts
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
undefinedfields are treated correctly.