fix: add a few missing zmodel validation checks #101
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce stricter validation logic for schema attributes and relations, enhance error reporting, and expand end-to-end testing for schema validation consistency. New attributes and validation rules are added at the language level, and a comprehensive test suite is implemented to verify schema correctness. Dependency configurations and documentation are also updated. Changes
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
ymc9
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for ZModel validation consistency with Prisma by introducing a new test file and fixing several validation bugs in the language package. The main purpose is to ensure ZenStack's validation behavior matches Prisma's expectations across various schema scenarios.
- Adds extensive E2E validation tests covering models, relations, constraints, and edge cases
- Fixes validation bugs for self-relations, many-to-many relations, and duplicate attributes
- Adds development documentation and updates TODO tracking
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/prisma-consistency/zmodel-validation.test.ts | New comprehensive test suite with 1000+ lines testing ZModel validation scenarios |
| tests/e2e/package.json | Adds @zenstackhq/cli as dev dependency for testing |
| packages/language/src/validators/datamodel-validator.ts | Fixes self-relation validation and many-to-many relation handling |
| packages/language/src/validators/attribute-application-validator.ts | Adds duplicate attribute validation and fixes unique constraint validation |
| packages/language/res/stdlib.zmodel | Adds @@@once attribute for single-use validation |
| TODO.md | Updates CLI command completion tracking |
| CLAUDE.md | New development documentation for Claude AI assistance |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
![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
🧹 Nitpick comments (3)
TODO.md (1)
10-12: Fix indentation to maintain consistency.The new TODO items have incorrect indentation (4 spaces instead of 2).
Apply this diff to fix the indentation:
- - [ ] validate - - [ ] format - - [ ] db seed + - [ ] validate + - [ ] format + - [ ] db seedtests/e2e/prisma-consistency/zmodel-validation.test.ts (2)
24-24: Consider making the CLI path more robust.The current path construction assumes a specific node_modules structure. Consider using a more robust approach like attempting to resolve the package first.
- this.cliPath = join(currentDir, '../node_modules/@zenstackhq/cli/bin/cli'); + // Try to resolve the CLI package more robustly + try { + const cliPackagePath = require.resolve('@zenstackhq/cli/package.json', { paths: [currentDir] }); + this.cliPath = join(dirname(cliPackagePath), 'bin/cli'); + } catch { + // Fallback to the current approach + this.cliPath = join(currentDir, '../node_modules/@zenstackhq/cli/bin/cli'); + }
136-138: Consider asserting specific error messages for better test precision.The current approach only checks that errors exist but doesn't verify the specific validation error. This could lead to false positives if a different error occurs.
expect(result.success).toBe(false); expect(result.errors.length).toBeGreaterThan(0); + expect(result.errors.some(e => e.includes('unique criterion') || e.includes('id'))).toBe(true);This pattern could be applied throughout the test file to make assertions more specific and ensure the correct validation is being triggered.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
TODO.md(1 hunks)packages/language/res/stdlib.zmodel(3 hunks)packages/language/src/validators/attribute-application-validator.ts(3 hunks)packages/language/src/validators/datamodel-validator.ts(3 hunks)tests/e2e/package.json(1 hunks)tests/e2e/prisma-consistency/zmodel-validation.test.ts(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
TODO.md
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (12)
packages/language/res/stdlib.zmodel (1)
227-230: LGTM! Good addition for preventing duplicate attribute applications.The introduction of the
@@@once()meta-attribute and its application to@idand@uniqueattributes is a valuable enhancement. This will help prevent accidental duplicate attribute applications at the language level, improving the robustness of schema definitions.Also applies to: 240-240, 255-255
tests/e2e/package.json (1)
11-13: LGTM! Appropriate dev dependency addition.Adding
@zenstackhq/clias a dev dependency is necessary for the new e2e tests that validate schema using CLI commands.packages/language/src/validators/attribute-application-validator.ts (2)
136-146: Well-implemented duplicate attribute detection.The
checkDuplicatedAttributesmethod correctly enforces the@@@onceconstraint by checking if the attribute declaration has the meta-attribute and then looking for duplicates in the container.
214-214: Good enhancement to field validation for @@id and @@unique.The addition of
@check('@@id')and the improved error handling for missing or invalid arguments strengthen the validation logic. The specific error messages for different failure cases (missing argument, non-array, empty array) provide clear feedback to users.Also applies to: 219-228
packages/language/src/validators/datamodel-validator.ts (5)
262-269: Good addition for self-relation validation.Requiring an explicit name for self-relation fields helps prevent ambiguity when a model has multiple self-referencing relationships.
275-275: Important fix to prevent self-matching.Excluding the current field from opposite field matching is crucial to avoid incorrect self-references in the validation logic.
334-347: Well-structured validation for implicit many-to-many relations.The check correctly enforces that implicit M2M relations (where both sides are arrays) cannot have explicit fields/references attributes, which aligns with Prisma's behavior.
349-379: Improved validation flow for relation ownership.The reorganized logic clearly enforces that exactly one side of a non-M2M relation must specify fields/references, with appropriate error messages for both sides when this constraint is violated.
388-431: Comprehensive one-to-one relation validation.The validation correctly ensures that foreign key fields in one-to-one relations are marked as unique, either through field-level attributes or model-level constraints. This prevents data integrity issues.
tests/e2e/prisma-consistency/zmodel-validation.test.ts (3)
1-12: LGTM!The imports and interface definition are appropriate for the test implementation.
92-104: LGTM!Good test setup with proper isolation using unique temp directories and cleanup.
105-1011: Excellent comprehensive test coverage!The test suite provides thorough coverage of ZenStack schema validation scenarios including:
- Model and field validations
- Relation types (one-to-one, one-to-many, many-to-many, self-relations)
- Constraints (unique, compound IDs)
- Database provider-specific validations
- Attribute validations
The tests are well-organized, follow consistent patterns, and cover both positive and negative cases effectively.
|
Claude finished @ymc9's task —— View job Code Review: ZModel Validation EnhancementsSummary: This PR significantly improves ZModel validation by adding stricter attribute validation, enhancing relation field checks, and providing comprehensive E2E test coverage. The changes align ZenStack validation behavior more closely with Prisma's expectations. ✅ Strengths1. Well-Designed Meta-Attribute System
2. Comprehensive Relation Validation
3. Excellent Test Coverage
|
|
Claude finished @ymc9's task —— View job Code Review: ZModel Validation EnhancementsSummary: This PR significantly improves ZModel validation by adding stricter attribute validation, enhancing relation field checks, and providing comprehensive E2E test coverage. The changes align ZenStack validation behavior more closely with Prisma's expectations. Progress:
✅ Strengths1. Well-Designed Meta-Attribute System
2. Comprehensive Relation Validation
3. Excellent Test Coverage
|
Summary by CodeRabbit
New Features
Chores