fix: validating currentModel and currentOperation properly #300
Conversation
WalkthroughRefactors function-invocation validator to centralize expression-context and validation-attribute detection via new private helpers. Adds multiple regression tests under tests/regression/test/v2-migrated covering issues 1955, 1964, 1978 (skipped), 1984, 1991, 1992, 1993 (skipped), 1994, 1997, and 1998. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job Code Review: PR #300 - Fix validating currentModel and currentOperation properlyTodo List:
SummaryThis PR fixes a critical validation bug where Code Quality Assessment✅ Excellent RefactoringThe main change extracts complex validation logic into well-structured private methods: packages/language/src/validators/function-invocation-validator.ts:101-117 private getExpressionContext(containerAttribute: DataModelAttribute | DataFieldAttribute | undefined) {
if (!containerAttribute) {
return undefined;
}
if (this.isValidationAttribute(containerAttribute)) {
return ExpressionContext.ValidationRule;
}
return match(containerAttribute?.decl.$refText)
.with('@default', () => ExpressionContext.DefaultValue)
.with(P.union('@@allow', '@@deny', '@allow', '@deny'), () => ExpressionContext.AccessPolicy)
.with('@@index', () => ExpressionContext.Index)
.otherwise(() => undefined);
}
private isValidationAttribute(attr: DataModelAttribute | DataFieldAttribute) {
return attr.decl.ref?.attributes.some((attr) => attr.decl.$refText === '@@@validation');
}✅ Proper Error HandlingThe fix correctly identifies validation contexts and produces clear error messages like: Bug Analysis🐛 Fixed Critical Bug
✅ Validation Logic CorrectnessAccording to the stdlib definition, these functions should only work in function currentModel(casing: String?): String {
} @@@expressionContext([AccessPolicy])
function currentOperation(casing: String?): String {
} @@@expressionContext([AccessPolicy])Test Coverage✅ Comprehensive Test SuiteThe PR adds 10 regression tests covering various scenarios:
✅ Test QualityTests follow good patterns:
Performance Considerations✅ Minimal Performance Impact
Security Considerations✅ Improves Security Posture
Minor Suggestions🔍 Type Safety EnhancementConsider adding stricter typing to private isValidationAttribute(attr: DataModelAttribute | DataFieldAttribute): attr is DataModelAttribute | DataFieldAttribute {
return attr.decl.ref?.attributes.some((attr) => attr.decl.$refText === '@@@validation') ?? false;
}📝 DocumentationThe extracted methods would benefit from JSDoc comments explaining their purpose and the Overall Assessment🟢 Excellent Quality
This is a high-quality fix that properly resolves the validation context issue while improving code maintainability. The extensive test coverage ensures the fix works correctly across various scenarios. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds validation to ensure that currentModel() and currentOperation() functions are properly restricted to access policy contexts only, preventing their misuse in validation rules.
Key Changes:
- Enhanced function invocation validation to properly detect validation attributes
- Added regression tests for issues 1955, 1964, 1978, 1984, 1991-1998
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/src/validators/function-invocation-validator.ts | Refactored expression context detection to properly identify validation attributes and restrict context-specific functions |
| tests/regression/test/v2-migrated/issue-1984.test.ts | Tests verifying currentModel/currentOperation are rejected in validation contexts but work in access policies |
| tests/regression/test/v2-migrated/issue-1955.test.ts | Tests for createManyAndReturn/updateManyAndReturn operations with policies |
| tests/regression/test/v2-migrated/issue-1964.test.ts | Tests for nested deleteMany operations with access policies |
| tests/regression/test/v2-migrated/issue-1978.test.ts | Tests for field-level policy support (skipped - feature not implemented) |
| tests/regression/test/v2-migrated/issue-1991.test.ts | Tests for JSON type handling in schema generation |
| tests/regression/test/v2-migrated/issue-1992.test.ts | Tests for model delegation with multiple user types |
| tests/regression/test/v2-migrated/issue-1993.test.ts | Tests for Zod schema generation with delegated models (skipped) |
| tests/regression/test/v2-migrated/issue-1994.test.ts | Tests for nested create operations with delegated models |
| tests/regression/test/v2-migrated/issue-1997.test.ts | Tests for createMany operations with composite keys |
| tests/regression/test/v2-migrated/issue-1998.test.ts | Tests for deleteMany operations with check() function in access policies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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 (7)
tests/regression/test/v2-migrated/issue-1992.test.ts (1)
1-60: LGTM! Schema validation test is correctly structured.The regression test properly validates that a schema with delegation-based inheritance and polymorphic models can be loaded without validation errors. The implicit assertion pattern (test passes if
loadSchemadoesn't throw) is standard and appropriate for schema validation tests.The schema covers the essential scenario:
- Enum-driven delegation via
@@delegate(type)- Base model (
MyAppUser) with three extending subclasses- Proper relations and unique constraints in
MyAppUserFolder
Optional: Consider adding a comment explaining issue 1992.
While not required, a brief comment describing what issue 1992 addressed would help future maintainers understand the test's purpose. For example:
// Issue 1992: Validates that schemas with @@delegate and inheritance // don't trigger false validation errors for currentModel/currentOperation it('verifies issue 1992', async () => {This is purely a documentation improvement and not necessary for the test to function correctly.
tests/regression/test/v2-migrated/issue-1991.test.ts (1)
4-5: Add a comment explaining issue 1991.While the test name references issue 1991, adding a brief comment explaining what the issue was would improve maintainability for future developers.
Consider adding a comment like:
+// Issue 1991: [brief description of what the issue was about] it('verifies issue 1991', async () => { await createPolicyTestClient(tests/regression/test/v2-migrated/issue-1998.test.ts (1)
4-56: Well-structured regression test validating policy enforcement with delegation and composite keys.The test effectively validates issue 1998 by:
- Setting up a schema with delegation and composite key relations
- Testing that delete permissions are correctly enforced based on related entities'
updatablestatus- Verifying both allowed (count: 1) and denied (count: 0) deletion scenarios
The use of
check(parent, 'update') && check(child, 'update')in the delete rule properly ensures both related entities must be updatable for deletion to succeed.Optional enhancement: Add explanatory comment
Consider adding a brief comment explaining what issue 1998 addresses for future maintainers:
+// Issue 1998: Validates that check() expressions in policy rules properly evaluate +// permissions on related entities when using delegation and composite keys it('verifies issue 1998', async () => {tests/regression/test/v2-migrated/issue-1994.test.ts (2)
4-4: Consider a more descriptive test name.The test name "verifies issue 1994" doesn't explain what specific behavior is being tested. Consider something like "nested rolePrivileges creation in delegated model hierarchy" to make the test's purpose clearer.
53-80: Clarify the purpose ofextraSourceFiles.The
extraSourceFilessection contains logic that's nearly identical to the main test body (lines 84-101). It's unclear:
- Whether this code actually executes during test setup
- What specific aspect of issue 1994 this is meant to test
- If the duplication is intentional for reproducing a specific validation scenario
Consider adding a comment explaining why
extraSourceFilesis needed here, or remove it if the main test body is sufficient.Can you clarify what role
extraSourceFilesplays in this test? Is it testing schema generation/validation during thecreateTestClientphase, or is this duplication serving another purpose?tests/regression/test/v2-migrated/issue-1964.test.ts (2)
37-37: Follow camelCase convention for variable names.The variable
newauthorshould be renamed tonewAuthorto follow JavaScript/TypeScript camelCase naming conventions.Apply this diff:
- const newauthor = await authDb.author.create({ + const newAuthor = await authDb.author.create({And update the references on lines 50 and 53:
- where: { orgId_name: { orgId: 'org', name: newauthor.name } }, + where: { orgId_name: { orgId: 'org', name: newAuthor.name } }, data: { name: `Bar ${Date.now()}`, - posts: { deleteMany: { id: { equals: newauthor.posts[0].id } } }, + posts: { deleteMany: { id: { equals: newAuthor.posts[0].id } } }, },
89-89: Remove unnecessaryorgIdfrom auth context.The User model in test 2 doesn't have an
orgIdfield, so settingorgId: 'org'in the auth context is unnecessary. This appears to be a copy-paste artifact from test 1.Apply this diff:
- const authDb = db.$setAuth({ id: 1, orgId: 'org' }); + const authDb = db.$setAuth({ id: 1 });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/language/src/validators/function-invocation-validator.ts(2 hunks)tests/regression/test/v2-migrated/issue-1955.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1964.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1978.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1984.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1991.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1992.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1993.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1994.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1997.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1998.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/validators/function-invocation-validator.tstests/regression/test/v2-migrated/issue-1984.test.tstests/regression/test/v2-migrated/issue-1998.test.tstests/regression/test/v2-migrated/issue-1992.test.tstests/regression/test/v2-migrated/issue-1993.test.tstests/regression/test/v2-migrated/issue-1955.test.tstests/regression/test/v2-migrated/issue-1994.test.tstests/regression/test/v2-migrated/issue-1991.test.tstests/regression/test/v2-migrated/issue-1997.test.tstests/regression/test/v2-migrated/issue-1964.test.tstests/regression/test/v2-migrated/issue-1978.test.ts
🧬 Code graph analysis (10)
packages/language/src/validators/function-invocation-validator.ts (1)
packages/language/src/generated/ast.ts (4)
DataModelAttribute(395-400)DataModelAttribute(402-402)DataFieldAttribute(348-353)DataFieldAttribute(355-355)
tests/regression/test/v2-migrated/issue-1984.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1998.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1993.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1955.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1994.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1991.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1997.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1964.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1978.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (9)
tests/regression/test/v2-migrated/issue-1991.test.ts (3)
1-2: LGTM!The imports are correct and appropriate for a regression test using ZenStack's test utilities.
6-24: LGTM!The schema definition is well-structured:
- Type definitions (FooMetadata, FooOptionMetadata) correctly define the metadata structures.
- Model definitions properly use PostgreSQL-specific features (
@db.Uuid) which align with thepostgresqlprovider.- JSON fields are correctly annotated with
@json.
25-40: LGTM!The test configuration and extra source file are appropriate for a validation test:
- The PostgreSQL provider matches the schema's use of
@db.Uuid.- The
extraSourceFiles.mainsnippet is designed to validate schema compilation and type correctness, not runtime execution (hence the{} as anyand the non-awaitedcreate()call).- This aligns with the PR's objective of "validating currentModel and currentOperation properly".
tests/regression/test/v2-migrated/issue-1964.test.ts (3)
1-4: LGTM!Imports and test suite declaration are appropriate for a regression test.
79-85: Verify the@uniqueconstraint onprofileId.The Address model has
profileId Int @unique, which enforces a one-to-one relationship between Profile and Address. However, the Profile model definesaddresses Address[], suggesting a one-to-many relationship. This inconsistency could cause confusion.If the intent is to allow multiple addresses per profile, remove the
@uniqueconstraint:model Address { id Int @id @default(autoincrement()) - profileId Int @unique + profileId Int profile Profile @relation(fields: [profileId], references: [id]) city String @@allow('all', true) }If the schema is intentionally configured this way to reproduce the specific bug in issue 1964, please add a comment explaining the design choice.
91-122: Test logic is correct.The test correctly creates a user with nested profile and address, updates to delete the address, and verifies the deletion. The test structure appropriately exercises the regression scenario.
tests/regression/test/v2-migrated/issue-1978.test.ts (1)
1-39: LGTM! Well-structured test for future field-level policy feature.The test correctly documents the expected behavior for field-level policies with relation-based conditions. The skip with TODO is appropriate since the feature isn't implemented yet, and the test structure will be ready when the feature is added.
tests/regression/test/v2-migrated/issue-1984.test.ts (1)
1-56: LGTM! Comprehensive validation of currentModel() and currentOperation() context restrictions.The test suite thoroughly validates:
- regression1: Runtime behavior of
currentModel()andcurrentOperation()in policy rules with various auth contexts- regression2 & regression3: Compile-time validation that these functions are properly rejected in validation rule contexts (both model-level
@@validateand field-level@contains)The error message validation ensures proper user feedback when these functions are misused.
tests/regression/test/v2-migrated/issue-1955.test.ts (1)
1-100: LGTM! Solid coverage of createManyAndReturn/updateManyAndReturn with policies.Both tests validate the PostgreSQL-specific bulk operations work correctly with ZenStack policies:
- Simple policy: Baseline test with unrestricted access
- Complex policy: Validates selective permissions based on relation conditions
The test structure is clear and assertions properly verify the returned data matches expectations.
|
Claude encountered an error —— View job Code Review: PR #300 - Fix validating currentModel and currentOperation properlyTodo List:
SummaryThis PR fixes a critical validation bug where Code Quality Assessment✅ Excellent RefactoringThe main change extracts complex validation logic into well-structured private methods: packages/language/src/validators/function-invocation-validator.ts:101-117 private getExpressionContext(containerAttribute: DataModelAttribute | DataFieldAttribute | undefined) {
if (!containerAttribute) {
return undefined;
}
if (this.isValidationAttribute(containerAttribute)) {
return ExpressionContext.ValidationRule;
}
return match(containerAttribute?.decl.$refText)
.with('@default', () => ExpressionContext.DefaultValue)
.with(P.union('@@allow', '@@deny', '@allow', '@deny'), () => ExpressionContext.AccessPolicy)
.with('@@index', () => ExpressionContext.Index)
.otherwise(() => undefined);
}
private isValidationAttribute(attr: DataModelAttribute | DataFieldAttribute) {
return attr.decl.ref?.attributes.some((attr) => attr.decl.$refText === '@@@validation');
}✅ Proper Error HandlingThe fix correctly identifies validation contexts and produces clear error messages like: Bug Analysis🐛 Fixed Critical Bug
✅ Validation Logic CorrectnessAccording to the stdlib definition, these functions should only work in function currentModel(casing: String?): String {
} @@@expressionContext([AccessPolicy])
function currentOperation(casing: String?): String {
} @@@expressionContext([AccessPolicy])Test Coverage✅ Comprehensive Test SuiteThe PR adds 10 regression tests covering various scenarios:
✅ Test QualityTests follow good patterns:
Performance Considerations✅ Minimal Performance Impact
Security Considerations✅ Improves Security Posture
Minor Suggestions🔍 Type Safety EnhancementConsider adding stricter typing to private isValidationAttribute(attr: DataModelAttribute | DataFieldAttribute): attr is DataModelAttribute | DataFieldAttribute {
return attr.decl.ref?.attributes.some((attr) => attr.decl.$refText === '@@@validation') ?? false;
}📝 DocumentationThe extracted methods would benefit from JSDoc comments explaining their purpose and the Overall Assessment🟢 Excellent Quality
This is a high-quality fix that properly resolves the validation context issue while improving code maintainability. The extensive test coverage ensures the fix works correctly across various scenarios. |
Summary by CodeRabbit
Refactor
Tests