From fbbece4bdaede0c5db8fd9ee5eaf26e4820dc018 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Wed, 19 Apr 2023 08:23:08 +0100 Subject: [PATCH] perf(core,mentions): limit `mentionedBy` post relation results (#3780) * perf(core,mentions): limit `mentionedBy` post relation results Signed-off-by: Sami Mazouz * Apply fixes from StyleCI * chore: use a static property to allow customization Signed-off-by: Sami Mazouz * chore: use a static property to allow customization Signed-off-by: Sami Mazouz * chore: include count in show post endpoint Signed-off-by: Sami Mazouz * chore: consistent locale key format Signed-off-by: Sami Mazouz * chore: forgot to delete `FilterVisiblePosts` Signed-off-by: Sami Mazouz * test: `mentionedByCount` must not include invisible posts to actor Signed-off-by: Sami Mazouz * fix: visibility scoping on `mentionedByCount` Signed-off-by: Sami Mazouz * fix: `loadAggregates` conflicts with visibility scopers Signed-off-by: Sami Mazouz * Apply fixes from StyleCI * chore: phpstan Signed-off-by: Sami Mazouz --------- Signed-off-by: Sami Mazouz Co-authored-by: StyleCI Bot --- composer.json | 1 + extensions/mentions/extend.php | 34 ++-- extensions/mentions/js/src/@types/shims.d.ts | 8 + .../js/src/forum/addMentionedByList.js | 38 ++++- .../src/forum/components/MentionedByModal.tsx | 73 +++++++++ extensions/mentions/js/src/forum/extend.ts | 3 +- .../src/forum/state/MentionedByModalState.ts | 27 ++++ extensions/mentions/locale/en.yml | 6 + .../src/Api/LoadMentionedByRelationship.php | 68 ++++++++ .../src/Filter/MentionedPostFilter.php | 31 ++++ .../mentions/src/FilterVisiblePosts.php | 100 ------------ .../integration/api/GroupMentionsTest.php | 6 +- .../tests/integration/api/ListPostsTest.php | 151 +++++++++++++++++- framework/core/composer.json | 1 + framework/core/less/common/Dropdown.less | 5 + framework/core/less/forum/Post.less | 5 +- .../Controller/ShowDiscussionController.php | 24 +-- .../src/Api/Controller/ShowPostController.php | 9 +- framework/core/src/Database/AbstractModel.php | 47 ++++++ .../core/src/Database/Eloquent/Collection.php | 50 ++++++ framework/core/src/Extend/ApiController.php | 4 +- framework/core/src/Post/Post.php | 2 + 22 files changed, 552 insertions(+), 141 deletions(-) create mode 100644 extensions/mentions/js/src/@types/shims.d.ts create mode 100644 extensions/mentions/js/src/forum/components/MentionedByModal.tsx create mode 100644 extensions/mentions/js/src/forum/state/MentionedByModalState.ts create mode 100644 extensions/mentions/src/Api/LoadMentionedByRelationship.php create mode 100644 extensions/mentions/src/Filter/MentionedPostFilter.php delete mode 100755 extensions/mentions/src/FilterVisiblePosts.php create mode 100644 framework/core/src/Database/Eloquent/Collection.php diff --git a/composer.json b/composer.json index f6d2417ca..da4d69099 100644 --- a/composer.json +++ b/composer.json @@ -127,6 +127,7 @@ "psr/http-server-middleware": "^1.0", "pusher/pusher-php-server": "^2.2", "s9e/text-formatter": "^2.3.6", + "staudenmeir/eloquent-eager-limit": "^1.0", "sycho/json-api": "^0.5.0", "sycho/sourcemap": "^2.0.0", "symfony/config": "^5.2.2", diff --git a/extensions/mentions/extend.php b/extensions/mentions/extend.php index 3dfe67b15..d2d79ea9f 100644 --- a/extensions/mentions/extend.php +++ b/extensions/mentions/extend.php @@ -18,6 +18,7 @@ use Flarum\Api\Serializer\PostSerializer; use Flarum\Approval\Event\PostWasApproved; use Flarum\Extend; use Flarum\Group\Group; +use Flarum\Mentions\Api\LoadMentionedByRelationship; use Flarum\Post\Event\Deleted; use Flarum\Post\Event\Hidden; use Flarum\Post\Event\Posted; @@ -64,15 +65,20 @@ return [ ->hasMany('mentionedBy', BasicPostSerializer::class) ->hasMany('mentionsPosts', BasicPostSerializer::class) ->hasMany('mentionsUsers', BasicUserSerializer::class) - ->hasMany('mentionsGroups', GroupSerializer::class), + ->hasMany('mentionsGroups', GroupSerializer::class) + ->attribute('mentionedByCount', function (BasicPostSerializer $serializer, Post $post) { + // Only if it was eager loaded. + return $post->getAttribute('mentioned_by_count') ?? 0; + }), (new Extend\ApiController(Controller\ShowDiscussionController::class)) ->addInclude(['posts.mentionedBy', 'posts.mentionedBy.user', 'posts.mentionedBy.discussion']) ->load([ - 'posts.mentionsUsers', 'posts.mentionsPosts', 'posts.mentionsPosts.user', 'posts.mentionedBy', - 'posts.mentionedBy.mentionsPosts', 'posts.mentionedBy.mentionsPosts.user', 'posts.mentionedBy.mentionsUsers', + 'posts.mentionsUsers', 'posts.mentionsPosts', 'posts.mentionsPosts.user', 'posts.mentionsGroups' - ]), + ]) + ->loadWhere('posts.mentionedBy', [LoadMentionedByRelationship::class, 'mutateRelation']) + ->prepareDataForSerialization([LoadMentionedByRelationship::class, 'countRelation']), (new Extend\ApiController(Controller\ListDiscussionsController::class)) ->load([ @@ -81,15 +87,17 @@ return [ ]), (new Extend\ApiController(Controller\ShowPostController::class)) - ->addInclude(['mentionedBy', 'mentionedBy.user', 'mentionedBy.discussion']), + ->addInclude(['mentionedBy', 'mentionedBy.user', 'mentionedBy.discussion']) + // We wouldn't normally need to eager load on a single model, + // but we do so here for visibility scoping. + ->loadWhere('mentionedBy', [LoadMentionedByRelationship::class, 'mutateRelation']) + ->prepareDataForSerialization([LoadMentionedByRelationship::class, 'countRelation']), (new Extend\ApiController(Controller\ListPostsController::class)) ->addInclude(['mentionedBy', 'mentionedBy.user', 'mentionedBy.discussion']) - ->load([ - 'mentionsUsers', 'mentionsPosts', 'mentionsPosts.user', 'mentionedBy', - 'mentionedBy.mentionsPosts', 'mentionedBy.mentionsPosts.user', 'mentionedBy.mentionsUsers', - 'mentionsGroups' - ]), + ->load(['mentionsUsers', 'mentionsPosts', 'mentionsPosts.user', 'mentionsGroups']) + ->loadWhere('mentionedBy', [LoadMentionedByRelationship::class, 'mutateRelation']) + ->prepareDataForSerialization([LoadMentionedByRelationship::class, 'countRelation']), (new Extend\ApiController(Controller\CreatePostController::class)) ->addOptionalInclude('mentionsGroups'), @@ -97,9 +105,6 @@ return [ (new Extend\ApiController(Controller\UpdatePostController::class)) ->addOptionalInclude('mentionsGroups'), - (new Extend\ApiController(Controller\AbstractSerializeController::class)) - ->prepareDataForSerialization(FilterVisiblePosts::class), - (new Extend\Settings) ->serializeToForum('allowUsernameMentionFormat', 'flarum-mentions.allow_username_format', 'boolval'), @@ -112,7 +117,8 @@ return [ ->listen(Deleted::class, Listener\UpdateMentionsMetadataWhenInvisible::class), (new Extend\Filter(PostFilterer::class)) - ->addFilter(Filter\MentionedFilter::class), + ->addFilter(Filter\MentionedFilter::class) + ->addFilter(Filter\MentionedPostFilter::class), (new Extend\ApiSerializer(CurrentUserSerializer::class)) ->attribute('canMentionGroups', function (CurrentUserSerializer $serializer, User $user, array $attributes): bool { diff --git a/extensions/mentions/js/src/@types/shims.d.ts b/extensions/mentions/js/src/@types/shims.d.ts new file mode 100644 index 000000000..1878233d1 --- /dev/null +++ b/extensions/mentions/js/src/@types/shims.d.ts @@ -0,0 +1,8 @@ +import type BasePost from 'flarum/common/models/Post'; + +declare module 'flarum/common/models/Post' { + export default interface Post { + mentionedBy(): BasePost[] | undefined | null; + mentionedByCount(): number; + } +} diff --git a/extensions/mentions/js/src/forum/addMentionedByList.js b/extensions/mentions/js/src/forum/addMentionedByList.js index 737621869..a86fe569a 100644 --- a/extensions/mentions/js/src/forum/addMentionedByList.js +++ b/extensions/mentions/js/src/forum/addMentionedByList.js @@ -6,6 +6,8 @@ import PostPreview from 'flarum/forum/components/PostPreview'; import punctuateSeries from 'flarum/common/helpers/punctuateSeries'; import username from 'flarum/common/helpers/username'; import icon from 'flarum/common/helpers/icon'; +import Button from 'flarum/common/components/Button'; +import MentionedByModal from './components/MentionedByModal'; export default function addMentionedByList() { function hidePreview() { @@ -36,14 +38,34 @@ export default function addMentionedByList() { // popup. m.render( $preview[0], - replies.map((reply) => ( -
  • - {PostPreview.component({ - post: reply, - onclick: hidePreview.bind(this), - })} -
    • - )) + <> + {replies.map((reply) => ( +
  • + {PostPreview.component({ + post: reply, + onclick: hidePreview.bind(this), + })} +
    • + ))} + {replies.length < post.mentionedByCount() ? ( +
  • + +
    • + ) : null} + ); $preview diff --git a/extensions/mentions/js/src/forum/components/MentionedByModal.tsx b/extensions/mentions/js/src/forum/components/MentionedByModal.tsx new file mode 100644 index 000000000..9a5d431a0 --- /dev/null +++ b/extensions/mentions/js/src/forum/components/MentionedByModal.tsx @@ -0,0 +1,73 @@ +import app from 'flarum/forum/app'; +import PostPreview from 'flarum/forum/components/PostPreview'; +import Modal, { IInternalModalAttrs } from 'flarum/common/components/Modal'; +import type Mithril from 'mithril'; +import type Post from 'flarum/common/models/Post'; +import LoadingIndicator from 'flarum/common/components/LoadingIndicator'; +import Button from 'flarum/common/components/Button'; +import MentionedByModalState from '../state/MentionedByModalState'; + +export interface IMentionedByModalAttrs extends IInternalModalAttrs { + post: Post; +} + +export default class MentionedByModal extends Modal< + CustomAttrs, + MentionedByModalState +> { + oninit(vnode: Mithril.Vnode) { + super.oninit(vnode); + + this.state = new MentionedByModalState({ + filter: { + mentionedPost: this.attrs.post.id()!, + }, + sort: 'number', + }); + + this.state.refresh(); + } + + className(): string { + return 'MentionedByModal'; + } + + title(): Mithril.Children { + return app.translator.trans('flarum-mentions.forum.mentioned_by.title'); + } + + content(): Mithril.Children { + return ( + <> +
    + {this.state.isInitialLoading() ? ( + + ) : ( + <> +
      + {this.state.getPages().map((page) => + page.items.map((reply) => ( +
    • + app.modal.close()} /> +
      • + )) + )} +
    + + )} +
    + {this.state.hasNext() && ( +
    +
    +
    + +
    +
    +
    + )} + + ); + } +} diff --git a/extensions/mentions/js/src/forum/extend.ts b/extensions/mentions/js/src/forum/extend.ts index d98a9613f..e14c8fca3 100644 --- a/extensions/mentions/js/src/forum/extend.ts +++ b/extensions/mentions/js/src/forum/extend.ts @@ -8,7 +8,8 @@ export default [ .add('user.mentions', '/u/:username/mentions', MentionsUserPage), new Extend.Model(Post) // - .hasMany('mentionedBy'), + .hasMany('mentionedBy') + .attribute('mentionedByCount'), new Extend.Model(User) // .attribute('canMentionGroups'), diff --git a/extensions/mentions/js/src/forum/state/MentionedByModalState.ts b/extensions/mentions/js/src/forum/state/MentionedByModalState.ts new file mode 100644 index 000000000..b2cdad8ca --- /dev/null +++ b/extensions/mentions/js/src/forum/state/MentionedByModalState.ts @@ -0,0 +1,27 @@ +import PaginatedListState, { PaginatedListParams } from 'flarum/common/states/PaginatedListState'; +import Post from 'flarum/common/models/Post'; + +export interface MentionedByModalParams extends PaginatedListParams { + filter: { + mentionedPost: string; + }; + sort?: string; + page?: { + offset?: number; + limit: number; + }; +} + +export default class MentionedByModalState

    extends PaginatedListState { + constructor(params: P, page: number = 1) { + const limit = 10; + + params.page = { ...(params.page || {}), limit }; + + super(params, page, limit); + } + + get type(): string { + return 'posts'; + } +} diff --git a/extensions/mentions/locale/en.yml b/extensions/mentions/locale/en.yml index 0a1ea3fba..108af9379 100644 --- a/extensions/mentions/locale/en.yml +++ b/extensions/mentions/locale/en.yml @@ -25,6 +25,11 @@ flarum-mentions: mention_tooltip: Mention a user, group or post reply_to_post_text: "Reply to #{number}" + # These translations are used by the mentioned by modal dialog. + mentioned_by: + title: Replies to this post + load_more_button: => core.ref.load_more + # These translations are used by the Notifications dropdown, a.k.a. "the bell". notifications: others_text: => core.ref.some_others @@ -34,6 +39,7 @@ flarum-mentions: # These translations are displayed beneath individual posts. post: + mentioned_by_more_text: "{count} more replies." mentioned_by_self_text: "{users} replied to this." # Can be pluralized to agree with the number of users! mentioned_by_text: "{users} replied to this." # Can be pluralized to agree with the number of users! others_text: => core.ref.some_others diff --git a/extensions/mentions/src/Api/LoadMentionedByRelationship.php b/extensions/mentions/src/Api/LoadMentionedByRelationship.php new file mode 100644 index 000000000..e6ba9f4fb --- /dev/null +++ b/extensions/mentions/src/Api/LoadMentionedByRelationship.php @@ -0,0 +1,68 @@ +with(['mentionsPosts', 'mentionsPosts.user', 'mentionsUsers']) + ->whereVisibleTo($actor) + ->oldest() + // Limiting a relationship results is only possible because + // the Post model uses the \Staudenmeir\EloquentEagerLimit\HasEagerLimit + // trait. + ->limit(self::$maxMentionedBy); + } + + /** + * Called using the @see ApiController::prepareDataForSerialization extender. + */ + public static function countRelation($controller, $data, ServerRequestInterface $request): void + { + $actor = RequestUtil::getActor($request); + $loadable = null; + + if ($data instanceof Discussion) { + // @phpstan-ignore-next-line + $loadable = $data->newCollection($data->posts)->filter(function ($post) { + return $post instanceof Post; + }); + } elseif ($data instanceof Collection) { + $loadable = $data; + } elseif ($data instanceof Post) { + $loadable = $data->newCollection([$data]); + } + + if ($loadable) { + $loadable->loadCount([ + 'mentionedBy' => function ($query) use ($actor) { + return $query->whereVisibleTo($actor); + } + ]); + } + } +} diff --git a/extensions/mentions/src/Filter/MentionedPostFilter.php b/extensions/mentions/src/Filter/MentionedPostFilter.php new file mode 100644 index 000000000..e9b32945d --- /dev/null +++ b/extensions/mentions/src/Filter/MentionedPostFilter.php @@ -0,0 +1,31 @@ +getQuery() + ->join('post_mentions_post', 'posts.id', '=', 'post_mentions_post.post_id') + ->where('post_mentions_post.mentions_post_id', $negate ? '!=' : '=', $mentionedId); + } +} diff --git a/extensions/mentions/src/FilterVisiblePosts.php b/extensions/mentions/src/FilterVisiblePosts.php deleted file mode 100755 index 47aec39d2..000000000 --- a/extensions/mentions/src/FilterVisiblePosts.php +++ /dev/null @@ -1,100 +0,0 @@ -posts = $posts; - } - - /** - * Apply visibility permissions to API data. - * - * Each post in an API document has a relationship with posts that have - * mentioned it (mentionedBy). This listener will manually filter these - * additional posts so that the user can't see any posts which they don't - * have access to. - * - * @param Controller\AbstractSerializeController $controller - * @param mixed $data - */ - public function __invoke(Controller\AbstractSerializeController $controller, $data, ServerRequestInterface $request) - { - $relations = []; - - // Firstly we gather a list of posts contained within the API document. - // This will vary according to the API endpoint that is being accessed. - if ($controller instanceof Controller\ShowDiscussionController) { - $posts = $data->posts; - } elseif ($controller instanceof Controller\ShowPostController - || $controller instanceof Controller\CreatePostController - || $controller instanceof Controller\UpdatePostController) { - $relations = [ - 'mentionsUsers', 'mentionsPosts', 'mentionsPosts.user', 'mentionedBy', 'mentionsGroups', - 'mentionedBy.mentionsPosts', 'mentionedBy.mentionsPosts.user', 'mentionedBy.mentionsUsers', 'mentionedBy.mentionsGroups.group' - ]; - - $posts = [$data]; - } elseif ($controller instanceof Controller\ListPostsController) { - $posts = $data; - } - - if (isset($posts)) { - $posts = new Collection($posts); - $actor = RequestUtil::getActor($request); - - $posts = $posts->filter(function ($post) { - return $post instanceof CommentPost; - }); - - // Load all of the users that these posts mention. This way the data - // will be ready to go when we need to sub in current usernames - // during the rendering process. - $posts->loadMissing($relations); - - // Construct a list of the IDs of all of the posts that these posts - // have been mentioned in. We can then filter this list of IDs to - // weed out all of the ones which the user is not meant to see. - $ids = []; - - foreach ($posts as $post) { - $ids = array_merge($ids, $post->mentionedBy->pluck('id')->all()); - } - - $ids = $this->posts->filterVisibleIds($ids, $actor); - - // Finally, go back through each of the posts and filter out any - // of the posts in the relationship data that we now know are - // invisible to the user. - foreach ($posts as $post) { - $post->setRelation('mentionedBy', $post->mentionedBy->filter(function ($post) use ($ids) { - return array_search($post->id, $ids) !== false; - })); - } - } - } -} diff --git a/extensions/mentions/tests/integration/api/GroupMentionsTest.php b/extensions/mentions/tests/integration/api/GroupMentionsTest.php index a5dbe9897..f001ba2bf 100644 --- a/extensions/mentions/tests/integration/api/GroupMentionsTest.php +++ b/extensions/mentions/tests/integration/api/GroupMentionsTest.php @@ -80,9 +80,11 @@ class GroupMentionsTest extends TestCase $this->request('GET', '/api/posts/4') ); - $this->assertEquals(200, $response->getStatusCode()); + $contents = $response->getBody()->getContents(); - $response = json_decode($response->getBody(), true); + $this->assertEquals(200, $response->getStatusCode(), $contents); + + $response = json_decode($contents, true); $this->assertStringContainsString('GroupMention', $response['data']['attributes']['contentHtml']); $this->assertStringContainsString('#80349E', $response['data']['attributes']['contentHtml']); diff --git a/extensions/mentions/tests/integration/api/ListPostsTest.php b/extensions/mentions/tests/integration/api/ListPostsTest.php index b3fd0b551..ab0964648 100644 --- a/extensions/mentions/tests/integration/api/ListPostsTest.php +++ b/extensions/mentions/tests/integration/api/ListPostsTest.php @@ -7,9 +7,10 @@ * LICENSE file that was distributed with this source code. */ -namespace Flarum\Tests\integration\api\discussions; +namespace Flarum\Mentions\Tests\integration\api\discussions; use Carbon\Carbon; +use Flarum\Mentions\Api\LoadMentionedByRelationship; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Illuminate\Support\Arr; @@ -107,4 +108,152 @@ class ListPostsTest extends TestCase $ids = Arr::pluck($data, 'id'); $this->assertEqualsCanonicalizing(['3', '2'], $ids, 'IDs do not match'); } + + protected function prepareMentionedByData(): void + { + $this->prepareDatabase([ + 'discussions' => [ + ['id' => 100, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 101, 'comment_count' => 12], + ], + 'posts' => [ + ['id' => 101, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 102, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 103, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    ', 'is_private' => 1], + ['id' => 104, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 105, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 106, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 107, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 108, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 109, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 110, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 111, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ['id' => 112, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '

    text

    '], + ], + 'post_mentions_post' => [ + ['post_id' => 102, 'mentions_post_id' => 101], + ['post_id' => 103, 'mentions_post_id' => 101], + ['post_id' => 104, 'mentions_post_id' => 101], + ['post_id' => 105, 'mentions_post_id' => 101], + ['post_id' => 106, 'mentions_post_id' => 101], + ['post_id' => 107, 'mentions_post_id' => 101], + ['post_id' => 108, 'mentions_post_id' => 101], + ['post_id' => 109, 'mentions_post_id' => 101], + ['post_id' => 110, 'mentions_post_id' => 101], + ['post_id' => 111, 'mentions_post_id' => 101], + ['post_id' => 112, 'mentions_post_id' => 101], + ['post_id' => 103, 'mentions_post_id' => 112], + ], + ]); + } + + /** @test */ + public function mentioned_by_relation_returns_limited_results_and_shows_only_visible_posts_in_show_post_endpoint() + { + $this->prepareMentionedByData(); + + // List posts endpoint + $response = $this->send( + $this->request('GET', '/api/posts/101', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + 'include' => 'mentionedBy', + ]) + ); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $this->assertEquals(200, $response->getStatusCode()); + + $mentionedBy = $data['relationships']['mentionedBy']['data']; + + // Only displays a limited amount of mentioned by posts + $this->assertCount(LoadMentionedByRelationship::$maxMentionedBy, $mentionedBy); + // Of the limited amount of mentioned by posts, they must be visible to the actor + $this->assertEquals([102, 104, 105, 106], Arr::pluck($mentionedBy, 'id')); + } + + /** @test */ + public function mentioned_by_relation_returns_limited_results_and_shows_only_visible_posts_in_list_posts_endpoint() + { + $this->prepareMentionedByData(); + + // List posts endpoint + $response = $this->send( + $this->request('GET', '/api/posts', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + 'filter' => ['discussion' => 100], + 'include' => 'mentionedBy', + ]) + ); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $this->assertEquals(200, $response->getStatusCode()); + + $mentionedBy = $data[0]['relationships']['mentionedBy']['data']; + + // Only displays a limited amount of mentioned by posts + $this->assertCount(LoadMentionedByRelationship::$maxMentionedBy, $mentionedBy); + // Of the limited amount of mentioned by posts, they must be visible to the actor + $this->assertEquals([102, 104, 105, 106], Arr::pluck($mentionedBy, 'id')); + } + + /** + * @dataProvider mentionedByIncludeProvider + * @test + */ + public function mentioned_by_relation_returns_limited_results_and_shows_only_visible_posts_in_show_discussion_endpoint(string $include) + { + $this->prepareMentionedByData(); + + // Show discussion endpoint + $response = $this->send( + $this->request('GET', '/api/discussions/100', [ + 'authenticatedAs' => 2, + ])->withQueryParams([ + 'include' => $include, + ]) + ); + + $included = json_decode($response->getBody()->getContents(), true)['included']; + + $mentionedBy = collect($included) + ->where('type', 'posts') + ->where('id', 101) + ->first()['relationships']['mentionedBy']['data']; + + // Only displays a limited amount of mentioned by posts + $this->assertCount(LoadMentionedByRelationship::$maxMentionedBy, $mentionedBy); + // Of the limited amount of mentioned by posts, they must be visible to the actor + $this->assertEquals([102, 104, 105, 106], Arr::pluck($mentionedBy, 'id')); + } + + public function mentionedByIncludeProvider(): array + { + return [ + ['posts,posts.mentionedBy'], + ['posts.mentionedBy'], + [''], + ]; + } + + /** @test */ + public function mentioned_by_count_only_includes_visible_posts_to_actor() + { + $this->prepareMentionedByData(); + + // List posts endpoint + $response = $this->send( + $this->request('GET', '/api/posts/112', [ + 'authenticatedAs' => 2, + ]) + ); + + $data = json_decode($response->getBody()->getContents(), true)['data']; + + $this->assertEquals(200, $response->getStatusCode()); + + $this->assertEquals(0, $data['attributes']['mentionedByCount']); + } } diff --git a/framework/core/composer.json b/framework/core/composer.json index 05684fa49..bb1ebccc6 100644 --- a/framework/core/composer.json +++ b/framework/core/composer.json @@ -76,6 +76,7 @@ "psr/http-server-handler": "^1.0", "psr/http-server-middleware": "^1.0", "s9e/text-formatter": "^2.3.6", + "staudenmeir/eloquent-eager-limit": "^1.0", "sycho/json-api": "^0.5.0", "sycho/sourcemap": "^2.0.0", "symfony/config": "^5.2.2", diff --git a/framework/core/less/common/Dropdown.less b/framework/core/less/common/Dropdown.less index bba41342b..da3163fa8 100644 --- a/framework/core/less/common/Dropdown.less +++ b/framework/core/less/common/Dropdown.less @@ -79,6 +79,11 @@ left: auto; right: 0; } +.Dropdown-menu--inline { + position: relative; + display: block; + box-shadow: none; +} .Dropdown-header { padding: 10px 15px; color: var(--heading-color); diff --git a/framework/core/less/forum/Post.less b/framework/core/less/forum/Post.less index 22b34c3a8..b2516c6d2 100644 --- a/framework/core/less/forum/Post.less +++ b/framework/core/less/forum/Post.less @@ -353,11 +353,14 @@ word-wrap: break-word; } - .Avatar { + &-badge, .Avatar { float: left; margin-left: -50px; .Avatar--size(32px); } + &-badge { + color: var(--control-color); + } .username { color: var(--text-color); font-weight: bold; diff --git a/framework/core/src/Api/Controller/ShowDiscussionController.php b/framework/core/src/Api/Controller/ShowDiscussionController.php index 33f7a7cb5..251254675 100644 --- a/framework/core/src/Api/Controller/ShowDiscussionController.php +++ b/framework/core/src/Api/Controller/ShowDiscussionController.php @@ -94,7 +94,8 @@ class ShowDiscussionController extends AbstractShowController $discussion = $this->discussions->findOrFail($discussionId, $actor); } - if (in_array('posts', $include)) { + // If posts is included or a sub relation of post is included. + if (in_array('posts', $include) || Str::contains(implode(',', $include), 'posts.')) { $postRelationships = $this->getPostRelationships($include); $this->includePosts($discussion, $request, $postRelationships); @@ -119,7 +120,7 @@ class ShowDiscussionController extends AbstractShowController $offset = $this->getPostsOffset($request, $discussion, $limit); $allPosts = $this->loadPostIds($discussion, $actor); - $loadedPosts = $this->loadPosts($discussion, $actor, $offset, $limit, $include); + $loadedPosts = $this->loadPosts($discussion, $actor, $offset, $limit, $include, $request); array_splice($allPosts, $offset, $limit, $loadedPosts); @@ -136,11 +137,7 @@ class ShowDiscussionController extends AbstractShowController return $discussion->posts()->whereVisibleTo($actor)->orderBy('number')->pluck('id')->all(); } - /** - * @param array $include - * @return array - */ - private function getPostRelationships(array $include) + private function getPostRelationships(array $include): array { $prefixLength = strlen($prefix = 'posts.'); $relationships = []; @@ -183,11 +180,11 @@ class ShowDiscussionController extends AbstractShowController * @param array $include * @return mixed */ - private function loadPosts($discussion, $actor, $offset, $limit, array $include) + private function loadPosts($discussion, $actor, $offset, $limit, array $include, ServerRequestInterface $request) { $query = $discussion->posts()->whereVisibleTo($actor); - $query->orderBy('number')->skip($offset)->take($limit)->with($include); + $query->orderBy('number')->skip($offset)->take($limit); $posts = $query->get(); @@ -195,7 +192,7 @@ class ShowDiscussionController extends AbstractShowController $post->discussion = $discussion; } - $this->loadRelations($posts, $include); + $this->loadRelations($posts, $include, $request); return $posts->all(); } @@ -221,8 +218,13 @@ class ShowDiscussionController extends AbstractShowController $postCallableRelationships = $this->getPostRelationships(array_keys($addedCallableRelations)); - return array_intersect_key($addedCallableRelations, array_flip(array_map(function ($relation) { + $relationCallables = array_intersect_key($addedCallableRelations, array_flip(array_map(function ($relation) { return "posts.$relation"; }, $postCallableRelationships))); + + // remove posts. prefix from keys + return array_combine(array_map(function ($relation) { + return substr($relation, 6); + }, array_keys($relationCallables)), array_values($relationCallables)); } } diff --git a/framework/core/src/Api/Controller/ShowPostController.php b/framework/core/src/Api/Controller/ShowPostController.php index 2762c1ee9..43d239b6b 100644 --- a/framework/core/src/Api/Controller/ShowPostController.php +++ b/framework/core/src/Api/Controller/ShowPostController.php @@ -12,6 +12,7 @@ namespace Flarum\Api\Controller; use Flarum\Api\Serializer\PostSerializer; use Flarum\Http\RequestUtil; use Flarum\Post\PostRepository; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Support\Arr; use Psr\Http\Message\ServerRequestInterface; use Tobscure\JsonApi\Document; @@ -52,6 +53,12 @@ class ShowPostController extends AbstractShowController */ protected function data(ServerRequestInterface $request, Document $document) { - return $this->posts->findOrFail(Arr::get($request->getQueryParams(), 'id'), RequestUtil::getActor($request)); + $post = $this->posts->findOrFail(Arr::get($request->getQueryParams(), 'id'), RequestUtil::getActor($request)); + + $include = $this->extractInclude($request); + + $this->loadRelations(new Collection([$post]), $include, $request); + + return $post; } } diff --git a/framework/core/src/Database/AbstractModel.php b/framework/core/src/Database/AbstractModel.php index 86e6ef995..9805ecf0d 100644 --- a/framework/core/src/Database/AbstractModel.php +++ b/framework/core/src/Database/AbstractModel.php @@ -9,9 +9,11 @@ namespace Flarum\Database; +use Flarum\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Model as Eloquent; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Support\Arr; +use Illuminate\Support\Str; use LogicException; /** @@ -61,6 +63,14 @@ abstract class AbstractModel extends Eloquent */ public static $defaults = []; + /** + * An alias for the table name, used in queries. + * + * @var string|null + * @internal + */ + protected $tableAlias = null; + /** * {@inheritdoc} */ @@ -213,4 +223,41 @@ abstract class AbstractModel extends Eloquent return parent::__call($method, $arguments); } + + public function newModelQuery() + { + $query = parent::newModelQuery(); + + if ($this->tableAlias) { + $query->from($this->getTable().' as '.$this->tableAlias); + } + + return $query; + } + + public function qualifyColumn($column) + { + if (Str::contains($column, '.')) { + return $column; + } + + return ($this->tableAlias ?? $this->getTable()).'.'.$column; + } + + public function withTableAlias(callable $callback) + { + static $aliasCount = 0; + $this->tableAlias = 'flarum_reserved_'.++$aliasCount; + + $result = $callback(); + + $this->tableAlias = null; + + return $result; + } + + public function newCollection(array $models = []) + { + return new Collection($models); + } } diff --git a/framework/core/src/Database/Eloquent/Collection.php b/framework/core/src/Database/Eloquent/Collection.php new file mode 100644 index 000000000..fbdd67de7 --- /dev/null +++ b/framework/core/src/Database/Eloquent/Collection.php @@ -0,0 +1,50 @@ +isEmpty()) { + return $this; + } + + return $this->first()->withTableAlias(function () use ($relations, $column, $function) { + return parent::loadAggregate($relations, $column, $function); + }); + } +} diff --git a/framework/core/src/Extend/ApiController.php b/framework/core/src/Extend/ApiController.php index 78cf72c6f..b87b84602 100644 --- a/framework/core/src/Extend/ApiController.php +++ b/framework/core/src/Extend/ApiController.php @@ -313,7 +313,7 @@ class ApiController implements ExtenderInterface * Allows loading a relationship with additional query modification. * * @param string $relation: Relationship name, see load method description. - * @param callable(\Illuminate\Database\Query\Builder|\Illuminate\Database\Eloquent\Relations\Relation, \Psr\Http\Message\ServerRequestInterface|null, array): void $callback + * @param array|(callable(\Illuminate\Database\Query\Builder|\Illuminate\Database\Eloquent\Relations\Relation, \Psr\Http\Message\ServerRequestInterface|null, array): void) $callback * * The callback to modify the query, should accept: * - \Illuminate\Database\Query\Builder|\Illuminate\Database\Eloquent\Relations\Relation $query: A query object. @@ -322,7 +322,7 @@ class ApiController implements ExtenderInterface * * @return self */ - public function loadWhere(string $relation, callable $callback): self + public function loadWhere(string $relation, callable $callback): self // @phpstan-ignore-line { $this->loadCallables = array_merge($this->loadCallables, [$relation => $callback]); diff --git a/framework/core/src/Post/Post.php b/framework/core/src/Post/Post.php index bdabe071e..fa86b8db1 100644 --- a/framework/core/src/Post/Post.php +++ b/framework/core/src/Post/Post.php @@ -18,6 +18,7 @@ use Flarum\Post\Event\Deleted; use Flarum\User\User; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Query\Expression; +use Staudenmeir\EloquentEagerLimit\HasEagerLimit; /** * @property int $id @@ -42,6 +43,7 @@ class Post extends AbstractModel { use EventGeneratorTrait; use ScopeVisibilityTrait; + use HasEagerLimit; protected $table = 'posts';