diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 6ef48254..9392187c 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -313,7 +313,18 @@ export async function runDbCommand( break; } case "rollback": { - const name = await rollback(db, migrationsDir); + let name: string | null; + try { + name = await rollback(db, migrationsDir); + } catch (error) { + // A migration with no down section is refused rather than half-undone. + // That is an operator decision to make, not a stack trace to read. + const message = error instanceof Error ? error.message : String(error); + if (!message.startsWith("WRN-DB-MIGRATION-IRREVERSIBLE")) throw error; + console.error(`✗ ${message.replace("WRN-DB-MIGRATION-IRREVERSIBLE: ", "")}`); + process.exitCode = 1; + break; + } console.log(name ? `✓ Rolled back ${name}${label}` : `Nothing to roll back${label}.`); break; } diff --git a/packages/db/src/migrate.ts b/packages/db/src/migrate.ts index aff7c0ee..4bff8ef6 100644 --- a/packages/db/src/migrate.ts +++ b/packages/db/src/migrate.ts @@ -231,7 +231,18 @@ export async function migrate( return applyMigrations(db, loadMigrations(dir), options); } -/** Roll back the most recently applied migration. Returns its name, or null. */ +/** + * Roll back the most recently applied migration. Returns its name, or null when + * nothing has been applied. + * + * Refuses a migration that changed the schema but defines no `-- +down` section. + * Removing its tracking row would leave the change in place and unrecorded, and + * the next `migrate` would replay statements such as `ALTER TABLE ... ADD COLUMN` + * that cannot run twice, stranding the database between the two states. + * + * A migration whose `up` is empty — a placeholder, or comments only — has + * nothing to undo, so it rolls back by dropping its tracking row alone. + */ export async function rollback( db: Db, dir: string, @@ -241,12 +252,21 @@ export async function rollback( const applied = await appliedMigrations(db); const last = applied[applied.length - 1]; if (!last) return null; + const migration = loadMigrations(dir).find((m) => m.name === last); + const reversible = + !migration || !hasExecutableSql(migration.up) || hasExecutableSql(migration.down); + if (!reversible) + throw new Error( + `WRN-DB-MIGRATION-IRREVERSIBLE: ${last} changes the schema but defines no "-- +down" section, ` + + "so rolling it back would drop its record while leaving the change applied. " + + "Add a `-- +down` section to that migration, or write a new migration that reverses it.", + ); + // Checked before the lock so a dry run reports the same refusal. if (options.dryRun) return last; const release = options.lock === false ? undefined : await acquireMigrationLock(db, options.lockTimeoutMs ?? 300_000); - const migration = loadMigrations(dir).find((m) => m.name === last); try { throwIfAborted(options.signal); await db.tx(async (tx) => { diff --git a/packages/db/test/db.test.ts b/packages/db/test/db.test.ts index 5ea5ae2c..51338118 100644 --- a/packages/db/test/db.test.ts +++ b/packages/db/test/db.test.ts @@ -145,6 +145,67 @@ test("an empty migration set does not touch the database", async () => { expect(await applyMigrations(db, [])).toEqual([]); }); +test("rollback refuses a schema change that defines no down section", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-nodown-")); + writeFileSync(join(dir, "0001_add.sql"), "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);"); + + const db = createDb(sqlite()); + expect(await migrate(db, dir)).toEqual(["0001_add"]); + + // Rolling this back would drop the record and leave the table, and the next + // migrate would replay a CREATE that cannot run twice. + await expect(rollback(db, dir)).rejects.toThrow(/WRN-DB-MIGRATION-IRREVERSIBLE/); + + // The refusal leaves the record alone, so the database stays consistent. + expect(await appliedMigrations(db)).toEqual(["0001_add"]); + expect((await status(db, dir))[0]!.applied).toBe(true); + db.close(); +}); + +test("a dry run reports the same refusal instead of promising a rollback", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-dry-")); + writeFileSync(join(dir, "0001_add.sql"), "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);"); + + const db = createDb(sqlite()); + await migrate(db, dir); + await expect(rollback(db, dir, { dryRun: true })).rejects.toThrow( + /WRN-DB-MIGRATION-IRREVERSIBLE/, + ); + db.close(); +}); + +test("rollback still undoes a migration that defines a down section", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-down-")); + writeFileSync( + join(dir, "0001_add.sql"), + "-- +up\nCREATE TABLE t (id INTEGER PRIMARY KEY);\n-- +down\nDROP TABLE t;", + ); + + const db = createDb(sqlite()); + await migrate(db, dir); + expect(await rollback(db, dir)).toBe("0001_add"); + expect(await appliedMigrations(db)).toEqual([]); + const remaining = await db.all<{ name: string }>( + "SELECT name FROM sqlite_master WHERE type='table' AND name='t'", + ); + expect(remaining.length).toBe(0); + db.close(); +}); + +test("a migration whose up is only comments has nothing to undo and rolls back", async () => { + const dir = mkdtempSync(join(tmpdir(), "wrn-mig-noop-")); + writeFileSync( + join(dir, "0001_placeholder.sql"), + "-- +up\n-- Nothing yet.\n-- +down\n-- Nothing to undo.", + ); + + const db = createDb(sqlite()); + await migrate(db, dir); + expect(await rollback(db, dir)).toBe("0001_placeholder"); + expect(await appliedMigrations(db)).toEqual([]); + db.close(); +}); + test("comment-only migrations are recorded without executing empty SQL", async () => { const db = createDb(sqlite()); const migrations = [