fix(drizzle): emit bare eql_v2_encrypted and rewrite all mangled ALTER forms#353
fix(drizzle): emit bare eql_v2_encrypted and rewrite all mangled ALTER forms#353
Conversation
…forms
@cipherstash/stack 0.15.0 returned `"public"."eql_v2_encrypted"` from the
Drizzle customType `dataType()` callback, aiming to avoid
`"undefined"."eql_v2_encrypted"` in drizzle-kit's ALTER COLUMN output. But
drizzle-kit's ALTER COLUMN path wraps the return value in double-quotes and
unconditionally prepends `"{typeSchema}".`, and custom types have no
typeSchema — so the pre-quoted form came out as
`"undefined".""public"."eql_v2_encrypted""`, which Postgres can't parse.
The CLI's migration rewriter regex matched neither that form nor the
original `"undefined"."eql_v2_encrypted"` form, so broken ALTER statements
survived into the final migration.
- Return the bare `eql_v2_encrypted` identifier. drizzle-kit's CREATE TABLE
path already omits the schema prefix when typeSchema is empty, so this
emits correctly there.
- Broaden rewriteEncryptedAlterColumns' regex to match all four forms
drizzle-kit may produce. The rewriter converts each into the safe
ADD COLUMN + DROP COLUMN + RENAME sequence.
- getEncryptedColumnConfig still accepts both the bare and the pre-quoted
sqlName values for back-compat with tables built against 0.15.0.
- Add tests for the two newly-matched forms.
🦋 Changeset detectedLatest commit: 8513705 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe changes fix how the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
packages/cli/src/__tests__/rewrite-migrations.test.ts (1)
54-82: Good variant coverage.Both new tests pin the exact mangled forms drizzle-kit emits and assert the full ADD+DROP+RENAME shape plus absence of
SET DATA TYPE, which is the right contract. Consider also adding a negative-ish assertion — e.g.expect(updated).not.toContain('"undefined"')— to guard against a future regex regression that keeps the original (mangled) statement intact while still matchingADD COLUMN. Optional, not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/__tests__/rewrite-migrations.test.ts` around lines 54 - 82, Add a negative assertion in each test to ensure the mangled namespace is fully removed: after calling rewriteEncryptedAlterColumns(...) and reading the updated file into the updated variable, add expect(updated).not.toContain('"undefined"') (or similar check for the exact mangled substring) in both tests ("rewrites the \"undefined\" schema form drizzle-kit emits for bare custom types" and "rewrites the double-quoted form produced by stack 0.15.0") so the regex change cannot accidentally leave the original '"undefined"' text in the output while still producing the ADD COLUMN sequence.packages/cli/src/commands/db/rewrite-migrations.ts (1)
21-22: Regex alternation ordering is correct, but the bareeql_v2_encryptedbranch is unanchored.Alternation ordering is fine (longer/more-specific
"undefined".""public"...comes first, so it's not shadowed by the shorter"undefined"."eql_v2_encrypted"branch). However, the finaleql_v2_encryptedalternative has no right-side boundary, so a hypotheticalSET DATA TYPE eql_v2_encrypted_v2;(or any suffix) would also match and be rewritten. Unlikely today, but cheap to harden:Optional: anchor the bare form with a non-identifier lookahead
- /ALTER TABLE "([^"]+)"\s+ALTER COLUMN "([^"]+)"\s+SET DATA TYPE (?:"undefined"\.""public"\."eql_v2_encrypted""|"undefined"\."eql_v2_encrypted"|"public"\."eql_v2_encrypted"|eql_v2_encrypted)[^;]*;/gi + /ALTER TABLE "([^"]+)"\s+ALTER COLUMN "([^"]+)"\s+SET DATA TYPE (?:"undefined"\.""public"\."eql_v2_encrypted""|"undefined"\."eql_v2_encrypted"|"public"\."eql_v2_encrypted"|eql_v2_encrypted\b)[^;]*;/giAlso worth noting:
[^;]*;greedily consumes anything up to the next;, which is fine for single-statement lines but would over-match if drizzle-kit ever emitted trailing clauses with embedded semicolons in string literals. Not a realistic concern for this path — flagging for awareness only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/db/rewrite-migrations.ts` around lines 21 - 22, The final bare alternative in ALTER_COLUMN_TO_ENCRYPTED_RE is unanchored and will wrongly match longer identifiers like eql_v2_encrypted_v2; update the regex (ALTER_COLUMN_TO_ENCRYPTED_RE) so the bare eql_v2_encrypted branch is anchored with a non-identifier lookahead (e.g., a word-boundary or (?=\W|;)) to ensure only the exact type name matches, leaving the other alternates unchanged; locate ALTER_COLUMN_TO_ENCRYPTED_RE in rewrite-migrations.ts and replace the last alternation accordingly.
🤖 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/cli/src/__tests__/rewrite-migrations.test.ts`:
- Around line 54-82: Add a negative assertion in each test to ensure the mangled
namespace is fully removed: after calling rewriteEncryptedAlterColumns(...) and
reading the updated file into the updated variable, add
expect(updated).not.toContain('"undefined"') (or similar check for the exact
mangled substring) in both tests ("rewrites the \"undefined\" schema form
drizzle-kit emits for bare custom types" and "rewrites the double-quoted form
produced by stack 0.15.0") so the regex change cannot accidentally leave the
original '"undefined"' text in the output while still producing the ADD COLUMN
sequence.
In `@packages/cli/src/commands/db/rewrite-migrations.ts`:
- Around line 21-22: The final bare alternative in ALTER_COLUMN_TO_ENCRYPTED_RE
is unanchored and will wrongly match longer identifiers like
eql_v2_encrypted_v2; update the regex (ALTER_COLUMN_TO_ENCRYPTED_RE) so the bare
eql_v2_encrypted branch is anchored with a non-identifier lookahead (e.g., a
word-boundary or (?=\W|;)) to ensure only the exact type name matches, leaving
the other alternates unchanged; locate ALTER_COLUMN_TO_ENCRYPTED_RE in
rewrite-migrations.ts and replace the last alternation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75c8d8d3-58d0-4589-bc7a-181dc225900d
📒 Files selected for processing (4)
.changeset/fix-drizzle-encrypted-datatype.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/cli/src/commands/db/rewrite-migrations.tspackages/stack/src/drizzle/index.ts
1 participant
Summary
@cipherstash/stack@0.15.0+@cipherstash/cli@0.6.0produce invalidALTER COLUMNmigrations whennpx drizzle-kit generateconverts an existing column toencryptedType, e.g.:Two bugs combine to produce this:
EQL_ENCRYPTED_DATA_TYPE = '"public"."eql_v2_encrypted"'(packages/stack/src/drizzle/index.ts). drizzle-kit's ALTER COLUMN path wraps thedataType()return value in double-quotes and unconditionally prepends"{typeSchema}".. Custom types have notypeSchema, so the pre-quoted string came out as"undefined".""public"."eql_v2_encrypted"". (The CREATE TABLE path behaves correctly — it skips the prefix whentypeSchemais empty — which is why this regression shipped.)ALTER_COLUMN_TO_ENCRYPTED_RE(packages/cli/src/commands/db/rewrite-migrations.ts) only matchedeql_v2_encryptedand"public"."eql_v2_encrypted", so neither the pre-0.15.0"undefined"."eql_v2_encrypted"form nor the 0.15.0"undefined".""public"."eql_v2_encrypted""form triggered the ADD + DROP + RENAME rewrite. Broken ALTERs survived into the final migration.Changes
encryptedTypenow returns the bareeql_v2_encryptedidentifier fromcustomType.dataType(). drizzle-kit's CREATE TABLE path already omits the schema prefix whentypeSchemais empty, so this emits correctly there. ALTER COLUMN output is still mangled by drizzle-kit, but the rewriter now catches it.rewriteEncryptedAlterColumnsregex broadened to match all four forms drizzle-kit may emit. Each is rewritten into the safeADD COLUMN … DROP COLUMN … RENAME COLUMNsequence.getEncryptedColumnConfigcontinues to accept both sqlName forms for back-compat with schemas built against 0.15.0.rewrite-migrations.test.tscovering the"undefined"."eql_v2_encrypted"and"undefined".""public"."eql_v2_encrypted""forms.@cipherstash/stackand@cipherstash/cli.Test plan
pnpm --filter @cipherstash/cli test— 126 passed (incl. 2 new rewriter tests)pnpm --filter @cipherstash/stack exec vitest run __tests__/drizzle-operators-jsonb.test.ts— 11 passedADD COLUMN "x__cipherstash_tmp" "public"."eql_v2_encrypted";+ DROP + RENAME with no"undefined"anywhereSummary by CodeRabbit
Release Notes