fix: ts schema generation for ignored models and fields #440
Conversation
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughThis pull request centralizes declaration filtering in the TypeScript schema generator by adding helper methods for retrieving data models and typedefs, refactors generator functions to use them, adds E2E tests that assert ignored fields/models are excluded from the ORM client API, and updates a test schema to include ignored elements and an access policy. 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)
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 PR Review: Fix TS Schema Generation for Ignored Models and Fields ✅This PR addresses a critical issue where ignored models and fields were still being included in TypeScript schema generation, breaking the intent of the Todo List:
Summary of ChangesProblem IdentifiedThe issue was that TypeScript schema generation wasn't properly filtering out:
Root CauseThe code was inconsistent in how it handled ignored entities:
The FixMain changes in
Code Quality Assessment✅ Strengths
✅ Test QualityThe test file
✅ No Security ConcernsThe changes are purely about TypeScript generation and don't affect runtime security. In fact, properly hiding ignored fields helps prevent accidental data exposure. ✅ No Performance ImpactThe refactoring doesn't change the algorithmic complexity - it just makes the existing filtering more consistent. Minor Observations
Recommendation✅ APPROVE - This is a clean, well-tested bug fix that:
The fix correctly ensures that:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes TypeScript schema generation to properly exclude models and fields marked with @@ignore and @ignore attributes. Previously, these ignored entities were being included in the generated TypeScript types, which could expose internal implementation details or database-only fields to the client API.
Key Changes
- Introduced
getAllDataModels()andgetAllTypeDefs()helper methods that filter out entities with@@ignoreattribute - Replaced inline filtering of
model.declarationswith calls to these helper methods throughout the schema generator - Added test coverage for ignored fields (
@ignore) and models (@@ignore)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/e2e/orm/schemas/basic/schema.zmodel | Added test fixtures: password field with @ignore on User model, and Foo model with @@ignore |
| tests/e2e/orm/client-api/ignore.test.ts | Added test cases verifying that ignored fields and models are excluded from TypeScript types (using @ts-expect-error annotations) |
| packages/sdk/src/ts-schema-generator.ts | Refactored schema generation to use getAllDataModels() and getAllTypeDefs() helper methods that filter out ignored entities, ensuring consistency across all generation paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/sdk/src/ts-schema-generator.ts (1)
112-119: Consider aligninghasComputedFieldswith the new ignore-aware helpers
hasComputedFieldscurrently checks allDataModeldeclarations, including those with@@ignore, while actual schema content and type aliases now usegetAllDataModels(model).If a computed field exists only on an ignored model, this will still trigger importing
OperandExpressioneven though no computed fields are emitted.Not a correctness issue, but you might want to change:
const hasComputedFields = model.declarations.some( (d) => isDataModel(d) && d.fields.some((f) => hasAttribute(f, '@computed')), );to use
this.getAllDataModels(model)for consistency and to avoid potential unused imports:const hasComputedFields = this.getAllDataModels(model).some((d) => d.fields.some((f) => hasAttribute(f, '@computed')), );Also applies to: 306-312
tests/e2e/orm/client-api/ignore.test.ts (1)
26-26:noophelper is fine; just ensure every@ts-expect-error-guarded use is meaningfulThe
noophelper is a standard trick to keep values “used” for type-level tests. Once you adjust the two tests above to rely purely on type-level generics (WhereInput,DefaultModelResult), usingnoopas a sink is perfectly reasonable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/sdk/src/ts-schema-generator.ts(4 hunks)tests/e2e/orm/client-api/ignore.test.ts(1 hunks)tests/e2e/orm/schemas/basic/schema.zmodel(3 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/client-api/ignore.test.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/basic/schema.zmodel
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
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.
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to **/*.zmodel : Always run `zenstack generate` after modifying ZModel schemas
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
📚 Learning: 2025-11-26T01:55:04.529Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/sdk/src/ts-schema-generator.tstests/e2e/orm/client-api/ignore.test.tstests/e2e/orm/schemas/basic/schema.zmodel
📚 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/ts-schema-generator.tstests/e2e/orm/client-api/ignore.test.ts
📚 Learning: 2025-11-26T01:55:04.529Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/ignore.test.ts
📚 Learning: 2025-11-26T01:55:04.529Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/ignore.test.ts
📚 Learning: 2025-11-26T01:55:04.529Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/client-api/ignore.test.ts
📚 Learning: 2025-11-26T01:55:04.529Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.529Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/ignore.test.ts
🧬 Code graph analysis (1)
tests/e2e/orm/client-api/ignore.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(95-244)packages/orm/src/client/crud-types.ts (1)
DefaultModelResult(50-69)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (5)
packages/sdk/src/ts-schema-generator.ts (2)
297-304: Centralized filtering for ignored models/typeDefs looks correct and consistentUsing
getAllDataModels/getAllTypeDefsto:
- build the
modelsobject,- build the
typeDefsobject,and filtering by
!hasAttribute(d, '@@ignore')ensures@@ignored declarations are consistently excluded from both the schema shape and downstream type generation. This directly supports the PR goal of hiding ignored models/typeDefs from the TS schema and ORM surface.Also applies to: 306-312, 314-321
1348-1381: Model/typeDef and input arg type generation now correctly exclude@@ignored modelsReusing
this.getAllDataModels(model)andthis.getAllTypeDefs(model)in:
generateModelsAndTypeDefsfor exported<Model>/<TypeDef>aliases, andgenerateInputTypesfor<Model>CreateArgs,<Model>WhereInput, etc.,means ignored models/typeDefs will no longer get any exported Model / TypeDef / input-arg types. That aligns with the E2E test expectations (
Fooshould not appear in the client API at the type level).Also applies to: 1503-1633
tests/e2e/orm/schemas/basic/schema.zmodel (3)
21-33:password @ignoreand new read policy fit the ignore tests; verify the policy semantics
- Adding
password String @ignoreonUseris a good fit for the ignore behavior tests: the field still exists in the DB/schema but should be hidden from the generated client API.- The new
@@allow('read', auth() != null)makes all authenticated users able to read anyUser, not just themselves. That’s probably fine for a test schema, but it’s a broader policy than@@allow('all', auth().id == id).Please double‑check that this wider read access is intentional for this fixture.
49-53: Comment model tweaks are purely structuralThe
Commentmodel changes (ordering/spacing ofcontent,post,postId) don’t alter semantics and remain consistent with the rest of the schema.
62-66:Foowith@@ignoreis a good minimal target for ignored-model testsDefining:
model Foo { id String @id @@ignore }gives you a clean, single-field model for asserting that ignored models do not surface in the generated schema or client API. This pairs well with the new E2E tests.
Don’t forget to run
zenstack generateso the TS schema stays in sync with this addition.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Claude finished @ymc9's task —— View job PR Review: Fix TS Schema Generation for Ignored Models and FieldsI've analyzed the changes and will provide comprehensive review feedback covering code quality, potential bugs, performance, security concerns, and test coverage. Todo List:
Summary of ChangesProblem IdentifiedThe issue was that TypeScript schema generation wasn't properly filtering out:
Root CauseThe code was inconsistent in how it handled ignored entities:
The FixMain changes in
Code Quality Assessment✅ Strengths
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Claude finished @ymc9's task —— View job PR Review: Fix TS Schema Generation for Ignored Models and Fields
|
|
Claude finished @ymc9's task —— View job PR Review: Fix TS Schema Generation for Ignored Models and Fields ✅I've analyzed this PR and will provide comprehensive feedback on code quality, potential bugs, performance, security, and test coverage. Todo List:
Summary of ChangesProblem IdentifiedThe issue was that TypeScript schema generation wasn't properly filtering out:
Root CauseThe code was inconsistent in how it handled ignored entities:
The FixMain changes in
Code Quality Assessment✅ Strengths
|
Summary by CodeRabbit
Tests
Refactor
Schema
✏️ Tip: You can customize this high-level summary in your review settings.