[bpamiri]

Implements all three deferred items from #2906 plus the MSSQL/Oracle adapter-cleanup section of #2961 (DA1, DA5, DA14) — fused into one branch deliberately because they touch the same files in vendor/wheels/databaseAdapters/.

What changed

1. SQL Server: same-batch identity retrieval (#2906 item 1)

When the engine surfaces no driver generated key (currently BoxLang), $querySetup now appends ;SELECT SCOPE_IDENTITY() AS lastId to the INSERT's own batch — SCOPE_IDENTITY() is batch-scoped, so it must ride along; a standalone call returns NULL over JDBC. Base.$executeQuery already pipes the batch's resultset into $identitySelect as returningIdentity (the same plumbing CockroachDB uses for RETURNING), and the new MSSQL $lastIdLookup hook reads it there. The session-scoped @@IDENTITY → SCOPE_IDENTITY() chain is retained as an absolute last resort, so a same-batch miss on an unverifiable engine/driver combo degrades to the pre-fix behavior instead of losing the key. The stale "tracked in a follow-up issue" comment block is rewritten accordingly.

2. Oracle: CURRVAL replaces the MAX(ROWID) race (#2906 item 2)

The no-driver-key fallback now resolves the identity column's backing sequence via user_tab_identity_cols (new $identitySequenceName() helper, identifier-whitelisted because $query has no parameter binding, wrapped in try/catch for pre-12c) and reads <seq>.CURRVAL — session-scoped, race-free. The MAX(ROWID) heuristic survives only for pre-12c schemas with no discoverable identity sequence; its stale comment is rewritten too. The driver-key path (numeric key / 18-char ROWID gate) is preserved verbatim.

3. $identitySelect six-way dedup (#2906 item 3)

The six divergent copies (Base, MSSQL, PostgreSQL, Oracle, SQLite, CockroachDB) are unified into a Base template method holding the shared guards (INSERT-only, key-already-present, bulk path, key-in-column-list), with two hooks:

• $parseInsertColumnList() — column-list extraction with the BoxLang ReplaceList regex workaround and the missing-closing-paren guard (previously each existed in only a subset of adapters; bare Base would crash on a paren-less INSERT)
• $lastIdLookup() — per-engine retrieval (Base/MySQL/H2: LAST_INSERT_ID(); SQLite: last_insert_rowid(); PostgreSQL: pg_get_serial_sequence + currval; CockroachDB: driver key / RETURNING resultset, never round-trips; MSSQL and Oracle as above)

Also adds $isBoxLangEngine() on Base so adapter test doubles can exercise the BoxLang-conditional branches from any engine — first-ever Lucee-side coverage of the BoxLang column-parse branch.

4. #2961 section B (same files)

• DA1: paginated SQL Server queries no longer drop their GROUP BY clause (local.afterWhere = ... clobbered the just-built GROUP BY prefix; now &=)
• DA5: Oracle $randomOrder() returns DBMS_RANDOM.VALUE instead of RANDOM() (not an Oracle function — ORA-00904)
• DA14: the $stripIdentifierQuotes(local.thirdSelect) recompute is hoisted out of the per-ORDER-BY-column loop (O(n²) → O(n)), with the stripped list kept in sync on append; a behavior-pin spec captures the exact pre-change pagination output for order columns absent from the select

Behavior change to note

Base/SQLite/MSSQL/Oracle previously ran a pointless identity-lookup query on bulk insertAll/upsertAll paths (empty $primaryKey hint); the guard PostgreSQL already had is hoisted into the Base template, so all adapters now skip it. No caller consumes the value on that path (bulk.cfc discards the $querySetup return).

Tests

17 new adapter-unit specs (no live DB needed — adapters/test-doubles instantiated directly, following the existing MicrosoftSQLServerUnitSpec/OracleUnitSpec pattern), plus three reusable probes under vendor/wheels/tests/_assets/adapters/ (BaseProbe, MSSQLProbe, OracleProbe) that capture $query() SQL and serve queued mock resultsets.

All specs were written first and confirmed red for the right reasons (GROUP BY missing from paginated SQL, no same-batch append, RANDOM(), MAX(ROWID)-first, missing $parseInsertColumnList, bulk-path query executed) before the implementation landed.

Lucee 7 + SQLite (docker harness)

• Baseline (develop + red specs only): 4294 pass / 13 fail / 3 error — of which 12 fails are the pre-existing wheels.tests.specs.internal.testClientSpec HTTP-self-request environment failures (status 0 — the docker-worktree harness can't self-connect; present before any source change), and 1 fail + 3 errors were my deliberately-red specs
• Final full suite: 4310 pass / 12 fail / 0 error — the 12 are the identical pre-existing testClientSpec env failures; zero new failures, pass count up exactly by the 17 new specs
• wheels.tests.specs.database: 62/62 green; interfaces: 57/57; model (live SQLite CRUD through the new Base template + SQLite hook): 883/883

Adobe CF 2023 + SQLite (docker harness)

• wheels.tests.specs.database: 62/62 green; interfaces: 57/57; model (live SQLite CRUD through the new Base template + SQLite hook): 883/883 — all touched areas, zero fails / zero errors

Live-DB verification gap (stated plainly)

• Oracle and SQL Server have no live local databases — both are covered here by adapter-unit specs only (the proven pattern for these adapters). Oracle is soft-fail in CI.
• The BoxLang × MSSQL same-batch resultset surface is unverifiable locally (whether BoxLang surfaces the trailing batch resultset as the named query result). If it doesn't, the retained @@IDENTITY safety net means behavior degrades exactly to pre-fix — never worse.
• The Oracle CURRVAL discovery couples the adapter to the user_tab_identity_cols catalog view (12c+); pre-12c falls through to the legacy lookup via the try/catch + empty-result path.

Changelog: fragment file changelog.d/db-adapter-identity-fallbacks-and-dedup.fixed.md (no direct CHANGELOG.md edit).

Fixes #2906
Refs #2961 (section B: DA1 / DA5 / DA14 only)

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

This PR fixes three real bugs (MSSQL GROUP BY drop, Oracle race-prone MAX(ROWID), Oracle wrong RANDOM() function), replaces them with correct implementations (same-batch SCOPE_IDENTITY(), CURRVAL via catalog, DBMS_RANDOM.VALUE), and simultaneously deduplicates six divergent $identitySelect copies into a Base template + per-engine hook. The template-method extraction is well-structured, the test-first approach is documented, and the dedup reduces future drift risk considerably. Overall verdict: comment — two minor findings, no blocking correctness or cross-engine issues.

---

Correctness

Test-key mismatch risk in Oracle specs (verified clean)

The new OracleUnitSpec.cfc tests assert expect(rv).toHaveKey("lastId"). Base's $identitySelect template stores the returned value under $generatedKey(), so those assertions are only correct if OracleModel.$generatedKey() returns "lastId". Confirmed it does (line 129 of the unchanged file), and MSSQL's "identitycol" (line 278) is similarly consistent with the spec assertions. No action needed — documenting the verification.

GROUP BY fix is unambiguously correct

vendor/wheels/databaseAdapters/MicrosoftSQLServer/MicrosoftSQLServerModel.cfc, line 273 (diff):

// Before — overwrites the GROUP BY just built:
local.afterWhere = "ORDER BY " & local.thirdOrder & ...

// After — appends:
local.afterWhere &= "ORDER BY " & local.thirdOrder & ...

The &= fix is minimal and correct. The behavior-pin spec that captures the exact pre-change SQL output for order-columns-absent-from-select is a good non-regression anchor.

---

Tests

Coverage gap — PK-in-column-list skip path not tested

vendor/wheels/tests/specs/database/AdapterIdentityTemplateSpec.cfc exercises the bulk-path guard (empty primaryKey → void) and the normal path (key absent from column list → LAST_INSERT_ID). The third guard in the Base template — PK present in the column list → return void without querying — has no spec:

// Base.cfc ~line 104
if (ListFindNoCase($parseInsertColumnList(local.sql), ListFirst(arguments.primaryKey))) {
    return;
}

This is the most common path in production (explicit PK on every INSERT). A test like the following would complete the coverage and confirm no spurious query fires on this path:

it("returns void when the primary key is explicitly included in the INSERT", () => {
    var probe = CreateObject("component", "wheels.tests._assets.adapters.BaseProbe");
    var rv = probe.$identitySelect(
        queryAttributes = {},
        result = {sql: "INSERT INTO users (id, firstname) VALUES (99,'x')"},
        primaryKey = "id",
        returningIdentity = ""
    );
    expect(IsNull(rv)).toBeTrue();
    expect(ArrayLen(probe.capturedSql)).toBe(0);
});

---

Cross-Engine

$parseInsertColumnList — whitespace handling diverges between the two branches

vendor/wheels/databaseAdapters/Base.cfc, lines 46–48 (diff):

// Non-BoxLang — strips EVERY space character in the column string:
local.columnList = ReplaceList(local.rawColumns, "#Chr(10)#,#Chr(13)#, ", ",,");

// BoxLang — only removes whitespace surrounding commas:
local.columnList = REReplace(local.rawColumns, "\s*,\s*", ",", "all");
local.columnList = REReplace(local.columnList, "[\r\n]", "", "all");
local.columnList = Trim(local.columnList);

The non-BoxLang ReplaceList strips every space in the raw column string, including spaces embedded within quoted identifiers (e.g. [first name] → [firstname]). The BoxLang regex path only removes whitespace surrounding commas, so [first name] survives as [first name]. After $stripIdentifierQuotes strips brackets, the results differ (firstname vs first name).

In practice, primary-key column names never contain spaces, so the divergence is harmless today. Since this consolidation was specifically intended to remove the per-adapter copies and unify the behavior, it is worth making the two branches equivalent. The BoxLang approach (strip whitespace around commas + trim) is the safer unification target because it does not destroy content inside identifier delimiters.

---

Docs

Changelog fragment changelog.d/db-adapter-identity-fallbacks-and-dedup.fixed.md is present; no direct [Unreleased] edit in CHANGELOG.md. Fragment format and placement are correct.

---

Commits

Single commit fix(model): MSSQL same-batch and Oracle CURRVAL identity fallbacks plus $identitySelect dedup. Header is 91 characters (under 100), type fix is in the allowlist, sentence case, not ALL-CAPS. The scope model is slightly imprecise for changes entirely in vendor/wheels/databaseAdapters/; adapters or db would be more descriptive, but scopes are unrestricted per commitlint.config.js so this is not a violation.

[wheels-bot]

Wheels Bot -- Reviewer B (round 1)

A review is substantively accurate -- both findings are real, no sycophancy, no false positives. The Oracle dollar-generatedKey line-129 claim was verifiable and correct. My only dispute is with the verdict: the findings collectively warrant request-changes rather than comment, for the reasons below.

Sycophancy

None detected. A opened with two concrete findings and used comment rather than approve -- appropriate for a PR with outstanding actionable items.

False positives

None detected.

• PK-in-column-list test gap: verified against the spec diff. AdapterIdentityTemplateSpec tests the bulk-path guard (empty primaryKey) and the normal LAST_INSERT_ID path, but has no spec for the ListFindNoCase short-circuit in the Base template. That guard now applies to every adapter. A's suggested test is correctly formed and matches the existing IsNull() pattern used in the bulk-path spec.

• parseInsertColumnList whitespace divergence: verified in vendor/wheels/databaseAdapters/Base.cfc. The non-BoxLang branch strips every space character (via ReplaceList) while the BoxLang regex path only removes whitespace around commas. A's example ([first name] collapses to [firstname] on non-BoxLang but is preserved on BoxLang) is correct. A accurately labels it "harmless today" for PK columns.

Missed issues

None beyond what A raised.

One minor note: A's "Correctness" section asserts Oracle's generatedKey() returns "lastId" "at line 129 of the unchanged file" without quoting the code. The claim is accurate (confirmed at vendor/wheels/databaseAdapters/Oracle/OracleModel.cfc line 129), but asserting facts about unshown code without citation is a weak verification posture. Negligible risk here since the tests would fail if wrong, but worth noting as a pattern.

Verdict alignment

A used comment but the preamble states "no blocking correctness or cross-engine issues" while Finding 2 describes a cross-engine behavioral inconsistency introduced by this PR. That is a contradiction. The dedup's own stated goal was to unify behavior -- that goal is not fully met when the two parsing branches produce different output for identifiers with spaces. A's own framing in Finding 2 reads like a request-changes rationale ("since this consolidation was specifically intended to remove the per-adapter copies and unify the behavior, it is worth making the two branches equivalent"). A verdict of comment undersells that.

Finding 1 (missing test for the most common production INSERT path, where the PK is explicit) is also stronger than a non-blocking comment. This is a new Base template that now runs for every adapter on every INSERT; leaving the common-path guard untested before merge is a real gap.

Convergence

Not aligned. A's two findings are correct, but the verdict should be request-changes. Specifically, A should:

1. Require the PK-in-column-list spec before merge -- not as a suggestion. This is the default path for most application INSERTs and it is now handled by the Base template for all six adapters.
2. Clarify whether the parseInsertColumnList whitespace divergence is a required fix in this PR or an explicitly-deferred follow-up (with a tracking issue). The current framing leaves it ambiguous. If deferred, a code comment and a tracking issue number would satisfy the stated goal -- but that needs to be an explicit decision, not an implied one from a non-blocking verdict.

Once A upgrades to request-changes and addresses those two points, B can converge.

[wheels-bot]

Wheels Bot — Reviewer A response (round 1)

B's critique is correct on the verdict. I concede both points: Finding 1 warrants blocking (the PK-in-column-list guard is the default execution path for most production INSERTs and is now untested in the new Base template), and my preamble's "no blocking correctness or cross-engine issues" directly contradicts my own Finding 2. The comment verdict was underselling the weight of what I flagged. My updated position is request-changes.

Conceded points

• Verdict should be request-changes: B correctly identifies that my preamble ("no blocking correctness or cross-engine issues") contradicts Finding 2 (a cross-engine behavioral inconsistency in $parseInsertColumnList). My framing of Finding 2 reads like a request-changes rationale — "it is worth making the two branches equivalent" — yet I applied a non-blocking verdict. The contradiction is mine to own.

• Finding 1 is blocking, not a suggestion: B is right that the PK-in-column-list guard in the Base template fires on most application INSERTs (explicit PK in the column list is the norm), and the guard now applies to all six adapters via the template. Leaving the most-traveled branch untested before merge is a real gap. The test I proposed in my initial review (BaseProbe, INSERT with id in the column list, assert IsNull(rv) and ArrayLen(probe.capturedSql) == 0) is correctly formed and should be required.

Defended points

• Finding 2 framing is accurate: The whitespace divergence between the BoxLang regex branch and the non-BoxLang ReplaceList branch was pre-existing in MSSQL's own copy before this PR. The PR's consolidation into Base promoted it from a per-adapter quirk to a shared behavior — which makes the divergence more visible and more worth resolving, but did not introduce it from scratch. B agrees the finding is real. My "harmless today for PK columns" caveat stands. B's ask to make the decision explicit — fix in this PR or add a comment with a tracking issue number — is the right framing, and that is what I am requiring.

• Oracle $generatedKey() verification posture: B's minor note (asserting a fact about line 129 without quoting the code) is a fair observation about verification hygiene, not a reversal of the correctness finding. Noted and agreed.

Updated verdict

Request-changes: the PR needs two small, targeted additions before merge — (1) a spec in AdapterIdentityTemplateSpec for the PK-in-column-list short-circuit path (the ListFindNoCase guard at Base.cfc line ~104), and (2) an explicit resolution of the $parseInsertColumnList whitespace divergence: either unify the two branches (BoxLang regex approach is the safer target, per my initial review) or add a code comment referencing a filed tracking issue number.