From e48b2039d6edd6f74825a16c0dda811dedaa958d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 16 Aug 2026 11:07:45 +0000 Subject: [PATCH] fix(sql-editor): bind Backup restore to dest connection; stop corrupting snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore from Compare Data Backup was executing reverse DML on the current Compare Destination, so switching Destination (or restoring from History while another credential was selected) could undo onto the wrong database. History also capped oversized snapshot JSON by appending a truncation marker, which made JSON.parse fail while Restore was still offered — silently destroying the durable undo safety net after partial migrates. Record connectionId in the snapshot, always restore against that credential, omit oversized/invalid snapshots instead of truncating them, and surface when History could not store the backup. Co-authored-by: huy.phan9 --- apps/web/src/backend/api/routes.ts | 4 +- .../data-migrate-history.module.test.ts | 32 +++++ .../modules/data-migrate-history.module.ts | 36 ++++-- apps/web/src/frontend/api/dataMigrateApi.ts | 5 +- .../components/sql-editor/DataMigrateBar.tsx | 107 +++++++++++++++-- .../src/frontend/lib/dataMigratePlans.test.ts | 112 +++++++++--------- apps/web/src/frontend/lib/dataMigratePlans.ts | 24 ++++ 7 files changed, 241 insertions(+), 79 deletions(-) create mode 100644 apps/web/src/backend/modules/data-migrate-history.module.test.ts diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index f859c3e8..1e66975e 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -1267,7 +1267,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt res.status(400).json({ error: 'dialect and script are required' }); return; } - const id = await dataMigrateHistory.start((req as AuthedRequest).userId!, { + const started = await dataMigrateHistory.start((req as AuthedRequest).userId!, { dialect: body.dialect, sourceHost: body.sourceHost, targetHost: body.targetHost, @@ -1287,7 +1287,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt script: body.script, snapshotJson: body.snapshotJson, }); - res.json({ id }); + res.json(started); } ); diff --git a/apps/web/src/backend/modules/data-migrate-history.module.test.ts b/apps/web/src/backend/modules/data-migrate-history.module.test.ts new file mode 100644 index 00000000..a312e7f8 --- /dev/null +++ b/apps/web/src/backend/modules/data-migrate-history.module.test.ts @@ -0,0 +1,32 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { + DATA_MIGRATE_MAX_TEXT_LEN, + storeableSnapshotJson, +} from './data-migrate-history.module'; + +describe('storeableSnapshotJson', () => { + it('stores intact JSON under the size limit', () => { + const json = JSON.stringify({ version: 1, rows: [], columns: ['id'] }); + expect(storeableSnapshotJson(json)).toEqual({ json, stored: true }); + }); + + it('omits oversized snapshots instead of appending a truncation marker', () => { + const json = `${'{"rows":['}${'1,'.repeat(DATA_MIGRATE_MAX_TEXT_LEN)}]`; + expect(json.length).toBeGreaterThan(DATA_MIGRATE_MAX_TEXT_LEN); + expect(storeableSnapshotJson(json)).toEqual({ json: undefined, stored: false }); + }); + + it('rejects non-JSON so History never offers a corrupt Restore payload', () => { + const truncated = `${JSON.stringify({ rows: [1], columns: ['id'] }).slice(0, 20)}\n… (truncated)`; + expect(storeableSnapshotJson(truncated)).toEqual({ json: undefined, stored: false }); + }); + + it('treats missing snapshot as not stored', () => { + expect(storeableSnapshotJson(undefined)).toEqual({ json: undefined, stored: false }); + }); +}); diff --git a/apps/web/src/backend/modules/data-migrate-history.module.ts b/apps/web/src/backend/modules/data-migrate-history.module.ts index 3c5ce008..00c010c6 100644 --- a/apps/web/src/backend/modules/data-migrate-history.module.ts +++ b/apps/web/src/backend/modules/data-migrate-history.module.ts @@ -68,11 +68,32 @@ interface Row { } const MAX_RUNS_PER_USER = 200; -const MAX_TEXT_LEN = 1_000_000; +/** Bound script / snapshot blobs so one run cannot bloat the metadata DB. */ +export const DATA_MIGRATE_MAX_TEXT_LEN = 1_000_000; -function cap(text: string | undefined, max = MAX_TEXT_LEN): string | undefined { +/** Truncate free-form script text for display; never used for machine-parsed JSON. */ +function capScript(text: string | undefined, max = DATA_MIGRATE_MAX_TEXT_LEN): string | undefined { if (text == null) return text; - return text.length > max ? `${text.slice(0, max)}\n… (truncated)` : text; + return text.length > max ? `${text.slice(0, max)}\n-- … (truncated)` : text; +} + +/** + * Store snapshot JSON only when it fits intact. Appending a truncation marker + * (the old `cap()` behavior) produced invalid JSON, so History still offered + * Restore while `JSON.parse` always failed — a silent loss of the undo safety net. + */ +export function storeableSnapshotJson( + text: string | undefined, + max = DATA_MIGRATE_MAX_TEXT_LEN +): { json: string | undefined; stored: boolean } { + if (text == null) return { json: undefined, stored: false }; + if (text.length > max) return { json: undefined, stored: false }; + try { + JSON.parse(text); + } catch { + return { json: undefined, stored: false }; + } + return { json: text, stored: true }; } function parseOps(raw: string | null): { insert: boolean; update: boolean; delete: boolean } { @@ -105,8 +126,9 @@ export class DataMigrateHistoryStore { script: string; snapshotJson?: string; } - ): Promise { + ): Promise<{ id: string; snapshotStored: boolean }> { const id = randomUUID(); + const snapshot = storeableSnapshotJson(input.snapshotJson); const store = await getStore(); await store.run( `INSERT INTO data_migrate_runs @@ -126,13 +148,13 @@ export class DataMigrateHistoryStore { JSON.stringify(input.opsEnabled), input.includeIdentity ? 1 : 0, JSON.stringify(input.keyColumns), - cap(input.script) ?? null, - cap(input.snapshotJson) ?? null, + capScript(input.script) ?? null, + snapshot.json ?? null, new Date().toISOString(), ] ); await this.prune(userId); - return id; + return { id, snapshotStored: snapshot.stored }; } private async prune(userId: string): Promise { diff --git a/apps/web/src/frontend/api/dataMigrateApi.ts b/apps/web/src/frontend/api/dataMigrateApi.ts index ff50aa2c..a6a1433e 100644 --- a/apps/web/src/frontend/api/dataMigrateApi.ts +++ b/apps/web/src/frontend/api/dataMigrateApi.ts @@ -64,12 +64,11 @@ export async function apiStartDataMigrate(input: { keyColumns: string[]; script: string; snapshotJson?: string; -}): Promise { - const { id } = await request<{ id: string }>('/data-migrations/start', { +}): Promise<{ id: string; snapshotStored: boolean }> { + return request<{ id: string; snapshotStored: boolean }>('/data-migrations/start', { method: 'POST', body: JSON.stringify(input), }); - return id; } export async function apiFinishDataMigrate( diff --git a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx index 8bb61f53..03e1d45f 100644 --- a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx +++ b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx @@ -19,7 +19,7 @@ import { type DataMigrateRunDetail, type DataMigrateRunSummary, } from '../../api/dataMigrateApi'; -import { buildDataMigratePlans, buildDestSnapshotJson, buildRestorePlansFromSnapshot } from '../../lib/dataMigratePlans'; +import { buildDataMigratePlans, buildDestSnapshotJson, buildRestorePlansFromSnapshot, isUsableDataMigrateSnapshot, snapshotTargetConnectionId } from '../../lib/dataMigratePlans'; import { classifyRowsByKey, DATA_MIGRATE_ROW_CAP, @@ -150,6 +150,8 @@ export const DataMigrateBar: React.FC = ({ const [backupEnabled, setBackupEnabled] = useState(true); const [lastBackup, setLastBackup] = useState<{ runId: string | null; + /** Destination that was migrated — Restore must not follow a later Destination switch. */ + connectionId: string; snapshotJson: string; results: DataMigrateOpResult[]; tableName: string; @@ -434,6 +436,7 @@ export const DataMigrateBar: React.FC = ({ ? buildDestSnapshotJson({ tableName, dialect: dest.dialect, + connectionId: dest.connectionId, destColumns: dest.columns, sourceColumns: source.columns, keyNames, @@ -457,8 +460,9 @@ export const DataMigrateBar: React.FC = ({ ); let runId: string | null = null; + let snapshotStored = false; try { - runId = await apiStartDataMigrate({ + const started = await apiStartDataMigrate({ dialect: dest.dialect, sourceHost: sourceConn?.host || source.label, targetHost: destConn?.host || dest.label, @@ -472,6 +476,15 @@ export const DataMigrateBar: React.FC = ({ script, snapshotJson, }); + runId = started.id; + snapshotStored = started.snapshotStored; + if (backupEnabled && snapshotJson && !snapshotStored) { + toast({ + tone: 'warning', + title: 'Backup too large for History', + body: 'The pre-apply snapshot exceeds the 1MB History limit, so durable Restore after reload is unavailable. Use Restore from the toast while this session still has the in-memory backup.', + }); + } } catch (e) { toast({ tone: 'warning', @@ -557,13 +570,19 @@ export const DataMigrateBar: React.FC = ({ !rolledBack && succeeded.length > 0; + // Capture at apply time — toast onAction must not read a later Destination switch. + const migratedConnectionId = dest.connectionId; + const migratedDialect = dest.dialect; + const migratedLabel = dest.label; + if (canOfferRestore && snapshotJson) { setLastBackup({ runId, + connectionId: migratedConnectionId, snapshotJson, results, tableName, - dialect: dest.dialect, + dialect: migratedDialect, }); } else { setLastBackup(null); @@ -583,7 +602,13 @@ export const DataMigrateBar: React.FC = ({ : `Finished with ${failCount} failure(s)`, body: failCount === 0 - ? `Destination: ${dest.label}.${backupEnabled ? ' Backup snapshot saved to history.' : ''}` + ? `Destination: ${migratedLabel}.${ + backupEnabled + ? snapshotStored + ? ' Backup snapshot saved to history.' + : ' Backup kept in-session only (too large for History).' + : '' + }` : `Failed: ${failedKeys.join('; ')}${ results.filter((r) => r.status === 'FAILED').length > 5 ? '…' : '' }. ${rolledBack ? 'Transaction rolled back. ' : ''}${ @@ -594,10 +619,11 @@ export const DataMigrateBar: React.FC = ({ ? () => void restoreFromBackup({ runId, + connectionId: migratedConnectionId, snapshotJson: snapshotJson!, results, tableName, - dialect: dest.dialect, + dialect: migratedDialect, }) : () => void openHistory(), durationMs: 12_000, @@ -607,6 +633,7 @@ export const DataMigrateBar: React.FC = ({ const restoreFromBackup = async (backup: { runId: string | null; + connectionId: string; snapshotJson: string; results: DataMigrateOpResult[]; tableName: string; @@ -621,6 +648,31 @@ export const DataMigrateBar: React.FC = ({ return; } + // Prefer the connection recorded at migrate time (snapshot / caller). Never + // silently follow a later Compare Destination switch — that would reverse + // DML onto the wrong database (same failure class as Lokee #256). + const targetConnectionId = + backup.connectionId.trim() || + snapshotTargetConnectionId(backup.snapshotJson) || + ''; + if (!targetConnectionId) { + toast({ + tone: 'warning', + title: 'Cannot restore', + body: 'This backup does not record which destination was migrated. Re-run migrate with Backup on, or restore only while the original Destination is selected.', + }); + return; + } + const targetConn = connections.find((c) => c.id === targetConnectionId); + if (!targetConn) { + toast({ + tone: 'warning', + title: 'Cannot restore', + body: 'The destination credential for this backup is no longer available.', + }); + return; + } + const { plans, errors } = buildRestorePlansFromSnapshot({ snapshotJson: backup.snapshotJson, successfulOps, @@ -647,12 +699,12 @@ export const DataMigrateBar: React.FC = ({ let restoreRunId: string | null = null; try { - restoreRunId = await apiStartDataMigrate({ + const started = await apiStartDataMigrate({ dialect: backup.dialect, sourceHost: 'backup-restore', - targetHost: destConn?.host || dest.label, - database: destConn?.database, - schema: destConn?.schema, + targetHost: targetConn.host || targetConn.name || targetConnectionId, + database: targetConn.database, + schema: targetConn.schema, tableName: backup.tableName, rowCount: plans.length, opsEnabled: { insert: true, update: true, delete: true }, @@ -664,6 +716,7 @@ export const DataMigrateBar: React.FC = ({ ].join('\n\n'), snapshotJson: backup.snapshotJson, }); + restoreRunId = started.id; } catch { /* history best-effort */ } @@ -671,9 +724,9 @@ export const DataMigrateBar: React.FC = ({ try { const out = await apiExecuteDataMigrate( { - connectionId: dest.connectionId, - password: sessionPasswords[dest.connectionId] || undefined, - schema: destConn?.schema?.trim() || undefined, + connectionId: targetConnectionId, + password: sessionPasswords[targetConnectionId] || undefined, + schema: targetConn.schema?.trim() || undefined, }, plans.map((p) => ({ op: p.op, @@ -1113,7 +1166,7 @@ export const DataMigrateBar: React.FC = ({ 
                           {historyDetail.snapshotJson || '(none — Backup was off)'}
- {historyDetail.snapshotJson && + {isUsableDataMigrateSnapshot(historyDetail.snapshotJson) && historyDetail.results.some((r) => r.status === 'SUCCESS') && historyDetail.status !== 'SUCCESS' && ( )} + {historyDetail.snapshotJson && + !isUsableDataMigrateSnapshot(historyDetail.snapshotJson) && ( +

+ Snapshot is not valid JSON — Restore is unavailable (likely an + older truncated History entry). +

+ )}
Script diff --git a/apps/web/src/frontend/lib/dataMigratePlans.test.ts b/apps/web/src/frontend/lib/dataMigratePlans.test.ts index 884a61f5..61901015 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.test.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.test.ts @@ -4,9 +4,30 @@ * SPDX-License-Identifier: Apache-2.0 */ import { describe, expect, it } from 'vitest'; -import { buildDataMigratePlans, buildDestSnapshotJson, buildRestorePlansFromSnapshot } from './dataMigratePlans'; +import { + buildDataMigratePlans, + buildDestSnapshotJson, + buildRestorePlansFromSnapshot, + isUsableDataMigrateSnapshot, + snapshotTargetConnectionId, +} from './dataMigratePlans'; import type { ClassifiedRowDiff } from './resultRowDiff'; +const DEST_CONN = 'conn-dest-b'; + +function snapshotFor(ops: ClassifiedRowDiff[], cols = ['id', 'name']) { + return buildDestSnapshotJson({ + tableName: 'customers', + dialect: 'sqlite', + connectionId: DEST_CONN, + destColumns: cols, + sourceColumns: cols, + keyNames: ['id'], + includeIdentity: true, + ops, + }); +} + describe('buildDataMigratePlans', () => { const cols = ['id', 'name']; const ops: ClassifiedRowDiff[] = [ @@ -42,19 +63,16 @@ describe('buildDataMigratePlans', () => { }); it('snapshots dest rows for update/delete and source rows for insert', () => { - const json = buildDestSnapshotJson({ - tableName: 'customers', - dialect: 'sqlite', - destColumns: cols, - sourceColumns: cols, - keyNames: ['id'], - includeIdentity: true, - ops, - }); - const parsed = JSON.parse(json) as { rows: Array<{ _op: string }>; includeIdentity: boolean }; + const json = snapshotFor(ops); + const parsed = JSON.parse(json) as { + rows: Array<{ _op: string }>; + includeIdentity: boolean; + connectionId: string; + }; expect(parsed.rows).toHaveLength(3); expect(parsed.rows.map((r) => r._op).sort()).toEqual(['delete', 'insert', 'update']); expect(parsed.includeIdentity).toBe(true); + expect(parsed.connectionId).toBe(DEST_CONN); }); it('omits identity columns from INSERT when includeIdentity is false', () => { @@ -131,15 +149,7 @@ describe('buildRestorePlansFromSnapshot', () => { ]; it('reverses successful insert/update/delete from Backup', () => { - const snapshotJson = buildDestSnapshotJson({ - tableName: 'customers', - dialect: 'sqlite', - destColumns: cols, - sourceColumns: cols, - keyNames: ['id'], - includeIdentity: true, - ops, - }); + const snapshotJson = snapshotFor(ops); const { plans, errors } = buildRestorePlansFromSnapshot({ snapshotJson, successfulOps: [ @@ -157,10 +167,23 @@ describe('buildRestorePlansFromSnapshot', () => { expect(plans[2]!.plan.sql.toLowerCase()).toContain('delete'); }); + it('records the destination connection id for Restore binding', () => { + const snapshotJson = snapshotFor(ops); + expect(snapshotTargetConnectionId(snapshotJson)).toBe(DEST_CONN); + expect(isUsableDataMigrateSnapshot(snapshotJson)).toBe(true); + }); + + it('rejects truncated / non-JSON History snapshots', () => { + const bad = `${snapshotFor(ops).slice(0, 40)}\n… (truncated)`; + expect(isUsableDataMigrateSnapshot(bad)).toBe(false); + expect(snapshotTargetConnectionId(bad)).toBeUndefined(); + }); + it('skips insert restore when identity was not preserved', () => { const snapshotJson = buildDestSnapshotJson({ tableName: 'customers', dialect: 'sqlite', + connectionId: DEST_CONN, destColumns: cols, sourceColumns: cols, keyNames: ['id'], @@ -176,22 +199,14 @@ describe('buildRestorePlansFromSnapshot', () => { }); it('restores update to the pre-apply dest values', () => { - const snapshotJson = buildDestSnapshotJson({ - tableName: 'customers', - dialect: 'sqlite', - destColumns: cols, - sourceColumns: cols, - keyNames: ['id'], - includeIdentity: true, - ops: [ - { - op: 'update', - keyLabel: 'id=1', - sourceRow: [1, 'Alice'], - destRow: [1, 'Bob'], - }, - ], - }); + const snapshotJson = snapshotFor([ + { + op: 'update', + keyLabel: 'id=1', + sourceRow: [1, 'Alice'], + destRow: [1, 'Bob'], + }, + ]); const { plans, errors } = buildRestorePlansFromSnapshot({ snapshotJson, successfulOps: [{ op: 'update', key: 'id=1' }], @@ -206,15 +221,9 @@ describe('buildRestorePlansFromSnapshot', () => { }); it('re-inserts a deleted dest row on restore', () => { - const snapshotJson = buildDestSnapshotJson({ - tableName: 'customers', - dialect: 'sqlite', - destColumns: cols, - sourceColumns: cols, - keyNames: ['id'], - includeIdentity: true, - ops: [{ op: 'delete', keyLabel: 'id=4', destRow: [4, 'Gone'] }], - }); + const snapshotJson = snapshotFor([ + { op: 'delete', keyLabel: 'id=4', destRow: [4, 'Gone'] }, + ]); const { plans, errors } = buildRestorePlansFromSnapshot({ snapshotJson, successfulOps: [{ op: 'delete', key: 'id=4' }], @@ -226,15 +235,9 @@ describe('buildRestorePlansFromSnapshot', () => { }); it('ignores FAILED ops and reports missing snapshot rows', () => { - const snapshotJson = buildDestSnapshotJson({ - tableName: 'customers', - dialect: 'sqlite', - destColumns: cols, - sourceColumns: cols, - keyNames: ['id'], - includeIdentity: true, - ops: [{ op: 'delete', keyLabel: 'id=4', destRow: [4, 'Gone'] }], - }); + const snapshotJson = snapshotFor([ + { op: 'delete', keyLabel: 'id=4', destRow: [4, 'Gone'] }, + ]); const { plans, errors } = buildRestorePlansFromSnapshot({ snapshotJson, successfulOps: [ @@ -260,6 +263,7 @@ describe('buildRestorePlansFromSnapshot', () => { expect(errors).toEqual([]); expect(plans).toHaveLength(1); expect(plans[0]!.op).toBe('insert'); + expect(snapshotTargetConnectionId(legacy)).toBeUndefined(); }); }); diff --git a/apps/web/src/frontend/lib/dataMigratePlans.ts b/apps/web/src/frontend/lib/dataMigratePlans.ts index e1b1d53f..ccad14da 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.ts @@ -197,6 +197,8 @@ export interface DataMigrateSnapshot { version: 1; tableName: string; dialect: string; + /** Destination credential the migrate wrote to — restore must use this, not the current Compare Destination. */ + connectionId?: string; columns: string[]; keyColumns: string[]; /** True when INSERT preserved source identity values (required to DELETE inserts on restore). */ @@ -213,6 +215,8 @@ export interface DataMigrateSnapshot { export function buildDestSnapshotJson(opts: { tableName: string; dialect: string; + /** Destination connection id — required so Restore cannot target a different Compare Destination. */ + connectionId: string; destColumns: string[]; sourceColumns: string[]; keyNames: string[]; @@ -222,6 +226,7 @@ export function buildDestSnapshotJson(opts: { const { tableName, dialect, + connectionId, destColumns, sourceColumns, keyNames, @@ -253,6 +258,7 @@ export function buildDestSnapshotJson(opts: { version: 1, tableName, dialect, + connectionId, columns: destColumns, keyColumns: keyNames, includeIdentity, @@ -261,6 +267,20 @@ export function buildDestSnapshotJson(opts: { return JSON.stringify(snapshot, null, 2); } +/** True when History can safely offer Restore (parseable JSON with rows/columns). */ +export function isUsableDataMigrateSnapshot(json: string | undefined | null): boolean { + if (!json) return false; + return !('error' in parseSnapshot(json)); +} + +/** Destination connection recorded in the snapshot, if any. */ +export function snapshotTargetConnectionId(json: string): string | undefined { + const parsed = parseSnapshot(json); + if ('error' in parsed) return undefined; + const id = parsed.connectionId?.trim(); + return id || undefined; +} + function parseSnapshot(json: string): DataMigrateSnapshot | { error: string } { try { const raw = JSON.parse(json) as Partial & { @@ -275,6 +295,10 @@ function parseSnapshot(json: string): DataMigrateSnapshot | { error: string } { version: 1, tableName: raw.tableName || '', dialect: raw.dialect || 'sqlite', + connectionId: + typeof raw.connectionId === 'string' && raw.connectionId.trim() + ? raw.connectionId.trim() + : undefined, columns: raw.columns, keyColumns: raw.keyColumns?.length ? raw.keyColumns : guessKeyColumns(raw.rows), includeIdentity: raw.includeIdentity !== false,