fix(migrator): renameSystemTables skips transaction wrapper on Oracle (#2745) - #2753
Conversation
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a clean, well-scoped fix for two distinct Oracle JDBC bugs in insertAll() and renameSystemTables(). The architecture mirrors the existing $upsertSQL() adapter hook and the implementation is correct. I have one documentation nit and one test-coverage observation, neither blocking. Overall verdict: comment (no changes required from the author; CI Oracle run is the final gate).
Correctness
No issues. Both fixes address real Oracle JDBC constraints with the right approach:
Bulk INSERT path (vendor/wheels/model/bulk.cfc + vendor/wheels/databaseAdapters/Base.cfc + OracleModel.cfc): Moving SQL generation into the adapter via a new $bulkInsertSQL() hook is the correct abstraction. The Base implementation keeps the existing single multi-row VALUES (...), (...) statement (unchanged fast path for non-Oracle). The Oracle override correctly emits one single-row INSERT per record, which sidesteps the JDBC driver's auto-RETURNING rewrite conflicting with multi-row VALUES. The loop in insertAll() over local.sqlBatches is safe — totalInserted is still computed from batch bounds, not from the result structs, so Oracle's N queries count identically to the single non-Oracle query. The fact that $querySetup()'s return value is ignored in insertAll() (unlike single-record save()) means Oracle's $identitySelect() is never involved here; no secondary concern about identity retrieval.
Migrator rename path (vendor/wheels/Migrator.cfc lines 720–737): The Oracle branch runs DDL bare, consistent with the existing comment that Oracle auto-commits DDL and forfeits any rollback guarantee. Other engines keep the transaction action="begin" wrapper. This is the minimal correct fix — no unnecessary restructuring.
Cross-engine
No issues introduced. Two items checked explicitly:
-
for (local.bSql in local.sqlBatches)on Adobe CF — Adobe CF copies array elements by value in afor-inloop. Since$querySetup()only reads (never mutates) the SQL array, a value copy is harmless. No behavioral difference across engines. -
$quoteIdentifier()call insideBase.$bulkInsertSQL()— Because$bulkInsertSQLis an adapter method,$quoteIdentifier()resolves tothis.$quoteIdentifier()on the concrete adapter class. Oracle overrides it with uppercase + double-quote; MySQL with backticks; Postgres/SQLite/H2/MSSQL with their own characters. The column-quoting is adapter-correct in all cases.
Tests
Gap worth noting (vendor/wheels/tests/specs/model/bulkOperationsSpec.cfc): The existing specs exercise insertAll() via a real DB round-trip (single multi-row path). There is no test that verifies the multi-batch loop itself — i.e., that an adapter returning N SQL arrays from $bulkInsertSQL() causes N calls to $querySetup(). The Oracle per-row behavior can only be exercised on Oracle CI, which the PR acknowledges. However, a unit-level test using a mock or subclassed adapter returning two SQL arrays for a 2-record insert would let non-Oracle CI catch regressions in the loop logic if someone later refactors insertAll(). Not blocking — just worth filing as a follow-up.
Docs
Stale docstring (vendor/wheels/databaseAdapters/Base.cfc line 831):
/**
* Builds parameter struct for a single value in a bulk operation.
* Used by adapter upsert implementations.
*/
public struct function $buildBulkParam(The $buildBulkParam function is now called by both $bulkInsertSQL() and upsert implementations. The line "Used by adapter upsert implementations" should read "Used by adapter bulk insert and upsert implementations." Minor; change inline if you touch this file again.
CHANGELOG entry is present and placed correctly under [Unreleased] > Fixed. Comprehensive.
Commits
fix(model,migrator): Oracle bulk insert and migrator rename (#2745) — valid conventional commit, scope unrestricted per commitlint.config.js, subject under 100 chars, references the originating issue.
Security
No concerns. Column and table names flow through $quoteIdentifier(). Record values flow through $buildBulkParam() as typed parameter structs consumed by $querySetup()'s parameterized query builder — no user data is ever interpolated as raw SQL strings.
Wheels Bot - Reviewer B (round 1)A's review is substantive and well-reasoned. Every correctness claim is grounded in the diff. No sycophancy, no blocking false positives. Two minor observations below, neither changing the verdict. SycophancyNone detected. A's verdict (comment, no changes required) is backed by explicit per-section analysis of each changed file. False positivesOne imprecise cross-engine citation, conclusion still sound:
Missed issuesOne substitution not explicitly noted:
Verdict alignmentA's comment verdict (no blocking issues, CI Oracle run is the final gate) is consistent with findings explicitly labeled non-blocking. Verdict is correct. ConvergenceAligned. A correctly identifies the change as a clean, well-scoped fix with no cross-engine regressions in the tested adapters. The two findings (stale docstring, test coverage gap) are genuine, both explicitly non-blocking. My re-scan adds no significant missed issues. Joint recommendation: clean for merge pending CI Oracle gate. |
`Model.insertAll()`: Oracle does not allow multi-row
`INSERT ... VALUES (...), (...)` together with the JDBC driver's
auto-RETURNING (triggered by `cfquery result="..."`). Move bulk-insert
SQL generation into the adapter behind a new `$bulkInsertSQL()` hook
(mirrors `$upsertSQL()`); Base keeps the multi-row form, Oracle
returns one single-row INSERT per record so the auto-RETURNING
transform only ever sees a plain `INSERT INTO ... VALUES (...)`.
`Migrator.renameSystemTables()`: Oracle implicitly commits DDL and
closes the JDBC statement, so the surrounding
`transaction action="begin" { ... commit }` then raised
`ORA: Closed statement`. Skip the transaction wrapper on Oracle (the
existing code comment already acknowledged it was a no-op there);
other engines keep their atomic-rollback path.
BoxLang × Oracle was the only engine exercising these paths because
Lucee 6/7 still skip Oracle on the soft-fail track from #2663.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
af996d7 to
723855d
Compare
|
Addressed reviewer feedback in 723855d:
|
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR fixes two distinct Oracle JDBC failures in insertAll() and renameSystemTables() by moving bulk-INSERT SQL generation into an adapter hook and stripping the transaction wrapper from the Oracle DDL path. The implementation is correct, the architecture mirrors the existing $upsertSQL() pattern, and the one docstring nit flagged in the previous review round has been addressed in this commit. One test-coverage gap (the multi-batch loop / per-row Oracle path) remains a CI-only gate. No blocking issues. Verdict: comment.
Correctness
No issues.
Bulk INSERT path (vendor/wheels/model/bulk.cfc + vendor/wheels/databaseAdapters/Base.cfc + OracleModel.cfc): The adapter hook ($bulkInsertSQL()) returns an array of SQL arrays. Base.$bulkInsertSQL() emits a single multi-row VALUES (...), (...) statement (the fast path for all non-Oracle adapters, unchanged). OracleModel.$bulkInsertSQL() emits one single-row INSERT per record, so Oracle's JDBC driver never sees a multi-row VALUES clause when its auto-RETURNING rewrite is active. The for (local.bSql in local.sqlBatches) loop in insertAll() handles both shapes (one-element array for non-Oracle, N-element array for Oracle) transparently.
The totalInserted counter is still computed from batch bounds (batchEnd - batchStart + 1), not from $querySetup() return values, so Oracle's N queries produce an identical count to the single non-Oracle query.
Migrator rename path (vendor/wheels/Migrator.cfc line 721): The Oracle branch runs DDL bare inside the outer try/catch, which is correct — Oracle auto-commits DDL and there is no rollback to forfeit. Non-Oracle engines remain in the transaction action="begin" { ... commit } branch unchanged. The outer error-catch still covers the Oracle path (sets rv.success = false on failure).
Cross-engine
No issues.
-
for (local.bSql in local.sqlBatches)on Adobe CF — Adobe CF copies array elements by value in a for-each loop. Since$querySetup()only readslocal.bSqland never mutates it, the value-copy is harmless across all engines. -
$quoteIdentifier()insideBase.$bulkInsertSQL()— The call resolves via late binding. When non-Oracle adapters (MySQL, PostgreSQL) inheritBase.$bulkInsertSQL(), their own$quoteIdentifier()override (backticks, double-quotes) is dispatched correctly. Oracle overrides$bulkInsertSQL()entirely, so its uppercase + double-quote$quoteIdentifier()is called fromOracleModel.$bulkInsertSQL()directly — no ambiguity. -
BoxLang method-receiver binding —
variables.wheels.class.adapter.$bulkInsertSQL(...)inbulk.cfcis a single-expression named-method call, not a bracket-call stored in a local variable. The receiver is bound correctly on BoxLang (cross-engine compat doc, "Method Reference Extraction Loses Receiver" section).
Tests
Gap worth tracking (vendor/wheels/tests/specs/model/bulkOperationsSpec.cfc): Existing specs exercise insertAll() via a real DB round-trip on SQLite (multi-row fast path). There is still no test for the loop path itself — i.e., that an adapter returning N SQL arrays from $bulkInsertSQL() causes N $querySetup() calls. A unit-level spec using a subclassed adapter returning two SQL arrays for a 2-record insert would let non-Oracle CI catch regressions if someone refactors insertAll() in the future. Not blocking — Oracle CI is the real reproduction gate.
Docs
The docstring nit from the previous review round ($buildBulkParam comment "Used by adapter upsert implementations") has been addressed in this commit:
- * Used by adapter upsert implementations.
+ * Used by adapter bulk insert and upsert implementations.
CHANGELOG entry is present under [Unreleased] > Fixed and is comprehensive.
Commits
fix(model,migrator): Oracle bulk insert and migrator rename (#2745) — valid conventional commit, type and scope are allowed by commitlint.config.js, subject under 100 chars, references the originating issue.
Security
No concerns. Column names flow through $quoteIdentifier(). Record values flow through $buildBulkParam() as typed parameter structs consumed by $querySetup()'s parameterized query builder — no user data is interpolated as raw SQL.
Wheels Bot — Reviewer B (round 1)A's review of the amended commit is accurate and well-grounded. No sycophancy, no false positives, no missed issues on the diff I re-scanned. A's `comment` verdict (no changes required, Oracle CI is the final gate) is consistent with the findings. SycophancyNone detected. A's verdict is backed by per-section analysis of every changed file. False positivesNone detected. Spot-checked all three cross-engine claims:
Missed issuesNone detected. The docstring fix and commit-body line-length fix in this amended commit are minor and accurately described. B's round-1 observations from the prior SHA (`$quoteColumn` → `$quoteIdentifier` substitution, anti-pattern citation precision) were acknowledged by the author with no code change required, and A's re-review correctly folds in the `$quoteIdentifier()` late-binding analysis. The remaining test-coverage gap (unit test for the multi-batch loop path) was previously noted as non-blocking and is still tracked as a follow-up. Verdict alignmentA's `comment` verdict (no changes required) is consistent with the findings: all prior review items resolved, one non-blocking test-coverage gap explicitly deferred. ConvergenceAligned. A and B agree the fix is correct and complete for the tested adapters; Oracle CI is the outstanding gate, not a code issue. Joint recommendation: clean for merge pending the BoxLang × Oracle CI run. |
…at-fd56f2 # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Third pass, triggered by the develop-merge commit (ebe3e592). The Oracle bulk-insert and migrator-rename fixes are unchanged since the previous review round and remain correct. The merge brought in CockroachDBModel.$supportsAdvisoryLocks() → false (PR #2746, already landed on develop) via a CHANGELOG conflict resolution — a clean one-liner with no new concerns. One test-coverage gap noted in prior rounds (the multi-batch loop path) is still open but non-blocking; CI Oracle is the reproduction gate. Verdict: comment.
Correctness
No new issues introduced by the merge commit.
Merge delta (ebe3e592 vs 723855d9): The only change visible in gh pr diff is the CHANGELOG merge-conflict resolution that placed the CockroachDB entry alongside the Oracle entry. CockroachDBModel.$supportsAdvisoryLocks() itself is not part of this PR's diff — it was already on develop before the merge and appears only in the git delta, not in the PR's cumulative diff against base.
Oracle bulk-insert path (vendor/wheels/model/bulk.cfc, vendor/wheels/databaseAdapters/Base.cfc, vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc): Unchanged and correct. The $bulkInsertSQL() hook returns an array of SQL arrays; Base emits a single multi-row VALUES statement (fast path unchanged for non-Oracle adapters), OracleModel emits one single-row INSERT per record. The for (local.bSql in local.sqlBatches) loop in bulk.cfc handles both shapes transparently. totalInserted is computed from batch bounds, not $querySetup() return values, so Oracle's N queries produce an identical count to the single non-Oracle query.
One detail verified this round: the old $buildBulkInsertSQL in bulk.cfc (now deleted) called $quoteColumn(local.col). The new Base.$bulkInsertSQL calls $quoteIdentifier(local.col) directly on the adapter instance. These are functionally identical — $quoteColumn is a model mixin defined at vendor/wheels/model/miscellaneous.cfc:197 that delegates to variables.wheels.class.adapter.$quoteIdentifier(col). No behavioral change in column quoting across any engine.
Migrator rename path (vendor/wheels/Migrator.cfc:721): Unchanged and correct. dbType is set at line 687 (var dbType = info.database_productname;) before the Oracle branch at line 721 — the variable is in scope. The Oracle DDL runs bare inside the outer try/catch (lines 720 / 747), which catches failures and sets rv.success = false. Other engines keep the transaction action="begin" { ... commit } wrapper unchanged.
Cross-engine
No new concerns. One item checked this round: withAdvisoryLock in vendor/wheels/model/locking.cfc:41 does not check $supportsAdvisoryLocks() before calling $acquireAdvisoryLock — the capability flag is test-infrastructure only at this point. Runtime behavior on CockroachDB (the adapter throws with "CockroachDB does not support advisory locks.") is unchanged and predates this PR. Only the four lockingSpec withAdvisoryLock specs now skip cleanly instead of erroring.
Tests
Ongoing gap (vendor/wheels/tests/specs/model/bulkOperationsSpec.cfc): No new test for the multi-batch loop path — i.e., an adapter returning N SQL arrays from $bulkInsertSQL() causing N $querySetup() calls. Noted in both prior rounds. Non-blocking; Oracle CI is the real reproduction gate.
Docs
CHANGELOG [Unreleased] > Fixed now contains both the Oracle entry (this PR) and the CockroachDB entry (from develop, merged in). Both are accurate and comprehensive.
Commits
fix(model,migrator): Oracle bulk insert and migrator rename (#2745)— valid conventional commit percommitlint.config.js, subject under 100 chars, references the originating issue.- Merge commit
ebe3e592: standard merge message; no commit-lint obligations apply to merge commits.
Security
No concerns. Column names flow through $quoteIdentifier(). Record values flow through $buildBulkParam() as typed parameter structs consumed by $querySetup()'s parameterized query builder — no user data is interpolated as raw SQL at any point in the new code paths.
…at-fd56f2 # Conflicts: # CHANGELOG.md # vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc # vendor/wheels/model/bulk.cfc
bpamiri
changed the title
|
Rebased to reconcile with #2749 which shipped the bulk-insert fix via a different approach ( |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Fourth pass, triggered by the develop-merge commit (5fb5e697). The commit brings in PR #2749 (Oracle INSERT ALL bulk-insert fix) and #2748 (MySQL addColumnOptionsSpec carve-out) from develop, and resolves conflicts in CHANGELOG.md, OracleModel.cfc, and bulk.cfc. The migrator-rename fix (this PR's only original change) is unchanged and remains correct. Two notable improvements in this merge: the gap flagged in prior review rounds (no unit test for the adapter-level bulk-insert contract) is now addressed by the new oracleBulkInsertSqlSpec.cfc, and the $bulkInsertSQL() API was simplified to a flat-array contract that removes the wrapping loop from insertAll(). No new correctness, cross-engine, or security concerns. Verdict: comment.
Correctness
No new issues.
Base.$bulkInsertSQL() return-type change (vendor/wheels/databaseAdapters/Base.cfc line ~784): The function previously returned [local.sql] (an array containing one SQL array, allowing adapters to return N entries for N queries). The merge commit changes it to return local.sql directly (a flat SQL array — the input to one $querySetup() call). bulk.cfc::insertAll() was updated in the same commit to pass the return value directly to $querySetup() rather than iterating:
// Before merge
for (local.bSql in local.sqlBatches) {
variables.wheels.class.adapter.$querySetup(parameterize = arguments.parameterize, sql = local.bSql);
}
// After merge
variables.wheels.class.adapter.$querySetup(parameterize = arguments.parameterize, sql = local.sql);Both shapes are consistent: Base now returns a single flat SQL array for the standard multi-row VALUES INSERT, and OracleModel now returns a single flat SQL array for the INSERT ALL ... SELECT 1 FROM dual form. The loop was only necessary when Oracle returned one entry per record; INSERT ALL collapses all rows into a single statement, making the loop superfluous. API and usage are in sync. ✓
OracleModel.$bulkInsertSQL() form change (vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc lines 200–238): Changed from emitting N single-row INSERT INTO ... VALUES (?) statements (one per record) to a single INSERT ALL INTO ... SELECT 1 FROM dual statement. The critical property is the same — neither form triggers Oracle JDBC's RETURNING expansion — but INSERT ALL achieves it in a single statement instead of N, which also means totalInserted remains exact without any change to the batchEnd - batchStart + 1 counter in insertAll(). ✓
Migrator rename path (vendor/wheels/Migrator.cfc lines 718–737): Unchanged from prior rounds. The Oracle branch runs bare DDL inside the existing outer try/catch; other engines keep the transaction action="begin" { ... commit } wrapper. ✓
Cross-engine
No new concerns.
INSERT ALL parameterization: Oracle's JDBC driver supports parameterized INSERT ALL statements (the whole motivation for using this form over multi-row VALUES). Each ? placeholder in the INSERT ALL body is bound in declaration order by $querySetup()'s cfqueryparam expansion — the same mechanism that handles every other parameterized SQL in the framework. ✓
for (var part in sql) in oracleBulkInsertSqlSpec.cfc: The spec iterates over the returned SQL array using a for-each loop, only concatenating IsSimpleValue(part) fragments. Adobe CF copies array elements by value in for-each loops (cross-engine compat doc, "Array by-value in struct literals" section), but since no mutation occurs inside the loop body, the value-copy is harmless across all engines. ✓
Inline regex flags (oracleBulkInsertSqlSpec.cfc line ~96): ReMatch("(?i)INTO\s+""AUTHORS""", text) uses a Java inline flag (?i) for case-insensitive matching. CFML delegates to Java's java.util.regex engine on all supported platforms (Lucee 5/6/7, Adobe CF 2018–2025, BoxLang), so (?i) is universally available. ✓
Tests
Gap closed. Prior review rounds noted the absence of a unit test for the adapter-level $bulkInsertSQL() contract. The new vendor/wheels/tests/specs/model/oracleBulkInsertSqlSpec.cfc (186 lines) covers:
INSERT ALL ... SELECT 1 FROM dualshape (not multi-row VALUES)- Exactly N
INTOclauses for N records in batch - Values flowing through
$buildBulkParamstructs (3 × 2 = 6 param structs, no inline interpolation) - Single-record batch produces
INSERT ALL(not a fallback to single-row INSERT) - Non-Oracle adapters (
SQLiteModel) keepINSERT INTO ... VALUESand never emitINSERT ALL
The specs instantiate OracleModel and SQLiteModel directly — testing only the SQL-builder logic, no live DB connection required. This correctly pins the adapter contract on every engine/DB slot in the compat matrix, not just the (currently soft-fail) BoxLang/Oracle slot. ✓
Docs
CHANGELOG [Unreleased] > Fixed now has four separate, clearly scoped bullets (migrator rename, model.insertAll(), addColumnOptionsSpec MySQL, CockroachDB advisory locks) rather than one combined entry. Each references its originating issue. ✓
.ai/wheels/cross-engine-compatibility.md was updated with two new sections: "MySQL — TEXT and FLOAT DEFAULT suppression" and "Oracle — Multi-Row INSERT and RETURNING Incompatibility". Both accurately describe the behavior and reference canonical implementations. ✓
Commits
fix(model): Oracle insertAll uses INSERT ALL form to avoid RETURNING-with-VALUES error (#2749)— valid conventional commit, subject under 100 chars.fix(migrator): carve out MySQL in addColumnOptionsSpec text-default assertion (#2748)— valid, under 100 chars.- Merge commit
5fb5e697: standard three-way merge message; no commitlint obligations apply to merge commits.
Security
No concerns. All record values flow through $buildBulkParam() as typed parameter structs consumed by $querySetup()'s parameterized query builder. Column and table names flow through $quoteIdentifier(). No user data is interpolated as raw SQL at any point in the new or changed code paths.
Wheels Bot -- Reviewer B (round 1)A's 4th-pass review is substantive and backs its verdict with per-section analysis. No sycophancy, no blocking false positives. Two observations below, neither changes the verdict. SycophancyNone detected. A's False positivesOne recurring imprecision in anti-pattern citation:
Missed issuesOne documentation gap not called out:
Also noted (not a missed issue, just an observation): A spends most of the review analyzing code that arrived via the Verdict alignmentA's ConvergenceAligned. A and B agree the fix is correct and complete. The two observations above are non-blocking: the anti-pattern citation imprecision is a recurring presentation issue (not a correctness issue), and the |
Add an `Oracle — DDL Auto-Commit and Transaction Wrapper` section to the cross-engine compat doc, mirroring the existing Oracle bulk-insert section. Documents the `Closed statement` failure mode and the adapter-branch pattern used in `Migrator.renameSystemTables`. Per Reviewer B's follow-up observation on PR #2753 — closes the deep-reference gap so future devs adding DDL operations find the pattern in `.ai/` instead of only in the inline Migrator.cfc comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
|
Addressed Reviewer B's follow-up observation about the missing The anti-pattern citation imprecision is on Reviewer A's side, not mine — no code change needed. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Fifth pass, triggered by the single new commit 1f8a946aa (docs(ai): document Oracle DDL auto-commit transaction-wrapper pattern). This is a docs-only addition that places the Oracle DDL Auto-Commit section in .ai/wheels/cross-engine-compatibility.md, closing the deep-reference gap that Reviewer B noted on this PR. The section is accurate, the code snippet matches the canonical implementation in Migrator.cfc verbatim, and the placement follows naturally after the existing Oracle bulk-insert section. One minor phrasing nit on the MySQL claim (inherited from the existing code comment, non-blocking). No correctness, cross-engine, or security concerns. Verdict: comment.
Docs
New section accurate (.ai/wheels/cross-engine-compatibility.md lines 324-350):
- Oracle DDL auto-commit behavior (RENAME, CREATE, ALTER, DROP all implicitly commit and close the JDBC statement) is correctly stated.
- The failure mode —
ORA: Closed statementwhentransaction action="commit"runs against a closed statement — matches the PR description and the original bug report. - The code snippet is taken verbatim from
vendor/wheels/Migrator.cfc::renameSystemTables(lines 721-737 as modified by this PR). No drift. - Placement after the Oracle multi-row INSERT section is logical: the two Oracle sections now group all Oracle JDBC quirks together before "Testing Across Engines".
Minor nit (.ai/wheels/cross-engine-compatibility.md line 326):
Other engines (Postgres, SQLite via SAVEPOINT, MySQL on InnoDB) honor the wrapper and roll the DDL back on error; Oracle cannot.
MySQL DDL (RENAME TABLE) also causes an implicit commit in the engine — it does not actually roll back via the InnoDB transaction wrapper. The atomicity guarantee for the MySQL path comes from RENAME TABLE accepting multiple pairs in a single statement (Migrator.cfc line 694: "RENAME TABLE " & ArrayToList(pairs, ", ")), not from the transaction. This sentence inherits the same slight imprecision from the existing inline comment at Migrator.cfc:716. Since .ai/wheels/ is the persistent reference for future maintainers, a small clarification here would prevent the same misconception from appearing in the next DDL operation someone adds:
- Other engines (Postgres, SQLite via SAVEPOINT, MySQL on InnoDB) honor the wrapper
- and roll the DDL back on error; Oracle cannot.
+ PostgreSQL and SQLite (via SAVEPOINT) honor the wrapper and will roll back DDL on error.
+ MySQL DDL also causes an implicit commit (the wrapper is a no-op there), but MySQL's
+ multi-rename form — RENAME TABLE a, b — is itself a single atomic statement, so no
+ partial-rename scenario arises. Oracle cannot use the wrapper at all.Non-blocking — clarifying, not correcting. Raise as a follow-up if desired.
Commits
docs(ai): document Oracle DDL auto-commit transaction-wrapper pattern — valid conventional commit: type docs, subject 62 chars (under the 100-char limit), sentence-case, not ALL-CAPS. Commit body names the originating PR (#2753) and the Reviewer B observation that motivated the addition. DCO sign-off present (Signed-off-by: Peter Amiri <peter@alurium.com>). Clean.
Per Reviewer A's nit on PR #2753: MySQL DDL also implicitly commits (InnoDB transaction wrapper is a no-op for DDL). The MySQL atomicity guarantee comes from `RENAME TABLE a, b` being a single multi-pair statement, not from the transaction. Persistent `.ai/` reference now reflects that distinction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)A's fifth-pass review is accurate and well-scoped. It correctly limits itself to the single new commit SycophancyNone detected. A's False positivesNone detected. Spot-checked A's key factual claims:
Missed issuesNone detected. The CHANGELOG.md entry carries the same MySQL imprecision ("MySQL on InnoDB keep their atomic-rollback behavior") but that entry is from a prior commit already reviewed in rounds 1-4. Scoping the fifth pass to Verdict alignmentA's ConvergenceAligned. A and B agree the PR is complete: Oracle migrator fix correct, docstring updated, |
….cfc Reviewer B noted the CHANGELOG entry inherits the same MySQL imprecision the `.ai/` doc had — "MySQL on InnoDB keep their atomic-rollback behavior" implies the transaction wrapper rolls MySQL DDL back, but MySQL DDL also implicitly commits. The atomicity actually comes from `RENAME TABLE a, b` being a single multi-pair statement. Update the CHANGELOG entry and the inline `Migrator.cfc` comment so all three reference points (`.ai/`, CHANGELOG, code comment) say the same thing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
|
Addressed both Reviewer A and Reviewer B's MySQL atomicity precision nit:
All three reference points now say the same thing: PG/SQLite honor the wrapper, MySQL DDL implicitly commits but |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Sixth pass, triggered by commit b25560b1f (docs(ai): clarify MySQL atomicity in DDL transaction-wrapper section). This is a single-sentence clarification in .ai/wheels/cross-engine-compatibility.md that directly addresses the MySQL atomicity nit raised in round five. The text is accurate, the commit is clean, and the PR is in good shape. One minor observation about a residual inconsistency in the CHANGELOG entry, non-blocking. Verdict: comment.
Docs
Nit addressed correctly (.ai/wheels/cross-engine-compatibility.md line 326):
The round-five review flagged this imprecise sentence:
Other engines (Postgres, SQLite via SAVEPOINT, MySQL on InnoDB) honor the wrapper and roll the DDL back on error; Oracle cannot.
The new text reads:
PostgreSQL and SQLite (via SAVEPOINT) honor the wrapper and will roll back DDL on error. MySQL DDL also causes an implicit commit (the wrapper is a no-op there), but MySQL's multi-rename form —
RENAME TABLE a TO a', b TO b'— is itself a single atomic statement, so no partial-rename scenario arises. Oracle cannot use the wrapper at all.
This is accurate. MySQL DDL causes an implicit commit on all storage engines (not just InnoDB), so the transaction wrapper is indeed a no-op for MySQL DDL. The atomicity of renameSystemTables on MySQL comes from RENAME TABLE a TO a', b TO b' being a single multi-pair statement — MySQL documents this form as atomic (either all renames succeed or all fail). The canonical implementation at vendor/wheels/Migrator.cfc line ~694 uses "RENAME TABLE " & ArrayToList(pairs, ", ") which produces exactly this form. The explanation matches the code.
Residual minor inconsistency (CHANGELOG.md, the Migrator.renameSystemTables() bullet from a prior commit):
The CHANGELOG entry still reads:
...other engines (Postgres, SQLite via SAVEPOINT, MySQL on InnoDB) keep their atomic-rollback behavior.
The phrase "MySQL on InnoDB" and "atomic-rollback behavior" is now slightly inconsistent with the clarified .ai/ doc, which correctly states the MySQL wrapper is a no-op and the atomicity comes from the multi-pair statement, not from rollback. User-facing CHANGELOG entries do not need the same precision as maintainer reference docs, and the sentence is not wrong from the end-user perspective (the function works correctly on MySQL). Worth a one-word tweak if the entry is edited for any other reason, but does not require a dedicated fixup commit.
Commits
docs(ai): clarify MySQL atomicity in DDL transaction-wrapper section — valid conventional commit: type docs, subject 57 chars (under 100-char limit), sentence-case, not ALL-CAPS. Commit body attributes the change to the round-five nit, names the PR, and includes DCO sign-off (Signed-off-by: Peter Amiri <peter@alurium.com>) plus Co-Authored-By trailer. Clean.
Wheels Bot — Reviewer B (round 1)A's sixth-pass review is substantive on the content it covers. However, A's review marker anchors to SycophancyNone detected. A's False positivesOne — A flags a residual inconsistency in CHANGELOG.md that was already resolved on the current head:
Missed issuesOne — A reviewed
Verdict alignmentA's ConvergenceAligned. The false positive (stale CHANGELOG flag) and the missed SHA are both attributable to |
1 participant
Summary
Follow-up to #2749 — addresses the remaining migrator-rename failure from #2745.
This PR originally fixed both
Model.insertAll()andMigrator.renameSystemTables()on Oracle. #2749 shipped the bulk-insert fix with a different approach (INSERT ALLsyntax instead of per-row INSERTs) — I rebased to drop my redundant bulk-insert changes and keep only the migrator fix that #2749 explicitly deferred.Migrator rename:
Migrator.renameSystemTables()wrapped DDL intransaction action="begin" { ... commit }, but Oracle implicitly commits DDL and closes the JDBC statement, so the subsequenttransaction action="commit"raisedORA: Closed statement. The transaction wrapper is now skipped on Oracle (the existing code comment already called out the wrapper as a no-op there); other engines (Postgres, SQLite via SAVEPOINT, MySQL on InnoDB) keep their atomic-rollback behavior.Also includes a minor docstring nit for
$buildBulkParamto read "Used by adapter bulk insert and upsert implementations" (it's no longer used only by upsert after #2749 wired up$bulkInsertSQL).Test plan
bash tools/test-local.sh migrator— passed on Lucee 7 + SQLite, no regression in rename pathbash tools/test-local.sh model— passed on Lucee 7 + SQLite, no regressiontools/test-matrix.sh boxlang oracle— the originalmigratorSpec :: renames c_o_r_e_levels -> wheels_levelsreproduction; CI will validate🤖 Generated with Claude Code