fix: check and complaint if @@index references fields fro m delegate base

[ymc9]

fixes #283

Summary by CodeRabbit

Bug Fixes

• Fixed validation for fields inherited from polymorphic base models when using unique and index constraints.
• Improved error messages to provide more specific attribute details during validation failures.

[Copilot]

Pull Request Overview

This PR fixes issue #283 by adding validation to prevent @@index attributes from referencing fields inherited from delegate base models, extending the existing validation that was already in place for @@unique and @@id attributes.

• Extended the field inheritance validation to also check @@index attributes
• Made error messages dynamic to reflect the actual attribute name being validated
• Added regression test to verify the fix

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
tests/regression/test/issue-283.test.ts	Adds test case verifying that @@index correctly rejects fields inherited from polymorphic base models
packages/language/src/validators/attribute-application-validator.ts	Extends validation to include @@index and makes error messages dynamic to work across all three attribute types

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[coderabbitai]

Walkthrough

The pull request introduces validation to prevent inherited fields from polymorphic base models from being used in @@index constraints. A new validation check is added to the attribute validator, with dynamic error messaging. A regression test verifies this constraint behavior.

Changes

Cohort / File(s)	Summary
Validator Enhancement packages/language/src/validators/attribute-application-validator.ts	Adds @check('@@index') validation decorator and introduces dynamic error messaging using attribute names (attrName) instead of hard-coded values. Expands the unique constraint check to include index validation for polymorphic base model field inheritance scenarios.
Regression Test tests/regression/test/issue-283.test.ts	New test file for issue #283 that validates schema loading error when a polymorphic Sub model attempts to reference inherited fields from a Base model in an @@index constraint.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes are focused and straightforward: a validation rule addition with consistent pattern (dynamic error messages) and a targeted regression test. The logic is well-scoped to a specific constraint scenario with minimal branching or complexity.

Possibly related PRs

• chore: migrate some v2 regression cases #287: Introduces the loadSchemaWithError test utility that is imported and used by the new regression test file.

Poem

🐰 Index dreams for inherited fields,
Now caught by rules the validator yields,
No polymorphic shortcuts allowed,
Validation strong, precise and proud!
A test is born, issue #283 bows. 🌟

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "fix: check and complaint if @@index references fields fro m delegate base" is directly related to the main changeset. Despite containing a typo ("fro m" should be "from"), the title clearly conveys that the fix involves adding validation checks and error messages for @@index constraints when they reference inherited fields from a base model. The title is specific and directly reflects the code changes that introduce validation logic for this scenario.
Linked Issues Check	✅ Passed	The PR addresses the objectives from issue #283 by adding validation that detects when @@index constraints reference fields inherited from a polymorphic base model and provides a clear error message: "Cannot use fields inherited from a polymorphic base model." The issue description indicates that either allowing the reference or providing a clear check/complaint would resolve the problem. The PR chooses the latter approach by detecting this scenario and providing explicit validation feedback instead of the confusing resolution error originally reported, which directly fulfills the stated objective of providing a clear check/complaint.
Out of Scope Changes Check	✅ Passed	All code changes in the PR are directly scoped to addressing issue #283. The modifications to the attribute-application-validator include adding a new @check('@@index') decorator and dynamic error messaging for inherited fields, while the new test file specifically validates the behavior described in the issue. No unrelated modifications, formatting changes, or auxiliary updates are present outside the scope of fixing the @@index validation for inherited base model fields.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/issue-283

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

Reviewing this PR that fixes issue #283 by adding validation for @@index attributes referencing delegate base fields.

Review Tasks:

• Read and analyze the validator changes
• Review the regression test implementation
• Check code quality and best practices
• Assess test coverage
• Look for potential bugs or security issues
• Provide comprehensive feedback

---

✅ Overall Assessment: LGTM

This is a clean, focused fix that properly extends existing validation logic to cover @@index attributes. The implementation is consistent with the existing codebase patterns and includes appropriate test coverage.

📋 Code Quality & Best Practices

✅ Excellent:

• Consistent with existing patterns: The fix reuses the existing validation function _checkUnique which already handled @@id and @@unique - simply adding @@index to the @check decorator list
• DRY principle: No code duplication - leverages existing validation logic
• Dynamic error messages: Made error messages dynamic using attrName variable instead of hardcoded strings, improving maintainability
• Minimal, surgical change: Only 2 lines added to fix the issue, keeping the change scope small and focused

Code Quality Score: 9/10

🧪 Test Coverage

✅ Good coverage:

• Regression test added: issue-283.test.ts properly validates the fix
• Follows existing test patterns: Uses the same loadSchemaWithError pattern as other similar tests (e.g., issue-1758.test.ts for @@unique)
• Clear test case: Tests the exact scenario that was broken - @@index([x, y]) where x is inherited from delegate base

Coverage Score: 8/10

🔒 Security

✅ No security concerns: This is a validation enhancement that prevents potentially problematic schema definitions. No security implications.

⚡ Performance

✅ No performance impact: The change reuses existing validation logic with no additional computational overhead.

🐛 Potential Issues & Suggestions

✅ No issues found. The implementation correctly:

• Uses the same validation logic for all three attributes (@@id, @@unique, @@index)
• Provides consistent error messages across all attribute types
• Follows the established error message format
• Maintains backward compatibility

📝 Specific Code Review

File: packages/language/src/validators/attribute-application-validator.ts

• Line 294: ✅ Correctly added @@index to the existing validation decorator
• Line 326: ✅ Excellent improvement making error messages dynamic with ${attrName}

File: tests/regression/test/issue-283.test.ts

• Lines 8-18: ✅ Well-structured test schema matching the issue description
• Line 20: ✅ Correct expected error message

🔍 Edge Cases Considered

Looking at the validation logic in _checkUnique method (lines 325-329), it properly handles:

• ✅ Multiple fields in the attribute array
• ✅ Validates each field reference individually
• ✅ Checks if fields are inherited from delegate models using isDelegateModel()
• ✅ Provides clear error messages with attribute name

🎯 Recommendation

APPROVE - This PR successfully fixes issue #283 with a minimal, well-tested change that follows existing code patterns perfectly.

---

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

tests/regression/test/issue-283.test.ts (2)

4-5: Test structure is clear.

The test suite properly references issue #283. The test name is functional, though a more descriptive name like 'prevents @@index from using inherited fields in polymorphic models' would better document the constraint being tested.

---

6-22: Consider expanding test coverage.

The test correctly validates the new error message for the issue, which is a significant improvement over the cryptic "Could not resolve reference" error mentioned in issue #283. However, consider adding these test cases to the suite for more comprehensive coverage:

1. Only inherited fields: Test @@index([x]) to ensure it also triggers the error
2. Only own fields: Test @@index([y]) to verify this valid case still works
3. Non-polymorphic inheritance: Test a base without @@delegate to clarify the constraint is specific to polymorphic models

Additionally, the error message could be more specific to aid debugging:

`Field 'x' is inherited from polymorphic base model 'Base' and cannot be used in @@index`

Would you like me to generate additional test cases to expand coverage?

packages/language/src/validators/attribute-application-validator.ts (1)

297-336: Consider renaming method to reflect broader scope.

The method _checkUnique now validates three distinct attributes (@@id, @@index, @@unique), making its name misleading. Consider renaming to better reflect its broader purpose.

Apply this diff to rename the method:

-    private _checkUnique(attr: AttributeApplication, accept: ValidationAcceptor) {
+    private _checkFieldBasedAttribute(attr: AttributeApplication, accept: ValidationAcceptor) {

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14eaf36 and db8b3b1.

📒 Files selected for processing (2)

• packages/language/src/validators/attribute-application-validator.ts (3 hunks)
• tests/regression/test/issue-283.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/, or tests/

Files:

• tests/regression/test/issue-283.test.ts
• packages/language/src/validators/attribute-application-validator.ts

🧬 Code graph analysis (1)

packages/language/src/validators/attribute-application-validator.ts (1)

packages/sdk/src/schema/schema.ts (1)

• AttributeApplication (39-42)

⏰ 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, sqlite)
• GitHub Check: claude-review
• GitHub Check: build-test (20.x, postgresql)

🔇 Additional comments (4)

tests/regression/test/issue-283.test.ts (1)

1-2: LGTM!

The imports are appropriate for a regression test that validates error behavior.

packages/language/src/validators/attribute-application-validator.ts (3)

299-299: Good addition for dynamic error messaging.

Introducing attrName to capture the attribute name dynamically improves error message specificity across @@id, @@index, and @@unique attributes.

---

308-308: LGTM! Dynamic error messages improve clarity.

The use of ${attrName} in error messages at lines 308 and 326 provides more specific feedback to users, clearly indicating which attribute constraint (@@id, @@index, or @@unique) is being validated.

Also applies to: 326-326

---

325-329: No issues found — validation is correct and well-tested.

The code at lines 325-329 correctly implements an intentional restriction. Issue #283 specifically tests this scenario: a Base model with @@delegate(type) where Sub extends Base and attempts to use inherited field x in @@index([x, y]). The expected error "Cannot use fields inherited from a polymorphic base model" confirms this is the intended behavior.

Additionally, the language design allows models to extend only delegate models (not regular models), so inherited fields are inherently from polymorphic base models. Regression tests are already in place for both @@index and @@unique attributes.

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

Reviewing this PR that fixes issue #283 by adding validation for @@index attributes referencing delegate base fields.

Review Tasks:

• Read and analyze the validator changes
• Review the regression test implementation
• Check code quality and best practices
• Assess test coverage
• Look for potential bugs or security issues
• Provide comprehensive feedback

---

✅ Overall Assessment: LGTM

This is a clean, focused fix that properly extends existing validation logic to cover @@index attributes. The implementation is consistent with the existing codebase patterns and includes appropriate test coverage.

📋 Code Quality & Best Practices

✅ Excellent:

• Consistent with existing patterns: The fix reuses the existing validation function _checkUnique which already handled @@id and @@unique - simply adding @@index to the @check decorator list
• DRY principle: No code duplication - leverages existing validation logic
• Dynamic error messages: Made error messages dynamic using attrName variable instead of hardcoded strings, improving maintainability
• Minimal, surgical change: Only 2 lines added to fix the issue, keeping the change scope small and focused

Code Quality Score: 9/10

🧪 Test Coverage

✅ Good coverage:

• Regression test added: issue-283.test.ts properly validates the fix
• Follows existing test patterns: Uses the same loadSchemaWithError pattern as other similar tests (e.g., issue-1758.test.ts for @@unique)
• Clear test case: Tests the exact scenario that was broken - @@index([x, y]) where x is inherited from delegate base

Coverage Score: 8/10

🔒 Security

✅ No security concerns: This is a validation enhancement that prevents potentially problematic schema definitions. No security implications.

⚡ Performance

✅ No performance impact: The change reuses existing validation logic with no additional computational overhead.

🐛 Potential Issues & Suggestions

✅ No issues found. The implementation correctly:

• Uses the same validation logic for all three attributes (@@id, @@unique, @@index)
• Provides consistent error messages across all attribute types
• Follows the established error message format
• Maintains backward compatibility

📝 Specific Code Review

File: packages/language/src/validators/attribute-application-validator.ts

• Line 294: ✅ Correctly added @@index to the existing validation decorator
• Line 326: ✅ Excellent improvement making error messages dynamic with ${attrName}

File: tests/regression/test/issue-283.test.ts

• Lines 8-18: ✅ Well-structured test schema matching the issue description
• Line 20: ✅ Correct expected error message

🔍 Edge Cases Considered

Looking at the validation logic in _checkUnique method (lines 325-329), it properly handles:

• ✅ Multiple fields in the attribute array
• ✅ Validates each field reference individually
• ✅ Checks if fields are inherited from delegate models using isDelegateModel()
• ✅ Provides clear error messages with attribute name

🎯 Recommendation

APPROVE - This PR successfully fixes issue #283 with a minimal, well-tested change that follows existing code patterns perfectly.

---