From 2ffb099338c8fc74877d7785f1c112b37aeda68a Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Sat, 16 May 2026 20:40:14 -0700 Subject: [PATCH 1/2] fix(test): work around BoxLang catch-scope discard in lockingSpec (#2744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lockingSpec :: "releases lock even when callback throws an exception"` failed on BoxLang × MySQL / Postgres / SQLite — the missing leg of #2665 that #2670 intentionally deferred. The test tracked exception propagation through `local.exceptionThrown = true` inside the `catch` block; on BoxLang, writes to the `local` scope inside a catch don't survive past the block (the catch body runs under a nested `local` that gets discarded on exit), so the post-catch `expect(local.exceptionThrown)` read the un-touched outer value and failed with "Expected [false] to be true". Lucee 6 / 7 and Adobe CF 2023 / 2025 share the enclosing function's `local` across the catch, so the assignment stuck on every other engine and the gap was BoxLang-only. Switched to the same struct-field pattern TenantResolverSpec already uses for the equivalent assertion: var state = {exceptionThrown = false}; try { ... } catch (TestException e) { state.exceptionThrown = true; } expect(state.exceptionThrown).toBeTrue(); Struct field assignment targets a heap object and survives the scope transition on every engine. `vendor/wheels/model/locking.cfc` is unchanged — the lock-release contract was already correct via the existing `try { callback() } finally { release }`; the lock-release bug the issue title called out turned out not to exist. Documented the BoxLang catch-scope quirk in `.ai/wheels/cross-engine-compatibility.md` so future spec authors don't re-hit it. Both the `var struct.field` form and the `var bareName` form are called out as portable; the `local.X = ...` form is called out as the trap. Fixes #2744 Signed-off-by: Peter Amiri --- .ai/wheels/cross-engine-compatibility.md | 37 +++++++++++++++++++ CHANGELOG.md | 1 + .../wheels/tests/specs/model/lockingSpec.cfc | 14 +++++-- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/.ai/wheels/cross-engine-compatibility.md b/.ai/wheels/cross-engine-compatibility.md index 5eb6e32d5c..bbf9c18b83 100644 --- a/.ai/wheels/cross-engine-compatibility.md +++ b/.ai/wheels/cross-engine-compatibility.md @@ -221,6 +221,43 @@ expect(found).toBeWheelsModel(); **Reference**: `vendor/wheels/wheelstest/system/Expectation.cfc::toBeWheelsModel`, issue #2662. +### `local.X = ...` Inside `catch` Doesn't Persist (BoxLang) + +Writes to the `local` scope inside a `catch` block don't survive past the block on BoxLang. The catch body apparently runs under a nested `local` that gets discarded when control leaves; on Lucee and Adobe CF the catch shares the enclosing function's `local`, so the assignment sticks. + +```cfm +// WRONG — passes on Lucee/Adobe, fails on BoxLang +local.caught = false; +try { + Throw(type = "TestException", message = "boom"); +} catch (TestException e) { + local.caught = true; // discarded when catch exits on BoxLang +} +expect(local.caught).toBeTrue(); // reads outer local — still false + +// RIGHT — struct field assignment targets a heap object (all engines) +var state = {caught = false}; +try { + Throw(type = "TestException", message = "boom"); +} catch (TestException e) { + state.caught = true; +} +expect(state.caught).toBeTrue(); + +// ALSO RIGHT — var-declared name without `local.` prefix +var caught = false; +try { + Throw(type = "TestException", message = "boom"); +} catch (TestException e) { + caught = true; +} +expect(caught).toBeTrue(); +``` + +**Why this fires only in specs**: production code rarely needs a catch to flip a boolean for a later read in the same function — typical catch blocks rethrow, log, or assign struct fields. Specs that use `try/catch` to *assert* exception propagation are the natural trap, since they need the post-catch flag. + +**Reference**: issue #2744, regression test `vendor/wheels/tests/specs/model/lockingSpec.cfc :: "releases lock even when callback throws an exception"`. The same pattern works in `vendor/wheels/tests/specs/middleware/TenantResolverSpec.cfc` because it tracks state via `var result = {threw = false}; result.threw = true`. + ### Private View Helpers Not Integrated `$integrateComponents()` only copies `public` methods into controllers. Private helper functions in view CFCs are never available. diff --git a/CHANGELOG.md b/CHANGELOG.md index 48d8d77ea9..77fd959f92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo - `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) - `Global.cfc` helpers now copy the `arguments` scope into a plain struct before passing it to `attributeCollection` on the underlying tag, so Adobe CF 2023 no longer rejects every request with `Failed to add HTML header`. Adobe 2023 is stricter than Lucee/BoxLang/Adobe 2021 about the shape passed to `attributeCollection` — the raw `arguments` scope is no longer accepted — which prevented any test request from booting past application init in the compat-matrix Adobe 2023 job (recorded `0 pass / 0 fail / 0 err` because no test endpoint completed its request). `$header()` is the visible blocker on the dispatch path, but the same engine-level restriction applies to every other helper that forwarded the raw scope (`$cache`, `$content`, `$mail`, `$directory`, `$file`, `$invoke`, `$location`, `$htmlhead`, `$wddx`, `$zip`, `$image`, `$dbinfo`), so the fix is applied uniformly across all twelve sites — covering both the string-interpolated form (`attributeCollection = "##arguments##"`) and the CFScript direct-struct form (`attributeCollection = arguments`). `$dbinfo()` rebuilds the local copy before each of its four `cfdbinfo` calls because the catch path mutates the `arguments` scope between calls. The existing `statusText` strip in `$header()` (added for Adobe CF 2025) collapses into the same single unconditional copy. Regression coverage in `vendor/wheels/tests/specs/global/headerSpec.cfc` (#2741) +- `lockingSpec :: "releases lock even when callback throws an exception"` now passes on BoxLang × MySQL/Postgres/SQLite — the missing leg of issue #2665 that #2670 intentionally deferred. The test was tracking exception propagation through `local.exceptionThrown = true` inside the `catch` block; on BoxLang, writes to the `local` scope inside a catch don't survive past the block (the catch body runs under a nested `local` that gets discarded on exit), so the post-catch `expect(local.exceptionThrown).toBeTrue()` always read the un-touched outer value and failed with "Expected [false] to be true". Switched to the same struct-field pattern `TenantResolverSpec` already uses for the equivalent assertion (`var state = {exceptionThrown = false}; ... state.exceptionThrown = true;`), which targets a heap object and survives the scope transition on every engine. `vendor/wheels/model/locking.cfc` is unchanged — the lock-release contract was already correct via the existing `try { callback() } finally { release }`. Adjacent to #2743/#2746 (CockroachDB advisory-lock skip) but a different fix shape — that PR fixed the capability flag, this one fixes the spec's BoxLang-incompatible state-tracking. New cross-engine compatibility doc entry covers the BoxLang catch-scope quirk so future spec authors don't re-hit it (#2744) - `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/tests/specs/model/lockingSpec.cfc b/vendor/wheels/tests/specs/model/lockingSpec.cfc index 0c666a71ab..c24ce655ab 100644 --- a/vendor/wheels/tests/specs/model/lockingSpec.cfc +++ b/vendor/wheels/tests/specs/model/lockingSpec.cfc @@ -30,15 +30,23 @@ component extends="wheels.WheelsTest" { }) it("releases lock even when callback throws an exception", () => { - local.exceptionThrown = false; + // Track state via a struct field, not `local.X`. On BoxLang, + // `local.X = value` inside a `catch` block writes to a + // catch-scoped local that gets discarded when the block exits, + // so a subsequent `expect(local.X)` reads the un-touched outer + // value. Struct field assignment (`state.X = ...`) targets a + // heap object and survives the scope transition on every + // engine. Same pattern as TenantResolverSpec's `result.threw`. + // Regression: issue #2744. + var state = {exceptionThrown: false}; try { g.model("author").withAdvisoryLock(name="test_lock_2", callback=function() { Throw(type="TestException", message="deliberate error"); }); } catch (TestException e) { - local.exceptionThrown = true; + state.exceptionThrown = true; } - expect(local.exceptionThrown).toBeTrue(); + expect(state.exceptionThrown).toBeTrue(); // Verify the lock was released by successfully acquiring it again local.result = g.model("author").withAdvisoryLock(name="test_lock_2", callback=function() { From dbef5e9f73d1e3e4c316728047476832150dc3c3 Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Sun, 17 May 2026 06:09:56 -0700 Subject: [PATCH 2/2] docs(claude): address Reviewer A nits on #2744 (CLAUDE.md invariant + doc + spec) - Collapse the 8-line in-spec comment to one line. The `.ai/` doc carries the full WHY; the spec only needs the pointer. Matches CLAUDE.md's "one short line max" rule for inline comments. - Switch struct literal in `lockingSpec.cfc` from colon (`:`) to equals (`=`) syntax to match the prior-art pattern in `TenantResolverSpec` (`var result = {threw = false}`) and the WRONG/RIGHT examples in the cross-engine doc. Both are valid CFML; consistency wins. - Add a "Why the bare-`var` form survives" paragraph to the `.ai/wheels/cross-engine-compatibility.md` BoxLang catch-scope section so future readers don't ask why `var caught; caught = true` survives when `local.caught = true` doesn't. Also recommends preferring the struct-field form since the bare-var mechanism may not be a stable contract across BoxLang releases. - Add invariant #10 to CLAUDE.md's "Cross-Engine Invariants" checklist. CLAUDE.md is the canonical drive-by checklist; spec authors who stop there need this rule visible at the surface level. One-liner pattern matches the existing 9 invariants. Signed-off-by: Peter Amiri --- .ai/wheels/cross-engine-compatibility.md | 2 ++ CLAUDE.md | 1 + vendor/wheels/tests/specs/model/lockingSpec.cfc | 11 ++--------- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/.ai/wheels/cross-engine-compatibility.md b/.ai/wheels/cross-engine-compatibility.md index bbf9c18b83..6125cad535 100644 --- a/.ai/wheels/cross-engine-compatibility.md +++ b/.ai/wheels/cross-engine-compatibility.md @@ -254,6 +254,8 @@ try { expect(caught).toBeTrue(); ``` +**Why the bare-`var` form survives**: BoxLang's catch-scoped local only shadows keys written via explicit `local.X = ...`; an unscoped write to a `var`-declared name appears to resolve through the var-declaration slot and escapes the catch-scope shadow. Prefer the struct-field form anyway — it's cleaner, mirrors the prior-art `TenantResolverSpec` pattern, and doesn't rely on this behaviour being preserved across BoxLang releases. + **Why this fires only in specs**: production code rarely needs a catch to flip a boolean for a later read in the same function — typical catch blocks rethrow, log, or assign struct fields. Specs that use `try/catch` to *assert* exception propagation are the natural trap, since they need the post-catch flag. **Reference**: issue #2744, regression test `vendor/wheels/tests/specs/model/lockingSpec.cfc :: "releases lock even when callback throws an exception"`. The same pattern works in `vendor/wheels/tests/specs/middleware/TenantResolverSpec.cfc` because it tracks state via `var result = {threw = false}; result.threw = true`. diff --git a/CLAUDE.md b/CLAUDE.md index 386f3b412e..1b8e932bc7 100755 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,6 +46,7 @@ The framework must run on Lucee 5/6/7, Adobe CF 2018/2021/2023/2025, and BoxLang 7. **`private` mixin functions are not integrated.** `$integrateComponents()` only copies `public` methods into model/controller objects. ALL helpers in `vendor/wheels/model/*.cfc`, view helpers, etc. MUST use `public` access with `$` prefix for internal scope. BoxLang passes; Lucee/Adobe fail. 8. **`Left(str, 0)` crashes Lucee 7.** Guard: `len > 0 ? Left(str, len) : ""`. 9. **`toBeInstanceOf("component")` fails on BoxLang** — returns the FQN, not the literal `"component"`. Use `toBeWheelsModel()` for finder results. +10. **`local.X = ...` inside `catch` doesn't persist on BoxLang.** Catch body runs under a nested `local` that gets discarded on exit, so `expect(local.X)` after the catch reads the un-touched outer value. Use a struct field: `var state = {flag = false}; ... state.flag = true;`. Bare `var bareName` + unscoped `bareName = true` also works but the struct form mirrors `TenantResolverSpec` and is the prior-art pattern. Verify Adobe CF fixes locally before pushing — don't iterate via CI: ```bash diff --git a/vendor/wheels/tests/specs/model/lockingSpec.cfc b/vendor/wheels/tests/specs/model/lockingSpec.cfc index c24ce655ab..8cbecd2bbc 100644 --- a/vendor/wheels/tests/specs/model/lockingSpec.cfc +++ b/vendor/wheels/tests/specs/model/lockingSpec.cfc @@ -30,15 +30,8 @@ component extends="wheels.WheelsTest" { }) it("releases lock even when callback throws an exception", () => { - // Track state via a struct field, not `local.X`. On BoxLang, - // `local.X = value` inside a `catch` block writes to a - // catch-scoped local that gets discarded when the block exits, - // so a subsequent `expect(local.X)` reads the un-touched outer - // value. Struct field assignment (`state.X = ...`) targets a - // heap object and survives the scope transition on every - // engine. Same pattern as TenantResolverSpec's `result.threw`. - // Regression: issue #2744. - var state = {exceptionThrown: false}; + // BoxLang: local.X inside catch doesn't persist — struct field survives. See .ai/wheels/cross-engine-compatibility.md (#2744). + var state = {exceptionThrown = false}; try { g.model("author").withAdvisoryLock(name="test_lock_2", callback=function() { Throw(type="TestException", message="deliberate error");