Uh oh!
There was an error while loading. Please reload this page.
fix(language): detect @@@onceInModel duplicates across inheritance - #2725
Merged
Conversation
Field-level attributes marked with `@@@onceInModel` (e.g. `@deletedAt` from the soft-delete plugin) are limited to one field per model, including inherited fields. The check previously ran per-attribute against the model that physically declares each field, so duplicates that only co-occur in a derived model — when every offending field is inherited from a separate mixin/base — were never flagged. Move the check to the model level, where `getAllFields` already exposes the full inherited field set. Errors report on the offending local field attributes when present, falling back to the model declaration when all duplicates are inherited. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe Changes@@onceInModel Validation Relocation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/language/test/attribute-application.test.ts (1)
564-588: ⚡ Quick winAdd one
extends-only inheritance regression case too.Nice addition for the two-mixin path. Since the objective also calls out base-type inheritance, adding a sibling test where duplicates arrive only through an
extendschain would close the remaining coverage gap.Proposed test addition
+ it('rejects when the attribute arrives only through base-model inheritance', async () => {+ await loadSchemaWithError(+ `+ datasource db {+ provider = 'sqlite'+ url = 'file:./dev.db'+ }++ attribute `@softDelete`() @@@targetField([DateTimeField]) @@@onceInModel++ model Base {+ id Int `@id` `@default`(autoincrement())+ deletedAt DateTime? `@softDelete`+ @@delegate+ }++ model Mid extends Base {+ removedAt DateTime? `@softDelete`+ @@delegate+ }++ model Leaf extends Mid {+ extra Int?+ }+ `,+ /Attribute "`@softDelete`" can only be applied to one field per model/,+ );+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/language/test/attribute-application.test.ts` around lines 564 - 588, Add a sibling test case after the existing 'rejects when the attribute arrives only through inheritance (two mixins)' test that verifies the same error condition but using base-type inheritance through extends instead of mixins. The new test should create a similar schema structure where the attribute is duplicated across an extends chain (for example, a Base type with the attribute that is extended by another Base type which is also extended by a model) to ensure the validation correctly rejects duplicates arriving through extends-based inheritance as well, closing the coverage gap for both inheritance patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/language/test/attribute-application.test.ts`:
- Around line 564-588: Add a sibling test case after the existing 'rejects when
the attribute arrives only through inheritance (two mixins)' test that verifies
the same error condition but using base-type inheritance through extends instead
of mixins. The new test should create a similar schema structure where the
attribute is duplicated across an extends chain (for example, a Base type with
the attribute that is extended by another Base type which is also extended by a
model) to ensure the validation correctly rejects duplicates arriving through
extends-based inheritance as well, closing the coverage gap for both inheritance
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c19ad46-1b60-4e2c-b726-48277b95d222
📒 Files selected for processing (3)
packages/language/src/validators/attribute-application-validator.tspackages/language/src/validators/datamodel-validator.tspackages/language/test/attribute-application.test.ts
💤 Files with no reviewable changes (1)
- packages/language/src/validators/attribute-application-validator.ts
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Field-level attributes marked with
@@@onceInModel(e.g.@deletedAtfrom the soft-delete plugin) may be applied to at most one field per model, including inherited fields. The check ran per-attribute against the model that physically declares each field, so duplicates that only co-occur in a derived model slipped through.Specifically uncaught: when every field carrying the attribute is inherited from a separate mixin/base (none declared on the leaf model itself). The locally-declared and mixed (one local + one inherited) cases were already caught.
Fix
Move the check to the model level (
DataModelValidator), wheregetAllFields(dm)already exposes the full inherited field set. Field attributes are grouped by their@@@onceInModel-marked declaration; any group with more than one occurrence is an error. Errors report on the offending local field attributes when present (preserving the per-field diagnostic), falling back to the model declaration when all duplicates are inherited.The old per-attribute
checkOnceInModel(and its now-unused import) is removed.Tests
attribute-application.test.tsfor the inheritance-only (two-mixin) case, alongside the existing local-only and mixed cases.tsc --noEmitclean.🤖 Generated with Claude Code
Summary by CodeRabbit
Validation & Constraint Enforcement
@@onceInModelattribute validation to properly handle inherited fields and mixins within models, ensuring the "only one field per model" constraint is enforced across the entire model hierarchy.Tests
@@onceInModelattribute behavior with inherited field scenarios.