Repository: salsita/node-pg-migrate
Pull request #530 (2019-12-07 14:30:30)

Allow 'Down' migrations in .sql files
This commit allows SQL files to have down migrations via comments.

Anything below `-- Up migration` will be treated as `up`, anything
below a `-- Down migration` will be treated as `down`.

A migration file can have either or both.


Patch:
diff --git a/README.md b/README.md
index 20b15f5..cd964fd 100644
--- a/README.md
+++ b/README.md
@@ -109,6 +109,7 @@ Want to know more? Read docs:
   - [Extensions](docs/extensions.md)
   - [Miscellaneous](docs/misc.md)
 - [Transpiling migrations](docs/transpiling.md)
+- [Troubleshooting](docs/troubleshooting.md)
 
 ## Explanation & Goals
 
diff --git a/docs/api.md b/docs/api.md
index ff6114e..55aa957 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -22,4 +22,4 @@ which takes options argument with following structure (similar to [command line
 - `fake` _[boolean]_ - Mark migrations as run without actually performing them (use with caution!)
 - `dryRun` _[boolean]_
 - `log` _[function]_ - Redirect log messages to this function, rather than `console.log`
-- `decamelize` _[boolean]_ - Runs [`decamelize`](https://github.com/sindresorhus/decamelize) on table/column/etc. names (experimental)
+- `decamelize` _[boolean]_ - Runs [`decamelize`](https://github.com/sindresorhus/decamelize) on table/column/etc. names
diff --git a/docs/cli.md b/docs/cli.md
index e5e4306..68dacb9 100644
--- a/docs/cli.md
+++ b/docs/cli.md
@@ -78,7 +78,7 @@ You can adjust defaults by passing arguments to `node-pg-migrate`:
 - `single-transaction` - Combines all pending migrations into a single transaction so that if any migration fails, all will be rolled back (defaults to `true`, to switch it off supply `--no-single-transaction` on command line).
 - `no-lock` - Disables locking mechanism and checks (useful for DBs which does not support SQL commands used for [locking](migrations.md#locking))
 - `fake` - Mark migrations as run without actually performing them (use with caution!)
-- `decamelize` - Runs [`decamelize`](https://github.com/sindresorhus/decamelize) on table/column/etc. names (experimental)
+- `decamelize` - Runs [`decamelize`](https://github.com/sindresorhus/decamelize) on table/column/etc. names
 
 See all by running `node-pg-migrate --help`.
 
diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md
new file mode 100644
index 0000000..a8933db
--- /dev/null
+++ b/docs/troubleshooting.md
@@ -0,0 +1,33 @@
+# Troubleshooting
+
+## SSL connection
+
+For SSL connection to DB you can set `PGSSLMODE` environment variable to value from [list](https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNECT-SSLMODE) other then `disable`.
+e.g. `PGSSLMODE=require node-pg-migrate up` ([pg](https://github.com/brianc/node-postgres/blob/master/CHANGELOG.md#v260) will take it into account)
+
+Or you can append `?ssl=true` to your `DATABASE_URL`
+
+For setting SSL certificates etc. you will need to use some form of JSON config [see](https://github.com/salsita/node-pg-migrate/blob/master/docs/cli.md)
+with proper SSL configuration [see](https://node-postgres.com/features/ssl)
+
+## Camel case, Snake case, case sensitivity
+
+In PostgreSQL unquoted identifiers are case-insensitive. Thus `SELECT * FROM hello` and `SELECT * FROM HELLO` are equivalent.
+However, quoted identifiers are case-sensitive. `SELECT * FROM "hello"`, `SELECT * FROM "HELLO"` and `SELECT * FROM "HeLLo"`
+are trying to read from three different tables.
+Unquoted identifiers are always folded to lower case.
+[see](https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS)
+
+`node-pg-migrate` always quotes all identifiers, so make sure you also quote them in your SQL (and so does eventual library you use).
+Or always use lower case identifiers to prevent confusion (unquoted identifiers are always folded to lower case - see above).
+
+`node-pg-migrate` also comes with `decamelize` flag which runs `decamelize` package on all table/column/etc. names ([see](https://github.com/salsita/node-pg-migrate/blob/master/docs/cli.md#configuration)). That means that your camel case identifiers will be converted to snake case (lower cased).
+
+## Refused connection issues
+
+- Password to your database may not contain some characters (which have special meaning in url schema) when used in `DATABASE_URL`.
+  Use other means to provide password or change the password. [see](https://github.com/salsita/node-pg-migrate/issues/439)
+- Make sure connection is not stopped by firewalls, security rules, etc.
+  Try `telnet url port` to try to connect (e.g. `telnet 127.0.0.1 5432`) to postgres server.
+  If command will not end with error, but will wait for further input from you, server (or some other service running on that port :man_shrugging:) is accessible and waits for correct user/password.
+  Otherwise server does not run or listen on specified port or there is some other connection problem. You have to investigate...
diff --git a/src/migration.ts b/src/migration.ts
index ea4f411..24357eb 100644
--- a/src/migration.ts
+++ b/src/migration.ts
@@ -83,7 +83,7 @@ export class Migration implements RunMigration {
 
   public readonly timestamp: number
 
-  public readonly up?: MigrationAction
+  public up?: false | MigrationAction
 
   public down?: false | MigrationAction
 
@@ -161,18 +161,16 @@ export class Migration implements RunMigration {
   }
 
   _getAction(direction: MigrationDirection) {
-    if (direction === 'down') {
-      if (this.down === false) {
-        throw new Error(`User has disabled down migration on file: ${this.name}`)
-      }
-
-      if (this.down === undefined) {
-        this.down = this.up
-      }
+    if (direction === 'down' && this.down === undefined) {
+      this.down = this.up
     }
 
     const action: MigrationAction | false | undefined = this[direction]
 
+    if (action === false) {
+      throw new Error(`User has disabled ${direction} migration on file: ${this.name}`)
+    }
+
     if (typeof action !== 'function') {
       throw new Error(
         `Unknown value for direction: ${direction}. Is the migration ${this.name} exporting a '${direction}' function?`,
diff --git a/src/runner.ts b/src/runner.ts
index 84421a4..a19c456 100644
--- a/src/runner.ts
+++ b/src/runner.ts
@@ -1,17 +1,14 @@
-import fs from 'fs'
 import path from 'path'
-import { promisify } from 'util'
 import Db, { DBConnection } from './db'
 import { ColumnDefinitions } from './operations/tablesTypes'
 import { Migration, loadMigrationFiles, RunMigration } from './migration'
 import { MigrationBuilderActions, MigrationDirection, RunnerOptionClient, RunnerOptionUrl, RunnerOption } from './types'
 import { createSchemalize, getMigrationTableSchema, getSchemas } from './utils'
+import migrateSqlFile from './sqlMigration'
 
 // Random but well-known identifier shared by all instances of node-pg-migrate
 const PG_MIGRATE_LOCK_ID = 7241865325823964
 
-const readFile = promisify(fs.readFile) // eslint-disable-line security/detect-non-literal-fs-filename
-
 const idColumn = 'id'
 const nameColumn = 'name'
 const runOnColumn = 'run_on'
@@ -20,26 +17,27 @@ const loadMigrations = async (db: DBConnection, options: RunnerOption, log: type
   try {
     let shorthands: ColumnDefinitions = {}
     const files = await loadMigrationFiles(options.dir, options.ignorePattern)
-    return files.map(file => {
-      const filePath = `${options.dir}/${file}`
-      const actions: MigrationBuilderActions =
-        path.extname(filePath) === '.sql'
-          ? // eslint-disable-next-line security/detect-non-literal-fs-filename
-            { up: async pgm => pgm.sql(await readFile(filePath, 'utf8')) }
-          : // eslint-disable-next-line global-require,import/no-dynamic-require,security/detect-non-literal-require
-            require(path.relative(__dirname, filePath))
-      shorthands = { ...shorthands, ...actions.shorthands }
-      return new Migration(
-        db,
-        filePath,
-        actions,
-        options,
-        {
-          ...shorthands,
-        },
-        log,
-      )
-    })
+    return Promise.all(
+      files.map(async file => {
+        const filePath = `${options.dir}/${file}`
+        const actions: MigrationBuilderActions =
+          path.extname(filePath) === '.sql'
+            ? await migrateSqlFile(filePath)
+            : // eslint-disable-next-line global-require,import/no-dynamic-require,security/detect-non-literal-require
+              require(path.relative(__dirname, filePath))
+        shorthands = { ...shorthands, ...actions.shorthands }
+        return new Migration(
+          db,
+          filePath,
+          actions,
+          options,
+          {
+            ...shorthands,
+          },
+          log,
+        )
+      }),
+    )
   } catch (err) {
     throw new Error(`Can't get migration files: ${err.stack}`)
   }
diff --git a/src/sqlMigration.ts b/src/sqlMigration.ts
new file mode 100644
index 0000000..d7b5e1c
--- /dev/null
+++ b/src/sqlMigration.ts
@@ -0,0 +1,35 @@
+import fs from 'fs'
+import { promisify } from 'util'
+import { MigrationBuilderActions } from './types'
+
+const readFile = promisify(fs.readFile) // eslint-disable-line security/detect-non-literal-fs-filename
+
+const createMigrationCommentRegex = (direction: 'up' | 'down') =>
+  new RegExp(`^\\s*--[\\s-]*${direction}\\s+migration`, 'im') // eslint-disable-line security/detect-non-literal-regexp
+
+export const getActions = (content: string): MigrationBuilderActions => {
+  const upMigrationCommentRegex = createMigrationCommentRegex('up')
+  const downMigrationCommentRegex = createMigrationCommentRegex('down')
+
+  const upMigrationStart = content.search(upMigrationCommentRegex)
+  const downMigrationStart = content.search(downMigrationCommentRegex)
+
+  const upSql =
+    upMigrationStart >= 0
+      ? content.substr(upMigrationStart, downMigrationStart < upMigrationStart ? undefined : downMigrationStart)
+      : content
+  const downSql =
+    downMigrationStart >= 0
+      ? content.substr(downMigrationStart, upMigrationStart < downMigrationStart ? undefined : upMigrationStart)
+      : undefined
+
+  return {
+    up: pgm => pgm.sql(upSql),
+    down: downSql === undefined ? false : pgm => pgm.sql(downSql),
+  }
+}
+
+export default async (sqlPath: string) => {
+  const content = await readFile(sqlPath, 'utf-8')
+  return getActions(content)
+}
diff --git a/src/types.ts b/src/types.ts
index f50da91..42c25f4 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -107,8 +107,8 @@ export type MigrationAction = (pgm: MigrationBuilder, run?: () => void) => Promi
 export type Literal = (v: Name) => string
 
 export interface MigrationBuilderActions {
-  up?: MigrationAction
-  down?: MigrationAction
+  up?: MigrationAction | false
+  down?: MigrationAction | false
   shorthands?: tables.ColumnDefinitions
 }
 
diff --git a/templates/migration-template.sql b/templates/migration-template.sql
index aed00f2..f27cf2d 100644
--- a/templates/migration-template.sql
+++ b/templates/migration-template.sql
@@ -1,1 +1,3 @@
--- up migration
\ No newline at end of file
+-- Up Migration
+
+-- Down Migration
\ No newline at end of file


Test patch:
diff --git a/test/sqlMigration-test.ts b/test/sqlMigration-test.ts
new file mode 100644
index 0000000..2be6242
--- /dev/null
+++ b/test/sqlMigration-test.ts
@@ -0,0 +1,105 @@
+import sinon from 'sinon'
+import { expect } from 'chai'
+import { getActions } from '../src/sqlMigration'
+
+/* eslint-disable no-unused-expressions */
+
+describe('lib/sqlMigration', () => {
+  describe('getActions', () => {
+    it('without comments', () => {
+      const content = 'SELECT 1 FROM something'
+      const { up, down } = getActions(content)
+      expect(up).to.exist
+      expect(down).to.be.false
+
+      const sql = sinon.spy()
+      expect(up({ sql })).to.not.exist
+      expect(sql.called).to.be.true
+      expect(sql.lastCall.args[0].trim()).to.eql(content.trim())
+    })
+
+    it('with up comment', () => {
+      const content = `
+-- Up Migration
+SELECT 1 FROM something
+`
+      const { up, down } = getActions(content)
+      expect(up).to.exist
+      expect(down).to.be.false
+
+      const sql = sinon.spy()
+      expect(up({ sql })).to.not.exist
+      expect(sql.called).to.be.true
+      expect(sql.lastCall.args[0].trim()).to.eql(content.trim())
+    })
+
+    it('with both comments', () => {
+      const upMigration = `
+-- Up Migration
+SELECT 1 FROM something`
+      const downMigration = `
+-- Down Migration
+SELECT 2 FROM something`
+      const content = `${upMigration}${downMigration}`
+      const { up, down } = getActions(content)
+      expect(up).to.exist
+      expect(down).to.exist
+
+      const upSql = sinon.spy()
+      expect(up({ sql: upSql })).to.not.exist
+      expect(upSql.called).to.be.true
+      expect(upSql.lastCall.args[0].trim()).to.eql(upMigration.trim())
+
+      const downSql = sinon.spy()
+      expect(down({ sql: downSql })).to.not.exist
+      expect(downSql.called).to.be.true
+      expect(downSql.lastCall.args[0].trim()).to.eql(downMigration.trim())
+    })
+
+    it('with both comments in reverse order', () => {
+      const upMigration = `
+-- Up Migration
+SELECT 1 FROM something`
+      const downMigration = `
+-- Down Migration
+SELECT 2 FROM something`
+      const content = `${downMigration}${upMigration}`
+      const { up, down } = getActions(content)
+      expect(up).to.exist
+      expect(down).to.exist
+
+      const upSql = sinon.spy()
+      expect(up({ sql: upSql })).to.not.exist
+      expect(upSql.called).to.be.true
+      expect(upSql.lastCall.args[0].trim()).to.eql(upMigration.trim())
+
+      const downSql = sinon.spy()
+      expect(down({ sql: downSql })).to.not.exist
+      expect(downSql.called).to.be.true
+      expect(downSql.lastCall.args[0].trim()).to.eql(downMigration.trim())
+    })
+
+    it('with both comments with some chars added', () => {
+      const upMigration = `
+ -- - up Migration to do Up migration
+SELECT 1 FROM something`
+      const downMigration = `
+  -- -- -- Down    migration to bring DB down
+SELECT 2 FROM something`
+      const content = `${upMigration}${downMigration}`
+      const { up, down } = getActions(content)
+      expect(up).to.exist
+      expect(down).to.exist
+
+      const upSql = sinon.spy()
+      expect(up({ sql: upSql })).to.not.exist
+      expect(upSql.called).to.be.true
+      expect(upSql.lastCall.args[0].trim()).to.eql(upMigration.trim())
+
+      const downSql = sinon.spy()
+      expect(down({ sql: downSql })).to.not.exist
+      expect(downSql.called).to.be.true
+      expect(downSql.lastCall.args[0].trim()).to.eql(downMigration.trim())
+    })
+  })
+})