From f4dfaff9c6c11df01b402f9af19c27909c04954e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 29 Jul 2026 00:33:29 +0200 Subject: [PATCH] refactor(datatables): drop best-effort fork schema diff for migrations Fixes GIT-890 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0168uEtDtsvdbk89S1LqtcLW --- .../windmill-api-workspaces/src/workspaces.rs | 33 +- backend/windmill-api/openapi.yaml | 8 +- backend/windmill-common/src/workspaces.rs | 8 +- cli/src/commands/datatable_migrations.ts | 28 +- .../datatable_migrations_run_unit.test.ts | 55 ++ .../lib/components/CompareWorkspaces.svelte | 4 +- .../lib/components/DatatableSchemaDiff.svelte | 477 ------------------ .../ForkedDatatablesWithoutMigrations.svelte | 46 ++ .../src/lib/components/datatableSchemaSql.ts | 100 +--- .../workspaceSettings/projectMigrations.ts | 2 +- 10 files changed, 133 insertions(+), 628 deletions(-) create mode 100644 cli/test/datatable_migrations_run_unit.test.ts delete mode 100644 frontend/src/lib/components/DatatableSchemaDiff.svelte create mode 100644 frontend/src/lib/components/ForkedDatatablesWithoutMigrations.svelte diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 9501ed96f9..54ff08641f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -6546,33 +6546,8 @@ async fn create_workspace_fork_branch( /// Update a forked workspace's datatable config to point to the new database. /// For instance datatables: updates resource_path in the datatable config. /// For resource datatables: updates the resource's dbname and marks it as ws_specific. -/// Snapshot the schema from the source datatable by connecting to its database. -async fn snapshot_datatable_schema( - db: &DB, - parent_w_id: &str, - dt_name: &str, -) -> Result { - let pg = get_datatable_resource_from_db_unchecked(db, parent_w_id, dt_name).await?; - let pg: PgDatabase = serde_json::from_value(pg) - .map_err(|e| Error::internal_err(format!("Failed to parse db credentials: {}", e)))?; - let (client, connection) = pg.connect(Some(db)).await?; - let join_handle = tokio::spawn(async move { connection.await }); - - let schema = windmill_common::query_builders::pg_get_full_schema(&client) - .await - .map_err(Error::internal_err)?; - - drop(client); - windmill_common::shutdown_pg_connection(join_handle).await?; - - serde_json::to_value(schema) - .map_err(|e| Error::internal_err(format!("Failed to serialize schema: {}", e))) -} - async fn apply_forked_datatable( - db: &DB, tx: &mut Transaction<'_, Postgres>, - parent_w_id: &str, forked_w_id: &str, fdt: &ForkedDatatableInfo, ) -> Result<()> { @@ -6584,9 +6559,9 @@ async fn apply_forked_datatable( ))); } - // Snapshot the schema from the source (parent) datatable - let schema = snapshot_datatable_schema(db, parent_w_id, &fdt.name).await?; - let forked_from = serde_json::json!({ "schema": schema }); + // Marks the data table as living in its own cloned database; schema changes + // reach it through SQL migrations. + let forked_from = serde_json::json!({}); // Read the datatable config from the forked workspace let config_val = sqlx::query_scalar!( @@ -7011,7 +6986,7 @@ async fn create_workspace_fork( // Update forked datatable settings to point to new databases for fdt in &nw.forked_datatables { - apply_forked_datatable(&db, &mut tx, &parent_workspace_id, &forked_id, fdt).await?; + apply_forked_datatable(&mut tx, &forked_id, fdt).await?; } // The settings clone copies the source's ducklake config verbatim — including a parent diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 381cf1b999..d685dff6f9 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -31310,12 +31310,8 @@ components: description: Whether the SQL migrations feature is opted in for this data table forked_from: type: object - description: Fork origin info with schema snapshot - properties: - schema: - type: object - description: Schema snapshot at fork time - additionalProperties: true + description: Set when the data table was cloned into its own database on fork + additionalProperties: true DatatableMigration: type: object required: [datatable, timestamp, name, code_up] diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index fb73ba7e2c..dd6949ae12 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -991,12 +991,10 @@ pub struct DataTable { pub migrations_enabled: Option, } +/// Marker set on a fork's data table config when the data table was cloned into +/// its own database. Carries no payload: schema changes propagate as migrations. #[derive(Deserialize, Serialize, Debug)] -pub struct DataTableForkedFrom { - /// Schema snapshot at fork time - #[serde(default, skip_serializing_if = "Option::is_none")] - pub schema: Option, -} +pub struct DataTableForkedFrom {} #[derive(Deserialize, Serialize, Debug)] pub struct DataTableDatabase { diff --git a/cli/src/commands/datatable_migrations.ts b/cli/src/commands/datatable_migrations.ts index 4fcaffd4fa..be8ac5be65 100644 --- a/cli/src/commands/datatable_migrations.ts +++ b/cli/src/commands/datatable_migrations.ts @@ -319,10 +319,22 @@ export async function offerToRunNewMigrations( log.info(colors.gray(` ${m.datatable}: ${m.timestamp} ${m.name}`)); } - // Running migrations mutates the data tables, so skip the prompt in - // non-interactive contexts (--yes, --json, no TTY). - const interactive = !opts?.jsonOutput && !opts?.yes && !!process.stdin.isTTY; - if (!interactive) { + const run = async () => { + for (const datatable of new Set(newMigrations.map((m) => m.datatable))) { + await runMigrations(workspace, datatable); + } + }; + + // `--yes` answers the prompt affirmatively: that is how a git-sync push (CI, + // no TTY) applies the migrations a merge brought in. + if (opts?.yes) { + await run(); + return; + } + + // Running migrations mutates the data tables, so never do it implicitly when + // nobody can answer (--json, no TTY). + if (opts?.jsonOutput || !process.stdin.isTTY) { return; } @@ -330,11 +342,7 @@ export async function offerToRunNewMigrations( message: "New migrations were pushed, run them?", default: false, }); - if (!shouldRun) { - return; - } - - for (const datatable of new Set(newMigrations.map((m) => m.datatable))) { - await runMigrations(workspace, datatable); + if (shouldRun) { + await run(); } } diff --git a/cli/test/datatable_migrations_run_unit.test.ts b/cli/test/datatable_migrations_run_unit.test.ts new file mode 100644 index 0000000000..6e0106f4dc --- /dev/null +++ b/cli/test/datatable_migrations_run_unit.test.ts @@ -0,0 +1,55 @@ +/** + * A git-sync push (CI, no TTY) is how merged migrations reach the workspace, so + * `--yes` must apply them without a prompt — and the no-TTY, no-`--yes` case must + * still leave the data tables untouched. + */ + +import { expect, test, describe, beforeEach, mock } from "bun:test"; + +let runCalls: { workspace: string; datatableName: string }[] = []; + +mock.module("../gen/services.gen.ts", () => ({ + runDatatableMigrations: async (args: { + workspace: string; + datatableName: string; + }) => { + runCalls.push(args); + return { applied: [{ version: 1, name: "m" }] }; + }, +})); + +const { offerToRunNewMigrations } = await import( + "../src/commands/datatable_migrations.ts" +); + +const NEW_MIGRATIONS = [ + { datatable: "orders", timestamp: 1, name: "create_orders" }, + { datatable: "orders", timestamp: 2, name: "add_status" }, + { datatable: "analytics", timestamp: 3, name: "create_events" }, +]; + +describe("offerToRunNewMigrations", () => { + beforeEach(() => { + runCalls = []; + }); + + test("--yes runs each affected data table once, without prompting", async () => { + await offerToRunNewMigrations("ws", NEW_MIGRATIONS, { yes: true }); + expect(runCalls.map((c) => c.datatableName)).toEqual([ + "orders", + "analytics", + ]); + expect(runCalls.every((c) => c.workspace === "ws")).toBe(true); + }); + + test("runs nothing when nobody can answer the prompt", async () => { + await offerToRunNewMigrations("ws", NEW_MIGRATIONS, { jsonOutput: true }); + await offerToRunNewMigrations("ws", NEW_MIGRATIONS, {}); + expect(runCalls).toEqual([]); + }); + + test("runs nothing when the push introduced no migrations", async () => { + await offerToRunNewMigrations("ws", [], { yes: true }); + expect(runCalls).toEqual([]); + }); +}); diff --git a/frontend/src/lib/components/CompareWorkspaces.svelte b/frontend/src/lib/components/CompareWorkspaces.svelte index fbebaf573a..856146fe3b 100644 --- a/frontend/src/lib/components/CompareWorkspaces.svelte +++ b/frontend/src/lib/components/CompareWorkspaces.svelte @@ -61,7 +61,7 @@ import CompareModeToggle, { type CompareMode } from './CompareModeToggle.svelte' import { editUrlFor } from './sessions/forkEditUrl' import { diffInMask } from './sessions/modifiedItemsMask' - import DatatableSchemaDiff from './DatatableSchemaDiff.svelte' + import ForkedDatatablesWithoutMigrations from './ForkedDatatablesWithoutMigrations.svelte' interface Props { currentWorkspaceId: string @@ -1508,7 +1508,7 @@ /> - + {#if pinnedItems.length > 0}
diff --git a/frontend/src/lib/components/DatatableSchemaDiff.svelte b/frontend/src/lib/components/DatatableSchemaDiff.svelte deleted file mode 100644 index ac215747e0..0000000000 --- a/frontend/src/lib/components/DatatableSchemaDiff.svelte +++ /dev/null @@ -1,477 +0,0 @@ - - -{#if applicableCount > 0} -
-

Datatable schema changes

- {#if loading} -
- Loading datatable diffs... -
- {:else if error} -
Failed to load datatable diffs: {error}
- {:else if diffs.length > 0} -
- {#each diffs as diff} - - - - {#if expandedDatatables.has(diff.datatableName)} -
- {#if diff.aheadChanges.length > 0} -
-
Fork changes (ahead)
- {#each diff.aheadChanges as change} -
- {#if change.kind === 'added'} - - {:else if change.kind === 'removed'} - - {:else} - - {/if} - {change.schemaName}. - {change.tableName} - {operationSummary(change)} - -
- {/each} -
- {/if} - {#if diff.behindChanges.length > 0} -
-
- Parent changes (behind) -
- {#each diff.behindChanges as change} -
- {#if change.kind === 'added'} - - {:else if change.kind === 'removed'} - - {:else} - - {/if} - {change.schemaName}. - {change.tableName} - {operationSummary(change)} - -
- {/each} -
- {/if} -
- {/if} - - {/each} -
- {:else} - No changes detected - {/if} -
-{/if} - - - {#if drawerChange && drawerDiff && drawerDirection} - {@const yaml = getDiffYaml()} - (drawerOpen = false)} - title="{drawerChange.schemaName}.{drawerChange.tableName} ({drawerDirection === 'ahead' - ? 'Fork → Parent' - : 'Parent → Fork'})" - > - {#snippet actions()} - - {/snippet} -
- - {#if drawerDirection == 'ahead'} - You have made these changes in {currentWorkspaceId} that are not yet deployed in {parentWorkspaceId}. - {:else if drawerDirection == 'behind'} - These changes were made in {parentWorkspaceId} but the current workspace is not up to date. - {/if} - - -
-
- Schema diff (parent ↔ fork) -
-
- {#await import('$lib/components/DiffEditor.svelte')} -
- -
- {:then Module} - - {/await} -
-
- - -
-
SQL migration
-
- -
-
-
- - {/if} - - - { - confirmDeployOpen = false - await runMigration() - }} - onCanceled={() => { - confirmDeployOpen = false - }} -> -

- This will run the following SQL on workspace {parentWorkspaceId}: -

- 
{migrationSql}
- - - - - diff --git a/frontend/src/lib/components/ForkedDatatablesWithoutMigrations.svelte b/frontend/src/lib/components/ForkedDatatablesWithoutMigrations.svelte new file mode 100644 index 0000000000..08e0d5551a --- /dev/null +++ b/frontend/src/lib/components/ForkedDatatablesWithoutMigrations.svelte @@ -0,0 +1,46 @@ + + +{#if names.length > 0} + + + A data table cloned into its own database only propagates schema changes through SQL + migrations, which are not enabled here — schema changes made in this workspace stay in it. + Enable migrations in + data table settings + to version schema changes and deploy them like any other item. + + +{/if} diff --git a/frontend/src/lib/components/datatableSchemaSql.ts b/frontend/src/lib/components/datatableSchemaSql.ts index cd3d0f9154..d3830ca10b 100644 --- a/frontend/src/lib/components/datatableSchemaSql.ts +++ b/frontend/src/lib/components/datatableSchemaSql.ts @@ -3,11 +3,6 @@ import type { TableEditorValuesColumn, TableEditorForeignKey } from '$lib/components/apps/components/display/dbtable/tableEditor' -import { - diffTableEditorValues, - type AlterTableValues, - makeAlterTableQueries -} from '$lib/components/apps/components/display/dbtable/queries/alterTable' import { renderForeignKey } from '$lib/components/apps/components/display/dbtable/queries/dbQueriesUtils' import type { GetDatatableFullSchemaResponse } from '$lib/gen' @@ -55,79 +50,9 @@ export function apiSchemaToEditorSchema(apiSchema: GetDatatableFullSchemaRespons return result } -export type TableDiff = { +export type TableRef = { schemaName: string tableName: string - kind: 'added' | 'removed' | 'modified' - operations?: AlterTableValues -} - -export type DatatableDiff = { - datatableName: string - aheadChanges: TableDiff[] - behindChanges: TableDiff[] - originalSchema: DatabaseSchema - parentSchema: DatabaseSchema - forkSchema: DatabaseSchema -} - -export function diffDatabaseSchemas( - original: DatabaseSchema, - current: DatabaseSchema -): TableDiff[] { - const diffs: TableDiff[] = [] - const allSchemas = new Set([...Object.keys(original), ...Object.keys(current)]) - for (const schemaName of allSchemas) { - const origTables = original[schemaName] ?? {} - const currTables = current[schemaName] ?? {} - const allTables = new Set([...Object.keys(origTables), ...Object.keys(currTables)]) - for (const tableName of allTables) { - const origTable = origTables[tableName] - const currTable = currTables[tableName] - if (!origTable && currTable) { - diffs.push({ schemaName, tableName, kind: 'added' }) - } else if (origTable && !currTable) { - diffs.push({ schemaName, tableName, kind: 'removed' }) - } else if (origTable && currTable) { - const currWithInitial: TableEditorValues = { - ...currTable, - columns: currTable.columns.map((col) => ({ - ...col, - initialName: col.name, - defaultValue: col.defaultValue ? `{${col.defaultValue}}` : undefined - })) - } - const origTableTransformed: TableEditorValues = { - ...origTable, - columns: origTable.columns.map((col) => ({ - ...col, - defaultValue: col.defaultValue ? `{${col.defaultValue}}` : undefined - })) - } - const diff = diffTableEditorValues(origTableTransformed, currWithInitial) - if (diff.operations.length > 0) { - diffs.push({ schemaName, tableName, kind: 'modified', operations: diff }) - } - } - } - } - return diffs -} - -export function computeDatatableDiff( - datatableName: string, - originalSchema: DatabaseSchema, - parentSchema: DatabaseSchema, - forkSchema: DatabaseSchema -): DatatableDiff { - return { - datatableName, - behindChanges: diffDatabaseSchemas(originalSchema, parentSchema), - aheadChanges: diffDatabaseSchemas(originalSchema, forkSchema), - originalSchema, - parentSchema, - forkSchema - } } /** Detect PostgreSQL auto-increment columns and return the serial type + cleaned props. @@ -152,7 +77,7 @@ function resolveColumnType(c: TableEditorValuesColumn): { * required for circular FKs, where no creation order satisfies inline FKs. */ export function generateAddedTableSql( - change: TableDiff, + change: TableRef, sourceSchema: DatabaseSchema, options?: { ifNotExists?: boolean } ): { create: string; constraints: string[] } | undefined { @@ -197,24 +122,3 @@ export function generateAddedTableSql( } return { create, constraints } } - -export function generateMigrationSql( - change: TableDiff, - sourceSchema: DatabaseSchema, - options?: { ifNotExists?: boolean } -): string { - if (change.kind === 'modified' && change.operations) { - const queries = makeAlterTableQueries(change.operations, 'postgresql', change.schemaName) - if (queries.length === 0) return '' - return 'BEGIN;\n' + queries.join('\n') + '\nCOMMIT;' - } - if (change.kind === 'added') { - const gen = generateAddedTableSql(change, sourceSchema, options) - if (!gen) return '' - return `BEGIN;\n${[gen.create, ...gen.constraints].join('\n')}\nCOMMIT;` - } - if (change.kind === 'removed') { - return `BEGIN;\nDROP TABLE IF EXISTS "${change.schemaName}"."${change.tableName}";\nCOMMIT;` - } - return '' -} diff --git a/frontend/src/lib/components/workspaceSettings/projectMigrations.ts b/frontend/src/lib/components/workspaceSettings/projectMigrations.ts index 8ff4da597d..7acfb00ffc 100644 --- a/frontend/src/lib/components/workspaceSettings/projectMigrations.ts +++ b/frontend/src/lib/components/workspaceSettings/projectMigrations.ts @@ -302,7 +302,7 @@ export async function generateDatatableMigrations( // caveat — an existing differently-shaped table is silently left as-is — // is acceptable for a best-effort, editable migration. const gen = generateAddedTableSql( - { schemaName: t.schemaName, tableName: t.tableName, kind: 'added' }, + { schemaName: t.schemaName, tableName: t.tableName }, prunedSchema, { ifNotExists: true } )