diff --git a/packages/sql/src/modules/migrations/sql-generator.module.test.ts b/packages/sql/src/modules/migrations/sql-generator.module.test.ts index 056801f94..b60aef7bd 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.test.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.test.ts @@ -1626,3 +1626,43 @@ describe('SQLite views: no CREATE OR REPLACE, no doubled header', () => { expect(sql).toMatch(/CREATE OR REPLACE VIEW v_customers AS/i); }); }); + +describe('trigger definitions are terminated', () => { + // SQLite's sqlite_master.sql ends a trigger at `END`, with no `;`. Emitted as + // it stands, the CREATE TRIGGER never ends and swallows the next statement. + const body = `CREATE TRIGGER trg_decrement_stock +AFTER INSERT ON order_items +FOR EACH ROW +BEGIN + UPDATE products SET stock = stock - NEW.qty WHERE id = NEW.product_id; +END`; + + const withTrigger = (): TableDiff => ({ + tableName: 'ORDER_ITEMS', + objectType: 'TABLE', + status: 'ADDED', + columnDiffs: [], + indexDiffs: [], + foreignKeyDiffs: [], + sourceTable: tableSchema({ + name: 'order_items', + columns: [{ name: 'qty', type: 'INTEGER', nullable: false, primaryKey: false }], + triggers: [{ name: 'trg_decrement_stock', timing: 'AFTER', event: 'INSERT', definition: body }], + }), + }); + + it('terminates a catalog definition that ends at END', () => { + const sql = gen.generateMigrationSql([withTrigger()], 'sqlite'); + expect(sql).toMatch(/END;/); + // nothing may follow END on the same statement without a terminator + expect(sql).not.toMatch(/END[ \t]*\n[ \t]*\n?[ \t]*(ALTER|CREATE|DROP)\b/i); + }); + + it('does not double the terminator when one is already there', () => { + const diff = withTrigger(); + diff.sourceTable!.triggers![0]!.definition = `${body};`; + const sql = gen.generateMigrationSql([diff], 'sqlite'); + expect(sql).toMatch(/END;/); + expect(sql).not.toMatch(/END;;/); + }); +}); diff --git a/packages/sql/src/modules/migrations/sql-generator.module.ts b/packages/sql/src/modules/migrations/sql-generator.module.ts index 2113fea9b..6e3d058ab 100644 --- a/packages/sql/src/modules/migrations/sql-generator.module.ts +++ b/packages/sql/src/modules/migrations/sql-generator.module.ts @@ -480,8 +480,11 @@ export class SqlGeneratorModule { } for (const trg of table.triggers ?? []) { + // ensureSemicolon, not a bare trim: a catalog definition can end at `END` + // with no terminator (SQLite's sqlite_master.sql does), and an + // unterminated CREATE TRIGGER swallows whatever statement follows it. sql += trg.definition - ? `${trg.definition.trim()}\n` + ? `${this.ensureSemicolon(trg.definition)}\n` : `-- Trigger ${trg.name} (${trg.timing ?? ''} ${trg.event ?? ''}) has no available definition\n`; } @@ -591,7 +594,10 @@ export class SqlGeneratorModule { for (const trg of source.triggers ?? []) { if (!trg.definition) continue; const stmt = dialect.createTriggerStatement?.(trg, name) ?? null; - statements.push(stmt ?? trg.definition.trim()); + // See renderCreateTable: an unterminated CREATE TRIGGER swallows the + // next statement. The dialect hooks already end in `;`; this is a no-op + // for them and supplies the terminator for a raw catalog definition. + statements.push(this.ensureSemicolon(stmt ?? trg.definition)); } } else if (obj.objectType === 'SEQUENCE' && source) { statements.push(this.renderCreateSequence({ ...source, name }, dialect)); @@ -946,7 +952,7 @@ export class SqlGeneratorModule { if (!trg.source?.definition) continue; const trgSpec = { name: trg.source.name, timing: trg.source.timing, event: trg.source.event, definition: trg.source.definition }; const stmt = dialect.createTriggerStatement?.(trgSpec, tableName) ?? null; - statements.push(stmt ?? trg.source.definition.trim()); + statements.push(this.ensureSemicolon(stmt ?? trg.source.definition)); } // Recreate any views dropped by the dialect's dropDependentViewsBlock above. diff --git a/packages/sql/src/modules/sql-text/sql-splitter.test.ts b/packages/sql/src/modules/sql-text/sql-splitter.test.ts index 4f96bac4f..79d47aebf 100644 --- a/packages/sql/src/modules/sql-text/sql-splitter.test.ts +++ b/packages/sql/src/modules/sql-text/sql-splitter.test.ts @@ -1109,3 +1109,47 @@ describe('splitting on the shared tokenizer', () => { expect(second).toMatchObject({ startLine: 3, endLine: 3 }); }); }); + +describe('splitSqlStatements: a `.` in a comment does not qualify the next identifier', () => { + // Found by running a real SQLite migration. The generator writes review notes + // above the statements they concern, e.g. + // -- review: skip FK … declare FOREIGN KEY in CREATE TABLE only (no ALTER ADD). + // Qualification used to be decided by scanning backwards over whitespace for a + // `.`, which cannot tell code from comment text. The note's trailing period made + // the following `CREATE` look like `schema.CREATE`, so it was skipped, the + // statement never registered as routine DDL, and the trigger body split at the + // `;` inside BEGIN … END — leaving a fragment starting with `END`. + const trigger = `CREATE TRIGGER trg_decrement_stock +AFTER INSERT ON order_items +FOR EACH ROW +BEGIN + UPDATE products SET stock = stock - NEW.qty WHERE id = NEW.product_id; +END;`; + + it('keeps a trigger whole after a comment ending in a period', () => { + const sql = `-- review: declare FOREIGN KEY in CREATE TABLE only (no ALTER ADD).\n${trigger}`; + const stmts = splitSqlStatements(sql); + expect(stmts).toHaveLength(1); + expect(stmts[0]!.text).toContain('UPDATE products'); + expect(stmts.some((s) => /^\s*END\b/i.test(s.text))).toBe(false); + }); + + it.each(['--.', '-- .', '--a.', '-- ends with a period.'])( + 'keeps a trigger whole after the comment %j', + (comment) => { + expect(splitSqlStatements(`${comment}\n${trigger}`)).toHaveLength(1); + } + ); + + it('still treats a genuinely qualified identifier as qualified', () => { + // `s.begin` / `s.end` are column names, not block delimiters: if they were + // read as a BEGIN opener the `;` would stop splitting. + expect(splitSqlStatements('SELECT s.begin FROM s;\nSELECT 1;')).toHaveLength(2); + expect(splitSqlStatements('SELECT s.end FROM s;\nSELECT 1;')).toHaveLength(2); + }); + + it('reads a qualifier across whitespace and comments', () => { + expect(splitSqlStatements('SELECT s . begin FROM s;\nSELECT 1;')).toHaveLength(2); + expect(splitSqlStatements('SELECT s /* c */ . begin FROM s;\nSELECT 1;')).toHaveLength(2); + }); +}); diff --git a/packages/sql/src/modules/sql-text/sql-splitter.ts b/packages/sql/src/modules/sql-text/sql-splitter.ts index 790013f82..a61dff6d1 100644 --- a/packages/sql/src/modules/sql-text/sql-splitter.ts +++ b/packages/sql/src/modules/sql-text/sql-splitter.ts @@ -361,12 +361,6 @@ const TRANSACTION_BEGIN = new Set([ /** Block frames pushed while scanning routine DDL bodies. */ type RoutineBlock = 'begin' | 'case' | 'try' | 'catch'; -function identIsQualified(sql: string, identStart: number): boolean { - let k = identStart - 1; - while (k >= 0 && /\s/.test(sql[k]!)) k--; - return k >= 0 && sql[k] === '.'; -} - /** Split a SQL-only buffer into `;`-terminated statements (no code fences). */ function splitSqlOnly(sql: string): SplitStatement[] { const out: SplitStatement[] = []; @@ -396,6 +390,19 @@ function splitSqlOnly(sql: string): SplitStatement[] { let blockStack: RoutineBlock[] = []; /** True at the start of a procedural statement (`BEGIN` body / after `;`). */ let atProcStmtStart = false; + /** + * The previous code token was `.`, so the identifier after it is qualified — + * `s.begin` is a column name, not a block opener. + * + * Read from the token stream rather than by scanning the raw text backwards. + * A backward scan that skips only whitespace cannot tell a `.` that is code + * from one that merely ends a comment, so a review note like + * `-- … in CREATE TABLE only (no ALTER ADD).` sitting above a trigger made + * its `CREATE` look qualified. `CREATE` was then skipped, the statement never + * registered as routine DDL, and the trigger body split at the first `;` + * inside `BEGIN … END`. + */ + let prevTokenWasDot = false; /** Swallow the next ident after `END IF` / `END CASE` so it is not an opener. */ let skipNextWord: string | null = null; @@ -452,7 +459,7 @@ function splitSqlOnly(sql: string): SplitStatement[] { const onWord = (wordStart: number, wordEnd: number) => { const raw = sql.slice(wordStart, wordEnd).toUpperCase(); const word = raw === 'PROC' ? 'PROCEDURE' : raw; - if (identIsQualified(sql, wordStart)) { + if (prevTokenWasDot) { if (inRoutineDdl) atProcStmtStart = false; return; } @@ -546,6 +553,10 @@ function splitSqlOnly(sql: string): SplitStatement[] { } else if (token.kind === 'word' && /[A-Za-z_]/.test(sql[token.start]!)) { onWord(token.start, token.end); } + // After onWord, so it describes the token before the next identifier. + // Comments and whitespace do not clear it: `a . b` and `a /* c */ . b` are + // both qualified, while a `.` that only ends a comment never gets here. + if (!isTrivia(token)) prevTokenWasDot = token.kind === 'punct' && token.upper === '.'; for (let k = token.start; k < token.end; k++) if (sql.charCodeAt(k) === 10) line++; } flush(len, false);