From 6659707d4d00341ba0c56016fe62c74373831e17 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 10:27:52 -0600 Subject: [PATCH 01/14] fix(lokee): make revert actually reach the database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing an e2e test for the revert flow proved it had never once landed. The plan looked correct and the driver rejected it: 500 POST /lokee/databases/…/revert {"error":"near \".\": syntax error"} CREATE INDEX IDX_CUSTOMERS_EMAIL ON main.customers (email); Two defects in one statement. **SQLite cannot take a qualified table in CREATE INDEX.** The schema belongs on the index name — `CREATE INDEX main.idx ON customers(email)` — while every other dialect qualifies the table, which is why the shared generator does. SQLite gets a `createIndexStatement` hook that moves the qualifier across. **The generator used compare's match key as an identifier.** CLAUDE.md opens with "The compare key is not an identifier", and this spread the source IndexInfo then overwrote `name` with the uppercased key, emitting IDX_CUSTOMERS_EMAIL for idx_customers_email. SQLite folds case so it survived there; on a case-sensitive target it creates a differently named index and the next compare reads a rename that never happened. The cause was a type: `IndexDiff.source` omitted `name`, so the generator could not reach the identifier compare had been passing all along. Widened it — optional, not required, because this package is published and a required field would break consumers constructing these objects. Both are pinned by unit tests, since the e2e suite is not in the CI gate. Alongside the fix, the History toolbar work this proved out: - The version pickers and the capture credential were two connection-shaped controls on two rows, reading as "which of these databases am I looking at?". They are one database — recorded and live — so capture moved onto the pickers' row and defaults to the saved connection matching the history database. - The graph no longer follows the pickers. Choosing Version 1 as Original used to hide every version between the sides, so the history overview changed as a side effect of choosing what to compare. The checkboxes are the only filter now. - Execute was dead on the Blueprint tab because the data-loss acknowledgement lives on Migration SQL. A risk chip now rides beside the button, and a blocked button carries the reader to the decision instead of greying out. HistoryCompareBar renders in TopToolbar while the fetching lives in LokeeWeaveView, so it asks for work by bumping a counter in the store. Both watchers compare against the value seen at mount: the store outlives the component, and replaying the last request would re-snapshot the database on every visit. Also repairs the existing e2e suite, which CI does not run and which the shared SchemaBlueprint change had broken: the summary is `N versions` rather than `Total Versions: N`, and the inspector's sections carry `blueprint-*` ids — with indexes now rendering where the old panel stored but never showed them. Verified: revert e2e 3/3 (reads the SQLite file, not the UI), history e2e 6/6, 1613 unit tests, tsc clean, eslint 0 errors. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 87 ++++++++- apps/e2e/src/tests/schema-history.test.ts | 9 +- apps/e2e/src/tests/schema-revert.test.ts | 169 ++++++++++++++++++ .../lokee-weave/HistoryCompareBar.tsx | 68 ++++++- .../components/lokee-weave/LokeeWeavePage.tsx | 23 +-- .../lokee-weave/LokeeWeaveView.test.tsx | 31 +++- .../components/lokee-weave/LokeeWeaveView.tsx | 162 +++++++++-------- .../lokee-weave/VersionCompareModal.tsx | 88 ++++++--- .../src/frontend/store/lokeeHistoryStore.ts | 23 +++ .../src/interfaces/diff.types.interface.ts | 13 +- .../src/modules/sql-generator.module.test.ts | 47 +++++ .../sql/src/modules/sql-generator.module.ts | 10 +- .../sqlLite/sqlite.sql-dialect.test.ts | 41 +++++ .../providers/sqlLite/sqlite.sql-dialect.ts | 18 ++ 14 files changed, 648 insertions(+), 141 deletions(-) create mode 100644 apps/e2e/src/tests/schema-revert.test.ts create mode 100644 packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 1af4980d..4131e32b 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -55,7 +55,7 @@ export class LokeeHistoryPage { (expected) => { const el = document.querySelector('[data-testid="lokee-summary"]'); const text = el?.textContent ?? ''; - return new RegExp(`Total Versions:\\s*${expected}\\b`, 'i').test(text); + return new RegExp(`\\b${expected}\\s+versions?\\b`, 'i').test(text); }, n, { timeout: timeoutMs } @@ -121,10 +121,21 @@ export class LokeeHistoryPage { } /** Sections render only once loaded, so callers must await the inspector first. */ - async inspectorHasSection(section: 'growth' | 'source' | 'columns' | 'indexes'): Promise { + async inspectorHasSection(section: 'growth' | 'source' | 'history'): Promise { return this.page.locator(`[data-testid="lokee-inspector-${section}"]`).isVisible(); } + /** + * Columns / indexes / keys / triggers are `SchemaBlueprint` now — the same + * component Compare Schema renders — so they carry `blueprint-*` ids wherever + * they appear rather than an inspector-specific one. + */ + async blueprintHasSection( + section: 'summary' | 'columns' | 'primary-key' | 'indexes' | 'foreign-keys' | 'triggers' + ): Promise { + return this.page.locator(`[data-testid="blueprint-${section}"]`).isVisible(); + } + async inspectorText(): Promise { return (await this.page.locator('[data-testid="lokee-object-inspector"]').innerText()) ?? ''; } @@ -145,7 +156,7 @@ export class LokeeHistoryPage { async versionCount(): Promise { const text = await this.summaryText(); - const match = text.match(/Total Versions:\s*(\d+)/i); + const match = text.match(/(\d+)\s+versions?\b/i); return match ? Number(match[1]) : 0; } @@ -158,4 +169,74 @@ export class LokeeHistoryPage { await box.waitFor({ state: 'visible', timeout: 10_000 }); if (!(await box.isChecked())) await box.check(); } + + // ── Compare versions → revert ──────────────────────────────────────────── + + /** Pick the Original side by its visible label (e.g. "Version 1"). */ + async selectOriginalVersion(label: string): Promise { + const select = this.page.locator('[data-testid="lokee-original-version"]'); + await select.waitFor({ state: 'visible', timeout: 20_000 }); + const option = select.locator('option', { hasText: label }); + await option.waitFor({ state: 'attached', timeout: 20_000 }); + const value = await option.getAttribute('value'); + if (!value) throw new Error(`No Original version option matching ${label}`); + await select.selectOption(value); + } + + async openCompareModal(): Promise { + await clickWhen(this.page, '[data-testid="lokee-compare-versions-btn"]'); + await this.page.waitForSelector('[data-testid="lokee-version-compare"][data-state="ready"]', { + timeout: 30_000, + }); + } + + async compareTab(tab: 'DIFF' | 'DDL_DIFF' | 'SQL'): Promise { + await clickWhen(this.page, `[data-testid="lokee-cmp-tab-${tab}"]`); + } + + async migrationSqlText(): Promise { + await this.compareTab('SQL'); + const pane = this.page.locator('[data-testid="lokee-cmp-ddl"]'); + await pane.waitFor({ state: 'visible', timeout: 20_000 }); + return pane.innerText(); + } + + /** + * Apply the revert. A lossy plan parks the button on "Review data loss…", + * which navigates to Migration SQL rather than running — acknowledge there, + * then press it again. A safe plan runs on the first press. + */ + async executeRevert(): Promise { + const run = this.page.locator('[data-testid="lokee-cmp-run-revert"]'); + await run.waitFor({ state: 'visible', timeout: 20_000 }); + await this.page.waitForFunction( + () => { + const el = document.querySelector('[data-testid="lokee-cmp-run-revert"]'); + return el instanceof HTMLButtonElement && !el.disabled; + }, + { timeout: 30_000 } + ); + if ((await run.innerText()).includes('Review data loss')) { + await run.click(); + const ack = this.page.locator('[data-testid="lokee-cmp-confirm-lossy"]'); + await ack.waitFor({ state: 'visible', timeout: 10_000 }); + await ack.check(); + } + await run.click(); + } + + /** Visible toast text, so a failed revert reports the driver's reason. */ + async toastText(): Promise { + const toasts = this.page.locator('[data-testid="app-toast"]'); + const count = await toasts.count(); + const parts: string[] = []; + for (let i = 0; i < count; i++) { + parts.push((await toasts.nth(i).innerText().catch(() => '')) ?? ''); + } + return parts.join(' | '); + } + + async compareModalOpen(): Promise { + return this.page.locator('[data-testid="lokee-version-compare"]').isVisible(); + } } diff --git a/apps/e2e/src/tests/schema-history.test.ts b/apps/e2e/src/tests/schema-history.test.ts index a82a9bb9..6282bae1 100644 --- a/apps/e2e/src/tests/schema-history.test.ts +++ b/apps/e2e/src/tests/schema-history.test.ts @@ -110,9 +110,12 @@ describe.skipIf(!ready)('Schema Sync · History (SQLite)', () => { await history.clickObjectNamed('customers'); const text = await history.inspectorText(); expect(text).toMatch(/email/i); - expect(await driver.locator('[data-testid="lokee-inspector-columns"]').isVisible()).toBe(true); - expect(await driver.locator('[data-testid="lokee-inspector-indexes"]').count()).toBe(0); - expect(await driver.locator('[data-testid="lokee-inspector-triggers"]').isVisible()).toBe(true); + // The blueprint is Compare Schema's own component now, so the sections + // carry `blueprint-*` ids — and indexes, which the old bespoke inspector + // stored but never surfaced, are among them. + expect(await history.blueprintHasSection('columns')).toBe(true); + expect(await history.blueprintHasSection('indexes')).toBe(true); + expect(await history.blueprintHasSection('triggers')).toBe(true); expect(text).toMatch(/trg_customers_audit/i); expect(await driver.locator('[data-testid="lokee-inspector-script-diff"]').isVisible()).toBe(true); }); diff --git a/apps/e2e/src/tests/schema-revert.test.ts b/apps/e2e/src/tests/schema-revert.test.ts new file mode 100644 index 00000000..80e105be --- /dev/null +++ b/apps/e2e/src/tests/schema-revert.test.ts @@ -0,0 +1,169 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Schema Sync → History → Compare versions → revert, against a local SQLite file. + * + * This is the one flow whose correctness cannot be argued from unit tests: the + * revert plan is built from stored objects, but whether it *lands* depends on + * the generator, the driver and the live schema agreeing. Everything up to the + * button was already covered; this drives the button and then reads the file on + * disk to prove the schema actually moved. + * + * Requires `npm run dev` and `sqlite3` on PATH. Skips when sqlite3 is missing. + */ +import { describe, it, beforeAll, afterAll, expect } from 'vitest'; +import { execFileSync, execSync } from 'node:child_process'; +import { mkdirSync, rmSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import type { Page } from 'playwright'; +import { buildDriver, quitDriver } from '../helpers/driver.js'; +import { AppPage } from '../pages/AppPage.js'; +import { SqlEditorPage } from '../pages/SqlEditorPage.js'; +import { LokeeHistoryPage } from '../pages/LokeeHistoryPage.js'; + +const RUN = Date.now().toString(36); +const DIR = `/tmp/foxschema-e2e-schema-revert-${RUN}`; +const DB = join(DIR, 'revert.db'); +const NAME = `E2E Revert ${RUN}`; + +function hasSqlite3(): boolean { + try { + execSync('which sqlite3', { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +function sqlite(input: string): string { + return execFileSync('sqlite3', [DB], { input, encoding: 'utf8' }); +} + +function schemaText(): string { + return execFileSync('sqlite3', [DB, '.schema'], { encoding: 'utf8' }); +} + +const ready = hasSqlite3(); + +describe.skipIf(!ready)('Schema Sync · History revert (SQLite)', () => { + let driver: Page; + let app: AppPage; + let sql: SqlEditorPage; + let history: LokeeHistoryPage; + + beforeAll(async () => { + rmSync(DIR, { recursive: true, force: true }); + mkdirSync(DIR, { recursive: true }); + sqlite(` +CREATE TABLE customers ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL, + email TEXT +); +CREATE INDEX idx_customers_email ON customers(email); +INSERT INTO customers (id, name, email) VALUES (1, 'Ada', 'ada@example.com'); +`); + expect(existsSync(DB)).toBe(true); + + driver = await buildDriver(); + app = new AppPage(driver); + sql = new SqlEditorPage(driver); + history = new LokeeHistoryPage(driver); + + await app.open(); + await sql.resetPersistedEditorState(); + await driver.reload(); + await driver.waitForSelector('[data-testid="toolbar"]'); + + await sql.addSqliteCredential(NAME, DB); + await driver.locator('[data-testid="view-sync-btn"]').click(); + await driver.waitForSelector('[data-testid="sync-pane-switcher"]'); + await history.selectSavedTargetByName(NAME); + }, 120_000); + + afterAll(async () => { + if (driver) await quitDriver(driver); + rmSync(DIR, { recursive: true, force: true }); + }); + + it('records the baseline, then a version with the index dropped', async () => { + await history.snapshotTarget(); + + // The change to undo. Dropping an index is the cleanest case to assert on: + // reverting it is a pure CREATE, so the plan is safe rather than lossy and + // the file tells us plainly whether it landed. + sqlite('DROP INDEX idx_customers_email;\n'); + expect(schemaText()).not.toMatch(/idx_customers_email/i); + + await history.snapshotTarget(); + await history.openHistoryPane(); + await history.selectHistoryDatabaseContaining(RUN); + await history.waitForGraph(); + await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + }, 120_000); + + it('shows the reverse DDL before anything is applied', async () => { + await history.selectOriginalVersion('Version 1'); + await history.openCompareModal(); + + // The tree is the shared SchemaDiffTree; the blueprint is SchemaBlueprint. + expect(await driver.locator('[data-testid="lokee-cmp-summary"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="schema-blueprint"]').isVisible()).toBe(true); + + const migration = await history.migrationSqlText(); + expect(migration, migration).toMatch(/CREATE INDEX/i); + expect(migration, migration).toMatch(/idx_customers_email/i); + + // Nothing has run yet — the point of comparing before deciding. + expect(schemaText()).not.toMatch(/idx_customers_email/i); + }, 120_000); + + it('applies the revert and records it as a new version', async () => { + // Record what the button actually sent. A silent no-op and a rejected + // request look identical from the file system, and toasts expire before a + // 30s poll finishes — so capture the response rather than infer it. + const calls: string[] = []; + driver.on('response', (res) => { + const url = res.url(); + if (url.includes('/lokee/') && url.includes('/revert') && res.request().method() === 'POST') { + calls.push(`${res.status()} ${url}`); + void res + .text() + .then((body) => calls.push(`body: ${body.slice(0, 300)}`)) + .catch(() => undefined); + } + }); + + await history.executeRevert(); + + // The file is the source of truth: the index is back. If it is not, the + // toast carries the driver's reason — without it this failure is just + // "nothing happened", which is the least actionable thing a test can say. + try { + await expect + .poll(() => schemaText(), { timeout: 30_000 }) + .toMatch(/idx_customers_email/i); + } catch (error) { + throw new Error( + `Revert did not reach the database.\nPOSTs: ${calls.join(' :: ') || '(none)'}\nToast: ${await history.toastText()}\nSchema: ${schemaText()}`, + { cause: error } + ); + } + + // A revert is itself a migration, so history gains a version for it rather + // than rewriting the one it reverted to. + await driver.waitForSelector('[data-testid="lokee-version-compare"]', { + state: 'detached', + timeout: 30_000, + }); + await expect + .poll(async () => history.versionCount(), { timeout: 30_000 }) + .toBeGreaterThanOrEqual(3); + + // The row survived: reverting an index must not rebuild the table. + const rows = sqlite('SELECT count(*) FROM customers;\n').trim(); + expect(rows).toBe('1'); + }, 180_000); +}); diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index 85fb5e59..d06832b4 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -7,8 +7,10 @@ * are versions of one captured database instead of two live connections. */ import React, { useMemo } from 'react'; -import { ArrowLeftRight, ArrowRight } from 'lucide-react'; +import { ArrowLeftRight, ArrowRight, Camera, Loader2, RefreshCw } from 'lucide-react'; import { useLokeeHistoryStore } from '../../store/lokeeHistoryStore'; +import { useSyncStore } from '../../store/useSyncStore'; +import { SQL_ICON_STROKE } from '../sql-editor/sqlIconStyle'; import { historyVersionLabel, lokeeDatabaseLabel, @@ -26,6 +28,12 @@ export function HistoryCompareBar(): React.ReactElement { const setOriginalVersionId = useLokeeHistoryStore((s) => s.setOriginalVersionId); const setTargetVersionId = useLokeeHistoryStore((s) => s.setTargetVersionId); const swapSides = useLokeeHistoryStore((s) => s.swapSides); + const connections = useSyncStore((s) => s.connections); + const captureConnectionId = useLokeeHistoryStore((s) => s.captureConnectionId); + const setCaptureConnectionId = useLokeeHistoryStore((s) => s.setCaptureConnectionId); + const capturing = useLokeeHistoryStore((s) => s.capturing); + const requestCapture = useLokeeHistoryStore((s) => s.requestCapture); + const requestRefresh = useLokeeHistoryStore((s) => s.requestRefresh); const newestFirst = useMemo(() => sortVersionsNewestFirst(versions), [versions]); const resolved = useMemo( @@ -37,8 +45,8 @@ export function HistoryCompareBar(): React.ReactElement { Boolean(resolved.original && resolved.target && resolved.original.id === resolved.target.id); return ( -
-
+
+
Original @@ -99,7 +107,7 @@ export function HistoryCompareBar(): React.ReactElement {
-
+
Target @@ -126,6 +134,58 @@ export function HistoryCompareBar(): React.ReactElement { ))}
+ + {/* Capture lives on this row too. It used to sit on a second bar with its + own credential picker, which read as a *third* connection control next + to the two above it — three pickers for two ideas. */} +
+
+
+ Capture +
+ +
+
+ + +
+
); } diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx index 371267cf..3ee66e11 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx @@ -58,8 +58,6 @@ export interface LokeeWeavePageProps { ) => Promise; /** Shorter header when shown inside Schema Sync. */ embedded?: boolean; - /** Original + Target version ids from the Compare-style History bar. */ - compareVersionIds?: readonly string[]; } const FILTERABLE_TYPES: LokeeObjectType[] = [ @@ -187,15 +185,14 @@ export const LokeeWeavePage: React.FC = ({ onSelectObject, onSaveVersionMeta, embedded = false, - compareVersionIds, }) => { - const [filters, setFilters] = useState(() => { - const base = freshFilters(); - if (compareVersionIds && compareVersionIds.length > 0) { - return { ...base, versionIds: new Set(compareVersionIds) }; - } - return base; - }); + // Deliberately independent of the Original/Target pickers. Driving this + // filter from them hid every version between the two sides — pick "Version 1" + // as Original and Version 2 silently vanished from the graph — so the history + // overview changed under the reader as a side effect of choosing what to + // compare. The pickers choose the diff; the graph shows the history; the + // checkboxes below are the only thing that filters it. + const [filters, setFilters] = useState(freshFilters); const [locked, setLocked] = useState(true); const [selectedVersionId, setSelectedVersionId] = useState(null); const [editName, setEditName] = useState(''); @@ -207,12 +204,6 @@ export const LokeeWeavePage: React.FC = ({ [dto.versions, selectedVersionId] ); - const compareKey = (compareVersionIds ?? []).join('|'); - useEffect(() => { - if (!compareKey) return; - setFilters((f) => ({ ...f, versionIds: new Set(compareKey.split('|')) })); - }, [compareKey]); - useEffect(() => { if (!selectedVersion) { setEditName(''); diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx index 8a5d5013..6d184870 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx @@ -9,7 +9,7 @@ */ import React from 'react'; import { describe, expect, it, vi, beforeEach } from 'vitest'; -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import type { VersionGraphDTO } from './graphTypes'; const listLokeeDatabases = vi.fn(); @@ -108,7 +108,8 @@ describe('LokeeWeaveView', () => { await waitFor(() => expect(screen.getByTestId('graph')).toBeTruthy()); // The subtitle must name the database, not the saved connection. expect(screen.getByTestId('graph').textContent).toContain('[postgres] localhost/foxdb.public'); - expect(screen.getByTestId('lokee-weave-chrome')).toBeTruthy(); + // The chrome row is gone — Refresh and Capture moved into HistoryCompareBar. + expect(screen.queryByTestId('lokee-weave-chrome')).toBeNull(); }); it('shows an empty state rather than an empty canvas', async () => { @@ -188,7 +189,10 @@ describe('LokeeWeaveView', () => { expect(useLokeeHistoryStore.getState().versions.map((v) => v.id)).toEqual(['v2', 'v1']); }); - it('captures a schema from the chosen credential', async () => { + it('captures when the toolbar bar asks for it', async () => { + // The credential picker and Capture button moved to HistoryCompareBar, which + // renders in TopToolbar. It asks by bumping the store counter, so that is + // the seam to test here rather than a button this component no longer owns. listLokeeDatabases.mockResolvedValueOnce([]).mockResolvedValue([DB]); loadVersionGraph.mockResolvedValue({ ...DTO, truncatedObjects: false }); captureSchema.mockResolvedValue({ @@ -202,12 +206,12 @@ describe('LokeeWeaveView', () => { }); render(); + await waitFor(() => expect(listLokeeDatabases).toHaveBeenCalled()); - await waitFor(() => expect(screen.getByTestId('lokee-capture-btn')).toBeTruthy()); - fireEvent.change(screen.getByTestId('lokee-capture-connection'), { - target: { value: 'c1' }, + act(() => { + useLokeeHistoryStore.getState().setCaptureConnectionId('c1'); + useLokeeHistoryStore.getState().requestCapture(); }); - fireEvent.click(screen.getByTestId('lokee-capture-btn')); await waitFor(() => expect(captureSchema).toHaveBeenCalledWith( @@ -218,4 +222,17 @@ describe('LokeeWeaveView', () => { expect(toast).toHaveBeenCalledWith(expect.objectContaining({ title: 'Captured v1' })) ); }); + + it('does not capture or refetch on mount — the counters start at zero', async () => { + // A naive effect on the request counters would fire once per mount, so a + // visit to History would silently snapshot the database. + listLokeeDatabases.mockResolvedValue([DB]); + loadVersionGraph.mockResolvedValue({ ...DTO, truncatedObjects: false }); + + render(); + await waitFor(() => expect(screen.getByTestId('graph')).toBeTruthy()); + + expect(captureSchema).not.toHaveBeenCalled(); + expect(loadVersionGraph).toHaveBeenCalledTimes(1); + }); }); diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx index 4fe07a03..e4728763 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx @@ -9,7 +9,7 @@ * load, or be empty is handled here, so the graph itself remains trivially * testable with a fixture and has no idea a network exists. */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { ArrowRight, Camera, GitBranch, Loader2, RefreshCw, TriangleAlert } from 'lucide-react'; import { LokeeWeavePage } from './LokeeWeavePage'; import { VersionCompareModal } from './VersionCompareModal'; @@ -73,12 +73,18 @@ export function LokeeWeaveView({ const setStoreDatabases = useLokeeHistoryStore((s) => s.setDatabases); const setStoreVersions = useLokeeHistoryStore((s) => s.setVersions); const originalVersionId = useLokeeHistoryStore((s) => s.originalVersionId); + const captureConnectionId = useLokeeHistoryStore((s) => s.captureConnectionId); + const setCaptureConnectionId = useLokeeHistoryStore((s) => s.setCaptureConnectionId); + const capturing = useLokeeHistoryStore((s) => s.capturing); + const setCapturing = useLokeeHistoryStore((s) => s.setCapturing); + const captureRequest = useLokeeHistoryStore((s) => s.captureRequest); + const refreshRequest = useLokeeHistoryStore((s) => s.refreshRequest); const targetVersionId = useLokeeHistoryStore((s) => s.targetVersionId); const [databases, setDatabases] = useState([]); - const [captureConnectionId, setCaptureConnectionId] = useState(''); + const [dto, setDto] = useState(EMPTY_DTO); const [loading, setLoading] = useState(true); - const [capturing, setCapturing] = useState(false); + const [error, setError] = useState(null); const [selectedObject, setSelectedObject] = useState(null); // Bumped to re-run the effect; a plain refetch() would race the in-flight one. @@ -135,11 +141,39 @@ export function LokeeWeaveView({ return [...new Set(ids)]; }, [dto.versions, originalVersionId, targetVersionId]); + /** + * Default the capture credential to the saved connection that *is* the history + * database being viewed. Two connection-shaped controls sitting side by side + * with unrelated values is what made the bar read as "which of these two + * databases am I looking at?" — when they are the same database, one recorded + * and one live. Compare's Target is the fallback, as before. + */ + const activeDatabase = useMemo( + () => databases.find((d) => d.id === activeId), + [databases, activeId] + ); + const matchingCredentialId = useMemo(() => { + if (!activeDatabase) return undefined; + const same = (a?: string | null, b?: string | null) => + (a ?? '').toLowerCase() === (b ?? '').toLowerCase(); + return connections.find( + (c) => + same(c.dialect, activeDatabase.dialect) && + same(c.host, activeDatabase.host) && + same(c.database, activeDatabase.database) + )?.id; + }, [connections, activeDatabase]); + useEffect(() => { - if (!captureConnectionId && selectedTargetConnectionId) { - setCaptureConnectionId(selectedTargetConnectionId); - } - }, [captureConnectionId, selectedTargetConnectionId]); + if (captureConnectionId) return; + const next = matchingCredentialId ?? selectedTargetConnectionId; + if (next) setCaptureConnectionId(next); + }, [ + captureConnectionId, + matchingCredentialId, + selectedTargetConnectionId, + setCaptureConnectionId, + ]); useEffect(() => { let cancelled = false; @@ -192,6 +226,16 @@ export function LokeeWeaveView({ const refresh = useCallback(() => setReloadToken((n) => n + 1), []); + // HistoryCompareBar renders in TopToolbar, so it asks for work by bumping a + // counter rather than holding a callback. 0 is the initial value — acting on + // it would refetch on every mount. + const seenRefreshRequest = useRef(refreshRequest); + useEffect(() => { + if (refreshRequest === seenRefreshRequest.current) return; + seenRefreshRequest.current = refreshRequest; + refresh(); + }, [refreshRequest, refresh]); + const saveVersionMeta = useCallback( async (versionId: string, patch: { name: string; description: string }) => { if (!activeId) return; @@ -266,74 +310,33 @@ export function LokeeWeaveView({ } finally { setCapturing(false); } - }, [captureConnectionId, capturing, connections, refresh, bumpLokeeEpoch, setStoreDatabaseId]); + }, [ + captureConnectionId, + capturing, + connections, + refresh, + bumpLokeeEpoch, + setStoreDatabaseId, + setCapturing, + ]); + + // Fire on the counter alone, through a ref. Depending on `runCapture` here + // is an infinite loop: it reads `capturing`, and it also *sets* it, so every + // toggle gives the callback a new identity, re-runs this effect while the + // counter is still non-zero, and captures again. Caught by a hanging test. + // + // The baseline is whatever the counter held at mount, not zero: the store + // outlives this component, so leaving History after a capture and coming + // back would otherwise re-fire that request and snapshot the database again. + const runCaptureRef = useRef(runCapture); + runCaptureRef.current = runCapture; + const seenCaptureRequest = useRef(captureRequest); + useEffect(() => { + if (captureRequest === seenCaptureRequest.current) return; + seenCaptureRequest.current = captureRequest; + void runCaptureRef.current(); + }, [captureRequest]); - const chrome = ( -
- {!embedded && ( - - )} - -
- - -
-
- ); @@ -366,8 +369,7 @@ export function LokeeWeaveView({ if (loading) { return (
- {chrome} -
+
Loading schema history…
@@ -378,8 +380,7 @@ export function LokeeWeaveView({ if (error) { return (
- {chrome} -
+
Could not load schema history
{error}
@@ -399,8 +400,7 @@ export function LokeeWeaveView({ if (!activeId || dto.versions.length === 0) { return (
- {chrome} -
+
No schema history yet
@@ -416,7 +416,6 @@ export function LokeeWeaveView({ return (
- {chrome} {compareBar} {dto.truncatedObjects && (
@@ -430,7 +429,6 @@ export function LokeeWeaveView({ dto={dto} subtitle={subtitle} embedded={embedded} - compareVersionIds={compareVersionIds} onSelectObject={handleSelectObject} onSaveVersionMeta={saveVersionMeta} /> diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 25e63e16..739a96e4 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -188,6 +188,27 @@ export function VersionCompareModal({ if (data) void loadPlan(); }, [data, loadPlan]); + /** + * Why Execute cannot run yet — the empty string means it can. + * + * The button lives in the toolbar and the data-loss acknowledgement lives on + * the Migration SQL tab, so a reader on the Blueprint tab saw a greyed-out + * button with no stated reason. Comparing before deciding is the whole point + * of this dialog; the decision has to be reachable from wherever you are. + */ + const blockedReason = useMemo(() => { + if (!captureConnectionId) return 'Choose a credential in the bar above to run this.'; + if (!plan) return planning ? 'Still planning…' : 'No plan yet.'; + if (plan.alreadyAtTarget) return 'The live schema already matches Original.'; + if (plan.statements.length === 0) return 'Nothing to apply.'; + if (plan.reversal.risk === 'blocked') { + return 'Blocked — this cannot be applied without losing data the schema cannot restore.'; + } + if (plan.reversal.risk === 'lossy' && !confirmLossy) return 'ACK_LOSSY'; + return ''; + }, [captureConnectionId, plan, planning, confirmLossy]); + const needsLossyAck = blockedReason === 'ACK_LOSSY'; + const runRevert = useCallback(async () => { if (!captureConnectionId || !plan) return; setRunning(true); @@ -340,29 +361,50 @@ export function VersionCompareModal({ testIdPrefix="lokee-cmp" size="compact" /> - +
+ {/* Risk travels with the button, not just with the tab + that happens to show the statements. */} + {plan && plan.statements.length > 0 && ( + + {riskStyle(plan.reversal.risk).label} + {plan.reversal.lossyCount > 0 ? ` · ${plan.reversal.lossyCount}` : ''} + + )} + +
diff --git a/apps/web/src/frontend/store/lokeeHistoryStore.ts b/apps/web/src/frontend/store/lokeeHistoryStore.ts index 7fdb8852..b0dc750b 100644 --- a/apps/web/src/frontend/store/lokeeHistoryStore.ts +++ b/apps/web/src/frontend/store/lokeeHistoryStore.ts @@ -25,6 +25,21 @@ interface LokeeHistoryState { setDatabases: (rows: LokeeDatabase[]) => void; setVersions: (rows: HistoryVersionOption[]) => void; swapSides: () => void; + + /** + * Capture belongs to the same toolbar row as the pickers, but the fetching + * and the graph live in LokeeWeaveView. Rather than passing callbacks + * through the store, the bar bumps a counter and the view watches it — the + * store stays plain data, which is what keeps it testable. + */ + captureConnectionId: string; + setCaptureConnectionId: (id: string) => void; + capturing: boolean; + setCapturing: (busy: boolean) => void; + captureRequest: number; + requestCapture: () => void; + refreshRequest: number; + requestRefresh: () => void; } export const useLokeeHistoryStore = create((set, get) => ({ @@ -41,6 +56,14 @@ export const useLokeeHistoryStore = create((set, get) => ({ setTargetVersionId: (targetVersionId) => set({ targetVersionId }), setDatabases: (databases) => set({ databases }), setVersions: (versions) => set({ versions }), + captureConnectionId: '', + setCaptureConnectionId: (captureConnectionId) => set({ captureConnectionId }), + capturing: false, + setCapturing: (capturing) => set({ capturing }), + captureRequest: 0, + requestCapture: () => set({ captureRequest: get().captureRequest + 1 }), + refreshRequest: 0, + requestRefresh: () => set({ refreshRequest: get().refreshRequest + 1 }), swapSides: () => { const next = swapHistoryCompare(get().versions, { originalVersionId: get().originalVersionId, diff --git a/packages/sql/src/interfaces/diff.types.interface.ts b/packages/sql/src/interfaces/diff.types.interface.ts index 28e2b2fa..8950ab4b 100644 --- a/packages/sql/src/interfaces/diff.types.interface.ts +++ b/packages/sql/src/interfaces/diff.types.interface.ts @@ -10,10 +10,19 @@ export interface ColumnDiff { } export interface IndexDiff { + /** Uppercased compare-key match name — NOT a real identifier, see source.name. */ name: string; status: 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; - source?: { columns: string[]; unique: boolean; constraint?: boolean }; - target?: { columns: string[]; unique: boolean; constraint?: boolean }; + /** + * `name` here is the index's own identifier in its native casing, which is + * what DDL must use. Compare has always passed the whole IndexInfo through; + * only this declaration hid the field, so generators reached for the + * uppercased key instead and emitted IDX_CUSTOMERS_EMAIL for + * idx_customers_email. Optional rather than required because this package is + * published — every producer inside the repo sets it. + */ + source?: { name?: string; columns: string[]; unique: boolean; constraint?: boolean }; + target?: { name?: string; columns: string[]; unique: boolean; constraint?: boolean }; /** * True when this ADDED/REMOVED pair is only an index rename: same columns + * uniqueness as an unmatched index on the other side. Does not mark the table diff --git a/packages/sql/src/modules/sql-generator.module.test.ts b/packages/sql/src/modules/sql-generator.module.test.ts index c087afed..d3be2dc0 100644 --- a/packages/sql/src/modules/sql-generator.module.test.ts +++ b/packages/sql/src/modules/sql-generator.module.test.ts @@ -1109,3 +1109,50 @@ describe('SqlGeneratorModule foreign key hardening', () => { ); }); }); + +describe('CREATE INDEX identifier', () => { + it('uses the index its own name, not compare\'s uppercased match key', () => { + // CLAUDE.md: "The compare key is not an identifier." The generator spread + // the source IndexInfo then overwrote `name` with the key, so a revert + // emitted IDX_CUSTOMERS_EMAIL for idx_customers_email — a different index + // on any dialect that does not fold case, and a phantom rename next compare. + const gen = new SqlGeneratorModule(); + const steps = gen.generateMigrationPlan( + [ + { + tableName: 'CUSTOMERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [], + foreignKeyDiffs: [], + indexDiffs: [ + { + name: 'IDX_CUSTOMERS_EMAIL', + status: 'ADDED', + source: { name: 'idx_customers_email', columns: ['email'], unique: false }, + }, + ], + sourceTable: { + name: 'customers', + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + }, + targetTable: { + name: 'customers', + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + }, + }, + ], + 'postgres' + ); + const sql = steps.flatMap((s) => s.statements).join('\n'); + expect(sql).toMatch(/idx_customers_email/); + expect(sql).not.toMatch(/IDX_CUSTOMERS_EMAIL/); + }); +}); + diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index 3cb2c9a2..8c28779b 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -679,7 +679,15 @@ export class SqlGeneratorModule { if (!srcIdx) continue; // Bare index name (its schema follows the qualified table) — a qualified // index name is a syntax error in Postgres/MySQL/SQL Server. - statements.push(this.createIndexSql({ ...srcIdx, name: this.bareName(idx.name) }, tableName, dialect)); + // + // `idx.name` is compare's uppercased match key, not an identifier, so + // it would create IDX_CUSTOMERS_EMAIL where the source has + // idx_customers_email — a different index on any dialect that does not + // fold case, and a phantom rename on the next compare. The source's own + // name is the identifier; the key is only the fallback for a source + // that somehow carries none. + const indexName = this.bareName(srcIdx.name || idx.name); + statements.push(this.createIndexSql({ ...srcIdx, name: indexName }, tableName, dialect)); } // Add new / recreate modified FK constraints (after all column changes are done). diff --git a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts new file mode 100644 index 00000000..4f75138e --- /dev/null +++ b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.test.ts @@ -0,0 +1,41 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { sqliteSqlDialect } from './sqlite.sql-dialect.js'; + +describe('sqlite CREATE INDEX', () => { + it('puts the schema on the index, never on the table', () => { + // `CREATE INDEX idx ON main.customers(email)` is `near ".": syntax error` + // in SQLite — the qualifier belongs on the index name. Found by driving a + // revert end to end; the plan looked right and the driver rejected it. + const sql = sqliteSqlDialect.createIndexStatement!( + { name: 'idx_customers_email', columns: ['email'], unique: false }, + 'main.customers' + ); + expect(sql).toBe( + 'CREATE INDEX IF NOT EXISTS main.idx_customers_email ON customers (email);' + ); + expect(sql).not.toMatch(/ON\s+main\./); + }); + + it('handles an unqualified table and a unique index', () => { + expect( + sqliteSqlDialect.createIndexStatement!( + { name: 'idx_u', columns: ['a', 'b'], unique: true }, + 'customers' + ) + ).toBe('CREATE UNIQUE INDEX IF NOT EXISTS idx_u ON customers (a, b);'); + }); + + it('carries a partial-index filter through', () => { + expect( + sqliteSqlDialect.createIndexStatement!( + { name: 'idx_active', columns: ['id'], unique: false, filter: 'active = 1' }, + 'main.t' + ) + ).toBe('CREATE INDEX IF NOT EXISTS main.idx_active ON t (id) WHERE active = 1;'); + }); +}); diff --git a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts index 487fb2be..061f38a2 100644 --- a/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts +++ b/packages/sql/src/providers/sqlLite/sqlite.sql-dialect.ts @@ -1,4 +1,5 @@ import type { SqlDialect, ColumnSpec } from '../../modules/sql-dialect.interface.js'; +import type { IndexInfo } from '../../interfaces/schema.interface.js'; import { makeDialectTypeFns, plain, sized, decimalAs } from '../../modules/type-mapping.js'; const types = makeDialectTypeFns({ @@ -97,6 +98,23 @@ export const sqliteSqlDialect: SqlDialect = { return `DROP INDEX IF EXISTS ${indexName};`; }, + /** + * SQLite takes the schema on the *index* name, never on the table: + * `CREATE INDEX main.idx ON customers(email)` is valid, while + * `CREATE INDEX idx ON main.customers(email)` is `near ".": syntax error`. + * Every other dialect qualifies the table, so the shared generator does too — + * this hook moves the qualifier across. + */ + createIndexStatement(index: IndexInfo, qualifiedTable: string): string { + const dot = qualifiedTable.lastIndexOf('.'); + const schema = dot < 0 ? '' : qualifiedTable.slice(0, dot); + const table = dot < 0 ? qualifiedTable : qualifiedTable.slice(dot + 1); + const name = schema ? `${schema}.${index.name}` : index.name; + const unique = index.unique ? ' UNIQUE' : ''; + const where = index.filter?.trim() ? ` WHERE ${index.filter.trim()}` : ''; + return `CREATE${unique} INDEX IF NOT EXISTS ${name} ON ${table} (${index.columns.join(', ')})${where};`; + }, + dropTriggerStatement(triggerName: string, _qualifiedTable: string): string { return `DROP TRIGGER IF EXISTS ${triggerName};`; }, From 1187d3962653c62bcb125c75d5f3d90541f88c68 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 10:34:28 -0600 Subject: [PATCH 02/14] fix(test): drop the unused fireEvent import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Left behind when the capture test moved from clicking a button to bumping the store counter — the button it used to click now lives in HistoryCompareBar. Co-Authored-By: Claude Opus 5 --- .../src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx index 6d184870..e0a5dc34 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.test.tsx @@ -9,7 +9,7 @@ */ import React from 'react'; import { describe, expect, it, vi, beforeEach } from 'vitest'; -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import type { VersionGraphDTO } from './graphTypes'; const listLokeeDatabases = vi.fn(); From bcdece9751303b248e293c517612f95f0f7dd761 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 17:49:34 -0600 Subject: [PATCH 03/14] feat(history): Browse pane, revert provenance, and a no-SQL change report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browse was a mode hiding inside Compare, reachable only from a button on one of Compare's two connection cards. It is its own pane now — Compare | Browse | History — with its own one-connection bar, type filters beside the tree they filter, and a card naming the database being read. Compare keeps its own bar untouched. A revert recorded `source: 'revert'` and nothing else, so history could say an undo happened but never which version was restored. Migration 15 adds `revert_from_version_id` / `revert_to_version_id`; the graph node now reads "↩ reverted to v1". The compare dialog gained a Markdown change report — deliberately no SQL, for the reviewer or the ticket rather than the person running the migration — and its Execute button now names its own blocker instead of showing a dead "(0)". Also: the History Compare button moved into the Target card (the pair is finished being chosen there), the minimap is themed so it stops rendering as a grey slab over a light canvas, and the deploy row's chips no longer wrap. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 11 ++ apps/e2e/src/tests/schema-browse.test.ts | 140 ++++++++++++++++ apps/e2e/src/tests/schema-revert.test.ts | 37 +++++ apps/web/src/backend/api/routes.ts | 21 ++- apps/web/src/backend/database/schema.ts | 18 ++ .../modules/lokee-weave.module.test.ts | 55 ++++++ .../src/backend/modules/lokee-weave.module.ts | 21 ++- .../web/src/frontend/components/BrowseBar.tsx | 96 +++++++++++ .../frontend/components/ObjectDetailPanel.tsx | 37 ++++- .../frontend/components/SchemaTreePanel.tsx | 78 ++++++++- .../src/frontend/components/TopToolbar.tsx | 37 ++--- .../lokee-weave/HistoryCompareBar.tsx | 21 ++- .../components/lokee-weave/LokeeWeavePage.tsx | 25 ++- .../components/lokee-weave/LokeeWeaveView.tsx | 35 ++-- .../lokee-weave/VersionCompareModal.tsx | 116 +++++++++++-- .../components/lokee-weave/buildGraph.ts | Bin 8804 -> 8854 bytes .../components/lokee-weave/graphTypes.ts | 2 + .../frontend/components/lokee-weave/nodes.tsx | 12 ++ .../src/frontend/lib/migrationReport.test.ts | 129 +++++++++++++++ apps/web/src/frontend/lib/migrationReport.ts | 156 ++++++++++++++++++ .../src/frontend/store/lokeeHistoryStore.ts | 5 + apps/web/src/frontend/store/uiStore.ts | 13 +- apps/web/src/frontend/store/useSyncStore.ts | 7 + apps/web/src/shared/lokee-wire.ts | 11 ++ 24 files changed, 990 insertions(+), 93 deletions(-) create mode 100644 apps/e2e/src/tests/schema-browse.test.ts create mode 100644 apps/web/src/frontend/components/BrowseBar.tsx create mode 100644 apps/web/src/frontend/lib/migrationReport.test.ts create mode 100644 apps/web/src/frontend/lib/migrationReport.ts diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 4131e32b..f5106883 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -236,6 +236,17 @@ export class LokeeHistoryPage { return parts.join(' | '); } + /** "↩ reverted to vN" labels on the version nodes, newest first. */ + async revertedToLabels(): Promise { + const marks = this.page.locator('[data-testid^="rf-version-revert-"]'); + const count = await marks.count(); + const out: string[] = []; + for (let i = 0; i < count; i++) { + out.push((await marks.nth(i).innerText().catch(() => '')) ?? ''); + } + return out; + } + async compareModalOpen(): Promise { return this.page.locator('[data-testid="lokee-version-compare"]').isVisible(); } diff --git a/apps/e2e/src/tests/schema-browse.test.ts b/apps/e2e/src/tests/schema-browse.test.ts new file mode 100644 index 00000000..029cd46a --- /dev/null +++ b/apps/e2e/src/tests/schema-browse.test.ts @@ -0,0 +1,140 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Schema Sync → Browse, against a local SQLite file. + * + * Browse used to be a mode hiding inside Compare, reachable only by pressing a + * button on one of Compare's two connection cards. It is its own pane now, so + * this covers the parts that only exist there: the search box and type filters + * on the left, and the connection card on the right naming the one database + * being read. + * + * Requires `npm run dev` and `sqlite3` on PATH. Skips when sqlite3 is missing. + */ +import { describe, it, beforeAll, afterAll, expect } from 'vitest'; +import { execFileSync, execSync } from 'node:child_process'; +import { mkdirSync, rmSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import type { Page } from 'playwright'; +import { buildDriver, quitDriver } from '../helpers/driver.js'; +import { AppPage } from '../pages/AppPage.js'; +import { SqlEditorPage } from '../pages/SqlEditorPage.js'; + +const RUN = Date.now().toString(36); +const DIR = `/tmp/foxschema-e2e-schema-browse-${RUN}`; +const DB = join(DIR, 'browse.db'); +const NAME = `E2E Browse ${RUN}`; + +function hasSqlite3(): boolean { + try { + execSync('which sqlite3', { stdio: 'ignore' }); + return true; + } catch { + return false; + } +} + +const ready = hasSqlite3(); + +describe.skipIf(!ready)('Schema Sync · Browse (SQLite)', () => { + let driver: Page; + let app: AppPage; + let sql: SqlEditorPage; + + beforeAll(async () => { + rmSync(DIR, { recursive: true, force: true }); + mkdirSync(DIR, { recursive: true }); + execFileSync('sqlite3', [DB], { + input: ` +CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT NOT NULL, email TEXT); +CREATE TABLE orders (id INTEGER PRIMARY KEY, customer_id INTEGER, total REAL); +CREATE INDEX idx_customers_email ON customers(email); +CREATE VIEW v_customers AS SELECT id, name FROM customers; +`, + }); + expect(existsSync(DB)).toBe(true); + + driver = await buildDriver(); + app = new AppPage(driver); + sql = new SqlEditorPage(driver); + + await app.open(); + await sql.resetPersistedEditorState(); + await driver.reload(); + await driver.waitForSelector('[data-testid="toolbar"]'); + await sql.addSqliteCredential(NAME, DB); + await driver.locator('[data-testid="view-sync-btn"]').click(); + await driver.waitForSelector('[data-testid="sync-pane-switcher"]'); + }, 120_000); + + afterAll(async () => { + if (driver) await quitDriver(driver); + rmSync(DIR, { recursive: true, force: true }); + }); + + it('offers Browse as its own pane beside Compare and History', async () => { + expect(await driver.locator('[data-testid="sync-pane-compare-btn"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="sync-pane-browse-btn"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="sync-pane-history-btn"]').isVisible()).toBe(true); + }); + + it('says what it wants before anything is loaded', async () => { + await driver.locator('[data-testid="sync-pane-browse-btn"]').click(); + const empty = driver.locator('[data-testid="schema-tree-empty"]'); + await empty.waitFor({ state: 'visible', timeout: 15_000 }); + // Not "No Comparison Active" — there is no comparison in this pane. + expect(await empty.innerText()).toMatch(/Nothing loaded/i); + }); + + it('reads one database from its own one-connection bar', async () => { + // Browse has no Original/Target and no swap — picking the database is the + // whole interaction, and it loads on pick. + // Browse's own bar, with no Original/Target and no swap. Compare keeps its + // two-sided grid and its own Browse buttons — untouched by this pane. + expect(await driver.locator('[data-testid="browse-bar"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="source-saved-select"]').count()).toBe(0); + + const select = driver.locator('[data-testid="browse-connection-select"]'); + const option = select.locator('option', { hasText: NAME }); + await option.waitFor({ state: 'attached', timeout: 15_000 }); + await select.selectOption((await option.getAttribute('value'))!); + + await driver.waitForSelector('[data-testid="browse-type-filter"]', { timeout: 30_000 }); + + // Left: the search box and the type filters, next to the list they filter. + expect(await driver.locator('input[placeholder*="Search objects"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="browse-type-TABLE"]').isVisible()).toBe(true); + expect(await driver.locator('[data-testid="browse-type-VIEW"]').isVisible()).toBe(true); + }, 120_000); + + it('narrows the tree by type', async () => { + await driver.locator('[data-testid="browse-type-VIEW"]').click(); + await expect + .poll(async () => driver.locator('[data-testid="diff-item"]').count(), { timeout: 15_000 }) + .toBe(1); + expect(await driver.locator('[data-testid="diff-item"]').innerText()).toMatch(/v_customers/i); + + // Back to everything. + await driver.locator('[data-testid="browse-type-VIEW"]').click(); + await expect + .poll(async () => driver.locator('[data-testid="diff-item"]').count(), { timeout: 15_000 }) + .toBeGreaterThan(1); + }, 120_000); + + it('names the database being browsed on the right', async () => { + // Compare names two connections in the toolbar; Browse has one, and before + // this it was nowhere on screen. + const card = driver.locator('[data-testid="browse-connection-card"]'); + if (!(await card.isVisible().catch(() => false))) { + // A row is selected by default, so clear the selection to reach the + // empty-detail state that carries the card. + await driver.locator('input[placeholder*="Search objects"]').fill('zzz-no-such-object'); + await card.waitFor({ state: 'visible', timeout: 15_000 }); + } + const text = await card.innerText(); + expect(text).toMatch(/SQLITE/i); + expect(text, text).toMatch(/browse\.db/i); + }, 120_000); +}); diff --git a/apps/e2e/src/tests/schema-revert.test.ts b/apps/e2e/src/tests/schema-revert.test.ts index 80e105be..992a8660 100644 --- a/apps/e2e/src/tests/schema-revert.test.ts +++ b/apps/e2e/src/tests/schema-revert.test.ts @@ -166,4 +166,41 @@ INSERT INTO customers (id, name, email) VALUES (1, 'Ada', 'ada@example.com'); const rows = sqlite('SELECT count(*) FROM customers;\n').trim(); expect(rows).toBe('1'); }, 180_000); + + it('reverts forward again, and logs which version each revert restored', async () => { + // Backward was proven above (v2 → v1). Forward is the same machinery with + // the sides the other way round: pick the *newer* version as Original and + // the live database moves up to it. A revert is just a migration to a + // stored state, so "forward" and "backward" must not be different code. + await history.selectHistoryDatabaseContaining(RUN); + await history.waitForGraph(); + + // v3 (the revert) restored v1, so the index is present. Reverting to v2 — + // the version that dropped it — moves forward and drops it again. + await history.selectOriginalVersion('Version 2'); + await history.openCompareModal(); + const forward = await history.migrationSqlText(); + expect(forward, forward).toMatch(/DROP INDEX/i); + + await history.executeRevert(); + await expect + .poll(() => schemaText(), { timeout: 30_000 }) + .not.toMatch(/idx_customers_email/i); + + await driver.waitForSelector('[data-testid="lokee-version-compare"]', { + state: 'detached', + timeout: 30_000, + }); + await expect + .poll(async () => history.versionCount(), { timeout: 30_000 }) + .toBeGreaterThanOrEqual(4); + + // Both reverts are legible from the graph: each node says which version it + // put back, which is the whole point of recording the provenance. + await history.waitForGraph(); + const restored = await history.revertedToLabels(); + expect(restored.join(' '), restored.join(' ')).toMatch(/reverted to v1/i); + expect(restored.join(' '), restored.join(' ')).toMatch(/reverted to v2/i); + }, 180_000); }); + diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index 46f51962..c81139c4 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -119,7 +119,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId: string, resolved: { dialect: string; option: ConnectionOptions; schema: string }, source: 'manual' | 'migrate' | 'revert', - migrationRunId?: string + extra?: { migrationRunId?: string; revert?: { fromVersionId: string; toVersionId: string } } ) { const { tables } = await loadScopedTables( resolved.dialect, @@ -135,7 +135,8 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt schema: resolved.schema ?? null, tables, source, - migrationRunId, + migrationRunId: extra?.migrationRunId, + revert: extra?.revert, }); } @@ -824,7 +825,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId, { dialect, option, schema }, 'migrate', - runId ?? undefined + { migrationRunId: runId ?? undefined } ); send({ type: 'lokee', phase: 'before', ...before }); } catch (error: unknown) { @@ -850,7 +851,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt userId, { dialect, option, schema }, 'migrate', - runId ?? undefined + { migrationRunId: runId ?? undefined } ); send({ type: 'lokee', phase: 'after', ...after }); } catch (error: unknown) { @@ -933,7 +934,7 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt (req as AuthedRequest).userId!, resolved, body.source === 'migrate' || body.source === 'revert' ? body.source : 'manual', - body.migrationRunId + { migrationRunId: body.migrationRunId } ); res.json(result); } catch (error: unknown) { @@ -1178,7 +1179,15 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt } try { - const capture = await captureLiveSchema(userId, { dialect, option, schema }, 'revert'); + // Record where this undo came from and where it went, so reading the + // history later answers "reverted to which version?" rather than just + // "a revert happened". + const capture = await captureLiveSchema(userId, { dialect, option, schema }, 'revert', { + revert: { + fromVersionId: plan.fromVersion.id, + toVersionId: plan.toVersion.id, + }, + }); res.json({ ok: true, capture, ...published }); } catch (error: unknown) { const message = error instanceof Error ? error.message : 'capture failed'; diff --git a/apps/web/src/backend/database/schema.ts b/apps/web/src/backend/database/schema.ts index 502421cc..a89e404f 100644 --- a/apps/web/src/backend/database/schema.ts +++ b/apps/web/src/backend/database/schema.ts @@ -373,6 +373,24 @@ const MIGRATIONS: Migration[] = [ ]; }, }, + { + id: 15, + name: 'lokee_version_revert_provenance', + statements: (d) => { + const t = types(d); + return [ + // Where a revert came from and where it went. A version recorded by a + // revert carried `source = 'revert'` and nothing else, so history could + // say a revert happened but never which version was restored — the one + // question you ask when reading back an undo. + // + // Nullable: every version captured before this, and every non-revert + // capture, legitimately has neither. + `ALTER TABLE lokee_versions ADD COLUMN revert_from_version_id ${t.id}`, + `ALTER TABLE lokee_versions ADD COLUMN revert_to_version_id ${t.id}`, + ]; + }, + }, ]; const SIGNUP_WIZARD_SHOWN_KEY = 'signup.wizard_shown'; diff --git a/apps/web/src/backend/modules/lokee-weave.module.test.ts b/apps/web/src/backend/modules/lokee-weave.module.test.ts index 7385cf35..52fcab7c 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.test.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.test.ts @@ -554,6 +554,61 @@ describe('inspectObject', () => { }); }); +describe('revert provenance', () => { + it('records which version a revert restored, and from where', async () => { + // `source: 'revert'` says an undo happened; on its own it loses the only + // thing you want when reading one back — which version was put back. + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [CUSTOMER], source: 'manual' }); + const widened = table('customer', [ + ['id', 'integer', false], + ['email', 'varchar(255)'], + ]); + const v2 = await weave.capture(USER, { ...IDENTITY, tables: [widened], source: 'migrate' }); + + // The revert itself is a capture of the restored schema, tagged with where + // it came from and where it went. + const v3 = await weave.capture(USER, { + ...IDENTITY, + tables: [CUSTOMER], + source: 'revert', + revert: { fromVersionId: v2.versionId, toVersionId: v1.versionId }, + }); + + const versions = await weave.listVersions(USER, v3.databaseId, 10); + const recorded = versions.find((v) => v.id === v3.versionId); + expect(recorded?.source).toBe('revert'); + expect(recorded?.revertFromVersionId).toBe(v2.versionId); + expect(recorded?.revertToVersionId).toBe(v1.versionId); + + // Ordinary captures carry neither, so the fields stay a positive signal. + const plain = versions.find((v) => v.id === v2.versionId); + expect(plain?.revertFromVersionId).toBeUndefined(); + expect(plain?.revertToVersionId).toBeUndefined(); + }); + + it('resolves the restored version to its number for the graph node', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [CUSTOMER], source: 'manual' }); + const v2 = await weave.capture(USER, { + ...IDENTITY, + tables: [table('customer', [['id', 'integer', false]])], + source: 'migrate', + }); + await weave.capture(USER, { + ...IDENTITY, + tables: [CUSTOMER], + source: 'revert', + revert: { fromVersionId: v2.versionId, toVersionId: v1.versionId }, + }); + + const dto = await weave.graph(USER, v1.databaseId, 10); + const revertNode = dto.versions.find((v) => v.number === 3); + expect(revertNode?.source).toBe('revert'); + expect(revertNode?.revertedToNumber).toBe(1); + }); +}); + describe('matchDatabaseIdentity', () => { it('accepts the same identity the history was captured under', async () => { const { weave } = await freshStore(); diff --git a/apps/web/src/backend/modules/lokee-weave.module.ts b/apps/web/src/backend/modules/lokee-weave.module.ts index d33ae542..3a6157be 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.ts @@ -110,6 +110,12 @@ export interface CaptureInput extends DatabaseIdentityInput { source: CaptureSource; /** Links the version to the run that caused it — this is the attribution. */ migrationRunId?: string; + /** + * For `source: 'revert'`: the head the database was at, and the version it was + * reverted to. Without it a revert records that *an* undo happened and loses + * the only thing you want when reading one back — which version was restored. + */ + revert?: { fromVersionId: string; toVersionId: string }; } /** @@ -135,6 +141,8 @@ interface VersionRow { last_observed_at: string; display_name?: string | null; description?: string | null; + revert_from_version_id?: string | null; + revert_to_version_id?: string | null; } interface DeltaRow { @@ -590,8 +598,8 @@ export class LokeeWeaveStore { `INSERT INTO lokee_versions (id, database_id, version_number, root_hash, parent_version_id, migration_run_id, author_user_id, source, object_count, change_count, observation_count, - created_at, last_observed_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?)`, + created_at, last_observed_at, revert_from_version_id, revert_to_version_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?, ?, ?)`, [ versionId, databaseId, @@ -605,6 +613,8 @@ export class LokeeWeaveStore { capture.changes.length, now, now, + input.revert?.fromVersionId ?? null, + input.revert?.toVersionId ?? null, ] ); await this.writeDelta(store, versionId, capture.changes); @@ -731,6 +741,8 @@ export class LokeeWeaveStore { description: r.description?.trim() || undefined, objectCount: Number(r.object_count) || 0, changeCount: Number(r.change_count) || 0, + revertFromVersionId: r.revert_from_version_id ?? undefined, + revertToVersionId: r.revert_to_version_id ?? undefined, })); } @@ -954,6 +966,11 @@ export class LokeeWeaveStore { author: v.author, name: v.name, description: v.description, + source: v.source, + // Resolve the id to the number the reader actually sees on the graph. + revertedToNumber: v.revertToVersionId + ? versions.find((other) => other.id === v.revertToVersionId)?.number + : undefined, })), objects, totalVersions: Number(totalRow?.n) || versions.length, diff --git a/apps/web/src/frontend/components/BrowseBar.tsx b/apps/web/src/frontend/components/BrowseBar.tsx new file mode 100644 index 00000000..78a33fde --- /dev/null +++ b/apps/web/src/frontend/components/BrowseBar.tsx @@ -0,0 +1,96 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Browse's toolbar: one database, nothing else. + * + * Browse used to borrow Compare's bar — two connection cards, a swap arrow, a + * direction label — for a pane that reads a single schema and has no direction + * at all. Half those controls were meaningless here, and the "Browse" buttons + * that started the read were buried on the Original/Target cards, which is why + * nobody found them. + * + * Picking a connection loads it. That is the whole interaction. + */ +import React from 'react'; +import { Loader2, Search } from 'lucide-react'; +import { useSyncStore } from '../store/useSyncStore'; +import { getSessionPassword } from '../lib/sessionPasswords'; + +export function BrowseBar(): React.ReactElement { + const connections = useSyncStore((s) => s.connections); + const sourceConfig = useSyncStore((s) => s.sourceConfig); + const isBrowsing = useSyncStore((s) => s.isBrowsing); + const browseMode = useSyncStore((s) => s.browseMode); + const selectedObjectTypes = useSyncStore((s) => s.selectedObjectTypes); + const applySavedConnection = useSyncStore((s) => s.applySavedConnection); + const browseSchema = useSyncStore((s) => s.browseSchema); + + const selectedId = sourceConfig.connectionId ?? ''; + + /** + * Browsing reads through the `source` side because that is where the store's + * connection config lives; the user never sees a side here. A saved + * connection with no stored password can still be browsed if this session + * already unlocked it — same rule Compare uses. + */ + const onPick = (id: string) => { + if (!id) return; + const conn = connections.find((c) => c.id === id); + if (!conn) return; + const password = conn.hasPassword ? undefined : getSessionPassword(id); + applySavedConnection('source', id, password); + void browseSchema('source'); + }; + + const label = sourceConfig.option.database + ? `${sourceConfig.dialect.toUpperCase()} · ${[sourceConfig.option.host, sourceConfig.option.database] + .filter(Boolean) + .join('/')}${sourceConfig.schema ? `.${sourceConfig.schema}` : ''}` + : null; + + return ( +
+ + Database + + + + + + {label && {label}} +
+ ); +} diff --git a/apps/web/src/frontend/components/ObjectDetailPanel.tsx b/apps/web/src/frontend/components/ObjectDetailPanel.tsx index 0c706ea7..e085c6c7 100644 --- a/apps/web/src/frontend/components/ObjectDetailPanel.tsx +++ b/apps/web/src/frontend/components/ObjectDetailPanel.tsx @@ -58,6 +58,7 @@ export const ObjectDetailPanel: React.FC = () => { targetConnected, compareResult, browseMode, + browseSide, syncSelection, toggleSyncSelection, nonDestructive, @@ -199,13 +200,47 @@ export const ObjectDetailPanel: React.FC = () => { }, [selectedTable, expandedTriggers, sourceConfig.dialect, targetConfig.dialect]); if (!selectedTable) { + // In Browse the left pane filters one database's objects, so this side + // should say *which* database that is. Comparing has two connections named + // in the toolbar already; browsing has one, and it was nowhere on screen. + const browsed = browseSide === 'target' ? targetConfig : sourceConfig; return (

Select Object to View Details

- Select an object from the left browser tree to inspect columns, indices, definitions, and generated migration DDL. + Select an object from the left browser tree to inspect columns, indices, definitions, + {browseMode ? ' and its CREATE script.' : ' and generated migration DDL.'}

+ {browseMode && ( +
+
Dialect
+
{browsed.dialect.toUpperCase()}
+ {browsed.option.host && ( + <> +
Host
+
{browsed.option.host}
+ + )} + {browsed.option.database && ( + <> +
Database
+
{browsed.option.database}
+ + )} + {browsed.schema && ( + <> +
Schema
+
{browsed.schema}
+ + )} +
Objects
+
{compareResult?.tables.length ?? 0}
+
+ )}
); } diff --git a/apps/web/src/frontend/components/SchemaTreePanel.tsx b/apps/web/src/frontend/components/SchemaTreePanel.tsx index 7fa2922a..173c6b05 100644 --- a/apps/web/src/frontend/components/SchemaTreePanel.tsx +++ b/apps/web/src/frontend/components/SchemaTreePanel.tsx @@ -1,5 +1,6 @@ import React, { useState, useEffect } from 'react'; import { useSyncStore } from '../store/useSyncStore'; +import { useUiStore } from '../store/uiStore'; import { Search, Layers, Table2, Eye, FunctionSquare, SquareTerminal, Zap, Hash, Box, Users } from 'lucide-react'; import type { TableDiff } from '../lib/types'; import { SchemaDiffTree, TYPE_META, TYPE_ORDER } from './SchemaDiffTree'; @@ -16,6 +17,7 @@ const MIN_WIDTH = 280; const MAX_WIDTH = 640; export const SchemaTreePanel: React.FC = () => { + const syncPane = useUiStore((s) => s.syncPane); const { compareResult, browseMode, @@ -29,6 +31,8 @@ export const SchemaTreePanel: React.FC = () => { searchTerm, setSearchTerm, typeFilter, + toggleTypeFilter, + clearTypeFilter, syncSelection, toggleSyncSelection, setAllSyncSelection, @@ -104,12 +108,22 @@ export const SchemaTreePanel: React.FC = () => { }, [compareResult, filteredTables, selectedTable, setSelectedTable]); if (!compareResult) { + // Browse is its own pane now, so the empty state has to name the thing the + // reader is actually looking at rather than always saying "comparison". + const browsing = syncPane === 'browse'; return ( -
+
-

No Comparison Active

+

+ {browsing ? 'Nothing loaded' : 'No Comparison Active'} +

- Connect and click "Compare Schemas" to view the difference tree — or "Browse" one side to search its objects. + {browsing + ? 'Pick a connection above and click "Browse" to read its objects.' + : 'Connect and click "Compare Schemas" to view the difference tree.'}

); @@ -223,10 +237,58 @@ export const SchemaTreePanel: React.FC = () => {
+ {/* Browse's own type filter. Compare puts these pills in the top toolbar, + which is full-width; Browse has no connection grid competing for that + bar, and the reader is filtering a single database's contents, so the + control belongs beside the list it filters. Written out here rather + than shared with the toolbar's row: same data, different container and + sizing, and one component bent to satisfy both would be worse than + twenty lines that read plainly. */} + {browseMode && ( +
+ + {TYPE_ORDER.filter((type) => + compareResult.tables.some((t) => t.objectType === type) + ).map((type) => { + const count = compareResult.tables.filter((t) => t.objectType === type).length; + const active = typeFilter.includes(type); + return ( + + ); + })} +
+ )} + {/* Deployment Selection Header — hidden in browse mode (nothing to deploy) */} {!browseMode && ( -
-
diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index d06832b4..bae14ac0 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -7,7 +7,7 @@ * are versions of one captured database instead of two live connections. */ import React, { useMemo } from 'react'; -import { ArrowLeftRight, ArrowRight, Camera, Loader2, RefreshCw } from 'lucide-react'; +import { ArrowLeftRight, ArrowRight, Camera, GitCompareArrows, Loader2, RefreshCw } from 'lucide-react'; import { useLokeeHistoryStore } from '../../store/lokeeHistoryStore'; import { useSyncStore } from '../../store/useSyncStore'; import { SQL_ICON_STROKE } from '../sql-editor/sqlIconStyle'; @@ -34,6 +34,7 @@ export function HistoryCompareBar(): React.ReactElement { const capturing = useLokeeHistoryStore((s) => s.capturing); const requestCapture = useLokeeHistoryStore((s) => s.requestCapture); const requestRefresh = useLokeeHistoryStore((s) => s.requestRefresh); + const requestCompare = useLokeeHistoryStore((s) => s.requestCompare); const newestFirst = useMemo(() => sortVersionsNewestFirst(versions), [versions]); const resolved = useMemo( @@ -114,13 +115,14 @@ export function HistoryCompareBar(): React.ReactElement {
Current database or older version
+
+ {/* The diff belongs to the pair, and the pair is finished being chosen + here — so the button that opens it sits with the second side rather + than on a strip of its own below the bar. */} + +
{/* Capture lives on this row too. It used to sit on a second bar with its diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx index 3ee66e11..bf3858a7 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeavePage.tsx @@ -28,6 +28,7 @@ import type { LokeeObjectType } from '@foxschema/sql'; import { OBJECT_STYLES, STATUS_STYLES, objectStyle, statusStyle } from '../../lib/lokeeColors'; import { LOKEE_NODE_TYPES } from './nodes'; import { buildVersionGraph } from './buildGraph'; +import { useUiStore } from '../../store/uiStore'; import { VersionCompareModal } from './VersionCompareModal'; import { DEFAULT_LAYOUT, @@ -193,6 +194,7 @@ export const LokeeWeavePage: React.FC = ({ // compare. The pickers choose the diff; the graph shows the history; the // checkboxes below are the only thing that filters it. const [filters, setFilters] = useState(freshFilters); + const isLight = useUiStore((s) => s.resolvedMode) === 'light'; const [locked, setLocked] = useState(true); const [selectedVersionId, setSelectedVersionId] = useState(null); const [editName, setEditName] = useState(''); @@ -628,17 +630,26 @@ export const LokeeWeavePage: React.FC = ({ n.type === 'versionNode' - ? 'var(--color-violet-400)' + ? '#a78bfa' : n.type === 'deletedObjectNode' - ? 'var(--color-rose-400)' - : 'var(--color-sky-400)' + ? '#fb7185' + : '#38bdf8' } + nodeStrokeWidth={3} /> diff --git a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx index e4728763..342a79ff 100644 --- a/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx +++ b/apps/web/src/frontend/components/lokee-weave/LokeeWeaveView.tsx @@ -340,29 +340,17 @@ export function LokeeWeaveView({ - // The two sides come from HistoryCompareBar in the toolbar; this is only the - // trigger that opens the diff for them, so it sits next to the graph it - // explains rather than in the bar. - const compareBar = compareVersionIds.length === 2 && ( -
- - - Original and Target are set in the bar above. - -
- ); + // HistoryCompareBar's Target card owns the Compare button now, so this view + // only has to open the modal for the pair the bar resolved. + const compareRequest = useLokeeHistoryStore((s) => s.compareRequest); + const seenCompareRequest = useRef(compareRequest); + useEffect(() => { + if (compareRequest === seenCompareRequest.current) return; + seenCompareRequest.current = compareRequest; + if (compareVersionIds.length === 2) { + setComparePair({ original: compareVersionIds[0]!, target: compareVersionIds[1]! }); + } + }, [compareRequest, compareVersionIds]); // Every hook above any early return — a rules-of-hooks crash has happened in // this codebase before. @@ -416,7 +404,6 @@ export function LokeeWeaveView({ return (
- {compareBar} {dto.truncatedObjects && (
Showing the objects that changed in this window. This schema has more objects than the diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 739a96e4..74189656 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -14,7 +14,7 @@ * forty columns across three tables is three rows to scan, not forty. */ import React, { useCallback, useEffect, useMemo, useState } from 'react'; -import { Loader2, Play, X } from 'lucide-react'; +import { Download, Loader2, Play, X } from 'lucide-react'; import type { TableDiff } from '@foxschema/sql'; import { compareLokeeVersions, @@ -28,6 +28,7 @@ import { getSessionPassword } from '../../lib/sessionPasswords'; import { toast } from '../../store/toastStore'; import { riskStyle } from '../../lib/lokeeColors'; import { SchemaBlueprint } from '../SchemaBlueprint'; +import { buildMigrationReport, migrationReportFilename } from '../../lib/migrationReport'; import { SchemaDiffTree, orderTablesForDisplay } from '../SchemaDiffTree'; import { DetailTabs, type DetailTab } from '../DetailTabs'; import { buildTableDdlDiffLines, DdlDiffLines } from '../SchemaDdlDiff'; @@ -196,18 +197,52 @@ export function VersionCompareModal({ * button with no stated reason. Comparing before deciding is the whole point * of this dialog; the decision has to be reachable from wherever you are. */ - const blockedReason = useMemo(() => { - if (!captureConnectionId) return 'Choose a credential in the bar above to run this.'; - if (!plan) return planning ? 'Still planning…' : 'No plan yet.'; - if (plan.alreadyAtTarget) return 'The live schema already matches Original.'; - if (plan.statements.length === 0) return 'Nothing to apply.'; + const blocked = useMemo((): { code: string; label: string; why: string } | null => { + if (!captureConnectionId) { + return { + code: 'credential', + label: 'No credential', + why: 'Choose a credential in the bar above to run this.', + }; + } + if (!plan) { + return { + code: 'planning', + label: planning ? 'Planning…' : 'No plan', + why: planning ? 'Still planning…' : 'No plan yet.', + }; + } + // Revert always moves the *live* database to whatever sits on the Original + // side; the Target picker only chooses what the diff above is showing. So + // putting the newest version on Original asks to revert to where you + // already are, and the plan is empty. "(0)" did not say that. + if (plan.alreadyAtTarget) { + return { + code: 'already', + label: 'Already at Original', + why: 'Original is the current head — put the version you want to restore on the Original side.', + }; + } + if (plan.statements.length === 0) { + return { code: 'empty', label: 'Nothing to apply', why: 'The plan is empty.' }; + } if (plan.reversal.risk === 'blocked') { - return 'Blocked — this cannot be applied without losing data the schema cannot restore.'; + return { + code: 'blocked', + label: 'Blocked', + why: 'This cannot be applied without losing data the schema cannot restore.', + }; } - if (plan.reversal.risk === 'lossy' && !confirmLossy) return 'ACK_LOSSY'; - return ''; + if (plan.reversal.risk === 'lossy' && !confirmLossy) { + return { + code: 'lossy', + label: 'Review data loss…', + why: 'This revert destroys data — review it on Migration SQL and confirm there.', + }; + } + return null; }, [captureConnectionId, plan, planning, confirmLossy]); - const needsLossyAck = blockedReason === 'ACK_LOSSY'; + const needsLossyAck = blocked?.code === 'lossy'; const runRevert = useCallback(async () => { if (!captureConnectionId || !plan) return; @@ -253,6 +288,31 @@ export function VersionCompareModal({ return buildTableDdlDiffLines(selectedDiff, dialect, dialect, (ddl) => ddl); }, [selectedDiff, data]); + /** + * Download the change report. A separate artefact from the Migration SQL tab + * on purpose: this one is for the reviewer or the ticket, so it carries no + * DDL at all. + */ + const exportReport = useCallback(() => { + if (!data) return; + const meta = { + originalLabel: versionDisplayName({ number: data.to.number, name: data.to.name }), + targetLabel: data.from + ? versionDisplayName({ number: data.from.number, name: data.from.name }) + : 'first capture', + generatedAt: new Date(), + }; + const blob = new Blob([buildMigrationReport(data.compare, meta)], { + type: 'text/markdown;charset=utf-8', + }); + const url = URL.createObjectURL(blob); + const anchor = document.createElement('a'); + anchor.href = url; + anchor.download = migrationReportFilename(meta); + anchor.click(); + URL.revokeObjectURL(url); + }, [data]); + const heading = useMemo(() => { if (!data) return 'Compare versions'; const reference = versionDisplayName({ number: data.to.number, name: data.to.name }); @@ -288,6 +348,17 @@ export function VersionCompareModal({
{heading}
+
{changed.length === 0 ? ( @@ -379,15 +462,13 @@ export function VersionCompareModal({ type="button" data-testid="lokee-cmp-run-revert" title={ - needsLossyAck - ? 'This revert destroys data — review it on Migration SQL and confirm there' - : blockedReason || - `Apply ${plan?.statements.length ?? 0} statement(s) and record a new version` + blocked?.why ?? + `Apply ${plan?.statements.length ?? 0} statement(s) and record a new version` } // A lossy plan keeps the button live so it can carry the // reader to the acknowledgement; every other blocker is a // genuine dead end and stays disabled. - disabled={running || (Boolean(blockedReason) && !needsLossyAck)} + disabled={running || (Boolean(blocked) && !needsLossyAck)} onClick={() => { if (needsLossyAck) { setTab('SQL'); @@ -400,9 +481,8 @@ export function VersionCompareModal({ {running ? 'Applying…' - : needsLossyAck - ? 'Review data loss…' - : `Execute migration (${plan?.statements.length ?? 0})`} + : (blocked?.label ?? + `Execute migration (${plan?.statements.length ?? 0})`)}
diff --git a/apps/web/src/frontend/components/lokee-weave/buildGraph.ts b/apps/web/src/frontend/components/lokee-weave/buildGraph.ts index 2efb6fe9c9c590aa5b4ca8f87068376380b4b675..abfdb122a6b566cb8d45742f7f279c0b5afa953d 100644 GIT binary patch delta 57 xcmaFjGR<{^l7xL+#k(C0FU!0krr-v@HSw_Nx3jlO46*~X` delta 12 TcmbQ{`ov{}lEmf^2`4T9A^HSa diff --git a/apps/web/src/frontend/components/lokee-weave/graphTypes.ts b/apps/web/src/frontend/components/lokee-weave/graphTypes.ts index fad940d9..50dc583e 100644 --- a/apps/web/src/frontend/components/lokee-weave/graphTypes.ts +++ b/apps/web/src/frontend/components/lokee-weave/graphTypes.ts @@ -76,6 +76,8 @@ export type VersionNodeData = { name?: string; description?: string; changeCount: number; + /** Version this one restored, when a revert produced it. */ + revertedToNumber?: number; }; /** Prefer a custom label; otherwise "Version N". */ diff --git a/apps/web/src/frontend/components/lokee-weave/nodes.tsx b/apps/web/src/frontend/components/lokee-weave/nodes.tsx index 8fb4c0cf..3c3c2fde 100644 --- a/apps/web/src/frontend/components/lokee-weave/nodes.tsx +++ b/apps/web/src/frontend/components/lokee-weave/nodes.tsx @@ -81,6 +81,18 @@ export const VersionNode = memo(({ data: d, selected }: NodeProps )} + {/* A revert reads as an ordinary version otherwise — same node, same + counts — and the one thing you want to know is which version it put + back. */} + {d.revertedToNumber != null && ( +
+ ↩ reverted to v{d.revertedToNumber} +
+ )} {d.changeCount > 0 && (
{d.changeCount} changed diff --git a/apps/web/src/frontend/lib/migrationReport.test.ts b/apps/web/src/frontend/lib/migrationReport.test.ts new file mode 100644 index 00000000..7d640dcb --- /dev/null +++ b/apps/web/src/frontend/lib/migrationReport.test.ts @@ -0,0 +1,129 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { buildMigrationReport, migrationReportFilename } from './migrationReport'; +import type { SchemaCompareResult } from './types'; + +const META = { + originalLabel: 'Version 2', + targetLabel: 'Version 3', + databaseLabel: '[postgres] localhost/foxdb.demo_b', + generatedAt: new Date('2026-08-16T12:34:56.000Z'), +}; + +const COMPARE: SchemaCompareResult = { + summary: { added: 1, removed: 0, modified: 1, unchanged: 4 }, + tables: [ + { + tableName: 'CUSTOMERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { name: 'id', status: 'UNCHANGED', source: { type: 'integer', nullable: false } }, + { + name: 'email', + status: 'MODIFIED', + source: { type: 'varchar(255)', nullable: false }, + target: { type: 'varchar(100)', nullable: true }, + }, + { name: 'phone', status: 'ADDED', source: { type: 'varchar(20)', nullable: true } }, + { name: 'fax', status: 'REMOVED', target: { type: 'varchar(20)', nullable: true } }, + ], + indexDiffs: [ + { + name: 'IDX_EMAIL', + status: 'ADDED', + source: { name: 'idx_email', columns: ['email'], unique: false }, + }, + ], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + { + tableName: 'AUDIT_LOG', + objectType: 'TABLE', + status: 'ADDED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + { + tableName: 'ORDERS', + objectType: 'TABLE', + status: 'UNCHANGED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + triggerDiffs: [], + }, + ], +}; + +describe('buildMigrationReport', () => { + const md = buildMigrationReport(COMPARE, META); + + it('never contains SQL — that is the whole point of this report', () => { + // The Migration SQL tab answers "what will run"; this answers "what + // changed", for a reader who does not read DDL. + // Keywords only. Banning `;` as well was too crude — English prose uses + // punctuation, and the first version of this failed on its own sentence. + expect(md).not.toMatch(/\b(ALTER|CREATE|DROP|SELECT|INSERT|UPDATE|DELETE)\b/i); + }); + + it('names both sides, the database and the time', () => { + expect(md).toContain('Version 2 → Version 3'); + expect(md).toContain('[postgres] localhost/foxdb.demo_b'); + expect(md).toContain('2026-08-16 12:34 UTC'); + }); + + it('summarises counts as a table', () => { + expect(md).toContain('| Added | 1 |'); + expect(md).toContain('| Changed | 1 |'); + expect(md).toContain('| Unchanged | 4 |'); + }); + + it('lists only the objects that differ', () => { + expect(md).toContain('`CUSTOMERS`'); + expect(md).toContain('`AUDIT_LOG`'); + expect(md).not.toContain('`ORDERS`'); + }); + + it('describes column changes in words, with the direction of the change', () => { + expect(md).toContain('Added column `phone` (varchar(20))'); + expect(md).toContain('Removed column `fax`'); + // Old → new, and the nullability flip stated plainly. + expect(md).toContain('varchar(100) → varchar(255)'); + expect(md).toContain('now required'); + }); + + it('uses the index own name rather than the uppercased compare key', () => { + expect(md).toContain('`idx_email`'); + expect(md).not.toContain('`IDX_EMAIL`'); + }); + + it('says so plainly when an object changed but has nothing to list', () => { + expect(md).toMatch(/No column, index or constraint changes were recorded/i); + }); + + it('reports an identical pair without inventing sections', () => { + const same = buildMigrationReport( + { summary: { added: 0, removed: 0, modified: 0, unchanged: 9 }, tables: [] }, + META + ); + expect(same).toContain('No objects differ'); + expect(same).not.toContain('## Details'); + }); +}); + +describe('migrationReportFilename', () => { + it('slugs both sides into a safe name', () => { + expect(migrationReportFilename(META)).toBe('schema-report-version-2-to-version-3.md'); + expect( + migrationReportFilename({ ...META, targetLabel: 'Current database (Version 4)' }) + ).toBe('schema-report-version-2-to-current-database-version-4.md'); + }); +}); diff --git a/apps/web/src/frontend/lib/migrationReport.ts b/apps/web/src/frontend/lib/migrationReport.ts new file mode 100644 index 00000000..c329e665 --- /dev/null +++ b/apps/web/src/frontend/lib/migrationReport.ts @@ -0,0 +1,156 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * A schema change report in Markdown, for people who do not read DDL. + * + * The Migration SQL tab already answers "what will run". This answers "what + * changed", for the reviewer, the ticket, or the change-approval board — so it + * deliberately contains **no SQL at all**. A column widening reads "email: + * varchar(100) → varchar(255)", not an ALTER statement. + * + * Pure and string-only: no DOM, no download, no store. The caller decides what + * to do with the text, and the shape of the output can be asserted directly. + */ +import type { SchemaCompareResult, TableDiff } from './types'; + +export interface MigrationReportMeta { + /** e.g. "Version 2" — the reference side. */ + originalLabel: string; + /** e.g. "Version 3" or "Current database". */ + targetLabel: string; + /** Database the history belongs to, as shown in the picker. */ + databaseLabel?: string; + /** Defaults to now; injected so tests are not clock-dependent. */ + generatedAt?: Date; +} + +const STATUS_WORD: Record = { + ADDED: 'Added', + REMOVED: 'Removed', + MODIFIED: 'Changed', + UNCHANGED: 'Unchanged', +}; + +/** `varchar(100) → varchar(255)`, or one side when the column exists once. */ +function typeChange(from?: string, to?: string): string { + if (from && to && from !== to) return `${from} → ${to}`; + return to ?? from ?? ''; +} + +/** Plain-language lines for what moved inside one object. */ +function objectDetails(diff: TableDiff): string[] { + const lines: string[] = []; + + for (const column of diff.columnDiffs) { + if (column.status === 'UNCHANGED') continue; + if (column.status === 'ADDED') { + const type = column.source?.type ?? ''; + lines.push(`- Added column \`${column.name}\`${type ? ` (${type})` : ''}`); + } else if (column.status === 'REMOVED') { + lines.push(`- Removed column \`${column.name}\``); + } else { + const change = typeChange(column.target?.type, column.source?.type); + const bits: string[] = []; + if (change.includes('→')) bits.push(change); + if (column.source && column.target && column.source.nullable !== column.target.nullable) { + bits.push(column.source.nullable ? 'now nullable' : 'now required'); + } + lines.push( + `- Changed column \`${column.name}\`${bits.length ? `: ${bits.join(', ')}` : ''}` + ); + } + } + + for (const index of diff.indexDiffs) { + if (index.status === 'UNCHANGED') continue; + const columns = (index.source ?? index.target)?.columns?.join(', ') ?? ''; + const name = index.source?.name ?? index.target?.name ?? index.name; + lines.push( + `- ${STATUS_WORD[index.status] ?? index.status} index \`${name}\`${columns ? ` on (${columns})` : ''}` + ); + } + + for (const fk of diff.foreignKeyDiffs) { + if (fk.status === 'UNCHANGED') continue; + const info = fk.source ?? fk.target; + const to = info?.referencedTable ? ` → \`${info.referencedTable}\`` : ''; + lines.push(`- ${STATUS_WORD[fk.status] ?? fk.status} foreign key \`${fk.name}\`${to}`); + } + + for (const trigger of diff.triggerDiffs ?? []) { + if (trigger.status === 'UNCHANGED') continue; + const name = trigger.source?.name ?? trigger.target?.name ?? trigger.name; + lines.push(`- ${STATUS_WORD[trigger.status] ?? trigger.status} trigger \`${name}\``); + } + + return lines; +} + +/** `# Schema change report` … in Markdown. Never contains DDL. */ +export function buildMigrationReport( + compare: SchemaCompareResult, + meta: MigrationReportMeta +): string { + const when = (meta.generatedAt ?? new Date()).toISOString().slice(0, 16).replace('T', ' '); + const changed = compare.tables.filter((t) => t.status !== 'UNCHANGED'); + + const out: string[] = [ + '# Schema change report', + '', + `**Comparing:** ${meta.originalLabel} → ${meta.targetLabel}`, + ]; + if (meta.databaseLabel) out.push(`**Database:** ${meta.databaseLabel}`); + out.push(`**Generated:** ${when} UTC`, ''); + + out.push( + '## Summary', + '', + '| Change | Objects |', + '| --- | ---: |', + `| Added | ${compare.summary.added} |`, + `| Changed | ${compare.summary.modified} |`, + `| Removed | ${compare.summary.removed} |`, + `| Unchanged | ${compare.summary.unchanged} |`, + '' + ); + + if (changed.length === 0) { + out.push('No objects differ between these two versions.', ''); + return out.join('\n'); + } + + out.push('## Objects changed', '', '| Object | Type | Change |', '| --- | --- | --- |'); + for (const table of changed) { + out.push( + `| \`${table.tableName}\` | ${table.objectType} | ${STATUS_WORD[table.status] ?? table.status} |` + ); + } + out.push(''); + + out.push('## Details', ''); + for (const table of changed) { + out.push(`### ${table.tableName}`, '', `${STATUS_WORD[table.status] ?? table.status} ${table.objectType.toLowerCase()}.`, ''); + const details = objectDetails(table); + if (details.length > 0) { + out.push(...details, ''); + } else { + // A view or routine whose body changed has no child diffs to list, and + // saying so beats an empty heading that looks like a rendering bug. + out.push('_No column, index or constraint changes were recorded for this object._', ''); + } + } + + return out.join('\n'); +} + +/** Filename-safe slug for the downloaded file. */ +export function migrationReportFilename(meta: MigrationReportMeta): string { + const slug = (text: string) => + text + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-|-$/g, ''); + return `schema-report-${slug(meta.originalLabel)}-to-${slug(meta.targetLabel)}.md`; +} diff --git a/apps/web/src/frontend/store/lokeeHistoryStore.ts b/apps/web/src/frontend/store/lokeeHistoryStore.ts index b0dc750b..c1767a02 100644 --- a/apps/web/src/frontend/store/lokeeHistoryStore.ts +++ b/apps/web/src/frontend/store/lokeeHistoryStore.ts @@ -40,6 +40,9 @@ interface LokeeHistoryState { requestCapture: () => void; refreshRequest: number; requestRefresh: () => void; + /** Bumped by the Target card's Compare button; the graph view opens the modal. */ + compareRequest: number; + requestCompare: () => void; } export const useLokeeHistoryStore = create((set, get) => ({ @@ -64,6 +67,8 @@ export const useLokeeHistoryStore = create((set, get) => ({ requestCapture: () => set({ captureRequest: get().captureRequest + 1 }), refreshRequest: 0, requestRefresh: () => set({ refreshRequest: get().refreshRequest + 1 }), + compareRequest: 0, + requestCompare: () => set({ compareRequest: get().compareRequest + 1 }), swapSides: () => { const next = swapHistoryCompare(get().versions, { originalVersionId: get().originalVersionId, diff --git a/apps/web/src/frontend/store/uiStore.ts b/apps/web/src/frontend/store/uiStore.ts index 079babfb..27793989 100644 --- a/apps/web/src/frontend/store/uiStore.ts +++ b/apps/web/src/frontend/store/uiStore.ts @@ -178,7 +178,14 @@ function applyToDocument(themeMode: ThemeMode, tone: ToneId, fontSize: FontSize, /** Top-level workspace views: schema sync (compare + history) vs the SQL Editor. */ export type ActiveView = 'sync' | 'sqlEditor'; /** Compare tree vs Lokee schema-history graph, both inside Schema Sync. */ -export type SyncPane = 'compare' | 'history'; +/** + * Browse is its own pane, not a mode hiding inside Compare. It answers a + * different question — "what is in this one database?" rather than "how do + * these two differ?" — and reaching it by pressing a button on one of Compare's + * two connection cards left the app showing a comparison workspace with no + * comparison in it. + */ +export type SyncPane = 'compare' | 'browse' | 'history'; interface UiState { themeMode: ThemeMode; @@ -189,7 +196,7 @@ interface UiState { resolvedMode: 'dark' | 'light'; /** Which workspace view is showing (persisted; purely local, not synced to server prefs). */ activeView: ActiveView; - /** Compare vs schema history, only meaningful when `activeView === 'sync'`. */ + /** Compare / browse / history, only meaningful when `activeView === 'sync'`. */ syncPane: SyncPane; /** Bumped after a Lokee capture so the history graph reloads. */ lokeeEpoch: number; @@ -230,7 +237,7 @@ function migrateUiPersist(persisted: unknown, _version: number): unknown { state.activeView = 'sync'; state.syncPane = 'history'; } - if (state.syncPane !== 'history' && state.syncPane !== 'compare') { + if (!['history', 'compare', 'browse'].includes(state.syncPane as string)) { state.syncPane = 'compare'; } if (typeof state.lokeeEpoch !== 'number') state.lokeeEpoch = 0; diff --git a/apps/web/src/frontend/store/useSyncStore.ts b/apps/web/src/frontend/store/useSyncStore.ts index 7d77694a..833e8c76 100644 --- a/apps/web/src/frontend/store/useSyncStore.ts +++ b/apps/web/src/frontend/store/useSyncStore.ts @@ -461,12 +461,19 @@ export const useSyncStore = create()( selectedTable: result.tables[0] || null, isBrowsing: false, }); + // Browse is a pane, so reading a schema takes you there. Before this the + // result landed in the Compare workspace, which then showed a comparison + // layout holding one database and no comparison. + useUiStore.getState().setSyncPane('browse'); } catch (e: any) { set({ errorMsg: e.message || `Failed to load ${side} schema`, isBrowsing: false }); } }, runSchemaComparison: async () => { + // The mirror of browseSchema: comparing leaves Browse behind, so the pane + // and the state it renders can never disagree. + useUiStore.getState().setSyncPane('compare'); set({ isComparing: true, errorMsg: null, warnings: [], compareResult: null, selectedTable: null, generatedSql: null, migrationExecuted: false, browseMode: false, browseSide: null }); try { // Re-read the available schemas so the comparison runs against current server state diff --git a/apps/web/src/shared/lokee-wire.ts b/apps/web/src/shared/lokee-wire.ts index cf9148cc..4596900b 100644 --- a/apps/web/src/shared/lokee-wire.ts +++ b/apps/web/src/shared/lokee-wire.ts @@ -88,6 +88,13 @@ export interface VersionSummary { description?: string; objectCount: number; changeCount: number; + /** + * Set only on a version a revert produced: the head the database was at, and + * the version that was restored. `source: 'revert'` says an undo happened; + * these say which one, which is the question you ask when reading it back. + */ + revertFromVersionId?: string; + revertToVersionId?: string; } export interface ObjectHistoryEntry { @@ -160,6 +167,10 @@ export interface VersionGraphVersion { /** Optional display name; falls back to `Version ${number}`. */ name?: string; description?: string; + /** How the version came to exist — a revert node is worth marking. */ + source?: CaptureSource; + /** Version number this revert restored, when this version is one. */ + revertedToNumber?: number; } export interface VersionGraphObject { From 19637b30e0953c502d5c4f4b0c8f2b7be13c360e Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:54:23 -0600 Subject: [PATCH 04/14] fix(lokee): scope a revert to the objects actually ticked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a revert could rewrite a database nobody asked it to touch. Executing with **zero** objects ticked sent `objectKeys: undefined`, which the backend reads as "the whole schema" — one click reverted an entire database from a dialog where nothing was selected. An empty tick set is now refused, and `objectKeys` is always sent explicitly. Since that made a whole-schema revert unreachable by accident, Select all / Clear plus an `N of M ticked` counter keep the destructive path available but deliberate. Worse, `planRevert` filtered the *risk verdicts* by the ticked keys while handing the **unfiltered** state maps to the compare that generates the SQL. The dialog said "1 object" and the migration rewrote every table. Both maps are narrowed now. Ticking a lone child also carries its `table:` container along as context — `hydrateTableSchemas` drops any group without one, so the plan came back empty — but only when that container exists on both sides, so it can never turn a one-column tick into a DROP TABLE. The existing tests asserted only on `reversal.verdicts`, which is exactly how the statement generator drifted unnoticed; the new ones assert on `statements`. Also fixes a stale `blocked` memo (missing `selectedKeys`/`changed` deps left the button saying "Tick objects to revert" after you had ticked one), and hides the Compare button rather than disabling it when both sides resolve to one version. Co-Authored-By: Claude Opus 5 --- apps/e2e/src/pages/LokeeHistoryPage.ts | 28 +++++- .../tests/schema-version-revert-edges.test.ts | 47 +++++++++- .../modules/lokee-weave.module.test.ts | 91 +++++++++++++++++++ .../src/backend/modules/lokee-weave.module.ts | 41 ++++++++- .../lokee-weave/HistoryCompareBar.tsx | 10 +- .../lokee-weave/VersionCompareModal.tsx | 44 ++++++++- 6 files changed, 249 insertions(+), 12 deletions(-) diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index 7a022357..b3d006b8 100644 --- a/apps/e2e/src/pages/LokeeHistoryPage.ts +++ b/apps/e2e/src/pages/LokeeHistoryPage.ts @@ -54,7 +54,10 @@ export class LokeeHistoryPage { await this.page.waitForFunction( (expected) => { const el = document.querySelector('[data-testid="lokee-summary"]'); - const text = el?.textContent ?? ''; + // innerText, not textContent: the summary is a row of sibling spans, so + // textContent runs them together as "…2 versions10 objects…" and the + // trailing \b never matches — `s` meets `1`, both word characters. + const text = (el as HTMLElement | null)?.innerText ?? ''; return new RegExp(`\\b${expected}\\s+versions?\\b`, 'i').test(text); }, n, @@ -309,9 +312,30 @@ export class LokeeHistoryPage { await box.click(); } + /** + * Tick every changed object. Required before Execute: an empty tick set is + * refused rather than silently widening to the whole schema, which is what + * used to revert an entire database from a dialog with nothing selected. + */ + async selectAllCompareObjects(): Promise { + await clickWhen(this.page, '[data-testid="lokee-cmp-select-all"]'); + } + async compareObjectNames(): Promise { const items = this.page.locator('[data-testid="diff-item"]'); - await items.first().waitFor({ state: 'visible', timeout: 20_000 }); + try { + await items.first().waitFor({ state: 'visible', timeout: 20_000 }); + } catch (error) { + // "no diff rows" and "the dialog says the versions are identical" look + // the same from a timeout, and they have opposite causes. + const open = await this.compareModalOpen(); + const identical = await this.compareIdenticalVisible().catch(() => false); + const summary = await this.compareSummaryText().catch(() => ''); + throw new Error( + `No objects in the compare tree. modalOpen=${open} identical=${identical} summary=${summary}`, + { cause: error } + ); + } const count = await items.count(); const names: string[] = []; for (let i = 0; i < count; i++) { diff --git a/apps/e2e/src/tests/schema-version-revert-edges.test.ts b/apps/e2e/src/tests/schema-version-revert-edges.test.ts index c57a9fd0..39f76160 100644 --- a/apps/e2e/src/tests/schema-version-revert-edges.test.ts +++ b/apps/e2e/src/tests/schema-version-revert-edges.test.ts @@ -71,7 +71,10 @@ async function boot( // ── 1. Versioning: capture, pickers, preview ──────────────────────────────── describe.skipIf(!ready)('History · versioning edge cases (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-edges`; const DIR = `/tmp/foxschema-e2e-version-edges-${RUN}`; const DB = join(DIR, 'versions.db'); const NAME = `E2E Versions ${RUN}`; @@ -197,7 +200,10 @@ INSERT INTO invoices (id, total) VALUES (1, 10); // ── 2. Revert: no-op ticks, scoped object, new version ─────────────────────── describe.skipIf(!ready)('History · revert scope edge cases (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-scope`; const DIR = `/tmp/foxschema-e2e-revert-scope-${RUN}`; const DB = join(DIR, 'scope.db'); const NAME = `E2E Revert Scope ${RUN}`; @@ -231,7 +237,19 @@ INSERT INTO invoices (id, total) VALUES (1, 10); await history.openHistoryPane(); await history.selectHistoryDatabaseContaining(RUN); await history.waitForGraph(); - await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + // `expect.poll` is only legal inside a test — this is a beforeAll hook, so + // wait on the page instead. The page object already owns that wait. + try { + await history.waitForVersionCount(2); + } catch (error) { + // A bare timeout here says "setup failed" and nothing else; the summary + // line names the database actually on screen, which is the difference + // between a missed capture and the wrong database being selected. + throw new Error( + `Setup never reached 2 versions.\nSummary: ${await history.summaryText()}`, + { cause: error } + ); + } }, 180_000); afterAll(async () => { @@ -305,7 +323,10 @@ INSERT INTO invoices (id, total) VALUES (1, 10); // ── 3. Revert: lossy ack, drop-table, pendulum ─────────────────────────────── describe.skipIf(!ready)('History · revert lossy and pendulum (SQLite)', () => { - const RUN = Date.now().toString(36); + // Suffixed per suite: all three describes evaluate in the same + // millisecond, so a bare timestamp is not unique and the history + // picker matched more than one database. + const RUN = `${Date.now().toString(36)}-lossy`; const DIR = `/tmp/foxschema-e2e-revert-lossy-${RUN}`; const DB = join(DIR, 'lossy.db'); const NAME = `E2E Revert Lossy ${RUN}`; @@ -347,7 +368,19 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.openHistoryPane(); await history.selectHistoryDatabaseContaining(RUN); await history.waitForGraph(); - await expect.poll(async () => history.versionCount(), { timeout: 30_000 }).toBe(2); + // `expect.poll` is only legal inside a test — this is a beforeAll hook, so + // wait on the page instead. The page object already owns that wait. + try { + await history.waitForVersionCount(2); + } catch (error) { + // A bare timeout here says "setup failed" and nothing else; the summary + // line names the database actually on screen, which is the difference + // between a missed capture and the wrong database being selected. + throw new Error( + `Setup never reached 2 versions.\nSummary: ${await history.summaryText()}`, + { cause: error } + ); + } }, 180_000); afterAll(async () => { @@ -359,6 +392,8 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 1'); await history.selectTargetCurrent(); await history.openCompareModal(); + // Zero ticks is refused by design, so say "all of it" explicitly. + await history.selectAllCompareObjects(); const run = driver.locator('[data-testid="lokee-cmp-run-revert"]'); await run.waitFor({ state: 'visible', timeout: 20_000 }); @@ -387,6 +422,7 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 1'); await history.selectTargetCurrent(); await history.openCompareModal(); + await history.selectAllCompareObjects(); const calls: string[] = []; driver.on('response', (res) => { @@ -424,6 +460,7 @@ INSERT INTO audit (id, body) VALUES (1, 'gone on revert'); await history.selectOriginalVersion('Version 2'); await history.selectTargetCurrent(); await history.openCompareModal(); + await history.selectAllCompareObjects(); await history.executeRevert(); try { diff --git a/apps/web/src/backend/modules/lokee-weave.module.test.ts b/apps/web/src/backend/modules/lokee-weave.module.test.ts index 52fcab7c..b1e20b2b 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.test.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.test.ts @@ -933,6 +933,46 @@ describe('selective revert', () => { expect(plan!.reversal.risk).toBe('lossy'); }); + it('generates DDL for the selection only, not for the whole schema', async () => { + // The verdicts above are the *classification*; these are what actually runs. + // They were filtered independently, and only the verdicts were narrowed — so + // the dialog said "1 object" while the migration rewrote every table. Assert + // on the statements, or the next divergence goes unnoticed again. + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'table:ORDERS', + ]); + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/orders/i); + // v1's customer.email is varchar(100) and HEAD's is varchar(255): a + // whole-schema revert would narrow it here. The user did not tick it. + expect(sql).not.toMatch(/customer/i); + }); + + it('reverts the ticked column without touching the sibling table', async () => { + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'column:CUSTOMER.EMAIL', + ]); + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/customer/i); + expect(sql).toMatch(/varchar\(100\)/i); + // ORDERS arrived in v2 and a whole-schema revert would drop it. + expect(sql).not.toMatch(/\borders\b/i); + }); + + it('will not drop a whole table because one of its columns was ticked', async () => { + // ORDERS arrived in v2, so reverting to v1 means it should not exist at all. + // Pulling its container in to satisfy hydration must not turn a one-column + // tick into a DROP TABLE — widening past the tick is the bug this whole + // group exists to prevent. Tick the table itself for that. + const { weave, databaseId, v1 } = await twoChanges(); + const plan = await weave.planRevert(USER, databaseId, v1.versionId, 'postgres', 'public', [ + 'column:ORDERS.ID', + ]); + expect(plan!.statements.join('\n')).not.toMatch(/drop table/i); + }); + it('treats an empty selection as nothing, not as everything', async () => { // A UI with no boxes ticked sends `[]`. If that were read as "no filter", // clicking Revert with nothing selected would rewrite the whole database. @@ -948,6 +988,57 @@ describe('selective revert', () => { }); }); +describe('awkward names survive a capture', () => { + // Object keys are `kind:OWNER.CHILD`, so a name holding a dot is the one that + // can corrupt the addressing scheme the whole history is built on. + const AWKWARD = table('Order Details', [ + ['order id', 'integer', false], + ['a.b', 'text'], + ]); + + it('reconstructs a table whose name and columns contain spaces', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const at = await weave.objectsAtVersion(USER, v1.databaseId, v1.versionId); + + // The container is addressable and keeps its real name for DDL. + const container = at.get('table:ORDER DETAILS'); + expect(container?.body.name).toBe('Order Details'); + + const spaced = at.get('column:ORDER DETAILS.ORDER ID'); + expect(spaced?.body.name).toBe('order id'); + }); + + it('does not lose a column whose name contains a dot', async () => { + // `a.b` makes the key `column:ORDER DETAILS.A.B`, which owner-parsing splits + // at the first dot. The column must still round-trip to its own name — if + // this ever regresses, the blueprint and every revert plan lose the column. + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const at = await weave.objectsAtVersion(USER, v1.databaseId, v1.versionId); + + const dotted = [...at.values()].filter((o) => o.body.name === 'a.b'); + expect(dotted).toHaveLength(1); + }); + + it('plans a revert for an awkward table without inventing changes', async () => { + const { weave } = await freshStore(); + const v1 = await weave.capture(USER, { ...IDENTITY, tables: [AWKWARD], source: 'manual' }); + const v2 = await weave.capture(USER, { + ...IDENTITY, + tables: [table('Order Details', [['order id', 'integer', false]])], + source: 'migrate', + }); + expect(v2.changed).toBe(true); + + const plan = await weave.planRevert(USER, v2.databaseId, v1.versionId, 'sqlite'); + // Reverting restores the dropped column, and the DDL must carry the real + // name — quoted, since it cannot be written bare. + const sql = plan!.statements.join('\n'); + expect(sql).toMatch(/"a\.b"|`a\.b`|\[a\.b]/); + }); +}); + describe('version compare (shared Compare engine)', () => { it('reads forward: what the newer version added', async () => { // Direction is the trap. CompareModule answers "what must TARGET change to diff --git a/apps/web/src/backend/modules/lokee-weave.module.ts b/apps/web/src/backend/modules/lokee-weave.module.ts index 3a6157be..3398f402 100644 --- a/apps/web/src/backend/modules/lokee-weave.module.ts +++ b/apps/web/src/backend/modules/lokee-weave.module.ts @@ -1184,6 +1184,40 @@ export class LokeeWeaveStore { return owner ? selectedOwners!.has(owner) : false; }; + /** + * Both sides narrowed to the ticked objects, so the generated DDL touches + * only them. + * + * Filtering `entries` alone classified the *risk* of the selection while + * the statements below still reverted everything: the dialog said "1 object" + * and the migration rewrote every table in the schema. + * + * Ticking a lone child needs its `table:` container carried along as + * context, because `hydrateTableSchemas` drops any group without one — a + * column with no table is not a schema, and the plan came back empty. + * That container is context only: it is added just when it exists on *both* + * sides, so it contributes the table's shape and never a CREATE/DROP of a + * table nobody ticked. A child whose container exists on one side only + * therefore reverts nothing on its own; tick the table for that. + */ + const contextOwners = selected + ? new Set( + [...new Set([...current.keys(), ...desired.keys()])] + .filter((key) => wanted(key)) + .map((key) => objectKeyOwner(key)) + ) + : null; + const isContainerFor = (key: string, owners: ReadonlySet): boolean => + !key.includes('.') && owners.has(objectKeyOwner(key)) && current.has(key) && desired.has(key); + const inSelection = ( + objects: ReadonlyMap + ): ReadonlyMap => + contextOwners + ? new Map( + [...objects].filter(([key]) => wanted(key) || isContainerFor(key, contextOwners)) + ) + : objects; + const entries: Array<{ key: string; current?: CanonicalObject; target?: CanonicalObject }> = []; for (const key of new Set([...current.keys(), ...desired.keys()])) { if (!wanted(key)) continue; @@ -1214,7 +1248,12 @@ export class LokeeWeaveStore { // then the same SQL generator the live migrate flow uses. const migration = dialectName ? migrationFromCompare( - await this.compareVersionStates(desired, current, dialectName, schemaName), + await this.compareVersionStates( + inSelection(desired), + inSelection(current), + dialectName, + schemaName + ), dialectName, { targetSchema: schemaName, sourceSchema: schemaName } ) diff --git a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx index bae14ac0..79bafbaf 100644 --- a/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx +++ b/apps/web/src/frontend/components/lokee-weave/HistoryCompareBar.tsx @@ -137,18 +137,24 @@ export function HistoryCompareBar(): React.ReactElement { {/* The diff belongs to the pair, and the pair is finished being chosen here — so the button that opens it sits with the second side rather - than on a strip of its own below the bar. */} + than on a strip of its own below the bar. + + Absent rather than disabled when the two sides resolve to the same + version: with a single-version history there is no pair and never + will be until another capture lands, so a permanently dead control + is just clutter. */} + {!sameSides && newestFirst.length >= 2 && ( + )}
diff --git a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx index 74189656..54d398c4 100644 --- a/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx +++ b/apps/web/src/frontend/components/lokee-weave/VersionCompareModal.tsx @@ -226,6 +226,16 @@ export function VersionCompareModal({ if (plan.statements.length === 0) { return { code: 'empty', label: 'Nothing to apply', why: 'The plan is empty.' }; } + // An empty tick set used to send `undefined`, which the backend reads as + // "the whole schema" — so pressing Execute with nothing selected reverted + // the entire database. Selecting nothing must mean nothing. + if (changed.length > 0 && selectedKeys.length === 0) { + return { + code: 'nothing-ticked', + label: 'Tick objects to revert', + why: 'Tick the objects to revert in the tree, or use Select all.', + }; + } if (plan.reversal.risk === 'blocked') { return { code: 'blocked', @@ -241,7 +251,10 @@ export function VersionCompareModal({ }; } return null; - }, [captureConnectionId, plan, planning, confirmLossy]); + // selectedKeys and changed belong here: without them, ticking an object + // left this memo stale and the button kept saying "Tick objects to revert" + // after the user had ticked one. + }, [captureConnectionId, plan, planning, confirmLossy, selectedKeys, changed]); const needsLossyAck = blocked?.code === 'lossy'; const runRevert = useCallback(async () => { @@ -253,7 +266,9 @@ export function VersionCompareModal({ connectionId: captureConnectionId, password: getSessionPassword(captureConnectionId), confirmLossy, - objectKeys: selectedKeys.length > 0 ? selectedKeys : undefined, + // Always explicit: the guard above refuses to run with an empty tick + // set, so this never silently widens to the whole schema. + objectKeys: selectedKeys, }); toast({ tone: 'success', @@ -423,6 +438,31 @@ export function VersionCompareModal({ // gesture, one component.
+
+ + + + {selectedKeys.length} of {changed.length} ticked + +
Date: Sun, 16 Aug 2026 20:54:40 -0600 Subject: [PATCH 05/14] fix(sql): emit DDL that parses, for names and types a real catalog contains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by two new harnesses rather than by hand: a seeded generator that builds adversarial schemas and checks properties across all 14 dialects at once, and a test that hands the generated DDL to a real `node:sqlite` engine and lets it judge. String assertions only ever prove the output matches what somebody expected it to be. * **Identifiers were never quoted.** A table called `Order Details` — Northwind ships one — produced unparseable SQL on every dialect, as did a column called `order id` or `select`. Adds an optional `quoteIdentifier` dialect hook (ANSI default; backticks for MySQL/MariaDB/TiDB, brackets for SQL Server/Azure), applied only when a name cannot be written bare, so every ordinary name is byte-identical to what this generator emitted before. `ident` is idempotent, which lets ADD COLUMN and CREATE INDEX be fixed for all 14 at one call site instead of in fourteen hooks. * **`ColumnDiff.name` is the uppercased compare key**, the same trap as `tableName`, and the ALTER paths emitted it — renaming a user's `new col` to `NEW COL`. Now uses `source?.name ?? target?.name`; the field is optional because this package is published. * **`ALTER TABLE … ADD CONSTRAINT` was emitted for SQLite and ClickHouse**, which reject it outright. `dialectSupportsFk` already knew this and only the blueprint UI was reading it. FKs now inline into CREATE TABLE where the dialect allows it, and otherwise emit `-- review:` — never DDL that cannot run. * **Decimal precision was dropped.** `NUMBER(10)` / `DECIMAL(10)` tokenize their single argument as a *length*, which `shapeCanonical` ignored for decimals, so an Oracle `NUMBER(10)` column silently widened to full 38-digit precision on every migration. Fixed in the shared shaper, not per dialect. * **Redshift rendered `varchar(max)`** for TEXT and XML. That is T-SQL syntax Redshift rejects; its documented maximum is `varchar(65535)`. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-runs.test.ts | 217 +++++++++ .../src/interfaces/diff.types.interface.ts | 13 +- packages/sql/src/modules/schema-fuzz.test.ts | 410 ++++++++++++++++++ .../sql/src/modules/sql-dialect.interface.ts | 11 + .../sql/src/modules/sql-generator.module.ts | 178 +++++++- packages/sql/src/modules/type-mapping.ts | 9 +- .../src/providers/mysql/mysql.sql-dialect.ts | 6 + .../redshift/redshift.sql-dialect.ts | 6 +- .../sqlServer/sqlserver.sql-dialect.ts | 6 + 9 files changed, 836 insertions(+), 20 deletions(-) create mode 100644 apps/web/src/backend/modules/generated-ddl-runs.test.ts create mode 100644 packages/sql/src/modules/schema-fuzz.test.ts diff --git a/apps/web/src/backend/modules/generated-ddl-runs.test.ts b/apps/web/src/backend/modules/generated-ddl-runs.test.ts new file mode 100644 index 00000000..f8ccfdb3 --- /dev/null +++ b/apps/web/src/backend/modules/generated-ddl-runs.test.ts @@ -0,0 +1,217 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Generated DDL, executed by a real SQL engine. + * + * Every other generator test asserts on strings, which only ever proves the + * output matches what somebody expected it to be. This hands the output to + * SQLite and lets the engine judge it: if the statement will not parse, the + * test fails with the engine's own error. + * + * It lives in `apps/web` rather than `packages/sql` on purpose — the sql + * package is pure and may not import Node built-ins (`purity.test.ts` enforces + * it), and this needs `node:sqlite`. + * + * SQLite is the only engine reachable without credentials, so it is the only + * one covered here. It still catches the whole class of "the identifier was + * never quoted" bugs, which is dialect-independent. + */ +import { DatabaseSync } from 'node:sqlite'; +import { describe, expect, it } from 'vitest'; +import { CompareModule, SqlGeneratorModule } from '@foxschema/sql'; +import type { TableSchema } from '@foxschema/sql'; + +const gen = new SqlGeneratorModule(); + +/** DDL for creating `tables` from nothing, as the migration flow would emit it. */ +async function createSql(tables: TableSchema[]): Promise { + const result = await new CompareModule().compare(tables, [], { + source: 'sqlite', + target: 'sqlite', + }); + return gen.generateMigrationPlan(result.tables, 'sqlite').flatMap((step) => step.statements); +} + +/** Runs every statement, surfacing the engine's complaint with the statement. */ +function runAll(statements: string[]): void { + const db = new DatabaseSync(':memory:'); + try { + for (const statement of statements) { + try { + db.exec(statement); + } catch (err) { + throw new Error( + `SQLite rejected:\n${statement}\n\n${err instanceof Error ? err.message : String(err)}` + ); + } + } + } finally { + db.close(); + } +} + +const table = (over: Partial & { name: string }): TableSchema => ({ + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + ...over, +}); + +describe('generated DDL actually parses', () => { + it('creates an ordinary table', async () => { + // The control: if this ever fails, the harness is wrong, not the generator. + await expect( + createSql([ + table({ + name: 'customers', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'email', type: 'VARCHAR(255)', nullable: true, primaryKey: false }, + ], + }), + ]).then(runAll) + ).resolves.toBeUndefined(); + }); + + it('creates a table whose name and columns contain spaces', async () => { + // Northwind ships `Order Details`; this is not a hypothetical name. Before + // identifiers were quoted, this produced `CREATE TABLE Order Details (...)`, + // which no engine accepts. + const sql = await createSql([ + table({ + name: 'Order Details', + columns: [ + { name: 'order id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'unit price', type: 'DECIMAL(10,2)', nullable: true, primaryKey: false }, + ], + primaryKey: { name: 'pk order details', columns: ['order id'] }, + }), + ]); + runAll(sql); + }); + + it('creates a table whose columns are reserved words', async () => { + // `select` and `order` are syntax errors bare; `key` and `user` are fine in + // SQLite but reserved in MySQL, so all four are quoted for every dialect. + const sql = await createSql([ + table({ + name: 'order', + columns: [ + { name: 'select', type: 'VARCHAR(10)', nullable: true, primaryKey: false }, + { name: 'order', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'key', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'user', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }), + ]); + runAll(sql); + }); + + it('creates a table with punctuation and non-ASCII letters in names', async () => { + const sql = await createSql([ + table({ + name: 'naïve-table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'café', type: 'TEXT', nullable: true, primaryKey: false }, + { name: 'a.b', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }), + ]); + runAll(sql); + }); + + it('creates a table whose name contains the quote character itself', async () => { + // The escaping case: a name holding `"` must double it, or the quoting + // that was meant to fix the statement is what breaks it. + const sql = await createSql([ + table({ + name: 'we"ird', + columns: [{ name: 'i"d', type: 'INTEGER', nullable: false, primaryKey: false }], + }), + ]); + runAll(sql); + }); + + it('indexes and foreign keys on awkward names still parse', async () => { + const sql = await createSql([ + table({ + name: 'parent table', + columns: [{ name: 'parent id', type: 'INTEGER', nullable: false, primaryKey: true }], + primaryKey: { columns: ['parent id'] }, + }), + table({ + name: 'child table', + columns: [ + { name: 'child id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'parent id', type: 'INTEGER', nullable: true, primaryKey: false }, + ], + primaryKey: { columns: ['child id'] }, + indices: [{ name: 'idx child parent', columns: ['parent id'], unique: false }], + foreignKeys: [ + { + name: 'fk child parent', + columns: ['parent id'], + referencedTable: 'parent table', + referencedColumns: ['parent id'], + }, + ], + }), + ]); + runAll(sql); + }); + + it('alters a table with awkward names', async () => { + // ADD / DROP COLUMN go through per-dialect hooks rather than the CREATE + // path, so they need their own proof. + const before = table({ + name: 'my table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'old col', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }); + const after = table({ + name: 'my table', + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'new col', type: 'TEXT', nullable: true, primaryKey: false }, + ], + }); + + runAll(await createSql([before])); + + const diff = await new CompareModule().compare([after], [before], { + source: 'sqlite', + target: 'sqlite', + }); + const alter = gen.generateMigrationPlan(diff.tables, 'sqlite').flatMap((s) => s.statements); + expect(alter.length).toBeGreaterThan(0); + + // Same database: create the original, then apply the migration to it. + const db = new DatabaseSync(':memory:'); + try { + for (const statement of await createSql([before])) db.exec(statement); + for (const statement of alter) { + try { + db.exec(statement); + } catch (err) { + throw new Error( + `SQLite rejected:\n${statement}\n\n${err instanceof Error ? err.message : String(err)}` + ); + } + } + const columns = db + .prepare(`PRAGMA table_info("my table")`) + .all() + .map((row) => String((row as { name: unknown }).name)); + expect(columns).toContain('new col'); + expect(columns).not.toContain('old col'); + } finally { + db.close(); + } + }); +}); diff --git a/packages/sql/src/interfaces/diff.types.interface.ts b/packages/sql/src/interfaces/diff.types.interface.ts index 8950ab4b..2edb48e8 100644 --- a/packages/sql/src/interfaces/diff.types.interface.ts +++ b/packages/sql/src/interfaces/diff.types.interface.ts @@ -3,10 +3,19 @@ import { type TableSchema, type DbObjectType } from './schema-provider.interface export type DiffType = 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; export interface ColumnDiff { + /** Uppercased compare-key match name — NOT a real identifier, see source.name. */ name: string; status: 'ADDED' | 'REMOVED' | 'MODIFIED' | 'UNCHANGED'; - source?: { type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; - target?: { type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; + /** + * `name` is the column's own identifier in its native casing, which is what + * DDL must use — the same trap as `IndexDiff` below. Compare has always + * passed the whole ColumnInfo through; only this declaration hid the field, + * so `ALTER TABLE … ADD "NEW COL"` was emitted for a column actually called + * `new col`. Optional rather than required because this package is + * published — every producer inside the repo sets it. + */ + source?: { name?: string; type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; + target?: { name?: string; type: string; nullable: boolean; defaultValue?: string; primaryKey?: boolean; identity?: boolean; collation?: string }; } export interface IndexDiff { diff --git a/packages/sql/src/modules/schema-fuzz.test.ts b/packages/sql/src/modules/schema-fuzz.test.ts new file mode 100644 index 00000000..f5eb91a0 --- /dev/null +++ b/packages/sql/src/modules/schema-fuzz.test.ts @@ -0,0 +1,410 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * A generated edge-case sweep across every dialect. + * + * The hand-written dialect tests cover the cases somebody thought of. This + * builds adversarial schemas instead — reserved words as identifiers, mixed + * casing, quote characters inside names, empty tables, composite keys — and + * asserts properties that must hold for **all 14 dialects at once**. Where a + * property is checked per dialect, the failure message names the dialect and + * the seed, so a red run reproduces exactly. + * + * Everything is seeded and deterministic: a fuzz test that finds a different + * bug on every CI run is a flaky test, not a fuzzer. + */ +import { describe, expect, it } from 'vitest'; +import { CompareModule } from './compare.module.js'; +import { SqlGeneratorModule } from './sql-generator.module.js'; +import { DIALECT_MAP } from './dialect-registry.js'; +import type { TableSchema, ColumnInfo } from '../interfaces/index.js'; + +const DIALECTS = Object.keys(DIALECT_MAP); +const gen = new SqlGeneratorModule(); + +/** Deterministic PRNG (mulberry32) — same seed, same schema, every run. */ +function rng(seed: number): () => number { + let a = seed >>> 0; + return () => { + a = (a + 0x6d2b79f5) >>> 0; + let t = Math.imul(a ^ (a >>> 15), 1 | a); + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t; + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; +} + +const pick = (next: () => number, items: readonly T[]): T => + items[Math.floor(next() * items.length) % items.length]!; + +/** + * Identifiers chosen to be awkward on purpose. + * + * Reserved words are the common production break (a column actually called + * `order` or `key`); the casing entries exist because the compare key is + * uppercased and has been mistaken for the real identifier before. + */ +const NAMES = [ + 'customer', + 'CUSTOMER', + 'Customer', + 'order', // reserved almost everywhere + 'select', + 'group', + 'user', + 'table', + 'index', + 'from', + 'primary', + 'mixed_Case_Name', + 'trailing_underscore_', + 'a', // single character + 'x'.repeat(64), // long, but under most identifier limits +]; + +/** + * Names that have to be quoted or escaped to be legal at all. + * + * Kept separate from NAMES so a failure says which class of hostility broke: + * a reserved word is a keyword problem, a name holding the dialect's own quote + * character is an escaping problem, and the two have different fixes. + */ +const NEEDS_QUOTING = [ + 'select', // reserved: a syntax error bare, on every engine + 'order', + 'key', // legal bare in SQLite, reserved in MySQL — quoted for all + 'user', + 'with space', + 'quote"inside', + 'back`tick', + 'bracket]close', + 'semi;colon', + 'dash-name', + 'naïve', + 'UPPER lower', +]; + +const TYPES = [ + 'INTEGER', + 'BIGINT', + 'SMALLINT', + 'VARCHAR(10)', + 'VARCHAR(255)', + 'CHAR(1)', + 'TEXT', + 'DECIMAL(10,2)', + 'DATE', + 'TIMESTAMP', + 'BOOLEAN', +]; + +function column(next: () => number, name: string): ColumnInfo { + return { + name, + type: pick(next, TYPES), + nullable: next() > 0.3, + primaryKey: false, + }; +} + +/** One table with 1..6 awkward columns, sometimes a PK, index or FK. */ +function table(next: () => number, name: string): TableSchema { + const count = 1 + Math.floor(next() * 6); + const used = new Set(); + const columns: ColumnInfo[] = []; + for (let i = 0; i < count; i++) { + // Names are matched case-insensitively, so two columns differing only by + // case are the *same* column — generating both would make an invalid table. + const candidate = `${pick(next, NAMES)}_${i}`; + if (used.has(candidate.toUpperCase())) continue; + used.add(candidate.toUpperCase()); + columns.push(column(next, candidate)); + } + if (columns.length === 0) columns.push(column(next, 'id_0')); + + const schema: TableSchema = { + name, + objectType: 'TABLE', + columns, + indices: [], + foreignKeys: [], + }; + + if (next() > 0.4) { + const pk = columns[0]!; + pk.nullable = false; + pk.primaryKey = true; + schema.primaryKey = { name: `pk_${name}`, columns: [pk.name] }; + } + if (next() > 0.6 && columns.length > 1) { + schema.indices = [ + { + name: `idx_${name}_${columns[1]!.name}`, + columns: [columns[1]!.name], + unique: next() > 0.7, + }, + ]; + } + return schema; +} + +function schemaOf(seed: number): TableSchema[] { + const next = rng(seed); + const count = 1 + Math.floor(next() * 4); + const names = new Set(); + const tables: TableSchema[] = []; + for (let i = 0; i < count; i++) { + const name = `${pick(next, NAMES)}_t${i}`; + if (names.has(name.toUpperCase())) continue; + names.add(name.toUpperCase()); + tables.push(table(next, name)); + } + return tables; +} + +/** A plausible next version: add, drop, widen, retype, toggle nullability. */ +function mutate(tables: TableSchema[], seed: number): TableSchema[] { + const next = rng(seed + 977); + return tables.map((t) => { + const columns = t.columns + .filter(() => next() > 0.2) // drop some + .map((c) => (next() > 0.6 ? { ...c, type: pick(next, TYPES) } : c)) + .map((c) => (next() > 0.8 ? { ...c, nullable: !c.nullable && !c.primaryKey } : c)); + if (next() > 0.5) columns.push(column(next, `added_${Math.floor(next() * 1000)}`)); + // A table must keep at least one column to be a table at all. + return { ...t, columns: columns.length > 0 ? columns : t.columns.slice(0, 1) }; + }); +} + +const SEEDS = Array.from({ length: 40 }, (_, i) => i + 1); + +/** Every statement a migration plan would run, flattened. */ +function planSql(diffs: Awaited>, dialect: string): string[] { + return gen + .generateMigrationPlan(diffs.tables, dialect) + .flatMap((step) => step.statements); +} + +describe('generated schemas · self-compare is empty', () => { + // The most basic promise the product makes: comparing a schema to itself + // reports no work. A false positive here means the app invents a migration + // for two identical databases. + it.each(DIALECTS)('%s reports no changes against itself', async (dialect) => { + for (const seed of SEEDS) { + const tables = schemaOf(seed); + const result = await new CompareModule().compare(tables, tables, { + source: dialect, + target: dialect, + }); + const drifted = result.tables.filter((t) => t.status !== 'UNCHANGED'); + expect( + drifted.map((t) => t.tableName), + `${dialect} seed ${seed} invented a change` + ).toEqual([]); + expect(planSql(result, dialect), `${dialect} seed ${seed} emitted DDL`).toEqual([]); + } + }); +}); + +describe('generated schemas · comparing the other way round', () => { + it.each(DIALECTS)('%s mirrors added and removed when the sides swap', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const forward = await new CompareModule().compare(a, b, { source: dialect, target: dialect }); + const back = await new CompareModule().compare(b, a, { source: dialect, target: dialect }); + + // "What must the target change to match the source" — swapping the sides + // turns every addition into a removal. If these ever disagree the two + // directions are telling the user different stories about one change. + expect(forward.summary.added, `${dialect} seed ${seed}`).toBe(back.summary.removed); + expect(forward.summary.removed, `${dialect} seed ${seed}`).toBe(back.summary.added); + expect(forward.summary.modified, `${dialect} seed ${seed}`).toBe(back.summary.modified); + } + }); +}); + +describe('generated schemas · the compare key never reaches the DDL', () => { + // `tableName` is the uppercased match key, not an identifier. Emitting it is + // a real bug that has shipped here before (and breaks case-sensitive MySQL). + it.each(DIALECTS)('%s writes the table its own name', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const lower = a.filter((t) => t.name !== t.name.toUpperCase()); + if (lower.length === 0) continue; + // Everything is new, so every table is written out in full. + const result = await new CompareModule().compare(a, [], { + source: dialect, + target: dialect, + }); + const sql = planSql(result, dialect).join('\n'); + for (const t of lower) { + expect(sql, `${dialect} seed ${seed} lost the real name of ${t.name}`).toContain(t.name); + } + } + }); +}); + +describe('generated schemas · names that cannot be written bare', () => { + // Every one of these is a legal name in a real database and none of them can + // be emitted raw. The expected wrapper comes from the dialect itself, so a + // dialect that changes its quoting style updates this test with it. + it.each(DIALECTS)('%s quotes hostile table and column names', async (dialect) => { + const quote = + DIALECT_MAP[dialect]!.quoteIdentifier ?? ((n: string) => `"${n.replace(/"/g, '""')}"`); + + for (const name of NEEDS_QUOTING) { + const result = await new CompareModule().compare( + [ + { + name, + objectType: 'TABLE', + columns: [{ name, type: 'INTEGER', nullable: true, primaryKey: false }], + indices: [], + foreignKeys: [], + }, + ], + [], + { source: dialect, target: dialect } + ); + const sql = planSql(result, dialect).join('\n'); + expect(sql, `${dialect} did not quote ${name}`).toContain(quote(name)); + } + }); +}); + +describe('generated schemas · statements are well-formed', () => { + it.each(DIALECTS)('%s emits balanced quotes and parentheses', async (dialect) => { + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const result = await new CompareModule().compare(a, b, { source: dialect, target: dialect }); + for (const statement of planSql(result, dialect)) { + // Strip escaped string literals before counting, so a legitimate + // '' inside a literal does not read as an unbalanced quote. + const withoutLiterals = statement.replace(/''/g, '').replace(/'[^']*'/g, "''"); + const singles = (withoutLiterals.match(/'/g) ?? []).length; + expect(singles % 2, `${dialect} seed ${seed}: odd quote count in ${statement}`).toBe(0); + + const opens = (statement.match(/\(/g) ?? []).length; + const closes = (statement.match(/\)/g) ?? []).length; + expect(opens, `${dialect} seed ${seed}: unbalanced parens in ${statement}`).toBe(closes); + } + } + }); +}); + +describe('type mapping · every dialect against every other', () => { + // The migration path for a cross-dialect move is parse on the source side, + // render on the target side. These are the properties that make that safe. + const NATIVE = [ + ...TYPES, + 'VARCHAR(4000)', + 'NUMERIC(38,10)', + 'DOUBLE PRECISION', + 'TIMESTAMP(6)', + 'CHARACTER VARYING(50)', + ]; + + it.each(DIALECTS)('%s renders a usable type for anything it parses', (dialect) => { + const d = DIALECT_MAP[dialect]!; + for (const native of NATIVE) { + const rendered = d.renderType(d.parseType(native)); + // An empty or `undefined` type reaches the DDL as `col ` and fails at the + // engine — the one outcome that must never happen silently. + expect(rendered.sql?.trim(), `${dialect} rendered nothing for ${native}`).toBeTruthy(); + expect(rendered.sql, `${dialect} rendered undefined for ${native}`).not.toMatch( + /undefined|NaN|\[object/i + ); + } + }); + + it.each(DIALECTS)('%s round-trips its own rendered type unchanged', (dialect) => { + // Rendering must be a fixed point: parse(render(x)) === render(x). If it is + // not, re-comparing a migrated database reports a change that is not there, + // and the tool proposes the same migration forever. + const d = DIALECT_MAP[dialect]!; + for (const native of NATIVE) { + const once = d.renderType(d.parseType(native)).sql; + const twice = d.renderType(d.parseType(once)).sql; + expect(twice, `${dialect}: ${native} → ${once} → ${twice}`).toBe(once); + } + }); + + it('translates every type between every pair of dialects without dropping it', () => { + const losses: string[] = []; + for (const from of DIALECTS) { + for (const to of DIALECTS) { + if (from === to) continue; + for (const native of NATIVE) { + const rendered = DIALECT_MAP[to]!.renderType(DIALECT_MAP[from]!.parseType(native)); + if (!rendered.sql?.trim()) losses.push(`${from} → ${to}: ${native} rendered empty`); + } + } + } + expect(losses, losses.join('\n')).toEqual([]); + }); +}); + +describe('generated schemas · dialects cross-checked against each other', () => { + it('agree on which objects need a migration', async () => { + // The comparison is dialect-aware (type equivalence differs), but *which + // tables changed* should not: a column dropped is dropped everywhere. One + // dialect disagreeing with the other thirteen is the signal worth having. + const disagreements: string[] = []; + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + const byDialect = new Map(); + for (const dialect of DIALECTS) { + const result = await new CompareModule().compare(a, b, { + source: dialect, + target: dialect, + }); + byDialect.set( + dialect, + result.tables + .filter((t) => t.status !== 'UNCHANGED') + .map((t) => `${t.tableName}:${t.status}`) + .sort() + .join(',') + ); + } + const tally = new Map(); + for (const [dialect, shape] of byDialect) { + tally.set(shape, [...(tally.get(shape) ?? []), dialect]); + } + if (tally.size > 1) { + const groups = [...tally.entries()] + .map(([shape, names]) => `${names.join('+')} → ${shape || '(no changes)'}`) + .join(' | '); + disagreements.push(`seed ${seed}: ${groups}`); + } + } + expect(disagreements, disagreements.join('\n')).toEqual([]); + }); + + it('every dialect can express a change it reported', async () => { + // A dialect that reports MODIFIED and then emits nothing has told the user + // there is work to do and quietly refused to do it. Real limits exist + // (SQLite cannot drop arbitrary columns), so this reports rather than + // asserting a hard equality — the list is the finding. + const silent: string[] = []; + for (const seed of SEEDS) { + const a = schemaOf(seed); + const b = mutate(a, seed); + for (const dialect of DIALECTS) { + const result = await new CompareModule().compare(a, b, { + source: dialect, + target: dialect, + }); + const changed = result.tables.filter((t) => t.status !== 'UNCHANGED'); + if (changed.length > 0 && planSql(result, dialect).length === 0) { + silent.push(`${dialect} seed ${seed}: ${changed.length} changed, 0 statements`); + } + } + } + expect(silent, silent.join('\n')).toEqual([]); + }); +}); diff --git a/packages/sql/src/modules/sql-dialect.interface.ts b/packages/sql/src/modules/sql-dialect.interface.ts index 91619bc8..9c382c58 100644 --- a/packages/sql/src/modules/sql-dialect.interface.ts +++ b/packages/sql/src/modules/sql-dialect.interface.ts @@ -120,6 +120,17 @@ export interface SqlDialect { */ nullableTypeWrapper?(typeSql: string, nullable: boolean): string; + /** + * Wrap an identifier that cannot be written bare — a name holding a space, + * punctuation, or a non-ASCII letter, as read from the live catalog. + * + * Omit for ANSI double quotes, which is right for every dialect here except + * MySQL's default mode (backticks) and SQL Server (brackets). The generator + * calls this **only** when the name actually needs it, so ordinary names are + * emitted bare exactly as before. + */ + quoteIdentifier?(name: string): string; + /** * Full ` COLLATE ...` clause (with leading space) for a column's collation, used in * CREATE TABLE / ADD COLUMN. Default: ` COLLATE `, unquoted — correct for diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index 8c28779b..ca214d89 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -1,8 +1,9 @@ -import { type TableDiff } from '../interfaces/index.js'; +import { type TableDiff, type ColumnDiff } from '../interfaces/index.js'; import { type TableSchema, type DbObjectType } from '../interfaces/index.js'; import type { IndexInfo } from '../interfaces/index.js'; import type { SqlDialect, ColumnSpec } from './sql-dialect.interface.js'; import { resolveDialect } from './dialect-registry.js'; +import { dialectSupportsFk, type FkFeatureSupport } from './dialect-fk-support.js'; export interface MigrationStep { objectName: string; @@ -45,10 +46,119 @@ const PROCEDURAL_TYPES: ReadonlySet = new Set(['VIEW', 'FUNCTION', // unlike VIEW/TRIGGER, which are ordered after ALTER (see generateMigrationPlan). const ROUTINE_TYPES: ReadonlySet = new Set(['FUNCTION', 'PROCEDURE']); +/** + * True when a name can be written into SQL exactly as it is. + * + * Deliberately conservative: letters, digits, underscore, `$` and `#` (Oracle + * and DB2 allow the last two bare), never starting with a digit. Anything else + * — a space, a dot, punctuation, a quote character, a non-ASCII letter — has to + * be quoted or the statement is a syntax error. + */ +/** + * Words that must be quoted to be usable as an identifier. + * + * The union across the supported engines, not any single one's list: a column + * called `key` is fine in SQLite and a syntax error in MySQL, and the generator + * emits for whichever dialect it was handed. Quoting a word one engine happens + * not to reserve costs nothing — the name is quoted exactly as the catalog + * spelled it, so it still resolves to the same object. + * + * Deliberately not exhaustive. It covers the clause keywords and the ones that + * turn up as real column names; a full per-dialect list is a bigger job and + * would only add words nobody names a column after. + */ +const RESERVED_WORDS: ReadonlySet = new Set([ + 'ADD', 'ALL', 'ALTER', 'AND', 'ANY', 'AS', 'ASC', 'BETWEEN', 'BOTH', 'BY', + 'CASE', 'CHECK', 'COLUMN', 'CONSTRAINT', 'CREATE', 'CROSS', 'CURRENT', + 'CURRENT_DATE', 'CURRENT_TIME', 'CURRENT_TIMESTAMP', 'CURRENT_USER', + 'DEFAULT', 'DELETE', 'DESC', 'DISTINCT', 'DROP', 'ELSE', 'END', 'EXCEPT', + 'EXISTS', 'FALSE', 'FETCH', 'FOR', 'FOREIGN', 'FROM', 'FULL', 'GRANT', + 'GROUP', 'HAVING', 'IN', 'INDEX', 'INNER', 'INSERT', 'INTERSECT', 'INTO', + 'IS', 'JOIN', 'KEY', 'LEADING', 'LEFT', 'LIKE', 'LIMIT', 'NATURAL', 'NOT', + 'NULL', 'OFFSET', 'ON', 'OR', 'ORDER', 'OUTER', 'PRIMARY', 'REFERENCES', + 'RENAME', 'REVOKE', 'RIGHT', 'ROW', 'ROWS', 'SELECT', 'SESSION_USER', 'SET', + 'SOME', 'TABLE', 'THEN', 'TO', 'TRAILING', 'TRUE', 'UNION', 'UNIQUE', + 'UPDATE', 'USER', 'USING', 'VALUES', 'VIEW', 'WHEN', 'WHERE', 'WITH', +]); + +function isBareIdentifier(name: string): boolean { + if (!/^[A-Za-z_][A-Za-z0-9_$#]*$/.test(name)) return false; + return !RESERVED_WORDS.has(name.toUpperCase()); +} + +/** ANSI quoting, correct everywhere except MySQL's default mode. */ +function ansiQuoteIdentifier(name: string): string { + return `"${name.replace(/"/g, '""')}"`; +} + +/** + * Already wrapped — by an earlier pass here, or by the catalog that handed it + * over. Quoting it again nests the quotes and breaks the statement, and + * `bareName` genuinely is applied twice on some paths (the primary-key name + * reaches it once on the way in and once on the way out). + */ +function isQuotedIdentifier(name: string): boolean { + if (name.length < 2) return false; + const first = name[0]!; + const last = name[name.length - 1]!; + return ( + (first === '"' && last === '"') || + (first === '`' && last === '`') || + (first === '[' && last === ']') + ); +} + export class SqlGeneratorModule { /** Table keys left in an FK cycle after Kahn sort (uppercase bare names). */ private fkCycleKeys = new Set(); + /** + * How this dialect wraps an identifier that cannot be written bare. Set from + * the resolved dialect at each entry point, like `fkCycleKeys` — threading it + * through all 30-odd `qualify`/`bareName` call sites buys nothing. + */ + private quoteIdentifier: (name: string) => string = ansiQuoteIdentifier; + + /** + * Dialect name for the run in progress, for the capability matrices that are + * keyed by name rather than by strategy object (`dialectSupportsFk`). + */ + private dialectName = ''; + + /** What this dialect can do with foreign keys. Empty name ⇒ the full matrix. */ + private fkSupport(): FkFeatureSupport { + return dialectSupportsFk(this.dialectName); + } + + /** + * A name as it should appear in SQL: bare when that is legal, quoted when it + * is not. + * + * Names come from a live catalog, so they are whatever somebody actually + * created — `Order Details` (Northwind ships with it), a column called + * `order id`, a Postgres table created with quotes. Emitting those raw + * produced SQL that could not parse on any dialect. + * + * Quoting only what *needs* it keeps every ordinary name byte-identical to + * what this generator emitted before, which is why this could be fixed + * without rewriting the dialect test suites. + */ + private ident(name: string): string { + if (isBareIdentifier(name) || isQuotedIdentifier(name)) return name; + return this.quoteIdentifier(name); + } + + /** + * A column's real identifier, ready to emit. + * + * `ColumnDiff.name` is the uppercased match key, exactly like `tableName` — + * emitting it renamed `new col` to `NEW COL`. The native casing lives on + * whichever side of the diff exists. + */ + private columnIdent(col: ColumnDiff): string { + return this.ident(col.source?.name ?? col.target?.name ?? col.name); + } + /** * Source catalog definitions qualify names with the source schema (HUY.GPX_FILE); * deploying into a different schema requires rewriting those qualifiers. @@ -64,9 +174,9 @@ export class SqlGeneratorModule { .replace(new RegExp(`\\b${escaped}\\.`, 'gi'), `${tgt}.`); } - /** Drops any leading "schema." prefix from an object name. */ + /** Drops any leading "schema." prefix from an object name, and quotes if needed. */ private bareName(name: string): string { - return name.replace(/^"?[^".]+"?\./, ''); + return this.ident(name.replace(/^"?[^".]+"?\./, '')); } /** @@ -191,7 +301,7 @@ export class SqlGeneratorModule { } else { typeSql = translated.sql + dialect.identityClause(c); } - let def = `${c.name} ${typeSql}`; + let def = `${this.ident(c.name)} ${typeSql}`; // COLLATE goes right after the type on every dialect that supports a per-column // collation (Postgres/MySQL/MariaDB/SQL Server/Oracle 12.2+), before DEFAULT/NOT // NULL. Dialects that never populate collation (DB2) simply never hit this. @@ -213,7 +323,22 @@ export class SqlGeneratorModule { // MySQL names every PK constraint "PRIMARY" — emitting CONSTRAINT PRIMARY PRIMARY KEY // is redundant and confusing. Skip the CONSTRAINT clause for that reserved name. const constraintName = pkName && pkName.toUpperCase() !== 'PRIMARY' ? `CONSTRAINT ${this.bareName(pkName)} ` : ''; - lines.push(` ${constraintName}PRIMARY KEY (${pkCols.join(', ')})`); + lines.push(` ${constraintName}PRIMARY KEY (${pkCols.map((c) => this.ident(c)).join(', ')})`); + } + + // A dialect that cannot ALTER a constraint in must declare it here or lose + // it: for SQLite the CREATE TABLE is the only chance the FK ever gets. + const fk = this.fkSupport(); + if (!fk.alterAdd && fk.createInline) { + for (const key of table.foreignKeys ?? []) { + const cols = (key.columns ?? []).map((c) => this.ident(c)); + const parent = (key.referencedColumns ?? []).map((c) => this.ident(c)); + if (cols.length === 0 || parent.length !== cols.length) continue; + const named = key.name ? `CONSTRAINT ${this.bareName(key.name)} ` : ''; + lines.push( + ` ${named}FOREIGN KEY (${cols.join(', ')}) REFERENCES ${this.qualify(key.referencedTable, mapping)} (${parent.join(', ')})` + ); + } } return `CREATE TABLE ${this.qualify(table.name, mapping)} (\n${lines.join(',\n')}\n);`; @@ -224,10 +349,15 @@ export class SqlGeneratorModule { * index (SQL Server renders it as ALTER TABLE ADD CONSTRAINT). `idx.name` must be bare. */ private createIndexSql(idx: IndexInfo, qualifiedTable: string, dialect?: SqlDialect): string { - if (dialect?.createIndexStatement) return dialect.createIndexStatement(idx, qualifiedTable); - const uniqueStr = idx.unique ? ' UNIQUE' : ''; - const whereClause = idx.filter?.trim() ? ` WHERE ${idx.filter.trim()}` : ''; - return `CREATE${uniqueStr} INDEX ${idx.name} ON ${qualifiedTable} (${idx.columns.join(', ')})${whereClause};`; + // Quote the column names *before* handing them to a dialect hook: the hooks + // build their own column list, so a hook-owning dialect (SQLite, SQL Server) + // would otherwise emit `ON t (order id)` while the generic path below got + // this right. `ident` is idempotent, so quoting here is safe either way. + const quoted: IndexInfo = { ...idx, columns: idx.columns.map((c) => this.ident(c)) }; + if (dialect?.createIndexStatement) return dialect.createIndexStatement(quoted, qualifiedTable); + const uniqueStr = quoted.unique ? ' UNIQUE' : ''; + const whereClause = quoted.filter?.trim() ? ` WHERE ${quoted.filter.trim()}` : ''; + return `CREATE${uniqueStr} INDEX ${quoted.name} ON ${qualifiedTable} (${quoted.columns.join(', ')})${whereClause};`; } /** @@ -247,9 +377,19 @@ export class SqlGeneratorModule { if (columns.length === 0 || refCols.length !== columns.length) { return `-- review: skip FK ${label} — referenced columns missing or length mismatch`; } + // SQLite and ClickHouse have no ALTER TABLE ... ADD CONSTRAINT. The matrix + // already knew that and only the blueprint UI was reading it, so the + // migration emitted a statement the engine rejects outright. A note beats + // DDL that cannot run. + const fk = this.fkSupport(); + if (!fk.alterAdd) { + return `-- review: skip FK ${label} — ${fk.hint}`; + } + const cols = columns.map((c) => this.ident(c)).join(', '); + const parentCols = refCols.map((c) => this.ident(c)).join(', '); const fkBody = multiline - ? `FOREIGN KEY (${columns.join(', ')}) REFERENCES ${referencedTable} (${refCols.join(', ')})` - : `FOREIGN KEY (${columns.join(', ')}) REFERENCES ${referencedTable} (${refCols.join(', ')})`; + ? `FOREIGN KEY (${cols}) REFERENCES ${referencedTable} (${parentCols})` + : `FOREIGN KEY (${cols}) REFERENCES ${referencedTable} (${parentCols})`; if (multiline) { return `ALTER TABLE ${qualifiedTable} ADD CONSTRAINT ${constraintName} \n ${fkBody};`; } @@ -286,6 +426,8 @@ export class SqlGeneratorModule { generateObjectDdl(table: TableSchema, dialectStr = 'db2'): string { const dialect = resolveDialect(dialectStr); + this.quoteIdentifier = dialect.quoteIdentifier ?? ansiQuoteIdentifier; + this.dialectName = dialectStr; if (table.objectType === 'SEQUENCE') return this.renderCreateSequence(table, dialect); if (table.objectType === 'TYPE') return this.renderCreateType(table, dialect); if (table.objectType !== 'TABLE' && table.objectType !== 'MQT') { @@ -604,7 +746,11 @@ export class SqlGeneratorModule { } else { addTypeSql = translated.sql; } - let colDef = `${col.name} ${addTypeSql}`; + // Pre-quoted here rather than inside each dialect's hook: the hooks all + // interpolate the name they are given, so quoting at the one call site + // fixes ADD COLUMN on all 14 at once. `ident` is idempotent, so a hook + // that already quotes is unaffected. + let colDef = `${this.columnIdent(col)} ${addTypeSql}`; if (col.source.collation) colDef += dialect.columnCollateClause?.(col.source.collation) ?? ` COLLATE ${col.source.collation}`; if (!dialect.nullableTypeWrapper) { // Oracle requires DEFAULT before NOT NULL; the standard allows either order. @@ -627,7 +773,7 @@ export class SqlGeneratorModule { const colSpec = this.isCrossDialect(mapping) ? { ...col.source, type: translated.sql, defaultValue: undefined, collation: undefined } : { ...col.source, type: translated.sql }; - statements.push(...dialect.modifyColumnStatements(tableName, col.name, colSpec, col.target?.nullable)); + statements.push(...dialect.modifyColumnStatements(tableName, this.columnIdent(col), colSpec, col.target?.nullable)); // The compare flags a column as MODIFIED when only its DEFAULT differs, but // the type/nullability statements above don't carry the default — apply it @@ -644,14 +790,14 @@ export class SqlGeneratorModule { const seqForDefault = dialect.serialSequenceFromDefault?.(srcDef ?? ''); if (seqForDefault) statements.push(`CREATE SEQUENCE IF NOT EXISTS ${this.qualify(seqForDefault, mapping)};`); const requalifiedDef = srcDef !== undefined ? this.requalifyDefault(srcDef, mapping) : srcDef; - statements.push(...dialect.setDefaultStatements(tableName, col.name, requalifiedDef)); + statements.push(...dialect.setDefaultStatements(tableName, this.columnIdent(col), requalifiedDef)); } } } if (!mapping?.nonDestructive) { for (const col of obj.columnDiffs.filter((c) => c.status === 'REMOVED')) { - statements.push(dialect.dropColumnStatement(tableName, col.name)); + statements.push(dialect.dropColumnStatement(tableName, this.columnIdent(col))); } } @@ -957,6 +1103,8 @@ export class SqlGeneratorModule { contextDiffs?: TableDiff[] ): MigrationStep[] { const dialect = resolveDialect(dialectStr); + this.quoteIdentifier = dialect.quoteIdentifier ?? ansiQuoteIdentifier; + this.dialectName = dialectStr; // Pin the target dialect into the mapping so the render helpers can detect a // cross-dialect migration and translate column types accordingly. const m: SchemaMapping = { ...mapping, targetDialect: mapping?.targetDialect ?? dialectStr }; diff --git a/packages/sql/src/modules/type-mapping.ts b/packages/sql/src/modules/type-mapping.ts index e1257bdf..ce9df715 100644 --- a/packages/sql/src/modules/type-mapping.ts +++ b/packages/sql/src/modules/type-mapping.ts @@ -69,7 +69,14 @@ function shapeCanonical(base: CanonicalBase, tok: TypeToken, raw: string): Canon if (base === 'char' || base === 'varchar' || base === 'binary' || base === 'varbinary') { if (tok.length !== undefined) t.length = tok.length; } else if (base === 'decimal') { - if (tok.precision !== undefined) t.precision = tok.precision; + // A single-argument `NUMBER(10)` / `DECIMAL(10)` tokenizes as a *length*, + // because the tokenizer cannot know which types read one arg as a + // precision. For a decimal it is always a precision (SQL defines + // DECIMAL(p) as DECIMAL(p,0)), and ignoring it rendered a bare `NUMBER` — + // silently widening an Oracle NUMBER(10) column to full 38-digit precision + // on every migration. + const precision = tok.precision ?? tok.length; + if (precision !== undefined) t.precision = precision; if (tok.scale !== undefined) t.scale = tok.scale; } return t; diff --git a/packages/sql/src/providers/mysql/mysql.sql-dialect.ts b/packages/sql/src/providers/mysql/mysql.sql-dialect.ts index cfed184a..2eae6fd0 100644 --- a/packages/sql/src/providers/mysql/mysql.sql-dialect.ts +++ b/packages/sql/src/providers/mysql/mysql.sql-dialect.ts @@ -81,6 +81,12 @@ const mysqlDialect: SqlDialect = { return c.identity ? ` AUTO_INCREMENT` : ''; }, + // Backticks, not ANSI double quotes: MySQL only reads `"x"` as an identifier + // under ANSI_QUOTES, which is off by default. An embedded backtick doubles. + quoteIdentifier(name: string): string { + return `\`${name.replace(/`/g, '``')}\``; + }, + addColumnStatement(tableName: string, colDef: string): string { return `ALTER TABLE ${tableName} ADD ${colDef};`; }, diff --git a/packages/sql/src/providers/redshift/redshift.sql-dialect.ts b/packages/sql/src/providers/redshift/redshift.sql-dialect.ts index bdeb3754..22cc13fb 100644 --- a/packages/sql/src/providers/redshift/redshift.sql-dialect.ts +++ b/packages/sql/src/providers/redshift/redshift.sql-dialect.ts @@ -48,7 +48,9 @@ const types = makeDialectTypeFns({ double: plain('double precision'), char: sized('char'), varchar: sized('varchar'), - text: plain('varchar(max)'), + // Redshift has no VARCHAR(MAX) — that is T-SQL, and the server rejects it. + // 65535 bytes is the documented maximum width for a Redshift VARCHAR. + text: plain('varchar(65535)'), binary: plain('varbyte'), varbinary: plain('varbyte'), blob: plain('varbyte'), @@ -58,7 +60,7 @@ const types = makeDialectTypeFns({ timestamptz: plain('timestamptz'), uuid: plain('varchar(36)'), json: plain('super'), - xml: plain('varchar(max)'), + xml: plain('varchar(65535)'), }, }); diff --git a/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts b/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts index d219ba0f..f02fe8de 100644 --- a/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts +++ b/packages/sql/src/providers/sqlServer/sqlserver.sql-dialect.ts @@ -64,6 +64,12 @@ export const sqlServerSqlDialect: SqlDialect = { return c.identity ? ` IDENTITY(1,1)` : ''; }, + // Brackets are the T-SQL form and work regardless of QUOTED_IDENTIFIER, which + // ANSI double quotes do not. A closing bracket inside the name is doubled. + quoteIdentifier(name: string): string { + return `[${name.replace(/]/g, ']]')}]`; + }, + addColumnStatement(tableName: string, colDef: string): string { return `ALTER TABLE ${tableName} ADD ${colDef};`; }, From 3d8582508888aac5a2603f6a83a7da4a1d551c6b Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:54:54 -0600 Subject: [PATCH 06/14] fix(sql-editor): count every joined table, and no phantom CTEs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `referencedTableNames` promises the *physical* tables a statement touches, and the multi-table write warning is built on it. It got both directions wrong. `FROM orders JOIN customers` reported only `orders`: the optional-alias group matched `JOIN` as the alias of `orders` and moved `lastIndex` past it, so `customers` was never scanned. A write across two tables looked like a write across one — the warning under-reported, which is the fail-open direction. The opposite error too: `WITH recent AS (…) SELECT * FROM recent` counted `recent`, a name that exists only inside the query, so the warning counted objects that do not exist. CTE names are now excluded — at this caller only, since autocomplete legitimately wants them. Adds 44 adversarial CTE/subquery cases against the safety gates: data-modifying CTEs (`WITH x AS (DELETE …) SELECT 1` leads with the word WITH), nested CTEs, `EXPLAIN ANALYZE`, and write verbs hidden inside string literals and comments. They encode the rule that a misread must fail closed — calling a read a write costs one dialog; calling a write a read runs unreviewed DDL. The existing gates passed all of them unchanged. The first version of the CTE-name scan was a regex with adjacent optional whitespace groups, which eslint's security plugin correctly flagged as ReDoS-prone — reachable from the editor, where the input is whatever the user typed. Replaced with a single-pass scanner, pinned by a timing test. Co-Authored-By: Claude Opus 5 --- packages/sql/src/modules/cte-syntax.test.ts | 184 ++++++++++++++++++++ packages/sql/src/modules/sql-splitter.ts | 113 +++++++++++- 2 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 packages/sql/src/modules/cte-syntax.test.ts diff --git a/packages/sql/src/modules/cte-syntax.test.ts b/packages/sql/src/modules/cte-syntax.test.ts new file mode 100644 index 00000000..3efd3435 --- /dev/null +++ b/packages/sql/src/modules/cte-syntax.test.ts @@ -0,0 +1,184 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Adversarial CTE and subquery syntax, aimed at the safety gates. + * + * Every guard in the SQL editor — the write-confirmation dialog, the RBAC + * `editor.write` check, the "this UPDATE has no WHERE" warning — decides what a + * statement *is* by scanning its text. A data-modifying CTE begins with the + * word `WITH`, so a scanner that reads only the leading verb calls + * `WITH x AS (DELETE FROM accounts) SELECT 1` a read and waves it straight past + * the confirmation. These cases exist to keep that hole shut. + * + * The rule they encode: **a misread must fail closed.** Calling a read a write + * costs one extra confirmation dialog; calling a write a read runs unreviewed + * DDL against the user's database. + */ +import { describe, expect, it } from 'vitest'; +import { + isWriteStatement, + requiresWritePermission, + splitSqlStatements, + dmlLacksWhere, + referencedTableNames, +} from './sql-splitter.js'; + +describe('data-modifying CTEs are writes', () => { + const WRITES = [ + // Postgres' data-modifying CTE: the DELETE runs, the SELECT reads what it + // removed. Leading verb is WITH. + 'WITH gone AS (DELETE FROM accounts RETURNING *) SELECT * FROM gone', + 'WITH x AS (SELECT 1) INSERT INTO audit SELECT * FROM x', + 'WITH x AS (UPDATE t SET c = 1 RETURNING id) SELECT count(*) FROM x', + // Nested one level down — the write is in a CTE of a CTE. + 'WITH a AS (WITH b AS (UPDATE t SET c = 1 RETURNING *) SELECT * FROM b) SELECT * FROM a', + // Recursive CTE whose tail is the write. + 'WITH RECURSIVE t(n) AS (SELECT 1 UNION ALL SELECT n+1 FROM t WHERE n < 5) DELETE FROM u WHERE id IN (SELECT n FROM t)', + // Casing and whitespace are not a defence. + 'WiTh X aS (dElEtE fRoM t) sElEcT 1', + '\n\n with x as (\n delete from t\n )\n select 1', + // A comment before the statement must not hide the verb. + '-- harmless looking\nWITH x AS (DELETE FROM t) SELECT 1', + '/* block */ WITH x AS (TRUNCATE TABLE t) SELECT 1', + // EXPLAIN ANALYZE executes its inner statement on Postgres. + 'EXPLAIN ANALYZE WITH x AS (DELETE FROM t) SELECT 1', + // SELECT … INTO creates a table; wrapped in a CTE it still does. + 'WITH x AS (SELECT 1 AS n) SELECT * INTO backup FROM x', + ]; + + it.each(WRITES)('classifies as a write: %s', (sql) => { + expect(isWriteStatement(sql), sql).toBe(true); + }); + + it.each(WRITES)('requires write permission: %s', (sql) => { + expect(requiresWritePermission(sql), sql).toBe(true); + }); +}); + +describe('read-only CTEs and subqueries stay readable', () => { + // The other half of the contract. If ordinary analytical SQL demanded a + // confirmation every time, people would learn to click through the dialog — + // which is how the dialog stops protecting anything. + const READS = [ + 'WITH recent AS (SELECT * FROM orders WHERE created_at > now() - interval \'7 days\') SELECT count(*) FROM recent', + 'WITH RECURSIVE tree(id, parent) AS (SELECT id, parent FROM nodes WHERE parent IS NULL UNION ALL SELECT n.id, n.parent FROM nodes n JOIN tree ON n.parent = tree.id) SELECT * FROM tree', + 'SELECT * FROM (SELECT id FROM t WHERE x = 1) AS sub', + 'SELECT (SELECT max(id) FROM orders) AS newest, (SELECT count(*) FROM users) AS people', + 'WITH a AS (SELECT 1), b AS (SELECT 2) SELECT * FROM a CROSS JOIN b', + // A write verb inside a string literal is data, not a statement. + "SELECT 'DELETE FROM accounts' AS example", + "WITH x AS (SELECT '; DROP TABLE t; --' AS s) SELECT * FROM x", + // …and inside a comment. + 'SELECT 1 -- DELETE FROM accounts', + 'SELECT /* UPDATE t SET x=1 */ 1', + ]; + + it.each(READS)('classifies as a read: %s', (sql) => { + expect(isWriteStatement(sql), sql).toBe(false); + }); +}); + +describe('splitting statements that contain CTEs and subqueries', () => { + it('keeps a CTE and its tail together as one statement', () => { + const sql = 'WITH x AS (SELECT 1) SELECT * FROM x'; + expect(splitSqlStatements(sql).filter((s) => s.text.trim())).toHaveLength(1); + }); + + it('does not split on a semicolon inside a CTE string literal', () => { + // Splitting here would run `WITH x AS (SELECT '` as a statement of its own. + const sql = "WITH x AS (SELECT 'a;b' AS s) SELECT * FROM x"; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(1); + expect(parts[0]!.text).toContain("'a;b'"); + }); + + it('splits two CTE statements at the boundary between them', () => { + const sql = 'WITH a AS (SELECT 1) SELECT * FROM a;\nWITH b AS (SELECT 2) SELECT * FROM b;'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(2); + expect(parts[1]!.text).toContain('b'); + }); + + it('treats a semicolon inside a dollar-quoted body as ordinary text', () => { + const sql = 'CREATE FUNCTION f() RETURNS int AS $$ BEGIN DELETE FROM t; RETURN 1; END $$ LANGUAGE plpgsql'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(1); + }); + + it('classifies each statement of a mixed batch on its own merits', () => { + const sql = 'SELECT 1;\nWITH x AS (DELETE FROM t) SELECT 1;'; + const parts = splitSqlStatements(sql).filter((s) => s.text.trim()); + expect(parts).toHaveLength(2); + expect(isWriteStatement(parts[0]!.text)).toBe(false); + expect(isWriteStatement(parts[1]!.text)).toBe(true); + }); +}); + +describe('missing-WHERE warning sees through a CTE', () => { + it('flags a CTE-wrapped DELETE with no WHERE', () => { + // The warning exists to stop `DELETE FROM accounts` emptying a table by + // accident. Wrapping it in a CTE must not silence it. + expect(dmlLacksWhere('WITH x AS (SELECT 1) DELETE FROM accounts')).toBe(true); + }); + + it('does not flag one that has a WHERE', () => { + expect(dmlLacksWhere('WITH x AS (SELECT 1) DELETE FROM accounts WHERE id = 1')).toBe(false); + }); +}); + +describe('referenced tables in CTE queries', () => { + it('names the real tables a CTE query reads', () => { + const names = referencedTableNames( + 'WITH recent AS (SELECT * FROM orders) SELECT * FROM recent JOIN customers ON true' + ).map((n) => n.toLowerCase()); + expect(names).toContain('orders'); + expect(names).toContain('customers'); + }); + + it('recognises a CTE that declares a column list', () => { + const names = referencedTableNames( + 'WITH t(a, b) AS (SELECT x, y FROM source) SELECT * FROM t' + ).map((n) => n.toLowerCase()); + expect(names).toEqual(['source']); + }); + + it('recognises MATERIALIZED and NOT MATERIALIZED fences', () => { + for (const fence of ['MATERIALIZED', 'NOT MATERIALIZED']) { + const names = referencedTableNames( + `WITH t AS ${fence} (SELECT * FROM source) SELECT * FROM t` + ).map((n) => n.toLowerCase()); + expect(names, fence).toEqual(['source']); + } + }); + + it('handles several CTEs, including one whose body holds a comma', () => { + const names = referencedTableNames( + 'WITH a AS (SELECT x, y FROM one), b AS (SELECT z FROM two) SELECT * FROM a JOIN b ON true' + ).map((n) => n.toLowerCase()); + expect(names.sort()).toEqual(['one', 'two']); + }); + + it('scans pathological input in linear time', () => { + // The regex this replaced had adjacent optional whitespace groups, which + // backtrack exponentially. Input like this is reachable from the editor, so + // a slow scan is a denial of service, not a performance nit. Generous + // bound: the point is "not exponential", not a benchmark. + const evil = `WITH ${' '.repeat(50_000)}`; + const started = Date.now(); + referencedTableNames(evil); + referencedTableNames(`WITH a AS ${'('.repeat(2_000)}`); + expect(Date.now() - started).toBeLessThan(2_000); + }); + + it('does not report a CTE alias as a table', () => { + // `recent` is a name that exists only inside the query. Reporting it as a + // table makes the multi-table write warning count phantom tables, and any + // dependency scan built on this would chase an object that never existed. + const names = referencedTableNames( + 'WITH recent AS (SELECT * FROM orders) SELECT * FROM recent' + ).map((n) => n.toLowerCase()); + expect(names).not.toContain('recent'); + }); +}); diff --git a/packages/sql/src/modules/sql-splitter.ts b/packages/sql/src/modules/sql-splitter.ts index deb49d4c..6fee4061 100644 --- a/packages/sql/src/modules/sql-splitter.ts +++ b/packages/sql/src/modules/sql-splitter.ts @@ -1330,15 +1330,116 @@ export function collectMultiTableWriteWarnings( return out; } -/** Unique bare/qualified table names from {@link extractTableAliases} values. */ +/** + * Names defined by this statement's `WITH` clause. + * + * A CTE name looks exactly like a table in `FROM recent`, but it exists only + * for the length of the query. Autocomplete still wants it (you can type + * `recent.`), so this filters at the caller rather than in + * {@link extractTableAliases}. + */ +function cteNames(sql: string): Set { + const names = new Set(); + const s = stripSqlStringsAndComments(sql); + let i = 0; + + // A hand-rolled scan rather than one regex. The pattern this replaces held + // several adjacent optional `\s*` groups, which is ambiguous enough to + // backtrack exponentially — a denial of service reachable from the SQL + // editor, where the input is whatever the user typed. This walks the string + // once instead, and reads more like the grammar it is matching. + const isSpace = (c: string | undefined): boolean => c === ' ' || c === '\t' || c === '\n' || c === '\r' || c === '\f' || c === '\v'; + const skipSpace = (): void => { + while (i < s.length && isSpace(s[i])) i++; + }; + /** Case-insensitive keyword match on a fixed-length slice, then consume it. */ + const eatWord = (word: string): boolean => { + const end = i + word.length; + if (s.slice(i, end).toLowerCase() !== word) return false; + const after = s[end]; + if (after !== undefined && /[\w$]/.test(after)) return false; + i = end; + return true; + }; + /** Consume a balanced `( … )` run, respecting nesting. */ + const skipParens = (): boolean => { + if (s[i] !== '(') return false; + let depth = 0; + for (; i < s.length; i++) { + if (s[i] === '(') depth++; + else if (s[i] === ')') { + depth--; + if (depth === 0) { + i++; + return true; + } + } + } + return false; // unbalanced — stop scanning rather than guess + }; + const readIdent = (): string | null => { + const open = s[i]; + const close = open === '"' ? '"' : open === '`' ? '`' : open === '[' ? ']' : null; + if (close) { + const end = s.indexOf(close, i + 1); + if (end < 0) return null; + const raw = s.slice(i, end + 1); + i = end + 1; + return stripIdentQuotes(raw); + } + const start = i; + while (i < s.length && /[\w$]/.test(s[i]!)) i++; + return i > start ? s.slice(start, i) : null; + }; + + skipSpace(); + if (s[i] === '(') { + i++; + skipSpace(); + } + if (!eatWord('with')) return names; + skipSpace(); + eatWord('recursive'); + + for (;;) { + skipSpace(); + const name = readIdent(); + if (!name) return names; + skipSpace(); + // Optional column list: `WITH t(a, b) AS (…)`. + if (s[i] === '(' && !skipParens()) return names; + skipSpace(); + if (!eatWord('as')) return names; + skipSpace(); + // Postgres optimisation fences. + if (eatWord('not')) skipSpace(); + if (eatWord('materialized')) skipSpace(); + if (s[i] !== '(') return names; + names.add(name.toLowerCase()); + if (!skipParens()) return names; + skipSpace(); + if (s[i] !== ',') return names; + i++; + } +} + +/** + * Unique **physical** table names from {@link extractTableAliases} values. + * + * CTE names are excluded: counting `recent` in + * `WITH recent AS (SELECT * FROM orders) SELECT * FROM recent` as a table made + * the multi-table write warning count objects that do not exist. + */ export function referencedTableNames(sql: string): string[] { const map = extractTableAliases(sql); + const ctes = cteNames(sql); const seen = new Set(); const out: string[] = []; for (const table of Object.values(map)) { const key = table.toLowerCase(); if (seen.has(key)) continue; seen.add(key); + if (ctes.has(key)) continue; out.push(table); } return out; @@ -1396,7 +1497,15 @@ export function extractTableAliases(sql: string): Record { if (!aliasRaw) continue; const alias = stripIdentQuotes(aliasRaw); if (!alias) continue; - if (ALIAS_KEYWORD_BLACKLIST.has(alias.toLowerCase())) continue; + if (ALIAS_KEYWORD_BLACKLIST.has(alias.toLowerCase())) { + // The optional alias group swallowed a keyword: in `FROM orders JOIN + // customers`, `JOIN` matched as the alias of `orders`, leaving lastIndex + // past it so `customers` was never scanned at all. Rewind to where the + // keyword starts so it can open its own match. The alias always begins + // after the match start, so this still moves forward — no loop. + re.lastIndex = m.index + m[0].length - aliasRaw.length; + continue; + } out[alias.toLowerCase()] = table; } return out; From 65555b475b0c63ff779e57d715d0ba7a7d32eb90 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 20:55:05 -0600 Subject: [PATCH 07/14] fix(history): draw the graph nodes on the minimap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The minimap rendered as an empty white box over a canvas full of nodes. React Flow measures the DOM for the canvas itself, but it only writes those measurements back to the caller's node objects through `onNodesChange` — and this graph is fully controlled without one. So from the minimap's side every node reported undefined dimensions and `MiniMap` skipped all of them (`nodeHasDimensions(userNode)`), while the canvas rendered perfectly from internal state. Measured before the fix: 14 canvas nodes, 0 minimap nodes, and a minimap SVG holding nothing but its mask path. Declaring `initialWidth`/`initialHeight` satisfies the check without pinning the rendered size, so nodes still grow to fit their content — and unlike adding `onNodesChange`, it introduces no state that could re-render in a loop. Verified in the browser: 14 of 14. Separately, this file held four literal NUL bytes as composite-key separators, which made git treat it as binary and every diff of it opaque. Written as `\x00` escapes they are the same string and the file is text again. Co-Authored-By: Claude Opus 5 --- .../components/lokee-weave/buildGraph.ts | Bin 8854 -> 10173 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/apps/web/src/frontend/components/lokee-weave/buildGraph.ts b/apps/web/src/frontend/components/lokee-weave/buildGraph.ts index abfdb122a6b566cb8d45742f7f279c0b5afa953d..fad2d6a45637d8b37656bff199425485a14b5e0b 100644 GIT binary patch delta 1367 zcmZ`(&ubGw6sFZ)0^>r6SLhKiDQM7}1Cq+lUoRpeYJk=}vZEvSVjwmYGR5u}J(M z6#s^T7a?a49zA&XEdB@n1HPHfrqSx2cA59S`M&RcZ$C6Yw?BPcnN;8=Y+)pr#u%iE z5e$EgCV-S{tu&W?un9uWW1ATN>Wdhjl%4=ca1SC~QEOuf5o~ zyPl0+hR{)pUM)pRPfE?Ctf(8Lc+*sf3h@@Qdlw|?r36Bzx zh_NA9Ps%FLm?>=`<|#^dkAW`I#pvL;B^g3MtXWPURd|Ga(oaYNi6;btW&(b^eP81B zwaQ7ef*!SUs*9Agq&Wa4V~4mv2$U3KK#~U;=% z)+!T%By~@lJDu%zYroarYVLQopEuz?92TZGfD8;EK~7ZZW8)UQ0p__Fd#_UTw;w!i zZtni4y}r<1zx_vdWqP!7@$Bin(fa!I%lSLYlUmKWUb^h9GGvWSTo7_@6E(1ozQHQ& zAd4(K6e@=lnHgv-0-}+L3eZDfTkR+F(M3!S86~Sr5M-z~EZ5xnHbs+8apGZUcI01| zMF(`1R)c<0!i7q2bAI6uMH%86 p#kdex$eXKc!rvRSsBT_d>N`<9lXrCaOd;>x)zx#;uUEdE`VBfY&L;o> delta 54 zcmdn%Kh1T+YSzuG*hTpn88#mlvtZinEq#v($kvsMVFXgW^3{x+uPCwtg(g3c7uoz# H`3y4v@YoTw From dcbd70263441edc392c05c351b41130cf92dd72f Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 21:30:04 -0600 Subject: [PATCH 08/14] test(sql): run the generated DDL on the real engines, not just SQLite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SQLite oracle needs no credentials, so it runs everywhere — but it cannot tell you whether the *per-dialect* quoting is right. Backticks for MySQL, brackets for T-SQL, double quotes elsewhere: only the servers know, and getting it wrong is how a migration dies halfway through a customer's database. Runs the same hostile schemas — spaces, reserved words, punctuation, non-ASCII, an index and an FK over awkward names — against Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB from `docker compose`. 30 cases, all green, with the identifier-quoting fix in place. Gated behind FOX_IT_DB=1 so the default run and CI stay DB-free; unreachable engines skip individually so a partial stack still tells you something. Each case asserts it generated statements before executing them, so an empty plan cannot pass vacuously, and every table it creates is dropped afterwards. Verified the harness can actually fail: the unquoted form of the same statement is rejected by the live server with `syntax error at or near "Order"`. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-live.test.ts | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 apps/web/src/backend/modules/generated-ddl-live.test.ts diff --git a/apps/web/src/backend/modules/generated-ddl-live.test.ts b/apps/web/src/backend/modules/generated-ddl-live.test.ts new file mode 100644 index 00000000..a1594c57 --- /dev/null +++ b/apps/web/src/backend/modules/generated-ddl-live.test.ts @@ -0,0 +1,211 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + * + * Generated DDL, executed by the real database servers. + * + * `generated-ddl-runs.test.ts` proves the same statements parse in SQLite, + * which needs no credentials and so runs everywhere. It cannot tell you whether + * the *per-dialect* quoting is right: backticks for MySQL, brackets for T-SQL, + * double quotes elsewhere. Only the servers can, and getting that wrong is how + * a migration fails halfway through against a customer's database. + * + * Gated behind FOX_IT_DB=1 so the default `vitest run` and CI stay DB-free: + * + * docker compose up -d + * FOX_IT_DB=1 npx vitest run apps/web/src/backend/modules/generated-ddl-live.test.ts + * + * Engines that are not up are skipped individually rather than failing the run, + * so a partial stack still tells you something. Oracle and DB2 are in the + * compose file but slow to boot; add them here once they are healthy. + */ +import { afterAll, describe, expect, it } from 'vitest'; +import { ConnectionFactory } from '@foxschema/db'; +import { CompareModule, SqlGeneratorModule } from '@foxschema/sql'; +import type { ConnectionOptions, TableSchema } from '@foxschema/sql'; + +const RUN = process.env.FOX_IT_DB === '1'; +const gen = new SqlGeneratorModule(); + +/** Unique per run, so a rerun never collides with rows an earlier one left. */ +const TAG = Date.now().toString(36).slice(-5); + +const TARGETS: Array<{ dialect: string; provider: string; options: ConnectionOptions }> = [ + { + dialect: 'postgres', + provider: 'postgres', + options: { host: 'localhost', port: 5432, database: 'foxdb', username: 'foxuser', password: 'foxpass', schema: 'public' }, + }, + { + dialect: 'mysql', + provider: 'mysql', + options: { host: 'localhost', port: 3306, database: 'foxdb', username: 'foxuser', password: 'foxpass' }, + }, + { + dialect: 'mariadb', + provider: 'mariadb', + options: { host: 'localhost', port: 3307, database: 'foxdb', username: 'foxuser', password: 'foxpass' }, + }, + { + dialect: 'sqlserver', + provider: 'sqlserver', + options: { host: 'localhost', port: 1433, database: 'master', username: 'sa', password: 'FoxPass123!', ssl: { enabled: false } }, + }, + { + dialect: 'cockroachdb', + provider: 'cockroachdb', + options: { host: 'localhost', port: 26257, database: 'defaultdb', username: 'root', schema: 'public' }, + }, + { + dialect: 'yugabytedb', + provider: 'yugabytedb', + options: { host: 'localhost', port: 5433, database: 'yugabyte', username: 'yugabyte', schema: 'public' }, + }, +]; + +const table = (over: Partial & { name: string }): TableSchema => ({ + objectType: 'TABLE', + columns: [], + indices: [], + foreignKeys: [], + ...over, +}); + +/** Names that are legal in a catalog and illegal in SQL unless quoted. */ +const CASES: Array<{ label: string; tables: TableSchema[] }> = [ + { + label: 'an ordinary table (control — proves the harness runs anything at all)', + tables: [ + table({ + name: `plain_${TAG}`, + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'email', type: 'VARCHAR(255)', nullable: true, primaryKey: false }, + ], + }), + ], + }, + { + label: 'spaces in the table and column names', + tables: [ + table({ + name: `Order Details ${TAG}`, + columns: [ + { name: 'order id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'unit price', type: 'DECIMAL(10,2)', nullable: true, primaryKey: false }, + ], + primaryKey: { name: `pk order ${TAG}`, columns: ['order id'] }, + }), + ], + }, + { + label: 'reserved words as identifiers', + tables: [ + table({ + name: `order_${TAG}`, + columns: [ + { name: 'select', type: 'VARCHAR(10)', nullable: true, primaryKey: false }, + { name: 'order', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'key', type: 'INTEGER', nullable: true, primaryKey: false }, + { name: 'user', type: 'VARCHAR(20)', nullable: true, primaryKey: false }, + ], + }), + ], + }, + { + label: 'punctuation and non-ASCII letters', + tables: [ + table({ + name: `naive-tbl-${TAG}`, + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: false }, + { name: 'café', type: 'VARCHAR(50)', nullable: true, primaryKey: false }, + ], + }), + ], + }, + { + label: 'an index and a foreign key over awkward names', + tables: [ + table({ + name: `parent tbl ${TAG}`, + columns: [{ name: 'parent id', type: 'INTEGER', nullable: false, primaryKey: true }], + primaryKey: { columns: ['parent id'] }, + }), + table({ + name: `child tbl ${TAG}`, + columns: [ + { name: 'child id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'parent id', type: 'INTEGER', nullable: true, primaryKey: false }, + ], + primaryKey: { columns: ['child id'] }, + indices: [{ name: `idx child ${TAG}`, columns: ['parent id'], unique: false }], + foreignKeys: [ + { + name: `fk child ${TAG}`, + columns: ['parent id'], + referencedTable: `parent tbl ${TAG}`, + referencedColumns: ['parent id'], + }, + ], + }), + ], + }, +]; + +/** DDL for creating `tables` from nothing, as the migration flow emits it. */ +async function ddlFor(tables: TableSchema[], dialect: string): Promise { + const result = await new CompareModule().compare(tables, [], { source: dialect, target: dialect }); + return gen.generateMigrationPlan(result.tables, dialect).flatMap((step) => step.statements); +} + +const reachable = new Map(); +const toDrop: Array<{ provider: string; options: ConnectionOptions; name: string }> = []; + +afterAll(async () => { + // Children before parents, so an FK never blocks the drop. + for (const { provider, options, name } of toDrop.reverse()) { + await ConnectionFactory.executeQuery(provider, options, `DROP TABLE ${name}`).catch(() => undefined); + } +}); + +describe.runIf(RUN)('generated DDL runs on the real engines', () => { + for (const target of TARGETS) { + describe(target.dialect, () => { + it('is reachable', async () => { + try { + await ConnectionFactory.executeQuery(target.provider, target.options, 'SELECT 1'); + reachable.set(target.dialect, true); + } catch (err) { + reachable.set(target.dialect, false); + // Not a failure: a partial stack should still test what is up. + console.warn(`[skip] ${target.dialect}: ${(err as Error).message.split('\n')[0]}`); + } + }); + + for (const testCase of CASES) { + it(`creates ${testCase.label}`, async () => { + if (reachable.get(target.dialect) === false) return; + const statements = (await ddlFor(testCase.tables, target.dialect)).filter( + (s) => !s.trim().startsWith('--') + ); + expect(statements.length, 'generated no DDL to execute').toBeGreaterThan(0); + + for (const statement of statements) { + const sql = statement.replace(/;\s*$/, ''); + try { + await ConnectionFactory.executeQuery(target.provider, target.options, sql); + } catch (err) { + throw new Error( + `${target.dialect} rejected:\n${sql}\n\n${(err as Error).message.split('\n')[0]}` + ); + } + const made = sql.match(/CREATE TABLE\s+("[^"]+"|`[^`]+`|\[[^\]]+\]|\S+)/i); + if (made) toDrop.push({ provider: target.provider, options: target.options, name: made[1]! }); + } + }); + } + }); + } +}); From 309e0dfa989fe24c113361291643d6adbe0eda0a Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 21:46:31 -0600 Subject: [PATCH 09/14] test(sql): cover the ALTER path on the live engines, and run plans faithfully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a table exercises one code path; ADD/DROP/MODIFY COLUMN go through the per-dialect hooks, which is where the dialects diverge most. Adds an ALTER case over awkward names to every live engine, and a DB2 target for when that container is up. The ALTER case failed on Postgres and YugabyteDB at first, and the failure was this harness, not the product — worth writing down because it looks so much like a real bug. Postgres's dependent-view hooks stash view definitions in a `CREATE TEMP TABLE … ON COMMIT DROP` and read them back several statements later, so the plan only holds together when it runs the way MigrationModule runs it: one unpooled connection, one transaction. A connection per statement loses the temp table with the session; a transaction per statement drops it at the first commit. Both report `relation "_fs_vdep_…" does not exist`. `runPlan` now mirrors MigrationModule exactly, so what the test proves is what the product actually does. 49 cases green across Postgres, MySQL, MariaDB, SQL Server, CockroachDB and YugabyteDB. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-live.test.ts | 114 ++++++++++++++++-- 1 file changed, 101 insertions(+), 13 deletions(-) diff --git a/apps/web/src/backend/modules/generated-ddl-live.test.ts b/apps/web/src/backend/modules/generated-ddl-live.test.ts index a1594c57..888a2ba9 100644 --- a/apps/web/src/backend/modules/generated-ddl-live.test.ts +++ b/apps/web/src/backend/modules/generated-ddl-live.test.ts @@ -21,7 +21,7 @@ * compose file but slow to boot; add them here once they are healthy. */ import { afterAll, describe, expect, it } from 'vitest'; -import { ConnectionFactory } from '@foxschema/db'; +import { ConnectionFactory, getAdapter } from '@foxschema/db'; import { CompareModule, SqlGeneratorModule } from '@foxschema/sql'; import type { ConnectionOptions, TableSchema } from '@foxschema/sql'; @@ -62,6 +62,14 @@ const TARGETS: Array<{ dialect: string; provider: string; options: ConnectionOpt provider: 'yugabytedb', options: { host: 'localhost', port: 5433, database: 'yugabyte', username: 'yugabyte', schema: 'public' }, }, + { + // Slowest of the set to boot (the compose healthcheck allows two minutes + // before it even starts probing) and the only one needing a native client + // driver, so it is the most likely to be skipped on a given machine. + dialect: 'db2', + provider: 'db2', + options: { host: 'localhost', port: 50000, database: 'foxdb', username: 'db2inst1', password: 'foxpass', schema: 'DB2INST1' }, + }, ]; const table = (over: Partial & { name: string }): TableSchema => ({ @@ -160,6 +168,61 @@ async function ddlFor(tables: TableSchema[], dialect: string): Promise return gen.generateMigrationPlan(result.tables, dialect).flatMap((step) => step.statements); } +/** + * Runs `statements` the way `MigrationModule` does: **one** unpooled connection, + * **one** transaction, for the whole plan. + * + * Both halves matter, and getting either wrong makes a correct plan look + * broken. Postgres's dependent-view hooks stash the view definitions in a + * `CREATE TEMP TABLE … ON COMMIT DROP` and read them back in a later statement: + * a connection per statement loses the table with the session, and a + * transaction per statement drops it at the first commit. Both produced + * `relation "_fs_vdep_…" does not exist`, which reads exactly like a product + * bug and was purely this harness being unfaithful. + */ +async function runPlan( + target: { dialect: string; provider: string; options: ConnectionOptions }, + statements: string[], + onCreate?: (name: string) => void +): Promise { + const connection = await ConnectionFactory.create(target.provider, target.options, { pooled: false }); + const adapter = getAdapter(target.provider); + try { + await adapter.beginTransaction(connection); + try { + for (const statement of statements) { + const sql = statement.replace(/;\s*$/, ''); + if (!sql.trim() || sql.trim().startsWith('--')) continue; + try { + await adapter.query(connection, sql, []); + } catch (err) { + throw new Error( + `${target.dialect} rejected:\n${sql}\n\n${(err as Error).message.split('\n')[0]}` + ); + } + const made = sql.match(/CREATE TABLE\s+("[^"]+"|`[^`]+`|\[[^\]]+\]|\S+)/i); + if (made && onCreate) onCreate(made[1]!); + } + await adapter.commitTransaction(connection); + } catch (err) { + await adapter.rollbackTransaction(connection).catch(() => undefined); + throw err; + } + } finally { + await ConnectionFactory.close(target.provider, connection).catch(() => undefined); + } +} + +/** DDL that migrates `from` into `to` — the ALTER path, per dialect hooks. */ +async function alterDdl( + from: TableSchema[], + to: TableSchema[], + dialect: string +): Promise { + const result = await new CompareModule().compare(to, from, { source: dialect, target: dialect }); + return gen.generateMigrationPlan(result.tables, dialect).flatMap((step) => step.statements); +} + const reachable = new Map(); const toDrop: Array<{ provider: string; options: ConnectionOptions; name: string }> = []; @@ -184,6 +247,40 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { } }); + it('alters a table whose names need quoting', async () => { + if (reachable.get(target.dialect) === false) return; + // ADD / DROP / MODIFY COLUMN go through per-dialect hooks rather than + // the CREATE path, so they need their own proof on a real server — + // this is where the dialects diverge most. + const before = table({ + name: `alter tbl ${TAG}`, + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'old col', type: 'VARCHAR(10)', nullable: true, primaryKey: false }, + ], + primaryKey: { columns: ['id'] }, + }); + const after = table({ + name: `alter tbl ${TAG}`, + columns: [ + { name: 'id', type: 'INTEGER', nullable: false, primaryKey: true }, + { name: 'new col', type: 'VARCHAR(50)', nullable: true, primaryKey: false }, + ], + primaryKey: { columns: ['id'] }, + }); + + await runPlan(target, await ddlFor([before], target.dialect), (name) => + toDrop.push({ provider: target.provider, options: target.options, name }) + ); + + const alters = (await alterDdl([before], [after], target.dialect)).filter( + (s) => !s.trim().startsWith('--') + ); + expect(alters.length, 'compare reported a change but generated no ALTER').toBeGreaterThan(0); + // One session for the whole plan, exactly as MigrationModule runs it. + await runPlan(target, alters); + }); + for (const testCase of CASES) { it(`creates ${testCase.label}`, async () => { if (reachable.get(target.dialect) === false) return; @@ -192,18 +289,9 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { ); expect(statements.length, 'generated no DDL to execute').toBeGreaterThan(0); - for (const statement of statements) { - const sql = statement.replace(/;\s*$/, ''); - try { - await ConnectionFactory.executeQuery(target.provider, target.options, sql); - } catch (err) { - throw new Error( - `${target.dialect} rejected:\n${sql}\n\n${(err as Error).message.split('\n')[0]}` - ); - } - const made = sql.match(/CREATE TABLE\s+("[^"]+"|`[^`]+`|\[[^\]]+\]|\S+)/i); - if (made) toDrop.push({ provider: target.provider, options: target.options, name: made[1]! }); - } + await runPlan(target, statements, (name) => + toDrop.push({ provider: target.provider, options: target.options, name }) + ); }); } }); From a357ba52c5b1147b6e6b29c1f287a92fcb42b957 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 22:01:21 -0600 Subject: [PATCH 10/14] fix(db2): reorg the table after a column drop, or it stays unwritable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DB2 leaves a table in *reorg-pending* after `ALTER TABLE … DROP COLUMN` (and some type changes). `SELECT` still works — every INSERT/UPDATE/DELETE, and every index or key rebuild, fails with **SQL0668N reason code 7** until REORG runs. So the migration reported success and handed back a table nobody could write to, and because reads kept working it could be a long time before anyone connected the two. Adds a `postColumnChangeStatements` hook, emitted after the column changes and *before* the keys and indexes are rebuilt — those rebuilds are blocked by the same pending state. DB2 implements it as `CALL SYSPROC.ADMIN_CMD('REORG TABLE …')`; bare `REORG TABLE` is a CLP command, not SQL, and cannot be sent over a client connection. No other dialect implements the hook, and a unit test holds them to that. Two harness faults had to be fixed before this bug could even be seen, both worth recording because each produced a confident green: * The liveness probe was `SELECT 1`, which DB2 rejects (SQL0104N — it wants a FROM clause). DB2 was therefore marked unreachable and every DB2 case returned early, reported as **passing** in 0ms while touching nothing. Targets now carry their own probe, and an unreachable engine calls `ctx.skip()` so it can never again be mistaken for a pass. * The post-migration check was a `SELECT`. Reads are exactly what reorg-pending still allows, so it went green against a table the user could no longer write to. It is an INSERT/DELETE now. Verified both directions against DB2 11.5: without the REORG the live suite fails with SQL0668N on the write-back; with it, all 49 cases pass across Postgres, MySQL, MariaDB, SQL Server, CockroachDB, YugabyteDB and DB2. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-live.test.ts | 67 ++++++++++++++++--- .../sql/src/modules/sql-dialect.interface.ts | 18 +++++ .../src/modules/sql-generator.module.test.ts | 51 ++++++++++++++ .../sql/src/modules/sql-generator.module.ts | 18 +++++ .../sql/src/providers/db2/db2.sql-dialect.ts | 17 +++++ 5 files changed, 162 insertions(+), 9 deletions(-) diff --git a/apps/web/src/backend/modules/generated-ddl-live.test.ts b/apps/web/src/backend/modules/generated-ddl-live.test.ts index 888a2ba9..4e991a05 100644 --- a/apps/web/src/backend/modules/generated-ddl-live.test.ts +++ b/apps/web/src/backend/modules/generated-ddl-live.test.ts @@ -31,7 +31,20 @@ const gen = new SqlGeneratorModule(); /** Unique per run, so a rerun never collides with rows an earlier one left. */ const TAG = Date.now().toString(36).slice(-5); -const TARGETS: Array<{ dialect: string; provider: string; options: ConnectionOptions }> = [ +/** + * `probe` is the liveness query, and it is not universal: DB2 rejects a bare + * `SELECT 1` (SQL0104N — it wants a FROM clause), Oracle wants `FROM DUAL`. + * Getting this wrong is worse than it sounds. The first version probed every + * engine with `SELECT 1`, so DB2 was marked unreachable and every DB2 case + * returned early — reported as **passing**, in 0ms, while touching nothing. + * A skip that looks like a pass is the most expensive kind of green there is. + */ +const TARGETS: Array<{ + dialect: string; + provider: string; + options: ConnectionOptions; + probe?: string; +}> = [ { dialect: 'postgres', provider: 'postgres', @@ -69,6 +82,7 @@ const TARGETS: Array<{ dialect: string; provider: string; options: ConnectionOpt dialect: 'db2', provider: 'db2', options: { host: 'localhost', port: 50000, database: 'foxdb', username: 'db2inst1', password: 'foxpass', schema: 'DB2INST1' }, + probe: 'SELECT 1 FROM SYSIBM.SYSDUMMY1', }, ]; @@ -238,7 +252,11 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { describe(target.dialect, () => { it('is reachable', async () => { try { - await ConnectionFactory.executeQuery(target.provider, target.options, 'SELECT 1'); + await ConnectionFactory.executeQuery( + target.provider, + target.options, + target.probe ?? 'SELECT 1' + ); reachable.set(target.dialect, true); } catch (err) { reachable.set(target.dialect, false); @@ -247,8 +265,10 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { } }); - it('alters a table whose names need quoting', async () => { - if (reachable.get(target.dialect) === false) return; + it('alters a table whose names need quoting', async (ctx) => { + // ctx.skip() rather than `return`: a skipped engine must not report a + // green tick it never earned. + if (reachable.get(target.dialect) === false) ctx.skip(); // ADD / DROP / MODIFY COLUMN go through per-dialect hooks rather than // the CREATE path, so they need their own proof on a real server — // this is where the dialects diverge most. @@ -269,9 +289,11 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { primaryKey: { columns: ['id'] }, }); - await runPlan(target, await ddlFor([before], target.dialect), (name) => - toDrop.push({ provider: target.provider, options: target.options, name }) - ); + const created: string[] = []; + await runPlan(target, await ddlFor([before], target.dialect), (name) => { + created.push(name); + toDrop.push({ provider: target.provider, options: target.options, name }); + }); const alters = (await alterDdl([before], [after], target.dialect)).filter( (s) => !s.trim().startsWith('--') @@ -279,11 +301,38 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { expect(alters.length, 'compare reported a change but generated no ALTER').toBeGreaterThan(0); // One session for the whole plan, exactly as MigrationModule runs it. await runPlan(target, alters); + + // "The statements ran" is not the same as "the table still works". + // + // It has to be a *write*. DB2 leaves a table in reorg-pending after + // DROP COLUMN, where SELECT still succeeds and every INSERT/UPDATE/ + // DELETE fails with SQL0668N reason code 7 — so a read-back probe went + // green against a table the user could no longer write to. That is the + // failure this case exists to catch, and reading was blind to it. + const quoted = created[created.length - 1]; + expect(quoted, 'no CREATE TABLE captured to write back').toBeTruthy(); + try { + await ConnectionFactory.executeQuery( + target.provider, + target.options, + `INSERT INTO ${quoted} (id) VALUES (4242)` + ); + await ConnectionFactory.executeQuery( + target.provider, + target.options, + `DELETE FROM ${quoted} WHERE id = 4242` + ); + } catch (err) { + throw new Error( + `${target.dialect}: the table cannot be written to after the migration — ` + + `${(err as Error).message.split('\n')[0]}` + ); + } }); for (const testCase of CASES) { - it(`creates ${testCase.label}`, async () => { - if (reachable.get(target.dialect) === false) return; + it(`creates ${testCase.label}`, async (ctx) => { + if (reachable.get(target.dialect) === false) ctx.skip(); const statements = (await ddlFor(testCase.tables, target.dialect)).filter( (s) => !s.trim().startsWith('--') ); diff --git a/packages/sql/src/modules/sql-dialect.interface.ts b/packages/sql/src/modules/sql-dialect.interface.ts index 9c382c58..342473a5 100644 --- a/packages/sql/src/modules/sql-dialect.interface.ts +++ b/packages/sql/src/modules/sql-dialect.interface.ts @@ -131,6 +131,24 @@ export interface SqlDialect { */ quoteIdentifier?(name: string): string; + /** + * Statements to run after a table's columns change, before its indexes, + * keys and triggers are rebuilt. + * + * DB2 puts a table into *reorg-pending* after DROP COLUMN and after some type + * changes: `SELECT` still works, but every write fails with SQL0668N reason + * code 7 until `REORG TABLE` runs. A migration that skips it reports success + * and hands back a table nobody can write to — and because reads still work, + * it can be a while before anyone notices. + * + * Omit unless the engine needs it. Runs inside the migration's transaction + * alongside the ALTERs. + */ + postColumnChangeStatements?( + qualifiedTable: string, + changed: { dropped: boolean; retyped: boolean } + ): string[]; + /** * Full ` COLLATE ...` clause (with leading space) for a column's collation, used in * CREATE TABLE / ADD COLUMN. Default: ` COLLATE `, unquoted — correct for diff --git a/packages/sql/src/modules/sql-generator.module.test.ts b/packages/sql/src/modules/sql-generator.module.test.ts index d3be2dc0..71295557 100644 --- a/packages/sql/src/modules/sql-generator.module.test.ts +++ b/packages/sql/src/modules/sql-generator.module.test.ts @@ -94,6 +94,57 @@ describe('SqlGeneratorModule.generateMigrationPlan', () => { sourceTable: tableSchema({ name, columns: [{ name: 'ID', type: 'INTEGER', nullable: false, primaryKey: false }] }), }); + describe('DB2 reorg-pending after column changes', () => { + // Verified against DB2 11.5: after ALTER TABLE … DROP COLUMN, a SELECT + // still succeeds while every INSERT fails with SQL0668N reason code 7. A + // migration without the REORG therefore reports success and hands back a + // table nobody can write to — and because reads work, it can be a long + // while before anyone notices. + const droppedColumn: TableDiff = { + tableName: 'CUSTOMER', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { name: 'OLD_COL', status: 'REMOVED', target: { name: 'old_col', type: 'VARCHAR(10)', nullable: true } }, + ], + indexDiffs: [], + foreignKeyDiffs: [], + sourceTable: tableSchema({ name: 'customer' }), + targetTable: tableSchema({ name: 'customer' }), + }; + + it('reorgs the table after a DROP COLUMN', () => { + const sql = gen.generateMigrationPlan([droppedColumn], 'db2').flatMap((s) => s.statements); + const drop = sql.findIndex((s) => /DROP COLUMN/i.test(s)); + const reorg = sql.findIndex((s) => /REORG TABLE/i.test(s)); + expect(drop, 'no DROP COLUMN emitted').toBeGreaterThanOrEqual(0); + expect(reorg, 'no REORG emitted — the table stays unwritable').toBeGreaterThan(drop); + // ADMIN_CMD is the callable form; bare `REORG TABLE` is a CLP command + // and cannot be sent over a client connection. + expect(sql[reorg]).toContain('SYSPROC.ADMIN_CMD'); + }); + + it('does not reorg when nothing was dropped or retyped', () => { + const added: TableDiff = { + ...droppedColumn, + columnDiffs: [ + { name: 'NEW_COL', status: 'ADDED', source: { name: 'new_col', type: 'VARCHAR(10)', nullable: true } }, + ], + }; + const sql = gen.generateMigrationPlan([added], 'db2').flatMap((s) => s.statements).join('\n'); + expect(sql).toMatch(/ADD/i); + expect(sql).not.toMatch(/REORG/i); + }); + + it('leaves other dialects alone', () => { + // Only DB2 implements the hook; nobody else should grow a REORG. + for (const dialect of ['postgres', 'mysql', 'sqlserver', 'oracle', 'sqlite']) { + const sql = gen.generateMigrationPlan([droppedColumn], dialect).flatMap((s) => s.statements).join('\n'); + expect(sql, dialect).not.toMatch(/REORG/i); + } + }); + }); + it('orders steps drop → create → alter', () => { const diffs: TableDiff[] = [ { ...addedTable('NEW') }, diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index ca214d89..d0e3f007 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -760,8 +760,12 @@ export class SqlGeneratorModule { statements.push(dialect.addColumnStatement(tableName, colDef)); } + // A type change can also leave a table needing maintenance (see + // `postColumnChangeStatements`), so it is tracked alongside the drops. + let retypedColumns = false; for (const col of obj.columnDiffs.filter((c) => c.status === 'MODIFIED')) { if (!col.source) continue; + if (col.target && col.source.type !== col.target.type) retypedColumns = true; const translated = this.translateType(col.source.type, mapping); if (translated.warning) statements.push(`-- review: ${col.name}: ${translated.warning}`); // Cross-dialect: the raw default AND collation are source-dialect vocabulary @@ -795,12 +799,26 @@ export class SqlGeneratorModule { } } + const droppedColumns = + !mapping?.nonDestructive && obj.columnDiffs.some((c) => c.status === 'REMOVED'); if (!mapping?.nonDestructive) { for (const col of obj.columnDiffs.filter((c) => c.status === 'REMOVED')) { statements.push(dialect.dropColumnStatement(tableName, this.columnIdent(col))); } } + // Before the keys and indexes are rebuilt, not after: an engine that + // needs maintenance here (DB2's REORG) rejects those rebuilds too while + // the table is pending. + if (droppedColumns || retypedColumns) { + statements.push( + ...(dialect.postColumnChangeStatements?.(tableName, { + dropped: droppedColumns, + retyped: retypedColumns, + }) ?? []) + ); + } + const srcPk = obj.sourceTable ? this.primaryKeyColumns(obj.sourceTable) : []; const tgtPk = obj.targetTable ? this.primaryKeyColumns(obj.targetTable) : []; if (!this.sameColumnSet(srcPk, tgtPk)) { diff --git a/packages/sql/src/providers/db2/db2.sql-dialect.ts b/packages/sql/src/providers/db2/db2.sql-dialect.ts index 0ec70d39..d5c163a9 100644 --- a/packages/sql/src/providers/db2/db2.sql-dialect.ts +++ b/packages/sql/src/providers/db2/db2.sql-dialect.ts @@ -93,6 +93,23 @@ export const db2SqlDialect: SqlDialect = { return `ALTER TABLE ${tableName} DROP COLUMN ${colName};`; }, + /** + * DROP COLUMN (and some type changes) leave the table in *reorg-pending*. + * `SELECT` still works, so nothing looks wrong — but every INSERT/UPDATE/ + * DELETE fails with SQL0668N reason code 7, and so does rebuilding the + * table's indexes and keys, until REORG runs. + * + * Verified against DB2 11.5: after `ALTER TABLE … DROP COLUMN`, a SELECT + * succeeded and an INSERT returned SQL0668N. Without this the migration + * reports success and hands back a table nobody can write to. + * + * ADMIN_CMD is the callable form — plain `REORG TABLE` is a CLP command, not + * SQL, and cannot be sent over a client connection. + */ + postColumnChangeStatements(qualifiedTable: string): string[] { + return [`CALL SYSPROC.ADMIN_CMD('REORG TABLE ${qualifiedTable.replace(/'/g, "''")}');`]; + }, + setDefaultStatements(tableName: string, colName: string, defaultValue: string | undefined): string[] { return defaultValue ? [`ALTER TABLE ${tableName} ALTER COLUMN ${colName} SET DEFAULT ${defaultValue};`] From 37d2678ed4e921954de5501b2e77436aa06c1e9e Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 22:17:45 -0600 Subject: [PATCH 11/14] =?UTF-8?q?fix(db2):=20converge=20on=20the=20real=20?= =?UTF-8?q?samples=20=E2=80=94=20phantom=20roles,=20dead=20Sessions,=20dri?= =?UTF-8?q?fting=20defaults?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ran the shipped DEMO_A/DEMO_B samples through the actual app path against DB2 11.5 — provider introspection, CompareModule, generated plan applied to a throwaway schema, then re-introspected and re-compared. Three bugs, each of which a string test could not have seen. **Every DB2 comparison carried six phantom ROLE objects.** `SYSCAT.ROLES` was read unfiltered, so DB2's own built-in roles (SYSDEBUG, SYSGEOADM, SYSTS_*) were reported as user objects in every schema. They cannot be recreated — DB2 reserves the SYS prefix and answers `CREATE ROLE SYSDEBUG` with SQL0707N — so the migration also contained eleven statements that can never succeed. In this database *every* role is a system role, so the whole ROLE section was noise. **Sessions never worked on DB2 at all.** The query selected `SESSION_DB_PARTITION_NUM`, which `MON_GET_CONNECTION` does not expose: DB2 answered SQL0206N and the utility failed outright. It is `CURRENT SERVER` now, which is what the `database_name` column claims to be — a partition number was the wrong value for that slot regardless. **Migrations to DB2 never converged.** DB2 rejects adding a NOT NULL column to a populated table without a default (SQL0193N), so the dialect appends `WITH DEFAULT` — correct, and documented. But that leaves the column holding a default (`''`, `0`) the source never declared, so re-comparing straight after a *successful* migration still reported the column as changed and proposed the same work again, for ever. A new `afterAddColumnStatements` hook drops the implicit default once the rows are backfilled; verified on the server that DROP DEFAULT is accepted immediately after the ADD, needs no REORG between, and leaves the catalog default NULL — matching the source exactly. End state on the samples: 30/30 statements execute, and re-comparing after the migration reports **no differences at all**. All five utilities (pool, sessions, system, sizes, index-fragmentation) run against the live server, and DEMO_A is byte-for-byte untouched by the run. Co-Authored-By: Claude Opus 5 --- packages/db/src/providers/db2/db2.provider.ts | 12 +++- .../src/modules/dialect-dba-utilities.test.ts | 14 +++++ .../sql/src/modules/dialect-dba-utilities.ts | 6 +- .../sql/src/modules/sql-dialect.interface.ts | 14 +++++ .../src/modules/sql-generator.module.test.ts | 57 +++++++++++++++++++ .../sql/src/modules/sql-generator.module.ts | 5 ++ .../sql/src/providers/db2/db2.sql-dialect.ts | 16 ++++++ 7 files changed, 121 insertions(+), 3 deletions(-) diff --git a/packages/db/src/providers/db2/db2.provider.ts b/packages/db/src/providers/db2/db2.provider.ts index c17ea0a4..225da288 100644 --- a/packages/db/src/providers/db2/db2.provider.ts +++ b/packages/db/src/providers/db2/db2.provider.ts @@ -414,14 +414,22 @@ export class Db2Provider implements SchemaProvider { [schemaName] ), // Roles are database-global (not schema-scoped). + // + // `SYS%` is excluded because those are DB2's own built-in roles + // (SYSDEBUG, SYSTS_ADM, SYSGEOADM, …). They exist in every database, + // belong to nobody, and cannot be recreated — DB2 reserves the SYS + // prefix and answers `CREATE ROLE SYSDEBUG` with SQL0707N. Reporting + // them put six phantom objects in every DB2 comparison and put eleven + // statements that can never succeed into the migration. ConnectionFactory.executeOnConnection( this.provider, conn, - `SELECT ROLENAME FROM SYSCAT.ROLES ORDER BY ROLENAME`, + `SELECT ROLENAME FROM SYSCAT.ROLES WHERE ROLENAME NOT LIKE 'SYS%' ORDER BY ROLENAME`, [] ), ConnectionFactory.executeOnConnection( this.provider, conn, - `SELECT ROLENAME, GRANTEE, GRANTEETYPE FROM SYSCAT.ROLEAUTH ORDER BY ROLENAME, GRANTEE`, + `SELECT ROLENAME, GRANTEE, GRANTEETYPE FROM SYSCAT.ROLEAUTH + WHERE ROLENAME NOT LIKE 'SYS%' ORDER BY ROLENAME, GRANTEE`, [] ) ]); diff --git a/packages/sql/src/modules/dialect-dba-utilities.test.ts b/packages/sql/src/modules/dialect-dba-utilities.test.ts index 86fe62e5..5eb4c214 100644 --- a/packages/sql/src/modules/dialect-dba-utilities.test.ts +++ b/packages/sql/src/modules/dialect-dba-utilities.test.ts @@ -112,3 +112,17 @@ describe('dialect-dba-utilities', () => { expect(q.params).toEqual(['CARTER', 'CARTER']); }); }); + +describe('DB2 session query columns', () => { + it('does not select SESSION_DB_PARTITION_NUM', () => { + // MON_GET_CONNECTION has no such column. DB2 answers SQL0206N, so the + // Sessions utility failed for every DB2 user until this was corrected. + const built = buildDbaUtilityQuery({ dialect: 'db2', kind: 'sessions' }); + expect('error' in built).toBe(false); + const sql = (built as { sql: string }).sql; + expect(sql).not.toContain('SESSION_DB_PARTITION_NUM'); + // Verified live on DB2 11.5: this returns the database, which is what the + // column claims to be — a partition number was wrong for it regardless. + expect(sql).toContain('CURRENT SERVER AS database_name'); + }); +}); diff --git a/packages/sql/src/modules/dialect-dba-utilities.ts b/packages/sql/src/modules/dialect-dba-utilities.ts index f72a516c..45dbc63e 100644 --- a/packages/sql/src/modules/dialect-dba-utilities.ts +++ b/packages/sql/src/modules/dialect-dba-utilities.ts @@ -448,6 +448,10 @@ SELECT * FROM ( }; } if (f === 'db2') { + // `database_name` is CURRENT SERVER, not a partition column. The original + // query selected a partition number here under a column name that promised + // the database — and picked one MON_GET_CONNECTION does not expose, so DB2 + // answered SQL0206N and Sessions failed outright for every DB2 user. return { mode: asMode(mode), params: [], @@ -456,7 +460,7 @@ SELECT CAST(APPLICATION_HANDLE AS VARCHAR(32)) AS session_id, SESSION_AUTH_ID AS user_name, CLIENT_HOSTNAME AS client_host, - SESSION_DB_PARTITION_NUM AS database_name, + CURRENT SERVER AS database_name, APPLICATION_NAME AS state, CAST(NULL AS VARCHAR(128)) AS wait_event, CAST(NULL AS VARCHAR(500)) AS query_text, diff --git a/packages/sql/src/modules/sql-dialect.interface.ts b/packages/sql/src/modules/sql-dialect.interface.ts index 342473a5..8a865ff3 100644 --- a/packages/sql/src/modules/sql-dialect.interface.ts +++ b/packages/sql/src/modules/sql-dialect.interface.ts @@ -144,6 +144,20 @@ export interface SqlDialect { * Omit unless the engine needs it. Runs inside the migration's transaction * alongside the ALTERs. */ + /** + * Statements that clean up after this dialect's own `addColumnStatement`. + * + * DB2 cannot add a NOT NULL column to a populated table without a default + * (SQL0193N), so the dialect appends `WITH DEFAULT` to make the ALTER + * succeed. That leaves the column carrying a default (`''`, `0`) the source + * column never had — so the very next comparison reports the column as + * changed, proposes the same migration again, and never converges. Dropping + * the implicit default afterwards makes the result actually match the source. + * + * Omit unless `addColumnStatement` adds something the source did not ask for. + */ + afterAddColumnStatements?(qualifiedTable: string, colName: string, col: ColumnSpec): string[]; + postColumnChangeStatements?( qualifiedTable: string, changed: { dropped: boolean; retyped: boolean } diff --git a/packages/sql/src/modules/sql-generator.module.test.ts b/packages/sql/src/modules/sql-generator.module.test.ts index 71295557..f0144db4 100644 --- a/packages/sql/src/modules/sql-generator.module.test.ts +++ b/packages/sql/src/modules/sql-generator.module.test.ts @@ -94,6 +94,63 @@ describe('SqlGeneratorModule.generateMigrationPlan', () => { sourceTable: tableSchema({ name, columns: [{ name: 'ID', type: 'INTEGER', nullable: false, primaryKey: false }] }), }); + describe('DB2 converges after adding a NOT NULL column', () => { + // DB2 cannot add a NOT NULL column to a populated table without a default + // (SQL0193N), so the dialect appends `WITH DEFAULT`. That leaves the column + // holding a default the source never declared, and the next comparison + // reports the same change again — for ever. Verified end to end against the + // shipped DEMO_A/DEMO_B samples on DB2 11.5: with this, re-comparing after + // the migration reports no differences at all. + const addNotNull = (over: Partial<{ nullable: boolean; defaultValue: string }> = {}): TableDiff => ({ + tableName: 'PRODUCTS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { + name: 'SKU', + status: 'ADDED', + source: { name: 'SKU', type: 'VARCHAR(50)', nullable: false, ...over }, + }, + ], + indexDiffs: [], + foreignKeyDiffs: [], + sourceTable: tableSchema({ name: 'PRODUCTS' }), + targetTable: tableSchema({ name: 'PRODUCTS' }), + }); + + it('drops the implicit default it had to add', () => { + const sql = gen.generateMigrationPlan([addNotNull()], 'db2').flatMap((s) => s.statements); + const add = sql.findIndex((s) => /ADD\b/i.test(s) && /WITH DEFAULT/i.test(s)); + const drop = sql.findIndex((s) => /DROP DEFAULT/i.test(s)); + expect(add, 'no ADD … WITH DEFAULT emitted').toBeGreaterThanOrEqual(0); + expect(drop, 'the implicit default is never dropped — this never converges').toBeGreaterThan(add); + }); + + it('keeps a default the source actually declares', () => { + const sql = gen + .generateMigrationPlan([addNotNull({ defaultValue: "'x'" })], 'db2') + .flatMap((s) => s.statements) + .join('\n'); + expect(sql).toContain("DEFAULT 'x'"); + expect(sql, 'dropped a default the source asked for').not.toMatch(/DROP DEFAULT/i); + }); + + it('leaves a nullable column alone', () => { + const sql = gen + .generateMigrationPlan([addNotNull({ nullable: true })], 'db2') + .flatMap((s) => s.statements) + .join('\n'); + expect(sql).not.toMatch(/DROP DEFAULT/i); + }); + + it('does not touch other dialects', () => { + for (const dialect of ['postgres', 'mysql', 'sqlserver', 'oracle']) { + const sql = gen.generateMigrationPlan([addNotNull()], dialect).flatMap((s) => s.statements).join('\n'); + expect(sql, dialect).not.toMatch(/DROP DEFAULT/i); + } + }); + }); + describe('DB2 reorg-pending after column changes', () => { // Verified against DB2 11.5: after ALTER TABLE … DROP COLUMN, a SELECT // still succeeds while every INSERT fails with SQL0668N reason code 7. A diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index d0e3f007..2bd017fe 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -758,6 +758,11 @@ export class SqlGeneratorModule { if (!col.source.nullable) colDef += ` NOT NULL`; } statements.push(dialect.addColumnStatement(tableName, colDef)); + // Let the dialect clean up after its own workaround (DB2's implicit + // `WITH DEFAULT`), or the migration never converges. + statements.push( + ...(dialect.afterAddColumnStatements?.(tableName, this.columnIdent(col), col.source) ?? []) + ); } // A type change can also leave a table needing maintenance (see diff --git a/packages/sql/src/providers/db2/db2.sql-dialect.ts b/packages/sql/src/providers/db2/db2.sql-dialect.ts index d5c163a9..84b089a8 100644 --- a/packages/sql/src/providers/db2/db2.sql-dialect.ts +++ b/packages/sql/src/providers/db2/db2.sql-dialect.ts @@ -80,6 +80,22 @@ export const db2SqlDialect: SqlDialect = { return `ALTER TABLE ${tableName} ADD ${def};`; }, + /** + * Undo the `WITH DEFAULT` above once the rows are backfilled. + * + * Without this the new column keeps a default (`''`, `0`) the source column + * never declared, so re-comparing straight after a *successful* migration + * still reports the column as changed — the tool proposes the same migration + * for ever and never converges. Verified on DB2 11.5: DROP DEFAULT is + * accepted immediately after the ADD, needs no REORG in between, and leaves + * the catalog default NULL, matching the source exactly. + */ + afterAddColumnStatements(tableName: string, colName: string, col: ColumnSpec): string[] { + const sourceHadDefault = col.defaultValue !== undefined && col.defaultValue !== null; + if (col.nullable || sourceHadDefault) return []; + return [`ALTER TABLE ${tableName} ALTER COLUMN ${colName} DROP DEFAULT;`]; + }, + modifyColumnStatements(tableName: string, colName: string, col: ColumnSpec): string[] { const stmts = [`ALTER TABLE ${tableName} ALTER COLUMN ${colName} SET DATA TYPE ${col.type};`]; // DB2 nullability is a separate clause — SET DATA TYPE does not carry it. From 36a82d73277c2f7806ebcd9e5aab11d6dc3f1419 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 22:25:12 -0600 Subject: [PATCH 12/14] fix(db2): a revert that hits an already-dropped FK no longer rolls itself back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drove the whole product loop against real DB2 for the first time: seed a scratch schema with MigrationModule, capture v1 with Lokee, migrate DEMO_A into it, capture v2, diff the versions, plan a revert, execute it, and check the database actually went back. It did not. The revert reported success and changed nothing. Table drops are ordered before the ALTERs, and dropping a parent table takes its inbound foreign keys with it — so `ALTER TABLE … DROP FOREIGN KEY` ran against a constraint DB2 had already removed and raised SQL0204N. DB2 has transactional DDL, so that one statement rolled the *entire* revert back. Everything else in the plan was correct; none of it survived. The dialect already knows this shape: its DROP TABLE/VIEW go through a SQL PL `CONTINUE HANDLER FOR SQLSTATE '42704'` because DB2 has no DROP IF EXISTS. The FK drop simply never got the same treatment, even though the generic fallback it overrides says `DROP CONSTRAINT IF EXISTS` for exactly this reason. It is wrapped now. Verified end to end on DB2 11.5: after the revert the live schema matches v1 exactly, and the new version's root hash equals v1's — content-addressed proof that what came back is identical, not merely similar. One note for whoever writes the next harness: `MigrationModule.execute` reports failure through its **event stream** (`{type:'done', success:false, rolledBack:true}`), not by throwing. My first pass wrapped it in try/catch, saw no exception, and cheerfully reported a rolled-back revert as applied. Co-Authored-By: Claude Opus 5 --- .../src/modules/sql-generator.module.test.ts | 20 +++++++++++++++++++ .../sql/src/providers/db2/db2.sql-dialect.ts | 16 ++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/sql/src/modules/sql-generator.module.test.ts b/packages/sql/src/modules/sql-generator.module.test.ts index f0144db4..a2f6e6c3 100644 --- a/packages/sql/src/modules/sql-generator.module.test.ts +++ b/packages/sql/src/modules/sql-generator.module.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import { SqlGeneratorModule } from './sql-generator.module.js'; +import { db2SqlDialect } from '../providers/db2/db2.sql-dialect.js'; import { TableDiff } from '../interfaces/index.js'; import { TableSchema } from '../interfaces/index.js'; @@ -94,6 +95,25 @@ describe('SqlGeneratorModule.generateMigrationPlan', () => { sourceTable: tableSchema({ name, columns: [{ name: 'ID', type: 'INTEGER', nullable: false, primaryKey: false }] }), }); + describe('DB2 tolerates a foreign key that is already gone', () => { + it('wraps the FK drop in the same 42704 handler as its other drops', () => { + // Dropping a parent table takes its inbound foreign keys with it, and + // table drops are ordered before the ALTERs — so by the time the explicit + // FK drop runs, the constraint often no longer exists. DB2 has no DROP + // CONSTRAINT IF EXISTS, and DB2 has transactional DDL, so the resulting + // SQL0204N rolled the *entire* migration back: on the live samples the + // revert reported no error and changed nothing at all. + const sql = db2SqlDialect.dropForeignKeyStatement!('S.CHILD', 'FK_CHILD_PARENT'); + expect(sql).toContain("SQLSTATE '42704'"); + expect(sql).toContain('DROP FOREIGN KEY FK_CHILD_PARENT'); + }); + + it('still drops the constraint it was asked to drop', () => { + const sql = db2SqlDialect.dropForeignKeyStatement!('S.CHILD', 'FK_X'); + expect(sql).toMatch(/ALTER TABLE S\.CHILD DROP FOREIGN KEY FK_X/); + }); + }); + describe('DB2 converges after adding a NOT NULL column', () => { // DB2 cannot add a NOT NULL column to a populated table without a default // (SQL0193N), so the dialect appends `WITH DEFAULT`. That leaves the column diff --git a/packages/sql/src/providers/db2/db2.sql-dialect.ts b/packages/sql/src/providers/db2/db2.sql-dialect.ts index 84b089a8..2fc262e1 100644 --- a/packages/sql/src/providers/db2/db2.sql-dialect.ts +++ b/packages/sql/src/providers/db2/db2.sql-dialect.ts @@ -64,6 +64,12 @@ function db2Drop(keyword: string, name: string): string { return `BEGIN\n DECLARE CONTINUE HANDLER FOR SQLSTATE '42704' BEGIN END;\n EXECUTE IMMEDIATE 'DROP ${keyword} ${safe}';\nEND`; } +/** Same tolerance for a constraint, which lives on ALTER TABLE rather than DROP. */ +function db2DropConstraint(tableName: string, fkName: string): string { + const safe = `ALTER TABLE ${tableName} DROP FOREIGN KEY ${fkName}`.replace(/'/g, "''"); + return `BEGIN\n DECLARE CONTINUE HANDLER FOR SQLSTATE '42704' BEGIN END;\n EXECUTE IMMEDIATE '${safe}';\nEND`; +} + export const db2SqlDialect: SqlDialect = { identityClause(c: ColumnSpec): string { return c.identity ? ` GENERATED ${c.identityGeneration ?? 'ALWAYS'} AS IDENTITY` : ''; @@ -138,7 +144,15 @@ export const db2SqlDialect: SqlDialect = { dropForeignKeyStatement(tableName: string, fkName: string): string { // DB2 has no DROP CONSTRAINT IF EXISTS; DROP FOREIGN KEY is the native form. - return `ALTER TABLE ${tableName} DROP FOREIGN KEY ${fkName};`; + // + // Wrapped in the same 42704 handler as the other drops, because the + // constraint may already be gone by the time this runs: dropping a parent + // table earlier in the plan takes its inbound foreign keys with it. Without + // the handler that raised SQL0204N, and since DB2 has transactional DDL the + // *whole* migration rolled back — a revert that reported no error and + // changed nothing. The generic fallback says `DROP CONSTRAINT IF EXISTS` + // for exactly this reason; this restores that tolerance for DB2. + return `${db2DropConstraint(tableName, fkName)};`; }, dropIndexStatement(indexName: string, qualifiedTable: string): string { From 70ce5087b06bed78a479b0e0d48744aaf4f42d5e Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 22:36:28 -0600 Subject: [PATCH 13/14] test(sql): round-trip procedures and functions on the live engines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routine bodies are the least portable thing in SQL and nothing here exercised them: the DB2 samples contain no routines at all (its seed's compound blocks are only drop-cleanup), so DEMO_A/DEMO_B never tested a single one. Adds the round trip, which is the only version of this test worth running: create a function and a procedure with native DDL, read them back through the provider, ask the generator to recreate them in a *second* schema, and execute that. Asserting "the object exists" would pass on a definition that is empty, missing its terminator, or has the source schema baked into it; executing the regenerated DDL somewhere else catches all three. Covers Postgres, MySQL, SQL Server and DB2 — each with its own body, since there is no portable one. Engines without a spec skip visibly rather than reporting a green they did not earn. All four pass: the captured definition is faithful enough to rebuild the routine elsewhere. MySQL needs the second *database* (a MySQL schema is a database), which the demo user has no rights to create, so that one target carries admin credentials — an environment limit, not a product one. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-live.test.ts | 136 +++++++++++++++++- 1 file changed, 135 insertions(+), 1 deletion(-) diff --git a/apps/web/src/backend/modules/generated-ddl-live.test.ts b/apps/web/src/backend/modules/generated-ddl-live.test.ts index 4e991a05..11dfaa4e 100644 --- a/apps/web/src/backend/modules/generated-ddl-live.test.ts +++ b/apps/web/src/backend/modules/generated-ddl-live.test.ts @@ -21,7 +21,7 @@ * compose file but slow to boot; add them here once they are healthy. */ import { afterAll, describe, expect, it } from 'vitest'; -import { ConnectionFactory, getAdapter } from '@foxschema/db'; +import { ConnectionFactory, getAdapter, getRegisteredProvider } from '@foxschema/db'; import { CompareModule, SqlGeneratorModule } from '@foxschema/sql'; import type { ConnectionOptions, TableSchema } from '@foxschema/sql'; @@ -247,6 +247,65 @@ afterAll(async () => { } }); +/** + * Native DDL for one function and one procedure, plus where to recreate them. + * + * Routine bodies are the least portable thing in SQL, so each engine gets its + * own. Engines absent here have no routine coverage rather than a pretend one. + */ +const ROUTINES: Record< + string, + { + from: string; + to: string; + makeSchema?: (s: string) => string[]; + ddl: (s: string) => string[]; + /** Credentials with rights the demo user lacks (creating a database). */ + admin?: Partial; + } +> = { + postgres: { + from: `fx_a_${TAG}`, + to: `fx_b_${TAG}`, + makeSchema: (s) => [`CREATE SCHEMA IF NOT EXISTS ${s}`], + ddl: (s) => [ + `CREATE FUNCTION ${s}.double_it(x integer) RETURNS integer AS $$ SELECT x * 2 $$ LANGUAGE sql`, + `CREATE PROCEDURE ${s}.touch_it() LANGUAGE plpgsql AS $$ BEGIN PERFORM 1; END; $$`, + ], + }, + mysql: { + // A MySQL schema *is* a database, so the round trip needs a second one. + from: 'foxdb', + to: `foxb_${TAG}`, + // The demo user cannot create a database ("Access denied for user + // 'foxuser'"), and this round trip needs a second one to recreate into. + admin: { username: 'root', password: 'foxrootpass' }, + makeSchema: (s) => [`CREATE DATABASE IF NOT EXISTS ${s}`], + ddl: () => [ + `CREATE FUNCTION double_it_${TAG}(x INT) RETURNS INT DETERMINISTIC RETURN x * 2`, + `CREATE PROCEDURE touch_it_${TAG}() BEGIN SELECT 1; END`, + ], + }, + sqlserver: { + from: 'dbo', + to: `foxb_${TAG}`, + makeSchema: (s) => (s === 'dbo' ? [] : [`IF SCHEMA_ID('${s}') IS NULL EXEC('CREATE SCHEMA ${s}')`]), + ddl: () => [ + `CREATE FUNCTION dbo.double_it_${TAG}(@x INT) RETURNS INT AS BEGIN RETURN @x * 2 END`, + `CREATE PROCEDURE dbo.touch_it_${TAG} AS BEGIN SELECT 1 END`, + ], + }, + db2: { + from: `FXA${TAG}`, + to: `FXB${TAG}`, + makeSchema: (s) => [`CREATE SCHEMA ${s}`], + ddl: (s) => [ + `CREATE FUNCTION ${s}.DOUBLE_IT(X INTEGER) RETURNS INTEGER LANGUAGE SQL RETURN X * 2`, + `CREATE PROCEDURE ${s}.TOUCH_IT() LANGUAGE SQL BEGIN DECLARE V INT; SET V = 1; END`, + ], + }, +}; + describe.runIf(RUN)('generated DDL runs on the real engines', () => { for (const target of TARGETS) { describe(target.dialect, () => { @@ -265,6 +324,81 @@ describe.runIf(RUN)('generated DDL runs on the real engines', () => { } }); + const routines = ROUTINES[target.dialect]; + it.runIf(routines)('captures a procedure and a function well enough to recreate them', async (ctx) => { + if (reachable.get(target.dialect) === false) ctx.skip(); + const spec = routines!; + const opts = (schema: string) => ({ + ...target.options, + ...spec.admin, + ...(target.dialect === 'mysql' ? { database: schema } : {}), + schema, + }); + + const admin = await ConnectionFactory.create(target.provider, opts(spec.from), { pooled: false }); + const adapter = getAdapter(target.provider); + const exec = (conn: unknown, sql: string) => adapter.query(conn as never, sql, []); + try { + for (const schema of [spec.from, spec.to]) + for (const stmt of spec.makeSchema?.(schema) ?? []) await exec(admin, stmt).catch(() => undefined); + for (const stmt of spec.ddl(spec.from)) await exec(admin, stmt); + + // Read them back the way the app does. + const provider = getRegisteredProvider(target.provider)!; + const objects = (await provider.getTables!(opts(spec.from), spec.from)) as TableSchema[]; + const mine = objects.filter( + (o) => + (o.objectType === 'FUNCTION' || o.objectType === 'PROCEDURE') && + new RegExp(`(double_it|touch_it)(_${TAG})?$`, 'i').test(o.name) + ); + expect(mine.map((r) => r.objectType).sort()).toEqual(['FUNCTION', 'PROCEDURE']); + + // A routine with no body cannot be migrated anywhere, and an empty + // string would still pass a "the object exists" assertion. + for (const r of mine) { + expect((r.definition ?? '').trim().length, `${r.name} was captured without a body`).toBeGreaterThan(0); + } + + // The round trip: regenerate into the other schema and run it. This + // is what catches a definition that is missing its terminator or has + // the source schema baked into it. + const compare = await new CompareModule().compare(mine, [], { + source: target.dialect, + target: target.dialect, + }); + const stmts = gen + .generateMigrationPlan(compare.tables, target.dialect, { sourceSchema: spec.from, targetSchema: spec.to }) + .flatMap((s) => s.statements) + .filter((s) => !s.trim().startsWith('--')); + expect(stmts.length, 'no DDL generated for the routines').toBeGreaterThan(0); + + const other = await ConnectionFactory.create(target.provider, opts(spec.to), { pooled: false }); + try { + for (const raw of stmts) { + const sql = raw.replace(/;\s*$/, ''); + try { + await exec(other, sql); + } catch (err) { + throw new Error( + `${target.dialect} rejected the regenerated routine:\n${sql}\n\n${(err as Error).message.split('\n')[0]}` + ); + } + } + } finally { + await ConnectionFactory.close(target.provider, other).catch(() => undefined); + } + } finally { + // Routines are dropped by name; the schemas themselves are left for + // the engine's own cleanup since each run uses a fresh TAG. + for (const stmt of spec.ddl(spec.from)) { + const kind = /FUNCTION/i.test(stmt) ? 'FUNCTION' : 'PROCEDURE'; + const name = stmt.match(/CREATE (?:FUNCTION|PROCEDURE)\s+(\S+?)\s*[(\s]/i)?.[1]; + if (name) await exec(admin, `DROP ${kind} ${name}`).catch(() => undefined); + } + await ConnectionFactory.close(target.provider, admin).catch(() => undefined); + } + }); + it('alters a table whose names need quoting', async (ctx) => { // ctx.skip() rather than `return`: a skipped engine must not report a // green tick it never earned. From 416de6d4240d85b2e9d0a61974e60f503593cb49 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 22:44:20 -0600 Subject: [PATCH 14/14] chore(sync): drop the bindings left behind when Compare lost its Browse buttons `browseSchema`, `isBrowsing`, `dialectOptions` and the `PROVIDER_SETTINGS` import stopped being referenced when Browse became its own pane and the buttons came out of Compare. Dead either way, but they read as if Compare still has a browse path. Co-Authored-By: Claude Opus 5 --- apps/web/src/frontend/components/TopToolbar.tsx | 5 ----- 1 file changed, 5 deletions(-) diff --git a/apps/web/src/frontend/components/TopToolbar.tsx b/apps/web/src/frontend/components/TopToolbar.tsx index 7900d2b3..8073316e 100644 --- a/apps/web/src/frontend/components/TopToolbar.tsx +++ b/apps/web/src/frontend/components/TopToolbar.tsx @@ -10,7 +10,6 @@ import { CredentialManager } from './CredentialManager'; import { MigrationHistory } from './MigrationHistory'; import { TYPE_META, TYPE_ORDER } from './SchemaTreePanel'; import type { DbObjectType } from '../lib/types'; -import { PROVIDER_SETTINGS } from '../lib/provider-settings'; import { ConnectionModal } from './ConnectionModal'; import { PasswordInput } from './PasswordInput'; import { useAuthStore } from '../store/authStore'; @@ -22,8 +21,6 @@ import { BrowseBar } from './BrowseBar'; const ProfileMenu = ProfileMenuNamed ?? ProfileMenuDefault; -const dialectOptions = Object.values(PROVIDER_SETTINGS); - export const TopToolbar: React.FC = () => { const { sourceConfig, @@ -38,8 +35,6 @@ export const TopToolbar: React.FC = () => { testTargetConnection, isComparing, runSchemaComparison, - browseSchema, - isBrowsing, compareResult, resetSync, selectedObjectTypes,