From b6f0b01307884b11388eff1ae2d814b7f57715aa Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> Date: Sun, 11 Apr 2021 22:21:56 -0400 Subject: [PATCH] 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! --- .../Controller/AbstractSerializeController.php | 6 ++++++ .../Controller/ListDiscussionsController.php | 3 ++- src/Api/Controller/ListPostsController.php | 3 ++- src/Api/Controller/ListUsersController.php | 3 ++- src/Filter/AbstractFilterer.php | 2 +- src/Query/ApplyQueryParametersTrait.php | 12 +++++++++--- src/Query/QueryCriteria.php | 11 ++++++++++- src/Search/AbstractSearcher.php | 2 +- ...earch.php => ListWithFulltextSearchTest.php} | 17 +++++++++-------- 9 files changed, 42 insertions(+), 17 deletions(-) rename tests/integration/api/discussions/{ListTestWithFulltextSearch.php => ListWithFulltextSearchTest.php} (78%) diff --git a/src/Api/Controller/AbstractSerializeController.php b/src/Api/Controller/AbstractSerializeController.php index cdb20b030..015a42667 100644 --- a/src/Api/Controller/AbstractSerializeController.php +++ b/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. * diff --git a/src/Api/Controller/ListDiscussionsController.php b/src/Api/Controller/ListDiscussionsController.php index f81be1c8e..9bc70b7e2 100644 --- a/src/Api/Controller/ListDiscussionsController.php +++ b/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 { diff --git a/src/Api/Controller/ListPostsController.php b/src/Api/Controller/ListPostsController.php index 73e7c8445..70fec3483 100644 --- a/src/Api/Controller/ListPostsController.php +++ b/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'), diff --git a/src/Api/Controller/ListUsersController.php b/src/Api/Controller/ListUsersController.php index 67b00dc5b..a4c6cd795 100644 --- a/src/Api/Controller/ListUsersController.php +++ b/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 { diff --git a/src/Filter/AbstractFilterer.php b/src/Filter/AbstractFilterer.php index 234c81ff6..7e216f468 100644 --- a/src/Filter/AbstractFilterer.php +++ b/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); diff --git a/src/Query/ApplyQueryParametersTrait.php b/src/Query/ApplyQueryParametersTrait.php index 05c540304..a9a69fc6c 100644 --- a/src/Query/ApplyQueryParametersTrait.php +++ b/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]); diff --git a/src/Query/QueryCriteria.php b/src/Query/QueryCriteria.php index 4f404cb6d..2a286d36d 100644 --- a/src/Query/QueryCriteria.php +++ b/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; } } diff --git a/src/Search/AbstractSearcher.php b/src/Search/AbstractSearcher.php index d0dd0329c..49978975e 100644 --- a/src/Search/AbstractSearcher.php +++ b/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); diff --git a/tests/integration/api/discussions/ListTestWithFulltextSearch.php b/tests/integration/api/discussions/ListWithFulltextSearchTest.php similarity index 78% rename from tests/integration/api/discussions/ListTestWithFulltextSearch.php rename to tests/integration/api/discussions/ListWithFulltextSearchTest.php index 629e01855..3e7ad5b73 100644 --- a/tests/integration/api/discussions/ListTestWithFulltextSearch.php +++ b/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' => '

not in text

'], ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

lightsail in text

'], + ['id' => 3, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

another lightsail for discussion 2!

'], + ['id' => 4, 'discussion_id' => 3, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

just one lightsail for discussion 3.

'], ]); // 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('posts')->whereIn('id', [1, 2])->delete(); + $this->database()->table('discussions')->whereIn('id', [1, 2, 3])->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(['3'], $ids, 'IDs do not match'); + $this->assertEquals(['2', '3'], $ids, 'IDs do not match'); } /** @@ -96,8 +98,7 @@ class ListTest extends TestCase return $row['id']; }, $data['data']); - // Order-independent comparison - $this->assertEquals(['3'], $ids, 'IDs do not match'); + $this->assertEquals(['2', '3'], $ids, 'IDs do not match'); } /**