fix(delegate): exclude uninheritable attributes properly #488
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. WalkthroughMove non-inheritable attribute filtering (exclude @@Map, @@unique, @@index) into core attribute retrieval in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-26T01:55:04.540ZApplied to files:
📚 Learning: 2025-11-26T01:55:04.540ZApplied to files:
🧬 Code graph analysis (1)tests/regression/test/v2-migrated/issue-1243.test.ts (1)
⏰ 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)
🔇 Additional comments (1)
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 Code Review: fix(delegate): exclude uninheritable attributes properlyTodo List:
SummaryThis PR fixes an issue with delegate inheritance where certain attributes ( Key Changes1. Language Utils Enhancement (
|
![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 (4)
packages/language/src/utils.ts(1 hunks)packages/sdk/src/prisma/prisma-schema-generator.ts(1 hunks)tests/e2e/orm/schemas/delegate/schema.ts(1 hunks)tests/e2e/orm/schemas/delegate/schema.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/schemas/delegate/schema.ts
**/*.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/orm/schemas/delegate/schema.zmodel
🧠 Learnings (3)
📚 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:
tests/e2e/orm/schemas/delegate/schema.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/sdk/src/prisma/prisma-schema-generator.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:
tests/e2e/orm/schemas/delegate/schema.zmodel
🧬 Code graph analysis (2)
tests/e2e/orm/schemas/delegate/schema.ts (1)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
packages/sdk/src/prisma/prisma-schema-generator.ts (1)
packages/language/src/utils.ts (1)
getAllAttributes(546-571)
⏰ 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: Agent
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (4)
tests/e2e/orm/schemas/delegate/schema.ts (1)
148-156: LGTM!The generated attributes and uniqueFields correctly reflect the
@@indexand@@uniqueconstraints added to Asset.ownerId in the source schema. The submodels (Video, Image, RatedVideo) appropriately do not inherit these non-inheritable attributes, which aligns with the PR's objective.packages/language/src/utils.ts (2)
564-565: LGTM!The filtering of non-inheritable attributes from base models is correctly applied here, consolidating the logic in one place for better maintainability.
573-576: LGTM!The helper function correctly identifies non-inheritable attributes. The three attributes (
@@map,@@unique,@@index) are appropriately excluded from inheritance as they define model-specific metadata that shouldn't propagate to submodels. The defensive fallback to$refTexthandles cases where the reference isn't resolved.packages/sdk/src/prisma/prisma-schema-generator.ts (1)
199-199: LGTM!The removal of inherited map attribute filtering is appropriate here since
getAllAttributesin utils.ts now handles filtering of non-inheritable attributes (including@@map,@@unique, and@@index) at the source. This consolidates the filtering logic in one location, improving maintainability.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in delegate model inheritance where structural database attributes (@@map, @@unique, @@index) were incorrectly being inherited from base models. The fix centralizes the logic for filtering non-inheritable attributes in the getAllAttributes function in packages/language/src/utils.ts.
Key changes:
- Added
isNonInheritableAttributehelper function that filters out@@map,@@unique, and@@indexattributes during base model inheritance - Simplified Prisma schema generator by removing duplicate filtering logic (
isInheritedMapAttributemethod) - Updated test schema to include
@@indexand@@uniqueattributes on delegate base model to validate the fix
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/language/src/utils.ts |
Added isNonInheritableAttribute function to filter structural attributes during delegate base model inheritance |
packages/sdk/src/prisma/prisma-schema-generator.ts |
Removed duplicate isInheritedMapAttribute method and simplified attribute filtering logic |
tests/e2e/orm/schemas/delegate/schema.zmodel |
Added @@index and @@unique attributes to Asset model for testing |
tests/e2e/orm/schemas/delegate/schema.ts |
Generated schema reflecting the new attributes and their non-inheritance in derived models |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review: fix(delegate): exclude uninheritable attributes properlyTodo List:
SummaryThis PR fixes an issue with delegate inheritance where certain attributes ( Key Changes1. Language Utils Enhancement (
|
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.