Allow 'Down' migrations in .sql files (#530)

* 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.

* Fix lint errors

* Use Title Case in SQL template file

* Make migrateSqlFile synchronous

* Review comments

* Fix regexp

* Rewritten, tests

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?`,

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}`)
   }

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)
+}

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
 }
 

templates/migration-template.sql

@@ -1 +1,3 @@
--- up migration
+-- Up Migration
+
+-- Down Migration

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())
+    })
+  })
+})