diff --git a/README.md b/README.md index c9e8e8d4..64016dbc 100644 --- a/README.md +++ b/README.md @@ -532,6 +532,26 @@ The [`FileLocker` class](./src/file-locker.ts) uses `beforeAll` and `afterAll` t All events are type-safe, so IDEs will prevent typos and supply strong types for the event payloads. +### Errors + +When a migration throws an error, it will be wrapped in a `MigrationError` which captures the migration metadata (name, path etc.) as well as the original error message, and _will be rethrown_. In most cases, this is expected behaviour, and doesn't require any special handling beyond standard error logging setups. + +If you expect failures and want to try to recover from them, you will need to try-catch the call to `umzug.up()`. You can access the original error from the `.cause` property if necessary: + +```js +try { + await umzug.up(); +} catch (e) { + if (e instanceof MigrationError) { + const original = e.cause; + // do something with the original error here + } + throw e; +} +``` + +Under the hood, [verror](https://npmjs.com/package/verror) is used to wrap errors. + ### CLI 🚧🚧🚧 The CLI is new to Umzug v3 beta and is not yet stable. Feedback on it is welcome in [discussions](https://github.com/sequelize/umzug/discussions) 🚧🚧🚧 diff --git a/package-lock.json b/package-lock.json index 5b062f03..827f7cb5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1009,6 +1009,11 @@ "integrity": "sha512-eQ9qFW/fhfGJF8WKHGEHZEyVWfZxrT+6CLIJGBcZPfxUh/+BnEj+UCGYMlr9qZuX/2AltsvwrGqp0LhEW8D0zQ==", "dev": true }, + "@types/verror": { + "version": "1.10.4", + "resolved": "https://registry.npmjs.org/@types/verror/-/verror-1.10.4.tgz", + "integrity": "sha512-OjJdqx6QlbyZw9LShPwRW+Kmiegeg3eWNI41MQQKaG3vjdU2L9SRElntM51HmHBY1cu7izxQJ1lMYioQh3XMBg==" + }, "@types/yargs": { "version": "15.0.4", "resolved": "https://registry.npmjs.org/@types/yargs/-/yargs-15.0.4.tgz", @@ -1556,8 +1561,7 @@ "assert-plus": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/assert-plus/-/assert-plus-1.0.0.tgz", - "integrity": "sha1-8S4PPF13sLHN2RRpQuTpbB5N1SU=", - "dev": true + "integrity": "sha1-8S4PPF13sLHN2RRpQuTpbB5N1SU=" }, "assign-symbols": { "version": "1.0.0", @@ -2304,8 +2308,7 @@ "core-util-is": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/core-util-is/-/core-util-is-1.0.2.tgz", - "integrity": "sha1-tf1UIgqivFq1eqtxQMlAdUUDwac=", - "dev": true + "integrity": "sha1-tf1UIgqivFq1eqtxQMlAdUUDwac=" }, "cosmiconfig": { "version": "7.0.0", @@ -3637,8 +3640,7 @@ "extsprintf": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/extsprintf/-/extsprintf-1.3.0.tgz", - "integrity": "sha1-lpGEQOMEGnpBT4xS48V06zw+HgU=", - "dev": true + "integrity": "sha1-lpGEQOMEGnpBT4xS48V06zw+HgU=" }, "fast-deep-equal": { "version": "3.1.1", @@ -9533,7 +9535,6 @@ "version": "1.10.0", "resolved": "https://registry.npmjs.org/verror/-/verror-1.10.0.tgz", "integrity": "sha1-OhBcoXBTr1XW4nDB+CiGguGNpAA=", - "dev": true, "requires": { "assert-plus": "^1.0.0", "core-util-is": "1.0.2", diff --git a/package.json b/package.json index 151805cb..93c3d159 100644 --- a/package.json +++ b/package.json @@ -15,10 +15,12 @@ ], "dependencies": { "@rushstack/ts-command-line": "~4.7.7", + "@types/verror": "^1.10.4", "emittery": "~0.7.2", "fs-jetpack": "~4.0.0", "glob": "~7.1.6", - "type-fest": "~0.19.0" + "type-fest": "~0.19.0", + "verror": "^1.10.0" }, "devDependencies": { "@types/jest": "26.0.16", diff --git a/src/umzug.ts b/src/umzug.ts index 587e0ed7..c89922bc 100644 --- a/src/umzug.ts +++ b/src/umzug.ts @@ -6,6 +6,7 @@ import * as templates from './templates'; import * as glob from 'glob'; import { UmzugCLI } from './cli'; import * as emittery from 'emittery'; +import * as VError from 'verror'; import { MigrateDownOptions, MigrateUpOptions, @@ -21,6 +22,45 @@ import { const globAsync = promisify(glob); +interface MigrationErrorParams extends MigrationParams { + direction: 'up' | 'down'; +} + +export class Rethrowable extends VError { + static wrap(throwable: unknown): VError { + if (throwable instanceof VError) { + return throwable; + } + + if (throwable instanceof Error) { + return new VError(throwable, 'Original error'); + } + + return new VError( + { + info: { original: throwable }, + }, + `Non-error value thrown. See info for full props: %s`, + throwable + ); + } +} + +export class MigrationError extends VError { + constructor(migration: MigrationErrorParams, original: unknown) { + super( + { + cause: Rethrowable.wrap(original), + name: 'MigrationError', + info: migration, + }, + 'Migration %s (%s) failed', + migration.name, + migration.direction + ); + } +} + export class Umzug extends emittery.Typed> { private readonly storage: UmzugStorage; /** @internal */ @@ -99,7 +139,7 @@ export class Umzug extends emittery.Typed> { try { return require(filepath); } catch (e: unknown) { - if (e instanceof Error && filepath.endsWith('.ts')) { + if (e instanceof SyntaxError && filepath.endsWith('.ts')) { e.message += '\n\n' + languageSpecificHelp['.ts']; } @@ -237,7 +277,11 @@ export class Umzug extends emittery.Typed> { this.logging({ event: 'migrating', name: m.name }); await this.emit('migrating', params); - await m.up(params); + try { + await m.up(params); + } catch (e: unknown) { + throw new MigrationError({ direction: 'up', ...params }, e); + } await this.storage.logMigration(m.name, params); @@ -293,7 +337,11 @@ export class Umzug extends emittery.Typed> { this.logging({ event: 'reverting', name: m.name }); await this.emit('reverting', params); - await m.down?.(params); + try { + await m.down?.(params); + } catch (e: unknown) { + throw new MigrationError({ direction: 'down', ...params }, e); + } await this.storage.unlogMigration(m.name, params); diff --git a/test/umzug.test.ts b/test/umzug.test.ts index 47099c95..19eecd8d 100644 --- a/test/umzug.test.ts +++ b/test/umzug.test.ts @@ -2,6 +2,7 @@ import { memoryStorage, RerunBehavior, Umzug } from '../src'; import * as path from 'path'; import { fsSyncer } from 'fs-syncer'; import { expectTypeOf } from 'expect-type'; +import * as VError from 'verror'; jest.mock('../src/storage', () => { const storage = jest.requireActual('../src/storage'); @@ -371,10 +372,105 @@ describe('alternate migration inputs', () => { expect(spy).toHaveBeenNthCalledWith(1, 'migration1-up'); }); + test('errors are wrapped helpfully', async () => { + // Raw errors usually won't tell you which migration threw. This test ensures umzug adds that information. + const umzug = new Umzug({ + migrations: [ + { + name: 'm1', + up: async () => {}, + down: async () => Promise.reject(new Error('Some cryptic failure')), + }, + { + name: 'm2', + up: async () => Promise.reject(new Error('Some cryptic failure')), + down: async () => {}, + }, + ], + logger: undefined, + }); + + await expect(umzug.up()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m2 (up) failed: Original error: Some cryptic failure"` + ); + + await expect(umzug.down()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m1 (down) failed: Original error: Some cryptic failure"` + ); + }); + + test('Error causes are propagated properly', async () => { + const umzug = new Umzug({ + migrations: [ + { + name: 'm1', + up: async () => {}, + down: async () => Promise.reject(new VError({ info: { extra: 'detail' } }, 'Some cryptic failure')), + }, + { + name: 'm2', + up: async () => Promise.reject(new VError({ info: { extra: 'detail' } }, 'Some cryptic failure')), + down: async () => {}, + }, + ], + logger: undefined, + }); + + await expect(umzug.up()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m2 (up) failed: Some cryptic failure"` + ); + + // slightly weird format verror uses, not worth validating much more than that the `cause` is captured + await expect(umzug.up()).rejects.toMatchObject({ + jse_cause: { + jse_info: { extra: 'detail' }, + }, + }); + + await expect(umzug.down()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m1 (down) failed: Some cryptic failure"` + ); + + await expect(umzug.down()).rejects.toMatchObject({ + jse_cause: { + jse_info: { extra: 'detail' }, + }, + }); + }); + + test('non-error throwables are wrapped helpfully', async () => { + // Migration errors usually won't tell you which migration threw. This test ensures umzug adds that information. + const umzug = new Umzug({ + migrations: [ + { + name: 'm1', + up: async () => {}, + // eslint-disable-next-line prefer-promise-reject-errors + down: async () => Promise.reject('Some cryptic failure'), + }, + { + name: 'm2', + // eslint-disable-next-line prefer-promise-reject-errors + up: async () => Promise.reject('Some cryptic failure'), + down: async () => {}, + }, + ], + logger: undefined, + }); + + await expect(umzug.up()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m2 (up) failed: Non-error value thrown. See info for full props: Some cryptic failure"` + ); + + await expect(umzug.down()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Migration m1 (down) failed: Non-error value thrown. See info for full props: Some cryptic failure"` + ); + }); + test('typescript migration files', async () => { const syncer = fsSyncer(path.join(__dirname, 'generated/umzug/typescript'), { 'm1.ts': `export const up = () => {}; export const down = () => {}`, - 'm2.ts': `throw Error('Error to simulate typescript modules not being registered')`, + 'm2.ts': `throw SyntaxError('Fake syntax error to simulate typescript modules not being registered')`, }); syncer.sync(); @@ -392,7 +488,7 @@ describe('alternate migration inputs', () => { expect([names(await umzug.pending()), names(await umzug.executed())]).toEqual([['m2.ts'], ['m1.ts']]); await expect(umzug.up()).rejects.toThrowErrorMatchingInlineSnapshot(` - "Error to simulate typescript modules not being registered + "Migration m2.ts (up) failed: Original error: Fake syntax error to simulate typescript modules not being registered TypeScript files can be required by adding \`ts-node\` as a dependency and calling \`require('ts-node/register')\` at the program entrypoint before running migrations." `);