diff --git a/.ai/wheels/cross-engine-compatibility.md b/.ai/wheels/cross-engine-compatibility.md index f472b8f43b..ee867aabc8 100644 --- a/.ai/wheels/cross-engine-compatibility.md +++ b/.ai/wheels/cross-engine-compatibility.md @@ -313,6 +313,14 @@ if (isMySQLFamily) { CockroachDB is in CI but marked as soft-fail — test failures are logged as warnings, not build failures. Controlled by `SOFT_FAIL_DBS` in `.github/workflows/tests.yml`. +### Oracle — Multi-Row INSERT and RETURNING Incompatibility + +Oracle 23 rejects `INSERT INTO t (cols) VALUES (?,?), (?,?), ...` (the SQL-standard table value constructor) when the JDBC driver also requests `RETURN_GENERATED_KEYS`. The Oracle JDBC driver translates `RETURN_GENERATED_KEYS` into a `RETURNING ROWID INTO` clause, and Oracle 23 does not permit `RETURNING` combined with multi-row VALUES. + +`OracleModel` overrides `$bulkInsertSQL()` to emit `INSERT ALL INTO t (cols) VALUES (...) INTO t (cols) VALUES (...) SELECT 1 FROM dual` — Oracle's idiomatic multi-row form, which avoids both the table value constructor and the RETURNING expansion. This is transparent to framework users; `insertAll()` works the same on Oracle as on other databases. + +If you write code that generates raw bulk-insert SQL for Oracle (or adds a new adapter), use `INSERT ALL ... SELECT 1 FROM dual` rather than multi-row VALUES. The canonical implementation is `vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc::$bulkInsertSQL`. + ## Testing Across Engines ### Local Test Procedure diff --git a/CHANGELOG.md b/CHANGELOG.md index eb88ef12c4..598480ce93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo ### Fixed +- `model.insertAll()` on Oracle no longer errors with `ORA: returning clause is not allowed with INSERT and Table Value Constructor` (and the related `ORA: no statement parsed` follow-on). The bulk-insert SQL was always emitted as the SQL-standard multi-row table value constructor — `INSERT INTO t (cols) VALUES (?,?), (?,?), ...` — which Oracle 23 rejects in combination with the JDBC driver's implicit `RETURN_GENERATED_KEYS` handling (the driver expands `RETURN_GENERATED_KEYS` into a `RETURNING ROWID` clause, and Oracle 23 disallows `RETURNING` paired with multi-row VALUES). Bulk-insert SQL generation moved off the model mixin (`vendor/wheels/model/bulk.cfc::$buildBulkInsertSQL`, removed) onto the database adapter (`$bulkInsertSQL` on `databaseAdapters/Base.cfc`, mirroring the existing `$upsertSQL` pattern), so adapters can override per-engine. `databaseAdapters/Oracle/OracleModel.cfc` overrides it to emit Oracle's idiomatic multi-row form — `INSERT ALL INTO t (cols) VALUES (...) INTO t (cols) VALUES (...) SELECT 1 FROM dual` — which neither uses the table value constructor nor triggers the RETURNING expansion. Non-Oracle adapters (MySQL, Postgres, SQLite, H2, SQL Server, CockroachDB) keep the standard multi-row VALUES shape unchanged. The migrator-rename "Closed statement" error in the same compat-matrix run is a separate Oracle JDBC lifecycle issue and remains tracked under the parent issue (#2745) - `addColumnOptionsSpec` now branches on `adapter.adapterName() == "MySQL"` for the `text` + non-empty default assertion, matching the existing `isPostgresFamily` carve-out. MySQL's `MySQLMigrator.optionsIncludeDefault` returns false for `text` / `mediumtext` / `longtext` / `float`, so the Abstract `addColumnOptions` short-circuits the entire DEFAULT clause for those types — emitting `NULL` rather than `DEFAULT ''`. The spec previously asserted `toInclude("DEFAULT")` unconditionally and failed on every MySQL leg of the compat matrix (lucee6/mysql, lucee7/mysql, boxlang/mysql). The MySQL adapter's `optionsIncludeDefault` doc-comment now also explains the legacy pre-8.0.13 TEXT/BLOB constraint that motivates the suppression and references the spec contract. Follow-up to #2661/#2669 - `CockroachDBModel` now overrides `$supportsAdvisoryLocks()` to return `false`, so the four `lockingSpec` `withAdvisoryLock` tests skip cleanly on CockroachDB instead of erroring with `CockroachDB does not support advisory locks.`. The PR that introduced the capability flag (#2670) claimed CockroachDB in its CHANGELOG entry but never added the override — CockroachDB inherits from `PostgreSQLModel`, which reports `true`, so the spec's `beforeEach` skip-guard never fired and the four specs proceeded to call `$acquireAdvisoryLock`, which the adapter throws from by design. Compat-matrix legs `lucee6/cockroachdb`, `lucee7/cockroachdb`, and `boxlang/cockroachdb` now report 4 skips where they previously reported 4 errors. No spec changes needed — the capability-flag layer added in #2670 already does the right thing once the flag is correct (#2743) - `wheels.middleware.Cors` now short-circuits unmatched `OPTIONS` preflight requests at the dispatch layer, preserving the legacy `set(allowCorsRequests=true)` contract under the new middleware pipeline. Previously, `$findMatchingRoute()` ran before middleware, so a preflight against a path that only declared `POST` (or any non-`OPTIONS` verb) 404'd with `Wheels.RouteNotFound` before the CORS middleware's preflight branch could fire — leaving the middleware strictly less capable than the 3.x global setting it was meant to replace and breaking cross-origin `POST`/`PUT`/`PATCH`/`DELETE` from configured browsers. `Dispatch.$request()` now checks for an `OPTIONS` verb plus a `wheels.middleware.Cors` instance in the global pipeline and, if both are present, runs the pipeline against a no-op core handler before route matching. Dispatch behavior for `OPTIONS` without CORS middleware (still 404s) and for non-`OPTIONS` verbs (still routed normally) is unchanged (#2703) diff --git a/vendor/wheels/databaseAdapters/Base.cfc b/vendor/wheels/databaseAdapters/Base.cfc index ce3179800c..d3c882b540 100755 --- a/vendor/wheels/databaseAdapters/Base.cfc +++ b/vendor/wheels/databaseAdapters/Base.cfc @@ -734,6 +734,59 @@ component output=false extends="wheels.Global"{ ); } + /** + * Generates a multi-row INSERT statement as an array compatible with `$querySetup()`. + * Default shape is `INSERT INTO ... VALUES (?,?), (?,?), ...` (SQL standard table value + * constructor) — used by every adapter except Oracle, which overrides this method to + * emit `INSERT ALL ... SELECT 1 FROM dual` because Oracle 23 rejects multi-row VALUES + * combined with the JDBC driver's implicit RETURNING (RETURN_GENERATED_KEYS) handling + * with `ORA: returning clause is not allowed with INSERT and Table Value Constructor`. + */ + public array function $bulkInsertSQL( + required string tableName, + required array columns, + required array validProperties, + required array records, + required numeric batchStart, + required numeric batchEnd, + required struct propertyInfo + ) { + local.sql = []; + + local.colList = ""; + for (local.col in arguments.columns) { + if (Len(local.colList)) { + local.colList &= ", "; + } + local.colList &= $quoteIdentifier(local.col); + } + + ArrayAppend(local.sql, "INSERT INTO #arguments.tableName# (#local.colList#) VALUES "); + + local.propCount = ArrayLen(arguments.validProperties); + for (local.r = arguments.batchStart; local.r <= arguments.batchEnd; local.r++) { + if (local.r > arguments.batchStart) { + ArrayAppend(local.sql, ", "); + } + ArrayAppend(local.sql, "("); + for (local.p = 1; local.p <= local.propCount; local.p++) { + if (local.p > 1) { + ArrayAppend(local.sql, ", "); + } + local.propName = arguments.validProperties[local.p]; + local.val = StructKeyExists(arguments.records[local.r], local.propName) ? arguments.records[local.r][local.propName] : ""; + ArrayAppend(local.sql, $buildBulkParam( + value = local.val, + propName = local.propName, + propertyInfo = arguments.propertyInfo + )); + } + ArrayAppend(local.sql, ")"); + } + + return local.sql; + } + /** * Generates database-specific UPSERT SQL as an array compatible with `$querySetup()`. * Base implementation throws an error — each adapter must override with its own syntax. diff --git a/vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc b/vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc index 42a6e4e6c1..95997770b2 100755 --- a/vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc +++ b/vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc @@ -176,6 +176,65 @@ component extends="wheels.databaseAdapters.Base" output=false { return """#UCase(arguments.name)#"""; } + /** + * Oracle bulk insert using `INSERT ALL INTO ... SELECT 1 FROM dual`. + * + * The default Base adapter shape — `INSERT INTO t (cols) VALUES (?,?), (?,?), ...` + * (SQL standard table value constructor) — was rejected on Oracle 23 with + * `ORA: returning clause is not allowed with INSERT and Table Value Constructor`. + * The CFML engine's `cfquery` for INSERT statements implicitly sets + * `Statement.RETURN_GENERATED_KEYS`, which the Oracle JDBC driver translates into a + * RETURNING clause — and Oracle 23 does not permit RETURNING with multi-row VALUES. + * + * `INSERT ALL` is the Oracle-idiomatic multi-row insert form, doesn't trigger the + * RETURNING-clause expansion, and works on every Oracle version Wheels targets. + * Uses parameterized values via `$buildBulkParam` — never interpolates user data + * into SQL. + */ + public array function $bulkInsertSQL( + required string tableName, + required array columns, + required array validProperties, + required array records, + required numeric batchStart, + required numeric batchEnd, + required struct propertyInfo + ) { + local.sql = []; + + local.colList = ""; + for (local.col in arguments.columns) { + if (Len(local.colList)) { + local.colList &= ", "; + } + local.colList &= $quoteIdentifier(local.col); + } + + ArrayAppend(local.sql, "INSERT ALL"); + + local.propCount = ArrayLen(arguments.validProperties); + for (local.r = arguments.batchStart; local.r <= arguments.batchEnd; local.r++) { + ArrayAppend(local.sql, " INTO #arguments.tableName# (#local.colList#) VALUES ("); + for (local.p = 1; local.p <= local.propCount; local.p++) { + if (local.p > 1) { + ArrayAppend(local.sql, ", "); + } + local.propName = arguments.validProperties[local.p]; + local.val = StructKeyExists(arguments.records[local.r], local.propName) ? arguments.records[local.r][local.propName] : ""; + ArrayAppend(local.sql, $buildBulkParam( + value = local.val, + propName = local.propName, + propertyInfo = arguments.propertyInfo + )); + } + ArrayAppend(local.sql, ")"); + } + + ArrayAppend(local.sql, " SELECT 1 FROM dual"); + + return local.sql; + } + /** * Oracle upsert using MERGE with USING (SELECT ... FROM dual UNION ALL ...) source. * Uses parameterized values via $buildBulkParam — never interpolates user data into SQL. diff --git a/vendor/wheels/model/bulk.cfc b/vendor/wheels/model/bulk.cfc index 5896af557f..8dd9fb6ae0 100644 --- a/vendor/wheels/model/bulk.cfc +++ b/vendor/wheels/model/bulk.cfc @@ -42,12 +42,14 @@ component { for (local.batchStart = 1; local.batchStart <= local.totalRecords; local.batchStart += local.batchSize) { local.batchEnd = Min(local.batchStart + local.batchSize - 1, local.totalRecords); - local.sql = $buildBulkInsertSQL( + local.sql = variables.wheels.class.adapter.$bulkInsertSQL( + tableName = $quotedTableName(), columns = local.mapped.columns, validProperties = local.mapped.validProperties, records = arguments.records, batchStart = local.batchStart, - batchEnd = local.batchEnd + batchEnd = local.batchEnd, + propertyInfo = variables.wheels.class.properties ); variables.wheels.class.adapter.$querySetup( @@ -196,53 +198,6 @@ component { return {columns: local.columns, validProperties: local.validProperties}; } - /** - * Builds the SQL array for a multi-row INSERT statement. - * Returns an array compatible with the adapter's `$querySetup()`. - */ - public array function $buildBulkInsertSQL( - required array columns, - required array validProperties, - required array records, - required numeric batchStart, - required numeric batchEnd - ) { - local.sql = []; - - local.colList = ""; - for (local.col in arguments.columns) { - if (Len(local.colList)) { - local.colList &= ", "; - } - local.colList &= $quoteColumn(local.col); - } - - ArrayAppend(local.sql, "INSERT INTO #$quotedTableName()# (#local.colList#) VALUES "); - - local.propCount = ArrayLen(arguments.validProperties); - for (local.r = arguments.batchStart; local.r <= arguments.batchEnd; local.r++) { - if (local.r > arguments.batchStart) { - ArrayAppend(local.sql, ", "); - } - ArrayAppend(local.sql, "("); - for (local.p = 1; local.p <= local.propCount; local.p++) { - if (local.p > 1) { - ArrayAppend(local.sql, ", "); - } - local.propName = arguments.validProperties[local.p]; - local.val = StructKeyExists(arguments.records[local.r], local.propName) ? arguments.records[local.r][local.propName] : ""; - ArrayAppend(local.sql, variables.wheels.class.adapter.$buildBulkParam( - value = local.val, - propName = local.propName, - propertyInfo = variables.wheels.class.properties - )); - } - ArrayAppend(local.sql, ")"); - } - - return local.sql; - } - /** * Adds `createdAt` and `updatedAt` timestamps to bulk record arrays when the model * is configured for automatic timestamping. diff --git a/vendor/wheels/tests/specs/model/oracleBulkInsertSqlSpec.cfc b/vendor/wheels/tests/specs/model/oracleBulkInsertSqlSpec.cfc new file mode 100644 index 0000000000..12ec5ffba5 --- /dev/null +++ b/vendor/wheels/tests/specs/model/oracleBulkInsertSqlSpec.cfc @@ -0,0 +1,186 @@ +component extends="wheels.WheelsTest" { + + function run() { + + // Adapter-level SQL-builder coverage so the Oracle bulk-insert path is + // verified on every engine/DB combo, not only the (currently soft-fail) + // boxlang/oracle slot in the compat matrix. The actual end-to-end behavior + // against a live Oracle datasource is covered by bulkOperationsSpec.cfc; + // this spec pins the contract the model layer relies on. + // + // See #2745 — BoxLang+Oracle errors: + // "returning clause is not allowed with INSERT and Table Value Constructor" + // caused by bulk INSERT emitting multi-row VALUES (?,?), (?,?) plus + // JDBC RETURN_GENERATED_KEYS handling on Oracle 23. + + describe("OracleModel.$bulkInsertSQL", () => { + + beforeEach(() => { + oracle = new wheels.databaseAdapters.Oracle.OracleModel(); + propertyInfo = { + firstName: {column: "firstName", type: "cf_sql_varchar", dataType: "varchar", scale: 0, nullable: true}, + lastName: {column: "lastName", type: "cf_sql_varchar", dataType: "varchar", scale: 0, nullable: true} + }; + records = [ + {firstName: "Alice", lastName: "Anderson"}, + {firstName: "Bob", lastName: "Brown"}, + {firstName: "Carol", lastName: "Clark"} + ]; + }); + + it("emits INSERT ALL ... SELECT 1 FROM dual instead of multi-row VALUES", () => { + var sql = oracle.$bulkInsertSQL( + tableName = """AUTHORS""", + columns = ["firstName", "lastName"], + validProperties = ["firstName", "lastName"], + records = records, + batchStart = 1, + batchEnd = 3, + propertyInfo = propertyInfo + ); + + expect(IsArray(sql)).toBeTrue(); + + var text = ""; + for (var part in sql) { + if (IsSimpleValue(part)) { + text &= part; + } + } + var collapsed = ReReplace(text, "[[:space:]]+", " ", "all"); + + // Oracle-idiomatic shape — avoids the table-value-constructor + + // RETURNING incompatibility on Oracle 23. + expect(collapsed).toInclude("INSERT ALL"); + expect(collapsed).toInclude(" INTO ""AUTHORS"" "); + expect(collapsed).toInclude("SELECT 1 FROM dual"); + + // And must NOT contain the multi-row VALUES tuple-list shape that + // Oracle JDBC rejects when RETURN_GENERATED_KEYS is requested. + expect(collapsed).notToMatch("VALUES \(.+\), ?\("); + }); + + it("emits one INTO clause per record in the batch", () => { + var sql = oracle.$bulkInsertSQL( + tableName = """AUTHORS""", + columns = ["firstName", "lastName"], + validProperties = ["firstName", "lastName"], + records = records, + batchStart = 1, + batchEnd = 3, + propertyInfo = propertyInfo + ); + + var text = ""; + for (var part in sql) { + if (IsSimpleValue(part)) { + text &= part; + } + } + + // Three records → three INTO clauses. + var intoCount = ArrayLen(ReMatch("(?i)INTO\s+""AUTHORS""", text)); + expect(intoCount).toBe(3); + }); + + it("parameterizes values via $buildBulkParam structs (no inline interpolation)", () => { + var sql = oracle.$bulkInsertSQL( + tableName = """AUTHORS""", + columns = ["firstName", "lastName"], + validProperties = ["firstName", "lastName"], + records = records, + batchStart = 1, + batchEnd = 3, + propertyInfo = propertyInfo + ); + + var paramCount = 0; + for (var part in sql) { + if (IsStruct(part) && StructKeyExists(part, "value") && StructKeyExists(part, "type")) { + paramCount++; + } + } + + // 3 records × 2 columns = 6 parameter structs. + expect(paramCount).toBe(6); + + // And the literal record values must NOT appear in the concatenated + // string parts — confirms values flow through $buildBulkParam. + var text = ""; + for (var part in sql) { + if (IsSimpleValue(part)) { + text &= part; + } + } + expect(text).notToInclude("Alice"); + expect(text).notToInclude("Anderson"); + }); + + it("handles a single-row batch without falling back to multi-row VALUES", () => { + var sql = oracle.$bulkInsertSQL( + tableName = """AUTHORS""", + columns = ["firstName", "lastName"], + validProperties = ["firstName", "lastName"], + records = [{firstName: "Solo", lastName: "Single"}], + batchStart = 1, + batchEnd = 1, + propertyInfo = propertyInfo + ); + + var text = ""; + for (var part in sql) { + if (IsSimpleValue(part)) { + text &= part; + } + } + var collapsed = ReReplace(text, "[[:space:]]+", " ", "all"); + + expect(collapsed).toInclude("INSERT ALL"); + expect(collapsed).toInclude("SELECT 1 FROM dual"); + }); + + }); + + describe("Base adapter $bulkInsertSQL (multi-row VALUES default)", () => { + + it("non-Oracle adapters keep the standard multi-row VALUES shape", () => { + // SQLite is the safest non-Oracle adapter to instantiate without + // touching the configured datasource — it has no $init side effects + // for SQL building. + var sqlite = new wheels.databaseAdapters.SQLite.SQLiteModel(); + var propertyInfo = { + firstName: {column: "firstName", type: "cf_sql_varchar", dataType: "varchar", scale: 0, nullable: true}, + lastName: {column: "lastName", type: "cf_sql_varchar", dataType: "varchar", scale: 0, nullable: true} + }; + + var sql = sqlite.$bulkInsertSQL( + tableName = """authors""", + columns = ["firstName", "lastName"], + validProperties = ["firstName", "lastName"], + records = [ + {firstName: "Alice", lastName: "Anderson"}, + {firstName: "Bob", lastName: "Brown"} + ], + batchStart = 1, + batchEnd = 2, + propertyInfo = propertyInfo + ); + + var text = ""; + for (var part in sql) { + if (IsSimpleValue(part)) { + text &= part; + } + } + var collapsed = ReReplace(text, "[[:space:]]+", " ", "all"); + + expect(collapsed).toInclude("INSERT INTO"); + expect(collapsed).toInclude("VALUES"); + expect(collapsed).notToInclude("INSERT ALL"); + }); + + }); + + } + +}