Fix relevance sort (#2773)

- Adds a field to QueryCriteria that determines whether the sort provided is the controller's default sort
- Set this field to true iff sort not in query params. Default it to false
- Override $sort if a new default sort has been set on search state, and the param is true.
- Add tests!

src/Api/Controller/AbstractSerializeController.php

@@ -12,6 +12,7 @@ namespace Flarum\Api\Controller;
 use Flarum\Api\JsonApiResponse;
 use Illuminate\Contracts\Container\Container;
 use Illuminate\Database\Eloquent\Collection;
+use Illuminate\Support\Arr;
 use Illuminate\Support\Str;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
@@ -255,6 +256,11 @@ abstract class AbstractSerializeController implements RequestHandlerInterface
         return new Parameters($request->getQueryParams());
     }
 
+    protected function sortIsDefault(ServerRequestInterface $request): bool
+    {
+        return ! Arr::get($request->getQueryParams(), 'sort');
+    }
+
     /**
      * Set the serializer that will serialize data for the endpoint.
      *

src/Api/Controller/ListDiscussionsController.php

@@ -89,12 +89,13 @@ class ListDiscussionsController extends AbstractListController
         $actor = RequestUtil::getActor($request);
         $filters = $this->extractFilter($request);
         $sort = $this->extractSort($request);
+        $sortIsDefault = $this->sortIsDefault($request);
 
         $limit = $this->extractLimit($request);
         $offset = $this->extractOffset($request);
         $include = array_merge($this->extractInclude($request), ['state']);
 
-        $criteria = new QueryCriteria($actor, $filters, $sort);
+        $criteria = new QueryCriteria($actor, $filters, $sort, $sortIsDefault);
         if (array_key_exists('q', $filters)) {
             $results = $this->searcher->search($criteria, $limit, $offset);
         } else {

src/Api/Controller/ListPostsController.php

@@ -79,12 +79,13 @@ class ListPostsController extends AbstractListController
 
         $filters = $this->extractFilter($request);
         $sort = $this->extractSort($request);
+        $sortIsDefault = $this->sortIsDefault($request);
 
         $limit = $this->extractLimit($request);
         $offset = $this->extractOffset($request);
         $include = $this->extractInclude($request);
 
-        $results = $this->filterer->filter(new QueryCriteria($actor, $filters, $sort), $limit, $offset);
+        $results = $this->filterer->filter(new QueryCriteria($actor, $filters, $sort, $sortIsDefault), $limit, $offset);
 
         $document->addPaginationLinks(
             $this->url->to('api')->route('posts.index'),

src/Api/Controller/ListUsersController.php

@@ -86,12 +86,13 @@ class ListUsersController extends AbstractListController
 
         $filters = $this->extractFilter($request);
         $sort = $this->extractSort($request);
+        $sortIsDefault = $this->sortIsDefault($request);
 
         $limit = $this->extractLimit($request);
         $offset = $this->extractOffset($request);
         $include = $this->extractInclude($request);
 
-        $criteria = new QueryCriteria($actor, $filters, $sort);
+        $criteria = new QueryCriteria($actor, $filters, $sort, $sortIsDefault);
         if (array_key_exists('q', $filters)) {
             $results = $this->searcher->search($criteria, $limit, $offset);
         } else {

src/Filter/AbstractFilterer.php

@@ -65,7 +65,7 @@ abstract class AbstractFilterer
             }
         }
 
-        $this->applySort($filterState, $criteria->sort);
+        $this->applySort($filterState, $criteria->sort, $criteria->sortIsDefault);
         $this->applyOffset($filterState, $offset);
         $this->applyLimit($filterState, $limit + 1);
 

src/Query/ApplyQueryParametersTrait.php

@@ -11,6 +11,9 @@ namespace Flarum\Query;
 
 use Illuminate\Support\Str;
 
+/**
+ * @internal
+ */
 trait ApplyQueryParametersTrait
 {
     /**
@@ -18,15 +21,18 @@ trait ApplyQueryParametersTrait
      *
      * @param AbstractQueryState $query
      * @param array $sort
+     * @param bool $sortIsDefault
      */
-    protected function applySort(AbstractQueryState $query, array $sort = null)
+    protected function applySort(AbstractQueryState $query, array $sort = null, bool $sortIsDefault = false)
     {
-        $sort = $sort ?: $query->getDefaultSort();
+        if ($sortIsDefault && ! empty($query->getDefaultSort())) {
+            $sort = $query->getDefaultSort();
+        }
 
         if (is_callable($sort)) {
             $sort($query->getQuery());
         } else {
-            foreach ($sort as $field => $order) {
+            foreach ((array) $sort as $field => $order) {
                 if (is_array($order)) {
                     foreach ($order as $value) {
                         $query->getQuery()->orderByRaw(Str::snake($field).' != ?', [$value]);

src/Query/QueryCriteria.php

@@ -41,6 +41,14 @@ class QueryCriteria
      */
     public $sort;
 
+    /**
+     * Is the sort for this request the default sort from the controller?
+     * If false, the current request specifies a sort.
+     *
+     * @var bool
+     */
+    public $sortIsDefault;
+
     /**
      * @param User $actor The user performing the query.
      * @param array $query The query params.
@@ -48,10 +56,11 @@ class QueryCriteria
      *     key, and the order is the value. The order may be 'asc', 'desc', or
      *     an array of IDs to order by.
      */
-    public function __construct(User $actor, $query, array $sort = null)
+    public function __construct(User $actor, $query, array $sort = null, bool $sortIsDefault = false)
     {
         $this->actor = $actor;
         $this->query = $query;
         $this->sort = $sort;
+        $this->sortIsDefault = $sortIsDefault;
     }
 }

src/Search/AbstractSearcher.php

@@ -54,7 +54,7 @@ abstract class AbstractSearcher
         $search = new SearchState($query->getQuery(), $actor);
 
         $this->gambits->apply($search, $criteria->query['q']);
-        $this->applySort($search, $criteria->sort);
+        $this->applySort($search, $criteria->sort, $criteria->sortIsDefault);
         $this->applyOffset($search, $offset);
         $this->applyLimit($search, $limit + 1);
 

tests/integration/api/discussions/ListWithFulltextSearchTest.php

@@ -13,7 +13,7 @@ use Carbon\Carbon;
 use Flarum\Testing\integration\RetrievesAuthorizedUsers;
 use Flarum\Testing\integration\TestCase;
 
-class ListTest extends TestCase
+class ListWithFulltextSearchTest extends TestCase
 {
     use RetrievesAuthorizedUsers;
 
@@ -31,12 +31,15 @@ class ListTest extends TestCase
         // We clean it up explcitly at the end.
         $this->database()->table('discussions')->insert([
             ['id' => 1, 'title' => 'lightsail in title', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1],
-            ['id' => 2, 'title' => 'not in title', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1],
+            ['id' => 2, 'title' => 'not in title and older', 'created_at' => Carbon::createFromDate(2020, 01, 01)->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1],
+            ['id' => 3, 'title' => 'not in title either', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'comment_count' => 1],
         ]);
 
         $this->database()->table('posts')->insert([
             ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>not in text</p></t>'],
             ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>lightsail in text</p></t>'],
+            ['id' => 3, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>another lightsail for discussion 2!</p></t>'],
+            ['id' => 4, 'discussion_id' => 3, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>just one lightsail for discussion 3.</p></t>'],
         ]);
 
         // We need to call these again, since we rolled back the transaction started by `::app()`.
@@ -52,8 +55,8 @@ class ListTest extends TestCase
     {
         parent::tearDown();
 
-        $this->database()->table('discussions')->whereIn('id', [1, 2])->delete();
+        $this->database()->table('discussions')->whereIn('id', [1, 2, 3])->delete();
-        $this->database()->table('posts')->whereIn('id', [1, 2])->delete();
+        $this->database()->table('posts')->whereIn('id', [1, 2, 3, 4])->delete();
     }
 
     /**
@@ -74,8 +77,7 @@ class ListTest extends TestCase
             return $row['id'];
         }, $data['data']);
 
-        // Order-independent comparison
+        $this->assertEquals(['2', '3'], $ids, 'IDs do not match');
-        $this->assertEquals(['3'], $ids, 'IDs do not match');
     }
 
     /**
@@ -96,8 +98,7 @@ class ListTest extends TestCase
             return $row['id'];
         }, $data['data']);
 
-        // Order-independent comparison
+        $this->assertEquals(['2', '3'], $ids, 'IDs do not match');
-        $this->assertEquals(['3'], $ids, 'IDs do not match');
     }
 
     /**