Skip to content

Commit 75f25c8

Browse files
stevebaumanstevebauman
authored
Merge pull request #806 from DirectoryTree/bug-802
Fix global scope query mutations
2 parents 7283a1e + 27ffcbf commit 75f25c8

4 files changed

Lines changed: 154 additions & 28 deletions

File tree

‎src/Query/Model/Builder.php‎

Lines changed: 46 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,17 @@ protected function callScope(callable $scope, array $parameters = []): static
133133
{
134134
array_unshift($parameters, $this);
135135

136-
return $scope(...array_values($parameters)) ?? $this;
136+
$result = null;
137+
138+
$scopeFilter = $this->captureScopeFilters(function () use ($scope, $parameters, &$result) {
139+
$result = $scope(...$parameters) ?? $this;
140+
});
141+
142+
if ($scopeFilter) {
143+
$this->query->addFilter($scopeFilter);
144+
}
145+
146+
return $result;
137147
}
138148

139149
/**
@@ -477,25 +487,47 @@ public function applyScopes(): static
477487
return $this;
478488
}
479489

480-
// Scopes should not be escapable, so we will wrap the
481-
// application of the scopes within a nested query.
482-
$this->where(function (self $query) {
483-
foreach ($this->scopes as $identifier => $scope) {
484-
if (isset($this->appliedScopes[$identifier])) {
485-
continue;
486-
}
490+
$builder = clone $this;
487491

492+
foreach ($this->scopes as $identifier => $scope) {
493+
if (! isset($builder->scopes[$identifier])) {
494+
continue;
495+
}
496+
497+
if (isset($builder->appliedScopes[$identifier])) {
498+
continue;
499+
}
500+
501+
$builder->callScope(function (self $builder) use ($scope) {
488502
if ($scope instanceof Scope) {
489-
$scope->apply($query, $this->getModel());
503+
$scope->apply($builder, $this->getModel());
490504
} else {
491-
$scope($this);
505+
$scope($builder);
492506
}
507+
});
493508

494-
$this->appliedScopes[$identifier] = $scope;
495-
}
496-
});
509+
$builder->appliedScopes[$identifier] = $scope;
510+
}
497511

498-
return $this;
512+
return $builder;
513+
}
514+
515+
/**
516+
* Capture the filters applied while executing the callback.
517+
*/
518+
protected function captureScopeFilters(Closure $callback): ?Filter
519+
{
520+
$originalFilter = $this->query->filter;
521+
522+
$this->query->filter = null;
523+
524+
try {
525+
$callback();
526+
527+
return $this->query->filter;
528+
} finally {
529+
$this->query->filter = $originalFilter;
530+
}
499531
}
500532

501533
/**

‎tests/Unit/Models/ActiveDirectory/ExchangeDatabaseTest.php‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?php
2+
3+
namespace LdapRecord\Tests\Unit\Models\ActiveDirectory;
4+
5+
use LdapRecord\Connection;
6+
use LdapRecord\Container;
7+
use LdapRecord\Models\ActiveDirectory\ExchangeDatabase;
8+
use LdapRecord\Testing\DirectoryFake;
9+
use LdapRecord\Testing\LdapFake;
10+
use LdapRecord\Tests\TestCase;
11+
12+
class ExchangeDatabaseTest extends TestCase
13+
{
14+
protected function setUp(): void
15+
{
16+
parent::setUp();
17+
18+
Container::addConnection(new Connection);
19+
20+
ExchangeDatabase::clearBootedModels();
21+
}
22+
23+
protected function tearDown(): void
24+
{
25+
DirectoryFake::tearDown();
26+
27+
parent::tearDown();
28+
}
29+
30+
public function test_configuration_context_scope_sets_the_query_base_dn()
31+
{
32+
DirectoryFake::setup()->getLdapConnection()->expect(
33+
LdapFake::operation('read')->once()->andReturn([
34+
[
35+
'dn' => '',
36+
'configurationNamingContext' => ['CN=Configuration,DC=local,DC=com'],
37+
],
38+
])
39+
);
40+
41+
$this->assertSame(
42+
'CN=Configuration,DC=local,DC=com',
43+
ExchangeDatabase::query()->toBase()->getDn()
44+
);
45+
}
46+
}

‎tests/Unit/Models/ModelScopeTest.php‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public function test_scopes_are_not_stacked_multiple_times()
8383
$query->toBase();
8484
$query->toBase();
8585

86-
$this->assertEquals('(&(foo=bar))', $query->toBase()->getQuery());
86+
$this->assertEquals('(foo=bar)', $query->toBase()->getQuery());
8787
}
8888

8989
public function test_local_scopes_can_be_called()
@@ -138,11 +138,11 @@ public function test_scopes_do_not_impact_model_find()
138138

139139
public function test_scopes_cannot_be_negated_by_or_clauses()
140140
{
141-
// The scope should be wrapped in its own AND group, so the OR clause
142-
// cannot negate it. The filter should be: (&(bypass=...)(&(foo=bar)))
143-
// NOT: (|(foo=bar)(bypass=...)) which would allow bypassing the scope
141+
// The scope should be composed onto the query using AND, so the OR clause
142+
// cannot negate it. The filter should be: (&(bypass=...)(foo=bar))
143+
// NOT: (|(foo=bar)(bypass=...)) which would allow bypassing the scope.
144144
$this->assertEquals(
145-
'(&(bypass=\61\74\74\65\6d\70\74)(&(foo=bar)))',
145+
'(&(bypass=\61\74\74\65\6d\70\74)(foo=bar))',
146146
ModelWithGlobalScopeTestStub::query()
147147
->orWhere('bypass', '=', 'attempt')
148148
->toBase()
@@ -154,7 +154,7 @@ public function test_scopes_cannot_be_negated_by_multiple_or_clauses()
154154
{
155155
// Scope must remain enforced regardless of OR clauses
156156
$this->assertEquals(
157-
'(&(|(bypass1=\61\74\74\65\6d\70\74\31)(bypass2=\61\74\74\65\6d\70\74\32))(&(foo=bar)))',
157+
'(&(|(bypass1=\61\74\74\65\6d\70\74\31)(bypass2=\61\74\74\65\6d\70\74\32))(foo=bar))',
158158
ModelWithGlobalScopeTestStub::query()
159159
->orWhere('bypass1', '=', 'attempt1')
160160
->orWhere('bypass2', '=', 'attempt2')
@@ -167,7 +167,7 @@ public function test_scopes_remain_enforced_with_complex_queries()
167167
{
168168
// The scope (foo=bar) must always be present and enforced at the root AND level
169169
$this->assertEquals(
170-
'(&(&(|(name=\4a\6f\68\6e)(name=\4a\61\6e\65))(active=\74\72\75\65))(&(foo=bar)))',
170+
'(&(|(name=\4a\6f\68\6e)(name=\4a\61\6e\65))(active=\74\72\75\65)(foo=bar))',
171171
ModelWithGlobalScopeTestStub::query()
172172
->where('name', '=', 'John')
173173
->orWhere('name', '=', 'Jane')

‎tests/Unit/Query/Model/BuilderScopeTest.php‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,16 @@ public function test_closure_scopes_can_be_applied()
1818
{
1919
$b = new Builder(new Entry, new QueryBuilder(new Connection));
2020

21-
$b->withGlobalScope('foo', function ($query) use ($b) {
22-
$this->assertSame($b, $query);
21+
$scoped = null;
22+
23+
$b->withGlobalScope('foo', function ($query) use (&$scoped) {
24+
$scoped = $query;
2325
});
2426

25-
$b->applyScopes();
27+
$applied = $b->applyScopes();
28+
29+
$this->assertNotSame($b, $applied);
30+
$this->assertSame($applied, $scoped);
2631
}
2732

2833
public function test_class_scopes_can_be_applied()
@@ -33,11 +38,54 @@ public function test_class_scopes_can_be_applied()
3338

3439
$b->withGlobalScope('foo', new TestModelScope);
3540

36-
// Scopes are wrapped in an AndGroup to prevent negation
37-
$this->assertEquals('(&(foo=LdapRecord\Models\Entry))', $b->getUnescapedQuery());
41+
// Scope filters are composed onto the scoped query builder.
42+
$scoped = $b->applyScopes();
43+
44+
$this->assertEquals('(foo=LdapRecord\Models\Entry)', $scoped->getUnescapedQuery());
45+
46+
$this->assertEmpty($b->appliedScopes());
47+
$this->assertCount(1, $scoped->appliedScopes());
48+
$this->assertArrayHasKey('foo', $scoped->appliedScopes());
49+
}
50+
51+
public function test_scope_or_filters_are_preserved_when_grouped()
52+
{
53+
$b = new Builder(new Entry, new QueryBuilder(new Connection));
54+
55+
$b->where('cn', '=', 'John Doe');
56+
57+
$b->withGlobalScope('foo', function ($query) {
58+
$query->orWhere('foo', '=', 'bar');
59+
$query->orWhere('bar', '=', 'baz');
60+
});
61+
62+
$this->assertEquals(
63+
'(&(cn=John Doe)(|(foo=bar)(bar=baz)))',
64+
$b->toBase()->getUnescapedQuery()
65+
);
66+
}
67+
68+
public function test_complex_scope_filters_cannot_be_negated_by_complex_queries()
69+
{
70+
$b = new Builder(new Entry, new QueryBuilder(new Connection));
71+
72+
$b->where('department', '=', 'Sales')
73+
->orWhere('department', '=', 'Support')
74+
->where('enabled', '=', 'true');
75+
76+
$b->withGlobalScope('foo', function ($query) {
77+
$query->orFilter(function ($query) {
78+
$query->where('type', '=', 'person');
79+
$query->where('type', '=', 'contact');
80+
});
81+
82+
$query->where('tenant', '=', 'acme');
83+
});
3884

39-
$this->assertCount(1, $b->appliedScopes());
40-
$this->assertArrayHasKey('foo', $b->appliedScopes());
85+
$this->assertEquals(
86+
'(&(|(department=Sales)(department=Support))(enabled=true)(&(|(type=person)(type=contact))(tenant=acme)))',
87+
$b->toBase()->getUnescapedQuery()
88+
);
4189
}
4290

4391
public function test_scopes_can_be_removed_after_being_added()

0 commit comments

Comments
 (0)