From 458a5cc6be82ef8bf5296f719a95d4b1801d0c6f Mon Sep 17 00:00:00 2001
From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Date: Sun, 28 Feb 2021 14:06:07 -0500
Subject: [PATCH] Use filterer for ListPostsController (#2479)
---
src/Api/Controller/ListPostsController.php | 98 +++------
src/Event/ConfigurePostsQuery.php | 3 +
src/Filter/AbstractFilterer.php | 8 +
src/Filter/FilterServiceProvider.php | 31 +--
src/Post/Filter/AuthorFilter.php | 45 ++++
src/Post/Filter/DiscussionFilter.php | 28 +++
src/Post/Filter/IdFilter.php | 29 +++
src/Post/Filter/NumberFilter.php | 28 +++
src/Post/Filter/PostFilterer.php | 40 ++++
src/Post/Filter/TypeFilter.php | 28 +++
src/Post/PostRepository.php | 26 ---
tests/integration/api/posts/ListTest.php | 226 +++++++++++++++++++++
12 files changed, 483 insertions(+), 107 deletions(-)
create mode 100644 src/Post/Filter/AuthorFilter.php
create mode 100644 src/Post/Filter/DiscussionFilter.php
create mode 100644 src/Post/Filter/IdFilter.php
create mode 100644 src/Post/Filter/NumberFilter.php
create mode 100644 src/Post/Filter/PostFilterer.php
create mode 100644 src/Post/Filter/TypeFilter.php
create mode 100644 tests/integration/api/posts/ListTest.php
diff --git a/src/Api/Controller/ListPostsController.php b/src/Api/Controller/ListPostsController.php
index 977dfdcf3..e1a7e1022 100644
--- a/src/Api/Controller/ListPostsController.php
+++ b/src/Api/Controller/ListPostsController.php
@@ -10,11 +10,10 @@
namespace Flarum\Api\Controller;
use Flarum\Api\Serializer\PostSerializer;
-use Flarum\Event\ConfigurePostsQuery;
-use Flarum\Post\PostRepository;
-use Illuminate\Database\Eloquent\Builder;
+use Flarum\Http\UrlGenerator;
+use Flarum\Post\Filter\PostFilterer;
+use Flarum\Search\SearchCriteria;
use Illuminate\Support\Arr;
-use Illuminate\Support\Str;
use Psr\Http\Message\ServerRequestInterface;
use Tobscure\JsonApi\Document;
use Tobscure\JsonApi\Exception\InvalidParameterException;
@@ -43,16 +42,23 @@ class ListPostsController extends AbstractListController
public $sortFields = ['createdAt'];
/**
- * @var \Flarum\Post\PostRepository
+ * @var PostFilterer
*/
- protected $posts;
+ protected $filterer;
/**
- * @param \Flarum\Post\PostRepository $posts
+ * @var UrlGenerator
*/
- public function __construct(PostRepository $posts)
+ protected $url;
+
+ /**
+ * @param PostFilterer $filterer
+ * @param UrlGenerator $url
+ */
+ public function __construct(PostFilterer $filterer, UrlGenerator $url)
{
- $this->posts = $posts;
+ $this->filterer = $filterer;
+ $this->url = $url;
}
/**
@@ -61,18 +67,25 @@ class ListPostsController extends AbstractListController
protected function data(ServerRequestInterface $request, Document $document)
{
$actor = $request->getAttribute('actor');
- $filter = $this->extractFilter($request);
+
+ $filters = $this->extractFilter($request);
+ $sort = $this->extractSort($request);
+
+ $limit = $this->extractLimit($request);
+ $offset = $this->extractOffset($request);
$include = $this->extractInclude($request);
- if ($postIds = Arr::get($filter, 'id')) {
- $postIds = explode(',', $postIds);
- } else {
- $postIds = $this->getPostIds($request);
- }
+ $results = $this->filterer->filter(new SearchCriteria($actor, $filters, $sort), $limit, $offset);
- $posts = $this->posts->findByIds($postIds, $actor);
+ $document->addPaginationLinks(
+ $this->url->to('api')->route('posts.index'),
+ $request->getQueryParams(),
+ $offset,
+ $limit,
+ $results->areMoreResults() ? null : 0
+ );
- return $posts->load($include);
+ return $results->getResults()->load($include);
}
/**
@@ -100,55 +113,4 @@ class ListPostsController extends AbstractListController
return parent::extractOffset($request);
}
-
- /**
- * @param ServerRequestInterface $request
- * @return array
- * @throws InvalidParameterException
- */
- private function getPostIds(ServerRequestInterface $request)
- {
- $actor = $request->getAttribute('actor');
- $filter = $this->extractFilter($request);
- $sort = $this->extractSort($request);
- $limit = $this->extractLimit($request);
- $offset = $this->extractOffset($request);
-
- $query = $this->posts->query()->whereVisibleTo($actor);
-
- $this->applyFilters($query, $filter);
-
- $query->skip($offset)->take($limit);
-
- foreach ((array) $sort as $field => $order) {
- $query->orderBy(Str::snake($field), $order);
- }
-
- return $query->pluck('id')->all();
- }
-
- /**
- * @param Builder $query
- * @param array $filter
- */
- private function applyFilters(Builder $query, array $filter)
- {
- if ($discussionId = Arr::get($filter, 'discussion')) {
- $query->where('discussion_id', $discussionId);
- }
-
- if ($number = Arr::get($filter, 'number')) {
- $query->where('number', $number);
- }
-
- if ($userId = Arr::get($filter, 'user')) {
- $query->where('user_id', $userId);
- }
-
- if ($type = Arr::get($filter, 'type')) {
- $query->where('type', $type);
- }
-
- event(new ConfigurePostsQuery($query, $filter));
- }
}
diff --git a/src/Event/ConfigurePostsQuery.php b/src/Event/ConfigurePostsQuery.php
index cfd98a0f5..b2163dec5 100644
--- a/src/Event/ConfigurePostsQuery.php
+++ b/src/Event/ConfigurePostsQuery.php
@@ -11,6 +11,9 @@ namespace Flarum\Event;
use Illuminate\Database\Eloquent\Builder;
+/**
+ * @deprecated beta 16, remove beta 17.
+ */
class ConfigurePostsQuery
{
/**
diff --git a/src/Filter/AbstractFilterer.php b/src/Filter/AbstractFilterer.php
index c7ddc0542..896e6dfef 100644
--- a/src/Filter/AbstractFilterer.php
+++ b/src/Filter/AbstractFilterer.php
@@ -9,6 +9,8 @@
namespace Flarum\Filter;
+use Flarum\Event\ConfigurePostsQuery;
+use Flarum\Post\Filter\PostFilterer;
use Flarum\Search\ApplySearchParametersTrait;
use Flarum\Search\SearchCriteria;
use Flarum\Search\SearchResults;
@@ -68,6 +70,12 @@ abstract class AbstractFilterer
$this->applyOffset($filterState, $offset);
$this->applyLimit($filterState, $limit + 1);
+ // DEPRECATED BC LAYER, REMOVE BETA 17
+ if (static::class === PostFilterer::class) {
+ event(new ConfigurePostsQuery($query, $criteria->query));
+ }
+ // END DEPRECATED BC LAYER
+
foreach ($this->filterMutators as $mutator) {
$mutator($query, $actor, $criteria->query, $criteria->sort);
}
diff --git a/src/Filter/FilterServiceProvider.php b/src/Filter/FilterServiceProvider.php
index 477ea7a30..530a598f1 100644
--- a/src/Filter/FilterServiceProvider.php
+++ b/src/Filter/FilterServiceProvider.php
@@ -9,15 +9,13 @@
namespace Flarum\Filter;
-use Flarum\Discussion\Filter\AuthorFilterGambit;
-use Flarum\Discussion\Filter\CreatedFilterGambit;
+use Flarum\Discussion\Filter as DiscussionFilter;
use Flarum\Discussion\Filter\DiscussionFilterer;
-use Flarum\Discussion\Filter\HiddenFilterGambit;
-use Flarum\Discussion\Filter\UnreadFilterGambit;
use Flarum\Foundation\AbstractServiceProvider;
use Flarum\Foundation\ContainerUtil;
-use Flarum\User\Filter\EmailFilterGambit;
-use Flarum\User\Filter\GroupFilterGambit;
+use Flarum\Post\Filter as PostFilter;
+use Flarum\Post\Filter\PostFilterer;
+use Flarum\User\Filter as UserFilter;
use Flarum\User\Filter\UserFilterer;
use Illuminate\Support\Arr;
@@ -33,15 +31,22 @@ class FilterServiceProvider extends AbstractServiceProvider
$this->app->singleton('flarum.filter.filters', function () {
return [
DiscussionFilterer::class => [
- AuthorFilterGambit::class,
- CreatedFilterGambit::class,
- HiddenFilterGambit::class,
- UnreadFilterGambit::class,
+ DiscussionFilter\AuthorFilterGambit::class,
+ DiscussionFilter\CreatedFilterGambit::class,
+ DiscussionFilter\HiddenFilterGambit::class,
+ DiscussionFilter\UnreadFilterGambit::class,
],
UserFilterer::class => [
- EmailFilterGambit::class,
- GroupFilterGambit::class,
- ]
+ UserFilter\EmailFilterGambit::class,
+ UserFilter\GroupFilterGambit::class,
+ ],
+ PostFilterer::class => [
+ PostFilter\AuthorFilter::class,
+ PostFilter\DiscussionFilter::class,
+ PostFilter\IdFilter::class,
+ PostFilter\NumberFilter::class,
+ PostFilter\TypeFilter::class,
+ ],
];
});
diff --git a/src/Post/Filter/AuthorFilter.php b/src/Post/Filter/AuthorFilter.php
new file mode 100644
index 000000000..493db3da9
--- /dev/null
+++ b/src/Post/Filter/AuthorFilter.php
@@ -0,0 +1,45 @@
+users = $users;
+ }
+
+ public function getFilterKey(): string
+ {
+ return 'author';
+ }
+
+ public function filter(FilterState $filterState, string $filterValue, bool $negate)
+ {
+ $usernames = trim($filterValue, '"');
+ $usernames = explode(',', $usernames);
+
+ $ids = $this->users->query()->whereIn('username', $usernames)->pluck('id');
+
+ $filterState->getQuery()->whereIn('posts.user_id', $ids, 'and', $negate);
+ }
+}
diff --git a/src/Post/Filter/DiscussionFilter.php b/src/Post/Filter/DiscussionFilter.php
new file mode 100644
index 000000000..9ebb809a1
--- /dev/null
+++ b/src/Post/Filter/DiscussionFilter.php
@@ -0,0 +1,28 @@
+getQuery()->where('posts.discussion_id', $negate ? '!=' : '=', $discussionId);
+ }
+}
diff --git a/src/Post/Filter/IdFilter.php b/src/Post/Filter/IdFilter.php
new file mode 100644
index 000000000..9835e5113
--- /dev/null
+++ b/src/Post/Filter/IdFilter.php
@@ -0,0 +1,29 @@
+getQuery()->whereIn('posts.id', $ids, 'and', $negate);
+ }
+}
diff --git a/src/Post/Filter/NumberFilter.php b/src/Post/Filter/NumberFilter.php
new file mode 100644
index 000000000..8b82fe4e3
--- /dev/null
+++ b/src/Post/Filter/NumberFilter.php
@@ -0,0 +1,28 @@
+getQuery()->where('posts.number', $negate ? '!=' : '=', $number);
+ }
+}
diff --git a/src/Post/Filter/PostFilterer.php b/src/Post/Filter/PostFilterer.php
new file mode 100644
index 000000000..7a0318f96
--- /dev/null
+++ b/src/Post/Filter/PostFilterer.php
@@ -0,0 +1,40 @@
+posts = $posts;
+ }
+
+ protected function getQuery(User $actor): Builder
+ {
+ return $this->posts->query()->whereVisibleTo($actor);
+ }
+}
diff --git a/src/Post/Filter/TypeFilter.php b/src/Post/Filter/TypeFilter.php
new file mode 100644
index 000000000..64a02d79c
--- /dev/null
+++ b/src/Post/Filter/TypeFilter.php
@@ -0,0 +1,28 @@
+getQuery()->where('posts.type', $negate ? '!=' : '=', $type);
+ }
+}
diff --git a/src/Post/PostRepository.php b/src/Post/PostRepository.php
index 20ddd6aeb..2b8f7829b 100644
--- a/src/Post/PostRepository.php
+++ b/src/Post/PostRepository.php
@@ -80,32 +80,6 @@ class PostRepository
return $query->get();
}
- /**
- * Find posts by their IDs, optionally making sure they are visible to a
- * certain user.
- *
- * @param array $ids
- * @param \Flarum\User\User|null $actor
- * @return \Illuminate\Database\Eloquent\Collection
- */
- public function findByIds(array $ids, User $actor = null)
- {
- $posts = $this->queryIds($ids, $actor)->get();
-
- $posts = $posts->sort(function ($a, $b) use ($ids) {
- $aPos = array_search($a->id, $ids);
- $bPos = array_search($b->id, $ids);
-
- if ($aPos === $bPos) {
- return 0;
- }
-
- return $aPos < $bPos ? -1 : 1;
- });
-
- return $posts;
- }
-
/**
* Filter a list of post IDs to only include posts that are visible to a
* certain user.
diff --git a/tests/integration/api/posts/ListTest.php b/tests/integration/api/posts/ListTest.php
new file mode 100644
index 000000000..4ed599b76
--- /dev/null
+++ b/tests/integration/api/posts/ListTest.php
@@ -0,0 +1,226 @@
+prepareDatabase([
+ 'discussions' => [
+ ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 2],
+ ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 2],
+ ],
+ 'posts' => [
+ ['id' => 1, 'number' => 1, 'discussion_id' => 1, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => 'something
'],
+ ['id' => 2, 'number' => 1, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => 'something
'],
+ ['id' => 3, 'number' => 2, 'discussion_id' => 1, 'created_at' => Carbon::now(), 'user_id' => 2, 'type' => 'comment', 'content' => 'something
'],
+ ['id' => 4, 'number' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 2, 'type' => 'comment', 'content' => 'something
'],
+ ['id' => 5, 'number' => 3, 'discussion_id' => 2, 'created_at' => Carbon::now(), 'user_id' => 2, 'type' => 'discussionRenamed', 'content' => 'something
'],
+ ],
+ 'users' => [
+ $this->normalUser(),
+ ],
+ ]);
+ }
+
+ private function forbidGuestsFromSeeingForum()
+ {
+ $this->database()->table('group_permission')->where('permission', 'viewDiscussions')->where('group_id', 2)->delete();
+ }
+
+ /**
+ * @test
+ */
+ public function guests_cant_see_anything_if_not_allowed()
+ {
+ $this->forbidGuestsFromSeeingForum();
+
+ $response = $this->send(
+ $this->request('GET', '/api/posts')
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals([], $data['data']);
+ }
+
+ /**
+ * @test
+ */
+ public function authorized_users_can_see_posts()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(5, count($data['data']));
+ }
+
+ /**
+ * @test
+ */
+ public function author_filter_works()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['author' => 'admin'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['1', '2'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function author_filter_works_with_multiple_values()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['author' => 'admin,normal'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['1', '2', '3', '4', '5'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function discussion_filter_works()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['discussion' => '1'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['1', '3'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function type_filter_works()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['type' => 'discussionRenamed'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['5'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function number_filter_works()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['number' => '2'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['3', '4'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function id_filter_works()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['id' => '4'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['4'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @test
+ */
+ public function id_filter_works_with_multiple_ids()
+ {
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ ->withQueryParams([
+ 'filter' => ['id' => '1,3,5'],
+ ])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['1', '3', '5'], Arr::pluck($data['data'], 'id'));
+ }
+
+ /**
+ * @deprecated beta 16, remove beta 17
+ * @test
+ */
+ public function deprecated_configure_posts_query_extension_still_works()
+ {
+ $this->app()->getContainer()->make('events')->listen(ConfigurePostsQuery::class, function (ConfigurePostsQuery $event) {
+ $event->query->where('id', '1');
+ });
+
+ $response = $this->send(
+ $this->request('GET', '/api/posts', ['authenticatedAs' => 1])
+ );
+
+ $this->assertEquals(200, $response->getStatusCode());
+ $data = json_decode($response->getBody()->getContents(), true);
+
+ $this->assertEquals(['1'], Arr::pluck($data['data'], 'id'));
+ }
+}