diff --git a/frontend/src/lib/components/DdlMigrationGuard.svelte b/frontend/src/lib/components/DdlMigrationGuard.svelte index ae5182f560..3bda6aa216 100644 --- a/frontend/src/lib/components/DdlMigrationGuard.svelte +++ b/frontend/src/lib/components/DdlMigrationGuard.svelte @@ -3,7 +3,7 @@ import Modal2 from './common/modal/Modal2.svelte' import NewDataTableMigrationModal from './workspaceSettings/NewDataTableMigrationModal.svelte' import DataTableMigrationsButton from './workspaceSettings/DataTableMigrationsButton.svelte' - import { splitSqlStatements, isDdlStatement } from './sqlDdl' + import { joinSqlStatements, splitSqlRuns } from './sqlDdl' import { logDdlGuardChoice } from './workspaceSettings/datatableTelemetry' import { CornerDownLeft } from 'lucide-svelte' @@ -11,7 +11,7 @@ type Choice = 'run' | 'migrate' | 'cancel' - let promptStatement = $state(undefined) + let promptStatements = $state([]) let promptOpen = $state(false) let resolvePrompt: ((choice: Choice) => void) | undefined = undefined let resolveMigrationClosed: ((created: boolean) => void) | undefined = undefined @@ -23,11 +23,15 @@ // toast action after a migration is created here. let migrationsModal = $state(undefined) + // The block is shown as-is in the prompt and becomes the migration body, where + // every statement inside the BEGIN; ... END; frame must be `;`-terminated. + let promptSql = $derived(joinSqlStatements(promptStatements)) + function finishPrompt(choice: Choice) { const r = resolvePrompt resolvePrompt = undefined promptOpen = false - promptStatement = undefined + promptStatements = [] r?.(choice) } @@ -49,10 +53,10 @@ } } - function promptDdl(statement: string): Promise { + function promptDdl(statements: string[]): Promise { return new Promise((resolve) => { resolvePrompt = resolve - promptStatement = statement + promptStatements = statements promptOpen = true }) } @@ -66,49 +70,51 @@ // Open the prefilled new-migration modal. Resolves with whether a migration // was actually created (false if the user cancelled / closed it). - function openMigrationModal(statement: string): Promise { + function openMigrationModal(sql: string): Promise { return new Promise((resolve) => { resolveMigrationClosed = (created: boolean) => resolve(created) - newMigrationModal?.open({ codeUp: statement }) + newMigrationModal?.open({ codeUp: sql }) }) } /** - * Inspect `code` for DDL statements. For each one, prompt the user to run it - * anyway or turn it into a migration (prompts shown one at a time). Returns - * whether to proceed and the code to run (with migrated statements stripped). + * Inspect `code` for DDL statements. Each run of adjacent DDL statements is + * prompted for once (runs shown one at a time) and becomes a single migration, + * so a chain of schema changes applies in one transaction instead of asking + * once per statement. Returns whether to proceed and the code to run (with + * migrated statements stripped). */ export async function guard( code: string ): Promise<{ proceed: boolean; code: string; ranMigration: boolean }> { migrationRan = false - const statements = splitSqlStatements(code) - if (!statements.some((s) => isDdlStatement(s))) { + const runs = splitSqlRuns(code) + if (!runs.some((r) => r.isDdl)) { return { proceed: true, code, ranMigration: false } } const kept: string[] = [] - for (const statement of statements) { - if (!isDdlStatement(statement)) { - kept.push(statement) + for (const run of runs) { + if (!run.isDdl) { + kept.push(...run.statements) continue } - // Re-prompt for this statement until the user makes a terminal choice; + // Re-prompt for this run until the user makes a terminal choice; // cancelling the migration modal returns to the prompt with the DDL intact. for (;;) { - const choice = await promptDdl(statement) + const choice = await promptDdl(run.statements) if (choice === 'cancel') { logDdlGuardChoice('cancelled') return { proceed: false, code, ranMigration: migrationRan } } if (choice === 'run') { logDdlGuardChoice('run_anyway') - kept.push(statement) + kept.push(...run.statements) break } - // migrate: only strip the statement once a migration is actually + // migrate: only strip the statements once a migration is actually // created; if the modal was cancelled, loop back to the prompt. - const created = await openMigrationModal(statement) + const created = await openMigrationModal(joinSqlStatements(run.statements)) if (created) { logDdlGuardChoice('migrated') break @@ -116,14 +122,14 @@ } } - return { proceed: true, code: kept.join(';\n'), ranMigration: migrationRan } + return { proceed: true, code: joinSqlStatements(kept), ranMigration: migrationRan } } 1 ? 'Schema changes detected' : 'Schema change detected'} fixedWidth="md" fixedHeight="adaptive" bind:isOpen={promptOpen} @@ -131,12 +137,17 @@ >

- This looks like a schema-changing (DDL) statement. Schema changes are best tracked as - migrations rather than run ad-hoc. Create a migration for it instead? + {#if promptStatements.length > 1} + These {promptStatements.length} consecutive statements are schema-changing (DDL). Schema changes + are best tracked as migrations rather than run ad-hoc. Create a single migration for them instead? + {:else} + This looks like a schema-changing (DDL) statement. Schema changes are best tracked as + migrations rather than run ad-hoc. Create a migration for it instead? + {/if} 

{promptStatement ?? ''}
{promptSql}
diff --git a/frontend/src/lib/components/sqlDdl.test.ts b/frontend/src/lib/components/sqlDdl.test.ts index 806435a809..7376692632 100644 --- a/frontend/src/lib/components/sqlDdl.test.ts +++ b/frontend/src/lib/components/sqlDdl.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from 'vitest' -import { endsWithUnterminatedStatement, splitSqlStatements, stripSqlComments } from './sqlDdl' +import { + endsWithUnterminatedStatement, + joinSqlStatements, + splitSqlRuns, + splitSqlStatements, + stripSqlComments +} from './sqlDdl' describe('endsWithUnterminatedStatement', () => { it('reports a missing terminator on the final statement', () => { @@ -67,3 +73,36 @@ describe('splitSqlStatements', () => { ]) }) }) + +describe('splitSqlRuns', () => { + it('groups adjacent DDL statements and breaks the run on a non-DDL one', () => { + expect( + splitSqlRuns( + 'CREATE TABLE a (id int); ALTER TABLE a ADD b int; INSERT INTO a VALUES (1); DROP TABLE a;' + ) + ).toEqual([ + { isDdl: true, statements: ['CREATE TABLE a (id int)', 'ALTER TABLE a ADD b int'] }, + { isDdl: false, statements: ['INSERT INTO a VALUES (1)'] }, + { isDdl: true, statements: ['DROP TABLE a'] } + ]) + }) + + it('keeps a leading comment with the statement it introduces', () => { + expect(splitSqlRuns('-- add it\nCREATE TABLE a (id int);')).toEqual([ + { isDdl: true, statements: ['-- add it\nCREATE TABLE a (id int)'] } + ]) + }) +}) + +describe('joinSqlStatements', () => { + it('keeps a terminator out of a statement-trailing line comment', () => { + expect(joinSqlStatements(['CREATE TABLE a (id int)', 'ALTER TABLE a ADD b int'])).toBe( + 'CREATE TABLE a (id int);\nALTER TABLE a ADD b int;' + ) + // A `;` appended after the comment would be commented out, gluing this + // statement onto the next one. + expect(joinSqlStatements(['CREATE TABLE a (id int) -- new', 'ALTER TABLE a ADD b int'])).toBe( + 'CREATE TABLE a (id int) -- new\n;\nALTER TABLE a ADD b int;' + ) + }) +}) diff --git a/frontend/src/lib/components/sqlDdl.ts b/frontend/src/lib/components/sqlDdl.ts index 21e566390f..c3c3e0c01c 100644 --- a/frontend/src/lib/components/sqlDdl.ts +++ b/frontend/src/lib/components/sqlDdl.ts @@ -258,3 +258,40 @@ export function isDdlStatement(statement: string): boolean { const firstWord = pruneComments(statement).trim().split(/\s+/)[0]?.toUpperCase() return !!firstWord && DDL_KEYWORDS.includes(firstWord) } + +export type SqlRun = { isDdl: boolean; statements: string[] } + +/** + * Join statements into one runnable block, `;`-terminating each. `splitSqlStatements` + * drops the `;` it splits on and keeps a statement's trailing comment, so a statement + * ending in a `--` comment would swallow a terminator appended on the same line and + * run into the next statement; that one gets its terminator on its own line. + */ +export function joinSqlStatements(statements: string[], backslashEscapes = true): string { + return statements + .map((s) => (endsWithUnterminatedStatement(`${s};`, backslashEscapes) ? `${s}\n;` : `${s};`)) + .join('\n') +} + +/** + * Split a script into alternating runs of DDL and non-DDL statements. Adjacent + * DDL statements share a run so they can become one migration instead of one + * prompt each; a non-DDL statement ends the run, since anything after it may + * depend on it and the surviving statements must keep their relative order. + * + * A run is deliberately not proven safe to run in one transaction — Postgres refuses + * to use an enum value in the same transaction that `ALTER TYPE` added it, and there + * are other such pairs. Grouping them anyway is the accepted trade: the body is shown + * in the migration editor before it runs, and a failed run is reverted with the + * database's own error, so splitting it by hand is a visible one-step fix. + */ +export function splitSqlRuns(code: string, backslashEscapes = true): SqlRun[] { + const runs: SqlRun[] = [] + for (const statement of splitSqlStatements(code, backslashEscapes)) { + const isDdl = isDdlStatement(statement) + const last = runs[runs.length - 1] + if (last && last.isDdl === isDdl) last.statements.push(statement) + else runs.push({ isDdl, statements: [statement] }) + } + return runs +} diff --git a/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte b/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte index d4dad04ac1..d16f948b88 100644 --- a/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte +++ b/frontend/src/lib/components/workspaceSettings/NewDataTableMigrationModal.svelte @@ -64,9 +64,8 @@ function wrapInTransaction(body: string): string { return `BEGIN;\n\n${body}\n\nEND;` } - // A statement inside BEGIN; ... END; must be `;`-terminated. The SQL splitter - // strips the trailing `;` when extracting a detected DDL statement, so re-add - // it when missing. + // Every statement inside BEGIN; ... END; must be `;`-terminated, so normalize + // a prefilled body that ends without one. function ensureTrailingSemicolon(body: string): string { const trimmed = body.trimEnd() return trimmed.endsWith(';') ? trimmed : `${trimmed};` diff --git a/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts b/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts index 8dab8e71f9..0798131ad0 100644 --- a/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts +++ b/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts @@ -28,13 +28,14 @@ export type DdlGuardChoice = | 'run_anyway' /** The DDL became a migration definition. */ | 'migrated' - /** The statement was abandoned, so nothing ran. */ + /** The statements were abandoned, so nothing ran. */ | 'cancelled' /** - * Counted once per prompt that reaches a terminal choice. Picking "create a migration" and then - * dismissing the modal loops back to the prompt instead, and is deliberately not counted: it is - * the same statement still undecided, not a fourth outcome. + * Counted once per prompt that reaches a terminal choice — a prompt covers a whole run of + * adjacent DDL statements, not one statement. Picking "create a migration" and then dismissing + * the modal loops back to the prompt instead, and is deliberately not counted: it is the same + * run still undecided, not a fourth outcome. */ export function logDdlGuardChoice(choice: DdlGuardChoice): void { logFeatureUsage('datatable', 'ddl_guard', { key: choice })