refactor: avoid creating nested query when mapping names #211
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. WalkthroughUnifies synthesized JSON aliasing to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 PR Review: Refactor Name Mapper
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the name mapping logic to avoid creating nested queries when handling database table and column name mappings. The changes optimize query generation by processing table selections differently and expanding select-all operations inline rather than wrapping them in subqueries.
Key changes:
- Refactored name mapping logic to process table selections more efficiently without nested queries
- Updated select-all expansion to work inline with proper aliasing
- Fixed column alias references in SQL dialect implementations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/schema/expression.ts | Added utility method to extract literal values from expressions |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Enhanced to use name mapping for table and column names when creating database schema |
| packages/runtime/src/client/executor/name-mapper.ts | Major refactor of query name mapping logic to avoid nested queries |
| packages/runtime/src/client/crud/dialects/sqlite.ts | Updated column alias from '$j' to '$t' |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Updated column alias from '$j' to '$t' |
| packages/runtime/test/client-api/name-mapping.test.ts | Added comprehensive tests for count, aggregate, and groupBy operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job Claude finished @ymc9's task —— View job PR Review: Refactor Name Mapper
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
93-103: Base-model FK doesn’t respect mapped table/column namesThe delegate FK uses raw
modelDef.baseModelandbaseModelDef.idFieldswithout applyinggetTableName/getColumnName. This will break when the base model or its id fields are mapped.Apply mapping to both local and referenced sides:
if (modelDef.baseModel) { // create fk constraint const baseModelDef = requireModel(this.schema, modelDef.baseModel); - table = table.addForeignKeyConstraint( - `fk_${modelDef.baseModel}_delegate`, - baseModelDef.idFields, - modelDef.baseModel, - baseModelDef.idFields, - (cb) => cb.onDelete('cascade').onUpdate('cascade'), - ); + table = table.addForeignKeyConstraint( + `fk_${modelDef.baseModel}_delegate`, + // local columns on the concrete model + modelDef.idFields.map((f) => this.getColumnName(modelDef.fields[f]!)), + // referenced table and columns (base model) + this.getTableName(baseModelDef), + baseModelDef.idFields.map((f) => this.getColumnName(baseModelDef.fields[f]!)), + (cb) => cb.onDelete('cascade').onUpdate('cascade'), + ); }
🧹 Nitpick comments (5)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
145-149: Constraint names still use model names (not mapped) — confirm intentPK/unique constraint names continue to use
modelDef.name. If you want DB artifacts to reflect mapped names, consider switching togetTableName(modelDef)for naming consistency.Also applies to: 165-171
packages/runtime/src/client/executor/name-mapper.ts (4)
288-295: Pop scopes robustly even if nested code pushes/popsCurrent
forEach(() => pop())assumes no extra pushes/pops occurred insidefn. Restore to a saved length to be safe.- private withScopes<T>(scopes: Scope[], fn: (...args: unknown[]) => T): T { - scopes.forEach((s) => this.pushScope(s)); - try { - return fn(); - } finally { - scopes.forEach(() => this.scopes.pop()); - } - } + private withScopes<T>(scopes: Scope[], fn: (...args: unknown[]) => T): T { + const baseLen = this.scopes.length; + scopes.forEach((s) => this.pushScope(s)); + try { + return fn(); + } finally { + this.scopes.length = baseLen; + } + }
311-321: Support positional @map/@@Map and guard non-literal argsOnly reading a named
namearg misses common positional usage and non-literal values.- const mapAttr = def.attributes?.find((attr) => attr.name === '@@map' || attr.name === '@map'); - if (mapAttr) { - const nameArg = mapAttr.args?.find((arg) => arg.name === 'name'); - if (nameArg && nameArg.value.kind === 'literal') { - return nameArg.value.value as string; - } - } + const mapAttr = def.attributes?.find((a) => a.name === '@@map' || a.name === '@map'); + if (mapAttr) { + const arg = mapAttr.args?.find((a) => a.name === 'name') ?? mapAttr.args?.[0]; + const v = arg?.value; + if (v && v.kind === 'literal' && typeof v.value === 'string') { + return v.value as string; + } + } return undefined;If you have a helper to extract literal values, consider delegating to it here.
352-360: Avoid aliasing to the same name when no table mapping is neededAlways aliasing to the model name adds noise (
FROM users AS users). Only alias when the physical table name differs or an explicit alias is provided.- const modelName = innerNode.table.identifier.name; - const mappedName = this.mapTableName(modelName); - return { - node: this.wrapAlias(TableNode.create(mappedName), alias ?? modelName), - scope: { - alias: alias ?? modelName, + const modelName = innerNode.table.identifier.name; + const mappedName = this.mapTableName(modelName); + const effectiveAlias = alias ?? (mappedName !== modelName ? modelName : undefined); + return { + node: this.wrapAlias(TableNode.create(mappedName), effectiveAlias), + scope: { + alias: effectiveAlias, model: modelName, namesMapped: !this.hasMappedColumns(modelName), }, };
170-178: Nil-guard for Delete.fromMinor: if Kysely ever emits a Delete without
from(unlikely), this throws. Safe-guarding avoids surprises.- const scopes: Scope[] = node.from.froms.map((node) => { + const scopes: Scope[] = node.from?.froms?.map((node) => { // ... - }); + }) ?? [];Apply the same guard where
fromsis re-mapped below.
📜 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 (6)
packages/runtime/src/client/crud/dialects/postgresql.ts(4 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(9 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(5 hunks)packages/runtime/src/schema/expression.ts(1 hunks)packages/runtime/test/client-api/name-mapping.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/runtime/src/schema/expression.tspackages/runtime/src/client/crud/dialects/sqlite.tspackages/runtime/test/client-api/name-mapping.test.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/client/helpers/schema-db-pusher.tspackages/runtime/src/client/executor/name-mapper.ts
🧬 Code graph analysis (3)
packages/runtime/src/schema/expression.ts (1)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)
packages/runtime/src/client/helpers/schema-db-pusher.ts (3)
packages/sdk/src/schema/schema.ts (2)
ModelDef(20-36)FieldDef(59-74)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-116)packages/runtime/src/client/query-utils.ts (1)
requireModel(20-26)
packages/runtime/src/client/executor/name-mapper.ts (3)
packages/runtime/src/client/executor/kysely-utils.ts (1)
stripAlias(7-14)packages/runtime/src/client/query-utils.ts (2)
getModel(16-18)requireModel(20-26)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ 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: build-test (20.x)
🔇 Additional comments (12)
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
213-219: Confirm: legacy JSON alias$jfully replaced
Verified via RipGrep acrosspackages/runtime/src/clientthat there are no remaining references to$j, either as an identifier or as a string literal. Merging approved.packages/runtime/src/schema/expression.ts (1)
113-115: Helper is precise and side-effect free — LGTM
getLiteralValuecleanly narrows for literals and returnsundefinedotherwise.packages/runtime/src/client/crud/dialects/postgresql.ts (1)
63-64: PostgreSQL JSON alias consistency confirmed – no$jreferences remainAll JSON alias occurrences have been switched from “$j” to “$t” in selection, aggregation, and nested relation references. A repository‐wide search for any lingering “.$j” paths under
packages/runtime/src/clientreturned no matches, confirming that there are no stale references.• Snippet at
packages/runtime/src/client/crud/dialects/postgresql.ts:63–64remains as:return joinedQuery.select(`${parentAlias}$${relationField}.$t as ${relationField}`);• No occurrences of
\.\$j\bwere found in the client runtime code.packages/runtime/src/client/helpers/schema-db-pusher.ts (2)
75-79: Table creation now respects @@Map — LGTMUsing
getTableNameat creation time is the right hook for mapped model names.
273-281: FK creation now respects mapped names — LGTMGood application of
getTableName/getColumnNamefor FK sides.packages/runtime/test/client-api/name-mapping.test.ts (3)
239-276: Count tests cover ORM and Kysely paths — LGTMGood coverage including relation-filtered counts and type coercion for provider differences.
278-341: Aggregate tests validate both ORM and Kysely — LGTMSolid assertions across _count/_min/_max and filtered aggregates.
343-433: GroupBy tests are comprehensive — LGTMCovers by/having paths and cross-checks with Kysely groupBy.
packages/runtime/src/client/executor/name-mapper.ts (4)
202-235: Selection mapping logic is good; preserves original field namesExpanding
select *only when needed and aliasing mapped columns back to original names is correct. Nice.
109-138: Reference/table qualifier handling looks correctQualifier resolution respects aliases vs models and only maps the table name when the qualifier is a model. Good.
238-238: Ensure Node 20+ Support or Refactor Array ReverseIt looks like neither your root nor packages/runtime
package.jsondeclare anengines.nodefield, so it isn’t guaranteed you’re targeting Node 20 orlib.es2023. You should either:
- Add an explicit
engines.nodeentry (e.g."engines": { "node": ">=20" }) to all relevantpackage.jsonfiles before usingtoReversed().- Or replace
toReversed()with a manual reverse loop for broader compatibility.Locations to review:
package.json(project root)packages/runtime/package.jsonSuggested alternative:
- for (const scope of this.scopes.toReversed()) { + for (let i = this.scopes.length - 1; i >= 0; i--) { + const scope = this.scopes[i]!;
62-75: MapJOIN … USINGand guard against missingonThe current code only handles
join.on(unconditionally callingthis.transformNode(join.on)) and ignores thejoin.usingproperty. Kysely’s AST representsUSINGclauses with aUsingNode(a subtype ofOperationNode), and theJoinTypeenum includes"Using", so joins usingUSING (…)will havejoin.usingset andjoin.onundefined (kysely-org.github.io). Unconditionally transformingjoin.onwill crash forUSINGjoins, and theusingcolumns remain unmapped under name-mapped schemas.Please update the join transformation to:
- const joins = node.joins - ? node.joins.map((join, i) => ({ - ...join, - table: processedJoins[i]!.node, - on: this.transformNode(join.on), - })) - : undefined; + const joins = node.joins + ? node.joins.map((join, i) => ({ + ...join, + table: processedJoins[i]!.node, + // only transform ON if present + on: join.on ? this.transformNode(join.on) : undefined, + // transform USING columns when present + using: join.using ? this.transformNode(join.using) : undefined, + })) + : undefined;• Ensure that
usingcolumns are correctly mapped throughtransformNode, and guardjoin.onsince it may beundefinedforUSINGjoins.
• Add or extend a test building a query withJOIN … USING (mapped_col)and assert that the output SQL uses the mapped column name. I can draft the test if needed.
Summary by CodeRabbit