feat(zmodel): add new functions currentModel and currentOperation
#1925
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the ZModel language server and SDK, focusing on adding two new functions, Changes
Sequence DiagramsequenceDiagram
participant Developer
participant ZModel
participant Validator
participant Transformer
Developer->>ZModel: Define policy with currentModel/currentOperation
ZModel->>Validator: Validate function arguments
Validator-->>ZModel: Validate casing options
ZModel->>Transformer: Transform expression
Transformer->>Transformer: Apply case transformation
Transformer-->>ZModel: Return transformed value
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Nitpick comments (3)
packages/sdk/src/typescript-expression-transformer.ts (1)
315-326: Potential string-escaping edge case
toStringWithCaseChangehandles casing well. However, if the string contains quotes or special characters, inline quoting might produce unexpected results. Consider adding or documenting string sanitization if user input is processed.packages/sdk/src/code-gen.ts (1)
50-54: Potential hidden type errors
Disabling type checking across all source files hides potential errors. If feasible, consider selectively disabling or addressing specific type errors to maintain type safety.packages/schema/src/res/stdlib.zmodel (1)
185-193: Document the “expanded” logic for theoperationmore thoroughly.
The doc comments reference that “all” operations are expanded to multiple operations. If logic is needed to reflect that expansion at runtime (or compile time), clarify how it’s handled or validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/schema/src/language-server/validator/function-invocation-validator.ts(1 hunks)packages/schema/src/res/stdlib.zmodel(1 hunks)packages/sdk/src/code-gen.ts(1 hunks)packages/sdk/src/typescript-expression-transformer.ts(10 hunks)tests/integration/tests/enhancements/with-policy/currentModel.test.ts(1 hunks)tests/integration/tests/enhancements/with-policy/currentOperation.test.ts(1 hunks)
🔇 Additional comments (27)
packages/sdk/src/typescript-expression-transformer.ts (6)
23-23: Import usage looks good
The import from langium is necessary for retrieving the containing data model. No concerns here.
44-45: Type definition is concise
Defining Casing as a union of string literals is clear and easy to maintain. Good approach for typed casing options.
156-156: Invocation argument pass-through
Calling handler.value.call(this, expr, args, normalizeUndefined) is consistent. Ensure handlers adapt to the new argument signature as expected.
327-327: No actionable change
This line is just a blank line. No issues here.
328-341: _currentModel
Logic properly retrieves the data model container and applies casing. Ensure all calling sites provide valid usage context since an error is thrown if no containing model is found.
342-360: _currentOperation
Mapping postUpdate to update ensures consistent naming. Good approach for unifying operation context with the new casing options.
tests/integration/tests/enhancements/with-policy/currentOperation.test.ts (9)
1-2: Imports
Imports from @zenstackhq/testtools are correct for loading schemas and errors. No issues here.
3-23: Test: works with specific rules
Thoroughly tests creation policies for User vs. Post. Covers the allow/deny logic well.
25-44: Test: works with all rule
Good demonstration of using @@allow('all', ...). Ensures policies apply consistently across CRUD operations, then verifying create specifically.
46-65: Test: works with upper case
Ensures currentOperation('upper') is recognized as 'CREATE' or 'READ'. Good coverage for uppercase transformations.
67-86: Test: works with lower case
Consistently checks 'create' vs. 'read' in lowercase. Demonstrates the function transformation well.
88-107: Test: works with capitalization
Validates 'Create' vs. 'create'. Ensures partial changes in letter casing are handled properly.
109-128: Test: works with uncapitalization
Similar to the previous test, but ensures the function returns 'create' in certain contexts. Good coverage for 'uncapitalize'.
130-140: Test: complains when used outside policies
Properly checks error handling for disallowed usage in a @default context. This ensures the function is restricted to an access policy scope.
142-153: Test: complains when casing argument is invalid
Confirms that an invalid argument triggers a descriptive error. Solid coverage for input validation.
tests/integration/tests/enhancements/with-policy/currentModel.test.ts (10)
1-2: Imports
No concerns. Standard test scaffolding from @zenstackhq/testtools.
3-24: Test: basic usage
Tests that User creation is allowed and Post creation is rejected. Good basic coverage of currentModel().
26-46: Test: upper case
Proves 'upper' transformation is recognized. Ensures @@allow('create', currentModel('upper') == 'USER').
48-68: Test: lower case
Checks 'lower' transformation. Good coverage for verifying that @@allow('create', currentModel('lower') == 'user') is enforced.
70-90: Test: capitalization
Validates 'capitalize' scenario. Demonstrates how a lowercased model name becomes 'User'.
92-112: Test: uncapitalization
Ensures 'uncapitalize' scenario flips the first character to lowercase. Good coverage for unusual naming styles.
114-134: Test: abstract base inheritance
Checks consistent behavior of currentModel() in child models. Ensures inherited attributes respect the same policy rules.
136-159: Test: delegate base inheritance
Same principle as the abstract base test, validating that currentModel() usage is consistent even when @@delegate is involved.
161-171: Test: complains when used outside policies
Properly verifies error message when currentModel() is used in a @default context.
173-184: Test: complains when casing argument is invalid
Ensures that the validator enforces one of the recognized casing options, rejecting 'foo'.
packages/schema/src/language-server/validator/function-invocation-validator.ts (1)
90-100: Validation for currentModel and currentOperation casing
Checking that the first argument is one of the allowed cases is appropriate. This prevents invalid usage and ensures consistency with the transformer. Consider referencing the same enumeration in the code for stronger consistency.
packages/schema/src/res/stdlib.zmodel (1)
174-181: Clarify the default behavior for the casing parameter.
The documentation indicates that the default is "original", but it's unclear what happens if the parameter is null or any unspecified string. Consider clarifying the fallback behavior or throwing a validation error when casing is invalid.
Would you like me to open a ticket to add parameter validation and default fallback logic within the function body?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration/tests/enhancements/with-policy/currentOperation.test.ts (3)
4-23: Consider expanding test coverage for other CRUD operations.While the test effectively validates the
createoperation, consider adding test cases for other operations (read,update,delete) to ensure comprehensive coverage.// Example additional test cases: await expect(db.user.findMany()).toResolveTruthy(); await expect(db.user.update({ where: { id: 1 }, data: { id: 2 } })).toResolveTruthy(); await expect(db.user.delete({ where: { id: 1 } })).toResolveTruthy();
130-140: Consider testing additional invalid contexts.While the test validates usage in
DefaultValuecontext, consider adding test cases for other invalid contexts (e.g.,@unique,@relation) to ensure comprehensive error handling.// Example additional test cases: await expect(loadModelWithError(` model User { id String @id email String @unique(sort: currentOperation()) } `)).resolves.toContain('not allowed in the current context: UniqueConstraint');
142-154: Consider testing non-string argument cases.While the test validates invalid string arguments, consider adding test cases for non-string arguments (e.g., numbers, booleans) to ensure robust type checking.
// Example additional test cases: await expect(loadModelWithError(` model User { id String @id @@allow('create', currentOperation(123) == 'create') } `)).resolves.toContain('argument must be a string literal');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/tests/enhancements/with-policy/currentOperation.test.ts(1 hunks)
🔇 Additional comments (5)
tests/integration/tests/enhancements/with-policy/currentOperation.test.ts (5)
46-86: LGTM! Comprehensive case sensitivity testing.
The test cases effectively cover both upper and lower case transformations with appropriate assertions for both success and failure scenarios.
88-107: LGTM! Well-structured capitalization tests.
The test cases properly validate the capitalization transformation with appropriate positive and negative scenarios.
109-128: LGTM! Thorough uncapitalization tests.
The test cases effectively validate the uncapitalization transformation with proper assertions.
1-154: Overall excellent test coverage with minor enhancement opportunities.
The test suite is well-structured and provides comprehensive coverage of the currentOperation function's features. It effectively validates:
- Basic functionality with specific rules
- Integration with 'all' permission rules
- Various case transformation scenarios
- Error handling for invalid contexts and arguments
The test organization is clean, and the assertions are meaningful. The suggested enhancements would further strengthen the test suite but are not critical for the current implementation.
25-44: Verify complete coverage of 'all' rule behavior.
The test validates that 'all' rule works with create operation, but it would be valuable to verify that it actually permits all operations when the condition is met.
✅ Verification successful
Let me search for more specific information about the currentOperation() function and its behavior.
Let me search for the implementation details of the 'all' permission rule.
Let me try a different search pattern to find the implementation of permissions.
Based on the test files and implementation, I can now provide a definitive response about the 'all' rule behavior.
Test coverage for 'all' rule is sufficient.
The test correctly validates that the 'all' rule works with the currentOperation() function. Looking at the test context:
-
The test verifies that when
@@allow('all', currentOperation() == 'create')is true:- The create operation is allowed (tested by
db.user.create()resolving) - Other operations are denied when the condition is false (tested by
db.post.create()being rejected)
- The create operation is allowed (tested by
-
The 'all' permission rule is a shorthand that applies the condition to all operations (create, read, update, delete), which is evident from other test files in the codebase that demonstrate this behavior, particularly in:
tests/integration/tests/enhancements/with-policy/toplevel-operations.test.tstests/integration/tests/enhancements/with-policy/connect-disconnect.test.ts
Therefore, the test provides adequate coverage of the 'all' rule behavior with currentOperation().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for documentation or implementation of 'all' rule behavior
rg -A 5 "@@allow\('all'" --type ts
Length of output: 67196
Script:
#!/bin/bash
# Search for currentOperation function implementation or tests
rg -A 5 "currentOperation\(\)" --type ts
Length of output: 3528
Script:
#!/bin/bash
# Search for implementation or documentation of 'all' permission
rg -A 10 "permission.*all" --type ts
Length of output: 38
Script:
#!/bin/bash
# Search for permission operations implementation
rg -A 10 "@@allow\('(create|read|update|delete|all)'" --type ts
Length of output: 76272
No description provided.