diff --git a/.changeset/table-scope-unmarked.md b/.changeset/table-scope-unmarked.md new file mode 100644 index 0000000..a8a990e --- /dev/null +++ b/.changeset/table-scope-unmarked.md @@ -0,0 +1,5 @@ +--- +"chkit": patch +--- + +`migrate --table` no longer silently skips hand-written migrations that have no `-- operation:` markers. Their target tables can't be determined, so they are now fail-safe **included** (rather than dropped, which left pending work unapplied while appearing successful) and reported — with a warning in human output and an `undeterminedMigrations` array in `--json` output. diff --git a/apps/docs/src/content/docs/cli/migrate.md b/apps/docs/src/content/docs/cli/migrate.md index a9583ee..e0cdb06 100644 --- a/apps/docs/src/content/docs/cli/migrate.md +++ b/apps/docs/src/content/docs/cli/migrate.md @@ -75,6 +75,8 @@ The journal is written after each migration, not batched. The table is created i The `--table` flag filters pending migrations to those containing operations targeting the matched tables. Migration SQL files are parsed for `-- operation:` comment markers to determine which tables they affect. +A hand-written migration with no `-- operation:` markers has no determinable target tables. Rather than silently skip it (which would leave pending work unapplied while appearing successful), `--table` **includes** such migrations and prints a warning listing them. In `--json` mode they are reported in an `undeterminedMigrations` array. If you do not want a hand-written migration applied under a scoped run, add an `-- operation: key=table:. risk=` marker so its scope can be determined. + ### Plugin hooks The `onBeforeApply` plugin hook runs before each migration is executed and can transform the SQL statements. The `onAfterApply` hook runs after successful execution. diff --git a/packages/cli/src/commands/migrate/command.ts b/packages/cli/src/commands/migrate/command.ts index 35f3173..b25611e 100644 --- a/packages/cli/src/commands/migrate/command.ts +++ b/packages/cli/src/commands/migrate/command.ts @@ -111,9 +111,17 @@ async function cmdMigrate( return 0 } - const pending = tableScope.enabled - ? await filterPendingByScope(migrationsDir, pendingAll, new Set(tableScope.matchedTables)) - : pendingAll + let pending = pendingAll + let undeterminedScope: string[] = [] + if (tableScope.enabled) { + const scoped = await filterPendingByScope( + migrationsDir, + pendingAll, + new Set(tableScope.matchedTables), + ) + pending = scoped.inScope + undeterminedScope = scoped.undetermined + } if (pending.length === 0) { if (jsonMode) { @@ -125,7 +133,12 @@ async function cmdMigrate( } if (jsonMode && !executeRequested) { - emitJson('migrate', { mode, scope: tableScope, pending }) + emitJson('migrate', { + mode, + scope: tableScope, + pending, + ...(undeterminedScope.length > 0 ? { undeterminedMigrations: undeterminedScope } : {}), + }) return 0 } @@ -134,6 +147,13 @@ async function cmdMigrate( console.log(`Table scope: ${tableScope.selector ?? ''} (${tableScope.matchCount} matched)`) for (const table of tableScope.matchedTables) console.log(`- ${table}`) } + if (undeterminedScope.length > 0) { + console.log( + `⚠ ${undeterminedScope.length} pending migration(s) have no table markers; ` + + "including them because their target tables can't be determined under --table:", + ) + for (const file of undeterminedScope) console.log(` - ${file}`) + } console.log(`Pending migrations: ${pending.length}`) for (const file of pending) { console.log(`- ${file}`) @@ -217,7 +237,12 @@ async function cmdMigrate( } if (jsonMode) { - emitJson('migrate', { mode: 'execute', scope: tableScope, applied: appliedNow }) + emitJson('migrate', { + mode: 'execute', + scope: tableScope, + applied: appliedNow, + ...(undeterminedScope.length > 0 ? { undeterminedMigrations: undeterminedScope } : {}), + }) return 0 } diff --git a/packages/cli/src/commands/migrate/scope.ts b/packages/cli/src/commands/migrate/scope.ts index 283bf01..646b29c 100644 --- a/packages/cli/src/commands/migrate/scope.ts +++ b/packages/cli/src/commands/migrate/scope.ts @@ -7,19 +7,42 @@ import { tableKeyFromOperationKey, } from '../../runtime/table-scope.js' +export interface ScopeFilterResult { + /** Migrations to apply under the scope (matched + fail-safe-included). */ + inScope: string[] + /** + * Migrations included because their target tables could NOT be determined + * (no parseable `-- operation:` markers — e.g. hand-written migrations). + * Surfaced to the caller so it can warn instead of silently skipping them. + */ + undetermined: string[] +} + export async function filterPendingByScope( migrationsDir: string, pending: string[], selectedTables: ReadonlySet, -): Promise { +): Promise { const selectedDatabases = new Set( [...selectedTables].map((table) => table.split('.')[0] ?? ''), ) const inScope: string[] = [] + const undetermined: string[] = [] for (const file of pending) { const sql = await readFile(join(migrationsDir, file), 'utf8') const operations = extractMigrationOperationSummaries(sql) + + // A migration with no parseable operation markers (a hand-written one) has + // undeterminable target tables. Silently skipping it under --table is a + // quiet correctness gap (#36): the user believes all pending work applied. + // Fail-safe by including it, and report it so the caller can warn. + if (operations.length === 0) { + inScope.push(file) + undetermined.push(file) + continue + } + const matches = operations.some((operation) => { const tableKey = tableKeyFromOperationKey(operation.key) if (tableKey) return selectedTables.has(tableKey) @@ -33,5 +56,5 @@ export async function filterPendingByScope( if (matches) inScope.push(file) } - return inScope + return { inScope, undetermined } } diff --git a/packages/cli/src/test/table-scope-unmarked.e2e.test.ts b/packages/cli/src/test/table-scope-unmarked.e2e.test.ts new file mode 100644 index 0000000..45d2066 --- /dev/null +++ b/packages/cli/src/test/table-scope-unmarked.e2e.test.ts @@ -0,0 +1,106 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { describe, expect, test } from 'bun:test' + +import { + CORE_ENTRY, + createJournalTableName, + createLiveExecutor, + createPrefix, + getRequiredEnv, + quoteIdent, + runCli, + waitForTable, +} from './e2e-testkit.js' + +/** + * #36: `migrate --apply --table ` used to SILENTLY skip hand-written + * migrations that carry no operation markers (their target tables can't be + * determined), so the user believed all pending work was applied. The fix + * fail-safe includes them and reports them as undetermined. + */ +describe('@chkit/cli table-scope unmarked migrations e2e (#36)', () => { + test('an unmarked migration is applied under --table, not silently skipped', async () => { + const liveEnv = getRequiredEnv() + const executor = createLiveExecutor(liveEnv) + const database = liveEnv.clickhouseDatabase + const journalTable = createJournalTableName('scope_unmarked') + const prefix = createPrefix('scope_unmarked') + const usersTable = `${prefix}_users` + const handTable = `${prefix}_hand` + const cliEnv = { CHKIT_JOURNAL_TABLE: journalTable, CI: '1' } + + const dir = await mkdtemp(join(tmpdir(), 'chkit-scope-unmarked-')) + const config = join(dir, 'clickhouse.config.ts') + const migrationsDir = join(dir, 'chkit/migrations') + const schemaPath = join(dir, 'schema.ts') + + try { + await mkdir(migrationsDir, { recursive: true }) + await writeFile( + schemaPath, + `import { schema, table } from '${CORE_ENTRY}'\n\n` + + `export default schema(table({\n` + + ` database: '${database}',\n` + + ` name: '${usersTable}',\n` + + ` columns: [{ name: 'id', type: 'UInt64' }],\n` + + ` engine: 'MergeTree()',\n` + + ` primaryKey: ['id'],\n` + + ` orderBy: ['id'],\n` + + `}))\n`, + 'utf8', + ) + await writeFile( + config, + `export default {\n` + + ` schema: '${schemaPath}',\n` + + ` outDir: '${join(dir, 'chkit')}',\n` + + ` migrationsDir: '${migrationsDir}',\n` + + ` metaDir: '${join(dir, 'chkit/meta')}',\n` + + ` clickhouse: {\n` + + ` url: '${liveEnv.clickhouseUrl}',\n` + + ` username: '${liveEnv.clickhouseUser}',\n` + + ` password: '${liveEnv.clickhousePassword}',\n` + + ` database: '${database}',\n` + + ` },\n}\n`, + 'utf8', + ) + + // Generated migration (carries `-- operation:` markers for usersTable), + // and a hand-written one with NO markers — both pending. + const gen = runCli(dir, ['generate', '--config', config, '--name', 'init', '--migration-id', '20990101000000', '--json'], cliEnv) + expect(gen.exitCode).toBe(0) + const handMigration = '20990101000001_hand_written.sql' + await writeFile( + join(migrationsDir, handMigration), + `CREATE TABLE IF NOT EXISTS ${quoteIdent(database)}.${quoteIdent(handTable)} (id UInt64) ENGINE = MergeTree ORDER BY id;\n`, + 'utf8', + ) + + // Scope to the generated table. The hand-written migration has no markers, + // so its target table is undetermined — it must still be applied (and + // reported), not silently dropped. + const migrate = runCli(dir, ['migrate', '--config', config, '--execute', '--table', usersTable, '--json'], cliEnv) + expect(migrate.exitCode).toBe(0) + + const payload = JSON.parse(migrate.stdout) as { + applied?: Array<{ name: string }> + undeterminedMigrations?: string[] + } + expect(payload.undeterminedMigrations).toContain(handMigration) + expect((payload.applied ?? []).map((entry) => entry.name)).toContain(handMigration) + + // The real proof: the hand-written migration's table actually exists. + // Before the fix it would have been skipped and never created. + await waitForTable(executor, database, handTable) + } finally { + await rm(dir, { recursive: true, force: true }) + await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(usersTable)}`) + await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(handTable)}`) + await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(journalTable)}`) + await executor.close() + } + }, 180_000) +})