Repository: salsita/node-pg-migrate (ts)

Issue:
Dollar-string escaping fails on Cockroachdb
Escaping strategy fails on Cockroachdb.

https://github.com/salsita/node-pg-migrate/blob/8489b732aefab12252b60c99f0bc7a5af776fdad/src/utils.ts#L39

See https://github.com/cockroachdb/cockroach/issues/54676.

Pull request:
Using string ids for Dollar-string escaping
Fixes #697

Patch:
diff --git a/src/utils.ts b/src/utils.ts
index b845bf6..ed8b165 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -21,6 +21,31 @@ export const createSchemalize = (shouldDecamelize: boolean, shouldQuote: boolean
   }
 }
 
+// credits to https://stackoverflow.com/a/12504061/4790644
+export class StringIdGenerator {
+  private ids: number[] = [0]
+
+  // eslint-disable-next-line no-useless-constructor
+  constructor(private readonly chars = 'abcdefghijklmnopqrstuvwxyz') {}
+
+  next() {
+    const idsChars = this.ids.map((id) => this.chars[id])
+    this.increment()
+    return idsChars.join('')
+  }
+
+  private increment() {
+    for (let i = this.ids.length - 1; i >= 0; i -= 1) {
+      this.ids[i] += 1
+      if (this.ids[i] < this.chars.length) {
+        return
+      }
+      this.ids[i] = 0
+    }
+    this.ids.unshift(0)
+  }
+}
+
 const isPgLiteral = (val: unknown): val is PgLiteral =>
   typeof val === 'object' && val !== null && 'literal' in val && (val as { literal: unknown }).literal === true
 
@@ -33,9 +58,10 @@ export const escapeValue = (val: Value): string | number => {
   }
   if (typeof val === 'string') {
     let dollars: string
-    let index = 0
+    const ids = new StringIdGenerator()
+    let index: string
     do {
-      index += 1
+      index = ids.next()
       dollars = `$pg${index}$`
     } while (val.indexOf(dollars) >= 0)
     return `${dollars}${val}${dollars}`


Test patch:
diff --git a/test/tables-test.ts b/test/tables-test.ts
index 6a255ed..18190d7 100644
--- a/test/tables-test.ts
+++ b/test/tables-test.ts
@@ -276,12 +276,12 @@ describe('lib/operations/tables', () => {
   "colA" integer,
   CONSTRAINT "myTableName_fk_colA" FOREIGN KEY ("colA") REFERENCES "otherTable"
 );
-COMMENT ON CONSTRAINT "myTableName_fk_colA" ON "myTableName" IS $pg1$example comment$pg1$;`)
+COMMENT ON CONSTRAINT "myTableName_fk_colA" ON "myTableName" IS $pga$example comment$pga$;`)
       expect(sql2).to.equal(`CREATE TABLE "my_table_name" (
   "col_a" integer,
   CONSTRAINT "my_table_name_fk_col_a" FOREIGN KEY ("col_a") REFERENCES "other_table"
 );
-COMMENT ON CONSTRAINT "my_table_name_fk_col_a" ON "my_table_name" IS $pg1$example comment$pg1$;`)
+COMMENT ON CONSTRAINT "my_table_name_fk_col_a" ON "my_table_name" IS $pga$example comment$pga$;`)
     })
 
     it('creates comments on column foreign keys', () => {
@@ -307,14 +307,14 @@ COMMENT ON CONSTRAINT "my_table_name_fk_col_a" ON "my_table_name" IS $pg1$exampl
   "colA" integer CONSTRAINT "myTableName_fk_colA" REFERENCES otherTable (a),
   "colB" integer CONSTRAINT "fkColB" REFERENCES "otherTableTwo"
 );
-COMMENT ON CONSTRAINT "myTableName_fk_colA" ON "myTableName" IS $pg1$fk a comment$pg1$;
-COMMENT ON CONSTRAINT "fkColB" ON "myTableName" IS $pg1$fk b comment$pg1$;`)
+COMMENT ON CONSTRAINT "myTableName_fk_colA" ON "myTableName" IS $pga$fk a comment$pga$;
+COMMENT ON CONSTRAINT "fkColB" ON "myTableName" IS $pga$fk b comment$pga$;`)
       expect(sql2).to.equal(`CREATE TABLE "my_table_name" (
   "col_a" integer CONSTRAINT "my_table_name_fk_col_a" REFERENCES otherTable (a),
   "col_b" integer CONSTRAINT "fk_col_b" REFERENCES "other_table_two"
 );
-COMMENT ON CONSTRAINT "my_table_name_fk_col_a" ON "my_table_name" IS $pg1$fk a comment$pg1$;
-COMMENT ON CONSTRAINT "fk_col_b" ON "my_table_name" IS $pg1$fk b comment$pg1$;`)
+COMMENT ON CONSTRAINT "my_table_name_fk_col_a" ON "my_table_name" IS $pga$fk a comment$pga$;
+COMMENT ON CONSTRAINT "fk_col_b" ON "my_table_name" IS $pga$fk b comment$pga$;`)
     })
 
     it('creates no comments on unnamed constraints', () => {
@@ -383,10 +383,10 @@ COMMENT ON CONSTRAINT "fk_col_b" ON "my_table_name" IS $pg1$fk b comment$pg1$;`)
       const sql2 = Tables.addConstraint(options2)(...args)
       expect(sql1).to.equal(`ALTER TABLE "myTableName"
   ADD CONSTRAINT "myConstraintName" PRIMARY KEY ("colA");
-COMMENT ON CONSTRAINT "myConstraintName" ON "myTableName" IS $pg1$this is an important primary key$pg1$;`)
+COMMENT ON CONSTRAINT "myConstraintName" ON "myTableName" IS $pga$this is an important primary key$pga$;`)
       expect(sql2).to.equal(`ALTER TABLE "my_table_name"
   ADD CONSTRAINT "my_constraint_name" PRIMARY KEY ("col_a");
-COMMENT ON CONSTRAINT "my_constraint_name" ON "my_table_name" IS $pg1$this is an important primary key$pg1$;`)
+COMMENT ON CONSTRAINT "my_constraint_name" ON "my_table_name" IS $pga$this is an important primary key$pga$;`)
     })
   })
 })
diff --git a/test/utils-test.ts b/test/utils-test.ts
index b9e2677..ba3c095 100644
--- a/test/utils-test.ts
+++ b/test/utils-test.ts
@@ -1,5 +1,5 @@
 import { expect } from 'chai'
-import { escapeValue, applyType, createSchemalize, createTransformer } from '../src/utils'
+import { escapeValue, applyType, createSchemalize, createTransformer, StringIdGenerator } from '../src/utils'
 import { ColumnDefinitions } from '../src/operations/tablesTypes'
 import PgLiteral from '../src/operations/PgLiteral'
 import { PgLiteralValue } from '../src/operations/generalTypes'
@@ -21,7 +21,7 @@ describe('lib/utils', () => {
     it('escape string', () => {
       const value = '#escape_me'
 
-      expect(escapeValue(value)).to.equal('$pg1$#escape_me$pg1$')
+      expect(escapeValue(value)).to.equal('$pga$#escape_me$pga$')
     })
 
     it('keep number as is', () => {
@@ -35,7 +35,7 @@ describe('lib/utils', () => {
       const value2 = [['a'], ['b']]
 
       expect(escapeValue(value)).to.equal('ARRAY[[1],[2]]')
-      expect(escapeValue(value2)).to.equal('ARRAY[[$pg1$a$pg1$],[$pg1$b$pg1$]]')
+      expect(escapeValue(value2)).to.equal('ARRAY[[$pga$a$pga$],[$pga$b$pga$]]')
     })
 
     it('parse PgLiteral to unescaped string', () => {
@@ -134,4 +134,41 @@ describe('lib/utils', () => {
       ).to.equal('INSERT INTO s (id) VALUES (1);')
     })
   })
+
+  describe('.StringIdGenerator', () => {
+    it('generates correct sequence', () => {
+      const chars = 'abcd'
+
+      const ids = new StringIdGenerator(chars)
+      const results = [
+        'a',
+        'b',
+        'c',
+        'd',
+        'aa',
+        'ab',
+        'ac',
+        'ad',
+        'ba',
+        'bb',
+        'bc',
+        'bd',
+        'ca',
+        'cb',
+        'cc',
+        'cd',
+        'da',
+        'db',
+        'dc',
+        'dd',
+        'aaa',
+        'aab',
+        'aac',
+        'aad',
+      ]
+      results.forEach((res) => {
+        expect(ids.next()).to.equal(res)
+      })
+    })
+  })
 })