fix(controller): rendering residuals C13/C16/C17 and $expandedAssociations metadata lock - #3152
Conversation
…log $includeFile blanked columns Closes the three rendering residuals of the #2961 roll-up (C13/C16/C17): - C17: $getStatusCodes() rebuilt a 63-entry constant struct on every render path. It is now built once per application lifetime and memoized in the application scope together with a deterministic reverse (text-to-code) lookup; $returnStatusCode() reads that lookup instead of running StructFindValue over the rebuilt struct, and duplicated status texts (Unassigned at 427/430/509) deterministically resolve to the lowest code. The numeric branch of $setRequestStatusCode() keeps its validation call but no longer assigns the unused text. - C16: $includeFile() re-ran ListToArray(query.columnList) inside both per-row loops; the column list is constant per query so it is tokenized once above the loops. - C13: the blanket catch that blanked a column $includeFile() could not read now logs a warning (once per column per render) to the wheels log naming the column, partial, first failing row, and underlying error before defaulting to an empty string. Refs #2961 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
…edAssociations The metadata fill-ins above the JOIN-variant memo (foreign/join keys, table name, column/property lists and structs) wrote the shared application-scoped association struct on every call without a lock — the same unlocked-shared-struct pattern #2910 fixed for the JOIN string itself, flagged in the #2952 coverage-audit comment. They are now filled once under the same double-checked named lock (wheelsJoinMemo), with a fill-once marker written last so lock-skipping readers only ever observe a fully populated metadata set. The values are derived solely from class data, so fill-once is equivalent to the previous per-call rewrite. Refs #2952 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Adobe CF validates built-in argument counts and ValueList() operands at COMPILE time, and the core runner compiles every spec in the directory, so each of these crashed the entire Adobe 2023 suite (0 specs run): - miscellaneousSpec.cfc (#3101): DirectoryCreate(path, true) — the createPath boolean is Lucee-only; Adobe's DirectoryCreate takes exactly one parameter. Three call sites, all with existing parents, switched to the single-argument form. - seederSpec.cfc (#3107): ValueList(model(...).findAll(...).id) — Adobe only accepts a plain query.column reference inside ValueList(). The query is assigned to a variable first. Verified: full core suite on Adobe 2023 + SQLite goes from a whole-suite onRequest compile error to 4462 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR closes the three rendering residuals of the #2961 roll-up (C13 logged-blank columns, C16 column-list hoist, C17 $getStatusCodes memoization), locks the context-independent metadata fill-ins in $expandedAssociations (#2952 residual), and fixes two Adobe-only whole-suite compile crashes in recently merged specs. I verified every claim against the checked-out head: the locking mirrors the shipped #2910 joinVariants pattern under the identical lock name, the memoization guard is sound, the fill-once semantics are behaviorally equivalent to the old per-call rewrite, and tests follow the established spec style with proper cleanup. No correctness, cross-engine, or security findings. Verdict: comment — three minor non-blocking notes below.
Correctness (verified, no findings)
vendor/wheels/model/sql.cfc:1296-1339— the double-checked lock uses the same name as the existing JOIN-variant memo lock (wheelsJoinMemo#application.applicationName#, line 1420), so metadata fill and join memoization serialize against each other. The marker-written-last pattern matches the prior art #2910 established.local.class(line 1263) is the owner of the association struct being filled, so the "context-independent" claim holds — for a given association struct, bothlocal.classandlocal.associatedClassare fixed. TheforeignKey/joinKeyfills were already fill-once before this PR (guarded by!Len(...)on a shared struct), so behavior is unchanged there too.vendor/wheels/controller/rendering.cfc:867-960— the memo-hit guard checks bothstatusCodesandstatusCodeLookupbefore returning, and the miss path rebuilds both idempotently, so the lock-free design is sound regardless of write interleaving.$returnStatusText(line 838) and$returnStatusCode(line 854) are the only framework readers and neither mutates the now-shared struct (verified viagit grep).vendor/wheels/controller/rendering.cfc:648-649— the C16 hoist is safe:local.queryis fixed after theStructDelete(arguments, "query")at line 643, andcolumnListis constant per query. Both per-row loops now index the hoisted array.vendor/wheels/controller/rendering.cfc:712-723— the C13 once-per-column dedup mutates an existing struct (local.unreadableColumns[local.property] = true) rather than binding a newlocalvariable inside thecatch, which is exactly the BoxLang-safe pattern from Cross-Engine Invariant 11. TheWriteLogshape (type="warning",[Wheels]prefix,file="wheels") matches existing prior art atvendor/wheels/Controller.cfc:172.
Conventions
- Nit (informational, not blocking):
$returnStatusCodenow resolves status text via a struct-key lookup (rendering.cfc:854), and CFML struct keys are case-insensitive — so e.g.$returnStatusCode("not found")resolves to 404 where the oldStructFindValuevalue-scan may not have matched, depending on engine. The direction is strictly more lenient on an internal$helper, so this is fine — just flagging the subtle contract widening in case anyone greps for it later. - Relatedly trivial: the numeric branch of
$setRequestStatusCodedropped its deadlocal.statusTextassignment (rendering.cfc:820), but theelsebranch still carries an equally deadlocal.statusText = local.status;at line 824 — nothing after the branch reads it. Optional cleanup, no action required.
Cross-engine
No findings. The two spec fixes (miscellaneousSpec.cfc single-arg DirectoryCreate, seederSpec.cfc hoisting the query out of ValueList()) are themselves cross-engine corrections for documented Adobe compile-time validation, and the PR body shows full-suite Docker runs on both Lucee 7 (4494/12/0, the 12 being the tolerated testClientSpec baseline) and Adobe 2023 (4504/1/1, both pre-existing on develop with this PR's changes stashed). The new framework code uses no risky idioms — script-context lock already shipped at sql.cfc:1420, the spec finally blocks contain no loops (Invariant 12), and the new catch writes only struct keys (Invariant 11).
Tests
- Good coverage where it counts: the memo describe in
renderingSpec.cfc:946-996covers memoization, memo reuse, both lookup directions, deterministic duplicate-text resolution, and the throw paths;contentSpec.cfc:97-108adds the first coverage of the grouped-partial branch (regression guard for the hoist);ExpandedAssociationsJoinMemoSpec.cfcpins the fill-once marker with a carefulfinallythat restores the real metadata by dropping the marker and re-expanding. - Nit (non-blocking): the C13 logging path itself has no spec — nothing provokes an unreadable column to assert the warning is written. I accept the rationale that the failing exception types are engine-specific (it's why the
catch (any)was deliberately kept), and the log call is wrapped best-effort so the worst failure mode is a silent no-log, identical to today's behavior. If a portable way to fake an unreadable column surfaces later, a spec would be welcome.
Commits
- All three commits use valid types (
fix,fix,test), are DCO-signed, and explain the "why." - Nit: the header of 4508a27 (
fix(controller): memoize $getStatusCodes, hoist column tokenization, log $includeFile blanked columns) is 101 characters — one over commitlint'sheader-max-lengthof 100. Not blocking because the repo squash-merges and CI lints only the PR title (.github/workflows/pr.yml:26), which at 88 chars is valid and becomes the landing subject — but worth trimming if you rebase for any other reason.
Docs
Changelog fragments are present and correctly shaped (changelog.d/*.fixed.md × 2, *.performance.md × 1 — valid types, complete bullet lines, no direct CHANGELOG.md edit). No public API changed, so no guides update is required.
Summary
Closes the last three rendering residuals of the #2961 roll-up (C13/C16/C17) and lands the
$expandedAssociationsmetadata-fill lock flagged in the #2952 coverage-audit comment. Verify-first confirmed all four defects still reproduced onorigin/developbefore coding; everything else in #2961 (C2, DA1/DA5/DA14, DC6, SEC-8/P14) had already merged.Fixes #2961 (C13/C16/C17 residuals) and Refs #2952 (the
$expandedAssociationslock).C17 —
$getStatusCodes()rebuilt a 63-entry constant struct per render (vendor/wheels/controller/rendering.cfc)application[appKey].statusCodes, alongside a reverse text-to-code lookupapplication[appKey].statusCodeLookup(assigned lookup-first so a lock-free concurrent reader that observesstatusCodesalways sees both; both writes are idempotent).$returnStatusCode()reads the reverse map instead ofStructFindValueover the freshly rebuilt struct. The reverse map is built in numeric key order, so the duplicated text"Unassigned"(427/430/509) now deterministically resolves to 427 (previously engine-hash-order dependent).$setRequestStatusCode()keeps its validation call ($returnStatusTextthrowsWheels.RenderingErroron unknown codes) but no longer assigns the unused text.C16 —
$includeFile()re-tokenized the column list per rowListToArray(query.columnList)ran inside both per-row loops (grouped and ungrouped partial rendering). It is now hoisted above the loops; the column list is constant per query.C13 —
$includeFile()blanket catch silently blanked columnscatch (any e) { arguments[property] = "" }now logs a warning to thewheelslog — once per column per render, naming the column, partial, first failing row, and underlying error — before defaulting to the empty string. Logging is wrapped best-effort so it can never break rendering. (Keptcatch (any)rather than narrowing: the exception types for unreadable/binary column reads are engine-specific.)#2952 residual — unlocked shared-struct writes in
$expandedAssociations(vendor/wheels/model/sql.cfc)wheelsJoinMemodouble-checked named lock, with anexpandedMetadataFilledmarker written last so lock-skipping readers only ever observe a fully populated set. Values derive solely from class data, so fill-once is equivalent to the previous per-call rewrite (a reload rebuilds class data and the marker with it).$expandThroughAssociationshasMany-shortcut fix, same file) — no conflict;hasManyShortcutSpecgreen in the runs below.Drive-by: two Adobe-only COMPILE crashes from recently merged specs (required to verify anything on Adobe)
The core runner compiles every spec in the directory, and Adobe validates built-in arg counts /
ValueList()operands at compile time, so each of these crashed the entire Adobe 2023 suite (HTTP 500, 0 specs run) on current develop:miscellaneousSpec.cfc(fix(controller): sendFile honours absolute directory outside the web root #3101):DirectoryCreate(path, true)— thecreatePathboolean is Lucee-only; Adobe'sDirectoryCreatetakes exactly one parameter. 3 call sites (parents always exist) switched to the single-argument form.seederSpec.cfc(fix(seed): make wheels seed --generate create rows and report honest success #3107):ValueList(model(...).findAll(...).id)— Adobe only accepts a plainquery.columnreference insideValueList(). Assigned the query to a variable first.What each issue still has left
$stripIdentifierQuoteshoist, verified present), DC6 (EventMethods memo), SEC-8/P14 (route tester) all landed previously; C13/C16/C17 land here.select=validation policy once the deprecation window closes (currently dev-mode warning only, per fix(model): scope-arg denylist removal, select= dev warning, per-WHERE Migration + ON-split fixes #3011), (2) optional fullwhereParamsparameterization for scope-handler args. The$expandedAssociationslock checkbox from the issue's remaining-work comment is done here.Tests (TDD red-first)
renderingSpec.cfc: new$getStatusCodes is memoizeddescribe (5 specs) — memoization in application scope, memo reuse, text↔code resolution, deterministic duplicate-text resolution, unknown-code/text throws. Red before the fix (Expected [false] to be trueon both memoization specs), green after.contentSpec.cfc+ new_groupRow.cfmasset: first coverage of the grouped-partial branch (includePartial(query=…, group=…)) — regression guard for the C16 hoist.ExpandedAssociationsJoinMemoSpec.cfc: new spec asserting theexpandedMetadataFilledmarker exists after expansion and that later calls do not rewrite the shared struct outside the lock (marker key did not exist pre-fix).Evidence
Docker harness from the worktree (lucee7 + adobe2023 images, SQLite), full core suite on the rebased branch (4524 specs):
internal.testClientSpecbaseline artifactsTargeted bundles (both engines):
renderingSpec92/0,contentSpec11/0 (was 10),ExpandedAssociationsJoinMemoSpec5/0 (was 4),UpdateAllIncludeJoinSpec4/0,miscellaneousSpec(controller) 44/0,seederSpec22/0.Pre-existing Adobe 2023 failures NOT addressed here (verified identical with this PR's framework changes stashed; both also survive a clean container restart, so they are not cache artifacts):
migrator.typedColumnDefaultsSpec :: float()—The key(s) [default] does not exist in the target object(Adobe metadata quirk, recent migrator spec).security.RouteTesterHardeningSpec :: HTML-encodes the path in the verb-mismatch message—Neither the method $$findMatchingRoutes was found in component Public.cfc(recent route-tester spec).These two deserve a follow-up; before this PR's compile fixes the Adobe suite couldn't even run to reveal them (whole-suite
DIRECTORYCREATEcompile crash).🤖 Generated with Claude Code