Fix redundant filter operators (#229)

fixes #228
Icinga · Jul 19, 2024 · 47560f9 · 47560f9
2 parents 6d5aaa5 + 6f076a4
commit 47560f9
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 13 deletions.
diff --git a/src/Filter/Renderer.php b/src/Filter/Renderer.php
@@ -32,9 +32,9 @@ public function __construct(Filter\Rule $filter)
      *
      * @return $this
      */
-    public function setStrict($strict = true)
+    public function setStrict(bool $strict = true): self
     {
-        $this->strict = (bool) $strict;
+        $this->strict = $strict;
 
         return $this;
     }
@@ -44,7 +44,7 @@ public function setStrict($strict = true)
      *
      * @return string
      */
-    public function render()
+    public function render(): string
     {
         if ($this->string !== null) {
             return $this->string;
@@ -54,7 +54,9 @@ public function render()
         $filter = $this->filter;
 
         if ($filter instanceof Filter\Chain) {
-            $this->renderChain($filter, $this->strict);
+            if ($this->strict || ! $filter->isEmpty()) {
+                $this->renderChain($filter, $this->strict);
+            }
         } else {
             /** @var Filter\Condition $filter */
             $this->renderCondition($filter);
@@ -71,12 +73,8 @@ public function render()
      *
      * @return void
      */
-    protected function renderChain(Filter\Chain $chain, $wrap = false)
+    protected function renderChain(Filter\Chain $chain, bool $wrap = false): void
     {
-        if (! $this->strict && $chain->isEmpty()) {
-            return;
-        }
-
         $chainOperator = null;
         switch (true) {
             case $chain instanceof Filter\All:
@@ -107,13 +105,15 @@ protected function renderChain(Filter\Chain $chain, $wrap = false)
 
         foreach ($chain as $rule) {
             if ($rule instanceof Filter\Chain) {
-                $this->renderChain($rule, $this->strict || $rule->count() > 1);
+                if ($this->strict || ! $rule->isEmpty()) {
+                    $this->renderChain($rule, $this->strict || $rule->count() > 1);
+                    $this->string .= $chainOperator;
+                }
             } else {
                 /** @var Filter\Condition $rule */
                 $this->renderCondition($rule);
+                $this->string .= $chainOperator;
             }
-
-            $this->string .= $chainOperator;
         }
 
         if (! $chain->isEmpty() && (! $this->strict || ! ($chain instanceof Filter\Any && $chain->count() === 1))) {
@@ -137,7 +137,7 @@ protected function renderChain(Filter\Chain $chain, $wrap = false)
      *
      * @return void
      */
-    protected function renderCondition(Filter\Condition $condition)
+    protected function renderCondition(Filter\Condition $condition): void
     {
         $value = $condition->getValue();
         if (is_bool($value) && ! $value) {

diff --git a/tests/FilterTest.php b/tests/FilterTest.php
@@ -687,4 +687,86 @@ public function testParserRespectsRedundantCharsInStrictMode()
             "Filter\Parser doesn't respect group operator for empty nested OR chains with non-empty siblings"
         );
     }
+
+    /* Non-Strict mode tests */
+
+    public function testRendererDoesNotDrawRedundantCharsInNonStrictMode()
+    {
+        $this->assertEquals(
+            '',
+            (new Renderer(Filter::all()))->render(),
+            "Filter\Renderer draws redundant parentheses for an empty root chain"
+        );
+        $this->assertEquals(
+            'foo=bar',
+            (new Renderer(Filter::equal('foo', 'bar')))->render(),
+            "Filter\Renderer draws redundant parentheses for a root chain with a single condition"
+        );
+        $this->assertEquals(
+            'foo=bar&bar=foo',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::equal('bar', 'foo')
+            )))->render(),
+            "Filter\Renderer draws redundant parentheses for a root chain with multiple conditions"
+        );
+        $this->assertEquals(
+            'foo=bar&bar=foo',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::all(
+                    Filter::equal('bar', 'foo')
+                )
+            )))->render(),
+            "Filter\Renderer draws redundant parentheses for nested chains with a single condition"
+        );
+        $this->assertEquals(
+            'foo=bar&bar=foo',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::any(
+                    Filter::equal('bar', 'foo')
+                )
+            )))->render(),
+            "Filter\Renderer draws redundant group operator for nested OR chains with a single condition"
+        );
+        $this->assertEquals(
+            'foo=bar',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::all()
+            )))->render(),
+            "Filter\Renderer draws redundant parentheses for empty nested chains"
+        );
+        $this->assertEquals(
+            'foo=bar&(bar=foo)',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::all(
+                    Filter::all(),
+                    Filter::equal('bar', 'foo')
+                )
+            )))->render(),
+            "Filter\Renderer draws redundant parentheses for empty nested chains with non-empty siblings"
+        );
+        $this->assertEquals(
+            'foo=bar',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::any()
+            )))->render(),
+            "Filter\Renderer draws redundant group operator for empty nested OR chains"
+        );
+        $this->assertEquals(
+            'foo=bar&(bar=foo)',
+            (new Renderer(Filter::all(
+                Filter::equal('foo', 'bar'),
+                Filter::any(
+                    Filter::any(),
+                    Filter::equal('bar', 'foo')
+                )
+            )))->render(),
+            "Filter\Renderer draws redundant group operator for empty nested OR chains with non-empty siblings"
+        );
+    }
 }