fix(orm): update to kysely 0.28.x and simplify typing #380
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 20 minutes and 23 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 (4)
WalkthroughStandardizes internal Kysely typing with an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 Code Review for Kysely 0.28.x Update ✅Todo List
Review SummaryThis is a well-executed dependency update that successfully migrates ZenStack's ORM to Kysely 0.28.x while simplifying TypeScript typing. The changes are clean, consistent, and maintain backward compatibility. ✅ Code Quality & Best PracticesExcellent Refactoring:
Strong Architecture:
🔒 Security ConsiderationsNo Security Issues Identified:
⚡ Performance ConsiderationsPerformance Neutral/Positive:
🧪 Test CoverageComprehensive Testing:
🔄 Breaking Changes AssessmentNo Breaking Changes for End Users:
Internal Changes Only:
📋 Specific Code Review Notes
✅ Recommendations
Overall Assessment: Excellent ⭐⭐⭐⭐⭐This PR demonstrates best practices for dependency updates:
Recommendation: Approve and merge - This update will improve developer experience with better type ergonomics while leveraging Kysely 0.28.x improvements. |
|
fixes #345 |
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the kysely dependency from version 0.27.6 to 0.28.8 and adapts the codebase to handle breaking changes introduced in the new version.
- Updates kysely from
^0.27.6to~0.28.8 - Adapts QueryId handling to match kysely 0.28.x API changes (QueryId is now an object type)
- Relaxes internal type constraints to use
stringinstead ofGetModels<Schema>for improved flexibility
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Updates kysely version to ~0.28.8 and @types/node to ^20.19.0 in catalog |
| pnpm-lock.yaml | Reflects dependency version updates including kysely 0.28.8 resolution |
| samples/next.js/package.json | Changes @types/node to use catalog reference instead of fixed version |
| packages/orm/src/client/plugin.ts | Updates queryId type from string to QueryId (kysely type) |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Adapts to kysely 0.28.x QueryId API changes |
| packages/orm/src/client/crud/operations/base.ts | Updates to use createQueryId() from kysely and removes custom QueryId implementation |
| packages/orm/src/client/crud/operations/group-by.ts | Adds explicit string cast for model parameter |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Changes model parameter type from GetModels to string |
| packages/orm/src/client/contract.ts | Updates $qbRaw type to use AnyKysely |
| packages/orm/src/client/client-impl.ts | Updates Kysely type references to AnyKysely |
| packages/orm/src/utils/kysely-utils.ts | Adds new visitor methods for kysely 0.28.x nodes and AnyKysely type alias |
| packages/plugins/policy/src/policy-handler.ts | Changes model parameters from GetModels to string throughout |
| packages/plugins/policy/src/expression-transformer.ts | Updates ExpressionTransformerContext to use string for model field |
| tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts | Adapts test to access queryId.queryId property due to QueryId type change |
| tests/regression/test/v2-migrated/issue-1698.test.ts | Removes debug console.log statements and unused variable assignments |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review for Kysely 0.28.x Update ✅ Todo List
Review SummaryThis is a well-executed dependency update that successfully migrates ZenStack's ORM to Kysely 0.28.x while simplifying TypeScript typing. The changes are clean, consistent, and maintain backward compatibility. ✅ Code Quality & Best PracticesExcellent Refactoring:
Strong Architecture:
🔒 Security ConsiderationsNo Security Issues Identified:
⚡ Performance ConsiderationsPerformance Neutral/Positive:
🧪 Test CoverageComprehensive Testing:
🔄 Breaking Changes AssessmentNo Breaking Changes for End Users:
Internal Changes Only:
📋 Specific Code Review Notes
✅ Recommendations
Overall Assessment: Excellent ⭐⭐⭐⭐⭐This PR demonstrates best practices for dependency updates:
Recommendation: Approve and merge - This update will improve developer experience with better type ergonomics while leveraging Kysely 0.28.x improvements. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
2101-2116: Type mismatch in executeQuery helper methods.The
executeQuery,executeQueryTakeFirst, andexecuteQueryTakeFirstOrThrowmethods still acceptToKysely<Schema>parameters, but they are called throughout the file withAnyKyselyarguments (e.g., lines 180, 361, 765, 1063, 1296, 1301, 1546, 1568, 1707, 1786, 1808, 1984).While this works due to TypeScript's structural typing, it's inconsistent with the refactoring pattern applied to other methods.
Apply this diff to align with the refactoring pattern:
- protected executeQuery(kysely: ToKysely<Schema>, query: Compilable, _operation: string) { + protected executeQuery(kysely: AnyKysely, query: Compilable, _operation: string) { return kysely.executeQuery(query.compile()); } - protected async executeQueryTakeFirst(kysely: ToKysely<Schema>, query: Compilable, _operation: string) { + protected async executeQueryTakeFirst(kysely: AnyKysely, query: Compilable, _operation: string) { const result = await kysely.executeQuery(query.compile()); return result.rows[0]; } - protected async executeQueryTakeFirstOrThrow(kysely: ToKysely<Schema>, query: Compilable, _operation: string) { + protected async executeQueryTakeFirstOrThrow(kysely: AnyKysely, query: Compilable, _operation: string) { const result = await kysely.executeQuery(query.compile()); if (result.rows.length === 0) { throw new ORMError(ORMErrorReason.NOT_FOUND, 'No rows found'); } return result.rows[0]; }
🧹 Nitpick comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
138-146: Inconsistent refactoring: Mix of old and new type signatures.Many methods throughout the file still use
ToKysely<Schema>andGetModels<Schema>while others have been updated toAnyKyselyandstring. Examples include:
- Not updated:
exists(line 138),processBaseModelCreate(line 393),processOwnedRelationForCreate(line 526),createMany(line 660),processBaseModelCreateMany(line 775),connectOrCreateRelation(line 1580),deleteRelation(line 1820),processDelegateRelationDelete(line 1987)- Updated:
read(line 149),readUnique(line 189),create(line 248),update(line 897),updateMany(line 1203),delete(line 1921)While TypeScript's structural typing allows this to work (since
AnyKyselyis compatible withToKysely<Schema>), the mixed signatures create maintenance confusion and suggest incomplete refactoring.Consider completing the refactor by updating all remaining methods to use consistent typing:
// Example for exists method protected async exists( kysely: AnyKysely, // instead of ToKysely<Schema> model: string, // instead of GetModels<Schema> filter: any, ): Promise<unknown | undefined>Also applies to: 393-424, 526-590, 660-772, 774-816, 1579-1598, 1819-1912, 1987-2013
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/crud/operations/base.ts(33 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/orm/src/client/crud/operations/base.ts (6)
packages/orm/src/utils/kysely-utils.ts (1)
AnyKysely(412-412)packages/orm/src/client/crud-types.ts (1)
FindArgs(648-664)packages/schema/src/schema.ts (2)
GetModels(110-110)FieldDef(61-76)packages/orm/src/client/contract.ts (2)
CRUD(225-225)CRUD(235-235)packages/orm/src/client/query-utils.ts (2)
requireIdFields(70-77)flattenCompoundUniqueFilters(266-308)packages/common-helpers/src/enumerable.ts (1)
enumerate(9-17)
⏰ 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: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (6)
packages/orm/src/client/crud/operations/base.ts (6)
3-20: LGTM: Import changes align with Kysely 0.28.x update.The addition of
createQueryIdandAnyKyselytype imports are appropriate for the Kysely 0.28.x migration.
106-108: LGTM: Getter simplified to use AnyKysely.This change aligns with the PR objective to simplify typing by using
AnyKyselyinstead of the parameterizedToKysely<Schema>.
148-192: LGTM: Read methods updated correctly for Kysely 0.28.x.The query compilation now properly uses
createQueryId()from Kysely, and the type signatures have been generalized toAnyKyselyandstringas intended.
2057-2067: LGTM: Transaction callback updated to use AnyKysely.The
safeTransactionmethod's callback signature has been correctly updated to acceptAnyKysely, maintaining consistency with the broader refactoring.
1194-1197: LGTM: Context comment signature updated consistently.The
_contextparameter now usesstringfor the model field, aligning with the string-based model identifier pattern throughout the refactor.
76-85: Type safety loss is mitigated by runtime validation.While removing the generic parameter does eliminate compile-time model name validation, all usages of
fromRelation.modelare passed through schema helper functions (requireField(),getManyToManyRelation(),getRelationForeignKeyFieldPairs()) that validate the model exists at runtime. Additionally, themodelparameter is validated at the entry point viarequireModel()in thecreate()method. Invalid model names would fail fast at runtime. This aligns with existing unsafe casts (as GetModels<Schema>) already in the codebase, suggesting this type/runtime validation tradeoff is intentional.
|
Claude finished @ymc9's task —— View job Code Review for Kysely 0.28.x Update ✅ Todo List
Review SummaryThis is a well-executed dependency update that successfully migrates ZenStack's ORM to Kysely 0.28.x while simplifying TypeScript typing. The changes are clean, consistent, and maintain backward compatibility. ✅ Code Quality & Best PracticesExcellent Refactoring:
Strong Architecture:
🔒 Security ConsiderationsNo Security Issues Identified:
⚡ Performance ConsiderationsPerformance Neutral/Positive:
🧪 Test CoverageComprehensive Testing:
🔄 Breaking Changes AssessmentNo Breaking Changes for End Users:
Internal Changes Only:
📋 Specific Code Review Notes
✅ Recommendations
Overall Assessment: Excellent ⭐⭐⭐⭐⭐This PR demonstrates best practices for dependency updates:
Recommendation: Approve and merge - This update will improve developer experience with better type ergonomics while leveraging Kysely 0.28.x improvements. |
|
Claude finished @ymc9's task —— View job Code Review for Kysely 0.28.x Update ✅ Todo List
Review SummaryThis is a well-executed dependency update that successfully migrates ZenStack's ORM to Kysely 0.28.x while simplifying TypeScript typing. The changes are clean, consistent, and maintain backward compatibility. ✅ Code Quality & Best PracticesExcellent Refactoring:
Strong Architecture:
🔒 Security ConsiderationsNo Security Issues Identified:
⚡ Performance ConsiderationsPerformance Neutral/Positive:
🧪 Test CoverageComprehensive Testing:
🔄 Breaking Changes AssessmentNo Breaking Changes for End Users:
Internal Changes Only:
📋 Specific Code Review Notes
✅ Recommendations
Overall Assessment: Excellent ⭐⭐⭐⭐⭐This PR demonstrates best practices for dependency updates:
Recommendation: Approve and merge - This update will improve developer experience with better type ergonomics while leveraging Kysely 0.28.x improvements. |
Summary by CodeRabbit
Refactor
New Features
Chores
Tests