From 722eab28ce55365ca2d538ba818f3bd27696c57b Mon Sep 17 00:00:00 2001 From: huyplb Date: Sun, 16 Aug 2026 23:06:53 -0600 Subject: [PATCH] fix(oracle): three bugs found by pointing the same harness at Oracle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same treatment DB2 got: the live DDL suite plus the shipped DEMO_A/DEMO_B samples, migrated into a throwaway schema and re-compared. Oracle passed every CREATE, ALTER and routine round trip already; the samples found three more. **`NO CYCLE` / `NO CACHE` are ORA-03049 on Oracle.** It spells them `NOCYCLE` and `NOCACHE`, one word. The spaced form Postgres and DB2 accept killed the CREATE SEQUENCE, and everything that leaned on it went down too: the table whose column default calls the sequence (ORA-02289), then the views over that table. Now behind `unspacedSequenceNoKeywords`, applied to both the create and alter paths. **Tables were created before the sequences they default to.** `sortAddedByDependency` only understands foreign keys, so `DEFAULT order_seq.NEXTVAL` was not a dependency it could see. Sequences, types and roles now go ahead of tables — none of them can depend on a table, so first is always safe. **Index and trigger drops were the only intolerant ones.** The dialect already has `oracleDrop` (version-aware: `IF EXISTS` on 23+, a SQLCODE guard below) and uses it for TABLE, VIEW, SEQUENCE, FUNCTION and PROCEDURE — but DROP INDEX and DROP TRIGGER were emitted bare, so ORA-01418 / ORA-04080 on an object that had already gone with its table failed the step. Also: a function-based index reads as its hidden `SYS_NC00006$` placeholder in ALL_IND_COLUMNS (the expression lives in ALL_IND_EXPRESSIONS, which the provider does not read). Emitting that name is ORA-00904, so the generator now skips it with `-- review:` instead of shipping DDL that cannot run. Capturing the real expression is the deeper fix and is not attempted here. End state on the Oracle samples: 18/18 steps execute and re-comparing after the migration reports **no differences**. All five utilities work, and DEMO_A is untouched. One environment note, not a code change: `docker/init/oracle/01_seed.sh` seeds `FREEPDB1` while compose provisions `ORACLE_DATABASE: FOXDB`, so the demo schemas land in a different PDB from the one the app config points at. The samples exist — just not where FOXDB is. Co-Authored-By: Claude Opus 5 --- .../modules/generated-ddl-live.test.ts | 26 ++++++++++++++++ .../sql/src/modules/sql-dialect.interface.ts | 9 ++++++ .../sql/src/modules/sql-generator.module.ts | 30 +++++++++++++++---- .../providers/oracle/oracle.sql-dialect.ts | 17 +++++++++-- 4 files changed, 75 insertions(+), 7 deletions(-) diff --git a/apps/web/src/backend/modules/generated-ddl-live.test.ts b/apps/web/src/backend/modules/generated-ddl-live.test.ts index 11dfaa4e..ca6c3af7 100644 --- a/apps/web/src/backend/modules/generated-ddl-live.test.ts +++ b/apps/web/src/backend/modules/generated-ddl-live.test.ts @@ -75,6 +75,14 @@ const TARGETS: Array<{ provider: 'yugabytedb', options: { host: 'localhost', port: 5433, database: 'yugabyte', username: 'yugabyte', schema: 'public' }, }, + { + // Oracle has no bare `SELECT 1` either — it wants a FROM, and DUAL is it. + // Schema and user are the same thing here, so the "schema" is the account. + dialect: 'oracle', + provider: 'oracle', + options: { host: 'localhost', port: 1521, database: 'FOXDB', username: 'foxuser', password: 'foxpass', schema: 'FOXUSER' }, + probe: 'SELECT 1 FROM DUAL', + }, { // Slowest of the set to boot (the compose healthcheck allows two minutes // before it even starts probing) and the only one needing a native client @@ -295,6 +303,24 @@ const ROUTINES: Record< `CREATE PROCEDURE dbo.touch_it_${TAG} AS BEGIN SELECT 1 END`, ], }, + oracle: { + // An Oracle schema *is* a user, so the round trip needs a second account — + // which only a DBA can create, hence the admin credentials. + from: 'FOXUSER', + to: `FXRT${TAG.toUpperCase()}`, + admin: { username: 'system', password: 'FoxPass123' }, + makeSchema: (s) => + s === 'FOXUSER' + ? [] + : [ + `CREATE USER ${s} IDENTIFIED BY foxpass QUOTA UNLIMITED ON USERS`, + `GRANT CREATE SESSION, CREATE PROCEDURE TO ${s}`, + ], + ddl: (s) => [ + `CREATE FUNCTION ${s}.DOUBLE_IT(X IN NUMBER) RETURN NUMBER AS BEGIN RETURN X * 2; END;`, + `CREATE PROCEDURE ${s}.TOUCH_IT AS BEGIN NULL; END;`, + ], + }, db2: { from: `FXA${TAG}`, to: `FXB${TAG}`, diff --git a/packages/sql/src/modules/sql-dialect.interface.ts b/packages/sql/src/modules/sql-dialect.interface.ts index 8a865ff3..b2d82ace 100644 --- a/packages/sql/src/modules/sql-dialect.interface.ts +++ b/packages/sql/src/modules/sql-dialect.interface.ts @@ -256,6 +256,15 @@ export interface SqlDialect { */ wrapCreateSequence?(qualifiedName: string, createSql: string): string; + /** + * Oracle spells the negative sequence options as single keywords — + * `NOCYCLE`, `NOCACHE`. The spaced `NO CYCLE` / `NO CACHE` that Postgres and + * DB2 accept is **ORA-03049** there, which kills the CREATE SEQUENCE and then + * everything that depends on it: the table whose default calls the sequence, + * and the views over that table. + */ + unspacedSequenceNoKeywords?: boolean; + /** * Wrap the standard `ALTER SEQUENCE name ...;` into a dialect-safe form. * Called with the qualified name and the full `ALTER SEQUENCE name ...;` string. diff --git a/packages/sql/src/modules/sql-generator.module.ts b/packages/sql/src/modules/sql-generator.module.ts index 2bd017fe..67ad209e 100644 --- a/packages/sql/src/modules/sql-generator.module.ts +++ b/packages/sql/src/modules/sql-generator.module.ts @@ -349,6 +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 { + // Oracle keeps a function-based index's expression in ALL_IND_EXPRESSIONS + // and puts a hidden `SYS_NC00006$` placeholder in ALL_IND_COLUMNS, which is + // what introspection reads. Emitting that name is ORA-00904 ("invalid + // identifier") — the index cannot be built from what we captured, so say so + // rather than shipping DDL that cannot run. + const hidden = idx.columns.find((c) => /^SYS_NC\d+\$$/i.test((c ?? '').trim())); + if (hidden) { + return `-- review: skip index ${idx.name} on ${qualifiedTable} — function-based index; its expression is not captured (column reads as ${hidden})`; + } // 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 @@ -405,8 +414,11 @@ export class SqlGeneratorModule { if (s.increment !== undefined) opts += ` INCREMENT BY ${s.increment}`; if (s.minValue !== undefined) opts += ` MINVALUE ${s.minValue}`; if (s.maxValue !== undefined) opts += ` MAXVALUE ${s.maxValue}`; - opts += s.cycle ? ` CYCLE` : ` NO CYCLE`; - if (s.cache !== undefined) opts += s.cache > 0 ? ` CACHE ${s.cache}` : ` NO CACHE`; + // `NO CYCLE` on most engines, `NOCYCLE` on Oracle — the spaced form is + // ORA-03049 there, and it takes the dependent table and views down with it. + const no = dialect?.unspacedSequenceNoKeywords ? 'NO' : 'NO '; + opts += s.cycle ? ` CYCLE` : ` ${no}CYCLE`; + if (s.cache !== undefined) opts += s.cache > 0 ? ` CACHE ${s.cache}` : ` ${no}CACHE`; const createSql = `CREATE SEQUENCE ${name}${opts};`; return dialect?.wrapCreateSequence?.(name, createSql) ?? `CREATE SEQUENCE IF NOT EXISTS ${name}${opts};`; } @@ -935,8 +947,9 @@ export class SqlGeneratorModule { if (s.increment !== undefined) alter += ` INCREMENT BY ${s.increment}`; if (s.minValue !== undefined) alter += ` MINVALUE ${s.minValue}`; if (s.maxValue !== undefined) alter += ` MAXVALUE ${s.maxValue}`; - alter += s.cycle ? ` CYCLE` : ` NO CYCLE`; - if (s.cache !== undefined) alter += s.cache > 0 ? ` CACHE ${s.cache}` : ` NO CACHE`; + const noAlter = dialect?.unspacedSequenceNoKeywords ? 'NO' : 'NO '; + alter += s.cycle ? ` CYCLE` : ` ${noAlter}CYCLE`; + if (s.cache !== undefined) alter += s.cache > 0 ? ` CACHE ${s.cache}` : ` ${noAlter}CACHE`; const alterSql = alter + `;`; statements.push(dialect.wrapAlterSequence?.(tableName, alterSql) ?? alterSql); } else if (obj.objectType === 'TYPE' && obj.sourceTable) { @@ -1144,7 +1157,14 @@ export class SqlGeneratorModule { // Structural ADDED objects (TABLE, SEQUENCE, TYPE, ROLE) come before MODIFIED so // that new tables can be referenced by FK constraints added in ALTER steps. const addedStructural = diffs.filter((d) => d.status === 'ADDED' && !PROCEDURAL_TYPES.has(d.objectType)); - for (const obj of this.sortAddedByDependency(addedStructural)) { + // Sequences, types and roles ahead of tables. `sortAddedByDependency` only + // knows about foreign keys, so a table whose column default calls a + // sequence (`DEFAULT order_seq.NEXTVAL`) was created before the sequence + // existed — ORA-02289 on Oracle, which then took out every view over that + // table. None of these can depend on a table, so first is always safe. + const supporting = addedStructural.filter((d) => d.objectType !== 'TABLE' && d.objectType !== 'MQT'); + const tables = addedStructural.filter((d) => d.objectType === 'TABLE' || d.objectType === 'MQT'); + for (const obj of [...supporting, ...this.sortAddedByDependency(tables)]) { const stmts = this.createObjectStatements(obj, dialect, m); steps.push({ objectName: obj.tableName, objectType: obj.objectType, action: 'CREATE', statements: stmts }); } diff --git a/packages/sql/src/providers/oracle/oracle.sql-dialect.ts b/packages/sql/src/providers/oracle/oracle.sql-dialect.ts index 5acd12f5..1c6e52fc 100644 --- a/packages/sql/src/providers/oracle/oracle.sql-dialect.ts +++ b/packages/sql/src/providers/oracle/oracle.sql-dialect.ts @@ -75,6 +75,12 @@ export const oracleSqlDialect: SqlDialect = { return c.identity ? ` GENERATED ${c.identityGeneration ?? 'ALWAYS'} AS IDENTITY` : ''; }, + // NOCYCLE / NOCACHE, not NO CYCLE / NO CACHE. Verified on Oracle 23 Free: the + // spaced form raises ORA-03049 ("SQL keyword 'NO' is not syntactically + // valid"), which failed the CREATE SEQUENCE and then cascaded — the table + // whose default calls the sequence (ORA-02289) and the views over it. + unspacedSequenceNoKeywords: true, + // Oracle can't RESTART a sequence portably (RESTART START WITH is 18c+; older has no // equivalent), so skip the clause rather than emit invalid SQL. alterSequenceRestart(): string { @@ -130,13 +136,20 @@ export const oracleSqlDialect: SqlDialect = { dropIndexStatement(indexName: string, qualifiedTable: string): string { const dot = qualifiedTable.indexOf('.'); const prefix = dot >= 0 ? qualifiedTable.slice(0, dot + 1) : ''; - return `DROP INDEX ${prefix}${indexName};`; + // Tolerant like the table/view/sequence drops above: an index can already + // be gone (dropped with its table, or never created because it is + // function-based and its expression was not captured), and ORA-01418 then + // failed the whole step. This hook gets no server version, so it always + // uses the SQLCODE guard, which is valid on every release. + return oracleDrop('INDEX', `${prefix}${indexName}`, -1418); }, dropTriggerStatement(triggerName: string, qualifiedTable: string): string { const dot = qualifiedTable.indexOf('.'); const prefix = dot >= 0 ? qualifiedTable.slice(0, dot + 1) : ''; - return `DROP TRIGGER ${prefix}${triggerName};`; + // Same tolerance as the index drop: a trigger goes away with its table, and + // ORA-04080 on an already-absent one failed the step for no good reason. + return oracleDrop('TRIGGER', `${prefix}${triggerName}`, -4080); }, createTriggerStatement(