fix(orm): use compact alias names when transforming ORM queries to Kysely#2406
fix(orm): use compact alias names when transforming ORM queries to Kysely#2406
Conversation
|
Claude finished @ymc9's task —— View job PR Review: Compact Alias Names for Kysely TransformationReviewing PR #2406 which fixes issue #2378 by implementing compact alias names when transforming ORM queries to Kysely. Review Checklist:
Review Feedback✅ Overall Assessment: LOOKS GOODThis is a well-implemented solution to a PostgreSQL identifier length limitation issue. The approach is clean, efficient, and properly tested. Problem AnalysisIssue #2378 occurs when deep nested includes generate alias names exceeding PostgreSQL's 63-byte identifier limit. The test case demonstrates a 5-level deep include: Previously, aliases like Implementation Review🔧 Core Solution: TempAliasTransformer (
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a temporary-alias system: adds TEMP_ALIAS_PREFIX and tmpAlias(), wraps generated SQL aliases across dialects, adds TempAliasTransformer to compact prefixed aliases before execution, adds ClientOptions.useCompactAliasNames flag, updates several internal aliases and exists-result naming, and adds a regression test for deep nested includes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 (2)
packages/orm/src/client/executor/zenstack-query-executor.ts (2)
635-640: Creating a newTempAliasTransformerinstance per query is correct — document the concurrency rationale.The
new TempAliasTransformer()per call avoids concurrent queries on the same executor racing over a sharedaliasMap. Given thatrun()already clears the map (making the class reuse-safe), a future reader might be tempted to hoist this to an instance field as an "optimization". A brief inline comment would prevent that regression.💬 Suggested comment
private processTempAlias<Node extends RootOperationNode>(query: Node): Node { if (this.options.useCompactAliasNames === false) { return query; } - return new TempAliasTransformer().run(query); + // A new instance is created per call intentionally: TempAliasTransformer holds + // per-query state (aliasMap) and a shared instance would race under concurrent queries. + return new TempAliasTransformer().run(query); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/executor/zenstack-query-executor.ts` around lines 635 - 640, Add an inline comment in processTempAlias explaining why we instantiate a new TempAliasTransformer per call: this avoids concurrent queries sharing a single transformer/aliasMap and racing; although TempAliasTransformer.run clears its map (making reuse technically safe), do not hoist it to an executor field as an "optimization" because that would reintroduce potential concurrency bugs—refer to processTempAlias and TempAliasTransformer.run/aliasMap in the comment so future readers understand the rationale.
635-640:new TempAliasTransformer()per call is the correct concurrency choice — document it.A future reader might hoist this to a private instance field as a "trivial" optimization (since
run()resets state). That would introduce a data race under concurrent queries sharing the same executor. A brief comment prevents the regression.💬 Suggested inline comment
private processTempAlias<Node extends RootOperationNode>(query: Node): Node { if (this.options.useCompactAliasNames === false) { return query; } - return new TempAliasTransformer().run(query); + // New instance per call: TempAliasTransformer holds per-query mutable state (aliasMap). + // Do NOT hoist to a shared field — concurrent queries on the same executor would race. + return new TempAliasTransformer().run(query); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/executor/zenstack-query-executor.ts` around lines 635 - 640, Add a short inline comment in processTempAlias explaining why we instantiate a new TempAliasTransformer for each call (instead of hoisting it to a private field): TempAliasTransformer.run() holds/resettable internal state so reusing one instance across concurrent queries would cause a data race; therefore keep "new TempAliasTransformer().run(query)" when options.useCompactAliasNames is true to preserve thread-safety/concurrency. Reference processTempAlias, TempAliasTransformer, run, and options.useCompactAliasNames in the comment so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/executor/temp-alias-transformer.ts`:
- Around line 16-26: The compact alias generator in transformIdentifier uses
plain `$tN` which can collide with user/dialect aliases; update the mapping so
mapped names use a more distinctive, collision-resistant prefix (e.g. `$$t` or
`_zt`) instead of `$t` and centralize that prefix as a constant (referencing
TEMP_ALIAS_PREFIX, aliasMap, transformIdentifier, and IdentifierNode.create) so
creation of mapped = `<NEW_PREFIX>${this.aliasMap.size + 1}` replaces the
current `$t` scheme; ensure the constant is well-named and used wherever compact
aliases are produced to avoid silent collisions with user-provided identifiers.
- Around line 19-23: The compact temp-alias generation risks colliding with real
aliases named like "$t1"; update the alias generation in
temp-alias-transformer.ts so mapped uses a distinctive prefix (e.g. "$$t")
instead of "$t" by deriving the name from the existing TEMP_ALIAS_PREFIX or
hardcoding a "$$t" prefix, continue to increment with this prefix plus
(this.aliasMap.size + 1), set this.aliasMap.set(node.name, mapped) as before,
and return IdentifierNode.create(mapped) so temp aliases are unlikely to collide
with user or dialect-generated identifiers.
---
Nitpick comments:
In `@packages/orm/src/client/executor/zenstack-query-executor.ts`:
- Around line 635-640: Add an inline comment in processTempAlias explaining why
we instantiate a new TempAliasTransformer per call: this avoids concurrent
queries sharing a single transformer/aliasMap and racing; although
TempAliasTransformer.run clears its map (making reuse technically safe), do not
hoist it to an executor field as an "optimization" because that would
reintroduce potential concurrency bugs—refer to processTempAlias and
TempAliasTransformer.run/aliasMap in the comment so future readers understand
the rationale.
- Around line 635-640: Add a short inline comment in processTempAlias explaining
why we instantiate a new TempAliasTransformer for each call (instead of hoisting
it to a private field): TempAliasTransformer.run() holds/resettable internal
state so reusing one instance across concurrent queries would cause a data race;
therefore keep "new TempAliasTransformer().run(query)" when
options.useCompactAliasNames is true to preserve thread-safety/concurrency.
Reference processTempAlias, TempAliasTransformer, run, and
options.useCompactAliasNames in the comment so future maintainers understand the
rationale.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CLAUDE.mdpackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/executor/temp-alias-transformer.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/options.tspackages/orm/src/client/query-utils.tstests/regression/test/issue-2378.test.ts
|
Claude finished @ymc9's task —— View job PR Review: Compact Alias Names ImplementationReviewing PR #2406 which implements compact alias names to fix PostgreSQL identifier length limitations. Review Checklist:
✅ Overall Assessment: EXCELLENT IMPLEMENTATIONThis is a well-architected solution that effectively addresses PostgreSQL's 63-byte identifier limit with minimal risk and good performance characteristics. Problem AnalysisRoot Cause: Deep nested includes generate alias names like Test Case: The regression test demonstrates a 5-level deep include chain: Implementation Review🔧 Core Solution: TempAliasTransformer (
|
|
Claude finished @ymc9's task —— View job PR Code Review: Compact Alias Names ImplementationConducting a comprehensive code review of PR #2406 which implements compact alias names to fix PostgreSQL identifier length limitations. Review Checklist:
✅ Overall Assessment: APPROVE WITH MINOR SUGGESTIONSThis is a well-implemented solution to a real PostgreSQL identifier length limitation issue. The approach is clean, efficient, and properly tested. Problem AnalysisIssue: Deep nested includes in ZenStack ORM generate PostgreSQL aliases exceeding the 63-byte identifier limit, causing query failures. The regression test demonstrates a 5-level deep include chain: Previously, aliases like Solution: Introduces Implementation Review🔧 Core Solution: TempAliasTransformer (
|
1 participant
fixes #2378
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation