fix(target-postgres): measure identifier length in bytes, not characters - #30127
fix(target-postgres): measure identifier length in bytes, not characters#30127MahathirMohammadShuvo wants to merge 1 commit into
Conversation
PostgreSQL truncates identifiers at NAMEDATALEN - 1, which is 63 *bytes*. quoteIdentifier compared identifier.length, so a name written in non-ASCII characters could sit well under 63 characters and still overrun: a 50-character Cyrillic column name is 96 UTF-8 bytes, and the warning never fired. validateEnumValueLength in the same module already measured bytes via TextEncoder. Both checks now share one byteLength helper so they cannot drift apart again, and the warning text says "byte" rather than "character". Signed-off-by: Mahathir Mohammad Shuvo <shuvo1728@gmail.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughPostgreSQL length validation now measures UTF-8 bytes through a shared helper. Identifier warnings use the 63-byte limit, enum validation reuses the helper, and tests cover multibyte identifiers above and at the limit. ChangesPostgreSQL UTF-8 length validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The change corrects identifier-length warnings for multibyte names and adds focused coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
All commits here are signed off per the DCO, as The CLA check may be stale, though.
and Happy to sign whatever is actually required — flagging it in case the integration is a leftover from before the switch to DCO, since it will block every outside contribution the same way. |
Linked issue
n/a — small change
Summary
quoteIdentifierwarned when an identifier exceeded 63 characters, butPostgreSQL measures
NAMEDATALEN - 1in bytes. A name written in non-ASCIIcharacters can sit well under 63 characters and still overrun — a 50-character
Cyrillic column name is 96 UTF-8 bytes — so the warning never fired and the
object was silently truncated server-side, leaving the declared name unable to
match the live one.
validateEnumValueLength, in the same module, already measured bytes viaTextEncoder. Both checks now read through onebyteLengthhelper so theycannot drift apart again, and the warning text says "byte" rather than
"character".
The warn-vs-throw split between the two functions is left alone: it mirrors
PostgreSQL, where an over-long enum label raises
invalid enum labelbut anover-long identifier is truncated by
pg_mbcliplen. The file already documentsthat distinction.
Testing performed
npx vitest runinpackages/3-targets/3-targets/postgres— 1589 tests,92 of 93 files. The one failing file,
test/migrations/render-typescript.test.ts,fails to load on a missing
@internal/cli/migration-clibuild artifact; it isunrelated and fails identically on
main.npx vitest run test/sql-utils.test.ts— 33 passed; withsrc/core/sql-utils.tsreverted tomain, 2 failed / 31 passed.biome checkon both changed files — exit 0.scripts/lint-deps-focused.mjson both changed files — no dependencyviolations.
I did not run
pnpm test:packages,test:integrationor the e2e suites — thisworkspace is only partially built, so those results would not be meaningful.
Skill update
n/a — internal only. The change is a length-check unit and a
console.warnstring; no CLI flag, public API,
prisma.config.tsfield, error code orglossary term is affected.
Checklist
git commit -s) per the DCO.skills-contrib/contrib-pr/SKILL.mdspecifies for outside contributions(the
TML-NNNNprefix in this checklist is a Linear ticket reference I donot have — say the word if you would rather I retitle).
Notes for the reviewer
On duplication.
packages/2-sql/1-core/schema-ir/src/naming.tsalready has abyteLengthhelper with the same shape, and this package does import from thatpackage's
./namingsubpath elsewhere — so consolidating is possible, and itwould need only an
exportadded there. I kept the helper local to stay withinone logical concern, but I am happy to switch to importing it if you would
rather have one copy. That is your call, not mine to make in this PR.
One caveat, pre-existing.
TextEncodermeasures UTF-8, while PostgreSQL'slimit is bytes in the server encoding. On a non-UTF-8 database the new warning
could fire early. Nothing in
packages/3-targets/*/srcreadsclient_encoding,and
naming.tsmakes the same UTF-8 assumption, so this is consistent with therest of the repo rather than something the change introduces.
Tests. The Cyrillic case is the regression test — it fails on
main. The'€'.repeat(21)case is exactly 63 bytes and passes either way; it is therebecause there was no exactly-63 boundary test before, and it is what catches a
>→>=slip.Summary by CodeRabbit