fix: stricter type def validation and compound unique field fix #223
Conversation
WalkthroughAdds a relation-field guard to type validation, updates SDK composite-unique key generation to honor named constraints, adjusts test plugin IDs, and introduces new runtime tests for mixins and multi-field unique constraints. Also updates TODOs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR: fix: stricter type def validation and compound unique field fix Todo List
Review FeedbackThis PR implements two main fixes: stricter type definition validation and compound unique field handling improvements. Overall, the changes are well-structured and properly tested. Code Quality & Best Practices ✅Strengths:
Minor suggestions:
Potential Issues
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves type definition validation and fixes compound unique field handling. The main changes enhance schema generation by supporting named compound unique constraints and add stricter validation for type definitions.
- Support for named compound unique constraints using the 'name' parameter
- Prevention of relation fields in type definitions
- Updated test plugin IDs to remove vendor prefixes
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/ts-schema-generator.ts | Adds support for named compound unique constraints through new getCompoundUniqueKey method |
| packages/language/src/validators/typedef-validator.ts | Adds validation to prevent relation fields in type definitions |
| packages/language/test/mixin.test.ts | Adds test case for the new type definition validation |
| packages/runtime/test/policy/multi-field-unique.test.ts | New comprehensive test file for multi-field unique constraints |
| packages/runtime/test/policy/mixin.test.ts | New test file for abstract model functionality |
| packages/runtime/test/policy/client-extensions.test.ts | Updates plugin IDs to remove vendor prefixes |
| TODO.md | Minor documentation updates |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/ts-schema-generator.ts (1)
775-777: Deduplicate multi-field constraints by canonical field-set key
Current logic uses the display key fromgetCompoundUniqueKeyfor deduplication, which can still emit duplicates when the same fields appear in both@@idand@@uniqueor when multiple constraints share a custom name. Change to use a sorted join of the field names as the dedup key, preserving the display key only for the emitted property name.- // it's possible to have the same set of fields in both `@@id` and `@@unique` - // so we need to deduplicate them - const seenKeys = new Set<string>(); + // it's possible to have the same set of fields in both `@@id` and `@@unique` + // so we need to deduplicate them by field-set, not by display key + const seenKeys = new Set<string>(); // stores sorted field-set key @@ - // multi-field unique - const key = this.getCompoundUniqueKey(attr, fieldNames); - if (seenKeys.has(key)) { - continue; - } - seenKeys.add(key); - properties.push( - ts.factory.createPropertyAssignment( - key, + // multi-field unique + const displayKey = this.getCompoundUniqueKey(attr, fieldNames); + const dedupKey = [...fieldNames].sort().join('_'); + if (seenKeys.has(dedupKey)) continue; + seenKeys.add(dedupKey); + properties.push( + ts.factory.createPropertyAssignment( + displayKey,Add a test covering a model with both
@@id([a,b])and@@unique([a,b], name: "named")and assert only one schema entry is generated for that field-set, while lookup uses the named key.
🧹 Nitpick comments (6)
TODO.md (1)
10-10: Fix list indentation to satisfy markdownlint (MD007).The new bullet uses 4 spaces; the linter expects 2. Either:
- Re-indent nested list items to 2 spaces across the file, or
- Update markdownlint config to allow 4-space indents.
Minimal code change (convert the CLI subsection to 2-space indents):
- - [x] generate - - [x] migrate - - [x] info - - [x] init - - [x] validate - - [ ] format - - [ ] repl - - [x] plugin mechanism - - [x] built-in plugins + - [x] generate + - [x] migrate + - [x] info + - [x] init + - [x] validate + - [ ] format + - [ ] repl + - [x] plugin mechanism + - [x] built-in pluginspackages/sdk/src/ts-schema-generator.ts (1)
829-836: Helper is fine; tighten validation (optional).
getCompoundUniqueKeyworks. If desired, guard against non-string literals to fail fast:- if (nameArg && isLiteralExpr(nameArg.value)) { - return nameArg.value.value as string; + if (nameArg) { + invariant(isLiteralExpr(nameArg.value) && typeof nameArg.value.value === 'string', 'name must be a string literal'); + return nameArg.value.value as string;packages/runtime/test/policy/multi-field-unique.test.ts (2)
7-15: Remove unused cwd save/restore (optional).
beforeAll/afterEachmanageprocess.chdirbut no test changes cwd. Consider removing to reduce noise.- let origDir: string; - - beforeAll(async () => { - origDir = path.resolve('.'); - }); - - afterEach(() => { - process.chdir(origDir); - });
96-104: Unique violation assertion is adequate.Asserting
QueryErroron duplicate composite key is correct. If you later surface specific codes, consider matching them for stronger guarantees.packages/language/test/mixin.test.ts (1)
110-123: Add coverage for array/optional relation forms.
Types like User[] and User? should also be rejected; add tests to prevent regressions.Apply:
@@ it('does not allow relation fields in type', async () => { @@ }); + + it('does not allow relation array fields in type', async () => { + await loadSchemaWithError( + ` + model User { + id Int @id @default(autoincrement()) + } + + type T { + us User[] + } + `, + 'Type field cannot be a relation', + ); + }); + + it('does not allow optional relation fields in type', async () => { + await loadSchemaWithError( + ` + model User { + id Int @id @default(autoincrement()) + } + + type T { + u User? + } + `, + 'Type field cannot be a relation', + ); + });packages/language/src/validators/typedef-validator.ts (1)
25-29: Include field name in the error message for clarity.
Improves UX when multiple fields fail in one type.Apply:
- if (isDataModel(field.type.reference?.ref)) { - accept('error', 'Type field cannot be a relation', { - node: field.type, - }); - } + if (isDataModel(field.type.reference?.ref)) { + accept('error', `Type field "${field.name}" cannot be a relation`, { + node: field.type, + }); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
TODO.md(1 hunks)packages/language/src/validators/typedef-validator.ts(2 hunks)packages/language/test/mixin.test.ts(1 hunks)packages/runtime/test/policy/client-extensions.test.ts(5 hunks)packages/runtime/test/policy/mixin.test.ts(1 hunks)packages/runtime/test/policy/multi-field-unique.test.ts(1 hunks)packages/sdk/src/ts-schema-generator.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/language/test/mixin.test.tspackages/runtime/test/policy/mixin.test.tspackages/sdk/src/ts-schema-generator.tspackages/runtime/test/policy/client-extensions.test.tspackages/language/src/validators/typedef-validator.tspackages/runtime/test/policy/multi-field-unique.test.ts
🧬 Code graph analysis (5)
packages/language/test/mixin.test.ts (1)
packages/language/test/utils.ts (1)
loadSchemaWithError(18-30)
packages/runtime/test/policy/mixin.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/sdk/src/ts-schema-generator.ts (1)
packages/language/src/generated/ast.ts (3)
DataModelAttribute(395-400)DataModelAttribute(402-402)isLiteralExpr(133-135)
packages/language/src/validators/typedef-validator.ts (1)
packages/language/src/generated/ast.ts (1)
isDataModel(391-393)
packages/runtime/test/policy/multi-field-unique.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
10-10: 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 (7)
packages/runtime/test/policy/client-extensions.test.ts (2)
25-31: Plugin id rename looks good.Consistent change to 'queryOverride' across tests; no functional issues spotted.
Also applies to: 56-62, 87-93, 118-124
147-155: Plugin id rename looks good.Renaming to 'resultMutation' is consistent with usage.
packages/runtime/test/policy/mixin.test.ts (1)
1-92: Solid coverage for policy-driven connect with mixins.Scenarios and expectations are clear; no issues spotted.
packages/runtime/test/policy/multi-field-unique.test.ts (1)
45-71: Named composite unique test aligns with generator changes.Good use of the named key (
myconstraint) throughout CRUD paths.packages/language/test/mixin.test.ts (1)
110-123: Good negative test; message assertion fits utils’ matcher.
Covers the new validator behavior and aligns with loadSchemaWithError’s substring check.packages/language/src/validators/typedef-validator.ts (2)
2-2: Import change is correct and type-only modifiers are preserved.
Tree-shake friendly; no runtime cost for types.
25-29: Correct relational-field guard for TypeDef.
Prevents model references in types and attaches error to the type node.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores