Skip to content

Commit b0e7dd6

Browse files
roxblnfkroxblnfk
committed
Support wrapWhere() method in QueryBuilder for joined scopes
1 parent b8b9468 commit b0e7dd6

5 files changed

Lines changed: 227 additions & 2 deletions

File tree

‎src/Select/QueryBuilder.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,9 @@ private function targetFunc(string $call): callable
204204
case 'andwhere':
205205
$call = 'and' . \ucfirst($this->forward);
206206
break;
207+
case 'wrapwhere':
208+
$call = 'wrap' . \ucfirst($this->forward);
209+
break;
207210
}
208211
}
209212

‎src/Select/ScopeInterface.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@
1515
*
1616
* The {@see apply()} method receives a {@see QueryBuilder} that proxies the
1717
* underlying query. For scopes attached to joined loaders the builder forwards
18-
* `where*` calls to the JOIN ON tokens (`onWhere`) rather than the top-level
19-
* WHERE — handle accordingly.
18+
* `where*` and `wrapWhere()` calls to JOIN ON tokens (`onWhere`/`wrapOnWhere`)
19+
* rather than the top-level WHERE — the recommended pattern below applies
20+
* uniformly in both cases.
2021
*
2122
* ## Registering a scope
2223
*

‎tests/ORM/Fixtures/WrappedQueryScope.php‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<?php
2+
3+
// phpcs:ignoreFile
4+
declare(strict_types=1);
5+
6+
namespace Cycle\ORM\Tests\Fixtures;
7+
8+
use Cycle\ORM\Select\QueryBuilder;
9+
use Cycle\ORM\Select\ScopeInterface;
10+
11+
/**
12+
* Like {@see \Cycle\ORM\Select\QueryScope}, but calls wrapWhere() before adding its
13+
* conditions — protects the scope's WHERE/ON tokens from being bypassed by a later
14+
* `orWhere`/`orOnWhere`. For joined-loader scopes, wrapWhere() is forwarded to
15+
* wrapOnWhere() by {@see QueryBuilder::targetFunc()}.
16+
*/
17+
final class WrappedQueryScope implements ScopeInterface
18+
{
19+
public function __construct(
20+
private array $where,
21+
private array $orderBy = [],
22+
) {}
23+
24+
public function apply(QueryBuilder $query): void
25+
{
26+
$query->wrapWhere();
27+
$query->where($this->where)->orderBy($this->orderBy);
28+
}
29+
}

‎tests/ORM/Functional/Driver/Common/Relation/HasMany/HasManyScopeTest.php‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Cycle\ORM\Tests\Fixtures\Comment;
1616
use Cycle\ORM\Tests\Fixtures\SortByIDScope;
1717
use Cycle\ORM\Tests\Fixtures\User;
18+
use Cycle\ORM\Tests\Fixtures\WrappedQueryScope;
1819
use Cycle\ORM\Tests\Traits\TableTrait;
1920
use Cycle\Database\Exception\StatementException;
2021

@@ -442,6 +443,109 @@ public function testInloadWithScopeOrderedAndWhere(): void
442443
$this->assertSame('msg 2.3', $res[0]->comments[0]->message);
443444
}
444445

446+
/**
447+
* Documents the historical bug: when a joined-loader scope adds a plain top-level
448+
* condition (no wrapWhere), an adversarial `orWhere` injected via the 'load' option
449+
* bypasses the scope due to AND-over-OR precedence inside the JOIN's ON clause.
450+
*/
451+
public function testInloadJoinedScopeWithoutWrapWhereIsBypassedByAdversarialOrLoad(): void
452+
{
453+
$this->orm = $this->withCommentsSchema([
454+
// Plain QueryScope — adds `level >= 2` as a flat AND token, no wrap.
455+
Schema::SCOPE => new Select\QueryScope(['@.level' => ['>=' => 2]]),
456+
]);
457+
458+
$res = (new Select($this->orm, User::class))
459+
->load('comments', [
460+
'method' => JoinableLoader::INLOAD,
461+
'load' => static function (Select\QueryBuilder $q): void {
462+
// top-level ON-condition WHERE + OR — the scope's AND lands at the
463+
// tail of the same flat token list:
464+
// ON join_key AND level = 1 OR message = 'msg 4' AND level >= 2
465+
// ≡ (join_key AND level = 1) OR (message = 'msg 4' AND level >= 2)
466+
// The first OR-arm ignores `level >= 2` — scope bypassed.
467+
$q->where('@.level', 1)
468+
->orWhere('@.message', 'msg 4');
469+
},
470+
])
471+
->orderBy('user.id')
472+
->fetchAll();
473+
474+
[$userA, $userB] = $res;
475+
476+
// User A: msg 1 (level=1) sneaks through the first OR-arm; msg 4 matches the
477+
// second arm legitimately.
478+
$this->assertCount(2, $userA->comments);
479+
$aMessages = $this->extractMessages($userA->comments);
480+
$this->assertSame(['msg 1', 'msg 4'], $aMessages);
481+
482+
// User B: msg 2.1 (level=1) sneaks through the first arm — scope leaks again.
483+
$this->assertCount(1, $userB->comments);
484+
$this->assertSame('msg 2.1', $userB->comments[0]->message);
485+
}
486+
487+
/**
488+
* Companion to the test above: with {@see WrappedQueryScope} (calls wrapWhere() first),
489+
* QueryBuilder::targetFunc() forwards the wrap to `wrapOnWhere` on the JOIN's ON tokens,
490+
* enclosing the user's OR group. The scope's `level >= 2` condition stays effective.
491+
*/
492+
public function testInloadJoinedScopeWithWrapWhereProtectsAgainstAdversarialOrLoad(): void
493+
{
494+
$this->orm = $this->withCommentsSchema([
495+
Schema::SCOPE => new WrappedQueryScope(['@.level' => ['>=' => 2]]),
496+
]);
497+
498+
$res = (new Select($this->orm, User::class))
499+
->load('comments', [
500+
'method' => JoinableLoader::INLOAD,
501+
'load' => static function (Select\QueryBuilder $q): void {
502+
$q->where('@.level', 1)
503+
->orWhere('@.message', 'msg 4');
504+
},
505+
])
506+
->orderBy('user.id')
507+
->fetchAll();
508+
509+
[$userA, $userB] = $res;
510+
511+
// ON ((join_key AND level = 1) OR message = 'msg 4') AND level >= 2
512+
// User A: only msg 4 (level = 4) matches the OR group AND scope.
513+
$this->assertCount(1, $userA->comments);
514+
$this->assertSame('msg 4', $userA->comments[0]->message);
515+
516+
// User B: no comment satisfies both the OR group and `level >= 2`.
517+
$this->assertCount(0, $userB->comments);
518+
}
519+
520+
/**
521+
* Sanity check: a wrap-aware scope works correctly in plain INLOAD joining without
522+
* any adversarial conditions — wrapWhere() on empty ON tokens is a no-op and the
523+
* scope adds its condition at the top level of the JOIN's ON clause.
524+
*/
525+
public function testInloadJoinedScopeWithWrapWhereWithoutAdversarialLoad(): void
526+
{
527+
$this->orm = $this->withCommentsSchema([
528+
Schema::SCOPE => new WrappedQueryScope(['@.level' => ['>=' => 3]]),
529+
]);
530+
531+
$res = (new Select($this->orm, User::class))
532+
->load('comments', [
533+
'method' => JoinableLoader::INLOAD,
534+
])
535+
->orderBy('user.id')
536+
->fetchAll();
537+
538+
[$userA, $userB] = $res;
539+
540+
// Only level >= 3 comments survive: msg 3, msg 4, msg 2.3
541+
$this->assertCount(2, $userA->comments);
542+
$aMessages = $this->extractMessages($userA->comments);
543+
$this->assertSame(['msg 3', 'msg 4'], $aMessages);
544+
545+
$this->assertCount(1, $userB->comments);
546+
$this->assertSame('msg 2.3', $userB->comments[0]->message);
547+
}
548+
445549
public function testInvalidOrderBy(): void
446550
{
447551
$this->expectException(StatementException::class);
@@ -457,6 +561,20 @@ public function testInvalidOrderBy(): void
457561
])->orderBy('user.id', 'DESC')->fetchAll();
458562
}
459563

564+
/**
565+
* @return list<string>
566+
*/
567+
private function extractMessages(iterable $comments): array
568+
{
569+
$messages = [];
570+
foreach ($comments as $comment) {
571+
$messages[] = $comment->message;
572+
}
573+
\sort($messages);
574+
575+
return $messages;
576+
}
577+
460578
public function setUp(): void
461579
{
462580
parent::setUp();

‎tests/ORM/Unit/Select/QueryBuilderTest.php‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,80 @@ public function testWhereCallDoesNotThrowTypeError(): void
4545
$this->assertNotEmpty($builder->getQuery()->getTokens()['where']);
4646
}
4747

48+
public function testWrapWhereForwardsToWrapOnWhereInJoinedMode(): void
49+
{
50+
// A scope attached to a joined loader gets a QueryBuilder with forward='onWhere'.
51+
// wrapWhere() must follow the same routing as where/orWhere/andWhere — otherwise
52+
// the scope would wrap the parent's WHERE instead of its own JOIN's ON tokens.
53+
$query = new SelectQuery(['users']);
54+
$query
55+
->leftJoin('posts')
56+
->on('posts.user_id', 'users.id')
57+
->onWhere('posts.published', true)
58+
->orOnWhere('posts.featured', true);
59+
60+
// Pretend we also have a user-level WHERE so we can verify wrapWhere
61+
// does NOT touch it under the forwarded routing.
62+
$query->where('users.active', true);
63+
64+
$loader = $this->createMock(AbstractLoader::class);
65+
$loader->method('fieldAlias')->willReturn(null);
66+
$loader->method('getAlias')->willReturn('users');
67+
$loader->method('getTarget')->willReturn('user');
68+
$loader->method('getParentLoader')->willReturn(null);
69+
70+
$builder = (new QueryBuilder($query, $loader))->withForward('onWhere');
71+
72+
$builder->wrapWhere();
73+
74+
$tokens = $query->getTokens();
75+
76+
// JOIN's ON tokens are now a single wrapped group.
77+
$onTokens = $tokens['join'][1]['on'];
78+
$this->assertSame(['AND', '('], $onTokens[0]);
79+
$this->assertSame(['', ')'], $onTokens[\count($onTokens) - 1]);
80+
81+
// Top-level WHERE is untouched — no extra parenthesizing on the user's clause.
82+
$whereTokens = $tokens['where'];
83+
$this->assertCount(1, $whereTokens);
84+
$this->assertSame('AND', $whereTokens[0][0]);
85+
$this->assertSame('users.active', $whereTokens[0][1][0]);
86+
}
87+
88+
public function testWrapWhereWithoutForwardTargetsRootWhere(): void
89+
{
90+
// Counter-case: without forwarding (root scope), wrapWhere wraps the
91+
// top-level WHERE tokens, not any JOIN's ON tokens.
92+
$query = new SelectQuery(['users']);
93+
$query
94+
->where('users.id', 1)
95+
->orWhere('users.id', 2)
96+
->leftJoin('posts')->on('posts.user_id', 'users.id')->onWhere('posts.published', true);
97+
98+
$loader = $this->createMock(AbstractLoader::class);
99+
$loader->method('fieldAlias')->willReturn(null);
100+
$loader->method('getAlias')->willReturn('users');
101+
$loader->method('getTarget')->willReturn('user');
102+
$loader->method('getParentLoader')->willReturn(null);
103+
104+
$builder = new QueryBuilder($query, $loader); // no forward
105+
106+
$builder->wrapWhere();
107+
108+
$tokens = $query->getTokens();
109+
110+
// WHERE tokens are wrapped.
111+
$whereTokens = $tokens['where'];
112+
$this->assertSame(['AND', '('], $whereTokens[0]);
113+
$this->assertSame(['', ')'], $whereTokens[\count($whereTokens) - 1]);
114+
115+
// JOIN ON tokens are NOT wrapped — they still start with the plain join key.
116+
$onTokens = $tokens['join'][1]['on'];
117+
$this->assertSame('AND', $onTokens[0][0]);
118+
// First token's payload is the [column, operator, value] triplet, not the opening paren.
119+
$this->assertIsArray($onTokens[0][1]);
120+
}
121+
48122
private function makeBuilder(): QueryBuilder
49123
{
50124
$query = new SelectQuery(['users']);

0 commit comments

Comments
 (0)