Skip to content

Commit fe54f1a

Browse files
bpamiribpamiri
committed
fix(model): scope a nested include's inner join to its own parent join
`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>
1 parent ab901cf commit fe54f1a

10 files changed

Lines changed: 596 additions & 113 deletions

‎changelog.d/3334-include-join-scoping.fixed.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
- `findAll(include="...")` no longer copies a nested association's `INNER JOIN` into unrelated sibling joins. When a nested group followed one or more shallow associations (e.g. `include="comments,classifications(tag)"`), the issue #449 parenthesized grouping spliced the nested `INNER JOIN` into every preceding `LEFT OUTER JOIN`, so those joins referenced a table the query had not introduced yet — Oracle rejected it with `ORA-00904: invalid identifier`, MySQL with `Unknown column '<table>.<column>' in 'on clause'`. Each `INNER JOIN` is now scoped to the single association it is nested under, taken from the include's association tree rather than re-derived from the generated SQL text. Reported with a working patch by Mike Grogan (#3334)
2+
- **Behaviour change:** `include` order no longer changes the SQL a query generates. Grouping used to be gated on an anchored pattern over the include string that only matched when the nested group came last, so `include="a(b),c"` and `include="c,a(b)"` produced structurally different joins for the same query. 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 that had no associated record. Both orderings now emit the same joins, so a query written in the nested-first form can return **more** rows than before — the rows a `hasMany`/`hasOne` include is meant to preserve. Pass `joinType="inner"` on the association if the filtering was intentional (#3334)

‎public/testbox/system/stubs/139184C0543CCDB338AEFB643110CB89.cfm‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<cfscript>
2+
variables[ "closeSSEStream" ] = variables[ "tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89" ];
3+
this[ "closeSSEStream" ] = variables[ "tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89" );
7+
structDelete( this, "tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89" );
8+
public void function tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "closeSSEStream";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["closeSSEStream"], arguments );
51+
}
52+
</cfscript>

‎public/testbox/system/stubs/16CA394252B074478BEBD2B3A9C8EA82.cfm‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<cfscript>
2+
variables[ "sendSSEComment" ] = variables[ "tmp_sendSSEComment_16CA394252B074478BEBD2B3A9C8EA82" ];
3+
this[ "sendSSEComment" ] = variables[ "tmp_sendSSEComment_16CA394252B074478BEBD2B3A9C8EA82" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_sendSSEComment_16CA394252B074478BEBD2B3A9C8EA82" );
7+
structDelete( this, "tmp_sendSSEComment_16CA394252B074478BEBD2B3A9C8EA82" );
8+
public void function tmp_sendSSEComment_16CA394252B074478BEBD2B3A9C8EA82(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "sendSSEComment";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["sendSSEComment"], arguments );
51+
}
52+
</cfscript>

‎public/testbox/system/stubs/30942F4D0BCB6139072EF27C66218715.cfm‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<cfscript>
2+
variables[ "checkError" ] = variables[ "tmp_checkError_30942F4D0BCB6139072EF27C66218715" ];
3+
this[ "checkError" ] = variables[ "tmp_checkError_30942F4D0BCB6139072EF27C66218715" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_checkError_30942F4D0BCB6139072EF27C66218715" );
7+
structDelete( this, "tmp_checkError_30942F4D0BCB6139072EF27C66218715" );
8+
public any function tmp_checkError_30942F4D0BCB6139072EF27C66218715(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "checkError";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["checkError"], arguments );
51+
52+
if (resultsLen neq 0) {
53+
if (internalCounter gt resultsLen) {
54+
resultsCounter = internalCounter - ( resultsLen * fix( ( internalCounter - 1 ) / resultsLen ) );
55+
return results[ resultsKey ][ resultsCounter ];
56+
} else {
57+
return results[ resultsKey ][ internalCounter ];
58+
}
59+
}
60+
61+
if ( callbackLen neq 0 ) {
62+
fCallBack = this._mockCallbacks[ resultsKey ].first();
63+
return fCallBack( argumentCollection : arguments );
64+
}
65+
66+
if ( not isSimpleValue( fCallBack ) ){
67+
return fCallBack( argumentCollection : arguments );
68+
}
69+
}
70+
</cfscript>

‎public/testbox/system/stubs/754BEF48E30B63BC11E518FA73C07785.cfm‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<cfscript>
2+
variables[ "sendSSEEvent" ] = variables[ "tmp_sendSSEEvent_754BEF48E30B63BC11E518FA73C07785" ];
3+
this[ "sendSSEEvent" ] = variables[ "tmp_sendSSEEvent_754BEF48E30B63BC11E518FA73C07785" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_sendSSEEvent_754BEF48E30B63BC11E518FA73C07785" );
7+
structDelete( this, "tmp_sendSSEEvent_754BEF48E30B63BC11E518FA73C07785" );
8+
public void function tmp_sendSSEEvent_754BEF48E30B63BC11E518FA73C07785(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "sendSSEEvent";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["sendSSEEvent"], arguments );
51+
}
52+
</cfscript>

‎public/testbox/system/stubs/B7681C49C5125395612F24D97559A4C2.cfm‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<cfscript>
2+
variables[ "initSSEStream" ] = variables[ "tmp_initSSEStream_B7681C49C5125395612F24D97559A4C2" ];
3+
this[ "initSSEStream" ] = variables[ "tmp_initSSEStream_B7681C49C5125395612F24D97559A4C2" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_initSSEStream_B7681C49C5125395612F24D97559A4C2" );
7+
structDelete( this, "tmp_initSSEStream_B7681C49C5125395612F24D97559A4C2" );
8+
public any function tmp_initSSEStream_B7681C49C5125395612F24D97559A4C2(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "initSSEStream";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["initSSEStream"], arguments );
51+
52+
if (resultsLen neq 0) {
53+
if (internalCounter gt resultsLen) {
54+
resultsCounter = internalCounter - ( resultsLen * fix( ( internalCounter - 1 ) / resultsLen ) );
55+
return results[ resultsKey ][ resultsCounter ];
56+
} else {
57+
return results[ resultsKey ][ internalCounter ];
58+
}
59+
}
60+
61+
if ( callbackLen neq 0 ) {
62+
fCallBack = this._mockCallbacks[ resultsKey ].first();
63+
return fCallBack( argumentCollection : arguments );
64+
}
65+
66+
if ( not isSimpleValue( fCallBack ) ){
67+
return fCallBack( argumentCollection : arguments );
68+
}
69+
}
70+
</cfscript>

‎public/testbox/system/stubs/C383511C024245809F1F73E0CA52E220.cfm‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<cfscript>
2+
variables[ "poll" ] = variables[ "tmp_poll_C383511C024245809F1F73E0CA52E220" ];
3+
this[ "poll" ] = variables[ "tmp_poll_C383511C024245809F1F73E0CA52E220" ];
4+
5+
// Clean up
6+
structDelete( variables, "tmp_poll_C383511C024245809F1F73E0CA52E220" );
7+
structDelete( this, "tmp_poll_C383511C024245809F1F73E0CA52E220" );
8+
public any function tmp_poll_C383511C024245809F1F73E0CA52E220(
9+
10+
) output=true {
11+
12+
var results = this._mockResults;
13+
var resultsKey = "poll";
14+
var resultsCounter = 0;
15+
var internalCounter = 0;
16+
var resultsLen = 0;
17+
var callbackLen = 0;
18+
var argsHashKey = resultsKey & "|" & this.mockBox.normalizeArguments( arguments );
19+
var fCallBack = "";
20+
21+
// If Method & argument Hash Results, switch the results struct
22+
if (structKeyExists( this._mockArgResults, argsHashKey) ) {
23+
// Check if it is a callback
24+
if (isStruct( this._mockArgResults[ argsHashKey ]) &&
25+
structKeyExists( this._mockArgResults[ argsHashKey ], "type" ) &&
26+
structKeyExists( this._mockArgResults[ argsHashKey ], "target" ) ) {
27+
fCallBack = this._mockArgResults[ argsHashKey ].target;
28+
} else {
29+
// switch context and key
30+
results = this._mockArgResults;
31+
resultsKey = argsHashKey;
32+
}
33+
}
34+
35+
// Get the statemachine counter
36+
if (isSimpleValue( fCallBack) ) {
37+
resultsLen = arrayLen( results[ resultsKey ] );
38+
}
39+
40+
// Get the callback counter, if it exists
41+
if (structKeyExists( this._mockCallbacks, resultsKey) ) {
42+
callbackLen = arrayLen( this._mockCallbacks[ resultsKey ] );
43+
}
44+
45+
// Log the Method Call
46+
this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] = this._mockMethodCallCounters[ listFirst( resultsKey, "|" ) ] + 1;
47+
48+
// Get the CallCounter Reference
49+
internalCounter = this._mockMethodCallCounters[listFirst(resultsKey,"|")];
50+
arrayAppend( this._mockCallLoggers["poll"], arguments );
51+
52+
if (resultsLen neq 0) {
53+
if (internalCounter gt resultsLen) {
54+
resultsCounter = internalCounter - ( resultsLen * fix( ( internalCounter - 1 ) / resultsLen ) );
55+
return results[ resultsKey ][ resultsCounter ];
56+
} else {
57+
return results[ resultsKey ][ internalCounter ];
58+
}
59+
}
60+
61+
if ( callbackLen neq 0 ) {
62+
fCallBack = this._mockCallbacks[ resultsKey ].first();
63+
return fCallBack( argumentCollection : arguments );
64+
}
65+
66+
if ( not isSimpleValue( fCallBack ) ){
67+
return fCallBack( argumentCollection : arguments );
68+
}
69+
}
70+
</cfscript>

0 commit comments

Comments
 (0)