Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds view-related validation: disallows Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 PR Review: View Validation EnhancementsI've completed my review of the view validation changes. Here's my comprehensive feedback: Todo List:
Code Quality & Implementation ✅Strong Points:
Implementation Details:attribute-application-validator.ts:297-312 @check('@id')
private _checkId(attr: AttributeApplication, accept: ValidationAcceptor) {
const dm = getContainingDataModel(attr);
if (dm?.isView) {
accept('error', `\`@id\` is not allowed for views`, { node: attr });
}
}
@check('@@id')
@check('@@index')
@check('@@unique')
private _checkConstraint(attr: AttributeApplication, accept: ValidationAcceptor) {
const dm = getContainingDataModel(attr);
if (dm?.isView && (attr.decl.$refText === '@@id' || attr.decl.$refText === '@@index')) {
accept('error', `\`${attr.decl.$refText}\` is not allowed for views`, { node: attr });
}
// ... rest of existing validation
}✅ Good: Uses consistent error messaging with backtick formatting datamodel-validator.ts:53 if (
!dm.isView &&
idFields.length === 0 &&
// ... rest of unique criteria checks
) {✅ Good: Simple, clear logic - views are exempt from unique criteria requirements Test Coverage ✅The test file
Potential test improvements: // Consider adding these test cases:
it('allows @@unique on views', async () => {
await expect(loadSchema(`
view A {
x Int
@@unique([x])
}
`)).resolves.toBeTruthy();
});
it('handles views with mixins and inheritance correctly', async () => {
// Test view-specific validation in inheritance scenarios
});Potential Issues & SuggestionsMinor Issues:
Security & Performance ✅
Recommendations
Overall Assessment: APPROVEDThis is a well-implemented PR that follows ZenStack's coding standards and architecture. The changes are focused, tested, and maintain backward compatibility. The minor test syntax issues should be fixed before merge. Files changed: 5 files, +86 lines, -1 line |
There was a problem hiding this comment.
Pull request overview
This PR adds validation rules to restrict certain attributes on database views in the ZModel schema language. Views are now exempt from requiring unique criteria, and specific attributes (@id, @@id, and @@index) are explicitly disallowed on views.
- Adds validation to prevent
@id,@@id, and@@indexattributes on views - Exempts views from the requirement to have unique identifier criteria
- Generalizes the
getContainingDataModelutility function to accept anyAstNodeinstead of justExpression
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/test/view.test.ts | Adds test cases for view validation, including tests to verify that @id, @@id, and @@index are properly rejected on views |
| packages/language/src/validators/datamodel-validator.ts | Adds !dm.isView check to exempt views from requiring unique identifier criteria |
| packages/language/src/validators/attribute-application-validator.ts | Adds validation logic to reject @id, @@id, and @@index attributes on views, and imports getContainingDataModel utility |
| packages/language/src/utils.ts | Generalizes getContainingDataModel function signature from Expression to AstNode parameter type |
| .gitignore | Adds .DS_Store to ignore macOS system files |
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignorepackages/language/src/utils.tspackages/language/src/validators/attribute-application-validator.tspackages/language/src/validators/datamodel-validator.tspackages/language/test/view.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/language/test/view.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/language/test/view.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/language/test/view.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/language/test/view.test.ts
🧬 Code graph analysis (2)
packages/language/src/utils.ts (2)
packages/language/src/ast.ts (3)
AstNode(4-4)AstNode(66-71)DataModel(49-54)packages/language/src/generated/ast.ts (2)
DataModel(377-387)DataModel(389-389)
packages/language/src/validators/attribute-application-validator.ts (2)
packages/schema/src/schema.ts (1)
AttributeApplication(40-43)packages/language/src/utils.ts (1)
getContainingDataModel(504-513)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (6)
.gitignore (1)
11-11: LGTM!Standard addition to ignore macOS system files.
packages/language/src/utils.ts (1)
504-513: LGTM!Widening the parameter type from
ExpressiontoAstNodeis appropriate. The traversal logic using$containerworks correctly for any AST node type, and this enables the function to be used with attribute nodes in the new view validators.packages/language/src/validators/datamodel-validator.ts (1)
52-58: LGTM!Correctly exempts views from requiring unique criteria. Views are read-only database constructs that don't need identity constraints.
Consider whether other @id-related validations in
validateFields(lines 66-89) could be reached for views. Since@idand@@idare now disallowed on views viaattribute-application-validator.ts, those code paths should not be exercised for views, but you may want to add early returns or guards for defensive clarity.packages/language/src/validators/attribute-application-validator.ts (3)
30-30: LGTM!Import added for the utility function used in view validation.
297-303: LGTM!Clean implementation of the field-level
@idvalidation for views.
305-312: LGTM!The validation correctly allows
@@uniqueon views while disallowing@@idand@@index. This seems intentional since unique constraints can be meaningful for views in a read context.Please confirm that
@@uniqueshould remain allowed on views. The current implementation permits it, which differs from@@idand@@index. If this is intentional, consider adding a test case to document that@@uniqueis explicitly allowed on views.
|
Claude finished @ymc9's task —— View job Claude finished @ymc9's task —— View job PR Review: View Validation EnhancementsI've completed my review of the view validation changes. Here's my comprehensive feedback: Todo List:
Code Quality & Implementation ✅Strong Points:
Implementation Details:attribute-application-validator.ts:297-312 @check('@id')
private _checkId(attr: AttributeApplication, accept: ValidationAcceptor) {
const dm = getContainingDataModel(attr);
if (dm?.isView) {
accept('error', `\`@id\` is not allowed for views`, { node: attr });
}
}
@check('@@id')
@check('@@index')
@check('@@unique')
private _checkConstraint(attr: AttributeApplication, accept: ValidationAcceptor) {
const dm = getContainingDataModel(attr);
if (dm?.isView && (attr.decl.$refText === '@@id' || attr.decl.$refText === '@@index')) {
accept('error', `\`${attr.decl.$refText}\` is not allowed for views`, { node: attr });
}
// ... rest of existing validation
}✅ Good: Uses consistent error messaging with backtick formatting datamodel-validator.ts:53 if (
!dm.isView &&
idFields.length === 0 &&
// ... rest of unique criteria checks
) {✅ Good: Simple, clear logic - views are exempt from unique criteria requirements Test Coverage ✅The test file
Note: The test implementation is correct - Potential Issues & SuggestionsIssues Identified:
Recommendations:
Performance & Security ✅
Architecture Alignment ✅The changes align well with ZenStack's architecture:
Overall Assessment: APPROVED with minor fixes neededThis is a well-implemented PR that follows ZenStack's coding standards. The validation logic is sound, test coverage is good, and the changes maintain backward compatibility. Critical Issue: Fix the syntax errors in test lines 46 and 62 before merge. Files changed: 7 files, +103 lines, -3 lines |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/language/test/view.test.ts (1)
21-68: Missing assertion chains prevent tests from validating errors.The syntax errors mentioned in previous reviews have been fixed (lines 46 and 62 now have proper closing parentheses). However, the
expect()calls still lack assertion chains:
- Lines 22-35:
await expect(loadSchemaWithError(...))with no.resolvesor.rejects- Lines 37-51: Same issue
- Lines 53-67: Same issue
Without chaining a matcher method like
.resolves.toBeTruthy()or.rejects.toThrow(), these tests create expectations but don't actually assert anything.🔎 Proposed fix
it('does not allow id or index on views', async () => { await expect( loadSchemaWithError( ` datasource db { provider = 'sqlite' } view A { id Int @id } `, '`@id` is not allowed for views', ), - ); + ).resolves.toBeTruthy(); await expect( loadSchemaWithError( ` datasource db { provider = 'sqlite' } view A { id Int @@id([id]) } `, '`@@id` is not allowed for views', ), - ); + ).resolves.toBeTruthy(); await expect( loadSchemaWithError( ` datasource db { provider = 'sqlite' } view A { id Int @@index([id]) } `, '`@@index` is not allowed for views', ), - ); + ).resolves.toBeTruthy(); });
🧹 Nitpick comments (1)
tests/e2e/github-repos/cal.com/schema.ts (1)
6559-6666: AlignBookingTimeStatusDenormalized.iduniqueness metadata with other unique identifiers
BookingTimeStatusDenormalized.idis now using@unique(good for a view under the new validation rules), but the field itself lacksunique: true, while other single-field uniques in this schema (including views likeBookingTimeStatus) typically set bothid: trueandunique: trueand then mirror that inuniqueFields.If any downstream logic or type-gen relies on the
field.uniqueflag (instead of only scanning attributes/uniqueFields), this model could be treated slightly differently from otherwise-identical views. It’s also a small consistency gap in this generated schema.Since this file is auto-generated, any adjustment should come from the generator or source schema; conceptually, the field would look like:
Suggested shape for the generated `id` field
- id: { - name: "id", - type: "Int", - id: true, - attributes: [{ name: "@unique" }] - }, + id: { + name: "id", + type: "Int", + id: true, + unique: true, + attributes: [{ name: "@unique" }] + },Also, because this lives under
tests/e2e/github-repos/cal.com/, it’s worth double-checking that the regenerated schema still matches the current cal.com view definition so the e2e compatibility test remains faithful. As per coding guidelines, this e2e fixture should reflect the real upstream schema behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/test/view.test.tstests/e2e/github-repos/cal.com/schema.tstests/e2e/github-repos/cal.com/schema.zmodel
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
tests/e2e/github-repos/cal.com/schema.zmodel
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/github-repos/cal.com/schema.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/language/test/view.test.tstests/e2e/github-repos/cal.com/schema.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/language/test/view.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
packages/language/test/view.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/language/test/view.test.ts
⏰ 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: claude-review
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (3)
tests/e2e/github-repos/cal.com/schema.zmodel (1)
1650-1650: LGTM! View correctly updated to remove@id.The change from
@id @uniqueto just@uniquealigns with the new validation rules that disallow@idattributes on views. This E2E test ensures the cal.com schema remains compatible with the updated validation.packages/language/test/view.test.ts (2)
5-19: LGTM! Basic view test is correctly structured.The test properly validates that a simple view schema compiles successfully, with the correct assertion chain
.resolves.toBeTruthy().
70-83: LGTM! @@unique test is correctly structured.The test properly validates that
@@uniqueis allowed on views, complementing the negative tests for disallowed constructs.
1 participant
Summary by CodeRabbit
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.