Merge pull request #805 from DirectoryTree/bug-804

Fix nested filter grouping order sensitivity

Author: stevebauman

src/Query/Builder.php

@@ -764,7 +764,7 @@ public function andFilter(Closure $closure): static
         $query = $this->newNestedInstance($closure);
 
         if ($filter = $query->getFilter()) {
-            $this->addFilter(new AndGroup(
+            $this->addFilter(AndGroup::nested(
                 ...$this->extractNestedFilters($filter)
             ), wrap: false);
         }
@@ -780,7 +780,7 @@ public function orFilter(Closure $closure): static
         $query = $this->newNestedInstance($closure);
 
         if ($filter = $query->getFilter()) {
-            $this->addFilter(new OrGroup(
+            $this->addFilter(OrGroup::nested(
                 ...$this->extractNestedFilters($filter)
             ), wrap: false);
         }
@@ -1162,11 +1162,11 @@ public function addFilter(Filter $filter, string $boolean = 'and', bool $wrap =
         // Flatten same-type groups to avoid deeply nested structures.
         // Ex: AndGroup(AndGroup(a, b), c) becomes AndGroup(a, b, c)
         if ($boolean === 'or') {
-            $this->filter = $this->filter instanceof OrGroup && $wrap
+            $this->filter = $this->filter instanceof OrGroup && ! $this->filter->isNested() && $wrap
                 ? new OrGroup(...[...$this->filter->getFilters(), $filter])
                 : new OrGroup($this->filter, $filter);
         } else {
-            $this->filter = $this->filter instanceof AndGroup && $wrap
+            $this->filter = $this->filter instanceof AndGroup && ! $this->filter->isNested() && $wrap
                 ? new AndGroup(...[...$this->filter->getFilters(), $filter])
                 : new AndGroup($this->filter, $filter);
         }

src/Query/ExtractsNestedFilters.php

@@ -2,6 +2,7 @@
 
 namespace LdapRecord\Query;
 
+use LdapRecord\Query\Filter\BooleanGroup;
 use LdapRecord\Query\Filter\Filter;
 use LdapRecord\Query\Filter\GroupFilter;
 
@@ -14,7 +15,11 @@ trait ExtractsNestedFilters
      */
     protected function extractNestedFilters(Filter $filter): array
     {
-        if (! $filter instanceof GroupFilter) {
+        if (! $filter instanceof BooleanGroup) {
+            return [$filter];
+        }
+
+        if ($filter->isNested()) {
             return [$filter];
         }
 

src/Query/Filter/AndGroup.php

@@ -2,54 +2,13 @@
 
 namespace LdapRecord\Query\Filter;
 
-class AndGroup implements GroupFilter
+class AndGroup extends BooleanGroup
 {
-    /**
-     * The filters in the group.
-     *
-     * @var Filter[]
-     */
-    protected array $filters;
-
-    /**
-     * Create a new AND group filter.
-     */
-    public function __construct(Filter ...$filters)
-    {
-        $this->filters = $filters;
-    }
-
-    /**
-     * Get the filters in the group.
-     *
-     * @return Filter[]
-     */
-    public function getFilters(): array
-    {
-        return $this->filters;
-    }
-
     /**
      * {@inheritdoc}
      */
     public function getOperator(): string
     {
         return '&';
     }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getRaw(): string
-    {
-        return '&'.implode($this->filters);
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function __toString(): string
-    {
-        return '('.$this->getRaw().')';
-    }
 }

src/Query/Filter/BooleanGroup.php

@@ -0,0 +1,72 @@
+<?php
+
+namespace LdapRecord\Query\Filter;
+
+abstract class BooleanGroup implements GroupFilter
+{
+    /**
+     * The filters in the group.
+     *
+     * @var Filter[]
+     */
+    protected array $filters;
+
+    /**
+     * Determine if this group should be preserved when nesting filters.
+     */
+    protected bool $nested = false;
+
+    /**
+     * Create a new boolean group filter.
+     */
+    public function __construct(Filter ...$filters)
+    {
+        $this->filters = $filters;
+    }
+
+    /**
+     * Create a new nested boolean group filter.
+     */
+    public static function nested(Filter ...$filters): static
+    {
+        $group = new static(...$filters);
+
+        $group->nested = true;
+
+        return $group;
+    }
+
+    /**
+     * Determine if this group should be preserved when nesting filters.
+     */
+    public function isNested(): bool
+    {
+        return $this->nested;
+    }
+
+    /**
+     * Get the filters in the group.
+     *
+     * @return Filter[]
+     */
+    public function getFilters(): array
+    {
+        return $this->filters;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getRaw(): string
+    {
+        return $this->getOperator().implode($this->filters);
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function __toString(): string
+    {
+        return '('.$this->getRaw().')';
+    }
+}

src/Query/Filter/OrGroup.php

@@ -2,54 +2,13 @@
 
 namespace LdapRecord\Query\Filter;
 
-class OrGroup implements GroupFilter
+class OrGroup extends BooleanGroup
 {
-    /**
-     * The filters in the group.
-     *
-     * @var Filter[]
-     */
-    protected array $filters;
-
-    /**
-     * Create a new OR group filter.
-     */
-    public function __construct(Filter ...$filters)
-    {
-        $this->filters = $filters;
-    }
-
-    /**
-     * Get the filters in the group.
-     *
-     * @return Filter[]
-     */
-    public function getFilters(): array
-    {
-        return $this->filters;
-    }
-
     /**
      * {@inheritdoc}
      */
     public function getOperator(): string
     {
         return '|';
     }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getRaw(): string
-    {
-        return '|'.implode($this->filters);
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function __toString(): string
-    {
-        return '('.$this->getRaw().')';
-    }
 }

src/Query/Model/Builder.php

@@ -730,7 +730,7 @@ public function andFilter(Closure $closure): static
         $query = $this->newNestedModelInstance($closure);
 
         if ($filter = $query->getQuery()->getFilter()) {
-            $this->query->addFilter(new AndGroup(
+            $this->query->addFilter(AndGroup::nested(
                 ...$this->extractNestedFilters($filter)
             ), wrap: false);
         }
@@ -746,7 +746,7 @@ public function orFilter(Closure $closure): static
         $query = $this->newNestedModelInstance($closure);
 
         if ($filter = $query->getQuery()->getFilter()) {
-            $this->query->addFilter(new OrGroup(
+            $this->query->addFilter(OrGroup::nested(
                 ...$this->extractNestedFilters($filter)
             ), wrap: false);
         }

tests/Unit/Query/BuilderTest.php

@@ -786,6 +786,36 @@ public function test_nested_or_filter_with_where_equals()
         $this->assertEquals('(|(foo=1)(foo=2))', $query);
     }
 
+    public function test_nested_or_filter_preserves_nested_and_filter_when_followed_by_where()
+    {
+        $b = $this->newBuilder();
+
+        $query = $b->orFilter(function ($query) {
+            $query->andFilter(function ($query) {
+                $query->whereStartsWith('givenName', 'John');
+                $query->whereStartsWith('sn', 'Smith');
+            });
+            $query->where('mail', '=', 'John Smith');
+        })->getUnescapedQuery();
+
+        $this->assertEquals('(|(&(givenName=John*)(sn=Smith*))(mail=John Smith))', $query);
+    }
+
+    public function test_nested_or_filter_preserves_nested_and_filter_when_preceded_by_where()
+    {
+        $b = $this->newBuilder();
+
+        $query = $b->orFilter(function ($query) {
+            $query->where('mail', '=', 'John Smith');
+            $query->andFilter(function ($query) {
+                $query->whereStartsWith('givenName', 'John');
+                $query->whereStartsWith('sn', 'Smith');
+            });
+        })->getUnescapedQuery();
+
+        $this->assertEquals('(|(mail=John Smith)(&(givenName=John*)(sn=Smith*)))', $query);
+    }
+
     public function test_nested_and_filter()
     {
         $b = $this->newBuilder();
@@ -816,6 +846,19 @@ public function test_nested_not_filter()
         $this->assertEquals('(!(&(one=one)(two=two)))', $query);
     }
 
+    public function test_nested_and_filter_preserves_not_filter()
+    {
+        $b = $this->newBuilder();
+
+        $query = $b->andFilter(function ($query) {
+            $query->notFilter(function ($query) {
+                $query->whereEquals('foo', 'bar');
+            });
+        })->getUnescapedQuery();
+
+        $this->assertEquals('(&(!(foo=bar)))', $query);
+    }
+
     public function test_nested_filters()
     {
         $b = $this->newBuilder();

tests/Unit/Query/Model/ActiveDirectoryBuilderTest.php

@@ -178,6 +178,36 @@ public function test_or_filter_extracts_filters_from_nested_query()
         $this->assertEquals('(|(foo=1)(foo=2))', $query);
     }
 
+    public function test_or_filter_preserves_nested_and_filter_when_followed_by_where()
+    {
+        $b = $this->newBuilder();
+
+        $query = $b->orFilter(function ($query) {
+            $query->andFilter(function ($query) {
+                $query->whereStartsWith('givenName', 'John');
+                $query->whereStartsWith('sn', 'Smith');
+            });
+            $query->where('mail', '=', 'John Smith');
+        })->getUnescapedQuery();
+
+        $this->assertEquals('(|(&(givenName=John*)(sn=Smith*))(mail=John Smith))', $query);
+    }
+
+    public function test_or_filter_preserves_nested_and_filter_when_preceded_by_where()
+    {
+        $b = $this->newBuilder();
+
+        $query = $b->orFilter(function ($query) {
+            $query->where('mail', '=', 'John Smith');
+            $query->andFilter(function ($query) {
+                $query->whereStartsWith('givenName', 'John');
+                $query->whereStartsWith('sn', 'Smith');
+            });
+        })->getUnescapedQuery();
+
+        $this->assertEquals('(|(mail=John Smith)(&(givenName=John*)(sn=Smith*)))', $query);
+    }
+
     public function test_and_filter_extracts_filters_from_nested_query()
     {
         $b = $this->newBuilder();