refactor: re-implement name mapper #203
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors name-mapping to a scope-based model across SELECT/FROM/JOIN handling, adds utilities for scoped resolution, and updates tests to a new client/test harness with generated schema/types for name mapping. Introduces a ZModel schema and related TypeScript types for tests, and tightens test utils around required workDir. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job Code Review CompleteI've conducted a comprehensive review of the "refactor: re-implement name mapper" PR. Here's my detailed analysis: Todo List:
Code Review Summary🎯 Overall AssessmentThis is a well-implemented refactor that significantly improves the name mapping functionality. The code quality is high with strong TypeScript usage and comprehensive test coverage. 🔍 Key Improvements
✅ StrengthsCode Quality
Performance Considerations
Security & Robustness
🧪 Test Coverage ExcellenceThe new test suite in
🚨 Areas for ImprovementMinor Issues
🔧 SuggestionsPerformance Optimization// Consider pre-computing reverse iteration to avoid repeated array creation
private *reverseScopes() {
for (let i = this.modelScopes.length - 1; i >= 0; i--) {
yield this.modelScopes[i];
}
}Error Handling EnhancementConsider adding more descriptive error messages: invariant(scope, `No scope found for field resolution: ${name}`);📊 Impact Assessment
🏆 Best Practices Followed
🎯 RecommendationAPPROVE - This is a high-quality refactor that improves code maintainability, performance, and test coverage. The minor suggestions above are optional optimizations rather than blocking issues. The refactor successfully addresses complex name mapping scenarios while maintaining clean, readable code with excellent test coverage. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
packages/runtime/test/utils.ts (3)
136-138: Make invariant explicit and avoid unconditional console noise
- Provide a message to
invariantso failures are actionable.- Consider gating the log behind a debug flag to keep CI output clean.
- invariant(workDir); - console.log(`Work directory: ${workDir}`); + invariant(workDir, 'createTestClient: workDir is required'); + if (process.env.DEBUG?.includes('zstack:test')) { + console.log(`Work directory: ${workDir}`); + }
146-153: Avoid pervasive non-null assertions by narrowingworkDironceThe repeated
workDir!is a smell and easy to regress. NarrowworkDirwith a typed assertion helper so the compiler knows it's a string afterward.Apply the local changes and helper:
- const r = await loadDocument(path.resolve(workDir!, 'schema.zmodel')); + assertWorkDir(workDir); + const r = await loadDocument(path.resolve(workDir, 'schema.zmodel')); @@ - fs.writeFileSync(path.resolve(workDir!, 'schema.prisma'), prismaSchemaText); + fs.writeFileSync(path.resolve(workDir, 'schema.prisma'), prismaSchemaText);Add this helper near the top of the file (outside the shown hunk):
function assertWorkDir(dir: string | undefined): asserts dir is string { invariant(dir, 'createTestClient: workDir is required'); }If you prefer not to add a helper, you can also refine the type with:
invariant(workDir, 'createTestClient: workDir is required'); const wd = workDir; // wd: string
176-178: Make SQLite test DB ephemeral to reduce cross-test interferenceUsing a stable filename under the workDir can leak state across tests if cleanup fails. Prefer an in-memory DB for speed or a unique temp file.
- _options.dialect = new SqliteDialect({ - database: new SQLite(path.join(workDir!, dbName)), - }); + const sqlitePath = + dbName === ':memory:' ? ':memory:' : path.join(workDir!, dbName.replace(/^\.?\//, '')); + _options.dialect = new SqliteDialect({ database: new SQLite(sqlitePath) });Optionally set the default
dbNameto a randomized file inworkDiror to':memory:'for tests that don’t require Prisma CLI.packages/runtime/test/schemas/name-mapping/schema.zmodel (1)
1-4: Keep the committed datasource benign; rely on test utils to rewriteCommitting
url = "file:./dev.db"is fine sincecreateTestClientrewrites the datasource, but it can create stray files if someone runs generators directly. Consider a placeholder URL (e.g.,file:./.ignored.db) to discourage accidental use.packages/runtime/src/client/executor/name-mapper.ts (3)
81-86: Prefer ReturningNode.create for consistencyReturning a plain object works if the shape matches, but using the node factory keeps parity with the rest of the transformer and reduces risk if Kysely evolves the shape.
- protected override transformReturning(node: ReturningNode) { - return { - kind: node.kind, - selections: this.processSelections(node.selections), - }; - } + protected override transformReturning(node: ReturningNode) { + return ReturningNode.create(this.processSelections(node.selections)); + }
347-354: Only introduce aliases when mapping changes
processSelectionaliases every non-aliased selection to the field name, even if no mapping occurs. This can clutter SQL and shadow upstream aliases. Consider aliasing only when the mapped name differs.One approach:
- Detect whether
super.transformNode(node)changed the column name- Apply
wrapAliasonly in that caseIf that’s noisy to implement here, at least skip adding an alias when
extractFieldName(node)is undefined.
307-324: Use node factories consistently in createSelectAllThe method mixes raw objects and factories. Prefer factories where available for uniformity and future-proofing.
- return { - kind: 'SelectQueryNode', - from: FromNode.create([TableNode.create(tableName)]), - selections: this.getModelFields(modelDef).map((fieldDef) => { + return { + kind: 'SelectQueryNode', + from: FromNode.create([TableNode.create(tableName)]), + selections: this.getModelFields(modelDef).map((fieldDef) => { const columnName = this.mapFieldName(model, fieldDef.name); const columnRef = ReferenceNode.create(ColumnNode.create(columnName), TableNode.create(tableName)); if (columnName !== fieldDef.name) { const aliased = AliasNode.create(columnRef, IdentifierNode.create(fieldDef.name)); return SelectionNode.create(aliased); } else { return SelectionNode.create(columnRef); } }), } as SelectQueryNode;packages/runtime/test/schemas/name-mapping/schema.ts (1)
8-89: Generated schema matches the zmodel and is coherent for tests
- Mappings and relations mirror the source zmodel.
- Using
as const satisfies SchemaDefis great for type safety.Minor note: the import path
../../../dist/schemaassumes a prior build; fine for CI but keep in mind for local test runs.packages/runtime/test/client-api/name-mapping.test.ts (3)
210-212: Avoid hard-coded IDs during create in delete testSame concern: don’t pin IDs in fixtures. Let the DB assign, since you’re deleting by title anyway.
Apply this diff:
- id: 1, title: 'Post1',
119-126: Strengthen whereRef semantics (current filter is tautological)Using whereRef('email', '=', 'email') is always true and doesn’t prove name mapping across scopes. Prefer a cross-scope ref comparison or an alias to validate the mapper.
Example refactor:
await expect( db.$qb .selectFrom('User as u1') .innerJoin('User as u2', 'u2.id', 'u1.id') .select(['u1.email']) .whereRef('u1.email', '=', 'u2.email') .orderBy(['u1.email']) .executeTakeFirst() ).resolves.toMatchObject({ email: '[email protected]' });
190-197: Cross-dialect function usage: verify concat translation on SQLiteIf the zmodel uses SQLite, eb.fn('concat', ...) may not exist natively (SQLite prefers ||). Confirm the executor maps concat correctly; otherwise consider a builder construct that’s translated per dialect.
Possible alternative if supported:
set({ email: (eb) => eb.ref('email').concat(eb.val('_updated')) })- or a raw expression mapped by dialect.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/runtime/src/client/executor/name-mapper.ts(4 hunks)packages/runtime/test/client-api/name-mapping.test.ts(1 hunks)packages/runtime/test/schemas/name-mapping/input.ts(1 hunks)packages/runtime/test/schemas/name-mapping/models.ts(1 hunks)packages/runtime/test/schemas/name-mapping/schema.ts(1 hunks)packages/runtime/test/schemas/name-mapping/schema.zmodel(1 hunks)packages/runtime/test/utils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Schema-first approach with ZModel DSL extension of Prisma schema language
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/runtime/test/schemas/name-mapping/models.tspackages/runtime/test/schemas/name-mapping/schema.ts
🔇 Additional comments (5)
packages/runtime/test/schemas/name-mapping/schema.zmodel (1)
6-19: Schema mappings look correct and minimal
@@map('users')and@map('user_email')for User.@@map('posts'),@map('post_title'),@map('author_id')for Post.- Relations are well-formed for the name-mapping tests.
packages/runtime/src/client/executor/name-mapper.ts (1)
356-369: I’ve added scripts to inspect theprocessSelectAllimplementation and find theSelectAllNodedefinition. Once we have those details, we can confirm whether qualified stars are being correctly handled or if adjustments/tests are needed.packages/runtime/test/schemas/name-mapping/models.ts (1)
1-12: Auto-generated types look correctType aliases align with the generated
SchemaTypeandModelResultutility; nothing to change.packages/runtime/test/client-api/name-mapping.test.ts (1)
10-16: Tests are already isolated bycreateTestClient—no further DB reset neededThe
createTestClientutility always starts from a clean database state:
- When
usePrismaPushis false (the default), it instantiates a brand-new in-memory SQLite database for each call viawhich guarantees a fresh, empty DB per test instance.dialect: new SqliteDialect({ database: new SQLite(':memory:') }), await client.$pushSchema();- When
usePrismaPushis true, it creates a fresh temporaryworkDiron each invocation, writes out aschema.prisma, and runsThenpx prisma db push --schema ./schema.prisma --skip-generate --force-reset--force-resetflag drops any existing DB schema before pushing, and since each call gets its own temp directory, there is no shared file or state between tests.As a result, repeated emails or fixed IDs will not collide across tests because the database is reset (or re-initialized) on every
beforeEach. No changes are needed here.packages/runtime/test/schemas/name-mapping/input.ts (1)
1-51: Generated model arg/payload aliases look consistent and completeTypes line up with the schema-centric approach and cover the full CRUD/aggregate surface for User and Post. No changes requested.
|
Claude finished @ymc9's task —— View job Code Review CompleteI've conducted a comprehensive review of the "refactor: re-implement name mapper" PR. Here's my detailed analysis: Todo List:
🎯 Overall AssessmentThis is a well-implemented refactor that significantly improves the name mapping functionality. The code quality is high with strong TypeScript usage and comprehensive test coverage. 🔍 Key Improvements
✅ StrengthsCode Quality
Performance Considerations
Security & Robustness
🧪 Test Coverage ExcellenceThe new test suite in
🚨 Areas for ImprovementMinor Issues
Performance Optimization// Consider pre-computing reverse iteration to avoid repeated array creation
private *reverseScopes() {
for (let i = this.modelScopes.length - 1; i >= 0; i--) {
yield this.modelScopes[i];
}
}🔧 Suggested FixesCritical Bug Fix (
|
|
Claude finished @ymc9's task —— View job Code Review CompleteI've conducted a comprehensive review of the "refactor: re-implement name mapper" PR. Here's my detailed analysis: Todo List:
Code Review Summary🎯 Overall AssessmentThis is a well-implemented refactor that significantly improves the name mapping functionality. The code quality is high with strong TypeScript usage and comprehensive test coverage. 🔍 Key Improvements
✅ StrengthsCode Quality
Performance Considerations
Security & Robustness
🧪 Test Coverage ExcellenceThe new test suite in
🚨 Areas for ImprovementCritical Issues
Minor Issues
🔧 Suggested FixesCritical Bug Fix (
|
Summary by CodeRabbit