blog: Multi-Tenancy Built In (recover failed auto-deploy)#2551
blog: Multi-Tenancy Built In (recover failed auto-deploy)#2551
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 PR recovers a blog post about Wheels 4.0 multi-tenancy that failed to auto-deploy. The prose is solid and the operational story is accurate, but the primary code example demonstrates a non-existent API ($setTenantDatasource()) and sets tenant state on the wrong request key. These are correctness errors that would confuse readers who copy the snippet. The commit type is also invalid under commitlint.config.js. Requesting changes on both counts.
Correctness
$setTenantDatasource() does not exist — lines 57 and 63
// line 57
$setTenantDatasource(arguments.request.tenant);// line 63 (prose)
"The middleware contract is the same — pull the identifier, call `$setTenantDatasource`, pass the request along."
A repo-wide search (grep -rn "setTenantDatasource") returns zero results. This function does not exist anywhere in the framework. The actual mechanism is that wheels.middleware.TenantResolver sets request.wheels.tenant = {id, dataSource, config} directly on the built-in CFML request scope; $performQuery() reads request.wheels.tenant.dataSource to route queries. Calling a nonexistent $setTenantDatasource() would throw a "function not found" error at runtime.
Wrong request key — line 56
// line 56
arguments.request.tenant = subdomain;arguments.request here is the middleware pipeline struct, not the built-in CFML request scope. Even setting aside the nonexistent function call, writing a string to arguments.request.tenant has no effect on tenant resolution; the framework reads request.wheels.tenant.dataSource (a struct, on the built-in request scope, set by the finally-guarded TenantResolver.handle()).
Custom CFC shown instead of built-in — lines 48-60
The example scaffolds a hand-written app.middleware.TenantResolver CFC. The framework ships wheels.middleware.TenantResolver with first-class subdomain, header, and custom-resolver strategies. Users should not write their own middleware CFC for this. The correct pattern (from vendor/wheels/middleware/TenantResolver.cfc and web/sites/guides/src/content/docs/v4-0-0-snapshot/digging-deeper/multi-tenancy.mdx) is:
// config/settings.cfm
set(middleware = [
new wheels.middleware.TenantResolver(
strategy = "subdomain",
resolver = function(req) {
var subdomain = req.$tenantSubdomain;
var t = model("Tenant").findOne(where="subdomain='#subdomain#'");
if (IsObject(t)) return {id: t.id, dataSource: t.dataSourceName, config: {}};
return {};
}
)
]);No custom CFC required. The existing guide at digging-deeper/multi-tenancy.mdx already shows this correctly; the blog post contradicts it.
Commits
The commit message blog: Multi-Tenancy Built In uses blog as the type. blog is not in the commitlint.config.js type allowlist (feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert). The correct form is:
docs(web/blog): Multi-Tenancy Built In
web/blog is an explicit allowlisted scope. Fix with git commit --amend -m "docs(web/blog): Multi-Tenancy Built In" followed by git push --force-with-lease.
Suggested fix for the code block
Replace lines 46–63 with:
// config/settings.cfm — register the built-in TenantResolver globally
set(middleware = [
new wheels.middleware.TenantResolver(
strategy = "subdomain",
resolver = function(req) {
// req.$tenantSubdomain is set by the "subdomain" strategy
var t = model("Tenant").findOne(where="subdomain='#req.$tenantSubdomain#'");
if (IsObject(t)) return {id: t.id, dataSource: t.dataSourceName, config: {}};
return {}; // no tenant matched — request proceeds with the app default datasource
}
)
]);And update the prose at line 63 to match: the contract is "return a struct with a non-empty dataSource key," not "call $setTenantDatasource."
bpamiri
force-pushed
the
peter/blog-multi-tenancy-built-in
branch
from
3ba2798 to
fb82109
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The author addressed the commit-message flag from the prior review — docs(web/blog): Multi-Tenancy Built In is now valid under commitlint.config.js. However, the three correctness issues that drove the previous --request-changes verdict are unchanged in the blog post content: the example calls $setTenantDatasource() (a function that does not exist anywhere in the framework), sets tenant state on the wrong request key, and hand-rolls a custom middleware CFC when the framework ships wheels.middleware.TenantResolver with built-in strategy support. Maintaining the request-changes verdict.
Correctness
$setTenantDatasource() still does not exist — diff lines 57 and 63
// diff line 57
$setTenantDatasource(arguments.request.tenant);// diff line 63 (prose)
"pull the identifier, call `$setTenantDatasource`, pass the request along."
grep -rn "setTenantDatasource" vendor/wheels/ returns zero results. This function was not added. Calling it at runtime throws "function not found." The actual mechanism is that wheels.middleware.TenantResolver.handle() writes request.wheels.tenant = local.tenant directly onto the built-in CFML request scope (vendor/wheels/middleware/TenantResolver.cfc:75); $performQuery() reads request.wheels.tenant.dataSource from there. The built-in resolver does this automatically — there is no $set* function to call.
Wrong request key — diff line 56
arguments.request.tenant = subdomain;arguments.request is the middleware pipeline struct (the dispatcher's request envelope), not the built-in CFML request scope. Writing a bare string to arguments.request.tenant has no effect on tenant resolution. The framework reads request.wheels.tenant.dataSource — a struct on the built-in request scope set inside TenantResolver.handle().
Hand-written CFC shown instead of the built-in — diff lines 46–65
The example scaffolds app.middleware.TenantResolver. The framework ships wheels.middleware.TenantResolver with strategy="subdomain", strategy="header", and strategy="custom" out of the box. No custom CFC is needed. Correct pattern (matching vendor/wheels/middleware/TenantResolver.cfc lines 9–18):
// config/settings.cfm
set(middleware = [
new wheels.middleware.TenantResolver(
strategy = "subdomain",
resolver = function(req) {
// req.$tenantSubdomain is injected by the "subdomain" strategy
var t = model("Tenant").where("subdomain", req.$tenantSubdomain).findOne();
if (IsObject(t)) return {id: t.id, dataSource: t.dataSourceName, config: {}};
return {};
}
)
]);Note: the query builder form (where("subdomain", req.$tenantSubdomain)) is used above rather than raw interpolation (where="subdomain='#subdomain#'"), which would expose a SQL injection vector on a user-controlled subdomain value.
Commits
Fixed since the prior review — docs(web/blog): Multi-Tenancy Built In is valid. No further action needed here.
Wheels Bot — Reviewer B (round 1)A's review is accurate and the CHANGES_REQUESTED verdict is correct. I verified all three factual claims against the framework source and found no sycophancy, no false positives, and only one minor missed point that is out of scope for this PR. SycophancyNone detected. A maintained CHANGES_REQUESTED despite the commit message fix, and justified each finding with specific file references and code citations. False positivesNone detected. I verified all three claims:
Missed issuesOne minor point, pre-existing and outside this PR's scope: the framework's own Verdict alignmentCHANGES_REQUESTED is consistent with the findings. All three correctness issues (non-existent function call, wrong scope write, unnecessary custom CFC) remain in the blog post unchanged from the prior review. Readers copying the example middleware CFC would hit a runtime "function not found" error immediately. ConvergenceAligned. A's three findings are factually verified and the verdict is appropriate. The blog post code example needs to be replaced with the correct built-in |
Wheels Bot — Reviewer B (round 1)A's core findings are accurate and the SycophancyNone detected. A requests changes, cites specific lines, and does not approve despite real correctness issues. False positivesNone detected. I verified all three claims:
Missed issuesSQL injection in A's suggested replacement A's proposed replacement contains: var t = model("Tenant").findOne(where="subdomain='" & req.$tenantSubdomain & "'");(A's actual snippet used CFML
The CLAUDE.md chainable query builder is documented as injection-safe for exactly this reason: // Safe — value is auto-quoted
var t = model("Tenant").where("subdomain", req.$tenantSubdomain).findOne();Note: Verdict alignmentA's ConvergenceNot aligned yet. A's verdict and the three primary findings are correct, but A's suggested replacement propagates an injection-unsafe query pattern that blog readers will copy. Before converging, A should revise the "Suggested fix" code block to use |
- Replace hand-rolled `app.middleware.TenantResolver` example with the
built-in `wheels.middleware.TenantResolver` and `strategy="subdomain"`
resolver-closure pattern (web/content/blog/posts/multi-tenancy-built-in.md
lines 46-58).
- Remove the call to the non-existent `$setTenantDatasource()` function
and the bare-string `arguments.request.tenant = subdomain` write —
the built-in resolver writes `request.wheels.tenant` itself.
- Use the injection-safe query builder form
(`model("Tenant").where("subdomain", req.$tenantSubdomain).findOne()`)
rather than raw interpolation, so copy-pasters don't ship a SQL
injection vector with their tenant lookup.
- Rewrite the prose paragraph at line 60 to describe the actual
contract (return a struct with a `dataSource` key) instead of the
non-existent "call \$setTenantDatasource" instruction, and surface
the three built-in strategies (subdomain / header / custom) with
their canonical use cases.
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
No test run: this is a pure-docs change to a single blog post under The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique is a convergence acknowledgment. All three correctness findings were independently verified against the framework source, no false positives were identified, and no sycophancy was detected. The one noted gap — that TenantResolver.cfc's own docblock (lines 12–14) uses the same raw SQL interpolation pattern — was correctly scoped out as a pre-existing issue not introduced by this PR. I agree with that scope call; addressing the docblock belongs in a separate issue against the framework source, not as a blocker here. Verdict is unchanged.
Conceded points
None.
Defended points
None required — B verified and aligned with all three findings:
$setTenantDatasource()does not exist: confirmed by B via repo-wide grep, zero results. Calling it at runtime throws "function not found."- Wrong request key: confirmed by B at
vendor/wheels/middleware/TenantResolver.cfc:75— the framework writes to the built-in CFMLrequestscope, not the middleware pipeline struct. - Built-in
wheels.middleware.TenantResolverexists: confirmed by B. The custom CFC in the blog post is unnecessary and misleading.
B also confirmed that my updated suggested fix (using model("Tenant").where("subdomain", req.$tenantSubdomain).findOne() — the injection-safe query builder) was correct and addressed the SQL injection concern B had raised against the prior SHA's example.
Updated verdict
Still request-changes: the blog post code example calls a nonexistent function, sets tenant state on the wrong scope, and scaffolds a custom CFC when the framework ships built-in support. These are copy-paste-breaking errors that readers will hit immediately at runtime. Verdict is unchanged from the initial review.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The round-1 fixup commit addresses all three correctness blockers from the prior two review rounds. The code example now uses the built-in wheels.middleware.TenantResolver, the nonexistent $setTenantDatasource() call is gone, the wrong-scope assignment is gone, and the prose correctly describes the resolver contract. Both commit messages are valid under commitlint.config.js. One minor housekeeping suggestion before merge; no blocking findings remain.
Correctness — resolved
All three blockers from the prior --request-changes verdict are addressed in 88e834bc:
| Issue | Prior state | Current state |
|---|---|---|
$setTenantDatasource() does not exist |
called at line 57 | removed |
Wrong request key (arguments.request.tenant = subdomain) |
present at line 56 | removed |
| Hand-rolled CFC instead of built-in | app.middleware.TenantResolver |
wheels.middleware.TenantResolver |
The current code example:
new wheels.middleware.TenantResolver(
strategy = "subdomain",
resolver = function(req) {
var t = model("Tenant").where("subdomain", req.$tenantSubdomain).findOne();
if (IsObject(t)) return {id: t.id, dataSource: t.dataSourceName, config: {}};
return {};
}
)matches the pattern in vendor/wheels/middleware/TenantResolver.cfc lines 9-18. The query builder form (where("subdomain", req.$tenantSubdomain)) is injection-safe — no raw interpolation of a user-controlled subdomain value.
Commits
Both commits are valid:
fb82109a—docs(web/blog): Multi-Tenancy Built In— typedocs, scopeweb/blog, both allowlisted.88e834bc—docs(web/blog): address Reviewer A/B consensus findings (round 1)— same, valid.
Minor suggestion (non-blocking): The second commit is a fixup of the first. For a single-file blog-post PR, squashing to one commit before merge keeps the log tidy and removes the "address Reviewer A/B" commit from develop history. Either git rebase -i squash or a squash-merge achieves this. Not a blocker — maintainer's call.
Security
Injection-safe query builder used throughout. No secrets or credentials in the diff.
Wheels Bot — Reviewer B (round 1)A's "response" was submitted against the current SHA ( SycophancyNone detected. A is not approving anything it shouldn't. False positivesAll three of A's ongoing findings are stale relative to the current diff:
Missed issuesNone. The current code is correct:
Minor, non-blocking: a squash merge to a single Verdict alignmentA's ConvergenceAligned, no changes needed. The blog post code example is correct in its current form. A's "still request-changes" position is based on observations from before the address review committed. Against the current diff there is nothing to block. |
Wheels Bot — Reviewer B (round 1)A's review is accurate. All three prior blockers are verified fixed in the current diff, both commit messages are valid, and the non-blocking squash suggestion is appropriate. No sycophancy, no false positives, no missed issues. SycophancyNone detected. A used COMMENTED state (not APPROVED), provided a verification table for each resolved blocker, and cited specific lines. The review is evidence-based throughout. False positivesNone detected. I verified all four factual claims against the actual diff:
Missed issuesNone detected. I re-scanned the diff for:
Verdict alignmentCOMMENTED with no blocking findings is the correct verdict for a docs-only PR where all prior ConvergenceAligned. A's findings are all accurate, A's verdict is appropriate, and I have no remaining disputes or missed issues to raise. The consensus is: this PR is review-clean for this SHA and ready for human merge (squash or otherwise, at maintainer discretion). |
* ci: retry blog visual baseline auto-push with fetch+rebase loop The "Commit pre-refreshed blog baseline" step in web-deploy.yml's visual-regression job pushed to develop with a single `git push origin HEAD:develop` — no retry. When develop moved during the ~3 min job runtime (e.g., another PR merging right behind a blog content push), the push was rejected with `! [rejected] HEAD -> develop (fetch first)` and the auto-refreshed baseline was silently dropped. Effect: every subsequent non-blog PR tripped visual-regression on a stale baseline until someone manually refreshed. Concrete trigger this week: PRs #2549 + #2551 merging back-to-back left blog.png predating both "Background Jobs Without Redis" and "Multi-Tenancy Built In" posts, blocking PR #2559 with 38,581-pixel diff. Fix: wrap the push in a 5-attempt fetch+rebase+push loop. Rebase conflict path exits clean (would only happen if a parallel run already refreshed the baseline, making ours redundant). Push-rejected path retries with a short randomized backoff. See run 25647803649 for the failure mode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: refresh stale blog baseline and address review nits Two pieces, both fallout from the race-condition bug this PR fixes: 1. Refresh `web/tests/visual-baselines/blog.png`. The May 9 → present auto-refresh pushes were dropped by the bug, so develop's baseline predates `Background Jobs Without Redis` and `Multi-Tenancy Built In` posts. Without this commit, PR #2560's own visual-regression check stays red and every non-blog PR keeps tripping. Regenerated locally via `pnpm --filter @wheels-dev/site-blog build` + `node scripts/visual-regression.mjs --update --site blog`; verified the freshly-written baseline passes a subsequent check run with 0-pixel diff. 2. Two minor nits from Reviewer A on the retry loop: - Guard the final-iteration `sleep` behind `[ "$attempt" -lt 5 ]` so the 5th rejected push doesn't sleep 2-6s before exiting. - Add `|| true` to `git rebase --abort` so a failed abort (e.g., already-clean rebase state) doesn't flip the intended exit-0 conflict path to exit-1 under `set -euo pipefail`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: don't print misleading "will retry" message on final attempt Reviewer B noted that the "Push rejected on attempt $attempt; will fetch + rebase + retry." echo runs unconditionally inside the for-loop body, so on attempt 5 it lies — there's no retry coming, the loop falls through to exit 1. Move the echo inside the same `[ "$attempt" -lt 5 ]` guard that already skips the backoff sleep, and emit a distinct "final attempt, no retry" message in the else branch. Pure cosmetic; no control-flow change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
develop.origin/develop:web/content/blog/posts/multi-tenancy-built-in.md(+129 lines).Why this PR exists
The blog publishing tool exported the post cleanly at 07:00 (DB
lastExportedAt=2026-05-10 07:00:00, file written to disk), but the auto-deploygit push origin developwas rejected:The publisher's wheels checkout was on a non-
developbranch.GitDeployer.commitAndPush()rebases the currently checked-out branch ontoorigin/developand then runsgit push origin develop— pushing the localdevelopref, which was never advanced. After the day's intervening framework PRs landed on origin/develop, the push was 3 commits behind (now 7) and rejected.Follow-up (not in this PR)
app/services/GitDeployer.cfcin the blog repo so the deploy path doesn't silently depend on an implicit "develop is checked out" precondition. Either pin to develop explicitly (switch + restore), operate againstorigin/developdirectly, or refuse to proceed whenHEADisn't develop.Test plan
blog.wheels.devshows "Multi-Tenancy Built In" as today's published post.🤖 Generated with Claude Code