From a4c819b4ba39b47ffb1f1b676e643578cd9f00f2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 17 May 2026 02:36:18 +0000 Subject: [PATCH 1/2] fix(model): Oracle insertAll uses INSERT ALL form to avoid RETURNING-with-VALUES error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ("ORA: returning clause is not allowed with INSERT and Table Value Constructor", plus a follow-on "ORA: no statement parsed" for related lifecycle reasons). Moves bulk-insert SQL building off the model mixin and onto the database adapter (`$bulkInsertSQL` on Base.cfc, mirroring the existing `$upsertSQL` pattern). OracleModel overrides it with `INSERT ALL INTO t (cols) VALUES (...) ... SELECT 1 FROM dual` — Oracle's idiomatic multi-row insert form, which neither uses the table value constructor nor triggers the RETURNING expansion. Non-Oracle adapters keep the standard multi-row VALUES shape. The migrator-rename "Closed statement" error in the same compat-matrix run is a separate Oracle JDBC lifecycle issue and is left for a follow-up. Fixes #2745 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --- CHANGELOG.md | 1 + vendor/wheels/databaseAdapters/Base.cfc | 53 +++++ .../databaseAdapters/Oracle/OracleModel.cfc | 59 ++++++ vendor/wheels/model/bulk.cfc | 53 +---- .../specs/model/oracleBulkInsertSqlSpec.cfc | 186 ++++++++++++++++++ 5 files changed, 303 insertions(+), 49 deletions(-) create mode 100644 vendor/wheels/tests/specs/model/oracleBulkInsertSqlSpec.cfc diff --git a/CHANGELOG.md b/CHANGELOG.md index 79ac88d9f0..2e1dec55ed 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) - `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) - `paginationNav()` `showFirst` / `showLast` / `showPrevious` / `showNext` args now accept the tri-state strings `"auto"` / `"always"` / `"never"` (with backwards-compatible boolean coercion: `true` → `"always"`, `false` → `"never"`) and default to `"auto"`. Under `"auto"` the first/last anchors only render when the visible page-number window does not already reach the boundary — restoring the legacy 3.x `paginationLinks(alwaysShowAnchors=false)` semantics that a like-for-like swap to `paginationNav()` previously lost. Under `"auto"` the previous/next anchors always delegate to `previousPageLink()` / `nextPageLink()`, which render a disabled `` at the boundary by default — preserving the legacy `showPrevious=true` / `showNext=true` boundary indicator unless callers opt out with `"never"`. Adds a `windowSize` arg on `paginationNav()` so the auto-mode predicates stay coherent with `pageNumberLinks()`'s window (now passed explicitly to `pageNumberLinks()` instead of leaking through the anchor sub-helpers). Invalid strings throw `Wheels.InvalidArgument` at the call site - `QueryBuilder.whereIn()` / `whereNotIn()` with an empty array no longer emit malformed SQL (`property IN ()`). Previously, passing an empty list or array to either method produced syntactically invalid SQL that surfaced as a generic JDBC syntax error from the database, with no pointer back to the call site that built the empty collection. `whereIn(prop, [])` now sets an `$alwaysEmpty` flag on the builder so every terminal method (`count`, `findAll`, `findOne`, `first`, `exists`, `updateAll`, `deleteAll`, `findEach`, `findInBatches`) short-circuits to the appropriate zero-row sentinel before going through the finder. `whereNotIn(prop, [])` is a no-op (exclude-none = match-all), so the chain proceeds normally. Matches the user-facing behaviour every mature ORM converged on (Rails, Sequel, Django, Laravel Eloquent: empty `IN` matches no rows, empty `NOT IN` matches every row). The flag-based design avoids a runtime trap from Wheels' WHERE-clause parser (`vendor/wheels/model/sql.cfc` runs a property-extraction regex over every clause it sees — a raw `1 = 0` literal would be parsed as property `1` and trip `Wheels.ColumnNotFound`). Fourteen new specs in `vendor/wheels/tests/specs/model/queryBuilderSpec.cfc` cover empty-array, empty-list, composition with other clauses, the `whereNotIn` mirrors, every patched terminal (`findAll`, `first` / `findOne`, `exists`, `count`, `updateAll`, `deleteAll`, `findEach`, `findInBatches`), and the documented `select()` / `include()` silent-ignore caveat on the short-circuit path. Both copies of the query-builder guide were updated to document the short-circuit in the methods table (#2736) 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"); + }); + + }); + + } + +} From 97d6380631bd2722eb96b58b5f53cb19600df8cd Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 17 May 2026 02:38:44 +0000 Subject: [PATCH 2/2] docs: note Oracle INSERT ALL requirement for bulk insert in cross-engine guide Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> --- .ai/wheels/cross-engine-compatibility.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.ai/wheels/cross-engine-compatibility.md b/.ai/wheels/cross-engine-compatibility.md index d497405f11..e3708a43a4 100644 --- a/.ai/wheels/cross-engine-compatibility.md +++ b/.ai/wheels/cross-engine-compatibility.md @@ -293,6 +293,14 @@ See `vendor/wheels/tests/specs/migrator/addColumnOptionsSpec.cfc` and `migration 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