From 9e68864498c265df7e24b9397c25ab887d2690db Mon Sep 17 00:00:00 2001 From: 3m1n3nc3 Date: Sun, 5 Jul 2026 09:18:51 +0100 Subject: [PATCH] fix(migrations): don't replay DB.raw side effects during plan collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A migration's up()/down() is invoked both to gather its schema operations and, when it uses DB.raw() directly, as a real side effect. Several flows re-invoke up() purely to (re)build column-mapping / feature metadata: MigrateCommand's per-migration plan, and syncPersistedColumnMappingsFromState / validatePersistedMetadataFeaturesForMigrations. On rollback, sync re-planned every *still-applied* migration's up(), replaying its raw SQL (e.g. an ADD CONSTRAINT) against a schema it was never meant to touch again — throwing 'already exists' and leaving state half-updated so later commands also failed. migrate survived only because the raw ran twice but idempotently. - Add a migration-planning context: while active, DB.raw is a no-op. It is anchored on globalThis via Symbol.for so the CLI bundle (dist/cli.mjs) and the library bundle (dist/index.mjs) share one AsyncLocalStorage — otherwise the flag would not cross from getMigrationPlan() to the migration's DB.raw(). - getMigrationPlan(migration, direction, { inert }) runs the body inertly. - Planning-only callers pass inert: MigrateCommand's plan step, column-mapping rebuild, and feature validation. Actual apply/rollback stay non-inert, so the raw runs exactly once. Verified end to end against a scratch database: migrate runs the raw once, and rollback of a later batch no longer replays a still-applied migration's raw SQL. Tests: DB.raw runs on normal planning but is suppressed under inert planning while operations are still collected. --- src/DB.ts | 5 +++ src/cli/commands/MigrateCommand.ts | 5 ++- src/helpers/column-mappings.ts | 4 +- src/helpers/migration-planning.ts | 41 +++++++++++++++++ src/helpers/migrations.ts | 13 +++++- tests/base/migration-planning.spec.ts | 63 +++++++++++++++++++++++++++ 6 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 src/helpers/migration-planning.ts create mode 100644 tests/base/migration-planning.spec.ts diff --git a/src/DB.ts b/src/DB.ts index 9a2598a..40488e2 100644 --- a/src/DB.ts +++ b/src/DB.ts @@ -7,6 +7,7 @@ import { import { getRuntimeAdapter, getUserConfig } from './helpers/runtime-config' import { ArkormCollection } from './Collection' +import { isMigrationPlanningActive } from './helpers/migration-planning' import { ArkormException } from './Exceptions/ArkormException' import type { DatabaseTableOptions } from './types/db' import type { ModelMetadata } from './types/metadata' @@ -71,6 +72,10 @@ export class DB { sql: string, bindings: unknown[] = [], ): Promise> { + // A migration invoked only to collect its schema plan must not re-execute its + // raw SQL side effects (see runInMigrationPlanning). + if (isMigrationPlanningActive()) return ArkormCollection.make([]) + const adapter = this.getAdapter() if (!adapter) throw new ArkormException('Raw queries require a configured database adapter.', { diff --git a/src/cli/commands/MigrateCommand.ts b/src/cli/commands/MigrateCommand.ts index e88e70e..6f03649 100644 --- a/src/cli/commands/MigrateCommand.ts +++ b/src/cli/commands/MigrateCommand.ts @@ -169,8 +169,11 @@ export class MigrateCommand extends Command { for (const [MigrationClassItem] of pending) { if (useDatabaseMigrations) { + // Planning-only: collect the migration's operations for column mappings + // without replaying its direct DB side effects (e.g. DB.raw). The actual + // side effects run once, in applyMigrationToDatabase below. const planned = await this.runWithDatabaseCreationRetry(adapter, () => - getMigrationPlan(MigrationClassItem, 'up'), + getMigrationPlan(MigrationClassItem, 'up', { inert: true }), ) if (!planned.ok) return diff --git a/src/helpers/column-mappings.ts b/src/helpers/column-mappings.ts index 8967b0e..29caf8a 100644 --- a/src/helpers/column-mappings.ts +++ b/src/helpers/column-mappings.ts @@ -627,7 +627,7 @@ export const rebuildPersistedColumnMappingsState = async ( ) } - const operations = await getMigrationPlan(migrationClass, 'up') + const operations = await getMigrationPlan(migrationClass, 'up', { inert: true }) nextState = applyOperationsToPersistedColumnMappingsState(nextState, operations, features) } @@ -659,7 +659,7 @@ export const validatePersistedMetadataFeaturesForMigrations = async ( let nextState = createEmptyPersistedColumnMappingsState() for (const [migrationClass] of migrations) { - const operations = await getMigrationPlan(migrationClass, 'up') + const operations = await getMigrationPlan(migrationClass, 'up', { inert: true }) nextState = applyOperationsToPersistedColumnMappingsState(nextState, operations, features) } } diff --git a/src/helpers/migration-planning.ts b/src/helpers/migration-planning.ts new file mode 100644 index 0000000..d110e0d --- /dev/null +++ b/src/helpers/migration-planning.ts @@ -0,0 +1,41 @@ +import { AsyncLocalStorage } from 'node:async_hooks' + +/** + * Tracks whether a migration body is currently being invoked purely to collect + * its schema plan (rather than to actually apply it). + * + * A migration's `up()`/`down()` is executed both to gather its {@link SchemaOperation}s + * and, when it uses helpers like `DB.raw()`, as real database side effects. Some + * flows (column-mapping sync, feature validation) re-invoke `up()` on already + * applied migrations solely to rebuild metadata — they must NOT re-run those side + * effects, or a rollback would replay every still-applied migration's raw SQL + * against a schema it was never meant to touch again. + * + * While this context is active, direct data-affecting `DB` calls become no-ops. + * Actual apply/rollback runs outside it, so side effects happen normally. + */ +// The CLI (`dist/cli.mjs`) and the library (`dist/index.mjs`) are separate +// bundles, so a plain module-scoped store would give each its own instance and +// the flag would not cross from the CLI's getMigrationPlan() to a migration's +// DB.raw() (imported from the library). Anchor a single instance on globalThis +// via the global symbol registry so every bundle shares it. +const STORAGE_KEY = Symbol.for('arkorm.migrationPlanningStorage') + +const globalScope = globalThis as typeof globalThis & { + [STORAGE_KEY]?: AsyncLocalStorage +} + +const migrationPlanningStorage: AsyncLocalStorage = + globalScope[STORAGE_KEY] ?? (globalScope[STORAGE_KEY] = new AsyncLocalStorage()) + +/** Runs `fn` in a side-effect-free migration-planning context. */ +export const runInMigrationPlanning = async ( + fn: () => Promise | TResult, +): Promise => { + return await migrationPlanningStorage.run(true, async () => await fn()) +} + +/** True when a migration body is being invoked only to collect its schema plan. */ +export const isMigrationPlanningActive = (): boolean => { + return migrationPlanningStorage.getStore() === true +} diff --git a/src/helpers/migrations.ts b/src/helpers/migrations.ts index c17e174..498b91f 100644 --- a/src/helpers/migrations.ts +++ b/src/helpers/migrations.ts @@ -20,6 +20,7 @@ import { existsSync, mkdirSync, writeFileSync } from 'node:fs' import { ArkormException } from '../Exceptions/ArkormException' import type { DatabaseAdapter } from 'src/types/adapter' import { SchemaBuilder } from '../database/SchemaBuilder' +import { runInMigrationPlanning } from './migration-planning' import { join } from 'node:path' import { readFileSync } from 'node:fs' import { spawnSync } from 'node:child_process' @@ -1046,12 +1047,20 @@ export const generateMigrationFile = ( export const getMigrationPlan = async ( migration: MigrationInstanceLike | (new () => MigrationInstanceLike), direction: 'up' | 'down' = 'up', + options: { inert?: boolean } = {}, ): Promise => { const instance = typeof migration === 'function' ? new migration() : migration const schema = new SchemaBuilder() - if (direction === 'up') await instance.up(schema) - else await instance.down(schema) + const invoke = async (): Promise => { + if (direction === 'up') await instance.up(schema) + else await instance.down(schema) + } + + // Planning-only callers (metadata sync/validation) run inert so a migration's + // direct DB side effects (e.g. DB.raw) are not replayed just to read its plan. + if (options.inert) await runInMigrationPlanning(invoke) + else await invoke() return schema.getOperations() } diff --git a/tests/base/migration-planning.spec.ts b/tests/base/migration-planning.spec.ts new file mode 100644 index 0000000..3f02c19 --- /dev/null +++ b/tests/base/migration-planning.spec.ts @@ -0,0 +1,63 @@ +import { DB, configureArkormRuntime, getMigrationPlan, resetArkormRuntimeForTests } from '../../src' +import { afterEach, describe, expect, it, vi } from 'vitest' + +class RawSideEffectMigration { + async up(schema: any) { + schema.createTable('banking_accounts', (table: any) => { + table.id() + table.integer('userId') + table.string('currency') + }) + // A direct raw side effect — the class of statement that broke rollback. + await DB.raw( + 'alter table banking_accounts add constraint banking_accounts_user_currency_unique unique (user_id, currency)', + ) + } + + async down(schema: any) { + schema.dropTable('banking_accounts') + } +} + +const configureSpyAdapter = () => { + const rawQuery = vi.fn(async () => [] as Record[]) + configureArkormRuntime(undefined, { adapter: { rawQuery } as never }) + + return rawQuery +} + +afterEach(() => { + resetArkormRuntimeForTests() +}) + +describe('migration planning side effects', () => { + it('runs a migration DB.raw when planning normally (apply path)', async () => { + const rawQuery = configureSpyAdapter() + + const operations = await getMigrationPlan(new RawSideEffectMigration(), 'up') + + expect(rawQuery).toHaveBeenCalledTimes(1) + expect(operations[0]).toMatchObject({ type: 'createTable', table: 'banking_accounts' }) + }) + + it('suppresses DB.raw during inert planning while still collecting operations', async () => { + const rawQuery = configureSpyAdapter() + + const operations = await getMigrationPlan(new RawSideEffectMigration(), 'up', { inert: true }) + + // The raw statement must NOT be replayed just to read the plan… + expect(rawQuery).not.toHaveBeenCalled() + // …but the schema operations are still gathered. + expect(operations[0]).toMatchObject({ type: 'createTable', table: 'banking_accounts' }) + }) + + it('leaves DB.raw active again after the inert plan completes', async () => { + const rawQuery = configureSpyAdapter() + + await getMigrationPlan(new RawSideEffectMigration(), 'up', { inert: true }) + expect(rawQuery).not.toHaveBeenCalled() + + await DB.raw('select 1') + expect(rawQuery).toHaveBeenCalledTimes(1) + }) +})