diff --git a/.changeset/recreate-warning.md b/.changeset/recreate-warning.md new file mode 100644 index 0000000..33b8bc0 --- /dev/null +++ b/.changeset/recreate-warning.md @@ -0,0 +1,5 @@ +--- +"chkit": patch +--- + +`chkit migrate` now flags a destructive table **recreate** (a `DROP TABLE` + `CREATE TABLE` caused by changing `engine`, `orderBy`, `primaryKey`, `partitionBy`, or `uniqueKey`) with a distinct `table_recreate_data_loss` warning instead of the generic `drop_table_data_loss`. The warning spells out that all rows are permanently deleted and the table is recreated empty, and recommends migrating via a temporary table to preserve data. Documented in the migrate and schema DSL reference pages. diff --git a/apps/docs/src/content/docs/cli/migrate.md b/apps/docs/src/content/docs/cli/migrate.md index a9583ee..407b65d 100644 --- a/apps/docs/src/content/docs/cli/migrate.md +++ b/apps/docs/src/content/docs/cli/migrate.md @@ -53,6 +53,7 @@ Each destructive operation includes a warning code, reason, impact description, | Warning code | Operation | |-------------|-----------| | `drop_table_data_loss` | `DROP TABLE` | +| `table_recreate_data_loss` | `DROP TABLE` + `CREATE TABLE` recreate from a structural change (`engine` / `orderBy` / `primaryKey` / `partitionBy` / `uniqueKey`) — all rows lost, table recreated empty | | `drop_column_irreversible` | `DROP COLUMN` | | `drop_view_dependency_break` | `DROP VIEW` / `DROP MATERIALIZED VIEW` | | `destructive_operation_review_required` | Other destructive operations | diff --git a/apps/docs/src/content/docs/schema/dsl-reference.md b/apps/docs/src/content/docs/schema/dsl-reference.md index b0c5cff..4e53afe 100644 --- a/apps/docs/src/content/docs/schema/dsl-reference.md +++ b/apps/docs/src/content/docs/schema/dsl-reference.md @@ -425,3 +425,7 @@ When a property changes, chkit determines whether the table can be altered in pl **Alterable** (ALTER in place): columns, indexes, projections, settings, TTL, comment Views and materialized views always use drop + recreate. + +:::danger +Changing a structural property on an existing table generates a `DROP TABLE` followed by `CREATE TABLE` — **all rows are permanently deleted and the table is recreated empty**. The data is not copied over. The drop is classified `risk=danger` (blocked without `--allow-destructive`) and `chkit migrate` flags it with the distinct `table_recreate_data_loss` warning. To preserve data, migrate by hand instead: create a new table with the desired structure, `INSERT INTO new SELECT ... FROM old`, then swap names and drop the old table. +::: diff --git a/packages/cli/src/runtime/safety-markers.ts b/packages/cli/src/runtime/safety-markers.ts index 39d1df1..3e23d52 100644 --- a/packages/cli/src/runtime/safety-markers.ts +++ b/packages/cli/src/runtime/safety-markers.ts @@ -98,12 +98,26 @@ export function extractMigrationOperationSummaries(sql: string): MigrationOperat return summaries } -function describeDestructiveOperation(type: string): { +function describeDestructiveOperation( + type: string, + opts: { recreate?: boolean } = {} +): { warningCode: string reason: string impact: string recommendation: string } { + if (type === 'drop_table' && opts.recreate) { + return { + warningCode: 'table_recreate_data_loss', + reason: + 'A change to engine, ORDER BY, PRIMARY KEY, PARTITION BY, or UNIQUE KEY can only be applied by dropping and recreating this table.', + impact: + 'ALL ROWS are permanently deleted — the table is recreated empty and existing data is NOT copied over.', + recommendation: + 'Back up the data first, or migrate via a temporary table (rename, INSERT ... SELECT, then drop) before approving.', + } + } if (type === 'drop_table') { return { warningCode: 'drop_table_data_loss', @@ -141,12 +155,22 @@ export function collectDestructiveOperationMarkers( migration: string, sql: string ): DestructiveOperationMarker[] { + // A table recreate emits a drop_table + create_table for the SAME key in the + // same migration (an engine/ORDER BY/PRIMARY KEY/PARTITION BY/UNIQUE KEY + // change). Flag those drops with a louder, distinct warning than a plain drop + // so the user knows all rows are lost and the table is recreated empty (#23). + const createdTableKeys = new Set( + extractMigrationOperationSummaries(sql) + .filter((op) => op.type === 'create_table') + .map((op) => op.key) + ) return extractDestructiveOperationSummaries(sql).map((summary) => { const parsed = parseOperationLine(summary, null) const type = parsed?.type ?? 'unknown' const key = parsed?.key ?? 'unknown' const risk = parsed?.risk ?? 'danger' - const detail = describeDestructiveOperation(type) + const recreate = type === 'drop_table' && createdTableKeys.has(key) + const detail = describeDestructiveOperation(type, { recreate }) return { migration, type, diff --git a/packages/cli/src/test/recreate-warning.e2e.test.ts b/packages/cli/src/test/recreate-warning.e2e.test.ts new file mode 100644 index 0000000..c658d42 --- /dev/null +++ b/packages/cli/src/test/recreate-warning.e2e.test.ts @@ -0,0 +1,97 @@ +import { 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' + +/** + * #23: a structural change (engine / ORDER BY / PRIMARY KEY / PARTITION BY / + * UNIQUE KEY) generates a DROP + CREATE that destroys all rows. The drop used + * to surface the same generic `drop_table_data_loss` warning as a deliberate + * drop. It now carries the distinct, louder `table_recreate_data_loss` warning. + * This run is BLOCKED (no --allow-destructive), so no data is actually dropped. + */ +describe('@chkit/cli table-recreate warning e2e (#23)', () => { + test('a structural change surfaces the distinct table_recreate_data_loss warning', async () => { + const liveEnv = getRequiredEnv() + const executor = createLiveExecutor(liveEnv) + const database = liveEnv.clickhouseDatabase + const journalTable = createJournalTableName('recreate') + const prefix = createPrefix('recreate') + const tableName = `${prefix}_t` + const cliEnv = { CHKIT_JOURNAL_TABLE: journalTable, CI: '1' } + + const dir = await mkdtemp(join(tmpdir(), 'chkit-recreate-')) + const config = join(dir, 'clickhouse.config.ts') + const schemaPath = join(dir, 'schema.ts') + + const renderSchema = (engine: string): string => + `import { schema, table } from '${CORE_ENTRY}'\n\n` + + `export default schema(table({\n` + + ` database: '${database}',\n` + + ` name: '${tableName}',\n` + + ` columns: [{ name: 'id', type: 'UInt64' }],\n` + + ` engine: '${engine}',\n` + + ` primaryKey: ['id'],\n` + + ` orderBy: ['id'],\n` + + `}))\n` + + try { + await writeFile(schemaPath, renderSchema('MergeTree()'), 'utf8') + await writeFile( + config, + `export default {\n` + + ` schema: '${schemaPath}',\n` + + ` outDir: '${join(dir, 'chkit')}',\n` + + ` migrationsDir: '${join(dir, 'chkit/migrations')}',\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', + ) + + const genInit = runCli(dir, ['generate', '--config', config, '--name', 'init', '--migration-id', '20990101000000', '--json'], cliEnv) + expect(genInit.exitCode).toBe(0) + const migrateInit = runCli(dir, ['migrate', '--config', config, '--execute', '--json'], cliEnv) + expect(migrateInit.exitCode).toBe(0) + await waitForTable(executor, database, tableName) + + // Structural change: engine MergeTree -> ReplacingMergeTree forces a + // DROP + CREATE recreate of the existing table. + await writeFile(schemaPath, renderSchema('ReplacingMergeTree()'), 'utf8') + const genRecreate = runCli(dir, ['generate', '--config', config, '--name', 'recreate', '--migration-id', '20990101000001', '--json'], cliEnv) + expect(genRecreate.exitCode).toBe(0) + + // Apply in CI without --allow-destructive: blocked (exit 3), and the + // blocked operation must carry the distinct recreate warning. + const migrate = runCli(dir, ['migrate', '--config', config, '--execute', '--json'], cliEnv) + expect(migrate.exitCode).toBe(3) + const payload = JSON.parse(migrate.stdout) as { + destructiveOperations?: Array<{ type: string; warningCode: string }> + } + const warningCodes = (payload.destructiveOperations ?? []).map((op) => op.warningCode) + expect(warningCodes).toContain('table_recreate_data_loss') + expect(warningCodes).not.toContain('drop_table_data_loss') + } finally { + await rm(dir, { recursive: true, force: true }) + await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(tableName)}`) + await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(journalTable)}`) + await executor.close() + } + }, 180_000) +}) diff --git a/packages/cli/src/test/runtime/safety-markers.test.ts b/packages/cli/src/test/runtime/safety-markers.test.ts index 9a343a3..9491717 100644 --- a/packages/cli/src/test/runtime/safety-markers.test.ts +++ b/packages/cli/src/test/runtime/safety-markers.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test' import { + collectDestructiveOperationMarkers, collectUnmarkedDestructiveStatements, extractExecutableStatements, extractMigrationOperationSummaries, @@ -203,3 +204,47 @@ describe('scanDestructiveSqlStatements (defense-in-depth for unmarked SQL)', () expect(marker?.summary).toContain('DROP COLUMN') }) }) + +describe('collectDestructiveOperationMarkers table-recreate warning (#23)', () => { + test('a drop+create of the same table gets the distinct recreate warning', () => { + // What the planner emits for an engine/ORDER BY/PRIMARY KEY/PARTITION BY/ + // UNIQUE KEY change: drop_table then create_table for the SAME key. + const sql = [ + '-- operation: drop_table key=table:default.events risk=danger', + 'DROP TABLE IF EXISTS default.events;', + '-- operation: create_table key=table:default.events risk=safe', + 'CREATE TABLE default.events (id UInt64) ENGINE = MergeTree ORDER BY id;', + ].join('\n') + + const markers = collectDestructiveOperationMarkers('20260101_recreate.sql', sql) + expect(markers).toHaveLength(1) + const marker = markers[0] + expect(marker?.type).toBe('drop_table') + expect(marker?.warningCode).toBe('table_recreate_data_loss') + expect(marker?.impact).toContain('ALL ROWS') + }) + + test('a plain drop (no matching create) keeps the generic drop warning', () => { + const sql = [ + '-- operation: drop_table key=table:default.old_events risk=danger', + 'DROP TABLE IF EXISTS default.old_events;', + ].join('\n') + + const markers = collectDestructiveOperationMarkers('20260101_drop.sql', sql) + expect(markers).toHaveLength(1) + expect(markers[0]?.warningCode).toBe('drop_table_data_loss') + }) + + test('dropping one table while creating a different one is not a recreate', () => { + const sql = [ + '-- operation: drop_table key=table:default.old_events risk=danger', + 'DROP TABLE IF EXISTS default.old_events;', + '-- operation: create_table key=table:default.new_events risk=safe', + 'CREATE TABLE default.new_events (id UInt64) ENGINE = MergeTree ORDER BY id;', + ].join('\n') + + const markers = collectDestructiveOperationMarkers('20260101_swap.sql', sql) + expect(markers).toHaveLength(1) + expect(markers[0]?.warningCode).toBe('drop_table_data_loss') + }) +})