fix(orm): typing issue when auth type is a typedef #444
Conversation
WalkthroughThis PR expands the ORM's TypeScript typings to support type-definition (TypeDef) based auth types, adds conditional utility types (PartialIf, ArrayIf), refines TypeDefResult with a Partial flag, introduces TypeDefFieldIsArray, and adds E2E tests and generated test schema for auth-type validation. (49 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: Auth Type Typedef FixI've thoroughly analyzed this PR that fixes typing issues when auth type is a typedef. Here's my comprehensive review: Todo List
📋 SummaryThis PR addresses a typing issue in ZenStack's ORM package where auth types defined as typedefs weren't properly supported. The fix adds support for typedef-based auth types alongside the existing model-based auth types. ✅ Excellent Work - Code Quality & ImplementationStrong Points:
Code Quality Highlights:
🔧 Technical Implementation ReviewCore Changes Analysis:
Type System Correctness:
🚫 No Issues Found✅ Security: No security concerns - purely type-level changes 📊 Test Coverage AssessmentExcellent test coverage:
🎯 RecommendationsThis PR is ready for merge. The implementation is:
Minor suggestion for future enhancement: Overall Assessment: ⭐⭐⭐⭐⭐ Excellent This is a high-quality fix that properly addresses the auth typedef typing issue with clean implementation and comprehensive test coverage. The solution is well-architected and ready for production use. |
![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
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/auth-typing.test.ts (1)
6-17: Missing requirednamefield in auth object.The
AuthTypeDef inschema.zmodelhas a requiredname: Stringfield, but this test omits it. SinceAuthTypeusesTypeDefResult<Schema, 'Auth', true>withPartial=true, all fields become optional for the auth context.This is likely intentional design (auth context can be partial), but consider adding a comment explaining that auth objects are intentionally partial, or add a second test case that includes all fields to validate the complete type works as well.
describe('Custom auth typing tests', () => { it('works with custom auth typing', async () => { const db = await createTestClient(schema); + // Auth context accepts partial data - not all fields required db.$setAuth({ id: 1, role: 'ADMIN', permissions: [ { actionCode: 'MANAGE', }, ], }); }); + + it('works with complete auth object', async () => { + const db = await createTestClient(schema); + db.$setAuth({ + id: 1, + name: 'Admin User', + role: 'ADMIN', + permissions: [{ actionCode: 'MANAGE' }], + }); + }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/orm/src/client/contract.ts(3 hunks)packages/orm/src/client/crud-types.ts(2 hunks)packages/orm/src/utils/type-utils.ts(1 hunks)packages/schema/src/schema.ts(1 hunks)tests/e2e/orm/client-api/auth-typing.test.ts(1 hunks)tests/e2e/orm/schemas/auth-type/input.ts(1 hunks)tests/e2e/orm/schemas/auth-type/models.ts(1 hunks)tests/e2e/orm/schemas/auth-type/schema.ts(1 hunks)tests/e2e/orm/schemas/auth-type/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/client-api/auth-typing.test.tstests/e2e/orm/schemas/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/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/auth-type/schema.zmodel
🧠 Learnings (8)
📓 Common learnings
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.540Z
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.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
📚 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/client-api/auth-typing.test.tstests/e2e/orm/schemas/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/input.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 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/auth-typing.test.tstests/e2e/orm/schemas/auth-type/models.tspackages/orm/src/client/contract.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/input.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 tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/auth-typing.test.tstests/e2e/orm/schemas/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.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/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.zmodeltests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/input.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 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/schemas/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/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/auth-type/models.tspackages/schema/src/schema.tspackages/orm/src/client/contract.tstests/e2e/orm/schemas/auth-type/schema.tspackages/orm/src/client/crud-types.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 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:
tests/e2e/orm/schemas/auth-type/models.tstests/e2e/orm/schemas/auth-type/schema.tstests/e2e/orm/schemas/auth-type/input.ts
🧬 Code graph analysis (2)
tests/e2e/orm/client-api/auth-typing.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(99-248)tests/e2e/orm/schemas/auth-type/schema.ts (1)
schema(73-73)
tests/e2e/orm/schemas/auth-type/schema.ts (1)
packages/schema/src/schema.ts (1)
SchemaDef(11-19)
⏰ 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 (8)
packages/orm/src/utils/type-utils.ts (1)
5-9: LGTM!The new conditional type utilities
PartialIfandArrayIffollow the established patterns in this file (similar toNullableIfon line 7) and provide clean, reusable type transformations for the TypeDef handling.tests/e2e/orm/schemas/auth-type/models.ts (1)
1-12: LGTM!Auto-generated file correctly exports the model and TypeDef result types. The
TypeDefResultusage forPermissionandAuthaligns with the PR's objective of supporting TypeDef-based auth types.packages/schema/src/schema.ts (1)
244-249: LGTM!The
TypeDefFieldIsArraytype follows the established pattern ofTypeDefFieldIsOptionaland correctly extracts the array status from TypeDef field definitions.tests/e2e/orm/schemas/auth-type/schema.ts (1)
1-74: LGTM!Auto-generated schema correctly represents the ZModel definitions. Key elements:
authType: "Auth"enables TypeDef-based auth typingpermissionsfield properly marked witharray: true- Schema satisfies
SchemaDefconstraintpackages/orm/src/client/crud-types.ts (2)
252-280: LGTM! Well-structured recursive TypeDef handling.The updated
TypeDefResultcorrectly:
- Adds a
Partialgeneric that propagates through nested TypeDefs- Uses
ArrayIfto wrap nested TypeDef results when the field is an array- When
Partial=true, all fields become optional viaPartialIf, and the optional keys becomenever(avoiding double-optionality)- When
Partial=false, maintains existing optionality behaviorThe recursive call on line 265 properly passes the
Partialflag to nested TypeDefs.
32-48: Imports correctly added.New imports for
TypeDefFieldIsArray,ArrayIf, andPartialIfare properly added to support the enhancedTypeDefResulttype.tests/e2e/orm/schemas/auth-type/schema.zmodel (1)
10-17:@idon TypeDef fields is valid and intentional in ZenStack.After verification, the Auth TypeDef with
@idis correct. This is an established pattern in ZenStack schemas—the CommonFields type used throughout the codebase also defines@id @default(cuid())on a TypeDef. The@@authattribute marks Auth as the authentication context type, and the@idfield is valid metadata for the auth schema structure.Likely an incorrect or invalid review comment.
tests/e2e/orm/schemas/auth-type/input.ts (1)
8-30: Generated Foo model aliases correctly mirror ORM CRUD genericsThe Foo-specific argument and payload aliases are wired cleanly to the generic
@zenstackhq/ormtypes ($Find*Args,$Create*Args,$Update*Args,$Delete*Args,$WhereInput,$SelectInput,$IncludeInput,$OmitInput,SelectIncludeOmit,SimplifiedModelResult).FooGetPayload’s generic constraints and defaultOptionsmatch the expected pattern and should give solid type inference for the e2e auth-type tests.
There was a problem hiding this comment.
Pull request overview
This PR fixes a TypeScript typing issue that occurred when using a typedef (custom type) as the authentication type with nested typedef fields that are arrays. The fix enhances the type system to properly handle recursive typedef resolution and array typing in auth contexts.
Key changes:
- Added support for recursive typedef field resolution with proper array and partial handling
- Extended auth type support from models-only to both models and typedefs
- Added utility types (
PartialIf,ArrayIf,TypeDefFieldIsArray) following existing patterns
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/schemas/auth-type/schema.zmodel | New test schema defining Auth typedef with nested Permission[] typedef field |
| tests/e2e/orm/schemas/auth-type/schema.ts | Auto-generated schema file with typedef metadata including array flags |
| tests/e2e/orm/schemas/auth-type/models.ts | Auto-generated type exports for typedefs using TypeDefResult |
| tests/e2e/orm/schemas/auth-type/input.ts | Auto-generated input types for the test model |
| tests/e2e/orm/client-api/auth-typing.test.ts | Typing test validating $setAuth works with typedef auth containing arrays |
| packages/schema/src/schema.ts | Added TypeDefFieldIsArray helper to check if typedef fields are arrays |
| packages/orm/src/utils/type-utils.ts | Added PartialIf and ArrayIf utility types for conditional type transformations |
| packages/orm/src/client/crud-types.ts | Enhanced TypeDefResult to handle recursive typedefs, arrays, and partial mode |
| packages/orm/src/client/contract.ts | Updated AuthType to support typedef-based auth with proper partial typing |
💡 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.
|
Claude finished @ymc9's task —— View job PR Review: Auth Type Typedef FixI've thoroughly analyzed this PR that fixes typing issues when auth type is a typedef. Here's my comprehensive review: Todo List
📋 SummaryThis PR addresses a typing issue in ZenStack's ORM package where auth types defined as typedefs weren't properly supported. The fix adds support for typedef-based auth types alongside the existing model-based auth types. ✅ Excellent Work - Code Quality & ImplementationStrong Points:
Code Quality Highlights:
🔧 Technical Implementation ReviewCore Changes Analysis:
Type System Correctness:
🚫 No Issues Found✅ Security: No security concerns - purely type-level changes 📊 Test Coverage AssessmentExcellent test coverage:
🎯 RecommendationsThis PR is ready for merge. The implementation is:
Minor suggestion for future enhancement: Overall Assessment: ⭐⭐⭐⭐⭐ Excellent This is a high-quality fix that properly addresses the auth typedef typing issue with clean implementation and comprehensive test coverage. The solution is well-architected and ready for production use. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/orm/src/client/crud-types.ts (1)
635-647: MapFieldDefType generalization looks correct; minor redundancy possibleThe generalized
MapFieldDefTypecorrectly:
- Handles enums via
GetEnums<Schema>.- Expands typedef-typed fields through
TypeDefResult<Schema, T['type'], Partial>.- Propagates
Partialfor nested typedefs and still defers nullability/array wrapping toWrapType.One tiny cleanup: since
TypeDefResultalready ends with& Record<string, unknown>, the extra& Record<string, unknown>in the typedef branch is redundant and could be dropped to simplify printed types, if you care about readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/orm/src/client/crud-types.ts(3 hunks)packages/schema/src/schema.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
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.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
📚 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/schema/src/schema.tspackages/orm/src/client/crud-types.ts
🧬 Code graph analysis (1)
packages/orm/src/client/crud-types.ts (2)
packages/schema/src/schema.ts (5)
SchemaDef(11-19)GetTypeDefs(142-142)GetTypeDefFields(172-175)GetTypeDefField(177-181)TypeDefFieldIsOptional(238-242)packages/orm/src/utils/type-utils.ts (3)
PartialIf(5-5)Optional(3-3)WrapType(25-31)
⏰ 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, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/schema/src/schema.ts (2)
18-18: Auth type now correctly accepts both model and typedef namesExtending
authTypetoGetModels<SchemaDef> | GetTypeDefs<SchemaDef>aligns the schema contract with typedef‑based auth without changing runtime behavior; looks good.
244-248: TypeDefFieldIsArray helper is consistent and reusableThis mirrors
TypeDefFieldIsOptionaland cleanly exposes thearrayflag for typedef fields; good addition for downstream conditional typing.packages/orm/src/client/crud-types.ts (1)
250-274: Partial-aware TypeDefResult preserves existing semanticsThe new
TypeDefResult<..., Partial>keeps the old behavior whenPartialisfalseand cleanly switches to “all fields optional” (including nested typedefs, viaMapFieldDefType) whenPartialistrue. The use ofOptionalfor schema‑declared optional fields plusPartialIffor the mode flag is sound and backwards‑compatible.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.