fix(orm): fallback to compact temp aliases for overlong names#2425
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAlways construct and run TempAliasTransformer with an explicit mode based on 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Hi @pkudinov , with the name compaction, the |
|
Hi @ymc9, excited to get to contribute to Zenstack. We caught this issue while migrating from V2 to V3. I did not flip For #2424 specifically, alias collision is reproducible due to Postgres identifier truncation (63 bytes). In the failing SQL, these distinct aliases are generated: All truncate to the same 63-byte prefix: So distinct qualifiers collapse to one identifier, and references like Is there some better way to fix this? |
Thanks for the explanation @pkudinov . I was just wondering if the temp alias compaction logic had any issues that made you set the flag to For this PR, I feel it's cleaner to move the name length check into the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/orm/src/client/executor/temp-alias-transformer.ts (1)
41-46: ValidatemaxIdentifierLengthat construction.A non-integer or non-positive value can silently make fallback behavior unreliable. Guarding once in the constructor will harden this path.
Proposed diff
constructor(options: TempAliasTransformerOptions = {}) { super(); this.mode = options.mode ?? 'alwaysCompact'; // PostgreSQL limits identifier length to 63 bytes and silently truncates overlong aliases. - this.maxIdentifierLength = options.maxIdentifierLength ?? 63; + const maxIdentifierLength = options.maxIdentifierLength ?? 63; + if (!Number.isInteger(maxIdentifierLength) || maxIdentifierLength <= 0) { + throw new Error('maxIdentifierLength must be a positive integer'); + } + this.maxIdentifierLength = maxIdentifierLength; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/executor/temp-alias-transformer.ts` around lines 41 - 46, Validate the maxIdentifierLength passed into the TempAliasTransformer constructor: in constructor of class TempAliasTransformer, ensure options.maxIdentifierLength is a finite integer > 0 (use Number.isInteger and isFinite) before assigning to this.maxIdentifierLength; if invalid, throw a clear RangeError/TypeError (e.g., "maxIdentifierLength must be a positive integer") so downstream fallback logic that relies on maxIdentifierLength can't silently misbehave.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/orm/src/client/executor/temp-alias-transformer.ts`:
- Around line 41-46: Validate the maxIdentifierLength passed into the
TempAliasTransformer constructor: in constructor of class TempAliasTransformer,
ensure options.maxIdentifierLength is a finite integer > 0 (use Number.isInteger
and isFinite) before assigning to this.maxIdentifierLength; if invalid, throw a
clear RangeError/TypeError (e.g., "maxIdentifierLength must be a positive
integer") so downstream fallback logic that relies on maxIdentifierLength can't
silently misbehave.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6425275-091b-46eb-b931-7f7e2cce7c38
📒 Files selected for processing (2)
packages/orm/src/client/executor/temp-alias-transformer.tspackages/orm/src/client/executor/zenstack-query-executor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/orm/src/client/executor/zenstack-query-executor.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 24-26: The temp-alias length check in transformIdentifier
currently uses character count (node.name.length) which fails for multibyte
UTF-8 characters; change the check in transformIdentifier (and any related
comparisons to this.maxIdentifierLength) to use the byte length of the
identifier (e.g., Buffer.byteLength(node.name, 'utf8') or Buffer.from(node.name,
'utf8').length) when deciding to set hasOverlongTempAlias for names starting
with TEMP_ALIAS_PREFIX so the 63-byte PostgreSQL limit is enforced correctly.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f959c56-ccdd-4d18-aa3a-cee251458a95
📒 Files selected for processing (1)
packages/orm/src/client/executor/temp-alias-transformer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/orm/src/client/executor/temp-alias-transformer.ts (1)
19-35: Consider early termination once an overlong alias is found.Once
hasOverlongTempAliasis set totrue, the checker continues traversing the entire AST unnecessarily. For deeply nested queries (which is the target use case), this could be optimized.♻️ Proposed optimization with early exit
+class TempAliasLengthChecker extends OperationNodeTransformer { + private hasOverlongTempAlias = false; + private readonly textEncoder = new TextEncoder(); + + constructor(private readonly maxIdentifierLength: number) { + super(); + } + run(node: OperationNode) { this.hasOverlongTempAlias = false; - this.transformNode(node); + try { + this.transformNode(node); + } catch (e) { + if (e !== 'early_exit') throw e; + } return this.hasOverlongTempAlias; } protected override transformIdentifier(node: IdentifierNode): IdentifierNode { if (!node.name.startsWith(TEMP_ALIAS_PREFIX)) { return node; } const aliasByteLength = this.textEncoder.encode(node.name).length; if (aliasByteLength > this.maxIdentifierLength) { this.hasOverlongTempAlias = true; + throw 'early_exit'; } return node; } }Alternatively, if Kysely's transformer supports a cleaner short-circuit mechanism, that would be preferable. This is a nice-to-have optimization given the target use case involves deep nesting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/executor/temp-alias-transformer.ts` around lines 19 - 35, The traversal should short-circuit once an overlong temp alias is detected: add a guard at the top of the AST traversal entry used by this transformer (override/modify transformNode) so it immediately returns the incoming node when this.hasOverlongTempAlias is true, and also add an early-return in transformIdentifier (check this.hasOverlongTempAlias before calling textEncoder.encode) to avoid doing byte-length checks after the flag is set; this uses the existing symbols run, transformNode, transformIdentifier, hasOverlongTempAlias, TEMP_ALIAS_PREFIX, textEncoder, and maxIdentifierLength so the traversal stops as soon as an overlong alias is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/orm/src/client/executor/temp-alias-transformer.ts`:
- Around line 19-35: The traversal should short-circuit once an overlong temp
alias is detected: add a guard at the top of the AST traversal entry used by
this transformer (override/modify transformNode) so it immediately returns the
incoming node when this.hasOverlongTempAlias is true, and also add an
early-return in transformIdentifier (check this.hasOverlongTempAlias before
calling textEncoder.encode) to avoid doing byte-length checks after the flag is
set; this uses the existing symbols run, transformNode, transformIdentifier,
hasOverlongTempAlias, TEMP_ALIAS_PREFIX, textEncoder, and maxIdentifierLength so
the traversal stops as soon as an overlong alias is found.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af85178c-daea-4064-b4a9-cafa2c527a15
📒 Files selected for processing (1)
packages/orm/src/client/executor/temp-alias-transformer.ts
|
Implemented in 6cb0088 — TempAliasLengthChecker now short-circuits traversal after detecting the first overlong temp alias (with an additional guard in transformIdentifier), so deep ASTs avoid unnecessary work. |
2 participants
Fixes #2424
Summary
This PR fixes deep nested include failures caused by overlong temporary relation aliases when
useCompactAliasNames: false.useCompactAliasNames: falsewhen aliases are short/safe.useCompactAliasNames: true) unchanged.Root Cause
Deep relation paths generate long temp aliases (via
$$_...).When aliases exceed identifier limits, they can be truncated/collide and break reference resolution in nested joins, resulting in errors like
42703(column ... does not exist).Summary by CodeRabbit
Bug Fixes
Documentation
Tests