Skip to content

fix(model): scope a nested include's inner join to its own parent join - #3354

Open
bpamiri wants to merge 2 commits into
developfrom
fix/3334-include-join-leakage
Open

fix(model): scope a nested include's inner join to its own parent join#3354
bpamiri wants to merge 2 commits into
developfrom
fix/3334-include-join-leakage

Conversation

@bpamiri

@bpamiri bpamiri commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Closes #3334.

The bug

findAll(include="...") copied every INNER JOIN into every LEFT OUTER JOIN. With a single outer join — the shape both #449 and #3245 exercise — that is indistinguishable from correct. Add a shallow sibling ahead of the nested group and it fans out:

model("post").findAll(include = "c_o_r_e_comments,classifications(tag)")
FROM posts
LEFT OUTER JOIN (comments        INNER JOIN tags ON classifications.tagid = tags.id) ON ...
LEFT OUTER JOIN (classifications INNER JOIN tags ON classifications.tagid = tags.id) ON ...

The first group references classifications before the query introduces it. Reported against Oracle as ORA-00904, but it is engine-independent — the same include errors on SQLite with no such column: c_o_r_e_classifications.tagid.

The fix

The information needed to place each join correctly already existed and was being discarded. $expandedAssociations walks the include with a levels stack, so it knows exactly which association nests under which — then returns a flat array, leaving $fromClause to re-derive parentage with a regex over the generated SQL text. Both prior patches to this code fought that same information loss.

Each entry now carries parentPosition. An INNER JOIN is grouped with exactly one OUTER JOIN: the association it is nested under. A root-level INNER JOIN has no enclosing group and stays flat, keeping the root FROM table in scope for its ON — the #3245 failure mode, now prevented structurally instead of by a gate.

That also fixes a root-level INNER JOIN being dropped from the output entirely. The old loop only ever emitted the outer-join array, so include="author,classifications(tag)" never emitted INNER JOIN authors at top level at all.

The gate is gone

It matched the include string against ^([^(]+)\(([^)]+)\)$ — which only matches when the nested group comes last. That is why the reporter's workaround (move the nested group to the front) worked: the two orderings took entirely different code paths. Whether a join is correctly scoped is a property of the association tree, not of where the user typed the parentheses.

Removing it also collapses the duplicated flat-join branch. Net: 200 lines changed, 42 fewer.

Behaviour change

include order no longer changes results, and one ordering now returns more rows.

In the nested-first form the nested INNER JOIN was emitted at the root, which demoted the sibling LEFT OUTER JOIN to an inner join and silently dropped parent rows with no associated record. On these fixtures that is 3 rows where the nested-last form returns 15. Both orderings now return 15 — the rows a hasMany/hasOne include exists to preserve.

Apps that relied on that filtering should set joinType="inner" on the association. Called out in the changelog fragment.

This is the one part of the PR worth a second opinion; it is isolated to removing the gate, so it is straightforward to revert on its own if you would rather ship only the fan-out fix.

Red-first

With sql.cfc reverted to develop and the specs in place — 945 pass / 3 fail / 1 error:

spec on develop
scopes a nested inner join to its own parent fails on the generated SQL
keeps a root-level inner join out of the nested group fails — the root join is missing entirely
emits the same joins wherever the nested group sits fails — the two orderings differ
returns the same rows wherever the nested group sits errorsno such column: c_o_r_e_classifications.tagid
tolerates an unbalanced include the way it always did errors without the guard — can not remove Element at position [0]

The last one is the reported symptom, reproduced.

One regression this branch introduced, found on re-read and fixed

$expandedAssociations pops a level on every ). The new parentPositions stack pops alongside it — and where ListDeleteAt on a one-element list tolerates an unbalanced include like include="c_o_r_e_comments)", ArrayDeleteAt(x, 0) throws.

develop resolves that input to a plain join and ignores the stray paren; this branch would have started throwing can not remove Element at position [0]. Guarded, and pinned by a spec — pulling the guard back out takes the suite to 949 pass / 1 error with that exact message.

Verification

lucee7 + sqlite, full core suite, back to back on one machine:

result
develop ab901cff7 4732 pass / 0 fail / 0 error
this branch 4737 pass / 0 fail / 0 error

Exactly +5 — the new specs, nothing else moved. The existing #449 and #3245 regression specs pass unchanged and byte-identically; neither expected string needed editing, which is the main evidence that the gate was redundant rather than load-bearing.

Compat matrix dispatched — this touches the join builder on every engine, so I am not calling it done on one database. Expect the usual red Wheels Test Results check on the head; that is the #3302 misattribution artifact from my own dispatch, not a regression.

Credit

Reported by Mike Grogan with a working patch attached, which correctly localised the problem to this loop. I went a different way on the mechanism: his version decides ownership by substring-searching the outer join's table name inside each inner join string, which misfires when one table name contains another (c_o_r_e_photos inside c_o_r_e_photogalleryphotos — already present in these fixtures) and leaves the ordering inconsistency in place.

🤖 Generated with Claude Code

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — This PR fixes #3334: findAll(include="...") copied a nested association's INNER JOIN into unrelated sibling LEFT OUTER JOINs, generating SQL that referenced a not-yet-introduced table (ORA-00904 / MySQL "unknown column in on clause"). The fix threads a parentPosition through $expandedAssociations so $fromClause groups each inner join with the single association it actually nests under, replacing an anchored regex over the include string that only matched when the nested group came last. The core change is correct, cross-engine-safe, and backed by a red-first spec set with confirmed fixtures. Verdict: comment — one non-blocking hygiene issue (unrelated committed test artifacts); the join fix itself is clean and I'd approve it on its own.

Correctness

The rewrite is sound. local.parentPositions mirrors the long-standing local.levels stack using the same (/) push/pop, but starts empty (vs levels seeded with the root model), so at root depth parentPosition correctly resolves to 0 and the > 0 guard keeps local.joins[local.parentPosition] from ever indexing position 0. Pushing local.i as the parent position is valid because exactly one entry is appended to local.rv per pass (sql.cfc:1553), so index i is this association's slot.

The nesting predicate at sql.cfc:145-149 is internally consistent: a join marked isNested must be INNER, while a nestedJoins key must be a non-INNER parent, so no join is ever both a nested child and a group owner. I traced include="c_o_r_e_comments,classifications(tag)", the order-swapped form, and include="author,classifications(tag)" (root-level inner) by hand and each produces the asserted SQL. The ReFindNoCase("LEFT OUTER JOIN ...") grouping dependency is pre-existing and safe — Wheels only ever emits INNER JOIN or LEFT OUTER JOIN, so a non-INNER parent is always a LEFT OUTER JOIN and the regex always matches.

The intentional behaviour change (nested-first ordering now returns the rows a hasMany/hasOne include is meant to preserve) is documented in the changelog fragment with a concrete migration path (joinType="inner"). Reported +4 on the full lucee7/sqlite suite (exactly the new specs) and byte-identical #449/#3245 specs are strong evidence the old gate was redundant rather than load-bearing.

Tests

Excellent coverage. Five new specs in crudSpec.cfc — four string assertions pinning the generated SQL plus one executable findAll proving both include orderings agree on the result set. I confirmed the fixtures exist and match: Post.cfc declares belongsTo("author"), hasMany("c_o_r_e_comments"), hasMany("classifications"); Classification.cfc declares belongsTo("tag") — so the specs genuinely exercise the outer/inner/root-inner shapes described. Red-first failures on develop are documented per-spec.

Docs

Non-blocking hygiene issue: this PR commits 7 unrelated MockBox stub artifacts under public/testbox/system/stubs/ (139184C0…, 16CA3942…, 30942F4D…, 754BEF48…, B7681C49…, C383511C…, CDCB73D6…). Their contents are runtime-generated mock method stubs for closeSSEStream, sendSSEEvent, sendSSEComment, initSSEStream, poll, checkError, and \$getChannelEngine — SSE/channels test byproducts with nothing to do with the join-scoping fix. They appear to be leftover cache from a local test run.

The repo already tracks at least one such file on develop (F952D54F… for \$requireRunningServer), so this is a recurring accidental-commit pattern rather than something introduced here — but please drop these 7 from the PR (git rm --cached) so the diff stays scoped to the fix, and consider a follow-up to add public/testbox/system/stubs/ to .gitignore. Not a merge blocker.

Commits

fix(model): scope a nested include's inner join to its own parent join — conforms to commitlint (valid type fix, valid scope model, lowercase subject, under 100 chars). The message reflects the "why." Changelog fragment changelog.d/3334-include-join-scoping.fixed.md uses the correct <slug>.<type>.md form with type fixed, not a direct CHANGELOG.md edit.


Nice work — the mechanism (carry parentage from the association tree instead of re-deriving it from generated SQL) is the right call, and choosing it over the reporter's substring-matching patch (which misfires on c_o_r_e_photosc_o_r_e_photogalleryphotos) is well-justified. Only the stub-file cleanup stands between this and an approve.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Wheels Test Results

     31 files    9 772 suites   21m 15s ⏱️
131 201 tests 130 731 ✅ 397 💤 38 ❌ 35 🔥
133 133 runs  132 663 ✅ 397 💤 38 ❌ 35 🔥

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

Results for commit b9d452a.

♻️ This comment has been updated with latest results.

@bpamiri
bpamiri force-pushed the fix/3334-include-join-leakage branch from fe54f1a to f66eb8e Compare August 4, 2026 06:42
`findAll(include="...")` copied every INNER JOIN into every LEFT OUTER JOIN. With a single
outer join — the shape both issue #449 and issue #3245 exercise — that is indistinguishable
from correct. Add a shallow sibling ahead of the nested group and it fans out:

  include = "c_o_r_e_comments,classifications(tag)"

  FROM posts
  LEFT OUTER JOIN (comments        INNER JOIN tags ON classifications.tagid = tags.id) ON ...
  LEFT OUTER JOIN (classifications INNER JOIN tags ON classifications.tagid = tags.id) ON ...

The first group references `classifications` before the query introduces it. Reported
against Oracle as ORA-00904, but it is engine-independent: on SQLite the same include errors
with `no such column: c_o_r_e_classifications.tagid`.

The information needed to place each join correctly already existed and was being thrown
away. `$expandedAssociations` walks the include with a `levels` stack, so it knows exactly
which association nests under which — and then returned a flat array, leaving `$fromClause`
to re-derive parentage by regex over the generated SQL text. Both prior patches to this code
fought that same information loss. Each entry now carries `parentPosition`, and an INNER
join is grouped with one OUTER join: the association it is actually nested under. A
root-level INNER join has no enclosing group and stays flat, which keeps the root FROM table
in scope for its ON condition — the #3245 failure mode, now prevented structurally rather
than by a gate.

That also fixes a root-level INNER join being dropped from the output entirely. The old loop
only ever emitted the outer-join array, so `include="author,classifications(tag)"` never
emitted `INNER JOIN authors` at top level at all — it appeared only inside the
classifications group, where its ON references the out-of-scope root.

The gate is now redundant, so it is gone. It tested the include STRING against
`^([^(]+)\(([^)]+)\)$`, which only matches when the nested group comes last, so `a(b),c` and
`c,a(b)` generated different SQL for the same query. Whether a join is correctly scoped is a
property of the association tree, not of where the user typed the parentheses. Removing it
also collapses the duplicated flat-join branch: 200 lines changed, 42 fewer.

Behaviour change, called out in the changelog: in the nested-first form the nested INNER
JOIN used to be emitted at the root, demoting the sibling LEFT OUTER JOIN to an inner join
and silently dropping parent rows with no associated record. On the fixtures that is 3 rows
where the nested-last form returns 15. Both orderings now return 15. A query written
nested-first can therefore return more rows than before — the rows a hasMany/hasOne include
exists to preserve. Apps that relied on the filtering should set joinType="inner".

Red-first, with sql.cfc reverted to develop: 945 pass / 3 fail / 1 error. The three string
assertions fail on the generated SQL and the executable one errors with the SQLite message
above — the reported symptom, reproduced.

Verification, lucee7 + sqlite, full core suite, back to back on one machine:

  develop ab901cf   4732 pass / 0 fail / 0 error
  this branch         4736 pass / 0 fail / 0 error

Exactly +4, the new specs. The existing #449 and #3245 regression specs pass unchanged and
byte-identically — neither expected string needed editing.

Reported by Mike Grogan, who also submitted a working patch. His fix localises the same
loop but decides ownership by substring-searching the outer join's table name inside each
inner join string, which misfires when one table name contains another (c_o_r_e_photos
inside c_o_r_e_photogalleryphotos, already present in these fixtures) and leaves the
ordering inconsistency in place.

Closes #3334

Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri
bpamiri force-pushed the fix/3334-include-join-leakage branch from f66eb8e to 0222f58 Compare August 4, 2026 06:44

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — This PR fixes #3334: findAll(include="...") copied a nested association's INNER JOIN into every preceding sibling LEFT OUTER JOIN, generating SQL that referenced a table before the query introduced it (ORA-00904 / MySQL "unknown column in on clause"). The fix threads a parentPosition through $expandedAssociations so join grouping reads the association tree instead of re-deriving parentage from generated SQL text, and drops the string-anchored gate that made a(b),c and c,a(b) produce different SQL. I traced the new grouping loop against the fan-out, root-level-inner, two-nested-group, and belongs-to-chain shapes and it holds up; the specs are red-first and cover both string and executable assertions. Verdict: comment — I found no correctness, cross-engine, or security issues; one non-blocking robustness observation below.

Correctness

Verified the new indexing invariant that the whole fix rests on. In $expandedAssociations (vendor/wheels/model/sql.cfc:13131553), local.parentPosition is read at the top of each pass from the parentPositions stack, the stack is pushed/popped in lockstep with local.levels, and each pass appends exactly once to local.rv with no early continue — so local.i really is the entry's final position and entry.parentPosition points at a valid earlier index. The grouping loop in $fromClause (:141:188) then keys nestedJoins by that position, and because a child only nests under a parent whose join is not INNER (:148), every nestedJoins key resolves to a non-nested OUTER join that the emit loop actually visits — no orphaned groups. I hand-traced a(b),c(d) (each inner scopes to its own parent), author,classifications(tag) (root-level inner stays flat via the parentPosition > 0 guard at :147), and the belongsTo-chain posts,user(galleries) (all flat) and all three match the specs.

Conventions

Non-blocking, pre-existingvendor/wheels/model/sql.cfc:146 and :148 classify a join as inner/outer with FindNoCase("INNER", local.joins[...]), i.e. a substring scan over the generated SQL:

if (
    FindNoCase("INNER", local.joins[local.i])
    && local.parentPosition > 0
    && !FindNoCase("INNER", local.joins[local.parentPosition])
) {

This is the same substring-matching fragility the PR body rightly calls out in the reporter's patch — a hasMany to a table whose name contains the literal substring inner (winners, spinners, …) produces LEFT OUTER JOIN ... winners ..., which FindNoCase("INNER", …) reports as an inner join. The association metadata already carries an unambiguous signal: local.associations[local.i].joinType is "inner" / "outer" (documented in vendor/wheels/model/associations.cfc:13, and it's the very field the join string is built from at sql.cfc:1440). Reading joinType instead of scanning the SQL text would close that gap.

I confirmed this is not a regression — develop's flat/nest branches used the identical FindNoCase("INNER", …) scan, so behavior is unchanged for such tables. Flagging only because the changed lines re-assert the pattern and a cleaner signal is one field away; ship-as-is is fine.

Tests

Strong coverage in vendor/wheels/tests/specs/model/crudSpec.cfc:13021366: the fan-out (c_o_r_e_comments,classifications(tag)), root-level-inner (author,classifications(tag)), order-independence (string equality across both orderings), and an executable row-count assertion that both orderings agree and preserve the parent rows a hasMany/hasOne include exists to keep. BDD, extends wheels.WheelsTest, red-first evidence documented in the PR body. The existing #449 / #3245 regression specs pass byte-identically, which is good evidence the removed gate was redundant rather than load-bearing.

Docs

Changelog fragment changelog.d/3334-include-join-scoping.fixed.md uses the correct <slug>.<type>.md form (no direct CHANGELOG.md [Unreleased] edit), and it honestly documents the behavior change — that nested-first ordering can now return more rows, with the joinType="inner" migration path for apps that relied on the old accidental filtering.

Commits

fix(model): scope a nested include's inner join to its own parent join — valid type, valid scope, subject well under 100 chars, describes the "why." Conforms to commitlint.config.js.

…lways did

Found on a clean-context re-read of this branch, not by a failing test.

`$expandedAssociations` walks the include's delimiter characters and pops a level on every
`)`. The new `parentPositions` stack pops alongside it — and where `ListDeleteAt` on a
one-element list quietly tolerates an unbalanced include like `"c_o_r_e_comments)"`,
`ArrayDeleteAt(x, 0)` throws `can not remove Element at position [0]`.

Develop resolves that input to a plain join and ignores the stray paren. This branch would
have started erroring on it — a new failure mode on user input, introduced by a change that
has nothing to do with malformed includes. Guarded, and pinned by a spec that reproduces it:
with the guard removed the suite goes to 949 pass / 1 error with that exact message.

lucee7 + sqlite, full core suite: 4737 pass / 0 fail / 0 error — develop 4732 plus the five
specs this branch now adds.

Signed-off-by: Peter Amiri <peter@alurium.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — Fixes #3334: findAll(include="...") copied a nested association's INNER JOIN into unrelated sibling LEFT OUTER JOINs, generating SQL that referenced a not-yet-introduced table (ORA-00904 / MySQL "unknown column in on clause", no such column on SQLite). The fix threads a parentPosition through $expandedAssociations so $fromClause scopes each inner join to the single association it nests under, and removes the anchored regex over the include string that only matched when the nested group came last. I traced the fan-out (c_o_r_e_comments,classifications(tag)), the root-level-inner (author,classifications(tag)), and both orderings by hand against the new specs and they hold. The change is cross-engine-safe and backed by red-first tests. Verdict: comment — no Correctness / Cross-engine / Security blockers; one non-blocking convention note below.

Process note (issue #2848). This review was dispatched against 0222f58be (the marker SHA below), but the PR head has since advanced to b9d452a2d, which adds an ArrayLen guard around the ArrayDeleteAt on the unbalanced-include path plus a regression test for it. My review targets the live diff (gh pr diff, i.e. b9d452a2d) — the state you see on the PR. The marker keeps 0222f58be verbatim as required.

Correctness

The rewrite is sound. In $expandedAssociations (vendor/wheels/model/sql.cfc:13131553), local.parentPositions mirrors the existing local.levels stack with matching (/) push/pop, but starts empty — so at root depth parentPosition resolves to 0 and the > 0 guard at :147 keeps a root-level inner join flat (the #3245 shape) and out of any group. Pushing local.i as the parent position (:1530) is valid because each pass appends to local.rv exactly once with no early continue, so local.i is that entry's final index.

The grouping loop in $fromClause (:141:188) then keys nestedJoins by that position, and the child-only-nests-under-a-non-INNER-parent rule (:148) means every nestedJoins key resolves to an OUTER join the emit loop actually visits — no orphaned groups, and no inner join ever spliced into a sibling. On the live head the unbalanced-include path is guarded (if (ArrayLen(local.parentPositions))) and covered by a test, so a malformed include="c_o_r_e_comments)" still resolves to a plain join rather than throwing on ArrayDeleteAt(x, 0).

I also confirmed the additive parentPosition key doesn't disturb the other consumers of $expandedAssociationsupdate.cfc:86, nestedproperties.cfc:104, view/miscellaneous.cfc:576, onmissingmethod.cfc, and the sibling call sites in sql.cfc all read the flat array's .join / .joinOnConditions, which are unchanged. The grouping rewrite is contained to $fromClause.

Conventions

Non-blocking, pre-existing. vendor/wheels/model/sql.cfc:146 and :148 classify a join as inner/outer by substring-scanning the generated SQL:

if (
    FindNoCase("INNER", local.joins[local.i])
    && local.parentPosition > 0
    && !FindNoCase("INNER", local.joins[local.parentPosition])
) {

A hasMany/hasOne to a table whose name contains the literal substring inner (winners, spinners, beginners, …) produces LEFT OUTER JOIN ...c_o_r_e_winners..., which FindNoCase("INNER", …) reports as an inner join — so an inner child under such an OUTER parent would fail the !FindNoCase(...) check and stay ungrouped. The association metadata already carries the unambiguous signal: each entry's joinType is "inner"/"outer" (it's the very field the join string is built from at sql.cfc:1440). Reading local.associations[local.i].joinType would close the gap. This is not a regression — develop's flat/nest branches used the identical FindNoCase("INNER", …) scan — so ship-as-is is fine; flagging only because the changed lines re-assert the pattern and a cleaner signal is one field away.

Tests

Strong. vendor/wheels/tests/specs/model/crudSpec.cfc adds the fan-out, root-level-inner, order-independence (string equality across both orderings), unbalanced-include tolerance, and an executable row-count assertion that both orderings agree and preserve the parent rows a hasMany/hasOne include exists to keep. BDD, extends wheels.WheelsTest, red-first evidence documented per-spec in the PR body. The existing #449 / #3245 specs pass byte-identically, which is good evidence the removed gate was redundant rather than load-bearing.

Cross-engine

Clean. The new code uses only ArrayAppend / ArrayDeleteAt / ArrayLen / StructKeyExists / FindNoCase / ReFindNoCase / ListAppend / Replace / Mid and sequential index assignment (local.joins[local.i] = ..., i = 1..n) — no struct.map() member calls, no closures capturing scope, no application-scope function members, no obj["key"]() in closures, no Left(str, 0). Nothing here is Lucee-only or Adobe-only.

Docs

Changelog fragment changelog.d/3334-include-join-scoping.fixed.md uses the correct <slug>.<type>.md form (no direct CHANGELOG.md [Unreleased] edit) and honestly documents the behaviour change — nested-first ordering can now return more rows, with the joinType="inner" migration path for apps that relied on the old accidental filtering. The 7 unrelated MockBox stub artifacts an earlier review flagged are no longer in the diff (file list is changelog.d/, sql.cfc, crudSpec.cfc only).

Commits

fix(model): scope a nested include's inner join to its own parent join — valid type fix, valid scope model, lowercase subject well under 100 chars, describes the "why." Conforms to commitlint.config.js.


Nice work — carrying parentage from the association tree instead of re-deriving it from generated SQL is the right mechanism, and the justification for choosing it over the reporter's substring-matching patch (which misfires on c_o_r_e_photosc_o_r_e_photogalleryphotos) is sound. Only the joinType-vs-substring hygiene note stands between this and an approve, and it's not a blocker.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer

TL;DR — This PR fixes #3334: findAll(include="...") copied a nested association's INNER JOIN into unrelated sibling LEFT OUTER JOINs, generating SQL that referenced a not-yet-introduced table (ORA-00904 / MySQL "unknown column in on clause"). The fix threads a parentPosition through $expandedAssociations so grouping is decided from the association tree instead of a regex over the generated SQL, and removes the anchored-string gate that made include order change results. I traced the logic against the fixtures and all the code paths hold up; the change is correct, well-tested, and cleanly documented. Verdict: comment — no blocking findings; one intentional behaviour change worth a maintainer's eyes and one low-priority robustness note.

Correctness

The parentPosition bookkeeping is sound. In $expandedAssociations (vendor/wheels/model/sql.cfc:1315) the parent is read off the stack top before the delimiter loop pushes/pops, and the pushed value is local.i — which equals the entry's index in local.rv because exactly one ArrayAppend happens per pass. I traced all four fixture shapes (c_o_r_e_comments,classifications(tag), the reversed order, author,classifications(tag), and posts,user(galleries)) and each produces the asserted SQL, including the #3245 flat-join case staying flat.

The unbalanced-include guard at vendor/wheels/model/sql.cfc:1537 is correctly placed: ListDeleteAt on the one-element levels list tolerates the stray ), and the if (ArrayLen(local.parentPositions)) guard makes the parallel ArrayDeleteAt tolerate it too, so include="c_o_r_e_comments)" resolves as before instead of throwing can not remove Element at position [0].

One low-priority robustness note (non-blocking, and consistent with the file's existing idiom): the parent join-type check at vendor/wheels/model/sql.cfc:148

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

detects the parent's join type by substring-scanning the generated SQL. A parent LEFT OUTER JOIN whose table name happens to contain the substring inner (e.g. spinner, beginner) would match "INNER", be treated as an inner join, and its nested child would be emitted flat instead of grouped — silently demoting the row-preservation the fix restores. This is the same FindNoCase("INNER", ...join) approach the pre-fix code already used (old sql.cfc:136/:172), so it's not a regression and no real schema in the fixtures hits it. If you want to harden it, reading the parent's joinType from the association metadata (local.associations[local.parentPosition]) rather than scanning the SQL text would remove the ambiguity entirely — same spirit as the PR's own "read the tree, not the string" thesis.

Tests

Strong coverage in crudSpec.cfc. The five new specs pin: nested-inner scoping (#3334), root-level inner kept flat (#3334/#3245 residual), unbalanced-include tolerance, order-independence of the emitted SQL (string assertion), and order-independence of the result set (executable findAll). The author,classifications(tag) case correctly exercises a root-level belongsTo inner join alongside a nested group, and the updated #3245 comment accurately re-describes why that case stays flat. Fixtures line up: Post has belongsTo("author"), hasMany("c_o_r_e_comments"), hasMany("classifications"); Classification has belongsTo("tag") — matching every asserted join.

Cross-engine

No cross-engine red flags. The new code uses only ArrayAppend/ArrayDeleteAt/ArrayLen/StructKeyExists and sequential local.joins[local.i] assignment — no closures, no .map(), no reserved-scope shadowing, no application-scope member calls. Numeric struct keys on local.nestedJoins/local.isNested are the standard CFML position-keyed pattern and coerce to strings uniformly across Lucee/Adobe/BoxLang. Since this touches the join builder on every engine, the dispatched compat matrix is the right call before merge.

Docs

Changelog fragment changelog.d/3334-include-join-scoping.fixed.md is correctly named (<slug>.fixed.md) and honestly documents both the fix and the behaviour change, including the joinType="inner" migration note — no direct CHANGELOG.md [Unreleased] edit. Good.

Commits

Both commits conform to commitlint.config.js: fix(model): ..., valid type/scope, subjects under 100 chars and not ALL-CAPS, and each describes the "why."

Behaviour change (flagging, not blocking)

The removal of the anchored-string gate means include="a(b),c" and include="c,a(b)" now generate identical SQL, and the nested-first ordering returns more rows (the parent rows a hasMany/hasOne include exists to preserve) where it previously demoted the sibling to an inner join. This is the correct behaviour and is called out in both the PR body and the changelog with a migration path. Surfacing it here so a maintainer signs off on the semantics deliberately — it's isolated to the gate removal and revertable on its own if you'd prefer to ship only the fan-out fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQL Generation Bug: Variable state leakage in findAll(include="...") when mixed nested and shallow joins are used

1 participant