fix(controller): namespace pagination handles under request.wheels.$pagination - #3340
Conversation
…agination Pagination handles are caller-supplied names, and `setPagination()` / `pagination()` wrote them straight into `request.wheels`. That put arbitrary user input in the same case-insensitive keyspace as framework-owned request state, and the collision ran both ways. Write direction: `setPagination(handle="tenant")` replaced the resolved tenant context with a pagination struct, dropping tenant datasource routing for the rest of the request. `handle="$queryCache"` did the same to the per-request finder cache namespace added in #3336. Neither is validated, so both were silent. Read direction: `pagination()` only checks that a handle exists when `showErrorInformation` is on. In production an unknown handle that happened to name a framework key returned that key's struct as though it were pagination data, rather than failing. The exposure is wider than the tenant case that prompted the issue. A scan of dot- and bracket-notation access shows `request.wheels` currently holds around thirty-five framework-owned keys — `params`, `execution`, `cycle`, `currentRoute`, `transactions`, `flashKeep`, `exception`, `controller`, `action` and more — every one of them reachable by a matching handle. Handles now live under the reserved `request.wheels.$pagination` sub-struct, so they can only ever resolve inside their own namespace. Same fix shape as #3336. `Wheels.QueryHandleNotFound` behaviour is unchanged. Spec sites that read the key directly are repointed. Note that the whole core suite runs inside one request, so `request.wheels` is shared across spec files: a teardown that deletes the `$pagination` namespace wholesale destroys handles belonging to other specs. Teardowns therefore remove only their own handles — a hazard the flat layout did not have, since each handle was its own key. 7 regression specs, written red first and verified: reverting only the two call sites while keeping the helper and the specs fails all four collision specs for the right reasons, including the tenant key coming back holding `[totalRecords, currentPage, perPage, TOTALPAGES, STARTROW, MAXROWS, ENDROW]`. Lucee 7 + SQLite full core suite: 4717 passed on develop, 4724 on this branch. Closes #3339 Signed-off-by: Peter Amiri <peter@alurium.com>
![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
TL;DR — This PR closes a real request-scope collision: pagination handles are caller-supplied names that were written straight into request.wheels, colliding case-insensitively with framework-owned keys (tenant, $queryCache, params, …) in both the write and read directions. Moving them under the reserved request.wheels.$pagination sub-struct — the same shape merged for the finder cache in #3336 — is the right fix, it's complete, and it's covered red-first. Verdict: approve.
Correctness
The fix is complete. I traced every pagination read/write in the framework:
- Writes go through
setPagination()(Global.cfc:3949), called byread.cfc:204(findAll) and by users directly. ✅ namespaced. - Reads go through
pagination()(Global.cfc:3860), called by every view helper (view/pagination.cfc×7,view/links.cfc:274-275). No helper readsrequest.wheels[handle]directly, so there are no orphaned readers left on the old layout.
$ensurePaginationStore() is correctly called at the top of pagination() too, not just setPagination() — without it, a pagination() call before any setPagination() would throw on StructKeyExists(request.wheels["$pagination"], …) because the sub-struct wouldn't exist yet. Good defensive ordering.
The production read path (showErrorInformation=false) now throws a raw "key doesn't exist" for a genuinely-unknown handle instead of silently returning a foreign framework struct — that matches the pre-fix behaviour for truly-unknown handles while removing the cross-over, which is exactly the intended improvement. Verified against paginationHandleCollisionSpec.cfc:82.
Cross-engine
Clean. $ensurePaginationStore() is public with a $ prefix (Invariant 7 for mixin helpers in Global.cfc), uses StructKeyExists + bracket-notation $-keys, declares no parameters (so Invariant 15's request-parameter hazard doesn't apply), and contains no closures. Matches the switchTenant/$tenantDataSource idioms already in the file.
Tests
Strong. paginationHandleCollisionSpec.cfc covers both directions (tenant + $queryCache write clobbering; production read cross-over), round-tripping, Wheels.QueryHandleNotFound in dev, and handle isolation. Fixtures check out — $tenantDataSource() (Global.cfc:757) reads request.wheels.tenant.dataSource, matching the struct the spec plants, and the author/Djurner rows exist in populate.cfm.
The shared-request teardown hazard the PR body calls out is handled correctly: requestSpec.cfc, miscellaneousSpec.cfc, and the new spec all delete only their own handles rather than wiping the shared $pagination namespace — the right call given the core suite runs in one request.
Docs
Changelog fragment present and correctly typed (changelog.d/3339-pagination-handle-namespace.fixed.md). The pagination()/setPagination() reference docs describe the public API, not the internal storage key, so no doc change is needed.
Non-blocking observation
The out-of-scope call-out for model/callbacks.cfc:331 (request.wheels[<queryHash>] = modelName) is sound — I confirmed $hashedKey(arguments.collection) has only that one write site and no reader, and after this change nothing reads a hash-shaped key back out for pagination. Leaving it for a separate change rather than bundling a framework-code removal is the correct judgment.
Wheels Test Results 32 files ± 0 8 004 suites - 1 712 17m 27s ⏱️ - 1m 17s For more details on these failures and errors, see this check. Results for commit 8616213. ± Comparison against base commit af69eb5. This pull request removes 23331 and adds 161 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
The #3339 specs called `application.wo.$ensurePaginationStore()` / `g.$ensurePaginationStore()` as bare statements. Adobe CF 2025's parser rejects a zero-argument call on a multi-level dotted reference in statement position, throwing at COMPILE time: coldfusion.compiler.CFMLParserBase$MissingNameException Invalid construct: Either argument or name is missing. Because the core suite compiles via `directory="wheels.tests.specs"`, that one construct zeroed out the whole adobe2025 leg: matrix run 30809467041 reported tests="0" for all six databases, while lucee6, lucee7, adobe2023 and boxlang each gained the expected +7 tests with failure and error counts identical to develop. Adobe attributes the error to the enclosing `describe(...)` line rather than the offending statement, which is why it read like a broken test-block signature. Isolated by bisecting a single-bundle probe against a local adobe2025 container: the same call with any argument (`application.wo.$get("showErrorInformation")`) compiles, and a zero-arg dotted call nested inside another call (`expect(application.wo.$statusCode()).toBe(418)`) is long-standing prior art. Specs now ensure the namespace inline. `$ensurePaginationStore()` returns the namespace struct so framework callers use the returned reference instead of a bare statement, and `pagination()` / `setPagination()` read and write through it. Documented as cross-engine invariant 16. Also drops a stray backslash from a spec name — CFML has no backslash string escapes, so "named \$queryCache" rendered the backslash literally. Verification, local containers, full core suite: lucee7 + sqlite 4724 pass, 0 fail, 0 error (4742 specs) adobe2025 + sqlite 4715 pass, 8 fail, 1 error (4742 specs, compiles) adobe2025 + sqlite 4693 pass, 21 fail, 3 error on develop in the same container The remaining adobe2025 failures are pre-existing and unrelated to pagination (csrf cookie, provides, rendering layouts, migrator typed defaults, storage, route tester); the branch has fewer of them than develop does in the same container, so they are environment/order noise. Refs #3339 Signed-off-by: Peter Amiri <peter@alurium.com>
The first wording claimed any zero-argument call on a multi-level dotted reference breaks Adobe 2025. That overclaims: cachingSpec, SessionStrategySpec and configSuperWarningSpec all have bare zero-arg dotted calls in closures that compile fine (`_controller.$clearCachableActions()`, `strategy.logout()`, `local.c.$warnIfConfigSkipsSuper()`), and RoutePrecedenceSpec chains from `application.wo.mapper()`. The trigger I actually verified is narrower: zero args routed through the `application` scope, in statement or whole-RHS position. Lists the compiling counter-examples explicitly so nobody "fixes" working code, and adds the single-probe bisect recipe. Refs #3339 Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — Re-review at head e475c283. This PR closes a real request-scope collision: pagination handles are caller-supplied names that were written straight into request.wheels, colliding case-insensitively with framework-owned keys (tenant, $queryCache, params, …) in both the write and read directions. Confining them to the reserved request.wheels.$pagination sub-struct — the same shape merged for the finder cache in #3336 — is the right fix, it's complete, and it's covered red-first. The second commit (fix(test): keep Adobe 2025 compiling the pagination namespace specs) correctly resolves the Adobe 2025 parser regression that got the prior approval dismissed. Verdict: approve.
Correctness
The fix is complete — every pagination read/write in the framework is namespaced:
- Writes go through
setPagination()(Global.cfc:3954), called byread.cfc:204(findAll) and by users directly. ✅ - Reads go through
pagination()(Global.cfc:3850), called by every view helper (view/pagination.cfc×7,view/links.cfc:274-275). No helper readsrequest.wheels[handle]directly, so no orphaned readers remain on the old layout.
$ensurePaginationStore() is correctly called at the top of pagination() (Global.cfc:3850), not just setPagination() — without it a pagination() call before any setPagination() would throw on the StructKeyExists(local.store, …) check because the sub-struct wouldn't exist. Good defensive ordering.
The production read path (showErrorInformation=false) now throws a raw "key doesn't exist" for a genuinely-unknown handle instead of silently returning a foreign framework struct — matching pre-fix behaviour for truly-unknown handles while removing the cross-over. Verified against paginationHandleCollisionSpec.cfc:82.
Cross-engine
Clean, and the Adobe 2025 delta is the point of the re-review.
$ensurePaginationStore() is public with a $ prefix (Invariant 7 for Global.cfc mixin helpers), uses StructKeyExists + bracket-notation $-keys, declares no parameters (Invariant 15 N/A), and has no closures.
The second commit resolves Invariant 16 (zero-arg dotted call in statement position breaks Adobe 2025's parser) two ways, both verified:
- Framework code calls the helper as a bare call assigned to a local (
local.store = \$ensurePaginationStore();,Global.cfc:3850/3954) — not aapplication.wo.\$…()dotted statement, so the parser hazard never applies here. - Test specs (
requestSpec.cfc:159,view/miscellaneousSpec.cfc:258,paginationHandleCollisionSpec.cfc:36) ensure the namespace inline (if (!StructKeyExists(request.wheels, "\$pagination")) { request.wheels["\$pagination"] = {} }) rather than calling the void helper throughapplication.wo— the construct that would have zeroed the whole adobe2025 leg. The remainingg.\$tenantDataSource()inpaginationHandleCollisionSpec.cfc:76is nested insideexpect(...), which the invariant confirms is safe.
Tests
Strong. paginationHandleCollisionSpec.cfc covers both directions (tenant + $queryCache write clobbering; production read cross-over), round-tripping, Wheels.QueryHandleNotFound in dev, and handle isolation. Fixtures check out — $tenantDataSource() (Global.cfc:757) reads request.wheels.tenant.dataSource, matching the struct the spec plants at line 68.
The shared-request teardown hazard is handled correctly: requestSpec.cfc, model/miscellaneousSpec.cfc, and the new spec all delete only their own handles rather than wiping the shared $pagination namespace — right call given the core suite runs in one request.
Docs
Changelog fragment present and correctly typed (changelog.d/3339-pagination-handle-namespace.fixed.md, fixed ∈ allowed set). CLAUDE.md gains Invariant 16 documenting the Adobe 2025 parser gotcha. The pagination()/setPagination() reference docs describe the public API, not the storage key, so no doc change is needed there.
Commits
Both conform to commitlint: fix(controller): and fix(test): — valid types, valid scopes, subjects within length and not ALL-CAPS.
Non-blocking observation
The out-of-scope call-out for model/callbacks.cfc:331 (request.wheels[<queryHash>] = modelName) is sound — $hashedKey(arguments.collection) has only that one write site and no reader, and after this change nothing reads a hash-shaped key back out for pagination. Leaving that framework-code removal for a separate change is the correct judgment.
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — Re-review at head 55b8873. This PR closes a real request-scope collision (#3339): pagination handles are caller-supplied names that were written straight into request.wheels, colliding case-insensitively with framework-owned keys like tenant and $queryCache in both the write and read directions. Handles now live under a reserved request.wheels.$pagination sub-struct — the same fix shape as #3336. The functional change was already approved at e475c28; the only new commit here is docs: tighten cross-engine invariant 16 to the verified boundary, a CLAUDE.md refinement I verified is accurate. Verdict: approve.
Correctness
The namespacing is complete — both directions are actually closed, not just the tenant symptom:
pagination()reads only from the returned store:local.store = $ensurePaginationStore(); … return local.store[arguments.handle];(vendor/wheels/Global.cfc:3850,3886).setPagination()writes only into it:local.store = $ensurePaginationStore(); local.store[arguments.handle] = local.args;(vendor/wheels/Global.cfc:3954-3955).- Every downstream reader goes through
pagination()—view/pagination.cfc(lines 20, 66, 112, 158, 204, 261, 493) andview/links.cfc:274-275— so no view helper reads a flatrequest.wheels[handle]that this PR left behind. - The one remaining write site,
model/callbacks.cfc:331, keys on a query hash (not a handle) and is explicitly scoped out in the PR body as a separate dead-code question. That deferral is correct: a hash-shaped key cannot collide with$pagination, and I confirmed$hashedKey(arguments.collection)has no matching reader.
$ensurePaginationStore() is idempotent and guards both levels (request.wheels then request.wheels["$pagination"], Global.cfc:3879-3887), so it is safe on a cold request.
Cross-engine
Clean, and the fix is itself shaped around a cross-engine constraint:
$ensurePaginationStore()ispublicwith a$prefix (Global.cfc:3879) — satisfies invariant 7 (private mixin functions are not integrated onto model/controller objects).- The helper deliberately returns the store so callers never invoke it as a bare
application.wo.$ensurePaginationStore()statement — the invariant-16 Adobe 2025 parser trap. Test specs (paginationHandleCollisionSpec.cfc:31-34,requestSpec.cfc:157-161,view/miscellaneousSpec.cfc:258-262) correctly ensure the namespace inline instead. - The collision spec
expect(g.$tenantDataSource()).toBe("tenant_acme")(paginationHandleCollisionSpec.cfc:91) is a zero-arg call throughapplication.wo, but nested insideexpect(...)— the verified-safe boundary the docs call out, so it compiles. - The tenant assertion uses
IsDefined("request.wheels.tenant")(paginationHandleCollisionSpec.cfc:88), matching invariant 15 guidance.
Invariant 16 refinement is verified accurate — each cited counter-example exists in the repo: _controller.$clearCachableActions() (cachingSpec.cfc:9), strategy.logout() (SessionStrategySpec.cfc:128), local.c.$warnIfConfigSkipsSuper() (configSuperWarningSpec.cfc:82), application.wo.mapper()... (RoutePrecedenceSpec.cfc:16), and expect(application.wo.$statusCode()).toBe(418) (renderingSpec.cfc:191). The tightened wording is a net improvement — it stops future agents from "fixing" working code.
Tests
vendor/wheels/tests/specs/controller/paginationHandleCollisionSpec.cfc (new, BDD extending wheels.WheelsTest) covers both directions: write-clobber of tenant/$queryCache, the production read path with showErrorInformation off, dev-mode Wheels.QueryHandleNotFound, round-tripping, and handle isolation. The shared-request teardown hazard is handled correctly — teardowns delete only their own handles (ownHandles list, lines 51-53) rather than wiping the shared $pagination namespace. The migrated specs (crudSpec, model/miscellaneousSpec, view/miscellaneousSpec, requestSpec) all track the new key path.
Docs
Changelog fragment present and well-formed: changelog.d/3339-pagination-handle-namespace.fixed.md (<slug>.fixed.md, valid fixed type) — no direct CHANGELOG.md [Unreleased] edit. CLAUDE.md invariant 16 updated with a verified boundary list and a bisect recipe.
Commits
All three conform to commitlint.config.js: fix(controller):, fix(test):, docs: — valid types, subjects well under 100 chars, DCO sign-off present.
Nothing blocking. Nice work — the red-first verification (reverting only the two call sites) and the explicit scoping-out of the callbacks.cfc hash-key question are exactly the right rigor for a request-scope change.
1 participant
Fixes #3339.
The collision
setPagination()/pagination()key on a caller-supplied handle name (default"query"), written straight intorequest.wheels. That put arbitrary user input in the same case-insensitive keyspace as framework-owned request state, and it ran both ways.Write direction —
setPagination(handle="tenant")replaced the resolved tenant context with a pagination struct, dropping tenant datasource routing for the rest of the request.handle="$queryCache"did the same to the finder cache namespace from #3336. Neither is validated, so both were silent.Read direction —
pagination()only checks that a handle exists whenshowErrorInformationis on. In production, an unknown handle that happened to name a framework key returned that key's struct as though it were pagination data.Handles now live under the reserved
request.wheels.$paginationsub-struct. Same fix shape as #3336.Wheels.QueryHandleNotFoundbehaviour is unchanged.The exposure is wider than the tenant case
Scanning both dot- and bracket-notation access (my first pass only caught brackets and missed 29 sites),
request.wheelscurrently holds ~35 framework-owned keys — every one reachable by a matching handle:pagination(handle="params")returning the params struct is the same bug as the tenant case, just less obviously fatal.Tests
vendor/wheels/tests/specs/controller/paginationHandleCollisionSpec.cfc— 7 specs covering both directions plus round-tripping and handle isolation.Written red first and verified precisely: reverting only the two call sites while keeping the helper and the specs fails all four collision specs for the right reasons. The tenant one is the clearest — after
setPagination(handle="tenant")the tenant key comes back holding pagination fields:(A full revert isn't a useful red here — the helper disappears and every spec errors in
beforeEach, which proves nothing.)A hazard the namespacing introduces
The whole core suite runs inside one request, so
request.wheelsis shared across spec files. Under the flat layout each handle was its own key, so a teardown could safely delete it. Now they share one struct, andStructDelete(request.wheels, "$pagination")in a teardown destroys other specs' handles.I hit this: my first run showed 6 unrelated pagination specs failing across
crudSpecandview/miscellaneousSpec. Teardowns now remove only their own handles. Worth knowing for anyone adding pagination specs later.Verification
Lucee 7 + SQLite,
bash tools/test-local.sh(full suite):develop@af69eb5Delta is exactly the 7 new specs.
Cross-engine matrix dispatched manually (it doesn't run on PRs — weekly cron +
workflow_dispatch,continue-on-error: true). Will post the per-engine diff against thedevelopbaseline as a comment. Given #3338's Adobe 2025 regression was invisible to every PR check, that's the gate for this one too.Constructs are the same
$ensure…shape merged in #3338 —StructKeyExists, bracket-notation$-keys, no closures, no reserved-scope parameter names.Global.cfchas no function declaring arequestparameter, so cross-engine invariant 15 doesn't apply here.Not included
model/callbacks.cfc:331writesrequest.wheels[<queryHash>] = modelName— the third dynamic-key family. A repo-wide search finds no reader:$hashedKey(arguments.collection)appears only at that write site, and nothing else reads a hash-shaped key out ofrequest.wheels. Its comment says the value exists "so that we can find it with pagination", which after this change is definitively not what happens. It looks like dead code, but removing framework code on that basis belongs in its own change — flagging rather than bundling.