Rework Sorting to Combinable QueryProperties.

- Found a failure in sorting which also effected rescoring - Add explicit tests for both - Standardized construction for both Rescoring and Sorting
Jeroen-G · Jul 1, 2023 · 93db134 · 93db134
1 parent ffb76aa
commit 93db134
Show file tree

Hide file tree

Showing 8 changed files with 197 additions and 36 deletions.
diff --git a/src/Domain/Query/Query.php b/src/Domain/Query/Query.php
@@ -9,6 +9,7 @@
 use JeroenG\Explorer\Domain\Query\QueryProperties\QueryProperty;
 use JeroenG\Explorer\Domain\Query\QueryProperties\Rescorers;
 use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring;
+use JeroenG\Explorer\Domain\Query\QueryProperties\Sorting;
 use JeroenG\Explorer\Domain\Syntax\Sort;
 use JeroenG\Explorer\Domain\Syntax\SyntaxInterface;
 use Webmozart\Assert\Assert;
@@ -21,9 +22,6 @@ class Query implements SyntaxInterface
 
     private array $fields = [];
 
-    /** @var Sort[] */
-    private array $sort = [];
-
     /** @var QueryProperty[]  */
     private array $queryProperties = [];
 
@@ -53,10 +51,6 @@ public function build(): array
             $query['size'] = $this->limit;
         }
 
-        if ($this->hasSort()) {
-            $query['sort'] = $this->buildSort();
-        }
-
         if ($this->hasFields()) {
             $query['fields'] = $this->fields;
         }
@@ -90,7 +84,14 @@ public function setFields(array $fields): void
 
     public function setSort(array $sort): void
     {
-        $this->sort = $sort;
+        $this->queryProperties = array_filter(
+            $this->queryProperties,
+            static fn (QueryProperty $queryProperty) => !($queryProperty instanceof Sorting)
+        );
+
+        if (count($sort) > 0) {
+            $this->queryProperties[] = Sorting::for(...$sort);
+        }
     }
 
     public function setQuery(SyntaxInterface $query): void
@@ -128,21 +129,11 @@ private function hasSize(): bool
         return !is_null($this->limit);
     }
 
-    private function hasSort(): bool
-    {
-        return !empty($this->sort);
-    }
-
     private function hasFields(): bool
     {
         return !empty($this->fields);
     }
 
-    private function buildSort(): array
-    {
-        return array_map(static fn ($item) => $item->build(), $this->sort);
-    }
-
     private function buildQueryProperties(): array
     {
         /** @var array<class-string, array<Combinable&QueryProperty>> $allCombinables */

diff --git a/src/Domain/Query/QueryProperties/Rescorers.php b/src/Domain/Query/QueryProperties/Rescorers.php
@@ -4,7 +4,7 @@
 
 namespace JeroenG\Explorer\Domain\Query\QueryProperties;
 
-class Rescorers implements QueryProperty, Combinable
+final class Rescorers implements QueryProperty, Combinable
 {
     private function __construct(
         private array $rescoringQueries = [],
@@ -17,17 +17,16 @@ public static function for(Rescoring ...$rescoring): self
 
     public function build(): array
     {
-
         return [
             'rescore' => array_map(static fn (Rescoring $rescoring) => $rescoring->build(), $this->rescoringQueries),
         ];
     }
 
     public function combine(...$self): self
     {
-        $all = [];
+        $all = $this->rescoringQueries;
         foreach($self as $rescorer) {
-            array_push($all, ...$this->rescoringQueries, ...$rescorer->rescoringQueries);
+            array_push($all, ...$rescorer->rescoringQueries);
         }
 
         return new self($all);

diff --git a/src/Domain/Query/QueryProperties/Sorting.php b/src/Domain/Query/QueryProperties/Sorting.php
@@ -0,0 +1,42 @@
+<?php
+
+declare(strict_types=1);
+
+namespace JeroenG\Explorer\Domain\Query\QueryProperties;
+
+use JeroenG\Explorer\Domain\Syntax\Sort;
+
+final class Sorting implements QueryProperty, Combinable
+{
+    /** @param Sort[] $sorts */
+    private function __construct(private array $sorts = [])
+    {
+    }
+
+    public static function for(Sort ...$sort): self
+    {
+        return new self($sort);
+    }
+
+    public function combine(...$self): QueryProperty
+    {
+        $all = $this->sorts;
+
+        /** @var Sorting[] $self */
+        foreach($self as $sorting) {
+            array_push($all, ...$sorting->sorts);
+        }
+
+        return new self($all);
+    }
+
+    public function build(): array
+    {
+        return [
+            'sort' => array_map(
+                static fn (Sort $sort) => $sort->build(),
+                $this->sorts,
+            ),
+        ];
+    }
+}
diff --git a/src/Domain/Syntax/Sort.php b/src/Domain/Syntax/Sort.php
@@ -6,7 +6,7 @@
 
 use Webmozart\Assert\Assert;
 
-class Sort implements SyntaxInterface
+class Sort
 {
     public const ASCENDING = 'asc';
 

diff --git a/tests/Unit/Domain/Query/QueryProperties/RescorersTest.php b/tests/Unit/Domain/Query/QueryProperties/RescorersTest.php
@@ -0,0 +1,72 @@
+<?php
+
+declare(strict_types=1);
+
+namespace JeroenG\Explorer\Tests\Unit\Domain\Query\QueryProperties;
+
+use JeroenG\Explorer\Domain\Query\QueryProperties\Rescorers;
+use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring;
+use JeroenG\Explorer\Domain\Syntax\MatchAll;
+use JeroenG\Explorer\Domain\Syntax\Term;
+use PHPUnit\Framework\TestCase;
+
+final class RescorersTest extends TestCase
+{
+    public function test_it_builds(): void
+    {
+        $sort = Rescorers::for(
+            Rescoring::create(new Term(':fld:', ':val:')),
+        );
+
+        self::assertSame(['rescore' => [self::rescoreQueryPart(':fld:', ':val:')]], $sort->build());
+    }
+
+    public function test_it_combines(): void
+    {
+        $a = Rescorers::for(
+            Rescoring::create(new Term(':fld1:', ':val1:')),
+            Rescoring::create(new Term(':fld2:', ':val2:')),
+        );
+        $b = Rescorers::for(
+            Rescoring::create(new Term(':fld3:', ':val3:')),
+            Rescoring::create(new Term(':fld4:', ':val4:')),
+        );
+        $c = Rescorers::for(
+            Rescoring::create(new Term(':fld5:', ':val5:')),
+        );
+        $d = Rescorers::for();
+
+        $result = $a->combine($b, $c, $d);
+
+        self::assertNotSame($a->build(), $result->build());
+        self::assertSame([
+            'rescore' => [
+                self::rescoreQueryPart(':fld1:', ':val1:'),
+                self::rescoreQueryPart(':fld2:', ':val2:'),
+                self::rescoreQueryPart(':fld3:', ':val3:'),
+                self::rescoreQueryPart(':fld4:', ':val4:'),
+                self::rescoreQueryPart(':fld5:', ':val5:'),
+            ],
+        ], $result->build());
+    }
+
+    private static function rescoreQueryPart(string $fld, string $val): array
+    {
+        return [
+            'window_size' => 10,
+            'query' => [
+                'score_mode' => 'total',
+                'rescore_query' => [
+                    'term' => [
+                        $fld => [
+                            'value' => $val,
+                            'boost' => 1.0,
+                        ]
+                    ]
+                ],
+                'query_weight' => 1.0,
+                'rescore_query_weight' => 1.0,
+            ]
+        ];
+    }
+}
diff --git a/tests/Unit/Domain/Query/QueryProperties/SortingTest.php b/tests/Unit/Domain/Query/QueryProperties/SortingTest.php
@@ -0,0 +1,50 @@
+<?php
+
+declare(strict_types=1);
+
+namespace JeroenG\Explorer\Tests\Unit\Domain\Query\QueryProperties;
+
+use JeroenG\Explorer\Domain\Query\QueryProperties\Sorting;
+use JeroenG\Explorer\Domain\Syntax\Sort;
+use PHPUnit\Framework\TestCase;
+
+final class SortingTest extends TestCase
+{
+    public function test_it_builds_sorting(): void
+    {
+        $sort = Sorting::for(
+            new Sort(':fld:', Sort::DESCENDING),
+        );
+
+        self::assertSame([ 'sort' => [ [ ':fld:' => 'desc' ]]],$sort->build());
+    }
+
+    public function test_it_combines(): void
+    {
+        $a = Sorting::for(
+            new Sort(':fld1:', Sort::DESCENDING),
+            new Sort(':fld2:', Sort::DESCENDING),
+        );
+        $b = Sorting::for(
+            new Sort(':fld3:', Sort::DESCENDING),
+            new Sort(':fld4:', Sort::DESCENDING),
+        );
+        $c = Sorting::for(
+            new Sort(':fld5:', Sort::DESCENDING),
+        );
+        $d = Sorting::for();
+
+        $result = $a->combine($b, $c, $d);
+
+        self::assertNotSame($a->build(), $result->build());
+        self::assertSame([
+            'sort' => [
+                [ ':fld1:' => 'desc' ],
+                [ ':fld2:' => 'desc' ],
+                [ ':fld3:' => 'desc' ],
+                [ ':fld4:' => 'desc' ],
+                [ ':fld5:' => 'desc' ],
+            ],
+        ], $result->build());
+    }
+}
diff --git a/tests/Unit/FakeResponseTest.php b/tests/Unit/FakeResponseTest.php
@@ -4,21 +4,8 @@
 
 namespace JeroenG\Explorer\Tests\Unit;
 
-use Elasticsearch\Client;
 use InvalidArgumentException;
-use JeroenG\Explorer\Application\AggregationResult;
-use JeroenG\Explorer\Application\SearchCommand;
-use JeroenG\Explorer\Domain\Aggregations\TermsAggregation;
-use JeroenG\Explorer\Domain\Query\Query;
-use JeroenG\Explorer\Domain\Syntax\Compound\BoolQuery;
-use JeroenG\Explorer\Domain\Syntax\Matching;
-use JeroenG\Explorer\Domain\Syntax\Sort;
-use JeroenG\Explorer\Domain\Syntax\Term;
-use JeroenG\Explorer\Infrastructure\Elastic\ElasticClientFactory;
 use JeroenG\Explorer\Infrastructure\Elastic\FakeResponse;
-use JeroenG\Explorer\Infrastructure\Elastic\Finder;
-use JeroenG\Explorer\Infrastructure\Scout\ScoutSearchCommandBuilder;
-use Mockery;
 use Mockery\Adapter\Phpunit\MockeryTestCase;
 
 class FakeResponseTest extends MockeryTestCase

diff --git a/tests/Unit/Query/QueryTest.php b/tests/Unit/Query/QueryTest.php
@@ -8,10 +8,12 @@
 use JeroenG\Explorer\Domain\Query\Query;
 use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring;
 use JeroenG\Explorer\Domain\Query\QueryProperties\SourceFilter;
+use JeroenG\Explorer\Domain\Query\QueryProperties\TrackTotalHits;
 use JeroenG\Explorer\Domain\Syntax\MatchAll;
 use JeroenG\Explorer\Domain\Syntax\Sort;
 use JeroenG\Explorer\Domain\Syntax\Term;
 use PHPUnit\Framework\TestCase;
+use TypeError;
 
 final class QueryTest extends TestCase
 {
@@ -43,6 +45,24 @@ public function test_it_builds_query_with_sort(): void
         self::assertEquals([$sort->build()], $result['sort'] ?? null);
     }
 
+    public function test_it_throws_on_invalid_sort_argument(): void
+    {
+        $this->expectException(TypeError::class);
+        $this->query->setSort([new Term(':fld:', ':val:')]);
+    }
+
+    public function test_it_reset_sort(): void
+    {
+        $this->query->addQueryProperties(TrackTotalHits::all());
+        $this->query->setSort([new Sort(':fld:')]);
+        $this->query->setSort([]);
+
+        $result = $this->query->build();
+
+        self::assertArrayNotHasKey('sort', $result);
+        self::assertArrayHasKey('track_total_hits', $result);
+    }
+
     public function test_it_builds_query_with_pagination(): void
     {
         $this->query->setLimit(10);