diff --git a/apps/web/src/backend/api/routes.ts b/apps/web/src/backend/api/routes.ts index 143af751..938a30f2 100644 --- a/apps/web/src/backend/api/routes.ts +++ b/apps/web/src/backend/api/routes.ts @@ -14,6 +14,7 @@ import { dialectSupportsIndexFragmentation, buildIndexFragmentationCustomTemplate, sqlStatementCategories, + statementVerb, type MigrationStep, type ConnectionOptions, type DbObjectType, @@ -521,11 +522,22 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt } // Grid CRUD also needs the matching Data grid permission so Access control // can allow SQL DML without exposing Add/Edit/Delete on Peek / results. + // Require the SQL verb to match the claimed action so a client cannot label + // datagridAction=insert while sending UPDATE/DELETE (or DDL). if (datagridAction !== undefined) { if (!isDatagridAction(datagridAction)) { res.status(400).json({ error: 'datagridAction must be insert, update, or delete.' }); return; } + for (const sql of statements as string[]) { + const verb = statementVerb(sql); + if (verb !== datagridAction) { + res.status(400).json({ + error: `datagridAction (${datagridAction}) must match SQL verb (${verb ?? 'unknown'}).`, + }); + return; + } + } needed.add(DATAGRID_ACTION_PERMISSION[datagridAction]); } if (needed.size > 0 && denyUnless(authed, res, ...needed)) return; @@ -826,10 +838,38 @@ export function createApiRoutes(connectionModule: ConnectionModule, connectionSt res.status(400).json({ error: 'Each op needs key and sql.' }); return; } + if (o.sql.length > MAX_STATEMENT_LENGTH) { + res.status(400).json({ error: `Each op.sql must be under ${MAX_STATEMENT_LENGTH} characters.` }); + return; + } if (o.params !== undefined && !Array.isArray(o.params)) { res.status(400).json({ error: 'op.params must be an array when set.' }); return; } + // Fail-closed like /sql/execute: classify the SQL itself so a client cannot + // label op=insert while sending DELETE/DDL/GRANT and bypass finer permissions. + const categories = sqlStatementCategories(o.sql); + if (categories.length === 0) { + res.status(400).json({ error: 'Could not classify op.sql.' }); + return; + } + for (const category of categories) { + const permission = CATEGORY_PERMISSION[category]; + if (permission) needed.add(permission); + if (category !== 'dml') { + res.status(400).json({ + error: `Data migrate op.sql must be DML (got ${category}).`, + }); + return; + } + } + const verb = statementVerb(o.sql); + if (verb !== o.op) { + res.status(400).json({ + error: `op.sql verb (${verb ?? 'unknown'}) must match op (${o.op}).`, + }); + return; + } needed.add(DATAGRID_ACTION_PERMISSION[o.op]); ops.push({ op: o.op, diff --git a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx index ab6c068c..c5730264 100644 --- a/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx +++ b/apps/web/src/frontend/components/sql-editor/DataMigrateBar.tsx @@ -6,7 +6,7 @@ * Side-by-side data migrate: key-based insert/update/delete onto a destination * grid (≤500 ops). Larger sets toast with Server Beam instructions. */ -import React, { useEffect, useMemo, useState } from 'react'; +import React, { useMemo, useState } from 'react'; import { createPortal } from 'react-dom'; import { ArrowRightLeft, History, Loader2, X } from 'lucide-react'; import { @@ -23,6 +23,7 @@ import { buildDataMigratePlans, buildDestSnapshotJson } from '../../lib/dataMigr import { classifyRowsByKey, DATA_MIGRATE_ROW_CAP, + migrateGridsAreComplete, selectMigrateOps, type ClassifiedRowDiff, } from '../../lib/resultRowDiff'; @@ -42,6 +43,10 @@ export interface DataMigrateGrid { columns: string[]; rows: unknown[][]; statementSql?: string; + /** 0-based page currently shown in the result grid. */ + pageIndex?: number; + /** True when more rows exist beyond this page (hasNext / truncated). */ + hasMore?: boolean; } type ProgressItem = { @@ -91,19 +96,16 @@ export const DataMigrateBar: React.FC = ({ const table: TableSchema | undefined = editTarget.ok ? editTarget.table : undefined; const tableName = table?.name ?? ''; - const defaultKeys = useMemo( - () => resolvePeekKeyColumns(table, source.columns).map((k) => k.name), + // Only PK / non-partial unique keys that appear in the result — never fall back + // to "first column" (non-unique WHERE would UPDATE/DELETE multiple rows). + const keyNames = useMemo( + () => + resolvePeekKeyColumns(table, source.columns) + .filter((k) => k.resultIndex >= 0) + .map((k) => k.name), [table, source.columns] ); - const [keyNamesLocal, setKeyNamesLocal] = useState([]); - const keyNames = keyNamesProp ?? keyNamesLocal; - const setKeyNames = onKeyNamesChange ?? setKeyNamesLocal; - useEffect(() => { - if (keyNamesProp) return; - setKeyNamesLocal(defaultKeys.length ? defaultKeys : source.columns.slice(0, 1)); - }, [defaultKeys.join('\0'), source.columns.join('\0'), keyNamesProp]); - /** User opts into each op — nothing selected until they choose. */ const [doInsert, setDoInsert] = useState(false); const [doUpdate, setDoUpdate] = useState(false); @@ -146,13 +148,6 @@ export const DataMigrateBar: React.FC = ({ [classification, doInsert, doUpdate, doDelete] ); - const toggleKey = (name: string) => { - const next = keyNames.some((k) => k.toLowerCase() === name.toLowerCase()) - ? keyNames.filter((k) => k.toLowerCase() !== name.toLowerCase()) - : [...keyNames, name]; - setKeyNames(next); - }; - const openHistory = async () => { setHistoryOpen(true); try { @@ -182,14 +177,49 @@ export const DataMigrateBar: React.FC = ({ }); return; } - if (keyNames.length === 0) { - toast({ tone: 'warning', title: 'Select at least one key column' }); + if (!editability.editable || keyNames.length === 0) { + toast({ + tone: 'warning', + title: 'Data migrate needs a unique key in the result', + body: + editability.reason || + 'Include the primary key (or a non-partial unique index) columns in the SELECT.', + }); + return; + } + if ( + !migrateGridsAreComplete({ + sourcePageIndex: source.pageIndex ?? 0, + destPageIndex: dest.pageIndex ?? 0, + sourceHasMore: Boolean(source.hasMore), + destHasMore: Boolean(dest.hasMore), + }) + ) { + toast({ + tone: 'warning', + title: 'Migrate needs the full result on page 1', + body: + 'Add / Edit / Delete classify only the rows currently loaded. ' + + 'Page both grids to page 1 with no “next page”, or tighten the SELECT ' + + `(LIMIT ≤ page size). Otherwise Delete can remove destination rows that ` + + 'still exist later in the source.', + }); return; } if (selected.uncappedCount === 0) { toast({ tone: 'info', title: 'Nothing to migrate', body: 'Grids match for the selected ops.' }); return; } + if (classification.duplicateKeys > 0) { + toast({ + tone: 'warning', + title: 'Duplicate keys in result grids', + body: + `${classification.duplicateKeys} duplicate key value(s) — migrate refuses to guess which row to write. ` + + 'Tighten the SELECT (DISTINCT / better keys) or pick a unique key set.', + }); + return; + } if (selected.uncappedCount > DATA_MIGRATE_ROW_CAP) { toast({ tone: 'warning', @@ -397,17 +427,21 @@ export const DataMigrateBar: React.FC = ({
Keys - {source.columns.map((c) => ( - - ))} + {keyNames.length > 0 ? ( + keyNames.map((c) => ( + + {c} + + )) + ) : ( + + {editability.reason || 'No PK/unique key in this result — migrate disabled.'} + + )}
@@ -499,6 +533,7 @@ export const DataMigrateBar: React.FC = ({ applying || !canDml || selected.uncappedCount === 0 || + classification.duplicateKeys > 0 || selected.uncappedCount > DATA_MIGRATE_ROW_CAP } onClick={() => void apply()} diff --git a/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx b/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx index f86da904..4b26c5e0 100644 --- a/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx +++ b/apps/web/src/frontend/components/sql-editor/ResultsPanel.tsx @@ -1057,6 +1057,11 @@ const SideBySideStatementSection: React.FC<{ columns: sourceGrid.result.columns, rows: sourceGrid.result.rows, statementSql: sourceGrid.statementSql, + pageIndex: + pageState?.[`${sourceGrid.connectionId}:${statementIndex}`]?.pageIndex ?? 0, + hasMore: Boolean( + sourceGrid.result.hasNext || sourceGrid.result.truncated + ), }} dest={{ connectionId: destGrid.connectionId, @@ -1065,6 +1070,9 @@ const SideBySideStatementSection: React.FC<{ columns: destGrid.result.columns, rows: destGrid.result.rows, statementSql: destGrid.statementSql, + pageIndex: + pageState?.[`${destGrid.connectionId}:${statementIndex}`]?.pageIndex ?? 0, + hasMore: Boolean(destGrid.result.hasNext || destGrid.result.truncated), }} ignoreColumns={triggerIgnoreColumns} keyNames={effectiveKeys} @@ -1088,10 +1096,9 @@ const SideBySideStatementSection: React.FC<{ /> {compareActive && (

- {keyAligned - ? 'Rows line up by Keys (same as Data migrate). Matching keys share a row; only-here rows show as missing/extra. ' - : 'No usable key yet — cells align by row index. Pick Keys in Data migrate for friendlier matching. '} - Choose Add / Edit / Delete yourself; Transaction and Stop / Continue are safety assists. + Cell colors align by row index. Migrate matches rows by PK/unique keys and only runs when + both grids show the full result on page 1. Choose Add / Edit / Delete yourself; Transaction + and Stop / Continue are safety assists. Skip trigger cols ignores createdAt / updatedBy. Cap: 500 ops — larger sets use Server Beam.

)} diff --git a/apps/web/src/frontend/lib/dataMigratePlans.test.ts b/apps/web/src/frontend/lib/dataMigratePlans.test.ts index 6745a581..18804961 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.test.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.test.ts @@ -47,6 +47,27 @@ describe('buildDataMigratePlans', () => { expect(parsed.rows).toHaveLength(2); }); + it('omits identity columns from INSERT when includeIdentity is false', () => { + const { plans, errors } = buildDataMigratePlans({ + tableName: 'customers', + dialect: 'sqlite', + sourceColumns: cols, + destColumns: cols, + keyNames: ['id'], + ops: [{ op: 'insert', keyLabel: 'id=3', sourceRow: [3, 'New'] }], + includeIdentity: false, + identityColumns: new Set(['id']), + }); + expect(errors).toEqual([]); + expect(plans).toHaveLength(1); + const sql = plans[0]!.plan.sql.toLowerCase(); + expect(sql).toContain('insert'); + expect(sql).toContain('name'); + // Must not bind/preserve the source id when Include identity is off. + expect(sql).not.toMatch(/\bid\b/); + expect(plans[0]!.plan.params).toEqual(['New']); + }); + it('omits ignored trigger columns from INSERT and UPDATE SQL', () => { const auditCols = ['id', 'name', 'createdAt', 'updatedBy']; const auditOps: ClassifiedRowDiff[] = [ diff --git a/apps/web/src/frontend/lib/dataMigratePlans.ts b/apps/web/src/frontend/lib/dataMigratePlans.ts index bd0da320..b5cbe8dc 100644 --- a/apps/web/src/frontend/lib/dataMigratePlans.ts +++ b/apps/web/src/frontend/lib/dataMigratePlans.ts @@ -97,19 +97,28 @@ export function buildDataMigratePlans(opts: { const plans: DataMigratePlanItem[] = []; const errors: string[] = []; + // When Include identity is off, omit identity/autoincrement columns entirely + // so the destination generates them. buildPeekInsert only skips *empty* + // identity values (Peek UX fills those blank) — source migrate rows always + // carry real IDs, so we must strip here or IDs are preserved contrary to UI. + const insertIgnore = new Set(ignoreLower); + if (!includeIdentity) { + for (const name of identityColumns) insertIgnore.add(name.toLowerCase()); + } + for (const op of ops) { if (op.op === 'insert') { if (!op.sourceRow) { errors.push(`insert ${op.keyLabel}: missing source row`); continue; } - const stripped = stripIgnoredColumns(sourceColumns, op.sourceRow, ignoreLower); + const stripped = stripIgnoredColumns(sourceColumns, op.sourceRow, insertIgnore); const built = buildPeekInsert({ tableName, dialect, values: rowToValues(stripped.columns, stripped.row), - // Empty skip-set when includeIdentity — keep source ID values. - identityColumns: includeIdentity ? undefined : identityColumns, + // Identity already stripped when !includeIdentity; when on, keep values. + identityColumns: undefined, }); if ('error' in built) { errors.push(`insert ${op.keyLabel}: ${built.error}`); diff --git a/apps/web/src/frontend/lib/resultRowDiff.test.ts b/apps/web/src/frontend/lib/resultRowDiff.test.ts index 0e6c4254..7b27d682 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.test.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.test.ts @@ -7,6 +7,7 @@ import { describe, expect, it } from 'vitest'; import { classifyRowsByKey, DATA_MIGRATE_ROW_CAP, + migrateGridsAreComplete, selectMigrateOps, } from './resultRowDiff'; @@ -66,6 +67,36 @@ describe('classifyRowsByKey', () => { expect(c.deletes).toHaveLength(0); }); + it('does not collide when key values contain delimiter characters', () => { + const source = { + columns: ['a', 'b', 'v'], + rows: [['foo|b=bar', 'baz', 1]], + }; + const dest = { + columns: ['a', 'b', 'v'], + rows: [['foo', 'bar|b=baz', 2]], + }; + const c = classifyRowsByKey({ source, dest, keyNames: ['a', 'b'] }); + expect(c.updates).toHaveLength(0); + expect(c.inserts).toHaveLength(1); + expect(c.deletes).toHaveLength(1); + }); + + it('counts duplicate keys instead of silently keeping the first row', () => { + const source = { + columns: ['id', 'name'], + rows: [ + [1, 'first'], + [1, 'second'], + ], + }; + const dest = { columns: ['id', 'name'], rows: [[1, 'dest']] }; + const c = classifyRowsByKey({ source, dest, keyNames: ['id'] }); + expect(c.duplicateKeys).toBe(1); + expect(c.updates).toHaveLength(1); + expect(c.updates[0]!.sourceRow).toEqual([1, 'first']); + }); + it('does not treat differing trigger columns as updates when ignored', () => { const columns = ['id', 'name', 'createdAt', 'updatedBy']; const source = { @@ -91,6 +122,37 @@ describe('classifyRowsByKey', () => { }); +describe('migrateGridsAreComplete', () => { + it('requires page 1 with no remaining pages on both sides', () => { + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 0, + sourceHasMore: false, + destHasMore: false, + }) + ).toBe(true); + // Dest on page 2 while source stays on page 1 → Delete would drop dest keys + // that still exist later in the source. + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 1, + sourceHasMore: false, + destHasMore: false, + }) + ).toBe(false); + expect( + migrateGridsAreComplete({ + sourcePageIndex: 0, + destPageIndex: 0, + sourceHasMore: true, + destHasMore: false, + }) + ).toBe(false); + }); +}); + describe('selectMigrateOps', () => { it('respects checkboxes and caps at 500', () => { const inserts = Array.from({ length: 300 }, (_, i) => ({ @@ -109,6 +171,7 @@ describe('selectMigrateOps', () => { updates, deletes: [], skippedNullKeys: 0, + duplicateKeys: 0, totalOps: 600, }; const selected = selectMigrateOps( diff --git a/apps/web/src/frontend/lib/resultRowDiff.ts b/apps/web/src/frontend/lib/resultRowDiff.ts index 89d38359..c5f20d46 100644 --- a/apps/web/src/frontend/lib/resultRowDiff.ts +++ b/apps/web/src/frontend/lib/resultRowDiff.ts @@ -34,6 +34,8 @@ export interface RowDiffClassification { updates: ClassifiedRowDiff[]; deletes: ClassifiedRowDiff[]; skippedNullKeys: number; + /** Duplicate key values seen in source or dest (unsafe for migrate). */ + duplicateKeys: number; /** Total ops before cap. */ totalOps: number; } @@ -59,20 +61,27 @@ export function keyColumnsForGrid( })); } +/** + * Stable map key for composite PK matching. JSON-array encoding avoids + * collisions when a key value contains `|` / `=` (naive `a=x|b=y` join could + * treat distinct composite keys as the same row and UPDATE/DELETE the wrong one). + * Values are stringified so number `1` and string `"1"` still match across dialects. + */ function rowKey( row: unknown[], keys: PeekKeyColumn[] ): { ok: true; key: string; label: string } | { ok: false } { - const parts: string[] = []; + const wire: [string, string][] = []; const labels: string[] = []; for (const k of keys) { if (k.resultIndex < 0) return { ok: false }; const v = row[k.resultIndex]; if (v === null || v === undefined) return { ok: false }; - parts.push(`${k.name.toLowerCase()}=${String(v)}`); - labels.push(`${k.name}=${String(v)}`); + const asText = typeof v === 'bigint' ? v.toString() : String(v); + wire.push([k.name.toLowerCase(), asText]); + labels.push(`${k.name}=${asText}`); } - return { ok: true, key: parts.join('|'), label: labels.join(', ') }; + return { ok: true, key: JSON.stringify(wire), label: labels.join(', ') }; } function nonKeyColumnsDiffer( @@ -125,6 +134,7 @@ export function classifyRowsByKey(opts: { updates: [], deletes: [], skippedNullKeys: 0, + duplicateKeys: 0, totalOps: 0, }; } @@ -132,6 +142,7 @@ export function classifyRowsByKey(opts: { const sourceMap = new Map(); const destMap = new Map(); let skippedNullKeys = 0; + let duplicateKeys = 0; for (const row of source.rows) { const k = rowKey(row, sourceKeys); @@ -139,7 +150,11 @@ export function classifyRowsByKey(opts: { skippedNullKeys += 1; continue; } - if (!sourceMap.has(k.key)) sourceMap.set(k.key, { row, label: k.label }); + if (sourceMap.has(k.key)) { + duplicateKeys += 1; + continue; + } + sourceMap.set(k.key, { row, label: k.label }); } for (const row of dest.rows) { const k = rowKey(row, destKeys); @@ -147,7 +162,11 @@ export function classifyRowsByKey(opts: { skippedNullKeys += 1; continue; } - if (!destMap.has(k.key)) destMap.set(k.key, { row, label: k.label }); + if (destMap.has(k.key)) { + duplicateKeys += 1; + continue; + } + destMap.set(k.key, { row, label: k.label }); } const inserts: ClassifiedRowDiff[] = []; @@ -188,6 +207,7 @@ export function classifyRowsByKey(opts: { updates, deletes, skippedNullKeys, + duplicateKeys, totalOps: inserts.length + updates.length + deletes.length, }; } @@ -206,3 +226,23 @@ export function selectMigrateOps( if (all.length <= cap) return { ops: all, truncated: false, uncappedCount }; return { ops: all.slice(0, cap), truncated: true, uncappedCount }; } + +/** + * Data migrate classifies only the rows currently loaded in each grid. + * If either side is on a later page or still has more pages / truncated + * rows, "missing from this page" is not "missing from the table" — Delete + * would destroy real destination rows that exist later in the source. + */ +export function migrateGridsAreComplete(opts: { + sourcePageIndex: number; + destPageIndex: number; + sourceHasMore: boolean; + destHasMore: boolean; +}): boolean { + return ( + opts.sourcePageIndex === 0 && + opts.destPageIndex === 0 && + !opts.sourceHasMore && + !opts.destHasMore + ); +} diff --git a/apps/web/src/frontend/lib/triggerManagedColumns.test.ts b/apps/web/src/frontend/lib/triggerManagedColumns.test.ts index 9ef5a131..5bda2a74 100644 --- a/apps/web/src/frontend/lib/triggerManagedColumns.test.ts +++ b/apps/web/src/frontend/lib/triggerManagedColumns.test.ts @@ -26,12 +26,20 @@ describe('isLikelyTriggerManagedColumn', () => { expect(isLikelyTriggerManagedColumn(name)).toBe(true); }); - it.each(['id', 'name', 'city', 'create_order', 'update_count', 'status'])( - 'does not flag %s', - (name) => { - expect(isLikelyTriggerManagedColumn(name)).toBe(false); - } - ); + it.each([ + 'id', + 'name', + 'city', + 'create_order', + 'update_count', + 'status', + // Bare names are often business columns — must not skip under migrate. + 'created', + 'updated', + 'modified', + ])('does not flag %s', (name) => { + expect(isLikelyTriggerManagedColumn(name)).toBe(false); + }); }); describe('detectTriggerManagedColumns', () => { diff --git a/apps/web/src/frontend/lib/triggerManagedColumns.ts b/apps/web/src/frontend/lib/triggerManagedColumns.ts index c47291d7..ed557105 100644 --- a/apps/web/src/frontend/lib/triggerManagedColumns.ts +++ b/apps/web/src/frontend/lib/triggerManagedColumns.ts @@ -39,9 +39,14 @@ const EXACT = new Set([ 'xmin', // Postgres system ]); -/** Suffix / contains patterns (lower-case, no separators normalized). */ +/** + * Audit-style names with an explicit time/actor suffix. + * Bare `created` / `updated` / `modified` are NOT matched — those are often + * business columns; skipping them under "Skip trigger cols" would silently + * drop real data from migrate INSERT/UPDATE. + */ const PATTERN = - /^(created|updated|modified|lastmodified)(at|on|by|date|time|timestamp)?$|^(created|updated|modified)_?(at|on|by|date|time|timestamp)$|_?(created|updated|modified)_?(at|on|by)$/; + /^(created|updated|modified|lastmodified)(at|on|by|date|time|timestamp)$|^(created|updated|modified)_?(at|on|by|date|time|timestamp)$|_(created|updated|modified)_?(at|on|by)$/; function normalize(name: string): string { return name.trim().toLowerCase().replace(/[^a-z0-9_]/g, ''); diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 816daf04..66fb4870 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -139,8 +139,12 @@ compare / migrate). It lives in the same local web UI you open with `foxschema`. default) ignores audit fields such as `createdAt` / `updatedBy`. 7. **Data migrate (≤500 row ops)** — with Compare on, **Data migrate** appears. - You choose **Add / Edit / Delete** (none selected until you check them). Safety - assists: **Transaction** (all-or-nothing when Stop is on) and **Stop on error** / + Rows match by the table’s primary key (or a non-partial unique index) present + in the SELECT. Migrate only runs when **both** grids show the **full** result + on **page 1** (no next page) — otherwise “missing on this page” is not “missing + from the table” and Delete could remove real destination rows. You choose + **Add / Edit / Delete** (none selected until you check them). Safety assists: + **Transaction** (all-or-nothing when Stop is on) and **Stop on error** / **Continue on error**. Optional **Include identity / IDs**. With **Skip trigger cols**, migrate does not treat audit columns as edits and omits them from INSERT/UPDATE so destination triggers can fill them. Progress lists each row; @@ -338,8 +342,10 @@ Tips: -- @end ``` -- **Safe mode** — when on, UPDATE / DELETE / MERGE and DDL need an extra - confirmation before run. Plain INSERT (including insert CTEs) does not. +- **Safe mode** — when on, UPDATE / DELETE / MERGE, upserts that can overwrite + rows (`ON CONFLICT DO UPDATE`, `ON DUPLICATE KEY UPDATE`, `INSERT OR REPLACE`), + and DDL need an extra confirmation before run. Plain INSERT (including insert + CTEs and `ON CONFLICT DO NOTHING`) does not. Writes and DDL are allowed when you confirm them. Some dialects (e.g. SQLite / ClickHouse adapters used for SELECT-only paths) may reject writes with a clear error diff --git a/packages/db/src/providers/duckDb/duckdb.adapter.test.ts b/packages/db/src/providers/duckDb/duckdb.adapter.test.ts new file mode 100644 index 00000000..5eab2f14 --- /dev/null +++ b/packages/db/src/providers/duckDb/duckdb.adapter.test.ts @@ -0,0 +1,21 @@ +/** + * Fox Schema (foxschema) + * Copyright 2024-2026 Huy Phan + * SPDX-License-Identifier: Apache-2.0 + */ +import { describe, expect, it } from 'vitest'; +import { coerceDriverBigint } from './duckdb.adapter'; + +describe('coerceDriverBigint', () => { + it('keeps safe integers as numbers', () => { + expect(coerceDriverBigint(42n)).toBe(42); + expect(coerceDriverBigint(BigInt(Number.MAX_SAFE_INTEGER))).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('stringifies integers past MAX_SAFE_INTEGER so PK WHERE keys are not rounded', () => { + const unsafe = 9007199254740993n; // Number(unsafe) === 9007199254740992 + expect(Number(unsafe)).toBe(9007199254740992); + expect(Number.isSafeInteger(Number(unsafe))).toBe(false); + expect(coerceDriverBigint(unsafe)).toBe('9007199254740993'); + }); +}); diff --git a/packages/db/src/providers/duckDb/duckdb.adapter.ts b/packages/db/src/providers/duckDb/duckdb.adapter.ts index fc956d18..158fb289 100644 --- a/packages/db/src/providers/duckDb/duckdb.adapter.ts +++ b/packages/db/src/providers/duckDb/duckdb.adapter.ts @@ -92,13 +92,27 @@ class DuckDbAdapter implements DriverAdapter { } } -/** Shallow-coerce bigint values in a row object to Number (introspection values are small). */ +/** + * Coerce driver bigint for JSON / grid use. + * Safe integers stay numbers (introspection + small PKs). Values beyond + * Number.MAX_SAFE_INTEGER stay digit strings so Peek / result-grid + * UPDATE/DELETE WHERE clauses are not silently rounded to the wrong key. + */ +export function coerceDriverBigint(v: bigint): number | string { + const asNumber = Number(v); + return Number.isSafeInteger(asNumber) ? asNumber : v.toString(); +} + +/** Shallow-coerce bigint values in a row object for callers that expect JSON-ish cells. */ function coerceBigints(row: Record): Record { let hasBig = false; for (const k in row) if (typeof row[k] === 'bigint') { hasBig = true; break; } if (!hasBig) return row; const out: Record = {}; - for (const k in row) out[k] = typeof row[k] === 'bigint' ? Number(row[k] as bigint) : row[k]; + for (const k in row) { + const v = row[k]; + out[k] = typeof v === 'bigint' ? coerceDriverBigint(v) : v; + } return out; } diff --git a/packages/sql/src/modules/sql-splitter.test.ts b/packages/sql/src/modules/sql-splitter.test.ts index 38f29576..e1f75ae6 100644 --- a/packages/sql/src/modules/sql-splitter.test.ts +++ b/packages/sql/src/modules/sql-splitter.test.ts @@ -353,6 +353,10 @@ describe('isInsertWriteStatement', () => { isInsertWriteStatement('WITH i AS (INSERT INTO t VALUES (1) RETURNING id) SELECT id FROM i;') ).toBe(true); expect(isInsertWriteStatement('EXPLAIN ANALYZE INSERT INTO t VALUES (1);')).toBe(true); + // DO NOTHING cannot overwrite existing rows — still insert-only. + expect( + isInsertWriteStatement('INSERT INTO t(id,x) VALUES (1,2) ON CONFLICT (id) DO NOTHING;') + ).toBe(true); }); it('rejects non-insert writes and mixed mutating CTEs', () => { @@ -367,6 +371,49 @@ describe('isInsertWriteStatement', () => { ) ).toBe(false); }); + + it('rejects upserts that can overwrite existing rows (Safe Mode must confirm)', () => { + expect( + isInsertWriteStatement( + 'INSERT INTO t(id,x) VALUES (1,2) ON CONFLICT (id) DO UPDATE SET x = EXCLUDED.x;' + ) + ).toBe(false); + expect( + isInsertWriteStatement( + 'INSERT INTO t(id,x) VALUES (1,2) ON DUPLICATE KEY UPDATE x = VALUES(x);' + ) + ).toBe(false); + expect(isInsertWriteStatement('INSERT OR REPLACE INTO t(id,x) VALUES (1,2);')).toBe(false); + expect(isInsertWriteStatement('REPLACE INTO t(id,x) VALUES (1,2);')).toBe(false); + }); +}); + +describe('isWriteStatement — MATERIALIZED / quoted CTE names', () => { + it('detects DELETE/UPDATE inside AS MATERIALIZED CTEs (Safe Mode)', () => { + expect( + isWriteStatement('WITH d AS MATERIALIZED (DELETE FROM t RETURNING id) SELECT id FROM d;') + ).toBe(true); + expect( + isWriteStatement( + 'WITH u AS NOT MATERIALIZED (UPDATE t SET x = 1 RETURNING *) SELECT * FROM u;' + ) + ).toBe(true); + expect( + isWriteStatement('WITH d AS MATERIALIZED (SELECT id FROM t) SELECT id FROM d;') + ).toBe(false); + }); + + it('detects DELETE inside quoted / bracketed CTE names', () => { + expect( + isWriteStatement('WITH "i" AS (DELETE FROM t RETURNING id) SELECT id FROM "i";') + ).toBe(true); + expect( + isWriteStatement('WITH [i] AS (DELETE FROM t OUTPUT DELETED.id) SELECT * FROM [i];') + ).toBe(true); + expect( + isWriteStatement('WITH `i` AS (DELETE FROM t RETURNING id) SELECT id FROM `i`;') + ).toBe(true); + }); }); describe('isPageableStatement', () => { diff --git a/packages/sql/src/modules/sql-splitter.ts b/packages/sql/src/modules/sql-splitter.ts index 0744d1f4..82b69d3a 100644 --- a/packages/sql/src/modules/sql-splitter.ts +++ b/packages/sql/src/modules/sql-splitter.ts @@ -674,13 +674,16 @@ function sqlTextIsWrite(text: string): boolean { return inner !== null && sqlTextIsWrite(inner); } if (kw === 'with') { - for (const body of withCteBodies(text)) { + const bodies: string[] = []; + // Recurse into CTE bodies + the tail rather than matching its leading verb, so + // `WITH … SELECT * INTO t` and `WITH … EXPLAIN ANALYZE DELETE` still warn. + // Unscannable WITH (parser hole) fails closed → treat as write so Safe Mode confirms. + const tail = walkWithCtes(text, (body) => bodies.push(body)); + if (tail === null) return true; + for (const body of bodies) { if (sqlTextIsWrite(body)) return true; } - // Recurse into the tail rather than matching its leading verb, so - // `WITH … SELECT * INTO t` and `WITH … EXPLAIN ANALYZE DELETE` still warn. - const tail = walkWithCtes(text); - return tail !== null && sqlTextIsWrite(tail); + return sqlTextIsWrite(tail); } return false; } @@ -927,20 +930,25 @@ function sqlTextIsMutatingDml(text: string): boolean { } if (MUTATING_DML.has(kw)) return true; if (kw === 'with') { - for (const body of withCteBodies(text)) { + const bodies: string[] = []; + const tail = walkWithCtes(text, (body) => bodies.push(body)); + // Match isWriteStatement: unscannable WITH is treated as mutating for Safe Mode. + if (tail === null) return true; + for (const body of bodies) { if (sqlTextIsMutatingDml(body)) return true; } - const tail = walkWithCtes(text); - return tail !== null && sqlTextIsMutatingDml(tail); + return sqlTextIsMutatingDml(tail); } return false; } /** - * True when every write in the statement is INSERT (plain `INSERT …`, - * `WITH … INSERT …`, or `WITH i AS (INSERT … RETURNING …) SELECT …`). + * True when every write in the statement is a plain INSERT (additive only): + * `INSERT …`, `WITH … INSERT …`, or `WITH i AS (INSERT … RETURNING …) SELECT …`. * Safe mode skips confirmation for these; UPDATE/DELETE/MERGE, DDL, - * `SELECT … INTO`, and mixed insert+mutating CTEs still confirm. + * `SELECT … INTO`, upserts that can overwrite rows (`ON CONFLICT DO UPDATE`, + * `ON DUPLICATE KEY UPDATE`, `INSERT OR REPLACE`), and mixed insert+mutating + * CTEs still confirm. */ export function isInsertWriteStatement(text: string): boolean { const parts = splitSqlStatements(text); @@ -956,6 +964,19 @@ export function isInsertWriteStatement(text: string): boolean { return sawInsert; } +/** + * True when an INSERT can overwrite existing rows (not a pure append). + * Strings/comments are stripped so literals cannot fake the keywords. + */ +function insertMayUpdateExisting(text: string): boolean { + const stripped = stripSqlStringsAndComments(text); + if (/\bINSERT\s+OR\s+REPLACE\b/i.test(stripped)) return true; + if (/\bON\s+DUPLICATE\s+KEY\s+UPDATE\b/i.test(stripped)) return true; + // Postgres/SQLite upsert that assigns columns — DO NOTHING stays insert-only. + if (/\bON\s+CONFLICT\b/i.test(stripped) && /\bDO\s+UPDATE\b/i.test(stripped)) return true; + return false; +} + function sqlTextIsInsertOnlyWrite(text: string): boolean { if (!sqlTextIsWrite(text)) return false; const kw = firstKeyword(text); @@ -964,12 +985,13 @@ function sqlTextIsInsertOnlyWrite(text: string): boolean { const inner = peelExplainAnalyze(text); return inner !== null && sqlTextIsInsertOnlyWrite(inner); } - if (kw === 'insert') return true; + if (kw === 'insert') return !insertMayUpdateExisting(text); if (kw === 'with') { - const bodies = withCteBodies(text); - const tail = walkWithCtes(text); - const chunks = tail ? [...bodies, tail] : bodies; - const writes = chunks.filter((c) => sqlTextIsWrite(c)); + const bodies: string[] = []; + const tail = walkWithCtes(text, (body) => bodies.push(body)); + // Unscannable WITH is a write (fail closed) but not insert-only → confirm. + if (tail === null) return false; + const writes = [...bodies, tail].filter((c) => sqlTextIsWrite(c)); return writes.length > 0 && writes.every((c) => sqlTextIsInsertOnlyWrite(c)); } return false; @@ -1206,8 +1228,9 @@ function keywordAfterWithCtes(text: string): string | null { } /** - * Scan `WITH [RECURSIVE] cte AS (body) [, …]` then return the remainder after - * the CTE list (main statement text). Invokes `onBody` for each CTE body. + * Scan `WITH [RECURSIVE] cte AS [NOT] MATERIALIZED (body) [, …]` then return + * the remainder after the CTE list (main statement text). Invokes `onBody` for + * each CTE body. CTE names may be bare or quoted (`"i"`, `` `i` ``, `[i]`). */ function walkWithCtes(text: string, onBody?: (body: string) => void): string | null { let i = 0; @@ -1220,9 +1243,9 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n const rec = nextWord(text, i); if (rec && rec.word === 'recursive') i = rec.end; - // Walk `name [(cols)] AS (…)` [, …] + // Walk `name [(cols)] AS [NOT] MATERIALIZED (…)` [, …] while (i < text.length) { - const name = nextWord(text, i); + const name = nextCteName(text, i); if (!name) return null; i = name.end; @@ -1237,6 +1260,16 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n if (!asKw || asKw.word !== 'as') return null; i = asKw.end; + // Postgres: AS MATERIALIZED (…) / AS NOT MATERIALIZED (…) + const mat = nextWord(text, i); + if (mat && mat.word === 'not') { + const mat2 = nextWord(text, mat.end); + if (!mat2 || mat2.word !== 'materialized') return null; + i = mat2.end; + } else if (mat && mat.word === 'materialized') { + i = mat.end; + } + const open = skipWsAndComments(text, i); if (open >= text.length || text[open] !== '(') return null; const close = skipBalancedParens(text, open); @@ -1255,6 +1288,32 @@ function walkWithCtes(text: string, onBody?: (body: string) => void): string | n return null; } +/** Bare or quoted CTE name; advances past the identifier. */ +function nextCteName(text: string, from: number): { end: number } | null { + const i = skipWsAndComments(text, from); + if (i >= text.length) return null; + const ch = text[i]!; + if (ch === '"' || ch === '`' || ch === '[') { + const close = ch === '[' ? ']' : ch; + let j = i + 1; + while (j < text.length) { + const c = text[j]!; + if (c === close) { + // SQL escaped quotes double the closer. + if (j + 1 < text.length && text[j + 1] === close) { + j += 2; + continue; + } + return { end: j + 1 }; + } + j++; + } + return null; + } + const bare = nextWord(text, i); + return bare ? { end: bare.end } : null; +} + function firstKeywordSpan(text: string): { word: string; end: number } | null { let mode: Mode = 'code'; for (let i = 0; i < text.length; i++) {