chore: merge from dev #1054
Merged
chore: merge from dev #1054
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
WalkthroughWalkthroughThe recent updates span across configuration, runtime enhancements, schema management, and integration tests. Key changes include adjustments in Jest configuration paths, enhanced input validation and policy compliance in runtime operations, refined schema location retrieval using 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: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (9)
package.jsonis excluded by:!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by:!**/*.jsonpackages/plugins/swr/package.jsonis excluded by:!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by:!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by:!**/*.jsonpackages/schema/package.jsonis excluded by:!**/*.jsonpackages/server/package.jsonis excluded by:!**/*.jsontests/integration/package.jsonis excluded by:!**/*.jsontests/integration/tests/tsconfig.template.jsonis excluded by:!**/*.json
Files selected for processing (15)
- jest.config.ts (1 hunks)
- packages/runtime/src/enhancements/policy/handler.ts (7 hunks)
- packages/runtime/src/enhancements/policy/policy-utils.ts (4 hunks)
- packages/schema/src/cli/cli-util.ts (2 hunks)
- packages/schema/src/language-server/validator/datamodel-validator.ts (4 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (2 hunks)
- packages/schema/src/utils/pkg-utils.ts (3 hunks)
- packages/schema/tests/schema/validation/datamodel-validation.test.ts (2 hunks)
- script/set-test-env.ts (1 hunks)
- script/test-global-setup.ts (1 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/regression/issue-1014.test.ts (1 hunks)
- tests/integration/tests/regression/issue-177.test.ts (1 hunks)
- tests/integration/tests/schema/refactor-pg.zmodel (3 hunks)
Files skipped from review due to trivial changes (1)
- script/set-test-env.ts
Additional comments: 34
script/test-global-setup.ts (1)
- 4-8: The global setup function correctly checks for the existence of the test scaffold and exits the process if not found, ensuring tests do not run without the necessary setup. Consider making the path to the test scaffold configurable to increase flexibility and maintainability.
tests/integration/tests/regression/issue-177.test.ts (1)
- 3-26: The regression test is well-structured and correctly uses asynchronous testing patterns to check for a specific error message. Ensure that the error message being tested for is stable to avoid brittle tests.
jest.config.ts (1)
- 12-14: The updates to
jest.config.tscorrectly usepath.joinfor defining paths toglobalSetupandsetupFiles, ensuring cross-platform compatibility. Verify that these paths accurately point to the intended setup scripts.tests/integration/tests/regression/issue-1014.test.ts (1)
- 3-51: The test cases for issue 1014 are well-structured and make appropriate use of asynchronous testing patterns. Ensure that the schema and operations used in the tests accurately reflect real use cases and that the assertions are robust enough to catch potential regressions.
tests/integration/tests/schema/refactor-pg.zmodel (1)
- 8-8: The addition of
@lowerand@trimdirectives to theUser,Post, andCommentmodels is a good practice for ensuring data integrity and consistency. Review the application logic to ensure it accounts for these automatic data transformations.Also applies to: 55-55, 70-70
packages/schema/src/utils/pkg-utils.ts (1)
- 7-37: The enhancements to the
findUpfunction introduce valuable flexibility and utility for locating files. The deprecation offindPackageJsonin favor offindUpis a logical step to consolidate functionality. Ensure thorough testing of thefindUpfunction to confirm its reliability across various use cases.packages/schema/src/cli/cli-util.ts (1)
- 292-292: The replacement of
findPackageJsonwithfindUpfor schema location retrieval improves flexibility. Verify that this change correctly handles various project structures and does not introduce regressions in determining schema locations.packages/schema/src/language-server/validator/datamodel-validator.ts (2)
- 124-124: The validation error message for an empty
"fields"value in a relation is clear and informative. This change enhances the robustness of the model validation by ensuring that relations are correctly defined.- 134-134: Similar to the previous comment, the validation for an empty
"references"value is a good addition. It ensures the integrity of relation definitions between models, which is crucial for maintaining data consistency.tests/integration/tests/enhancements/with-policy/field-validation.test.ts (3)
- 48-48: The addition of the
@lowerdirective to theslugfield validation rule is a good enhancement. It ensures that the slug values are stored in a consistent format, which is important for data integrity and usability. This change aligns with best practices for handling user-generated identifiers.- 528-536: The update operation for
userDatathat tests string transformations, including the@lowerdirective, is correctly implemented. This test case effectively validates the transformation logic for string fields. It's a good practice to include such detailed tests to ensure the application behaves as expected after data transformations.- 579-593: The test case for the
upsertoperation with the@lowerdirective on theslugfield demonstrates a good understanding of testing complex operations. It effectively tests both the creation and update paths of theupsertoperation, ensuring that the@lowerdirective is applied correctly in both scenarios. This is crucial for maintaining data consistency.packages/schema/tests/schema/validation/datamodel-validation.test.ts (2)
- 13-13: The adjustment in spacing within the template literal in the
safelyLoadModelcall enhances readability and consistency with the rest of the codebase. This change is purely stylistic and does not impact the functionality of the test.- 131-131: Similar to the previous comment, the adjustment in spacing within the template literal here also improves consistency and readability. It's a good practice to maintain consistent code formatting throughout the project.
packages/schema/src/plugins/prisma/schema-generator.ts (2)
- 60-60: The import statement for
findUpfrom../../utils/pkg-utilscorrectly replaces the previousfindPackageJsonfunction. This change is expected to enhance the flexibility and reliability of locatingpackage.jsonfiles by supporting multiple paths and levels.- 669-669: The usage of
findUp(['package.json'], path.dirname(schemaPath))to locate thepackage.jsonfile is a significant improvement over the previous method. It allows for a more flexible search that can accommodate various project structures. However, it's important to ensure that thefindUpfunction is thoroughly tested in different scenarios to confirm its reliability in locating thepackage.jsonfile in all expected cases.tests/integration/tests/enhancements/with-policy/refactor.test.ts (11)
- 147-155: The test case for user creation correctly enforces email case conversion to lowercase. This is a good practice for ensuring consistency in email handling. However, it's important to ensure that this behavior is documented and expected across all parts of the application to avoid confusion or mismatches in other areas where email addresses are used or validated.
- 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]
Trimming whitespace from the title during post creation is a good practice for data cleanliness. However, ensure that this trimming behavior is consistently applied across all endpoints that accept a title input to maintain data integrity and user experience 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 handling of
createManywithskipDuplicatesoption and subsequent trimming of the title is correctly implemented. This approach helps in avoiding errors due to duplicate entries and ensures data consistency. It's important to verify that theskipDuplicateslogic aligns with the application's data integrity requirements and does not inadvertently ignore important data conflicts.
- 427-438: The test for
createManyoperation correctly demonstrates the trimming of whitespace from titles. This consistency in handling titles is crucial for maintaining data quality. Ensure that similar data sanitization steps are applied to all relevant fields across the application to prevent data inconsistencies.- 524-525: Updating the user's email and enforcing lowercase conversion is consistent with the application's handling of email addresses. It's crucial to ensure that all parts of the application that interact with email addresses are aware of and adhere to this normalization rule to prevent issues with email matching or validation.
- 583-586: Trimming the title during an update operation and ensuring the updated data matches the expected format is a good practice. This test case reinforces the importance of consistent data handling rules across create and update operations. It's important to ensure that these data sanitization practices are documented and consistently applied across the application.
- 611-628: The test case demonstrates creating a new post with comments, including trimming whitespace from the comment content. This is a good practice for maintaining clean data. However, ensure that the application consistently applies similar data sanitization practices across all entities and operations to maintain overall data quality.
- 660-668: The
createManyoperation with title trimming and successful creation of multiple posts demonstrates consistent data handling practices. It's important to ensure that the application's data integrity rules support the intended behavior ofcreateMany, especially regarding handling duplicates and data validation.- 748-754: The test case for updating a post's title with trimming applied is consistent with the application's data handling practices. This reinforces the importance of consistent data sanitization across different operations. Ensure that these practices are well-documented and understood by all developers working on the application to maintain data integrity.
- 825-832: The use of
upsertto either update or create a post, with title trimming applied, demonstrates a flexible approach to handling data. This operation's success is contingent on correctly identifying when to create versus update. Ensure that the logic for determining the existence of records is robust and accounts for all necessary conditions to prevent unintended data duplication or loss.- 1120-1123: The
updateManyoperation with title trimming applied to multiple posts reinforces the application's commitment to clean data. This operation's success highlights the importance of consistent data handling rules across the application. Ensure that bulk update operations like this are carefully monitored to prevent unintended data modifications, especially in large datasets.packages/runtime/src/enhancements/policy/policy-utils.ts (4)
- 320-320: The method
hasAuthGuardno longer explicitly returns a boolean type. While this change simplifies the code, ensure that all usages of this method correctly handle its return values, especially in conditional statements where explicit boolean checks might be expected.- 329-342: The addition of the
hasOverrideAuthGuardmethod enhances the flexibility of policy handling by checking for field-level override guards. This is a valuable addition for more granular access control. Ensure that this method is properly integrated and used wherever field-level policy checks are necessary.- 650-650: The logic within
checkPolicyForUniquemethod now considers field-level override guards when determining if an operation is allowed. This is a crucial update for supporting more complex policy scenarios. However, ensure that the integration of this new check does not inadvertently alter the intended behavior of policy enforcement in edge cases.- 823-823: The
tryRejectmethod's logic has been updated to consider field-level override guards. This change is consistent with the overall enhancement of policy handling mechanisms in this class. Verify that this update aligns with the intended policy enforcement flow and does not introduce any unintended side effects.packages/runtime/src/enhancements/policy/handler.ts (3)
- 484-484: The introduction of Zod schema validation for create operations enhances data integrity and validation logic. However, ensure that all potential edge cases are covered, especially with complex nested structures or when dealing with optional fields.
- 814-819: The validation of update payloads using Zod schemas within nested updates is a robust addition. However, ensure that the validation logic correctly handles complex nested updates and respects optional fields and Prisma-specific update operations (e.g.,
increment).- 1063-1084: The approach to only validate literal fields in update payloads is prudent, given Prisma's support for non-literal update operations. However, ensure that this filtering does not inadvertently exclude fields that should be validated or transform the data in unexpected ways.
Comment on lines
+517
to
+536
| // 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.policyUtils.replace(item, validationResult); | ||
| } | ||
|
|
||
| const inputCheck = this.policyUtils.checkInputGuard(this.model, item, 'create'); | ||
| if (inputCheck === false) { | ||
| // unconditionally deny | ||
| throw this.policyUtils.deniedByPolicy( | ||
| this.model, | ||
| 'create', | ||
| undefined, | ||
| CrudFailureReason.ACCESS_POLICY_VIOLATION | ||
| ); | ||
| } else if (inputCheck === true) { | ||
| const r = this.validateCreateInputSchema(this.model, item); | ||
| if (r !== item) { | ||
| this.policyUtils.replace(item, r); | ||
| } | ||
| // unconditionally allow |
There was a problem hiding this comment.
While iterating over args.data for createMany operations, the validation and input check logic is sound. However, consider optimizing this loop to avoid potential performance issues with large datasets by minimizing the number of times validateCreateInputSchema and checkInputGuard are called.
- for (const item of enumerate(args.data)) {
+ for (const [index, item] of enumerate(args.data).entries()) {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
| // 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.policyUtils.replace(item, validationResult); | |
| } | |
| const inputCheck = this.policyUtils.checkInputGuard(this.model, item, 'create'); | |
| if (inputCheck === false) { | |
| // unconditionally deny | |
| throw this.policyUtils.deniedByPolicy( | |
| this.model, | |
| 'create', | |
| undefined, | |
| CrudFailureReason.ACCESS_POLICY_VIOLATION | |
| ); | |
| } else if (inputCheck === true) { | |
| const r = this.validateCreateInputSchema(this.model, item); | |
| if (r !== item) { | |
| this.policyUtils.replace(item, r); | |
| } | |
| // unconditionally allow | |
| // 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 [index, item] of enumerate(args.data).entries()) { | |
| const validationResult = this.validateCreateInputSchema(this.model, item); | |
| if (validationResult !== item) { | |
| this.policyUtils.replace(item, validationResult); | |
| } | |
| const inputCheck = this.policyUtils.checkInputGuard(this.model, item, 'create'); | |
| if (inputCheck === false) { | |
| // unconditionally deny | |
| throw this.policyUtils.deniedByPolicy( | |
| this.model, | |
| 'create', | |
| undefined, | |
| CrudFailureReason.ACCESS_POLICY_VIOLATION | |
| ); | |
| } else if (inputCheck === true) { | |
| // unconditionally allow |
Comment on lines
+1120
to
+1121
| args.data = this.validateUpdateInputSchema(this.model, args.data); | ||
|
|
There was a problem hiding this comment.
Applying Zod schema validation to updateMany operations is consistent with the approach for single updates. However, consider the impact on performance and ensure that the validation logic is optimized for handling bulk operations efficiently.
- args.data = this.validateUpdateInputSchema(this.model, args.data);
+ // Consider batching or optimizing validation for bulk operationsCommittable 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
| args.data = this.validateUpdateInputSchema(this.model, args.data); | |
| // Consider batching or optimizing validation for bulk operations |
Comment on lines
+67
to
+68
| const isScalar = SCALAR_TYPES.includes(idField.type.type as (typeof SCALAR_TYPES)[number]); | ||
| const isValidType = isScalar || isEnum(idField.type.reference?.ref); |
There was a problem hiding this comment.
The logic for determining if a field's type is scalar or an enum is correctly implemented. However, consider adding a comment explaining why these specific checks are necessary for fields with @id attributes, as it might not be immediately clear to someone unfamiliar with the domain-specific rules.
+ // Ensure @id fields are either scalar types or enums for database compatibility
const isScalar = SCALAR_TYPES.includes(idField.type.type as (typeof SCALAR_TYPES)[number]);
const isValidType = isScalar || isEnum(idField.type.reference?.ref);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
| const isScalar = SCALAR_TYPES.includes(idField.type.type as (typeof SCALAR_TYPES)[number]); | |
| const isValidType = isScalar || isEnum(idField.type.reference?.ref); | |
| // Ensure @id fields are either scalar types or enums for database compatibility | |
| const isScalar = SCALAR_TYPES.includes(idField.type.type as (typeof SCALAR_TYPES)[number]); | |
| const isValidType = isScalar || isEnum(idField.type.reference?.ref); |
Comment on lines
+160
to
+167
| if (!field.type.optional && fields[i].$resolvedType?.nullable) { | ||
| // if relation is not optional, then fk field must not be nullable | ||
| if (accept) { | ||
| accept( | ||
| 'error', | ||
| `relation "${field.name}" is not optional, but field "${fields[i].target.$refText}" is optional`, | ||
| { node: fields[i].target.ref! } | ||
| ); |
There was a problem hiding this comment.
This block of code introduces a validation check to ensure that if a relation is not optional, then the foreign key field must not be nullable. This is an important check for data integrity and consistency. However, consider adding a brief comment to explain the rationale behind this check for future maintainability.
+ // Validate non-optional relations to ensure foreign key fields are not nullable
if (!field.type.optional && fields[i].$resolvedType?.nullable) {
if (accept) {
accept(
'error',
`relation "${field.name}" is not optional, but field "${fields[i].target.$refText}" is optional`,
{ node: fields[i].target.ref! }
);
}
}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
| if (!field.type.optional && fields[i].$resolvedType?.nullable) { | |
| // if relation is not optional, then fk field must not be nullable | |
| if (accept) { | |
| accept( | |
| 'error', | |
| `relation "${field.name}" is not optional, but field "${fields[i].target.$refText}" is optional`, | |
| { node: fields[i].target.ref! } | |
| ); | |
| // Validate non-optional relations to ensure foreign key fields are not nullable | |
| if (!field.type.optional && fields[i].$resolvedType?.nullable) { | |
| // if relation is not optional, then fk field must not be nullable | |
| if (accept) { | |
| accept( | |
| 'error', | |
| `relation "${field.name}" is not optional, but field "${fields[i].target.$refText}" is optional`, | |
| { node: fields[i].target.ref! } | |
| ); |
Comment on lines
+511
to
+526
| let ud = await db.userData.create({ | ||
| data: { | ||
| userId: '1', | ||
| a: 1, | ||
| b: 0, | ||
| c: -1, | ||
| d: 0, | ||
| text1: 'abc123', | ||
| text2: 'def', | ||
| text3: 'aaa', | ||
| text4: 'abcab', | ||
| text6: ' AbC ', | ||
| text7: 'abc', | ||
| }, | ||
| }); | ||
| expect(ud).toMatchObject({ text6: 'abc', text7: 'ABC' }); |
There was a problem hiding this comment.
The test case for creating userData with various validations, including the new @lower directive, is well-implemented. It effectively tests the validation logic for optional and transformed string fields. However, ensure that the test descriptions accurately reflect the specific validations being tested, including the new @lower directive for text6.
+ // Test description should mention testing the @lower directive for text6
let ud = await db.userData.create({
data: {
userId: '1',
a: 1,
b: 0,
c: -1,
d: 0,
text1: 'abc123',
text2: 'def',
text3: 'aaa',
text4: 'abcab',
text6: ' AbC ',
text7: 'abc',
},
});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
| let ud = await db.userData.create({ | |
| data: { | |
| userId: '1', | |
| a: 1, | |
| b: 0, | |
| c: -1, | |
| d: 0, | |
| text1: 'abc123', | |
| text2: 'def', | |
| text3: 'aaa', | |
| text4: 'abcab', | |
| text6: ' AbC ', | |
| text7: 'abc', | |
| }, | |
| }); | |
| expect(ud).toMatchObject({ text6: 'abc', text7: 'ABC' }); | |
| // Test description should mention testing the @lower directive for text6 | |
| let ud = await db.userData.create({ | |
| data: { | |
| userId: '1', | |
| a: 1, | |
| b: 0, | |
| c: -1, | |
| d: 0, | |
| text1: 'abc123', | |
| text2: 'def', | |
| text3: 'aaa', | |
| text4: 'abcab', | |
| text6: ' AbC ', | |
| text7: 'abc', | |
| }, | |
| }); | |
| expect(ud).toMatchObject({ text6: 'abc', text7: 'ABC' }); |
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
ZENSTACK_TEST.@lowerdirective for slug field validation and a@trimdirective to various model fields.package.jsonfiles to improve package discovery.