diff --git a/apps/e2e/src/pages/LokeeHistoryPage.ts b/apps/e2e/src/pages/LokeeHistoryPage.ts index c1d49589..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++) { @@ -343,4 +367,15 @@ export class LokeeHistoryPage { { timeout: 30_000 } ); } + + /** "↩ 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; + } } 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/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/api/routes.ts b/apps/web/src/backend/api/routes.ts index 98cb999b..514d7b0c 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/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/apps/web/src/backend/modules/lokee-weave.module.test.ts b/apps/web/src/backend/modules/lokee-weave.module.test.ts index 7385cf35..b1e20b2b 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(); @@ -878,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. @@ -893,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 d33ae542..3398f402 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, @@ -1167,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; @@ -1197,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/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..79bafbaf 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. + + 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 && ( + + )} +
{/* 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..54d398c4 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,65 @@ 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.' }; + } + // 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 '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 { + code: 'lossy', + label: 'Review data loss…', + why: 'This revert destroys data — review it on Migration SQL and confirm there.', + }; } - if (plan.reversal.risk === 'lossy' && !confirmLossy) return 'ACK_LOSSY'; - return ''; - }, [captureConnectionId, plan, planning, confirmLossy]); - const needsLossyAck = blockedReason === 'ACK_LOSSY'; + return null; + // 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 () => { if (!captureConnectionId || !plan) return; @@ -218,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', @@ -253,6 +303,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 +363,17 @@ export function VersionCompareModal({
{heading}
+ + + + {selectedKeys.length} of {changed.length} ticked + + { if (needsLossyAck) { setTab('SQL'); @@ -400,9 +521,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 2efb6fe9..fad2d6a4 100644 Binary files a/apps/web/src/frontend/components/lokee-weave/buildGraph.ts and b/apps/web/src/frontend/components/lokee-weave/buildGraph.ts differ 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 { 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/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/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/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; 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};`; },