merge dev to main (v2.5.2) #1722
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe pull request introduces several updates across multiple files, primarily focusing on version updates, enhancements to model handling, validation improvements, and the addition of new tests. Key changes include the increment of the project version, the introduction of new methods for better query and mutation handling, and the integration of validation logic in the Zod schema generator. Additionally, several new test cases and files have been added to ensure the functionality and integrity of the updated features. Changes
Possibly related PRs
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: 13
Outside diff range and nitpick comments (9)
tests/regression/tests/issue-1695.test.ts (1)
4-20: Consider adding assertions to verify the expected behavior.The test is loading a model schema, but it's not making any assertions to verify the expected behavior. To make this a more effective regression test, consider adding assertions that verify things like:
- The model schema is loaded successfully without any errors.
- The deny policies are enforced correctly when attempting to read or update soft-deleted records.
packages/runtime/src/enhancements/node/omit.ts (1)
74-77: Consider adding unit tests for the updateddoPostProcessmethodThe integration of
QueryUtilsand the use ofgetDelegateConcreteModelmodify the behavior of thedoPostProcessmethod. To ensure the method behaves as expected across different scenarios, consider adding unit tests that cover cases whererealModelvaries, including when it might beundefinedornull.packages/runtime/src/enhancements/node/query-utils.ts (2)
227-230: Validate the type ofconcreteModelNamebefore returningEnsure that
concreteModelNameis a string to avoid unexpected issues elsewhere in the codebase. There might be cases wheredata[modelInfo.discriminator]is not a string:if (concreteModelName) { + if (typeof concreteModelName === 'string') { return concreteModelName; + } }
218-234: Consider adding unit tests forgetDelegateConcreteModelTo ensure the correctness of
getDelegateConcreteModeland prevent future regressions, it's important to have unit tests covering various scenarios, such as:
- When
dataisnullorundefined.- When
datais an array.- When
datalacks the discriminator property.- When the discriminator value is not a string.
Would you like me to help generate unit tests for this method or open a GitHub issue to track this task?
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1)
281-281: Consider using consistent policy directives for clarity.In the
Assetmodel, thefoofield uses@allow('read', auth().id == 1), whereas in thePostmodel, thebarfield uses@deny('read', auth().id != 1). For consistency and improved readability, it would be beneficial to use the same directive (@allowor@deny) when the access conditions are similar.You can update the
barfield to use@allowfor consistency:- bar String @deny('read', auth().id != 1) + bar String @allow('read', auth().id == 1)Also applies to: 289-289
packages/schema/src/plugins/zod/transformer.ts (3)
474-474: Simplify type assertion formodeassignmentThe type assertion
(options.mode as string)may be unnecessary ifoptions.modeis already of typestring | undefined. You can simplify the assignment by removing the type assertion.Apply this diff to streamline the code:
- const mode = (options.mode as string) ?? 'strict'; + const mode = options.mode ?? 'strict';
474-474: Consider handling invalidmodevaluesCurrently, the
switchstatement inwrapWithZodObjectdoes not handle unexpectedmodevalues, which could lead to no method being appended when an invalidmodeis provided. To enhance robustness, consider validating themodeparameter or adding a warning for unrecognized modes.Apply this diff to add validation:
switch (mode) { case 'strip': // zod strips by default break; case 'passthrough': wrapped += '.passthrough()'; break; case 'strict': wrapped += '.strict()'; break; + default: + throw new Error(\`Invalid mode '\${mode}' provided to wrapWithZodObject\`); }
405-424: Optimize comment usage for clarityThe comment
// eslint-disable-next-line @typescript-eslint/no-non-null-assertionis used in multiple places. Consider removing unnecessary comments or consolidating them at the top of the file if they apply globally, to enhance readability.tests/integration/tests/plugins/zod.test.ts (1)
854-911: Enhance test descriptions for better clarityThe current test descriptions are brief and may not fully convey the purpose of each test. Updating the descriptions can improve readability and understanding.
Consider revising the test descriptions:
-it('is strict by default', async () => { +it('should enforce strict mode by default by rejecting extra properties', async () => { -it('works in strip mode', async () => { +it('should strip extra properties when mode is set to "strip"', async () => { -it('works in passthrough mode', async () => { +it('should allow extra properties when mode is set to "passthrough"', async () => {Also applies to: 913-954, 956-997
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (14)
package.jsonis excluded by!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/misc/redwood/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/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
Files selected for processing (19)
- packages/ide/jetbrains/build.gradle.kts (1 hunks)
- packages/runtime/src/enhancements/node/delegate.ts (13 hunks)
- packages/runtime/src/enhancements/node/omit.ts (3 hunks)
- packages/runtime/src/enhancements/node/policy/policy-utils.ts (1 hunks)
- packages/runtime/src/enhancements/node/query-utils.ts (1 hunks)
- packages/schema/src/language-server/zmodel-scope.ts (1 hunks)
- packages/schema/src/plugins/zod/generator.ts (6 hunks)
- packages/schema/src/plugins/zod/transformer.ts (13 hunks)
- packages/schema/src/res/stdlib.zmodel (1 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/omit-interaction.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/password-interaction.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/validation.test.ts (1 hunks)
- tests/integration/tests/plugins/zod.test.ts (1 hunks)
- tests/regression/tests/issue-1693.test.ts (1 hunks)
- tests/regression/tests/issue-1695.test.ts (1 hunks)
- tests/regression/tests/issue-1698.test.ts (1 hunks)
- tests/regression/tests/issue-1710.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/schema/src/res/stdlib.zmodel
Additional comments not posted (35)
tests/regression/tests/issue-1693.test.ts (1)
1-20: LGTM!The test file is well-structured and the schema definition is clear and concise. The use of the
loadSchemafunction from@zenstackhq/testtoolsis appropriate for loading the schema in the test.The schema defines an
Animalmodel with a@@delegatedirective and aDogmodel that extendsAnimal, which is likely the scenario that caused the regression in issue 1693.The test is marked as asynchronous and the
itblock has a clear description of "regression", indicating that this test is checking for a regression related to the issue.Overall, the test file looks good and should help catch any future regressions related to this issue.
tests/regression/tests/issue-1695.test.ts (1)
1-21: LGTM! The test file structure and model schema look good.The test file follows the standard describe-it block format and the model schema is using best practices like:
- Inheriting fields from an abstract model for reusability.
- Using deny policies with the
deletedfield to control access to soft-deleted records.- Using the
future()function to prevent updates to soft-deleted records.tests/integration/tests/enhancements/with-delegate/validation.test.ts (1)
3-25: LGTM!The test case is correctly testing the validation of polymorphic input. The test case is creating a
Postrecord and then trying to update thetitlefield using thedelegate_aux_postfield. The test case is correctly expecting the update operation to throw an error with the message "Auxiliary relation field "delegate_aux_post" cannot be set directly".tests/integration/tests/enhancements/with-delegate/password-interaction.test.ts (2)
30-37: LGTM!The test case is well-structured and covers the expected behavior of hashing passwords when a
Postrecord is created directly.
39-49: LGTM!The test case is well-structured and covers the expected behavior of hashing passwords when a
Postrecord is created nested under aUserrecord.tests/regression/tests/issue-1698.test.ts (1)
1-74: LGTM!The regression test covers the basic functionality of the 'delegate' enhancement and the relationships between the models. The test loads the schema, creates instances of the models, and verifies the relationships using
findFirstandfindManywithinclude. The test also usesexpectandtoMatchObjectto verify the results.The test does not cover all possible scenarios, but it is a good starting point for a regression test.
tests/integration/tests/enhancements/with-delegate/omit-interaction.test.ts (4)
29-40: LGTM!The test case correctly asserts that fields marked with
@omitare not returned when querying a concrete model directly.
42-54: LGTM!The test case correctly asserts that fields marked with
@omitare not returned when querying a polymorphic model via its base model, while fields not marked with@omitare returned.
56-73: LGTM!The test case correctly asserts that fields marked with
@omitare not returned when the discriminator field is not selected in the query.
75-90: LGTM!The test case correctly asserts that fields marked with
@omitare not returned when querying a polymorphic model in a nested context (via a relation), while fields not marked with@omitare returned.packages/schema/src/language-server/zmodel-scope.ts (1)
207-207: Refactoring of scope creation logic looks good!The change from
createScopeForNodestocreateScopeForModelsuggests a refactoring of the scope creation logic to operate at the model level. This seems like a reasonable enhancement.To ensure the correctness of this change, please verify that
createScopeForModelgenerates the expected scope for the model and its fields. You can use the following script to check the scope creation:Verification successful
Verification successful:
createScopeForModelimplementation is correctThe
createScopeForModelfunction is implemented correctly and handles the model and global scope as expected. It creates a scope for the model's fields when given a DataModel node, and returns an empty scope otherwise. The change increateScopeForContainingModelto usecreateScopeForModelis consistent with the rest of the class implementation and appears to be a valid refactoring.Key points:
- The function correctly checks if the node is a DataModel.
- It uses
getModelFieldsWithBasesto include fields from base models.- The function is used consistently throughout the class.
The refactoring enhances the code organization by centralizing the model scope creation logic in a single method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `createScopeForModel` generates the expected scope. # Test: Search for the `createScopeForModel` function and analyze its implementation. # Expect: The function should correctly handle the model and global scope to generate the expected scope. rg --type typescript -A 10 $'createScopeForModel\('Length of output: 87
Script:
#!/bin/bash # Description: Search for createScopeForModel function and its usage # Find TypeScript files and search for createScopeForModel echo "Searching for createScopeForModel function definition:" fd -e ts -e tsx | xargs grep -n "createScopeForModel.*(" -A 10 echo -e "\nSearching for createScopeForModel function usage:" fd -e ts -e tsx | xargs grep -n "createScopeForModel(" echo -e "\nSearching for ZModelScopeProvider class:" fd -e ts -e tsx | xargs grep -n "class ZModelScopeProvider" -A 20Length of output: 10194
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (4)
1366-1394: The schema defines a hierarchy of models using delegation.The loaded schema defines several models:
Assetserves as a base class with a delegate directive based on thetypefield. It has a one-to-many relationship withComment.PostextendsAsset.Commentalso has a delegate directive and a many-to-one relationship withAsset.TextCommentextendsComment.This setup allows testing the merging of hierarchies with delegation at multiple levels.
1396-1400: The test creates aPostand an associatedTextComment.
- A database instance
dbis created by enhancing the loaded schema.- A
Postinstance is created with a title and a view count.- A
TextCommentinstance is created with text and moderated fields, and is associated with the previously createdPost.This setup provides the necessary data to test the merging of the
AssetandCommenthierarchies.
1403-1404: The test queries anAssetinstance with its comments included and asserts the result.
- The test performs a query to retrieve an
Assetinstance usingdb.asset.findFirst()withinclude: { comments: true }.- The returned object
ris expected to match an object containing theviewCountfrom theAssetand thecommentcreated earlier.This assertion verifies that querying the base class
Assetcorrectly includes the associatedCommentsubclass instance.
1407-1408: The test queries aPostinstance with its comments included and asserts the result.
- The test performs a query to retrieve a
Postinstance usingdb.post.findFirst()withinclude: { comments: true }.- The returned object
ris expected to match an object containing all fields from thepostcreated earlier, as well as the associatedcomment.This assertion verifies that querying the subclass
Postcorrectly includes the associatedCommentsubclass instance, demonstrating the successful merging of the hierarchies.tests/regression/tests/issue-1710.test.ts (5)
1-3: Test Suite Initialization Looks GoodThe test suite
'issue 1710'and the test case'regression'are properly set up with async functionality.
4-25: Schema Definitions Are Correctly StructuredThe
Profile,User, andOrganizationmodels are well-defined, making appropriate use of inheritance and access control annotations like@@delegate,@allow,@deny, and@omit. The usage of@@delegate(type)effectively delegates behaviors based on thetypefield.
28-33: Access Control Validations Are AppropriateThe test correctly asserts that
user.emailanduser.passwordareundefinedafter creation, aligning with the@deny('read', true)and@omitannotations in the schema.
34-37: Consistent Enforcement of Access ControlsBy retrieving the user via
db.profile.findUnique, the test ensures that access controls are consistently enforced across inherited models. The assertions thatfoundUser.emailandfoundUser.passwordareundefinedconfirm this behavior.
38-51: Correct Handling of Forbidden Update OperationsThe test appropriately expects an error when attempting to update the
delegate_aux_userfield, ensuring that unauthorized operations are properly rejected with the message'Auxiliary relation field'.packages/runtime/src/enhancements/node/omit.ts (2)
8-8: ImportingQueryUtilsis correctly implementedThe
QueryUtilsmodule is imported appropriately, enabling its usage within theOmitHandlerclass.
25-26: Initialization ofqueryUtilsis properly handledThe private member
queryUtilsis declared and initialized correctly in the constructor ofOmitHandler.Also applies to: 29-29
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (2)
272-291: The new test case effectively verifies field-level policies.The models and field-level policies are correctly defined to test access control based on
auth().id. The use of@allowand@denydirectives appropriately restricts access to thefooandbarfields as intended.
293-311: Test assertions comprehensively validate access control.The test cases accurately verify that users with different
idvalues have appropriate access to thefooandbarfields. The assertions confirm that user withid: 1can access both fields, while user withid: 2cannot, ensuring the field-level policies are enforced correctly.packages/schema/src/plugins/zod/generator.ts (6)
2-2: ImportingPluginErrorenhances error handling.The addition of
PluginErrorimport allows for more precise and contextual error reporting within the plugin.
58-68: Proper validation ofmodeoption added.The validation ensures that
this.options.mode, if provided, is a string and one of the allowed values ('strip', 'strict', or 'passthrough'). Throwing aPluginErrorwith a clear message improves robustness and provides clear feedback to the user.
338-349: Correct implementation of schema generation based onmode.The switch statement adjusts the schema generation according to
this.options.mode:
'strip': Uses Zod's default stripping behavior.'passthrough': Appends.passthrough()to allow unknown keys.- Default (
'strict'): Appends.strict()to disallow unknown keys.This ensures that the generated schemas behave as expected based on the specified mode.
397-401: Appropriate handling of discriminator fields in schema omission.The code correctly identifies discriminator fields using
isDiscriminatorFieldand constructsomitDiscriminatorsto omit these fields from the schemas when necessary. This prevents potential conflicts or compilation issues related to discriminator fields.
512-519: Properly handles fields with defaults increateSchema.The logic ensures that fields with default values are made optional in the
createSchema, excluding discriminator fields to avoid compilation errors. The filtering accurately targets the appropriate fields.
Line range hint
490-499:
Ensures Prisma update schema allows partial updates and unknown keys.By applying
makePartialandmakePassthroughtoprismaUpdateSchema, the code allows for partial updates and permits additional fields, aligning with Prisma's behavior for update inputs. Documenting it as for internal use maintains proper encapsulation.packages/schema/src/plugins/zod/transformer.ts (2)
405-424: Well-structured implementation ofwrapWithZodObjectwithmodeparameterThe addition of the
modeparameter and the switch statement effectively handle different wrapping behaviors ('strict','strip','passthrough'). The default case ensures that unknown keys are handled as per the specified mode.
693-694: Adjustcountfield to align with Zod schema expectationsThe
selectfield within thecountoperation includes a union type that might not accurately represent the Prisma client's expected input. Ensure that theselectfield correctly reflects the input type for count operations.Run the following script to verify the
countoperation'sselectfield:packages/runtime/src/enhancements/node/delegate.ts (2)
362-363: Ensure consistent sanitization in mutation methodsThe
sanitizeMutationPayloadmethod is correctly called to validate mutation payloads in thecreatemethod. Please ensure that this sanitization is consistently applied across all mutation methods to prevent improper manipulation of auxiliary relation fields.
638-639: Consistent application ofsanitizeMutationPayloadin update methodsIt's good to see that
sanitizeMutationPayloadis invoked in theupdate,updateMany, andupsertmethods to validate mutation payloads. This helps maintain data integrity by preventing direct manipulation of auxiliary relation fields.Also applies to: 659-660, 688-690
packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)
1385-1387: Correctly handling polymorphic entities inpostProcessForReadThe addition of determining the concrete model with
getDelegateConcreteModelensures that polymorphic entities are processed using the appropriate model. This change allowsdoPostProcessForReadto apply field-level policies based on the actual entity type, which is crucial for accurate policy enforcement.
|
|
||
| group = "dev.zenstack" | ||
| version = "2.5.1" | ||
| version = "2.5.2" |
There was a problem hiding this comment.
Tip
Codebase Verification
Changelog is severely outdated and missing version 2.5.2
The changelog has not been updated to include version 2.5.2 or any other 2.x versions. In fact, the most recent entry in the main CHANGELOG.md file is for version 0.5.0, dated 2022-12-15. This significant gap in documentation is a critical issue that needs immediate attention.
Action items:
- Update the CHANGELOG.md file to include all major versions and changes from 0.5.0 up to the current 2.5.2 release.
- Ensure that the changelog includes a detailed entry for version 2.5.2, highlighting all new features, improvements, and breaking changes.
- Implement a process to consistently update the changelog with each new release to prevent this issue from recurring.
Analysis chain
LGTM!
The version update aligns with the PR objective of merging dev to main for v2.5.2.
Please verify that the changelog has been updated to reflect the changes included in this release.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the changelog has been updated for version 2.5.2.
# Test: Search for the version in the changelog. Expect: At least one match.
rg --type markdown $'2\.5\.2' $(fd CHANGELOG.md)
Length of output: 433
Script:
#!/bin/bash
# Description: Search for different version formats in changelog files
# Search for "v2.5.2" (with 'v' prefix)
echo "Searching for 'v2.5.2':"
rg --type markdown 'v2\.5\.2' $(fd CHANGELOG.md)
# Search for "2.5.2" with surrounding characters
echo "Searching for '2.5.2' with surrounding characters:"
rg --type markdown '[^0-9]2\.5\.2[^0-9]' $(fd CHANGELOG.md)
# List the content of the main CHANGELOG.md file
echo "Content of the main CHANGELOG.md file:"
cat CHANGELOG.md
Length of output: 4639
| const realModel = this.queryUtils.getDelegateConcreteModel(model, entityData); | ||
|
|
||
| for (const field of getModelFields(entityData)) { | ||
| const fieldInfo = await resolveField(this.options.modelMeta, model, field); | ||
| const fieldInfo = await resolveField(this.options.modelMeta, realModel, field); |
There was a problem hiding this comment.
Ensure realModel is valid before proceeding
When calling this.queryUtils.getDelegateConcreteModel(model, entityData);, there is a potential for realModel to be undefined or null. This could lead to errors when resolveField is called with an invalid realModel. Consider adding a check to verify that realModel is valid before proceeding.
Apply this diff to add a validation check:
private async doPostProcess(entityData: any, model: string) {
const realModel = this.queryUtils.getDelegateConcreteModel(model, entityData);
+ if (!realModel) {
+ // Handle the error appropriately
+ return;
+ }
for (const field of getModelFields(entityData)) {
const fieldInfo = await resolveField(this.options.modelMeta, realModel, field);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 realModel = this.queryUtils.getDelegateConcreteModel(model, entityData); | |
| for (const field of getModelFields(entityData)) { | |
| const fieldInfo = await resolveField(this.options.modelMeta, model, field); | |
| const fieldInfo = await resolveField(this.options.modelMeta, realModel, field); | |
| const realModel = this.queryUtils.getDelegateConcreteModel(model, entityData); | |
| if (!realModel) { | |
| // Handle the error appropriately | |
| return; | |
| } | |
| for (const field of getModelFields(entityData)) { | |
| const fieldInfo = await resolveField(this.options.modelMeta, realModel, field); |
| const modelInfo = getModelInfo(this.options.modelMeta, model); | ||
| if (modelInfo?.discriminator) { |
There was a problem hiding this comment.
Handle potential undefined from getModelInfo
getModelInfo might return undefined if the model is not found in this.options.modelMeta. Accessing modelInfo.discriminator without verifying modelInfo could lead to a runtime error. Consider adding a null check for modelInfo:
const modelInfo = getModelInfo(this.options.modelMeta, model);
-if (modelInfo?.discriminator) {
+if (modelInfo && modelInfo.discriminator) {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 modelInfo = getModelInfo(this.options.modelMeta, model); | |
| if (modelInfo?.discriminator) { | |
| const modelInfo = getModelInfo(this.options.modelMeta, model); | |
| if (modelInfo && modelInfo.discriminator) { |
| if (!data || typeof data !== 'object') { | ||
| return model; | ||
| } |
There was a problem hiding this comment.
Ensure data is not an array to prevent unexpected behavior
Currently, the condition if (!data || typeof data !== 'object') does not exclude arrays since typeof [] === 'object'. If data is expected to be a plain object, consider adding a check to exclude arrays:
if (!data || typeof data !== 'object' || Array.isArray(data)) {
return model;
}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.
| if (!data || typeof data !== 'object') { | |
| return model; | |
| } | |
| if (!data || typeof data !== 'object' || Array.isArray(data)) { | |
| return model; | |
| } |
| const fields = `${selectZodSchemaLineLazy} ${includeZodSchemaLineLazy} where: ${modelName}WhereUniqueInputObjectSchema`; | ||
| codeBody += `findUnique: ${this.wrapWithZodObject(fields, mode)},`; |
There was a problem hiding this comment.
Refactor repeated patterns in input schema generation
The pattern of defining fields and appending to codeBody using this.wrapWithZodObject(fields, mode) is repeated across multiple CRUD operations (findUnique, findFirst, findMany, create, createMany, delete, deleteMany, update, updateMany, upsert, aggregate, count). Consolidate this logic into a helper function to reduce code duplication and enhance maintainability.
Also applies to: 499-500, 511-512, 528-529, 537-538, 546-547, 555-556, 573-574, 591-592, 614-615, 661-664, 693-694
| expect(parsed.success).toBeTruthy(); | ||
| expect(parsed.data.x).toBe(1); | ||
|
|
There was a problem hiding this comment.
Add success check before accessing 'data' in parsed results
In these assertions, accessing parsed.data.x without verifying parsed.success could result in errors if parsing fails. Adding a success check ensures robustness.
Update the assertions as follows:
expect(parsed.success).toBeTruthy();
+if (parsed.success) {
expect(parsed.data.x).toBe(1);
+}Also applies to: 989-991, 995-997
| await expect( | ||
| loadSchema( | ||
| ` | ||
| datasource db { | ||
| provider = 'postgresql' | ||
| url = env('DATABASE_URL') | ||
| } | ||
| generator js { | ||
| provider = 'prisma-client-js' | ||
| } | ||
| plugin zod { | ||
| provider = '@core/zod' | ||
| mode = 'xyz' | ||
| } | ||
| model User { | ||
| id Int @id @default(autoincrement()) | ||
| email String @unique @email @endsWith('@zenstack.dev') | ||
| password String | ||
| @@validate(length(password, 6, 20)) | ||
| } | ||
| `, | ||
| { addPrelude: false, pushDb: false } | ||
| ) | ||
| ).rejects.toThrow(/Invalid mode/); | ||
| }); |
There was a problem hiding this comment.
Use exact error message in exception assertion for precise testing
The test currently checks for an error message matching /Invalid mode/, which may be too broad. Using the exact error message ensures that the test is validating the specific expected failure.
Modify the assertion to check for the exact error message:
).rejects.toThrow(/Invalid mode/);
+).rejects.toThrow('Invalid mode: xyz. Valid modes are "strict", "strip", or "passthrough".');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.
| await expect( | |
| loadSchema( | |
| ` | |
| datasource db { | |
| provider = 'postgresql' | |
| url = env('DATABASE_URL') | |
| } | |
| generator js { | |
| provider = 'prisma-client-js' | |
| } | |
| plugin zod { | |
| provider = '@core/zod' | |
| mode = 'xyz' | |
| } | |
| model User { | |
| id Int @id @default(autoincrement()) | |
| email String @unique @email @endsWith('@zenstack.dev') | |
| password String | |
| @@validate(length(password, 6, 20)) | |
| } | |
| `, | |
| { addPrelude: false, pushDb: false } | |
| ) | |
| ).rejects.toThrow(/Invalid mode/); | |
| }); | |
| await expect( | |
| loadSchema( | |
| ` | |
| datasource db { | |
| provider = 'postgresql' | |
| url = env('DATABASE_URL') | |
| } | |
| generator js { | |
| provider = 'prisma-client-js' | |
| } | |
| plugin zod { | |
| provider = '@core/zod' | |
| mode = 'xyz' | |
| } | |
| model User { | |
| id Int @id @default(autoincrement()) | |
| email String @unique @email @endsWith('@zenstack.dev') | |
| password String | |
| @@validate(length(password, 6, 20)) | |
| } | |
| `, | |
| { addPrelude: false, pushDb: false } | |
| ) | |
| ).rejects.toThrow('Invalid mode: xyz. Valid modes are "strict", "strip", or "passthrough".'); | |
| }); |
| schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x | ||
| ).toBe(1); | ||
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); |
There was a problem hiding this comment.
Ensure parsing is successful before accessing 'data' property
In the assertion accessing parseResult.data.x, if the parsing fails, data might be undefined, leading to unexpected errors.
Modify the code to check if parsing is successful before accessing data:
- expect(schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x).toBe(1);
+ const parseResult = schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 });
+ expect(parseResult.success).toBe(true);
+ if (parseResult.success) {
+ expect(parseResult.data.x).toBe(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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x | |
| ).toBe(1); | |
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); | |
| const parseResult = schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }); | |
| expect(parseResult.success).toBe(true); | |
| if (parseResult.success) { | |
| expect(parseResult.data.x).toBe(1); | |
| } | |
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); |
| it('is strict by default', async () => { | ||
| const { zodSchemas } = await loadSchema( | ||
| ` | ||
| datasource db { | ||
| provider = 'postgresql' | ||
| url = env('DATABASE_URL') | ||
| } | ||
| generator js { | ||
| provider = 'prisma-client-js' | ||
| } | ||
| plugin zod { | ||
| provider = '@core/zod' | ||
| } | ||
| model User { | ||
| id Int @id @default(autoincrement()) | ||
| email String @unique @email @endsWith('@zenstack.dev') | ||
| password String | ||
| @@validate(length(password, 6, 20)) | ||
| } | ||
| `, | ||
| { addPrelude: false, pushDb: false } | ||
| ); | ||
|
|
||
| const schemas = zodSchemas.models; | ||
| expect( | ||
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123' }).success | ||
| ).toBeTruthy(); | ||
| expect( | ||
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | ||
| ).toBeFalsy(); | ||
|
|
||
| expect( | ||
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123' }).success | ||
| ).toBeTruthy(); | ||
| expect( | ||
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | ||
| ).toBeFalsy(); | ||
|
|
||
| // Prisma create/update schema should always non-strict | ||
| expect( | ||
| schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x | ||
| ).toBe(1); | ||
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); | ||
|
|
||
| expect( | ||
| zodSchemas.input.UserInputSchema.create.safeParse({ | ||
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123' }, | ||
| }).success | ||
| ).toBeTruthy(); | ||
| expect( | ||
| zodSchemas.input.UserInputSchema.create.safeParse({ | ||
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }, | ||
| }).success | ||
| ).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Consider refactoring repeated schema definitions into a constant
The schema definitions within the loadSchema calls are repeated in multiple tests. Extracting the common parts into a constant or a helper function can improve maintainability and reduce code duplication.
Apply this diff to refactor the code:
+const baseSchema = `
+ datasource db {
+ provider = 'postgresql'
+ url = env('DATABASE_URL')
+ }
+
+ generator js {
+ provider = 'prisma-client-js'
+ }
+
+ plugin zod {
+ provider = '@core/zod'
+ // mode will be set per test
+ }
+
+ model User {
+ id Int @id @default(autoincrement())
+ email String @unique @email @endsWith('@zenstack.dev')
+ password String
+ @@validate(length(password, 6, 20))
+ }
+`;
+
it('is strict by default', async () => {
- const { zodSchemas } = await loadSchema(
- \`
- datasource db {
- provider = 'postgresql'
- url = env('DATABASE_URL')
- }
-
- generator js {
- provider = 'prisma-client-js'
- }
-
- plugin zod {
- provider = '@core/zod'
- }
-
- model User {
- id Int @id @default(autoincrement())
- email String @unique @email @endsWith('@zenstack.dev')
- password String
- @@validate(length(password, 6, 20))
- }
- \`,
+ const schema = baseSchema.replace('// mode will be set per test', '');
+ const { zodSchemas } = await loadSchema(
+ schema,
{ addPrelude: false, pushDb: false }
);
// rest of the test code...
});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.
| it('is strict by default', async () => { | |
| const { zodSchemas } = await loadSchema( | |
| ` | |
| datasource db { | |
| provider = 'postgresql' | |
| url = env('DATABASE_URL') | |
| } | |
| generator js { | |
| provider = 'prisma-client-js' | |
| } | |
| plugin zod { | |
| provider = '@core/zod' | |
| } | |
| model User { | |
| id Int @id @default(autoincrement()) | |
| email String @unique @email @endsWith('@zenstack.dev') | |
| password String | |
| @@validate(length(password, 6, 20)) | |
| } | |
| `, | |
| { addPrelude: false, pushDb: false } | |
| ); | |
| const schemas = zodSchemas.models; | |
| expect( | |
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123' }).success | |
| ).toBeTruthy(); | |
| expect( | |
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | |
| ).toBeFalsy(); | |
| expect( | |
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123' }).success | |
| ).toBeTruthy(); | |
| expect( | |
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | |
| ).toBeFalsy(); | |
| // Prisma create/update schema should always non-strict | |
| expect( | |
| schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x | |
| ).toBe(1); | |
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); | |
| expect( | |
| zodSchemas.input.UserInputSchema.create.safeParse({ | |
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123' }, | |
| }).success | |
| ).toBeTruthy(); | |
| expect( | |
| zodSchemas.input.UserInputSchema.create.safeParse({ | |
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }, | |
| }).success | |
| ).toBeFalsy(); | |
| }); | |
| const baseSchema = ` | |
| datasource db { | |
| provider = 'postgresql' | |
| url = env('DATABASE_URL') | |
| } | |
| generator js { | |
| provider = 'prisma-client-js' | |
| } | |
| plugin zod { | |
| provider = '@core/zod' | |
| // mode will be set per test | |
| } | |
| model User { | |
| id Int @id @default(autoincrement()) | |
| email String @unique @email @endsWith('@zenstack.dev') | |
| password String | |
| @@validate(length(password, 6, 20)) | |
| } | |
| `; | |
| it('is strict by default', async () => { | |
| const schema = baseSchema.replace('// mode will be set per test', ''); | |
| const { zodSchemas } = await loadSchema( | |
| schema, | |
| { addPrelude: false, pushDb: false } | |
| ); | |
| const schemas = zodSchemas.models; | |
| expect( | |
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123' }).success | |
| ).toBeTruthy(); | |
| expect( | |
| schemas.UserSchema.safeParse({ id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | |
| ).toBeFalsy(); | |
| expect( | |
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123' }).success | |
| ).toBeTruthy(); | |
| expect( | |
| schemas.UserCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).success | |
| ).toBeFalsy(); | |
| // Prisma create/update schema should always non-strict | |
| expect( | |
| schemas.UserPrismaCreateSchema.safeParse({ email: 'abc@zenstack.dev', password: 'abc123', x: 1 }).data.x | |
| ).toBe(1); | |
| expect(schemas.UserPrismaUpdateSchema.safeParse({ x: 1 }).data.x).toBe(1); | |
| expect( | |
| zodSchemas.input.UserInputSchema.create.safeParse({ | |
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123' }, | |
| }).success | |
| ).toBeTruthy(); | |
| expect( | |
| zodSchemas.input.UserInputSchema.create.safeParse({ | |
| data: { id: 1, email: 'abc@zenstack.dev', password: 'abc123', x: 1 }, | |
| }).success | |
| ).toBeFalsy(); | |
| }); |
| private ensureDiscriminatorSelection(model: string, args: any) { | ||
| const modelInfo = getModelInfo(this.options.modelMeta, model); | ||
| if (!modelInfo?.discriminator) { | ||
| return; | ||
| } | ||
|
|
||
| if (args.select && typeof args.select === 'object') { | ||
| args.select[modelInfo.discriminator] = true; | ||
| return; | ||
| } | ||
|
|
||
| if (args.omit && typeof args.omit === 'object') { | ||
| args.omit[modelInfo.discriminator] = false; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Verify the usage of args.omit in ensureDiscriminatorSelection
It appears that args.omit is being checked and modified, but omit is not a standard parameter in Prisma query arguments. Typically, Prisma uses select and include for field selection. Please ensure that omit is a valid and supported parameter in this context.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
packages/runtime/src/enhancements/node/create-enhancement.ts (1)
124-133: Consider refactoring the globalprocessIncludeRelationPayloadcallback.The TODO comment indicates that using a global callback shared among all enhancements is not ideal. This could lead to tight coupling and make the codebase harder to maintain. Refactoring to allow each enhancement to handle its own inclusion payload processing would promote modularity and reduce potential side effects.
Do you want me to propose a refactored design or open a GitHub issue to track this improvement?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/runtime/src/enhancements/node/create-enhancement.ts (4 hunks)
- packages/runtime/src/enhancements/node/delegate.ts (25 hunks)
- packages/runtime/src/enhancements/node/policy/index.ts (2 hunks)
- packages/runtime/src/enhancements/node/policy/policy-utils.ts (2 hunks)
- packages/runtime/src/enhancements/node/proxy.ts (2 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/enhancements/node/policy/policy-utils.ts
Additional context used
Biome
packages/runtime/src/enhancements/node/delegate.ts
[error] 361-361: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (13)
packages/runtime/src/enhancements/node/proxy.ts (2)
72-72: LGTM!Using
PrismaProxyActionsinstead ofkeyof PrismaProxyHandlerdirectly in the method signature improves readability by using a named type. The change is not introducing any functional changes.
87-87: LGTM!Using
PrismaProxyActionsinstead ofkeyof PrismaProxyHandlerdirectly in the method signature improves readability by using a named type. The change is not introducing any functional changes.tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (4)
272-329: LGTM!The test case is well-structured and covers the expected behavior comprehensively. It verifies that when an
Assetis marked as deleted, it cannot be accessed through thePostmodel, and that creating, updating, and deleting aPostwith the deleted flag set to true is correctly rejected by policy.
331-407: LGTM!The test case is well-structured and covers the expected behavior comprehensively. It checks that when a
Postis created, its fields are accessible. After marking thePostas deleted, it confirms that thePostfields are no longer readable, while theAssetfields remain accessible. It also verifies that updates and deletions respect the same policy constraints.
409-485: LGTM!The test case is well-structured and covers the expected behavior comprehensively. It follows the same logic as the previous case, checking accessibility before and after marking the
Postas deleted, ensuring that the policies are enforced correctly. It also verifies that updates and deletions respect the same policy constraints.
487-526: LGTM!The test case is well-structured and covers the expected behavior comprehensively. It creates
Postinstances for users with different IDs and verifies that certain fields are only accessible to specific users based on their ID, confirming that the policies are enforced as expected.packages/runtime/src/enhancements/node/delegate.ts (6)
111-126: [Duplicate Comment] Verify the usage ofargs.omitinensureDiscriminatorSelectionThe past review comment is still valid.
args.omitis being checked and modified, butomitis not a standard parameter in Prisma query arguments. Typically, Prisma usesselectandincludefor field selection. Please ensure thatomitis a valid and supported parameter in this context.
402-418: LGTM!The
sanitizeMutationPayloadfunction correctly validates the mutation payload by checking for auxiliary relation fields and throwing an error if any are detected. The implementation looks good.
Line range hint
178-224: Looks good!The changes to
injectSelectIncludeHierarchyimprove the logic by refetching the field select/include value and using it in subsequent operations. This ensures that the latest state is used after potential modifications during injection. The implementation is correct.
Line range hint
331-367: Nice enhancement!The changes to
injectConcreteIncludeRecursivelyintroduce a newincludePayloadobject and utilize theprocessIncludeRelationPayloadcallback from options to process the include payload. This provides flexibility for enhancements like 'policy' to perform additional operations on the include payload. The implementation looks good.Tools
Biome
[error] 361-361: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Line range hint
1242-1303: Good refactoring!The renaming of
assembleUptoassembleHierarchyimproves clarity and reflects the purpose of the function more accurately. The logic remains unchanged, recursively assembling the hierarchy of the entity. The refactoring is a positive change.
432-433: Validation enhancement!The changes to
createMany,update, andupdateManymethods to callsanitizeMutationPayloadon their respective data arguments are a good enhancement. This ensures that all mutation payloads are properly validated before proceeding with the operations, preventing any attempts to set auxiliary relation fields directly. The implementation is correct.Also applies to: 661-662, 682-683
packages/runtime/src/enhancements/node/policy/index.ts (1)
70-80: Inconsistent parameter usage in method calls.In the function
policyProcessIncludeRelationPayload, there is an inconsistency in how parameters are passed toutilsmethods:
- Line 78:
await utils.injectForRead(prisma, model, payload);- Line 79:
await utils.injectReadCheckSelect(model, payload);Since
prismais already provided when creating theutilsinstance, passingprismaagain toinjectForReadmay be redundant. For consistency and to avoid potential confusion, consider removing theprismaargument from theinjectForReadcall if it's not required.Apply this diff if
prismais not needed as a parameter:await utils.injectForRead( - prisma, model, payload );To verify if
prismais required as a parameter ininjectForRead, run the following script:Check the output to see if
injectForReadis defined withprismaas a parameter.
| /** | ||
| * Function for processing a payload for including a relation field in a query. | ||
| * @param model The relation's model name | ||
| * @param payload The payload to process | ||
| */ |
There was a problem hiding this comment.
Complete the JSDoc comments for all parameters.
The function policyProcessIncludeRelationPayload has several parameters, but the JSDoc comments only document model and payload. To improve code readability and maintainability, please add @param entries for prisma, options, and context.
Apply this diff to update the JSDoc:
/**
* Function for processing a payload for including a relation field in a query.
+ * @param prisma The Prisma client instance
* @param model The relation's model name
* @param payload The payload to process
+ * @param options Internal enhancement options
+ * @param context The enhancement context (optional)
*/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.
| /** | |
| * Function for processing a payload for including a relation field in a query. | |
| * @param model The relation's model name | |
| * @param payload The payload to process | |
| */ | |
| /** | |
| * Function for processing a payload for including a relation field in a query. | |
| * @param prisma The Prisma client instance | |
| * @param model The relation's model name | |
| * @param payload The payload to process | |
| * @param options Internal enhancement options | |
| * @param context The enhancement context (optional) | |
| */ |
| export async function policyProcessIncludeRelationPayload( | ||
| prisma: DbClientContract, | ||
| model: string, | ||
| payload: unknown, |
There was a problem hiding this comment.
Specify a more precise type for the 'payload' parameter.
Currently, payload is typed as unknown, which may lead to issues with type safety and code clarity. Consider defining a more specific type for payload to enhance maintainability and reduce potential errors.
Apply this diff to update the parameter type:
export async function policyProcessIncludeRelationPayload(
prisma: DbClientContract,
model: string,
- payload: unknown,
+ payload: SpecificPayloadType,
options: InternalEnhancementOptions,
context: EnhancementContext | undefined
) {Replace SpecificPayloadType with the appropriate interface or type that accurately represents the structure of payload.
Committable suggestion was skipped due to low confidence.
| processIncludeRelationPayload?: ( | ||
| prisma: DbClientContract, | ||
| model: string, | ||
| payload: unknown, | ||
| options: InternalEnhancementOptions, | ||
| context: EnhancementContext | undefined | ||
| ) => Promise<void>; |
There was a problem hiding this comment.
Specify a more precise type for the payload parameter.
The payload parameter in processIncludeRelationPayload is currently typed as unknown. Providing a more specific type can enhance type safety and improve code readability.
If there is a defined type that represents the structure of the payload (e.g., IncludeRelationPayload), consider updating the parameter type:
processIncludeRelationPayload?: (
prisma: DbClientContract,
model: string,
- payload: unknown,
+ payload: IncludeRelationPayload,
options: InternalEnhancementOptions,
context: EnhancementContext | undefined
) => Promise<void>;Committable suggestion was skipped due to low confidence.
1 participant
No description provided.