From 4d2d757f7055538fde205d676bfeb741e49c57ae Mon Sep 17 00:00:00 2001 From: Misha Kaletsky <15040698+mmkal@users.noreply.github.com> Date: Tue, 15 Dec 2020 15:50:12 +0000 Subject: [PATCH] Create context per run (#419) This adds a "lazy" option to the `context` and `migrations` options, which allows for: 1) more customisation in child classes 2) it's a less dangerous flow if two call sites try to call `up` at the same time and do things like mutating `context`, which in theory is possible now Breaking change to the `beta` tag: `beforeAll`/`afterAll` are replaced by `beforeCommand`/`afterCommand`, which also run on `executed` and `pending` commands too now. For hooks that should only run before migrations are applied/reverted, use: ```js umzug.on('beforeCommand', ev => { if (ev.command !== 'up' && ev.command !== 'down') { return } // ...whatever you were doing in beforeAll/afterAll before }) ``` Easiest to view change without whitespace: https://github.com/sequelize/umzug/pull/419/files?w=1 --- README.md | 4 +- src/file-locker.ts | 6 +- src/types.ts | 10 +-- src/umzug.ts | 219 +++++++++++++++++++++++---------------------- test/umzug.test.ts | 26 ++++++ 5 files changed, 150 insertions(+), 115 deletions(-) diff --git a/README.md b/README.md index 64016dbc..66a9c38f 100644 --- a/README.md +++ b/README.md @@ -525,8 +525,8 @@ Umzug is an [emittery event emitter](https://www.npmjs.com/package/emittery). Ea These events run at the beginning and end of `up` and `down` calls. They'll receive an object containing a `context` property: -- `beforeAll` - Before any of the migrations are run. -- `afterAll` - After all the migrations have been executed. Note: this will always run, even if migrations throw an error. +- `beforeCommand` - Before any command (`'up' | 'down' | 'executed' | 'pending'`) is run. +- `afterCommand` - After any command (`'up' | 'down' | 'executed' | 'pending'`) is run. Note: this will always run, even if the command throws an error. The [`FileLocker` class](./src/file-locker.ts) uses `beforeAll` and `afterAll` to implement a simple filesystem-based locking mechanism. diff --git a/src/file-locker.ts b/src/file-locker.ts index de83d081..a7067b73 100644 --- a/src/file-locker.ts +++ b/src/file-locker.ts @@ -47,10 +47,10 @@ export class FileLocker { locker.attachTo(umzug); } - /** Attach `beforeAll` and `afterAll` events to an umzug instance */ + /** Attach lock handlers to `beforeCommand` and `afterCommand` events on an umzug instance */ attachTo(umzug: Umzug): void { - umzug.on('beforeAll', async () => this.getLock()); - umzug.on('afterAll', async () => this.releaseLock()); + umzug.on('beforeCommand', async () => this.getLock()); + umzug.on('afterCommand', async () => this.releaseLock()); } private async readFile(filepath: string): Promise { diff --git a/src/types.ts b/src/types.ts index 40e6c003..dfb4cf77 100644 --- a/src/types.ts +++ b/src/types.ts @@ -12,7 +12,7 @@ export type Promisable = T | PromiseLike; export type LogFn = (message: Record) => void; /** Constructor options for the Umzug class */ -export interface UmzugOptions { +export interface UmzugOptions> { /** The migrations that the Umzug instance should perform */ migrations: InputMigrations; /** A logging function. Pass `console` to use stdout, or pass in your own logger. Pass `undefined` explicitly to disable logging. */ @@ -20,7 +20,7 @@ export interface UmzugOptions { /** The storage implementation. By default, `JSONStorage` will be used */ storage?: UmzugStorage; /** An optional context object, which will be passed to each migration function, if defined */ - context?: Ctx; + context?: Ctx | (() => Ctx); /** Options for file creation */ create?: { /** @@ -83,7 +83,7 @@ export type GlobInputMigrations = { export type InputMigrations = | GlobInputMigrations | Array> - | ((context: T) => Promisable>>); + | ((context: T) => Promisable>); /** A function which takes a migration name, path and context, and returns an object with `up` and `down` functions. */ export type Resolver = (params: MigrationParams) => RunnableMigration; @@ -144,6 +144,6 @@ export interface UmzugEvents { migrated: MigrationParams; reverting: MigrationParams; reverted: MigrationParams; - beforeAll: { context: Ctx }; - afterAll: { context: Ctx }; + beforeCommand: { command: string; context: Ctx }; + afterCommand: { command: string; context: Ctx }; } diff --git a/src/umzug.ts b/src/umzug.ts index 6c303869..bdd91e52 100644 --- a/src/umzug.ts +++ b/src/umzug.ts @@ -8,6 +8,7 @@ import { CommandLineParserOptions, UmzugCLI } from './cli'; import * as emittery from 'emittery'; import * as VError from 'verror'; import { + InputMigrations, MigrateDownOptions, MigrateUpOptions, MigrationMeta, @@ -61,10 +62,10 @@ export class MigrationError extends VError { } } -export class Umzug extends emittery.Typed> { +export class Umzug extends emittery.Typed> { private readonly storage: UmzugStorage; /** @internal */ - readonly migrations: () => Promise>>; + readonly migrations: (ctx: Ctx) => Promise>>; /** * Compile-time only property for type inference. After creating an Umzug instance, it can be used as type alias for @@ -102,12 +103,7 @@ export class Umzug extends emittery.Typed> { super(); this.storage = verifyUmzugStorage(options.storage ?? new JSONStorage()); - this.migrations = this.getMigrationsResolver(); - } - - private get context(): Ctx { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- safe because options.context is only undefined if Ctx's type is undefined - return this.options.context!; + this.migrations = this.getMigrationsResolver(this.options.migrations); } private logging(message: Record) { @@ -186,8 +182,8 @@ export class Umzug extends emittery.Typed> { ): Umzug { return new Umzug({ ...this.options, - migrations: async () => { - const migrations = await this.migrations(); + migrations: async context => { + const migrations = await this.migrations(context); return transform(migrations); }, }); @@ -195,16 +191,18 @@ export class Umzug extends emittery.Typed> { /** Get the list of migrations which have already been applied */ async executed(): Promise { - const list = await this._executed(); - // We do the following to not expose the `up` and `down` functions to the user - return list.map(m => ({ name: m.name, path: m.path })); + return this.runCommand('executed', async ({ context }) => { + const list = await this._executed(context); + // We do the following to not expose the `up` and `down` functions to the user + return list.map(m => ({ name: m.name, path: m.path })); + }); } /** Get the list of migrations which have already been applied */ - private async _executed(): Promise>> { + private async _executed(context: Ctx): Promise>> { const [migrations, executedNames] = await Promise.all([ - this.migrations(), - this.storage.executed({ context: this.context }), + this.migrations(context), + this.storage.executed({ context }), ]); const executedSet = new Set(executedNames); return migrations.filter(m => executedSet.has(m.name)); @@ -212,26 +210,33 @@ export class Umzug extends emittery.Typed> { /** Get the list of migrations which are yet to be applied */ async pending(): Promise { - const list = await this._pending(); - // We do the following to not expose the `up` and `down` functions to the user - return list.map(m => ({ name: m.name, path: m.path })); + return this.runCommand('pending', async ({ context }) => { + const list = await this._pending(context); + // We do the following to not expose the `up` and `down` functions to the user + return list.map(m => ({ name: m.name, path: m.path })); + }); } - private async _pending(): Promise>> { + private async _pending(context: Ctx): Promise>> { const [migrations, executedNames] = await Promise.all([ - this.migrations(), - this.storage.executed({ context: this.context }), + this.migrations(context), + this.storage.executed({ context }), ]); const executedSet = new Set(executedNames); return migrations.filter(m => !executedSet.has(m.name)); } - private async withBeforeAfterHooks(cb: () => Promise): Promise { - await this.emit('beforeAll', { context: this.context }); + protected async runCommand(command: string, cb: (commandParams: { context: Ctx }) => Promise): Promise { + const context: Ctx = + typeof this.options.context === 'function' + ? (this.options.context as () => Ctx)() + : ((this.options.context ?? {}) as Ctx); + + await this.emit('beforeCommand', { command, context }); try { - return await cb(); + return await cb({ context }); } finally { - await this.emit('afterAll', { context: this.context }); + await this.emit('afterCommand', { command, context }); } } @@ -240,24 +245,24 @@ export class Umzug extends emittery.Typed> { * @see MigrateUpOptions for other use cases using `to`, `migrations` and `rerun`. */ async up(options: MigrateUpOptions = {}): Promise { - const eligibleMigrations = async () => { + const eligibleMigrations = async (context: Ctx) => { if (options.migrations && options.rerun === RerunBehavior.ALLOW) { // Allow rerun means the specified migrations should be run even if they've run before - so get all migrations, not just pending - const list = await this.migrations(); + const list = await this.migrations(context); return this.findMigrations(list, options.migrations); } if (options.migrations && options.rerun === RerunBehavior.SKIP) { - const executedNames = new Set((await this._executed()).map(m => m.name)); + const executedNames = new Set((await this._executed(context)).map(m => m.name)); const filteredMigrations = options.migrations.filter(m => !executedNames.has(m)); - return this.findMigrations(await this.migrations(), filteredMigrations); + return this.findMigrations(await this.migrations(context), filteredMigrations); } if (options.migrations) { - return this.findMigrations(await this._pending(), options.migrations); + return this.findMigrations(await this._pending(context), options.migrations); } - const allPending = await this._pending(); + const allPending = await this._pending(context); let sliceIndex = options.step ?? allPending.length; if (options.to) { @@ -267,12 +272,12 @@ export class Umzug extends emittery.Typed> { return allPending.slice(0, sliceIndex); }; - return this.withBeforeAfterHooks(async () => { - const toBeApplied = await eligibleMigrations(); + return this.runCommand('up', async ({ context }) => { + const toBeApplied = await eligibleMigrations(context); for (const m of toBeApplied) { const start = Date.now(); - const params: MigrationParams = { name: m.name, path: m.path, context: this.context }; + const params: MigrationParams = { name: m.name, path: m.path, context }; this.logging({ event: 'migrating', name: m.name }); await this.emit('migrating', params); @@ -299,23 +304,23 @@ export class Umzug extends emittery.Typed> { * @see MigrateDownOptions for other use cases using `to`, `migrations` and `rerun`. */ async down(options: MigrateDownOptions = {}): Promise { - const eligibleMigrations = async () => { + const eligibleMigrations = async (context: Ctx) => { if (options.migrations && options.rerun === RerunBehavior.ALLOW) { - const list = await this.migrations(); + const list = await this.migrations(context); return this.findMigrations(list, options.migrations); } if (options.migrations && options.rerun === RerunBehavior.SKIP) { - const pendingNames = new Set((await this._pending()).map(m => m.name)); + const pendingNames = new Set((await this._pending(context)).map(m => m.name)); const filteredMigrations = options.migrations.filter(m => !pendingNames.has(m)); - return this.findMigrations(await this.migrations(), filteredMigrations); + return this.findMigrations(await this.migrations(context), filteredMigrations); } if (options.migrations) { - return this.findMigrations(await this._executed(), options.migrations); + return this.findMigrations(await this._executed(context), options.migrations); } - const executedReversed = (await this._executed()).slice().reverse(); + const executedReversed = (await this._executed(context)).slice().reverse(); let sliceIndex = options.step ?? 1; if (options.to === 0 || options.migrations) { @@ -327,12 +332,12 @@ export class Umzug extends emittery.Typed> { return executedReversed.slice(0, sliceIndex); }; - return this.withBeforeAfterHooks(async () => { - const toBeReverted = await eligibleMigrations(); + return this.runCommand('down', async ({ context }) => { + const toBeReverted = await eligibleMigrations(context); for (const m of toBeReverted) { const start = Date.now(); - const params: MigrationParams = { name: m.name, path: m.path, context: this.context }; + const params: MigrationParams = { name: m.name, path: m.path, context }; this.logging({ event: 'reverting', name: m.name }); await this.emit('reverting', params); @@ -362,71 +367,73 @@ export class Umzug extends emittery.Typed> { allowConfusingOrdering?: boolean; skipVerify?: boolean; }): Promise { - const isoDate = new Date().toISOString(); - const prefixes = { - TIMESTAMP: isoDate.replace(/\.\d{3}Z$/, '').replace(/\W/g, '.'), - DATE: isoDate.split('T')[0].replace(/\W/g, '.'), - NONE: '', - }; - const prefixType = options.prefix ?? 'TIMESTAMP'; - const fileBasename = [prefixes[prefixType], options.name].filter(Boolean).join('.'); - - const allowedExtensions = options.allowExtension - ? [options.allowExtension] - : ['.js', '.cjs', '.mjs', '.ts', '.sql']; + await this.runCommand('create', async ({ context }) => { + const isoDate = new Date().toISOString(); + const prefixes = { + TIMESTAMP: isoDate.replace(/\.\d{3}Z$/, '').replace(/\W/g, '.'), + DATE: isoDate.split('T')[0].replace(/\W/g, '.'), + NONE: '', + }; + const prefixType = options.prefix ?? 'TIMESTAMP'; + const fileBasename = [prefixes[prefixType], options.name].filter(Boolean).join('.'); + + const allowedExtensions = options.allowExtension + ? [options.allowExtension] + : ['.js', '.cjs', '.mjs', '.ts', '.sql']; + + const existing = await this.migrations(context); + const last = existing[existing.length - 1]; + + const confusinglyOrdered = existing.find(e => e.path && path.basename(e.path) > fileBasename); + if (confusinglyOrdered && !options.allowConfusingOrdering) { + throw new Error( + `Can't create ${fileBasename}, since it's unclear if it should run before or after existing migration ${confusinglyOrdered.name}. Use allowConfusingOrdering to bypass this error.` + ); + } - const existing = await this.migrations(); - const last = existing[existing.length - 1]; + const folder = options.folder || this.options.create?.folder || (last?.path && path.dirname(last.path)); - const confusinglyOrdered = existing.find(e => e.path && path.basename(e.path) > fileBasename); - if (confusinglyOrdered && !options.allowConfusingOrdering) { - throw new Error( - `Can't create ${fileBasename}, since it's unclear if it should run before or after existing migration ${confusinglyOrdered.name}. Use allowConfusingOrdering to bypass this error.` - ); - } + if (!folder) { + throw new Error(`Couldn't infer a directory to generate migration file in. Pass folder explicitly`); + } - const folder = options.folder || this.options.create?.folder || (last?.path && path.dirname(last.path)); + const filepath = path.join(folder, fileBasename); - if (!folder) { - throw new Error(`Couldn't infer a directory to generate migration file in. Pass folder explicitly`); - } + const template = this.options.create?.template ?? Umzug.defaultCreationTemplate; - const filepath = path.join(folder, fileBasename); + const toWrite = template(filepath); + if (toWrite.length === 0) { + toWrite.push([filepath, '']); + } - const template = this.options.create?.template ?? Umzug.defaultCreationTemplate; + toWrite.forEach(pair => { + if (!Array.isArray(pair) || pair.length !== 2) { + throw new Error( + `Expected [filepath, content] pair. Check that the file template function returns an array of pairs.` + ); + } - const toWrite = template(filepath); - if (toWrite.length === 0) { - toWrite.push([filepath, '']); - } + const ext = path.extname(pair[0]); + if (!allowedExtensions.includes(ext)) { + const allowStr = allowedExtensions.join(', '); + const message = `Extension ${ext} not allowed. Allowed extensions are ${allowStr}. See help for allowExtension to avoid this error.`; + throw new Error(message); + } - toWrite.forEach(pair => { - if (!Array.isArray(pair) || pair.length !== 2) { - throw new Error( - `Expected [filepath, content] pair. Check that the file template function returns an array of pairs.` - ); - } + fs.mkdirSync(path.dirname(pair[0]), { recursive: true }); + fs.writeFileSync(pair[0], pair[1]); + this.logging({ event: 'created', path: pair[0] }); + }); - const ext = path.extname(pair[0]); - if (!allowedExtensions.includes(ext)) { - const allowStr = allowedExtensions.join(', '); - const message = `Extension ${ext} not allowed. Allowed extensions are ${allowStr}. See help for allowExtension to avoid this error.`; - throw new Error(message); + if (!options.skipVerify) { + const pending = await this.pending(); + if (!pending.some(p => p.path && path.resolve(p.path) === path.resolve(filepath))) { + throw new Error( + `Expected ${filepath} to be a pending migration but it wasn't! You should investigate this. Use skipVerify to bypass this error.` + ); + } } - - fs.mkdirSync(path.dirname(pair[0]), { recursive: true }); - fs.writeFileSync(pair[0], pair[1]); - this.logging({ event: 'created', path: pair[0] }); }); - - if (!options.skipVerify) { - const pending = await this.pending(); - if (!pending.some(p => p.path && path.resolve(p.path) === path.resolve(filepath))) { - throw new Error( - `Expected ${filepath} to be a pending migration but it wasn't! You should investigate this. Use skipVerify to bypass this error.` - ); - } - } } private static defaultCreationTemplate(filepath: string): Array<[string, string]> { @@ -476,17 +483,19 @@ export class Umzug extends emittery.Typed> { } /** helper for parsing input migrations into a callback returning a list of ready-to-run migrations */ - private getMigrationsResolver(): () => Promise>> { - const inputMigrations = this.options.migrations; - // Safe to non-null assert - if there's no context passed in, the type of `Ctx` must be `undefined` anyway. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const context = this.options.context!; + private getMigrationsResolver( + inputMigrations: InputMigrations + ): (ctx: Ctx) => Promise>> { if (Array.isArray(inputMigrations)) { return async () => inputMigrations; } if (typeof inputMigrations === 'function') { - return async () => inputMigrations(context); + // Lazy migrations definition, recurse. + return async ctx => { + const resolved = await inputMigrations(ctx); + return this.getMigrationsResolver(resolved)(ctx); + }; } const fileGlob = inputMigrations.glob; @@ -494,7 +503,7 @@ export class Umzug extends emittery.Typed> { const resolver: Resolver = inputMigrations.resolve ?? Umzug.defaultResolver; - return async () => { + return async context => { const paths = await globAsync(globString, { ...globOptions, absolute: true }); return paths.map(unresolvedPath => { const filepath = path.resolve(unresolvedPath); diff --git a/test/umzug.test.ts b/test/umzug.test.ts index 19eecd8d..c220a384 100644 --- a/test/umzug.test.ts +++ b/test/umzug.test.ts @@ -46,6 +46,30 @@ describe('basic usage', () => { }); }); +describe('custom context', () => { + test(`mutating context doesn't affect separate invocations`, async () => { + const spy = jest.fn(); + const umzug = new Umzug({ + migrations: [{ name: 'm1', up: spy }], + context: () => ({ counter: 0 }), + logger: undefined, + }); + + umzug.on('beforeCommand', ev => ev.context.counter++); + + await Promise.all([umzug.up(), umzug.up()]); + + expect(spy).toHaveBeenCalledTimes(2); + + expect(spy.mock.calls).toMatchObject([ + // because the context is lazy (returned by an inline function), both `up()` calls should + // get a fresh counter set to 0, which they each increment to 1 + [{ context: { counter: 1 } }], + [{ context: { counter: 1 } }], + ]); + }); +}); + describe('alternate migration inputs', () => { test('with file globbing', async () => { const spy = jest.fn(); @@ -608,10 +632,12 @@ describe('alternate migration inputs', () => { expect(spy).toHaveBeenNthCalledWith(1, { name: 'm1.sql', path: path.join(syncer.baseDir, 'directory1/m1.sql'), + context: {}, }); expect(spy).toHaveBeenNthCalledWith(2, { name: 'm2.sql', path: path.join(syncer.baseDir, 'deeply/nested/directory2/m2.sql'), + context: {}, }); }); });