diff --git a/packages/sql/src/index.ts b/packages/sql/src/index.ts index e3c127f..ab8b684 100644 --- a/packages/sql/src/index.ts +++ b/packages/sql/src/index.ts @@ -59,8 +59,10 @@ export { assembleBlueprint, blueprintChildCounts, countSourceLines, + isLokeeContainerType, objectKeyKind, objectKeyOwner, + pickOwnerContainer, hydrateTableSchemas, buildRevertMigration, } from './modules/lokee-weave/index.js'; diff --git a/packages/sql/src/modules/lokee-weave/blueprint.test.ts b/packages/sql/src/modules/lokee-weave/blueprint.test.ts index 8696264..d395668 100644 --- a/packages/sql/src/modules/lokee-weave/blueprint.test.ts +++ b/packages/sql/src/modules/lokee-weave/blueprint.test.ts @@ -12,6 +12,7 @@ import { countSourceLines, objectKeyKind, objectKeyOwner, + pickOwnerContainer, type StoredWeaveObject, } from './blueprint.js'; @@ -28,6 +29,23 @@ describe('objectKeyOwner / objectKeyKind', () => { }); }); +describe('pickOwnerContainer', () => { + it('prefers table:OWNER over a child trigger that shares the owner', () => { + const group = [ + { key: 'trigger:CUSTOMER.TRG_AUDIT', type: 'trigger' }, + { key: 'column:CUSTOMER.ID', type: 'column' }, + { key: 'table:CUSTOMER', type: 'table' }, + ]; + expect(pickOwnerContainer(group)?.key).toBe('table:CUSTOMER'); + }); + + it('still selects a standalone trigger container', () => { + expect(pickOwnerContainer([{ key: 'trigger:TRG_AUDIT', type: 'trigger' }])?.key).toBe( + 'trigger:TRG_AUDIT' + ); + }); +}); + describe('countSourceLines', () => { it('counts physical lines, not collapsed whitespace', () => { expect(countSourceLines(undefined)).toBe(0); diff --git a/packages/sql/src/modules/lokee-weave/blueprint.ts b/packages/sql/src/modules/lokee-weave/blueprint.ts index b2c5b4f..8055ab3 100644 --- a/packages/sql/src/modules/lokee-weave/blueprint.ts +++ b/packages/sql/src/modules/lokee-weave/blueprint.ts @@ -94,6 +94,27 @@ function containerKeyForOwner( return null; } +/** + * Pick the owner-level container from a same-owner object group. + * + * Table-owned triggers are also typed `trigger` and share the table's owner + * (`trigger:CUSTOMER.TRG` → owner `CUSTOMER`). A naïve + * `find(isLokeeContainerType)` can therefore treat the child trigger as the + * table when it appears first in the group — hydrate then emits wrong DDL + * (e.g. `ALTER TABLE trg …` instead of `customer`). Prefer the same ordered + * `type:OWNER` keys {@link assembleBlueprint} uses. + */ +export function pickOwnerContainer(objects: readonly T[]): T | undefined { + if (objects.length === 0) return undefined; + const owner = objectKeyOwner(objects[0]!.key); + const byKey = new Map(objects.map((object) => [object.key, object])); + for (const type of CONTAINER_TYPES) { + const hit = byKey.get(`${type}:${owner}`); + if (hit) return hit; + } + return undefined; +} + export interface ObjectBlueprint { focusKey: string; /** Table / view / routine the focus belongs to. */ diff --git a/packages/sql/src/modules/lokee-weave/hydrate.test.ts b/packages/sql/src/modules/lokee-weave/hydrate.test.ts index d44587d..6d6b6ae 100644 --- a/packages/sql/src/modules/lokee-weave/hydrate.test.ts +++ b/packages/sql/src/modules/lokee-weave/hydrate.test.ts @@ -71,6 +71,22 @@ describe('hydrateTableSchemas', () => { expect(hydrateTableSchemas(objects)).toEqual([]); }); + it('does not treat a table-owned trigger as the table container', () => { + // objectsAtVersion inserts by hash order; a child trigger can appear before + // `table:CUSTOMER`. Both are container types — picking the trigger would + // emit ALTER TABLE trg_audit … instead of customer. + const objects = canonicalizeObject(customer()); + const trigger = objects.find((o) => o.type === 'trigger'); + expect(trigger).toBeTruthy(); + const rest = objects.filter((o) => o.type !== 'trigger'); + const hydrated = hydrateTableSchemas([trigger!, ...rest]); + expect(hydrated).toHaveLength(1); + expect(hydrated[0]!.name).toBe('customer'); + expect(hydrated[0]!.objectType).toBe('TABLE'); + expect(hydrated[0]!.columns.map((c) => c.name)).toEqual(['id', 'email']); + expect(hydrated[0]!.triggers?.[0]?.name).toBe('trg_audit'); + }); + it('keeps two tables independent after a whole-schema round trip', () => { const other: TableSchema = { name: 'orders', diff --git a/packages/sql/src/modules/lokee-weave/hydrate.ts b/packages/sql/src/modules/lokee-weave/hydrate.ts index e5665df..214c942 100644 --- a/packages/sql/src/modules/lokee-weave/hydrate.ts +++ b/packages/sql/src/modules/lokee-weave/hydrate.ts @@ -19,7 +19,7 @@ import type { TriggerInfo, } from '../../interfaces/schema.interface.js'; import type { CanonicalObject } from './canonical.js'; -import { isLokeeContainerType, objectKeyOwner } from './blueprint.js'; +import { objectKeyOwner, pickOwnerContainer } from './blueprint.js'; function asString(value: unknown): string | undefined { return typeof value === 'string' && value.length > 0 ? value : undefined; @@ -107,7 +107,9 @@ export function hydrateTableSchemas(objects: readonly CanonicalObject[]): TableS const tables: TableSchema[] = []; for (const group of byOwner.values()) { - const container = group.find((item) => isLokeeContainerType(item.type)); + // Prefer `table:OWNER` over a child `trigger:OWNER.NAME` that shares the + // owner — both are container types, and Map/hash load order is unstable. + const container = pickOwnerContainer(group); if (!container) continue; const pk = group.find((item) => item.type === 'primary_key'); diff --git a/packages/sql/src/modules/lokee-weave/index.ts b/packages/sql/src/modules/lokee-weave/index.ts index 458d4fc..afba238 100644 --- a/packages/sql/src/modules/lokee-weave/index.ts +++ b/packages/sql/src/modules/lokee-weave/index.ts @@ -63,8 +63,10 @@ export { assembleBlueprint, blueprintChildCounts, countSourceLines, + isLokeeContainerType, objectKeyKind, objectKeyOwner, + pickOwnerContainer, type ObjectBlueprint, type StoredWeaveObject, } from './blueprint.js'; diff --git a/packages/sql/src/modules/sql-splitter.test.ts b/packages/sql/src/modules/sql-splitter.test.ts index c53a4d7..d6939ea 100644 --- a/packages/sql/src/modules/sql-splitter.test.ts +++ b/packages/sql/src/modules/sql-splitter.test.ts @@ -306,6 +306,66 @@ BEGIN END;`; expect(splitSqlStatements(sql)).toHaveLength(1); }); + + it('does not treat bare END identifiers as block closers (AS end / SELECT end)', () => { + // A depth counter that decrements on every END used to split after + // `AS end`, turning the following DELETE into a top-level write cell. + const sql = `CREATE PROCEDURE p() +BEGIN + SELECT created_at AS start, updated_at AS end FROM events; + DELETE FROM victims WHERE id = 1; +END; +SELECT 9;`; + const stmts = splitSqlStatements(sql); + expect(stmts).toHaveLength(2); + expect(stmts[0]!.text).toMatch(/CREATE PROCEDURE/i); + expect(stmts[0]!.text).toContain('DELETE FROM victims'); + expect(stmts[1]!.text.trim()).toBe('SELECT 9;'); + expect(stmts.some((s) => /^\s*DELETE FROM victims/i.test(s.text))).toBe(false); + }); + + it('does not split on DECLARE end / SELECT end inside the body', () => { + const sql = `CREATE PROCEDURE p() +BEGIN + DECLARE end INT; + SELECT end FROM some_table; + DELETE FROM victims; +END;`; + const stmts = splitSqlStatements(sql); + expect(stmts).toHaveLength(1); + expect(stmts[0]!.text).toContain('DELETE FROM victims'); + }); + + it('keeps CASE expressions that end with END AS … inside one cell', () => { + const sql = `CREATE PROCEDURE p() +BEGIN + SELECT CASE WHEN 1=1 THEN 1 ELSE 0 END AS flag FROM t; + UPDATE t SET x = 1; +END; +SELECT 2;`; + const stmts = splitSqlStatements(sql); + expect(stmts).toHaveLength(2); + expect(stmts[0]!.text).toContain('UPDATE t SET x = 1'); + expect(stmts[1]!.text.trim()).toBe('SELECT 2;'); + }); + + it('keeps SQL Server BEGIN TRY / CATCH inside one cell and splits after', () => { + const sql = `CREATE PROCEDURE p AS +BEGIN + BEGIN TRY + UPDATE t SET x = 1; + END TRY + BEGIN CATCH + SELECT 1; + END CATCH +END; +DROP TABLE victims;`; + const stmts = splitSqlStatements(sql); + expect(stmts).toHaveLength(2); + expect(stmts[0]!.text).toMatch(/CREATE PROCEDURE/i); + expect(stmts[0]!.text).toContain('END CATCH'); + expect(stmts[1]!.text.trim()).toBe('DROP TABLE victims;'); + }); }); describe('checkStatement', () => { diff --git a/packages/sql/src/modules/sql-splitter.ts b/packages/sql/src/modules/sql-splitter.ts index 337b8ff..deb49d4 100644 --- a/packages/sql/src/modules/sql-splitter.ts +++ b/packages/sql/src/modules/sql-splitter.ts @@ -334,9 +334,15 @@ const NOT_ROUTINE_OBJECT = new Set([ 'SUBSCRIPTION', 'SERVER', 'FOREIGN', 'FULLTEXT', 'SPATIAL', 'COLUMN', 'CONSTRAINT', 'UNIQUE', ]); -/** `END IF` / `END LOOP` / … close a compound statement, not `BEGIN`. */ +/** + * `END IF` / `END LOOP` / … close an IF/LOOP/… compound, not `BEGIN`. + * `TRY` / `CATCH` pair with `BEGIN TRY` / `BEGIN CATCH` (SQL Server). + */ const END_COMPOUND = new Set(['IF', 'LOOP', 'WHILE', 'REPEAT', 'TRY', 'CATCH']); +/** Block frames pushed while scanning routine DDL bodies. */ +type RoutineBlock = 'begin' | 'case' | 'try' | 'catch'; + /** Skip whitespace and SQL comments; return the next non-trivia index. */ function skipSqlTrivia(sql: string, start: number): number { let i = start; @@ -406,8 +412,14 @@ function splitSqlOnly(sql: string): SplitStatement[] { let lookingForRoutine = false; /** CREATE/ALTER FUNCTION|PROCEDURE|TRIGGER — inner `;` do not split. */ let inRoutineDdl = false; - /** BEGIN/CASE nesting while `inRoutineDdl`. */ - let blockDepth = 0; + /** + * Nesting stack while `inRoutineDdl`. A bare counter treated every `END` + * (including `AS end` / `SELECT end`) as a closer and split the body so + * later UPDATE/DELETE cells ran as top-level SQL — see routine DDL tests. + */ + let blockStack: RoutineBlock[] = []; + /** True at the start of a procedural statement (`BEGIN` body / after `;`). */ + let atProcStmtStart = false; /** Swallow the next ident after `END IF` / `END CASE` so it is not an opener. */ let skipNextWord: string | null = null; @@ -416,7 +428,8 @@ function splitSqlOnly(sql: string): SplitStatement[] { stmtVerb = null; lookingForRoutine = false; inRoutineDdl = false; - blockDepth = 0; + blockStack = []; + atProcStmtStart = false; skipNextWord = null; }; @@ -472,7 +485,8 @@ function splitSqlOnly(sql: string): SplitStatement[] { markCode(i); // Routine bodies (CREATE PROCEDURE … BEGIN … END) contain many `;` // that must not become extra editor cells. - if (inRoutineDdl && blockDepth > 0) { + if (inRoutineDdl && blockStack.length > 0) { + atProcStmtStart = true; i += 1; continue; } @@ -487,6 +501,9 @@ function splitSqlOnly(sql: string): SplitStatement[] { const word = ident.word === 'PROC' ? 'PROCEDURE' : ident.word; if (!identIsQualified(sql, i)) { if (skipNextWord && word === skipNextWord) { + // Keep `atProcStmtStart` from the opener/closer that armed the + // skip (`BEGIN TRY` → still at stmt start; `END TRY` → next + // `END` may close the outer `BEGIN`). skipNextWord = null; i = ident.end; continue; @@ -506,23 +523,61 @@ function splitSqlOnly(sql: string): SplitStatement[] { if (inRoutineDdl) { if (word === 'BEGIN') { const nxt = peekSqlKeyword(sql, ident.end); - if (nxt !== 'TRAN' && nxt !== 'TRANSACTION' && nxt !== 'WORK') { - blockDepth++; + if (nxt === 'TRAN' || nxt === 'TRANSACTION' || nxt === 'WORK') { + // BEGIN TRAN — not a nesting frame. + } else if (nxt === 'TRY') { + blockStack.push('try'); + skipNextWord = 'TRY'; + atProcStmtStart = true; + } else if (nxt === 'CATCH') { + blockStack.push('catch'); + skipNextWord = 'CATCH'; + atProcStmtStart = true; + } else { + blockStack.push('begin'); + atProcStmtStart = true; } } else if (word === 'CASE') { - blockDepth++; + blockStack.push('case'); + atProcStmtStart = false; } else if (word === 'END') { const nxt = peekSqlKeyword(sql, ident.end); if (nxt && END_COMPOUND.has(nxt)) { skipNextWord = nxt; + // END TRY / END CATCH close the matching BEGIN TRY/CATCH. + // END IF / LOOP / … do not pop BEGIN (IF never pushed). + if (nxt === 'TRY' || nxt === 'CATCH') { + const want = nxt === 'TRY' ? 'try' : 'catch'; + if (blockStack[blockStack.length - 1] === want) blockStack.pop(); + } + // Compound `END …` is its own statement; a following bare + // `END` may close the enclosing `BEGIN`. + atProcStmtStart = true; } else if (nxt === 'CASE') { skipNextWord = 'CASE'; - blockDepth = Math.max(0, blockDepth - 1); + if (blockStack[blockStack.length - 1] === 'case') blockStack.pop(); + atProcStmtStart = true; + } else if (blockStack[blockStack.length - 1] === 'case') { + // CASE *expression* terminator (`… END`, `… END AS x`). + blockStack.pop(); + atProcStmtStart = false; + } else if ( + atProcStmtStart && + blockStack[blockStack.length - 1] === 'begin' + ) { + // Only a statement-leading `END` closes `BEGIN` — not + // `SELECT end`, `AS end`, or `DECLARE end INT`. + blockStack.pop(); + atProcStmtStart = false; } else { - blockDepth = Math.max(0, blockDepth - 1); + atProcStmtStart = false; } + } else { + atProcStmtStart = false; } } + } else if (inRoutineDdl) { + atProcStmtStart = false; } i = ident.end; continue;