refactor: refactor policy definition and generation code #1468
Conversation
WalkthroughWalkthroughThe changes focus on refactoring and simplifying policy-related code across multiple packages. This includes removing field-level access control constants, restructuring policy definitions, and modifying test cases to remove the Changes
Sequence Diagram(s) (Beta)The changes are too varied and do not introduce a new feature or a clear modification to the control flow that can be visualized effectively with a sequence diagram. Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
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.
Actionable comments posted: 21
Outside diff range and nitpick comments (18)
packages/runtime/src/enhancements/types.ts (2)
3-3: Import statement for types should be grouped with other type imports.Consider grouping all type imports together for better readability and maintainability.
Line range hint
19-19: Usage ofanytype should be avoided where possible.Using
anytype can potentially lead to runtime errors and makes the code less safe. If possible, replaceanywith a more specific type or add appropriate type guards.Also applies to: 81-81, 86-86
tests/integration/tests/enhancements/with-policy/fluent-api.test.ts (2)
Line range hint
2-2: Importpathmodule using thenode:protocol.For clarity and to adhere to best practices in Node.js, import built-in modules using the
node:protocol:- import path from 'path'; + import path from 'node:path';
Line range hint
145-145: Avoid usinganytype to ensure type safety.Using
anycan lead to potential runtime errors and makes the codebase less robust. Consider specifying more precise types:- const rawEvents: any[] = []; + const rawEvents: Array<{ event: string }> = [];Also applies to: 257-257
tests/integration/tests/enhancements/with-policy/subscription.test.ts (2)
Line range hint
2-2: Importpathmodule using thenode:protocol.For clarity and to adhere to best practices in Node.js, import built-in modules using the
node:protocol:- import path from 'path'; + import path from 'node:path';
Line range hint
62-62: Specify types instead of usingany.Using
anytype can lead to potential runtime errors and makes the codebase less robust. Consider specifying more precise types for better type safety and code clarity.Also applies to: 63-63, 64-64, 119-119, 120-120, 173-173, 174-174, 242-242, 243-243, 254-254
tests/integration/tests/enhancements/with-policy/nested-to-one.test.ts (1)
Line range hint
2-2: Importpathmodule using thenode:protocol.For clarity and to adhere to best practices in Node.js, import built-in modules using the
node:protocol:- import path from 'path'; + import path from 'node:path';packages/testtools/src/schema.ts (5)
Line range hint
13-13: Consider using thenode:protocol when importing Node.js built-in modules.- import * as fs from 'fs'; + import * as fs from 'node:fs'; - import * as path from 'path'; + import * as path from 'node:path'; - import { execSync } from 'child_process'; + import { execSync } from 'node:child_process';Also applies to: 14-14, 16-16
Line range hint
36-42: Refactor to avoid usinganytype for better type safety.Consider specifying more precise types for the event types and callbacks in the
FullDbClientContractinterface to enhance type safety and code maintainability.
Line range hint
72-74: Thiselseclause can be omitted for cleaner code, as previous branches break early.- else { - curr = normalizePath(path.dirname(curr)); - }
Line range hint
251-254: Consider usingfor...ofinstead offorEachfor better performance and readability.- localInstallDeps.forEach((d) => path.join(workspaceRoot, d)).join(' ') + for (const d of localInstallDeps) { + path.join(workspaceRoot, d) + }Also applies to: 268-270
301-302: Theanytype is used for theenhanceandenhanceRawproperties. Consider specifying a more precise type.It would be beneficial to define explicit types for these properties to improve code clarity and ensure type safety.
tests/integration/tests/enhancements/with-policy/post-update.test.ts (1)
Line range hint
2-2: Node.js built-in modules should be imported using thenode:protocol to ensure compatibility and clarity.- import path from 'path'; + import path from 'node:path';tests/regression/tests/issues.test.ts (1)
Line range hint
2-2: Use thenode:protocol for importing Node.js built-in modules.- import path from 'path'; + import path from 'node:path';packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (2)
148-148: Avoid using template literals when not necessary.Consider replacing template literals with simple strings in these instances to improve code clarity and performance.
Also applies to: 162-162, 287-287, 301-301, 327-327, 367-367, 369-369, 376-376, 415-415, 418-418
Line range hint
242-248: Use template literals for better readability and maintainability.- let expr = denies.length > 0 ? '!(' + denies.map((deny) => { return transformer.transform(deny); }).join(' || ') + ')' : undefined; + let expr = denies.length > 0 ? `!(${denies.map(deny => transformer.transform(deny)).join(' || ')})` : undefined; - writer.write('return ' + expr); + writer.write(`return ${expr}`);Also applies to: 258-258, 262-262
tests/integration/tests/enhancements/with-policy/refactor.test.ts (2)
Line range hint
1-1: Consider using type-only imports for types that are only used for type annotations to prevent them from being included in the JavaScript output.- import { AuthUser, PrismaErrorCode } from '@zenstackhq/runtime'; + import type { AuthUser } from '@zenstackhq/runtime'; + import { PrismaErrorCode } from '@zenstackhq/runtime';
Line range hint
3-3: Use thenode:protocol when importing Node.js built-in modules to ensure compatibility and clarity.- import path from 'path'; + import path from 'node:path';
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (25)
- packages/runtime/src/constants.ts (1 hunks)
- packages/runtime/src/enhancements/policy/policy-utils.ts (9 hunks)
- packages/runtime/src/enhancements/types.ts (3 hunks)
- packages/schema/src/plugins/enhancer/policy/index.ts (1 hunks)
- packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (8 hunks)
- packages/schema/src/plugins/enhancer/policy/utils.ts (1 hunks)
- packages/sdk/src/policy.ts (1 hunks)
- packages/testtools/src/schema.ts (4 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (7 hunks)
- tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (2 hunks)
- tests/integration/tests/enhancements/with-policy/fluent-api.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/nested-to-one.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/post-update.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/prisma-omit.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/refactor.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-policy/subscription.test.ts (4 hunks)
- tests/integration/tests/plugins/policy.test.ts (3 hunks)
- tests/regression/tests/issue-1014.test.ts (1 hunks)
- tests/regression/tests/issue-1080.test.ts (1 hunks)
- tests/regression/tests/issue-1241.test.ts (1 hunks)
- tests/regression/tests/issue-1271.test.ts (1 hunks)
- tests/regression/tests/issue-1435.test.ts (1 hunks)
- tests/regression/tests/issue-1451.test.ts (1 hunks)
- tests/regression/tests/issue-961.test.ts (1 hunks)
- tests/regression/tests/issues.test.ts (1 hunks)
Files not summarized due to errors (1)
- packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts: Error: Message exceeds token limit
Files skipped from review due to trivial changes (6)
- packages/runtime/src/constants.ts
- tests/integration/tests/enhancements/with-policy/prisma-omit.test.ts
- tests/regression/tests/issue-1080.test.ts
- tests/regression/tests/issue-1271.test.ts
- tests/regression/tests/issue-1451.test.ts
- tests/regression/tests/issue-961.test.ts
Additional Context Used
Biome (123)
packages/runtime/src/enhancements/policy/policy-utils.ts (20)
26-26: Unexpected any. Specify a different type.
57-61: This else clause can be omitted because previous branches break early.
59-61: This else clause can be omitted because previous branches break early.
71-75: This else clause can be omitted because previous branches break early.
73-75: This else clause can be omitted because previous branches break early.
84-88: This else clause can be omitted because previous branches break early.
86-88: This else clause can be omitted because previous branches break early.
97-102: This else clause can be omitted because previous branches break early.
108-110: This else clause can be omitted because previous branches break early.
134-134: Unexpected any. Specify a different type.
135-135: Unexpected any. Specify a different type.
149-149: Unexpected any. Specify a different type.
154-154: The computed expression can be simplified without the use of a string literal.
166-166: Unexpected any. Specify a different type.
171-171: The computed expression can be simplified without the use of a string literal.
183-183: Unexpected any. Specify a different type.
191-191: Prefer for...of instead of forEach.
192-192: The computed expression can be simplified without the use of a string literal.
257-257: Unexpected any. Specify a different type.
355-355: Unexpected any. Specify a different type.
packages/runtime/src/enhancements/types.ts (4)
19-19: Unexpected any. Specify a different type.
81-81: Unexpected any. Specify a different type.
86-86: Unexpected any. Specify a different type.
1-2: All these imports are only used as types.
packages/schema/src/plugins/enhancer/policy/index.ts (1)
1-1: All these imports are only used as types.
packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts (20)
30-30: A Node.js builtin module should be imported with the node: protocol.
148-148: Do not use template literals if interpolation and special-character handling are not needed.
162-162: Do not use template literals if interpolation and special-character handling are not needed.
178-178: Forbidden non-null assertion.
242-248: Template literals are preferred over string concatenation.
258-258: Template literals are preferred over string concatenation.
262-262: Template literals are preferred over string concatenation.
287-287: Do not use template literals if interpolation and special-character handling are not needed.
301-301: Do not use template literals if interpolation and special-character handling are not needed.
327-327: Do not use template literals if interpolation and special-character handling are not needed.
367-367: Do not use template literals if interpolation and special-character handling are not needed.
369-369: Do not use template literals if interpolation and special-character handling are not needed.
376-376: Do not use template literals if interpolation and special-character handling are not needed.
388-388: Forbidden non-null assertion.
415-415: Do not use template literals if interpolation and special-character handling are not needed.
417-420: This else clause can be omitted because previous branches break early.
418-418: Do not use template literals if interpolation and special-character handling are not needed.
424-424: Forbidden non-null assertion.
491-491: Forbidden non-null assertion.
505-505: Forbidden non-null assertion.
packages/schema/src/plugins/enhancer/policy/utils.ts (18)
58-60: This else clause can be omitted because previous branches break early.
100-104: This else clause can be omitted because previous branches break early.
112-112: Unexpected any. Specify a different type.
178-180: This else clause can be omitted because previous branches break early.
181-183: This else clause can be omitted because previous branches break early.
184-203: This else clause can be omitted because previous branches break early.
191-193: This else clause can be omitted because previous branches break early.
194-203: This else clause can be omitted because previous branches break early.
197-203: This else clause can be omitted because previous branches break early.
208-208: Prefer for...of instead of forEach.
251-253: Prefer for...of instead of forEach.
254-256: Prefer for...of instead of forEach.
260-262: This else clause can be omitted because previous branches break early.
318-318: Do not use template literals if interpolation and special-character handling are not needed.
325-325: Template literals are preferred over string concatenation.
367-367: Do not use template literals if interpolation and special-character handling are not needed.
18-32: Some named imports are only used as types.
33-34: All these imports are only used as types.
packages/testtools/src/schema.ts (20)
13-13: A Node.js builtin module should be imported with the node: protocol.
14-14: A Node.js builtin module should be imported with the node: protocol.
16-16: A Node.js builtin module should be imported with the node: protocol.
36-36: Unexpected any. Specify a different type.
36-36: Unexpected any. Specify a different type.
37-37: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
39-39: Unexpected any. Specify a different type.
40-40: Unexpected any. Specify a different type.
40-40: Unexpected any. Specify a different type.
41-41: Unexpected any. Specify a different type.
42-42: Unexpected any. Specify a different type.
72-74: This else clause can be omitted because previous branches break early.
251-254: Prefer for...of instead of forEach.
268-270: Prefer for...of instead of forEach.
299-299: Unexpected any. Specify a different type.
300-300: Unexpected any. Specify a different type.
302-302: Unexpected any. Specify a different type.
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (3)
237-237: This let declares a variable that is only assigned once.
624-624: This let declares a variable that is only assigned once.
631-631: This let declares a variable that is only assigned once.
tests/integration/tests/enhancements/with-policy/fluent-api.test.ts (3)
2-2: A Node.js builtin module should be imported with the node: protocol.
145-145: Unexpected any. Specify a different type.
257-257: Unexpected any. Specify a different type.
tests/integration/tests/enhancements/with-policy/nested-to-one.test.ts (1)
2-2: A Node.js builtin module should be imported with the node: protocol.
tests/integration/tests/enhancements/with-policy/post-update.test.ts (1)
2-2: A Node.js builtin module should be imported with the node: protocol.
tests/integration/tests/enhancements/with-policy/refactor.test.ts (2)
3-3: A Node.js builtin module should be imported with the node: protocol.
1-1: Some named imports are only used as types.
tests/integration/tests/enhancements/with-policy/subscription.test.ts (12)
2-2: A Node.js builtin module should be imported with the node: protocol.
62-62: Unexpected any. Specify a different type.
63-63: Unexpected any. Specify a different type.
64-64: Unexpected any. Specify a different type.
119-119: Unexpected any. Specify a different type.
120-120: Unexpected any. Specify a different type.
173-173: Unexpected any. Specify a different type.
174-174: Unexpected any. Specify a different type.
242-242: Unexpected any. Specify a different type.
243-243: Unexpected any. Specify a different type.
254-254: Unexpected any. Specify a different type.
254-254: Unexpected any. Specify a different type.
tests/integration/tests/plugins/policy.test.ts (14)
41-41: Don't use 'Function' as a type.
42-42: Don't use 'Function' as a type.
44-44: Don't use 'Function' as a type.
45-45: Don't use 'Function' as a type.
46-46: Don't use 'Function' as a type.
47-47: Don't use 'Function' as a type.
49-49: Don't use 'Function' as a type.
50-50: Don't use 'Function' as a type.
51-51: Don't use 'Function' as a type.
52-52: Don't use 'Function' as a type.
71-71: Don't use 'Function' as a type.
74-74: Don't use 'Function' as a type.
104-104: Don't use 'Function' as a type.
108-108: Don't use 'Function' as a type.
tests/regression/tests/issue-1241.test.ts (2)
2-2: A Node.js builtin module should be imported with the node: protocol.
75-75: Unexpected any. Specify a different type.
tests/regression/tests/issue-1435.test.ts (1)
5-5: Unexpected any. Specify a different type.
tests/regression/tests/issues.test.ts (1)
2-2: A Node.js builtin module should be imported with the node: protocol.
Additional comments not posted (30)
packages/schema/src/plugins/enhancer/policy/index.ts (1)
7-7: LGTM! The refactoring simplifies the policy generation process by directly using thePolicyGenerator.tests/regression/tests/issue-1014.test.ts (1)
40-40: The test setup and assertions are clear and well-structured.packages/sdk/src/policy.ts (1)
5-6: The addition ofPolicyAnalysisResultenhances type safety and clarity in policy analysis functions.packages/runtime/src/enhancements/types.ts (1)
92-93: Refactored policy definitions to enhance clarity and modularity.The restructuring of policy definitions into
ModelPolicyDefandFieldCrudDefalong with the introduction of Zod schemas for validation significantly improves the clarity and modularity of the policy system.Also applies to: 102-102, 105-219, 224-231
tests/integration/tests/enhancements/with-delegate/policy-interaction.test.ts (2)
92-95: Enhanced database instance creation and usage.The refactoring to use
enhancedirectly withprismaand specific user contexts simplifies the setup and makes the test scenarios clearer.
188-188: Simplified test setup for policy interaction.The streamlined setup using
loadSchemaandenhancedirectly in the test case reduces boilerplate and focuses on the policy interaction logic.tests/integration/tests/enhancements/with-policy/fluent-api.test.ts (1)
44-44: Ensure the schema definition is correctly terminated.The schema definition appears to be abruptly terminated. Please verify if this is intentional or if additional configuration is missing:
packages/schema/src/plugins/enhancer/policy/utils.ts (1)
194-203: Theelseclause here is unnecessary and can be removed to simplify the code.- } else { - // recurse - const children = streamContents(expr) - .filter((child): child is Expression => isExpression(child)) - .toArray(); - return children.flatMap((child) => collectReferencePaths(child)); - } + return [];Likely invalid or redundant comment.
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (7)
24-24: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
103-103: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
112-112: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
125-125: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
143-143: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
901-901: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.
9-9: Ensure the removal oflogPrismaQuery: truedoes not affect the expected behavior of logging during tests.packages/runtime/src/enhancements/policy/policy-utils.ts (15)
8-8: Ensure the newly added imports are utilized effectively in the code.
223-230: The definition ofFULL_OPEN_MODEL_POLICYas a constant within the class is a good practice for clarity and reusability.
233-244: The methodgetModelPolicyDefcorrectly handles the scenario where policy enhancement is not enabled by returning a fully open model policy. This is a robust design choice.
246-248: The methodgetModelGuardForOperationefficiently retrieves the guard for a specific operation. It uses nullish coalescing to provide a default value, which enhances fault tolerance.
258-266: The methodgetAuthGuardeffectively handles different types of guards (boolean or function). This polymorphism increases the flexibility of guard definitions.
274-286: The methodgetFieldOverrideReadAuthGuardprovides a clear mechanism for handling field-level read authorization with an override option. This is crucial for fine-grained access control.
294-306: The methodgetFieldUpdateAuthGuardis well-implemented, providing a default behavior when no specific guard is defined. This default to true (allow) is a sensible choice for update operations.
314-326: The methodgetFieldOverrideUpdateAuthGuardmirrors the structure ofgetFieldOverrideReadAuthGuard, providing consistency in handling override guards for update operations.
334-335: The methodhasAuthGuardis concise and correctly determines if a non-default guard exists for a model operation.
342-347: The methodhasOverrideAuthGuardis well-designed to check for the existence of any field-level override guards for read and update operations, which is essential for dynamic policy enforcement.
356-367: The methodcheckInputGuardprovides a mechanism to evaluate input guards statically, which is a critical feature for validating operations at the model creation phase.
555-577: The methodgetCheckerConstrainthandles the absence of policy enhancement gracefully by returning a constant true, and throws an informative error when necessary. This robust error handling is commendable.
1098-1099: The methodhasFieldLevelPolicyis a simple and effective way to check for the existence of field-level policies, which is essential for conditional logic in policy enforcement.
15-15: The addition of new type imports from../typesis crucial for the refactoring. Verify that these types are correctly used throughout the class.Verification successful
The newly added types
CheckerFunc,ModelPolicyDef,PolicyDef,PolicyFunc, andZodSchemasare correctly used throughout thePolicyUtilclass.
PolicyDefis used as a type for thepolicyproperty.ZodSchemasis used as a type for thezodSchemasproperty.ModelPolicyDefis used in theFULL_OPEN_MODEL_POLICYproperty and thegetModelPolicyDefmethod.PolicyFuncis used in thegetModelGuardForOperationmethod.CheckerFuncis used in thegetCheckerConstraintmethod.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of newly added types in `PolicyUtil` class. # Test: Search for the usage of the types. Expect: Usage instances in method signatures and type annotations. rg --type typescript --multiline $'type { CheckerFunc, ModelPolicyDef, PolicyDef, PolicyFunc, ZodSchemas }' packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 326
Script:
#!/bin/bash # Description: Verify the usage of newly added types in `PolicyUtil` class. # Test: Search for the usage of the types. Expect: Usage instances in method signatures and type annotations. rg 'CheckerFunc|ModelPolicyDef|PolicyDef|PolicyFunc|ZodSchemas' packages/runtime/src/enhancements/policy/policy-utils.tsLength of output: 1205
1072-1089: The methodsgetPreValueSelect,getFieldReadCheckSelector, andcheckReadFieldare crucial for fetching and checking field-level read permissions. Ensure these methods are covered by unit tests to verify their correctness.
| const denies = getPolicyExpressions(model, 'deny', 'create'); | ||
| if (this.canCheckCreateBasedOnInput(model, allows, denies)) { | ||
| const inputCheckFunc = this.generateCreateInputCheckerFunction(model, allows, denies, sourceFile); | ||
| writer.write(`inputChecker: ${inputCheckFunc.getName()!},`); |
There was a problem hiding this comment.
Avoid using non-null assertions.
Non-null assertions can lead to runtime errors if not handled properly. Consider adding null checks or using optional chaining.
Also applies to: 388-388, 424-424, 491-491, 505-505
| } else { | ||
| // recurse | ||
| const children = streamContents(expr) | ||
| .filter((child): child is Expression => isExpression(child)) | ||
| .toArray(); | ||
| return children.flatMap((child) => collectReferencePaths(child)); | ||
| writer.write(`permissionChecker: true,`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This else clause can be omitted because previous branches break early.
- } else {
- writer.write(`permissionChecker: true,`);
- return;
- }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.
| } else { | |
| // recurse | |
| const children = streamContents(expr) | |
| .filter((child): child is Expression => isExpression(child)) | |
| .toArray(); | |
| return children.flatMap((child) => collectReferencePaths(child)); | |
| writer.write(`permissionChecker: true,`); | |
| return; | |
| } | |
| writer.write(`permissionChecker: true,`); | |
| return; |
| } from '@zenstackhq/sdk'; | ||
| import { getPrismaClientImportSpec } from '@zenstackhq/sdk/prisma'; | ||
| import { streamAllContents, streamAst, streamContents } from 'langium'; | ||
| import { streamAst } from 'langium'; | ||
| import { lowerCaseFirst } from 'lower-case-first'; | ||
| import path from 'path'; |
There was a problem hiding this comment.
Use the node: protocol for importing Node.js built-in modules.
- import path from 'path';
+ import path from 'node:path';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.
| import path from 'path'; | |
| import path from 'node:path'; |
| @@ -311,7 +310,7 @@ export async function loadSchema(schema: string, options?: SchemaLoadOptions) { | |||
| : path.join(projectDir, opt.output) | |||
| : path.join(projectDir, 'node_modules', DEFAULT_RUNTIME_LOAD_PATH); | |||
|
|
|||
| const policy = require(path.join(outputPath, 'policy')).default; | |||
| const policy: PolicyDef = require(path.join(outputPath, 'policy')).default; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process revealed that none of the identified policy modules contain a default export, except for packages/schema/src/plugins/enhancer/policy/policy-guard-generator.ts. This indicates a potential issue with the dynamic require statement in the code.
packages/runtime/src/enhancements/policy/policy-utils.tsneeds verification.packages/sdk/src/policy.tsneeds verification.tests/integration/tests/enhancements/with-delegate/policy-interaction.test.tsneeds verification.tests/integration/tests/enhancements/with-policy/empty-policy.test.tsneeds verification.tests/integration/tests/enhancements/with-policy/field-level-policy.test.tsneeds verification.tests/integration/tests/plugins/policy.test.tsneeds verification.
Analysis chain
The policy variable is dynamically required which might lead to runtime errors if the module is not found or the default export is not as expected.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the 'policy' module exists and contains a default export.
# Test: Check for the existence of the policy module and its structure.
if [ -f "$(dirname ${outputPath})/policy.js" ] && grep -q "export default" "$(dirname ${outputPath})/policy.js"; then
echo "Policy module is correctly structured."
else
echo "Policy module structure needs verification."
fi
Length of output: 199
Script:
#!/bin/bash
# Description: Verify that the 'policy' module exists and contains a default export.
# Test: Check for the existence of the policy module and its structure.
# Find all instances of 'policy.js' in the repository
policy_files=$(fd policy.js)
# Check each found file for a default export
for file in $policy_files; do
if grep -q "export default" "$file"; then
echo "Policy module $file is correctly structured."
else
echo "Policy module $file structure needs verification."
fi
done
Length of output: 31
Script:
#!/bin/bash
# Description: Verify that the 'policy' module exists and contains a default export.
# Test: Check for the existence of the policy module and its structure.
# Find all instances of 'policy' files in the repository (considering possible extensions)
policy_files=$(fd policy --extension js --extension ts --extension mjs --extension cjs)
# Check each found file for a default export
for file in $policy_files; do
if grep -q "export default" "$file"; then
echo "Policy module $file is correctly structured."
else
echo "Policy module $file structure needs verification."
fi
done
Length of output: 2845
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| import { z } from 'zod'; |
There was a problem hiding this comment.
Imports are only used as types, consider using import type.
To clarify that these imports are only used for type checking and not included in the JavaScript output, you can use the import type statement:
- import { z } from 'zod';
+ import type { z } from 'zod';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.
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |
| import { z } from 'zod'; | |
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |
| import type { z } from 'zod'; |
| denies.forEach((rule) => { | ||
| writer.write(`if (${transformer.transform(rule, false)}) { return ${FALSE}; }`); | ||
| }); |
There was a problem hiding this comment.
Use for...of loops instead of forEach for better performance and readability.
- denies.forEach((rule) => {
- writer.write(`if (${transformer.transform(rule, false)}) { return ${FALSE}; }`);
- });
- allows.forEach((rule) => {
- writer.write(`if (${transformer.transform(rule, false)}) { return ${TRUE}; }`);
- });
+ for (const rule of denies) {
+ writer.write(`if (${transformer.transform(rule, false)}) { return ${FALSE}; }`);
+ }
+ for (const rule of allows) {
+ writer.write(`if (${transformer.transform(rule, false)}) { return ${TRUE}; }`);
+ }Also applies to: 254-256
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.
| denies.forEach((rule) => { | |
| writer.write(`if (${transformer.transform(rule, false)}) { return ${FALSE}; }`); | |
| }); | |
| for (const rule of denies) { | |
| writer.write(`if (${transformer.transform(rule, false)}) { return ${FALSE}; }`); | |
| } | |
| for (const rule of allows) { | |
| writer.write(`if (${transformer.transform(rule, false)}) { return ${TRUE}; }`); | |
| } |
| @@ -38,8 +38,7 @@ describe('issue 1241', () => { | |||
|
|
|||
| @@allow('all', true) | |||
| } | |||
| `, | |||
| { logPrismaQuery: true } | |||
| ` | |||
There was a problem hiding this comment.
Consider specifying a more precise type than any for the prisma object to enhance type safety and code clarity.
| expect((m.read.guard as Function)({ user: undefined })).toEqual(FALSE); | ||
| expect((m.read.guard as Function)({ user: { id: '1' } })).toEqual(TRUE); | ||
|
|
||
| expect((m.create.guard as Function)({ user: undefined })).toEqual(FALSE); | ||
| expect((m.create.guard as Function)({ user: { id: '1' } })).toEqual(FALSE); | ||
| expect((m.create.guard as Function)({ user: { id: '1', value: 0 } })).toEqual(FALSE); | ||
| expect((m.create.guard as Function)({ user: { id: '1', value: 1 } })).toEqual(TRUE); | ||
|
|
||
| expect((m.update.guard as Function)({ user: undefined })).toEqual(FALSE); | ||
| expect((m.update.guard as Function)({ user: { id: '1' } })).toEqual(FALSE); | ||
| expect((m.update.guard as Function)({ user: { id: '1', value: 0 } })).toEqual(FALSE); | ||
| expect((m.update.guard as Function)({ user: { id: '1', value: 1 } })).toEqual(TRUE); |
There was a problem hiding this comment.
Replace Function with a more specific function type to enhance type safety and maintainability.
Also applies to: 71-74, 104-109
| @@ -83,7 +83,7 @@ describe('issue 1435', () => { | |||
| reference String @id | |||
| } | |||
| `, | |||
| { provider: 'postgresql', dbUrl, logPrismaQuery: true } | |||
| { provider: 'postgresql', dbUrl } | |||
There was a problem hiding this comment.
Consider specifying a more precise type than any for the prisma object to enhance type safety and code clarity.
| } else if (isCollectionPredicate(expr)) { | ||
| const path = visit(expr.left); | ||
| if (path) { | ||
| // recurse into RHS | ||
| const rhs = collectReferencePaths(expr.right); | ||
| // combine path of LHS and RHS | ||
| return rhs.map((r) => [...path, ...r]); | ||
| } else { | ||
| return []; | ||
| } | ||
| } else if (isInvocationExpr(expr)) { | ||
| // recurse into function arguments | ||
| return expr.args.flatMap((arg) => collectReferencePaths(arg.value)); | ||
| } else { | ||
| // recurse | ||
| const children = streamContents(expr) | ||
| .filter((child): child is Expression => isExpression(child)) | ||
| .toArray(); | ||
| return children.flatMap((child) => collectReferencePaths(child)); | ||
| } |
There was a problem hiding this comment.
The else clause here is unnecessary and can be removed to simplify the code.
- } else if (isCollectionPredicate(expr)) {
- const path = visit(expr.left);
- if (path) {
- // recurse into RHS
- const rhs = collectReferencePaths(expr.right);
- // combine path of LHS and RHS
- return rhs.map((r) => [...path, ...r]);
- } else {
- return [];
- }
- } else if (isInvocationExpr(expr)) {
- // recurse into function arguments
- return expr.args.flatMap((arg) => collectReferencePaths(arg.value));
- } else {
- // recurse
- const children = streamContents(expr)
- .filter((child): child is Expression => isExpression(child))
- .toArray();
- return children.flatMap((child) => collectReferencePaths(child));
- }
+ return [];Committable suggestion was skipped due low confidence.
No description provided.