fix(model): read a join's type from association metadata, not from the SQL text

[bpamiri]

Follow-up to #3334 (PR #3354, merged as 2c9864434). Raised by wheels-bot on that PR, and it was right.

The problem

The grouping decision #3334 introduced still answered "is this an INNER join?" by searching the generated SQL:

FindNoCase("INNER", local.joins[local.parentPosition])

That misclassifies every table whose name contains the substring — winners, spinners, beginners. A parent LEFT OUTER JOIN on c_o_r_e_winners reads as INNER, so its nested child is emitted flat instead of grouped, which silently drops exactly the parent rows #3334 exists to preserve.

Why fix it if it isn't a regression

It isn't one — the pre-#3334 code scanned the same way, and no fixture table hits it. The bot classified it non-blocking on that basis and that's fair.

But it is the precise anti-pattern #3334 argues against. That PR's whole thesis is structure should come from the association tree, not from re-parsing the SQL string — and this was ten lines of new code doing the opposite. winners is an ordinary table name, and the failure mode is silent wrong results rather than an error.

The fix

$associationJoinsInner() reads the association's declared joinType — the value $expandedAssociations turns into the leading INNER JOIN / LEFT OUTER JOIN text in the first place, so it is authoritative by construction rather than by inference. It falls back to the text scan only when an entry carries no joinType, which keeps it total for a caller assembling association structs by hand.

Red-first, and a warning about how I nearly got it wrong

With the helper reverted to the bare text scan, the new spec fails:

✗ 959 passed, 1 failed
  Failed: reads the join type from metadata, not from the table name (issue #3334)
          Expected [true] to be false

That red only appears on a cold server. Run against a warm one, the reverted build reported a clean green — tools/test-local.sh reuses a running server and the ?reload=true did not recompile the changed CFC. I recorded a passing red-check before noticing. Any red-check on this suite needs the server killed first, or it proves nothing.

The spec is a unit test on the decision function rather than a fixture, deliberately: pinning one boolean isn't worth a c_o_r_e_winners table and its DDL on all seven databases.

Verification

	result
develop 2c9864434	4755 pass / 0 fail / 0 error
this branch	4756 pass / 0 fail / 0 error

Exactly +1 — the new spec.

Compat matrix not dispatched yet: this narrows a condition that already gated correctly on every fixture the matrix exercises, so I expect it to be a no-op there. Say the word if you'd like it run before merge and I'll block on it.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer

TL;DR — This is a follow-up to #3334 that replaces a substring scan of the generated SQL (FindNoCase("INNER", ...)) with a read of the association's declared joinType when deciding which joins get pulled into a parenthesized group. The old scan misclassifies any table whose name contains inner (winners, spinners, beginners), which would silently drop the very parent rows #3334 exists to preserve. The fix is correct, authoritative by construction, and covered by a focused unit spec. Verdict: comment — the change is sound; only a cosmetic doc-comment duplication is worth cleaning up.

Correctness

The fix is correct against the real code path. $expandedAssociations returns entries built with local.entry = StructCopy(local.classAssociations[local.name]) (vendor/wheels/model/sql.cfc:1574), so every entry $fromClause iterates carries joinType. That is the same field the leading join text is derived from at vendor/wheels/model/sql.cfc:1467:

local.joinType = UCase(ReplaceNoCase(local.classAssociations[local.name].joinType, "outer", "left outer", "one"));

so $associationJoinsInner reads the authoritative source rather than re-inferring it from the text — exactly the anti-pattern #3334 argues against. The && local.parentPosition > 0 guard still short-circuits ahead of the parent-index access at vendor/wheels/model/sql.cfc:155, so there is no index-0 deref. The empty-joinType fallback (vendor/wheels/model/sql.cfc:1308) keeps the helper total for hand-built structs, and CFML's case-insensitive == makes joinType == "inner" robust to "INNER"/"Inner". No correctness issue.

Conventions

Minor, non-blocking: the insertion left a duplicate/orphaned doc block. vendor/wheels/model/sql.cfc:1291-1293 now holds a bare /** Internal function. */ immediately above the new function's own doc block at :1294-1306, and $expandedAssociations (:1314) — which that orphaned block originally documented — is now left with no docstring. Consider deleting the stray :1291-1293 block so the new comment stands alone and $expandedAssociations keeps its own.

Tests

vendor/wheels/tests/specs/model/crudSpec.cfc:1345 unit-tests the decision function in both directions (an outer join whose table name contains inner renders false; a genuine inner renders true) plus the no-joinType fallback both ways. The ##3334 in the it() description is correctly hash-escaped, so it will not trip the suite-wide crash noted in CLAUDE.md. Pinning the boolean directly rather than adding a c_o_r_e_winners fixture across seven databases is a reasonable trade-off and is documented in the spec comment.

Cross-engine

Clean. The helper is public with a $ prefix (satisfies the mixin-integration invariant for vendor/wheels/model/*.cfc), and uses only StructKeyExists / Len / == / FindNoCase — no closures, struct member-function calls, application-scope function members, or reserved-scope parameter names.

Commits

fix(model): read a join's type from association metadata, not from the SQL text — valid conventional-commit header (type fix, scope model, ~78 chars, not ALL-CAPS), body explains the "why," and carries a Signed-off-by trailer. No changelog fragment is needed: #3334's own changelog.d/3334-include-join-scoping.fixed.md is still unreleased, so this correction lands before any user sees the bug.

[wheels-bot]

Wheels Bot — Reviewer

TL;DR — Follow-up to #3334 that replaces a substring scan of the generated SQL (FindNoCase("INNER", ...)) with a read of the association declared joinType when deciding which joins get pulled into a parenthesized group. The text scan misclassifies any table whose name contains "inner" (winners, spinners, beginners), silently dropping the parent rows #3334 exists to preserve. Since the prior COMMENTED review, the branch added a second commit that scopes JobClassRoundTripSpec locals with local., a clean test-hygiene change that is itself cross-engine-correct. The fix is authoritative by construction and covered by a focused unit spec. Verdict: comment — sound change; only a cosmetic orphaned doc block is worth cleaning up.

Correctness
The fix is correct against the real code path. $expandedAssociations builds each returned entry as local.entry = StructCopy(local.classAssociations[local.name]) (vendor/wheels/model/sql.cfc:1575), so every entry $fromClause iterates carries joinType. That is the same field the leading join text is derived from at vendor/wheels/model/sql.cfc:1467, so $associationJoinsInner reads the authoritative source rather than re-inferring from the emitted text — precisely the anti-pattern #3334 argues against. joinType is documented as only ever inner or outer (vendor/wheels/model/associations.cfc:13), so the exhaustive joinType == "inner" check at sql.cfc:1309 is total, and case-insensitive == makes it robust to casing. The parentPosition GT 0 guard still short-circuits ahead of the parent-index access at sql.cfc:155, so there is no index-0 deref, and the empty-joinType fallback (sql.cfc:1308) keeps the helper total for hand-built structs. No correctness issue.

Conventions
Minor, non-blocking, carried over from the prior review and still present: the insertion left a duplicate/orphaned doc block. vendor/wheels/model/sql.cfc:1291-1293 holds a bare Internal-function docstring immediately above the new function own doc block at :1294-1306, and $expandedAssociations (:1314), which that stray block originally documented, is now left with no docstring. Delete the orphaned :1291-1293 block so the new comment stands alone, and keep an Internal-function docstring above $expandedAssociations.

Tests
vendor/wheels/tests/specs/model/crudSpec.cfc:1346 unit-tests the decision function in both directions (an outer join whose table name contains "inner" renders false; a genuine inner renders true) plus the no-joinType fallback both ways. The escaped issue reference in the it() description is correctly hash-escaped, avoiding the suite-wide crash CLAUDE.md warns about. Unscoped locals (m, outerSql) match the file existing convention (sibling it() blocks at :1311, :1329 use the same bare-assignment style), so no inconsistency there. Pinning the boolean directly rather than adding a c_o_r_e_winners fixture across seven databases is a reasonable, documented trade-off.

Cross-engine
Clean. $associationJoinsInner is public with a $ prefix (satisfies the mixin-integration invariant for vendor/wheels/model/*.cfc) and uses only StructKeyExists / Len / == / FindNoCase. The JobClassRoundTripSpec scoping commit is also compliant: every catch-body write mutates a struct field on a struct declared before the try (local.thrown.type = e.type, local.resolved.name in the miscased-path test), which is exactly the persists-on-BoxLang pattern from cross-engine invariant 11 — it never declares a fresh simple local.X inside a catch. Hoisted bridge = new wheels.Job() receivers are preserved, so invariant 16a stays satisfied.

Commits
Both headers are valid conventional commits: "fix(model): read a join type from association metadata, not from the SQL text" (type fix, scope model, under 100 chars) and "test(job): scope JobClassRoundTripSpec locals with local." (type test, scope job). Bodies explain the why; DCO sign-off present. No changelog fragment is needed — #3334 own changelog.d/3334-include-join-scoping.fixed.md is still unreleased, so this correction lands before any user sees the bug.

[github-actions]

Wheels Test Results

     32 files    9 856 suites   21m 31s ⏱️
131 733 tests 131 263 ✅ 397 💤 38 ❌ 35 🔥
133 665 runs  133 195 ✅ 397 💤 38 ❌ 35 🔥

For more details on these failures and errors, see this check.

Results for commit 7ecbf0c.

♻️ This comment has been updated with latest results.

[wheels-bot]

Wheels Bot — Reviewer

TL;DR — Follow-up to #3334/#3354 that stops answering "is this an INNER join?" by substring-scanning the generated SQL (FindNoCase("INNER", …)) and instead reads the association's declared joinType. The core fix is correct, well-documented, cross-engine-safe, and backed by a red-first unit test; the two accompanying test-scoping commits correctly apply cross-engine invariants 11 and 16a. Verdict: comment — no blocking findings, only two minor hygiene/docs nits. This does not change the two prior COMMENTED reviews' non-blocking posture.

Correctness

No issues. vendor/wheels/model/sql.cfc:1307 $associationJoinsInner() reads association.joinType when present and only falls back to the text scan for hand-built structs with no joinType:

if (StructKeyExists(arguments.association, "joinType") && Len(arguments.association.joinType)) {
    return arguments.association.joinType == "inner";
}
return FindNoCase("INNER", arguments.join) > 0;

Verified the value it compares is authoritative: $expandedAssociations copies joinType onto each entry (StructCopy(local.classAssociations[local.name])) and builds the leading join text from the same field at sql.cfc:1467 (UCase(ReplaceNoCase(joinType, "outer", "left outer", "one"))). The raw values are "inner"/"outer", and CFML == is case-insensitive, so the check is robust even if a value arrives cased. The winners/spinners/beginners misclassification the old FindNoCase path could produce is genuinely eliminated.

Cross-engine

No issues — both correctly handled:

• JobClassRoundTripSpec.cfc:91,120 keep the catch-observed struct as var thrown with bare access (thrown.type = e.type), exactly the invariant-11 pattern the PR also expands in CLAUDE.md. The local.-scoped conversion in the sibling describe block (three its with no catch dependency) is safe.
• JobClassRoundTripSpec.cfc:95 hoists the receiver (local.bridge = new wheels.Job()) rather than (new wheels.Job()).$instantiateJobClass(…), avoiding the Adobe MissingNameException (invariant 16a).
• $associationJoinsInner is public with a $ prefix, so it is integrated onto model instances (invariant 7) — the new spec calls it as m.$associationJoinsInner(…), which only works because of that.

Tests

Good. crudSpec.cfc:1346 adds a focused unit test that pins the exact trap (an outer join whose table name contains inner), the genuine-inner case, and both no-joinType fallback branches. PR body documents a cold-server red-first check. Unit-testing the boolean instead of standing up a c_o_r_e_winners fixture across seven DBs is a reasonable call.

Docs

Two minor, non-blocking nits:

1. Unrelated stub churn in the fix(test) commit. 7ecbf0cde adds 7 TestBox/MockBox generated stub files under public/testbox/system/stubs/ (471 insertions — e.g. tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89). These are regenerated test artifacts unrelated to catch-block scoping and bloat the diff. Consider dropping them from the PR (and, separately, whether public/testbox/system/stubs/** belongs in .gitignore).
2. No changelog fragment. This is a user-facing fix(model) (silent parent-row loss for tables whose name contains inner under a nested include), yet changelog.d/ has no new entry. A one-line changelog.d/3360-jointype-metadata.fixed.md would match the checklist. Skippable if you judge the substring edge case too narrow to surface to users — noting it so the omission is a decision, not an oversight.

Commits

All three conform to commitlint.config.js: fix(model), test(job), fix(test) — valid types, valid scopes, subjects under 100 chars, DCO sign-off present. Messages explain the "why" well (the fix(test) body in particular documents the BoxLang-only silent failure mode).