feat(orm): implement field omission #441
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. WalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: Field Omission Implementation ✅Reviewing the field omission feature implementation across 30 files. Todo List:
Review SummaryThis is a well-implemented feature that adds field omission capabilities to ZenStack v3. The implementation is comprehensive, type-safe, and properly tested. ✅ Strengths1. Clean Architecture & Design
2. Implementation Quality
3. Type Safety
4. Test Coverage
🎯 Technical AnalysisPerformance Impact: ✅ Minimal
Security Considerations: ✅ Good
Backward Compatibility: ✅ Maintained
🔍 Minor Observations1. Test Typo (Line 54 in omit.test.ts): // Missing parentheses
expect(user.password).toBeUndefined; // ❌
expect(user.password).toBeUndefined(); // ✅2. Potential Enhancement: 📋 Code Quality Assessment
✅ Recommendation: APPROVEThis PR implements a valuable feature with:
The field omission feature adds significant value for protecting sensitive data while maintaining ZenStack's excellent type safety and developer experience. |
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive field omission functionality for ORM query results with three levels of precedence: query-level > client options-level > schema-level. The implementation adds an @omit attribute to the schema language, extends client type definitions to support omission options, and updates all ORM dialects to respect omission rules.
Key changes:
- Adds
@omitattribute to ZModel schema language and FieldDef schema structure - Extends ClientContract and ClientOptions with generic Options parameter to track omit configuration at type level
- Implements three-level omission precedence in type system and runtime
- Updates TypeScript from 5.8.0 to 5.9.3 (potentially to support new type features)
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/res/stdlib.zmodel | Adds @omit attribute definition to schema language |
| packages/schema/src/schema.ts | Adds omit?: boolean field to FieldDef interface |
| packages/orm/src/client/options.ts | Defines OmitOptions type for client-level field omission configuration |
| packages/orm/src/client/contract.ts | Extends ClientContract with Options generic parameter throughout |
| packages/orm/src/client/crud-types.ts | Implements complex type-level omission logic with precedence rules |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Implements shouldOmitField runtime method with precedence logic |
| packages/orm/src/client/crud/dialects/postgresql.ts | Updates relation field selection to respect omit options |
| packages/orm/src/client/crud/dialects/sqlite.ts | Updates relation field selection to respect omit options |
| packages/orm/src/client/client-impl.ts | Removes generic constraints for performance, uses type erasure |
| packages/testtools/src/client.ts | Updates createTestClient signatures to support Options parameter |
| packages/sdk/src/ts-schema-generator.ts | Generates omit field in schema and adds ClientOptions to GetPayload types |
| tests/e2e/orm/client-api/omit.test.ts | Comprehensive test coverage for all omission scenarios |
| pnpm-workspace.yaml | Updates TypeScript version to 5.9.3 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 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.
Co-authored-by: Copilot <[email protected]>
|
Claude finished @ymc9's task —— View job PR Review: Field Omission Implementation ✅Reviewing the field omission feature implementation across 30 files. Todo List:
Review SummaryThis is a well-implemented feature that adds field omission capabilities to ZenStack v3. The implementation is comprehensive, type-safe, and properly tested. ✅ Strengths1. Clean Architecture & Design
2. Implementation Quality
3. Type Safety
4. Test Coverage
🎯 Technical AnalysisPerformance Impact: ✅ Minimal
Security Considerations: ✅ Good
Backward Compatibility: ✅ Maintained
🔍 Minor Observations1. Test Typo (Line 54 in omit.test.ts): // Missing parentheses
expect(user.password).toBeUndefined; // ❌
expect(user.password).toBeUndefined(); // ✅2. Potential Enhancement: 📋 Code Quality Assessment
✅ Recommendation: APPROVEThis PR implements a valuable feature with:
The field omission feature adds significant value for protecting sensitive data while maintaining ZenStack's excellent type safety and developer experience. |
![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 (3)
packages/orm/src/client/client-impl.ts (1)
170-189: Double-cast pattern is necessary but consider documenting.The
as unknown as ClientContract<SchemaDef>casts on lines 176 and 186 are necessary due to the class's non-generic design. The actual schema instance is correctly propagated at runtime.Consider adding a brief inline comment explaining why the double cast is needed, to help future maintainers understand the design decision.
packages/language/res/stdlib.zmodel (1)
396-401: @omit attribute definition aligns with the new ORM omit semanticsThe
@omit()attribute and its documentation correctly describe default field omission, override via client options or query-timeomit, and restriction to ORM query APIs. One optional improvement would be to clarify in the comment whether applying@omitto relation fields is supported or only intended for scalar fields, to avoid ambiguity for schema authors. Also ensure generators that consumestdlib.zmodelhave been re-run so the new attribute is reflected in generated TypeScript.As per coding guidelines, remember to run
zenstack generateafter modifying.zmodelschemas.packages/orm/src/client/crud/dialects/base-dialect.ts (1)
872-881: Centralized omit resolution and precedence look good; consider slightly stricter guardUsing
shouldOmitFieldhere correctly applies query-level → client-options-level → schema-level precedence and matches the new E2E tests. The implementation is clear and keeps runtime behavior aligned with the new type-levelomitmodel.If you want to be extra defensive, you could replace the
typeof omit === 'object'check withisPlainObject(omit)for the query-level branch to avoid accidentally treating arrays or odd prototypes as omit maps, but that's optional and not required for correctness.Also applies to: 904-922
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
package.json(2 hunks)packages/language/res/stdlib.zmodel(1 hunks)packages/orm/src/client/client-impl.ts(26 hunks)packages/orm/src/client/contract.ts(20 hunks)packages/orm/src/client/crud-types.ts(5 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(2 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(1 hunks)packages/orm/src/client/crud/operations/base.ts(1 hunks)packages/orm/src/client/crud/validator/index.ts(1 hunks)packages/orm/src/client/options.ts(3 hunks)packages/orm/src/client/promise.ts(1 hunks)packages/schema/src/schema.ts(1 hunks)packages/sdk/src/ts-schema-generator.ts(4 hunks)packages/testtools/src/client.ts(4 hunks)pnpm-workspace.yaml(1 hunks)tests/e2e/apps/rally/zenstack/input.ts(28 hunks)tests/e2e/orm/client-api/omit.test.ts(1 hunks)tests/e2e/orm/schemas/basic/input.ts(5 hunks)tests/e2e/orm/schemas/default-auth/input.ts(4 hunks)tests/e2e/orm/schemas/delegate/input.ts(8 hunks)tests/e2e/orm/schemas/name-mapping/input.ts(3 hunks)tests/e2e/orm/schemas/omit/input.ts(1 hunks)tests/e2e/orm/schemas/omit/models.ts(1 hunks)tests/e2e/orm/schemas/omit/schema.ts(1 hunks)tests/e2e/orm/schemas/omit/schema.zmodel(1 hunks)tests/e2e/orm/schemas/petstore/input.ts(4 hunks)tests/e2e/orm/schemas/todo/input.ts(6 hunks)tests/e2e/orm/schemas/typing/input.ts(7 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/omit.test.tstests/e2e/orm/schemas/omit/schema.tstests/e2e/orm/schemas/name-mapping/input.tstests/e2e/orm/schemas/todo/input.tstests/e2e/orm/schemas/omit/models.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/petstore/input.tstests/e2e/orm/schemas/delegate/input.tstests/e2e/orm/schemas/default-auth/input.tstests/e2e/orm/schemas/typing/input.tstests/e2e/apps/rally/zenstack/input.tstests/e2e/orm/schemas/omit/input.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/omit/schema.zmodelpackages/language/res/stdlib.zmodel
🧠 Learnings (9)
📚 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/omit.test.tstests/e2e/orm/schemas/omit/schema.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/promise.tspackages/orm/src/client/crud-types.tstests/e2e/orm/schemas/name-mapping/input.tspackages/orm/src/client/options.tstests/e2e/orm/schemas/todo/input.tstests/e2e/orm/schemas/omit/models.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/petstore/input.tstests/e2e/orm/schemas/delegate/input.tstests/e2e/orm/schemas/default-auth/input.tspackages/orm/src/client/contract.tstests/e2e/orm/schemas/typing/input.tspackages/orm/src/client/client-impl.tspackages/testtools/src/client.tstests/e2e/apps/rally/zenstack/input.tstests/e2e/orm/schemas/omit/input.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/omit.test.tstests/e2e/orm/schemas/omit/schema.tstests/e2e/orm/schemas/name-mapping/input.tstests/e2e/orm/schemas/todo/input.tstests/e2e/orm/schemas/omit/models.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/delegate/input.tstests/e2e/orm/schemas/default-auth/input.tspackages/testtools/src/client.tstests/e2e/orm/schemas/omit/input.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/omit.test.tspackages/orm/src/client/crud/dialects/base-dialect.tstests/e2e/orm/schemas/omit/schema.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/promise.tspackages/orm/src/client/crud-types.tstests/e2e/orm/schemas/name-mapping/input.tspackages/orm/src/client/options.tstests/e2e/orm/schemas/todo/input.tstests/e2e/orm/schemas/omit/models.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/petstore/input.tstests/e2e/orm/schemas/delegate/input.tspackages/sdk/src/ts-schema-generator.tstests/e2e/orm/schemas/default-auth/input.tspackages/orm/src/client/contract.tstests/e2e/orm/schemas/typing/input.tspackages/orm/src/client/client-impl.tstests/e2e/apps/rally/zenstack/input.tstests/e2e/orm/schemas/omit/input.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} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/crud/dialects/base-dialect.tstests/e2e/orm/schemas/omit/schema.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/promise.tspackages/orm/src/client/crud-types.tstests/e2e/orm/schemas/name-mapping/input.tspackages/orm/src/client/options.tstests/e2e/orm/schemas/todo/input.tstests/e2e/orm/schemas/omit/models.tstests/e2e/orm/schemas/basic/input.tstests/e2e/orm/schemas/petstore/input.tstests/e2e/orm/schemas/delegate/input.tstests/e2e/orm/schemas/default-auth/input.tspackages/orm/src/client/contract.tstests/e2e/orm/schemas/typing/input.tspackages/orm/src/client/client-impl.tstests/e2e/apps/rally/zenstack/input.tstests/e2e/orm/schemas/omit/input.ts
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
packages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/dialects/postgresql.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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
tests/e2e/orm/schemas/omit/schema.tspackages/orm/src/client/promise.tstests/e2e/orm/schemas/omit/schema.zmodeltests/e2e/orm/schemas/omit/models.tspackages/sdk/src/ts-schema-generator.tspackages/orm/src/client/contract.tspackages/orm/src/client/client-impl.tstests/e2e/apps/rally/zenstack/input.tstests/e2e/orm/schemas/omit/input.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:
tests/e2e/orm/schemas/omit/schema.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: Use `pnpm` with workspaces for package management, pinned to version `pnpm10.12.1`
Applied to files:
pnpm-workspace.yamlpackage.json
📚 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: Use Turbo for build orchestration and run `pnpm build`, `pnpm watch`, `pnpm lint`, and `pnpm test` for development tasks
Applied to files:
package.json
🧬 Code graph analysis (16)
tests/e2e/orm/client-api/omit.test.ts (4)
packages/testtools/src/client.ts (1)
createTestClient(99-248)packages/orm/src/client/crud/operations/base.ts (3)
schema(98-100)options(102-104)read(148-187)packages/orm/src/client/crud/validator/index.ts (1)
schema(60-62)tests/e2e/orm/schemas/omit/schema.ts (1)
schema(141-141)
packages/orm/src/client/crud/dialects/base-dialect.ts (2)
packages/orm/src/client/crud/operations/base.ts (1)
requireField(129-131)packages/orm/src/client/query-utils.ts (1)
requireField(45-63)
tests/e2e/orm/schemas/omit/schema.ts (4)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/crud/operations/base.ts (1)
schema(98-100)packages/orm/src/client/crud/validator/index.ts (1)
schema(60-62)
packages/orm/src/client/promise.ts (1)
packages/orm/src/client/contract.ts (1)
ClientContract(61-181)
packages/orm/src/client/crud-types.ts (2)
packages/schema/src/schema.ts (11)
SchemaDef(11-19)GetModels(121-121)IsDelegateModel(280-283)GetSubModels(127-132)NonRelationFields(205-209)GetModelFields(147-150)GetModelField(152-156)GetModelDiscriminator(158-164)ModelFieldIsOptional(232-236)FieldIsArray(250-254)RelationFieldType(223-230)packages/orm/src/client/options.ts (1)
ClientOptions(45-101)
tests/e2e/orm/schemas/name-mapping/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (1)
UserGetPayload(50-50)tests/e2e/orm/schemas/basic/input.ts (2)
UserGetPayload(30-30)PostGetPayload(50-50)tests/e2e/orm/schemas/default-auth/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/delegate/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (2)
UserGetPayload(30-30)PostGetPayload(50-50)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (2)
UserGetPayload(30-30)PostGetPayload(50-50)
tests/e2e/orm/schemas/todo/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (2)
SpaceGetPayload(270-270)UserGetPayload(50-50)tests/e2e/orm/schemas/basic/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/default-auth/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/delegate/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/name-mapping/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/typing/input.ts (1)
UserGetPayload(30-30)
tests/e2e/orm/schemas/basic/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (2)
UserGetPayload(50-50)CommentGetPayload(230-230)tests/e2e/orm/schemas/default-auth/input.ts (2)
UserGetPayload(30-30)ProfileGetPayload(50-50)tests/e2e/orm/schemas/delegate/input.ts (2)
UserGetPayload(30-30)CommentGetPayload(50-50)tests/e2e/orm/schemas/name-mapping/input.ts (2)
UserGetPayload(30-30)PostGetPayload(50-50)tests/e2e/orm/schemas/omit/input.ts (2)
UserGetPayload(30-30)PostGetPayload(50-50)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (3)
UserGetPayload(30-30)PostGetPayload(50-50)ProfileGetPayload(70-70)
tests/e2e/orm/schemas/petstore/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (1)
UserGetPayload(50-50)tests/e2e/orm/schemas/basic/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/default-auth/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/delegate/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/name-mapping/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (1)
UserGetPayload(30-30)
tests/e2e/orm/schemas/delegate/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (2)
UserGetPayload(50-50)CommentGetPayload(230-230)tests/e2e/orm/schemas/basic/input.ts (2)
UserGetPayload(30-30)CommentGetPayload(70-70)tests/e2e/orm/schemas/default-auth/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/name-mapping/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (1)
UserGetPayload(30-30)
packages/sdk/src/ts-schema-generator.ts (2)
packages/sdk/src/model-utils.ts (1)
hasAttribute(59-64)packages/language/src/utils.ts (1)
hasAttribute(55-57)
tests/e2e/orm/schemas/default-auth/input.ts (8)
tests/e2e/apps/rally/zenstack/input.ts (1)
UserGetPayload(50-50)tests/e2e/orm/schemas/basic/input.ts (2)
UserGetPayload(30-30)ProfileGetPayload(90-90)tests/e2e/orm/schemas/delegate/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/name-mapping/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (2)
UserGetPayload(30-30)ProfileGetPayload(70-70)
packages/orm/src/client/contract.ts (3)
packages/orm/src/client/options.ts (1)
ClientOptions(45-101)packages/orm/src/client/promise.ts (1)
ZenStackPromise(7-13)packages/orm/src/client/crud-types.ts (1)
DefaultModelResult(51-75)
packages/testtools/src/client.ts (3)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/options.ts (1)
ClientOptions(45-101)packages/orm/src/client/contract.ts (1)
ClientContract(61-181)
tests/e2e/apps/rally/zenstack/input.ts (7)
tests/e2e/orm/schemas/basic/input.ts (2)
UserGetPayload(30-30)CommentGetPayload(70-70)tests/e2e/orm/schemas/default-auth/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/name-mapping/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/omit/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/petstore/input.ts (1)
UserGetPayload(30-30)tests/e2e/orm/schemas/todo/input.ts (1)
UserGetPayload(70-70)tests/e2e/orm/schemas/typing/input.ts (1)
UserGetPayload(30-30)
tests/e2e/orm/schemas/omit/input.ts (1)
tests/e2e/apps/rally/zenstack/input.ts (20)
UserFindManyArgs(31-31)UserFindUniqueArgs(32-32)UserFindFirstArgs(33-33)UserCreateArgs(34-34)UserCreateManyArgs(35-35)UserCreateManyAndReturnArgs(36-36)UserUpdateArgs(37-37)UserUpdateManyArgs(38-38)UserUpdateManyAndReturnArgs(39-39)UserUpsertArgs(40-40)UserDeleteArgs(41-41)UserDeleteManyArgs(42-42)UserCountArgs(43-43)UserAggregateArgs(44-44)UserGroupByArgs(45-45)UserWhereInput(46-46)UserSelect(47-47)UserInclude(48-48)UserOmit(49-49)UserGetPayload(50-50)
⏰ 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). (2)
- GitHub Check: Upload results
- GitHub Check: claude-review
🔇 Additional comments (62)
packages/schema/src/schema.ts (1)
72-72: LGTM!The
omit?: booleanproperty is well-placed withinFieldDefalongside related field metadata. This cleanly supports the schema-level omission feature being introduced.package.json (2)
5-5: Verify pnpm version alignment with project standards.The packageManager is set to
[email protected], but learnings indicate the project should use[email protected]. Please confirm whether this version bump is intentional and if the learnings need to be updated.Based on learnings, the expected version is
[email protected].
28-35: LGTM!Using
catalog:references forprismaandyamldependencies aligns with the pnpm workspace pattern defined inpnpm-workspace.yaml.tests/e2e/orm/schemas/basic/input.ts (5)
9-10: LGTM!The import addition of
ClientOptions as $ClientOptionscorrectly supports the new Options generic parameter in GetPayload types.
30-30: LGTM!The
UserGetPayloadsignature update to include theOptionsgeneric parameter with a sensible default is consistent with the pattern across other test schemas (petstore, todo, omit, delegate, etc.) and aligns with the broader Options-aware typing introduced in this PR.
50-50: Consistent with PR pattern.
PostGetPayloadsignature follows the same Options-aware pattern.
70-70: Consistent with PR pattern.
CommentGetPayloadsignature follows the same Options-aware pattern.
90-90: Consistent with PR pattern.
ProfileGetPayloadsignature follows the same Options-aware pattern. As per coding guidelines, E2E tests should validate real-world schema compatibility, and these generated types enable that validation.packages/orm/src/client/client-impl.ts (7)
46-52: LGTM on ZenStackClient factory.The factory correctly casts
optionstoClientOptions<SchemaDef>to work with the non-genericClientImplclass. This maintains the public API's type safety while allowing internal simplification.
54-67: Intentional simplification for tsc performance.The removal of the
Schemageneric parameter fromClientImplin favor ofSchemaDefis a deliberate architectural decision. Type safety is preserved at the public API boundary throughZenStackClientfactory andClientContract<Schema>types.
140-149: LGTM on transaction overloads.The transaction overloads are correctly updated to use
SchemaDef-based contract types, maintaining consistency with the non-generic class design.
255-287: LGTM on plugin methods.The explicit typing of
newPluginsandnewOptionswithRuntimePlugin<SchemaDef>[]andClientOptions<SchemaDef>respectively improves code clarity while supporting the tsc performance optimization mentioned in comments.
368-373: Intentional any-typing for tsc performance.The use of
anyfor type parameters increateModelCrudHandleris a deliberate trade-off for TypeScript compilation performance, as noted in the comment on line 421. Runtime safety is maintained throughInputValidatorand the public API boundary preserves full type safety.
422-594: Consistent any-typing across operation handlers.All operation handler instantiations (
FindOperationHandler<any>,CreateOperationHandler<any>, etc.) follow the same pattern, which is appropriate given the performance optimization goal. The explicitas ModelOperations<any, any>cast on line 594 makes the trade-off visible.
346-366: LGTM on createClientProxy simplification.The non-generic function correctly bridges the
ClientImplinstance to the genericInputValidatorand model CRUD handlers through necessaryanycasts, maintaining the proxy pattern while supporting the simplified class design.pnpm-workspace.yaml (1)
12-12: TypeScript 5.9.3 is a valid and released version.The version is published on npm and was released on October 1, 2025. The bump from ^5.8.0 to ^5.9.3 is supported by legitimate new features in TypeScript 5.9, making this dependency upgrade appropriate for the PR's expanded generics and Options-based typing changes.
tests/e2e/apps/rally/zenstack/input.ts (1)
9-10: Options-aware GetPayload typings look consistent and safeThe added
ClientOptions as $ClientOptionsimport and the newOptions extends $ClientOptions<$Schema> = $ClientOptions<$Schema>parameter threaded into all*GetPayloadaliases are consistent with other schemas and keep existing call sites working via the default. No issues from a typing or behavior standpoint.Also applies to: 30-550
packages/orm/src/client/crud/dialects/postgresql.ts (1)
15-15: Centralized omit handling and error reporting are correct hereUsing
this.shouldOmitField(omit, relationModel, name)when selecting scalar relation fields correctly delegates omit resolution to the shared dialect helper, so schema/options/query-level omit are applied consistently. The newcreateInternalErrorusage ingetFieldSqlTypefor relation fields also tightens error reporting without changing behavior.Also applies to: 307-317, 441-443
packages/orm/src/client/crud/dialects/sqlite.ts (1)
200-207: SQLite relation scalar omission wired through shouldOmitField correctlyIn the
payload === true || !payload.selectbranch, derivingomitfrompayload.omitand filtering scalar fields with!this.shouldOmitField(omit, relationModel, name)cleanly reuses the shared omit resolution. This keeps SQLite behavior aligned with PostgreSQL and the base dialect.packages/orm/src/client/crud/validator/index.ts (1)
260-293: Omit support in input validation is well-integratedThe new validation rules for
omitlook coherent:
makeFindSchemaaddsomitand refines soselectcannot be combined withomit, while still allowinginclude+omit.- Relation selection (
makeRelationSelectIncludeSchema) now supports nestedomitwith the sameselectvsomitexclusivity.makeOmitSchemarestricting omit to non-relation fields matches the runtime’s scalar-only omit behavior.- Create/Update/Upsert/Delete schemas all accept
omitand are guarded byrefineForSelectOmitMutuallyExclusive, keeping the API surface consistent.Overall this aligns the Zod layer with the new omit semantics threaded through the client.
Also applies to: 740-790, 872-883, 889-896, 1150-1193, 1301-1309, 1516-1521
packages/orm/src/client/crud/operations/base.ts (1)
2147-2163: Read-back decision now respects omit configurationThe updated
mutationNeedsReadBackcorrectly excludes fields thatthis.dialect.shouldOmitField(args.omit, model, f)marks as omitted, both in the implicit “all scalars” case and the explicitselectcase. That means omitted fields no longer force unnecessary read-backs nor get treated as part of the selected scalar set, while relation/computed-field detection remains intact.tests/e2e/orm/schemas/omit/models.ts (1)
1-13: Generated ModelResult aliases are correctThe new
User,Post,Base, andSubaliases to$ModelResult<$Schema, "...">are straightforward and consistent with other schema fixtures, and will pick up the default Options generic fromModelResult. No changes needed here.tests/e2e/orm/schemas/omit/schema.ts (1)
8-137: Generated schema correctly reflects @omit and delegate structureThe generated schema mirrors the zmodel:
User.passwordandSub.contentare marked withomit: trueand have@omitinattributes, and delegate metadata forBase/Sublooks consistent. This should work well withshouldOmitFieldand the new omit pipeline.packages/orm/src/client/options.ts (1)
45-101: Client-level OmitOptions wiring is consistent with dialect omit logicThreading
omit?: OmitOptions<Schema>throughClientOptionsand constraining it to scalar fields viaScalarFields<Schema, Model>aligns with howBaseCrudDialect.shouldOmitFieldreadsthis.options.omit. This should give good type safety for client-level omit configuration without affecting relation handling.Also applies to: 103-110
packages/sdk/src/ts-schema-generator.ts (1)
554-556: @omit schema emission and Options-aware GetPayload generation are consistentAdding
omit: truewhen a field has@omitincreateDataFieldObjectcorrectly surfaces schema-level omission for the runtime (and matches the generated omit schema). The updated input-type generation—importingClientOptions as $ClientOptionsand definingModelGetPayloadwith anOptionstype parameter passed into$SimplifiedModelResult—aligns with the new Options-threaded payload model used elsewhere.This should keep generated schemas and input types in sync with the central omit and options behavior.
Also applies to: 1557-1571, 1619-1650
tests/e2e/orm/schemas/name-mapping/input.ts (1)
9-10: Name-mapping GetPayload types now correctly parameterized by OptionsImporting
ClientOptions as $ClientOptionsand updatingUserGetPayload/PostGetPayloadto take anOptions extends $ClientOptions<$Schema> = $ClientOptions<$Schema>parameter keeps these generated types consistent with the rest of the schemas using$SimplifiedModelResult<$Schema, Model, Options, Args>.Also applies to: 30-30, 50-50
tests/e2e/orm/schemas/omit/schema.zmodel (1)
1-29: Omit-focused test schema looks correct; ensure generators stay in syncThe zmodel cleanly exercises
@omitonUser.passwordandSub.content, plus a simple delegate hierarchy forBase/Sub, which matches the generatedschema.tsyou added.Per the project guidelines, just make sure
zenstack generatehas been (and will be) re-run whenever this schema changes so the generated TS artifacts remain consistent.tests/e2e/orm/schemas/petstore/input.ts (1)
9-10: Auto-generated types correctly updated for Options threading.The
GetPayloadtypes for User, Pet, and Order now correctly include theOptionsgeneric parameter with proper defaults, and the import ofClientOptionsis added. The pattern is consistent with other generated files in the codebase.Also applies to: 30-30, 50-50, 70-70
tests/e2e/orm/schemas/todo/input.ts (1)
9-10: Auto-generated types correctly updated for all models.All five GetPayload types (Space, SpaceUser, User, List, Todo) are updated with the
Optionsgeneric parameter following the consistent pattern across the codebase.Also applies to: 30-30, 50-50, 70-70, 90-90, 110-110
tests/e2e/orm/schemas/typing/input.ts (1)
9-10: Auto-generated types correctly updated for all models.All six GetPayload types (User, Post, Profile, Tag, Region, Meta) are updated with the
Optionsgeneric parameter, consistent with the PR-wide pattern.Also applies to: 30-30, 50-50, 70-70, 90-90, 110-110, 130-130
tests/e2e/orm/schemas/delegate/input.ts (1)
9-10: Auto-generated types correctly updated for all models including delegate hierarchy.All seven GetPayload types (User, Comment, Asset, Video, RatedVideo, Image, Gallery) are updated with the
Optionsgeneric parameter. This correctly covers the delegate model hierarchy (Asset with Video/RatedVideo/Image subtypes).Also applies to: 30-30, 50-50, 70-70, 90-90, 110-110, 130-130, 150-150
packages/orm/src/client/crud-types.ts (4)
77-116: Well-designed omit precedence system.The three-level omit resolution (query → options → schema) is cleanly implemented:
- Each level returns
undefinedwhen not specified, allowing fallthrough to the next levelSchemaLevelOmitis the terminal case, always returningtrueorfalse- The
extends booleancheck correctly distinguishes explicittrue/falsefromundefined
51-75: Options threading in DefaultModelResult and DelegateUnionResult is correct.The
Optionsgeneric is properly threaded through both types, andShouldOmitFieldis correctly applied to filter non-relation fields based on the multi-level omit precedence.Also applies to: 117-125
127-180: ModelSelectResult correctly threads Options to nested relations.The
Optionsparameter is properly passed to nestedModelResultandDefaultModelResultcalls for relation fields, enabling consistent omit behavior across nested query results.
240-247: SimplifiedModelResult requires explicit Options parameter.The
SimplifiedModelResulttype requiresOptionswithout a default, which is intentional. The generatedGetPayloadtypes provide the default (Options extends $ClientOptions<$Schema> = $ClientOptions<$Schema>), ensuring type safety at the call site while allowing flexibility for explicit option customization.tests/e2e/orm/schemas/default-auth/input.ts (4)
9-10: LGTM!The
ClientOptionsimport and its usage in theGetPayloadtypes are correctly added, aligning with the new Options-aware type system for field omission support.
30-30: Consistent with the Options threading pattern.The
UserGetPayloadtype signature with the newOptionsgeneric parameter matches the pattern used across other generated files (e.g.,tests/e2e/apps/rally/zenstack/input.ts,tests/e2e/orm/schemas/basic/input.ts).
50-50: ProfileGetPayload signature is correct.
70-70: AddressGetPayload signature is correct.packages/testtools/src/client.ts (7)
35-88: Clean extraction of test-specific options.Separating
ExtraTestClientOptionsfromClientOptionsimproves clarity and maintainability. The type structure is well-documented with JSDoc comments.
90-91: Type composition looks correct.
CreateTestClientOptionsproperly combines client options (minus dialect) with extra test options.
99-102: Implementation signature correctly uses SchemaDef.Using
SchemaDefas the base type in the implementation is appropriate since it handles both the typed schema object and string-parsed schema cases.
104-104: Variable renamed to_schema: SchemaDef.Consistent with the updated generic approach.
125-125: Cast toSchemaDefis appropriate.The generated schema from string parsing needs to be cast since the generator returns a generic schema object.
155-157: Destructuring excludespluginscorrectly.The
pluginsare handled separately and not passed directly to the client options. The rest spread includes extra test options, but sinceZenStackClientwill ignore unknown properties, this is acceptable.
93-98: I'll help you verify the overload type inference and the filtering ofExtraTestClientOptionsproperties. Let me start by examining the relevant code sections.
<function_calls>
#!/bin/bashFirst, let's view the lines around 155-157 to see how _options is constructed
wc -l packages/testtools/src/client.ts
</function_calls>Looking at the file size first, then I'll examine the specific lines mentioned in the review comment.
<function_calls>
#!/bin/bashView lines 150-165 to see the _options construction
sed -n '150,165p' packages/testtools/src/client.ts
</function_calls>Now let me search for the
ExtraTestClientOptionsdefinition and understand the filtering logic:
<function_calls>
#!/bin/bashSearch for ExtraTestClientOptions definition
rg -n "ExtraTestClientOptions" packages/testtools/src/client.ts -A 5 -B 2
</function_calls>Let me also view the complete context of lines 93-98 and the function implementation:
<function_calls>
#!/bin/bashView lines 85-110 to see overloads and function start
sed -n '85,110p' packages/testtools/src/client.ts
</function_calls><function_calls>
#!/bin/bashLet's look at the createTestClientOptions type definition
rg -n "CreateTestClientOptions" packages/testtools/src/client.ts -A 5 -B 2
</function_calls><function_calls>
#!/bin/bashCheck the complete function implementation from line 99 onwards
sed -n '99,180p' packages/testtools/src/client.ts
</function_calls>packages/orm/src/client/contract.ts (11)
61-67: Options generic properly threaded through ClientContract.The
Optionsparameter with its defaultClientOptions<Schema>maintains backward compatibility while enabling Options-aware result types. The$optionsproperty correctly uses theOptionstype.
115-121: Methods returning new clients preserve Options.
$setAuthand$setInputValidationcorrectly returnClientContract<Schema, Options>to maintain type consistency across client transformations.
136-139: Transaction callback type updated correctly.The transaction callback receives a client with the same Options type, ensuring omit behavior is consistent within transactions.
152-162: Plugin methods preserve Options type.
$use,$unuse, and$unuseAllcorrectly return clients with the same Options type.
180-181: Model operations mapped with Options.The intersection type correctly passes
OptionstoModelOperations, enabling Options-aware result types for all model operations.
186-189: TransactionClientContract properly parameterized.Consistent with the main ClientContract signature.
230-234: ClientConstructor signature updated.The constructor correctly accepts and propagates the Options type parameter.
258-262: AllModelOperations now Options-aware.This enables all CRUD operations to return properly typed results with omit support.
344-346: findMany return type includes Options.
SimplifiedModelResult<Schema, Model, Options, T>ensures omit configuration is reflected in the result type.
819-827: ModelOperations delegates to AllModelOperations with Options.The Omit wrapper correctly handles delegate model exclusions while preserving the Options parameter.
837-843: AuthModelType intentionally uses default ClientOptions.Using
ClientOptions<Schema>without the custom Options parameter for auth context is correct—auth data should not be affected by user-configured omit options since it's used internally for authorization checks.tests/e2e/orm/schemas/omit/input.ts (5)
1-10: Auto-generated file with correct structure.The file follows the standard generated input pattern with proper imports including
ClientOptionsfor Options-aware types.
11-30: User model types correctly generated.All CRUD operation types and the
UserGetPayloadwith Options parameter are consistent with the omit feature implementation.
31-50: Post model types correctly generated.
51-70: Base model types correctly generated.Including delegate model support for the omit schema tests.
71-90: Sub model types correctly generated.The Sub model (inheriting from Base) is properly typed for testing omit behavior with model inheritance. As per coding guidelines, this E2E test schema validates real-world schema compatibility with the new omit feature.
|
Claude finished @ymc9's task —— View job PR Review: Field Omission Implementation ✅Reviewing the field omission feature implementation across 30 files. Todo List:
Review SummaryThis is a well-implemented feature that adds field omission capabilities to ZenStack v3. The implementation is comprehensive, type-safe, and properly tested. ✅ Strengths1. Clean Architecture & Design
2. Implementation Quality
3. Type Safety
4. Test Coverage
🎯 Technical AnalysisPerformance Impact: ✅ Minimal
Security Considerations: ✅ Good
Backward Compatibility: ✅ Maintained
🔍 Minor Observations1. Test Typo (Line 54 in omit.test.ts): // Missing parentheses
expect(user.password).toBeUndefined; // ❌
expect(user.password).toBeUndefined(); // ✅2. Potential Enhancement: 📋 Code Quality Assessment
✅ Recommendation: APPROVEThis PR implements a valuable feature with:
The field omission feature adds significant value for protecting sensitive data while maintaining ZenStack's excellent type safety and developer experience. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/orm/client-api/omit.test.ts (1)
39-77: LGTM!The test correctly validates client-level omit configuration and precedence rules. Line 54's assertion is properly invoked with
toBeUndefined(), resolving the past review concern.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/omit.test.ts (1)
5-139: Consider adding tests for batch operations and additional finder methods.The current test coverage is solid. To further strengthen the E2E validation, consider adding tests for:
- Batch operations:
createMany,updateMany,deleteManywith omit behavior- Additional finders:
findMany,findUnique,findUniqueOrThrowwith omit configurations- Aggregation operations if omit should affect those
This aligns with the guideline that E2E tests should validate real-world compatibility, and batch operations are common in production scenarios.
Based on learnings, ORM package tests should include comprehensive client API tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/orm/client-api/omit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/omit.test.ts
🧠 Learnings (3)
📚 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/omit.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/omit.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/omit.test.ts
🔇 Additional comments (4)
tests/e2e/orm/client-api/omit.test.ts (4)
1-3: LGTM!The imports are appropriate for an E2E test validating field omission functionality.
6-37: LGTM!The test comprehensively validates schema-level omit across all major CRUD operations (create, read, update, delete) and nested relations. All assertions are correctly invoked.
79-122: LGTM!The test thoroughly validates query-level omit precedence and override behavior across all CRUD operations and nested relations. The use of different IDs (1 and 2) properly avoids potential conflicts.
124-138: LGTM!The test correctly validates that field omission works with delegate model patterns, including proper override behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/orm/src/client/executor/zenstack-query-executor.ts (1)
356-377: Consider removing the redundantas stringcast.All match branches either return
string(identifier names) or throw an error (returningnever). TypeScript should infer the return type asstringwithout the explicit cast. Ifts-patterntype inference requires it, consider adding a brief comment explaining why..otherwise((node) => { throw createInternalError(`Invalid query node: ${node}`); - }) as string; + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/executor/zenstack-query-executor.ts(11 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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:
packages/orm/src/client/executor/zenstack-query-executor.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} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/executor/zenstack-query-executor.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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/orm/src/client/executor/zenstack-query-executor.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:
packages/orm/src/client/executor/zenstack-query-executor.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/orm/src/client/executor/zenstack-query-executor.ts
🧬 Code graph analysis (1)
packages/orm/src/client/executor/zenstack-query-executor.ts (3)
packages/orm/src/client/executor/name-mapper.ts (1)
QueryNameMapper(48-719)packages/schema/src/schema.ts (1)
SchemaDef(11-19)packages/orm/src/client/contract.ts (1)
ClientContract(61-181)
⏰ 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: claude-review
🔇 Additional comments (6)
packages/orm/src/client/executor/zenstack-query-executor.ts (6)
27-42: LGTM!The import and type changes align well with the architectural shift from generic parameterization to concrete
SchemaDef-based types. TheMutationInfotype is appropriately simplified to use string-based model identifiers.
44-64: LGTM!The class declaration and constructor changes correctly remove the generic parameter while preserving the initialization logic. The
schemaHasMappedNamescheck continues to work correctly withclient.$schema.
66-75: LGTM!The method correctly inspects both
schema.modelsandschema.typeDefsfor mapped names, which is important since TypeDefs can be inherited by models and their mapped field names need processing. Based on learnings, this pattern is correct.
137-156: LGTM!The hook type and client cast changes are consistent with the non-generic architecture. The double cast (
as unknown as ClientContract<SchemaDef>) is a necessary workaround given thatClientImplis now non-generic whileClientContractstill expects a schema type parameter.
379-457: LGTM!The mutation hook methods (
callBeforeMutationHooksandcallAfterMutationHooks) are consistently updated to useSchemaDef-based types. The hook invocation logic and callback handling remain functionally correct.
459-475: LGTM!The
loadEntitiesmethod correctly accepts a string model name and uses it with Kysely'sselectFrom. The change from strongly-typedGetModels<Schema>tostringaligns with the broader architectural shift in this PR.
|
Claude finished @ymc9's task —— View job PR Review: Field Omission Implementation ✅Reviewing the field omission feature implementation across 30 files. Todo List:
Review SummaryThis is a well-implemented feature that adds field omission capabilities to ZenStack v3. The implementation is comprehensive, type-safe, and properly tested. ✅ Strengths1. Clean Architecture & Design
2. Implementation Quality
3. Type Safety
4. Test Coverage
🎯 Technical AnalysisPerformance Impact: ✅ Minimal
Security Considerations: ✅ Good
Backward Compatibility: ✅ Maintained
🔍 Minor Observations1. Test Typo (Line 54 in omit.test.ts): // Missing parentheses
expect(user.password).toBeUndefined; // ❌
expect(user.password).toBeUndefined(); // ✅2. Potential Enhancement: 📋 Code Quality Assessment
✅ Recommendation: APPROVEThis PR implements a valuable feature with:
The field omission feature adds significant value for protecting sensitive data while maintaining ZenStack's excellent type safety and developer experience. |
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @ymc9's task —— View job PR Review: Field Omission Implementation ✅Reviewing the field omission feature implementation across 30 files. Todo List:
Review SummaryThis is a well-implemented feature that adds field omission capabilities to ZenStack v3. The implementation is comprehensive, type-safe, and properly tested. ✅ Strengths1. Clean Architecture & Design
2. Implementation Quality
3. Type Safety
4. Test Coverage
🎯 Technical AnalysisPerformance Impact: ✅ Minimal
Security Considerations: ✅ Good
Backward Compatibility: ✅ Maintained
🔍 Minor Observations1. Test Typo (Line 54 in omit.test.ts): // Missing parentheses
expect(user.password).toBeUndefined; // ❌
expect(user.password).toBeUndefined(); // ✅2. Potential Enhancement: 📋 Code Quality Assessment
✅ Recommendation: APPROVEThis PR implements a valuable feature with:
The field omission feature adds significant value for protecting sensitive data while maintaining ZenStack's excellent type safety and developer experience. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/omit.test.ts (1)
67-79: Minor naming and assertion-strength nits in override testsTwo small optional tweaks:
- The names
'allows override at query options level'(Lines 67–79) vs'allows override at query level'(Lines 115–126) are quite similar; consider renaming to make the distinction between “client options override” and “per-query override” immediately clear.- Where you currently use
toBeTruthy()to assert fields are not omitted (e.g., Lines 78–79, 87–88, 90–91, 106–107, 109–110, 112–113, 125–126, 141–142), you could use more explicit matchers liketoBe('abc')ortoBe('Foo')if you want stricter guarantees about the returned values, not just their presence.Current code is fine; these are purely readability/strictness improvements.
Also applies to: 81-113, 115-126, 128-142
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
samples/next.js/package.json(1 hunks)tests/e2e/orm/client-api/omit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/omit.test.ts
🧠 Learnings (3)
📚 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:
samples/next.js/package.jsontests/e2e/orm/client-api/omit.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/omit.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/omit.test.ts
⏰ 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: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (2)
samples/next.js/package.json (1)
35-35: Aligns with monorepo version management.Updating TypeScript to use the catalog-based version ensures consistency with other dependencies in the workspace and reduces the risk of version mismatches across packages.
tests/e2e/orm/client-api/omit.test.ts (1)
5-143: Comprehensive E2E coverage of omit semantics across layersThis suite does a solid job exercising omit at schema, client options, and query levels across create/read/update/delete, nested includes, and delegate models, with
@ts-expect-errorensuring type-level omission as well. This aligns well with the guideline that E2E tests validate realistic schema usage patterns.
ymc9
changed the title
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.