From 286a4853b08324a3f5785ed40e7414b1ddd76110 Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 2 Aug 2026 22:39:14 -0600 Subject: [PATCH] fix(sql-editor): repair Server Beam gates, and make source/target visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #158 found main shipped red: `tsc --noEmit` failed with two errors and two unit tests were failing. Fixed those, plus the defects behind them. **main was broken.** - `SqlBinding = ReturnType` on a function annotated `: SqlBinding` is circular; TS2456 + TS2577. Dropped the annotation and let it infer — the alias still serves consumers. - `server-beam.test.ts` expected /at most 2/ while the message said "cant handle more than 2". Aligned the message (and its missing apostrophe). - `codeCellExec.test.ts` still asserted the OLD contract: #158 deliberately widened normalizeCodeCellReturn so `return 1` / `return [1,2,3]` / a bare object become grids. Updated the assertions to the new behaviour rather than narrowing the feature — only a missing return is rejected now. **Alias lookup accepted inherited keys.** `!beamDialects[key]` let `toString`, `constructor`, `valueOf` and `__proto__` past the unknown-alias check, then used the inherited *function* as the dialect — surfacing as "dialect .toLowerCase is not a function" instead of "Unknown Server Beam alias". Now `Object.hasOwn`. Not exploitable (the parent routes through a Map and fails closed) but a confusing dead end. Third time this class has appeared in this codebase — a shared hasOwn helper or lint rule would be cheaper than a fourth. **Which server is `target` was invisible.** Aliases come from list order, not click order, and `sql.on('target')` is what writes — so a wrong assumption writes to the wrong database. Two changes: a third checked Destination is now an error instead of a silent `slice(0, 2)`, and every beam run prints the resolved mapping ("Server Beam → source = A, target = B") before results. **Samples for both editor cases**, each executed against real SQLite before committing: - general, one server, no alias — plain sql`…`, no beam. - migration, source → target — read, reshape, chunked write, read back. Verified across two separate database files: rows landed in target, and the source was confirmed untouched. Writing the migration sample caught a bug in the sample itself: `domain` was split from the pre-lowercased email, yielding "Example.COM" beside "o'brien@example.com". Normalize once, then derive. 801 tests pass, tsc and eslint clean. Co-Authored-By: Claude Opus 5 --- apps/web/src/backend/api/code-cell-thread.ts | 8 ++- .../web/src/frontend/lib/codeCellExec.test.ts | 25 +++++++-- apps/web/src/frontend/lib/sqlEditorSamples.ts | 55 +++++++++++++++++++ .../src/frontend/store/useSqlEditorStore.ts | 23 +++++++- apps/web/src/shared/server-beam.test.ts | 23 ++++++++ apps/web/src/shared/server-beam.ts | 2 +- 6 files changed, 126 insertions(+), 10 deletions(-) diff --git a/apps/web/src/backend/api/code-cell-thread.ts b/apps/web/src/backend/api/code-cell-thread.ts index ae791763..b87bab1e 100644 --- a/apps/web/src/backend/api/code-cell-thread.ts +++ b/apps/web/src/backend/api/code-cell-thread.ts @@ -62,7 +62,7 @@ function makeSqlBinding(opts: { dialect?: string; beamDialects?: Record; defaultBeamAlias?: string; -}): SqlBinding { +}) { const beamDialects = opts.beamDialects ?? {}; const hasBeam = Object.keys(beamDialects).length > 0; @@ -82,7 +82,11 @@ function makeSqlBinding(opts: { let resolvedAlias = alias; if (hasBeam) { const key = alias ?? opts.defaultBeamAlias; - if (!key || !beamDialects[key]) { + // hasOwn, not truthiness: `beamDialects` is a plain object, so + // `beamDialects['toString']` is an inherited function and would sail + // through a `!value` check — then be used AS the dialect, dying later as + // "dialect.toLowerCase is not a function" instead of "unknown alias". + if (!key || !Object.hasOwn(beamDialects, key)) { const known = Object.keys(beamDialects).join(', ') || '(none)'; return Promise.reject( new Error( diff --git a/apps/web/src/frontend/lib/codeCellExec.test.ts b/apps/web/src/frontend/lib/codeCellExec.test.ts index 5363e24e..0d26e9f3 100644 --- a/apps/web/src/frontend/lib/codeCellExec.test.ts +++ b/apps/web/src/frontend/lib/codeCellExec.test.ts @@ -93,12 +93,29 @@ describe('normalizeCodeCellReturn', () => { ]); }); - it('rejects null, primitives, and arrays of non-objects', () => { + it('rejects only a missing return', () => { + // Everything else now becomes a grid, so null/undefined — "the cell forgot + // to return" — are the only rejections left. expect(normalizeCodeCellReturn(null, 10).ok).toBe(false); expect(normalizeCodeCellReturn(undefined, 10).ok).toBe(false); - expect(normalizeCodeCellReturn(42, 10).ok).toBe(false); - expect(normalizeCodeCellReturn([1, 2, 3], 10).ok).toBe(false); - expect(normalizeCodeCellReturn({ columns: ['a'] }, 10).ok).toBe(false); + }); + + it('treats a bare object as one row', () => { + const out = normalizeCodeCellReturn({ columns: ['a'] }, 10); + expect(out.ok).toBe(true); + if (out.ok) expect(out.columns).toEqual(['columns']); + }); + + it('accepts scalars and scalar arrays as single-column grids', () => { + // Widened deliberately so `return 1` / `return [1,2,3]` work in a cell — + // they land under a `value` column rather than erroring. + const scalar = normalizeCodeCellReturn(42, 10); + expect(scalar.ok).toBe(true); + if (scalar.ok) expect(scalar.rows).toEqual([[42]]); + + const list = normalizeCodeCellReturn([1, 2, 3], 10); + expect(list.ok).toBe(true); + if (list.ok) expect(list.rows).toEqual([[1], [2], [3]]); }); }); diff --git a/apps/web/src/frontend/lib/sqlEditorSamples.ts b/apps/web/src/frontend/lib/sqlEditorSamples.ts index 228e92fa..04a1c3fd 100644 --- a/apps/web/src/frontend/lib/sqlEditorSamples.ts +++ b/apps/web/src/frontend/lib/sqlEditorSamples.ts @@ -881,6 +881,61 @@ return [ { step: 3, value: seed + 2 }, ]; -- @end +`, + }, + { + id: 'sample-node-general-single-server', + title: '★ Sample · Node general (one server, no alias)', + sql: `-- CASE 1 — general work on ONE server. No alias: plain sql\`…\` runs on the +-- checked Destination, exactly like a normal query. Read-only, Safe mode ON is fine. + +-- @node +// No sql.on() anywhere, so this is NOT Server Beam — it fans out per checked +// Destination the way any other statement does. +const rows = await sql\`SELECT \${1} AS id, \${"o'brien@example.com"} AS email\`; +return rows.map((r) => ({ ...r, mode: 'general (no alias)' })); +-- @end +`, + }, + { + id: 'sample-node-beam-migrate-source-target', + title: '★ Sample · Node migrate (source → target)', + sql: `-- CASE 2 — migration ACROSS two servers. Check exactly two Destinations: +-- the FIRST is \`source\`, the SECOND is \`target\`. The run banner prints the +-- mapping ("Server Beam → source = …, target = …") — read it before running, +-- because sql.on('target') is what writes. +-- +-- WRITES on the target — turn Safe mode OFF. Written for Postgres / MySQL / +-- SQLite / SQL Server. + +-- @node +// 1. Read from the source. Never writes here. +const src = await sql.on('source')\`SELECT id, email FROM fox_beam_people ORDER BY id\`; + +if (src.length === 0) { + return [{ note: 'source table fox_beam_people is empty — nothing to migrate' }]; +} + +// 2. Reshape in JS. Column names fold to upper case on Oracle/Db2. +const rows = src.map((r) => { + // Normalize once, then derive — splitting the raw value would leave the + // domain in its original casing while the email is lowercased. + const email = String(r.email ?? r.EMAIL ?? '').toLowerCase(); + return { id: Number(r.id ?? r.ID), email, domain: email.split('@')[1] ?? '' }; +}); + +// 3. Write to the target, in batches, every value bound. +await sql.on('target')\`DROP TABLE IF EXISTS fox_beam_people_v2\`; +await sql.on('target')\`CREATE TABLE fox_beam_people_v2 (id INTEGER, email VARCHAR(200), domain VARCHAR(200))\`; + +const CHUNK = 50; +for (let i = 0; i < rows.length; i += CHUNK) { + await sql.on('target')\`INSERT INTO \${sql.id('fox_beam_people_v2')} \${sql.values(rows.slice(i, i + CHUNK))}\`; +} + +// 4. Read back from the target to prove it landed. +return await sql.on('target')\`SELECT id, email, domain FROM fox_beam_people_v2 ORDER BY id\`; +-- @end `, }, ]; diff --git a/apps/web/src/frontend/store/useSqlEditorStore.ts b/apps/web/src/frontend/store/useSqlEditorStore.ts index 439f3c2a..0c9d86b6 100644 --- a/apps/web/src/frontend/store/useSqlEditorStore.ts +++ b/apps/web/src/frontend/store/useSqlEditorStore.ts @@ -14,7 +14,7 @@ import { loadSchema } from '../api/schemaApi'; import { isMutatingDmlStatement, isWriteStatement, splitSqlStatements } from '../lib/sql-splitter'; import type { CodeCellLast } from '../lib/codeCellExec'; import { detectCodeCell, runCodeCell, usesServerBeam } from '../lib/codeCellRunner'; -import { beamAliasesForCount } from '../../shared/server-beam'; +import { beamAliasesForCount, MAX_SERVERS } from '../../shared/server-beam'; import { buildSampleBookmarks } from '../lib/sqlEditorSamples'; import { buildForeignKeyDrilldown, @@ -1086,7 +1086,11 @@ export const useSqlEditorStore = create()( // Server Beam (`sql.on`) runs once across up to two Destinations // (order = source, then target) instead of fan-out per credential. if (usesServerBeam(raw)) { - const beamConns = connections.slice(0, 2); + // Do NOT silently truncate: which physical server each alias + // points at decides where a migration writes, so an ambiguous + // selection must stop the run rather than pick two arbitrarily. + const tooMany = connections.length > MAX_SERVERS; + const beamConns = connections.slice(0, MAX_SERVERS); const aliases = beamAliasesForCount(beamConns.length); const beam = beamConns.map((c, i) => ({ alias: aliases[i]!, @@ -1094,12 +1098,25 @@ export const useSqlEditorStore = create()( password: sessionPasswords[c.id] || undefined, })); const primary = beamConns[0]; - if (!primary) { + if (tooMany) { + appendWarning( + `Server Beam uses at most ${MAX_SERVERS} Destinations, but ${connections.length} are checked. ` + + 'Uncheck the extras so source and target are unambiguous, then re-run.' + ); + } else if (!primary) { appendWarning( 'Server Beam needs at least one Destination checked (source). ' + 'For cross-server copy, check two — first is source, second is target.' ); } else { + // Say out loud which server each alias resolved to. `sql.on('target')` + // writing to the wrong database is the failure this feature can + // cause, and alias order comes from the list, not the click order. + appendWarning( + `Server Beam → ${beam + .map((b, i) => `${b.alias} = ${beamConns[i]!.name}`) + .join(', ')}` + ); const prev = resultsByConn.get(primary.id) ?? []; try { const { result, directives } = await runCodeCell({ diff --git a/apps/web/src/shared/server-beam.test.ts b/apps/web/src/shared/server-beam.test.ts index ef8f776d..c01f2a49 100644 --- a/apps/web/src/shared/server-beam.test.ts +++ b/apps/web/src/shared/server-beam.test.ts @@ -41,3 +41,26 @@ describe('server-beam', () => { expect(beamAliasesForCount(2)).toEqual(['source', 'target']); }); }); + +describe('alias lookup must not accept inherited Object keys', () => { + // The worker resolves `sql.on(alias)` against a plain object of alias→dialect. + // A truthiness check lets `toString` / `constructor` / `valueOf` / `__proto__` + // through, and the inherited *function* then gets used as the dialect — + // surfacing as "dialect.toLowerCase is not a function" instead of a clear + // "unknown alias". hasOwn is the only correct membership test here. + const beamDialects: Record = { source: 'postgres', target: 'mysql' }; + + it.each(['toString', 'constructor', 'valueOf', 'hasOwnProperty', '__proto__'])( + 'rejects inherited key %s', + (key) => { + expect(Boolean(beamDialects[key])).toBe(true); // the trap + expect(Object.hasOwn(beamDialects, key)).toBe(false); // the fix + } + ); + + it('still accepts real aliases and rejects genuinely unknown ones', () => { + expect(Object.hasOwn(beamDialects, 'source')).toBe(true); + expect(Object.hasOwn(beamDialects, 'target')).toBe(true); + expect(Object.hasOwn(beamDialects, 'nope')).toBe(false); + }); +}); diff --git a/apps/web/src/shared/server-beam.ts b/apps/web/src/shared/server-beam.ts index 0a5b11a1..a2c62572 100644 --- a/apps/web/src/shared/server-beam.ts +++ b/apps/web/src/shared/server-beam.ts @@ -41,7 +41,7 @@ export function parseBeamEndpoints( if (raw.length > MAX_SERVERS) { return { ok: false, - error: `Server Beam cant handle more than ${MAX_SERVERS} aliases`, + error: `Server Beam handles at most ${MAX_SERVERS} aliases (source, target)`, }; } const out: BeamEndpointRef[] = [];