Filter\Renderer: Don't draw redundant operators

Icinga · Jul 19, 2024 · 6f076a4 · 6f076a4
1 parent e24651e
commit 6f076a4
Show file tree

Hide file tree

Showing 2 changed files with 90 additions and 8 deletions.
diff --git a/src/Filter/Renderer.php b/src/Filter/Renderer.php
@@ -54,7 +54,9 @@ public function render(): string
         $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);
@@ -73,10 +75,6 @@ public function render(): string
      */
     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, bool $wrap = false): void
 
         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))) {

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"
+        );
+    }
 }