Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add migration lifecycle hooks #5541

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3f8e597
feat: add migration lifecycle hooks
timorthi Apr 12, 2023
2d53af4
test: add tests for migration lifecycle hooks
timorthi Apr 12, 2023
ffcc089
test: fix test setup
timorthi Apr 12, 2023
c822fac
fix: add missing await
timorthi Apr 18, 2023
999cef0
feat: add beforeAll/afterAll to up, down, rollback
timorthi Apr 18, 2023
5dafbbe
Merge branch 'master' of github.com:knex/knex into timorthi/issue2983…
timorthi Dec 19, 2023
9c53658
test: rename to count
timorthi Dec 19, 2023
81d19c1
test: create separate file for lifecycle hooks tests
timorthi Dec 19, 2023
0f45133
test: remove extraneous setup
timorthi Dec 19, 2023
d681c86
test: rename to count
timorthi Dec 19, 2023
e2bc612
test: add more comprehensive tests
timorthi Dec 19, 2023
25cd3bc
test: force clean slate before each test
timorthi Dec 19, 2023
5bb8d4e
feat: short circuit if no migrations to run
timorthi Dec 20, 2023
6f975bd
refactor: remove unnecessary asyncs
timorthi Dec 20, 2023
772caa1
refactor: remove unnecessary asyncs
timorthi Dec 20, 2023
c88ff9c
fix: run hooks and migrations only if there are pending migrations
timorthi Dec 20, 2023
6eaaf7f
Update test/integration2/migrate/migration-lifecycle-integration.spec.js
timorthi Dec 20, 2023
87016d4
test: always return error value
timorthi Dec 20, 2023
cce4371
test: simplify expression
timorthi Dec 20, 2023
30b8711
feat: always pass list of migrations in hooks
timorthi Dec 20, 2023
cc2680b
feat: add types for lifecycle hooks
timorthi Dec 20, 2023
ce0ace0
Merge branch 'timorthi/issue2983-migration-lifecycle-hooks' of github…
timorthi Dec 20, 2023
f7a3147
refactor: use brackets to denote array
timorthi Dec 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 31 additions & 18 deletions lib/migrations/migrate/Migrator.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,18 @@ class Migrator {

let batchNo = await this._latestBatchNumber(trx);
if (direction === 'up') batchNo++;
const res = await this._waterfallBatch(
batchNo,
migrations,
direction,
trx
);

// Run any hooks before/after this batch
const beforeAll = this.config.beforeAll || (() => {});
const afterAll = this.config.afterAll || (() => {});

let res = [];
if (migrations.length > 0) {
await beforeAll(trx || this.knex);
res = await this._waterfallBatch(batchNo, migrations, direction, trx);
await afterAll(trx || this.knex);
}

await this._freeLock(canGetLockInTransaction ? trx : undefined);
return res;
} catch (error) {
Expand Down Expand Up @@ -496,30 +502,40 @@ class Migrator {
const migrationContent =
this.config.migrationSource.getMigration(migration);

const beforeEach = this.config.beforeEach || (() => {});
const afterEach = this.config.afterEach || (() => {});

// We're going to run each of the migrations in the current "up".
current = current
.then(async () => await migrationContent) //maybe promise
.then((migrationContent) => {
.then(async (migrationContent) => {
this._activeMigration.fileName = name;
if (
!trx &&
this._useTransaction(migrationContent, disableTransactions)
) {
this.knex.enableProcessing();
return this._transaction(
this.knex,
migrationContent,
direction,
name
);
return await this.knex.transaction(async (trx) => {
await beforeEach(trx, migration);
const migrationResult = await checkPromise(
this.knex.client.logger,
migrationContent[direction](trx),
name
);
await afterEach(trx, migration);
return migrationResult;
});
}

trxOrKnex.enableProcessing();
return checkPromise(
await beforeEach(trxOrKnex, migration);
const migrationResult = await checkPromise(
this.knex.client.logger,
migrationContent[direction](trxOrKnex),
name
);
await afterEach(trxOrKnex, migration);
return migrationResult;
})
.then(() => {
trxOrKnex.disableProcessing();
Expand Down Expand Up @@ -584,12 +600,9 @@ function getNewMigrations(migrationSource, all, completed) {
});
}

function checkPromise(logger, migrationPromise, name, commitFn) {
function checkPromise(logger, migrationPromise, name) {
if (!migrationPromise || typeof migrationPromise.then !== 'function') {
logger.warn(`migration ${name} did not return a promise`);
if (commitFn) {
commitFn();
}
Comment on lines -590 to -592
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the explicit commit so that we can run the afterEach hook in the same transaction (line 527)

}
return migrationPromise;
}
Expand Down
303 changes: 303 additions & 0 deletions test/integration2/migrate/migration-lifecycle-integration.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,303 @@
'use strict';
const sinon = require('sinon');
const chai = require('chai');
chai.use(require('chai-as-promised'));
const { expect } = chai;

const path = require('path');
const rimraf = require('rimraf');
const logger = require('../../integration/logger');
const { getAllDbs, getKnexForDb } = require('../util/knex-instance-provider');

describe('Migrations Lifecycle Hooks', function () {
getAllDbs().forEach((db) => {
describe(db, () => {
let knex;

// Force clean slate before each test
beforeEach(async () => {
rimraf.sync(path.join(__dirname, './migration'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure - this setup block was copied from migration-integration.spec.js and I chose not to change anything.

knex = logger(getKnexForDb(db));
// make sure lock was not left from previous failed test run
await knex.schema.dropTableIfExists('knex_migrations');
await knex.schema.dropTableIfExists('migration_test_1');
await knex.schema.dropTableIfExists('migration_test_2');
await knex.schema.dropTableIfExists('migration_test_2_1');
await knex.migrate.forceFreeMigrationsLock({
directory: 'test/integration2/migrate/test',
});
});

describe('knex.migrate.latest', function () {
describe('beforeAll', function () {
it('runs before the migrations batch', async function () {
let count;
await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
beforeAll: async (knexOrTrx) => {
const data = await knexOrTrx('knex_migrations').select('*');
count = data.length;
},
});

const data = await knex('knex_migrations').select('*');
expect(data.length).to.equal(count + 2);
});

it('does not run the migration or beforeEach/afterEach/afterAll hooks if it fails', async function () {
const beforeAll = sinon
.stub()
.throws(new Error('force beforeAll hook failure'));
const beforeEach = sinon.stub();
const afterEach = sinon.stub();
const afterAll = sinon.stub();

await knex.migrate
.latest({
directory: 'test/integration2/migrate/test',
beforeAll,
beforeEach,
afterEach,
afterAll,
})
.catch((error) => {
expect(error.message).to.equal('force beforeAll hook failure');
});

// Should not have run the migration
const hasTableCreatedByMigration = await knex.schema.hasTable(
'migration_test_1'
);
expect(hasTableCreatedByMigration).to.be.false;

// Should not have called the other hooks
expect(beforeEach.called).to.be.false;
expect(afterEach.called).to.be.false;
expect(afterAll.called).to.be.false;
});
});

describe('afterAll', function () {
it('runs after the migrations batch', async function () {
let count;
await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
afterAll: async (knexOrTrx) => {
const data = await knexOrTrx('knex_migrations').select('*');
count = data.length;
},
});

const data = await knex('knex_migrations').select('*');
expect(data.length).to.equal(count);
});

it('is not called if the migration fails', async function () {
const afterAll = sinon.stub();

await knex.migrate
.latest({
directory: 'test/integration2/migrate/test_with_invalid',
afterAll,
})
.catch(() => {});

expect(afterAll.called).to.be.false;
});
});

describe('beforeEach', function () {
it('runs before each migration', async function () {
const tableExistenceChecks = [];
const beforeEach = sinon.stub().callsFake(async (trx) => {
const hasFirstTestTable = await trx.schema.hasTable(
'migration_test_1'
);
const hasSecondTestTable = await trx.schema.hasTable(
'migration_test_2'
);
tableExistenceChecks.push({
hasFirstTestTable,
hasSecondTestTable,
});
});

await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
beforeEach,
});

expect(beforeEach.callCount).to.equal(2);
expect(tableExistenceChecks).to.deep.equal([
{
hasFirstTestTable: false,
hasSecondTestTable: false,
},
{
hasFirstTestTable: true,
hasSecondTestTable: false,
},
]);
});

it('does not run the migration or the afterEach/afterAll hooks if the hook fails', async function () {
timorthi marked this conversation as resolved.
Show resolved Hide resolved
const beforeEach = sinon
.stub()
.throws(new Error('force beforeEach hook failure'));
const afterEach = sinon.stub();
const afterAll = sinon.stub();

await knex.migrate
.latest({
directory: 'test/integration2/migrate/test',
beforeEach,
afterEach,
afterAll,
})
.catch((error) => {
expect(error.message).to.equal('force beforeEach hook failure');
timorthi marked this conversation as resolved.
Show resolved Hide resolved
});

// Should not have run the migration
const hasTableCreatedByMigration = await knex.schema.hasTable(
'migration_test_1'
);
expect(hasTableCreatedByMigration).to.be.false;

// Should not have called the after hooks
expect(afterEach.called).to.be.false;
expect(afterAll.called).to.be.false;
});
});

describe('afterEach', function () {
it('runs after each migration', async function () {
const tableExistenceChecks = [];
const afterEach = sinon.stub().callsFake(async (trx) => {
const hasFirstTestTable = await trx.schema.hasTable(
'migration_test_1'
);
const hasSecondTestTable = await trx.schema.hasTable(
'migration_test_2'
);
tableExistenceChecks.push({
hasFirstTestTable,
hasSecondTestTable,
});
});

await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
afterEach,
});

expect(afterEach.callCount).to.equal(2);
expect(tableExistenceChecks).to.deep.equal([
{
hasFirstTestTable: true,
hasSecondTestTable: false,
},
{
hasFirstTestTable: true,
hasSecondTestTable: true,
},
]);
});

it('is not called after a migration fails', async function () {
const afterEach = sinon.stub();

await knex.migrate
.latest({
directory: 'test/integration2/migrate/test_with_invalid',
afterEach,
})
.catch(() => {});

// The afterEach hook should have run for the first two successful migrations, but
// not after failed third migration
expect(afterEach.callCount).to.equal(2);
expect(
afterEach.args.map(([_knex, args]) => {
return args.file;
})
).to.deep.equal([
'20131019235242_migration_1.js',
'20131019235306_migration_2.js',
]);
});

it('does not run the afterAll hook if the hook fails', async function () {
const afterEach = sinon
.stub()
.throws(new Error('force afterEach hook failure'));
const afterAll = sinon.stub();

await knex.migrate
.latest({
directory: 'test/integration2/migrate/test',
afterEach,
afterAll,
})
.catch((error) => {
expect(error.message).to.equal('force afterEach hook failure');
});

expect(afterAll.called).to.be.false;
});
});

describe('execution order', function () {
it('runs in the expected order of beforeAll -> beforeEach -> afterEach -> afterAll', async function () {
const order = [];

await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
beforeAll: () => order.push('beforeAll'),
beforeEach: (_knex, { file }) => order.push(`beforeEach-${file}`),
afterEach: (_knex, { file }) => order.push(`afterEach-${file}`),
afterAll: () => order.push('afterAll'),
});

expect(order).to.deep.equal([
'beforeAll',
'beforeEach-20131019235242_migration_1.js',
'afterEach-20131019235242_migration_1.js',
'beforeEach-20131019235306_migration_2.js',
'afterEach-20131019235306_migration_2.js',
'afterAll',
]);
});
});

describe('when there are no pending migrations', function () {
it('does not run any of the hooks', async function () {
// Fire the migrations once to get the DB up to date
await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
});

// Now there should not be any pending migrations
const beforeAll = sinon.stub();
const beforeEach = sinon.stub();
const afterEach = sinon.stub();
const afterAll = sinon.stub();

await knex.migrate.latest({
directory: 'test/integration2/migrate/test',
beforeAll,
beforeEach,
afterEach,
afterAll,
});

expect(beforeAll.called).to.be.false;
expect(beforeEach.called).to.be.false;
expect(afterEach.called).to.be.false;
expect(afterAll.called).to.be.false;
});
});
});
});
});
});
Loading