diff --git a/CHANGELOG.md b/CHANGELOG.md index ebc90ca..faacf28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,16 @@ # Changelog -## V5 +## 5.1.0 + +- Validate migration ordering when loading files (instead of when applying migrations) +- Expose `loadMigrationFiles` publicly, which can be used to validate files in e.g. a pre-push hook +- Add `pg-validate-migrations` bin script + +## 5.0.0 - [BREAKING] Update `pg` to version 8. See the [pg changelog](https://github.com/brianc/node-postgres/blob/master/CHANGELOG.md#pg800) for details. -## V4 +## 4.0.0 - [BREAKING] Updated whole project to TypeScript - some types might differ, no functional change diff --git a/README.md b/README.md index ed5ec13..e561fd9 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,16 @@ async function() { } ``` +### Validating migration files + +Occasionally, if two people are working on the same codebase independently, they might both create a migration at the same time. For example, `5_add-table.sql` and `5_add-column.sql`. If these both get pushed, there will be a conflict. + +While the migration system will notice this and refuse to apply the migrations, it can be useful to catch this as early as possible. + +The `loadMigrationFiles` function can be used to check if the migration files satisfy the rules. + +Alternatively, use the `pg-validate-migrations` bin script: `pg-validate-migrations "path/to/migration/files"`. + ## Design decisions ### No down migrations diff --git a/package.json b/package.json index e5807e6..d787ca6 100644 --- a/package.json +++ b/package.json @@ -4,6 +4,9 @@ "description": "Stack Overflow style database migrations for PostgreSQL", "main": "dist/index.js", "types": "dist/index.d.ts", + "bin": { + "pg-validate-migrations": "./dist/bin/validate.js" + }, "author": "Thom Wright", "keywords": [ "postgres", diff --git a/src/__unit__/migration-file-validation/fixtures/conflict/1_add-column.sql b/src/__unit__/migration-file-validation/fixtures/conflict/1_add-column.sql new file mode 100644 index 0000000..e69de29 diff --git a/src/__unit__/migration-file-validation/fixtures/conflict/1_create-table.sql b/src/__unit__/migration-file-validation/fixtures/conflict/1_create-table.sql new file mode 100644 index 0000000..e69de29 diff --git a/src/__unit__/migration-file-validation/validate.ts b/src/__unit__/migration-file-validation/validate.ts new file mode 100644 index 0000000..50e0055 --- /dev/null +++ b/src/__unit__/migration-file-validation/validate.ts @@ -0,0 +1,15 @@ +// tslint:disable no-console +import test from "ava" +import {loadMigrationFiles} from "../.." +process.on("uncaughtException", function (err) { + console.log(err) +}) + +test("two migrations with the same id", async (t) => { + const error = await t.throwsAsync(async () => + loadMigrationFiles( + "src/__unit__/migration-file-validation/fixtures/conflict", + ), + ) + t.regex(error.message, /non-consecutive/) +}) diff --git a/src/__unit__/migration-file/index.ts b/src/__unit__/migration-file/index.ts index 6190966..ab9a66e 100644 --- a/src/__unit__/migration-file/index.ts +++ b/src/__unit__/migration-file/index.ts @@ -1,10 +1,10 @@ import test from "ava" -import {load} from "../../migration-file" +import {loadMigrationFile} from "../../migration-file" test("Hashes of JS files should be the same when the SQL is the same", async (t) => { const [js1, js2] = await Promise.all([ - load(__dirname + "/fixtures/different-js-same-sql-1/1_js.js"), - load(__dirname + "/fixtures/different-js-same-sql-2/1_js.js"), + loadMigrationFile(__dirname + "/fixtures/different-js-same-sql-1/1_js.js"), + loadMigrationFile(__dirname + "/fixtures/different-js-same-sql-2/1_js.js"), ]) t.is(js1.hash, js2.hash) @@ -12,8 +12,8 @@ test("Hashes of JS files should be the same when the SQL is the same", async (t) test("Hashes of JS files should be different when the SQL is different", async (t) => { const [js1, js2] = await Promise.all([ - load(__dirname + "/fixtures/same-js-different-sql-1/1_js.js"), - load(__dirname + "/fixtures/same-js-different-sql-2/1_js.js"), + loadMigrationFile(__dirname + "/fixtures/same-js-different-sql-1/1_js.js"), + loadMigrationFile(__dirname + "/fixtures/same-js-different-sql-2/1_js.js"), ]) t.not(js1.hash, js2.hash) diff --git a/src/bin/validate.ts b/src/bin/validate.ts new file mode 100755 index 0000000..545b183 --- /dev/null +++ b/src/bin/validate.ts @@ -0,0 +1,16 @@ +#!/usr/bin/env node +// tslint:disable no-console + +import {argv} from "process" +import {loadMigrationFiles} from "../files-loader" + +async function main(args: Array) { + const directory = args[0] + + await loadMigrationFiles(directory, (x) => console.error(x)) +} + +main(argv.slice(2)).catch((e) => { + console.error(`ERROR: ${e.message}`) + process.exit(1) +}) diff --git a/src/files-loader.ts b/src/files-loader.ts index 67592f1..013dd2f 100644 --- a/src/files-loader.ts +++ b/src/files-loader.ts @@ -1,35 +1,49 @@ import * as fs from "fs" import * as path from "path" import {promisify} from "util" -import {load as loadMigrationFile} from "./migration-file" +import {loadMigrationFile} from "./migration-file" import {Logger, Migration} from "./types" +import {validateMigrationOrdering} from "./validation" const readDir = promisify(fs.readdir) const isValidFile = (fileName: string) => /\.(sql|js)$/gi.test(fileName) -export const load = async ( +/** + * Load the migration files and assert they are reasonably valid. + * + * 'Reasonably valid' in this case means obeying the file name and + * consecutive ordering rules. + * + * No assertions are made about the validity of the SQL. + */ +export const loadMigrationFiles = async ( directory: string, - log: Logger, + // tslint:disable-next-line no-empty + log: Logger = () => {}, ): Promise> => { log(`Loading migrations from: ${directory}`) const fileNames = await readDir(directory) log(`Found migration files: ${fileNames}`) - if (fileNames != null) { - const migrationFiles = [ - path.join(__dirname, "migrations/0_create-migrations-table.sql"), - ...fileNames.map((fileName) => path.resolve(directory, fileName)), - ].filter(isValidFile) + if (fileNames == null) { + return [] + } - const unorderedMigrations = await Promise.all( - migrationFiles.map(loadMigrationFile), - ) + const migrationFiles = [ + path.join(__dirname, "migrations/0_create-migrations-table.sql"), + ...fileNames.map((fileName) => path.resolve(directory, fileName)), + ].filter(isValidFile) - // Arrange in ID order - return unorderedMigrations.sort((a, b) => a.id - b.id) - } + const unorderedMigrations = await Promise.all( + migrationFiles.map(loadMigrationFile), + ) + + // Arrange in ID order + const orderedMigrations = unorderedMigrations.sort((a, b) => a.id - b.id) + + validateMigrationOrdering(orderedMigrations) - return [] + return orderedMigrations } diff --git a/src/index.ts b/src/index.ts index 4f38b31..75eb9fe 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,6 @@ export {createDb} from "./create" export {migrate} from "./migrate" +export {loadMigrationFiles} from "./files-loader" export { ConnectionParams, diff --git a/src/migrate.ts b/src/migrate.ts index 414c99f..59846de 100644 --- a/src/migrate.ts +++ b/src/migrate.ts @@ -1,6 +1,6 @@ import * as pg from "pg" import SQL from "sql-template-strings" -import {load} from "./files-loader" +import {loadMigrationFiles} from "./files-loader" import {runMigration} from "./run-migration" import { BasicPgClient, @@ -10,6 +10,7 @@ import { Migration, MigrationError, } from "./types" +import {validateMigrationHashes} from "./validation" import {withConnection} from "./with-connection" import {withAdvisoryLock} from "./with-lock" @@ -32,7 +33,7 @@ export async function migrate( if (typeof migrationsDirectory !== "string") { throw new Error("Must pass migrations directory as a string") } - const intendedMigrations = await load(migrationsDirectory, log) + const intendedMigrations = await loadMigrationFiles(migrationsDirectory, log) if ("client" in dbConfig) { // we have been given a client to use, it should already be connected @@ -78,7 +79,7 @@ function runMigrations(intendedMigrations: Array, log: Logger) { log, ) - validateMigrations(intendedMigrations, appliedMigrations) + validateMigrationHashes(intendedMigrations, appliedMigrations) const migrationsToRun = filterMigrations( intendedMigrations, @@ -135,36 +136,6 @@ so the database is new and we need to run all migrations.`) return appliedMigrations } -/** Validates mutation order and hash */ -function validateMigrations( - migrations: Array, - appliedMigrations: Record, -) { - const indexNotMatch = (migration: Migration, index: number) => - migration.id !== index - const invalidHash = (migration: Migration) => { - const appliedMigration = appliedMigrations[migration.id] - return appliedMigration != null && appliedMigration.hash !== migration.hash - } - - // Assert migration IDs are consecutive integers - const notMatchingId = migrations.find(indexNotMatch) - if (notMatchingId) { - throw new Error( - `Found a non-consecutive migration ID on file: '${notMatchingId.fileName}'`, - ) - } - - // Assert migration hashes are still same - const invalidHashes = migrations.filter(invalidHash) - if (invalidHashes.length > 0) { - // Someone has altered one or more migrations which has already run - gasp! - const invalidFiles = invalidHashes.map(({fileName}) => fileName) - throw new Error(`Hashes don't match for migrations '${invalidFiles}'. -This means that the scripts have changed since it was applied.`) - } -} - /** Work out which migrations to apply */ function filterMigrations( migrations: Array, diff --git a/src/migration-file.ts b/src/migration-file.ts index 8df62ff..a766b57 100644 --- a/src/migration-file.ts +++ b/src/migration-file.ts @@ -31,7 +31,7 @@ const getSqlStringLiteral = ( } } -export const load = async (filePath: string) => { +export const loadMigrationFile = async (filePath: string) => { const fileName = getFileName(filePath) try { @@ -49,9 +49,6 @@ export const load = async (filePath: string) => { sql, } } catch (err) { - throw new Error(`${err.message} -Offending file: '${fileName}'.`) + throw new Error(`${err.message} - Offending file: '${fileName}'.`) } } - -// module.exports._fileNameParser = fileNameParser diff --git a/src/validation.ts b/src/validation.ts new file mode 100644 index 0000000..e088ada --- /dev/null +++ b/src/validation.ts @@ -0,0 +1,34 @@ +import {Migration} from "./types" + +const indexNotMatch = (migration: Migration, index: number) => + migration.id !== index + +/** Assert migration IDs are consecutive integers */ +export function validateMigrationOrdering(migrations: Array) { + const notMatchingId = migrations.find(indexNotMatch) + if (notMatchingId) { + throw new Error( + `Found a non-consecutive migration ID on file: '${notMatchingId.fileName}'`, + ) + } +} + +/** Assert hashes match */ +export function validateMigrationHashes( + migrations: Array, + appliedMigrations: Record, +) { + const invalidHash = (migration: Migration) => { + const appliedMigration = appliedMigrations[migration.id] + return appliedMigration != null && appliedMigration.hash !== migration.hash + } + + // Assert migration hashes are still same + const invalidHashes = migrations.filter(invalidHash) + if (invalidHashes.length > 0) { + // Someone has altered one or more migrations which has already run - gasp! + const invalidFiles = invalidHashes.map(({fileName}) => fileName) + throw new Error(`Hashes don't match for migrations '${invalidFiles}'. +This means that the scripts have changed since it was applied.`) + } +}