1
0
mirror of https://github.com/flarum/core.git synced 2025-08-20 07:11:31 +02:00

perf(core,mentions): limit mentionedBy post relation results (#3780)

* perf(core,mentions): limit `mentionedBy` post relation results

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* Apply fixes from StyleCI

* chore: use a static property to allow customization

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* chore: use a static property to allow customization

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* chore: include count in show post endpoint

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* chore: consistent locale key format

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* chore: forgot to delete `FilterVisiblePosts`

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* test: `mentionedByCount` must not include invisible posts to actor

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* fix: visibility scoping on `mentionedByCount`

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* fix: `loadAggregates` conflicts with visibility scopers

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

* Apply fixes from StyleCI

* chore: phpstan

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

---------

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Co-authored-by: StyleCI Bot <bot@styleci.io>
This commit is contained in:
Sami Mazouz
2023-04-19 08:23:08 +01:00
committed by GitHub
parent 13e655aca5
commit fbbece4bda
22 changed files with 552 additions and 141 deletions

View File

@@ -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 {

View File

@@ -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;
}
}

View File

@@ -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) => (
<li data-number={reply.number()}>
{PostPreview.component({
post: reply,
onclick: hidePreview.bind(this),
})}
</li>
))
<>
{replies.map((reply) => (
<li data-number={reply.number()}>
{PostPreview.component({
post: reply,
onclick: hidePreview.bind(this),
})}
</li>
))}
{replies.length < post.mentionedByCount() ? (
<li className="Post-mentionedBy-preview-more">
<Button
className="PostPreview Button"
onclick={() => {
hidePreview.call(this);
app.modal.show(MentionedByModal, { post });
}}
>
<span className="PostPreview-content">
<span className="PostPreview-badge Avatar">{icon('fas fa-reply-all')}</span>
<span>
{app.translator.trans('flarum-mentions.forum.post.mentioned_by_more_text', { count: post.mentionedByCount() - replies.length })}
</span>
</span>
</Button>
</li>
) : null}
</>
);
$preview

View File

@@ -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<CustomAttrs extends IMentionedByModalAttrs = IMentionedByModalAttrs> extends Modal<
CustomAttrs,
MentionedByModalState
> {
oninit(vnode: Mithril.Vnode<CustomAttrs, this>) {
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 (
<>
<div className="Modal-body">
{this.state.isInitialLoading() ? (
<LoadingIndicator />
) : (
<>
<ul className="MentionedByModal-list Dropdown-menu Dropdown-menu--inline Post-mentionedBy-preview">
{this.state.getPages().map((page) =>
page.items.map((reply) => (
<li data-number={reply.number()}>
<PostPreview post={reply} onclick={() => app.modal.close()} />
</li>
))
)}
</ul>
</>
)}
</div>
{this.state.hasNext() && (
<div className="Modal-footer">
<div className="Form Form--centered">
<div className="Form-group">
<Button className="Button Button--block" onclick={() => this.state.loadNext()} loading={this.state.isLoadingNext()}>
{app.translator.trans('flarum-mentions.forum.mentioned_by.load_more_button')}
</Button>
</div>
</div>
</div>
)}
</>
);
}
}

View File

@@ -8,7 +8,8 @@ export default [
.add('user.mentions', '/u/:username/mentions', MentionsUserPage),
new Extend.Model(Post) //
.hasMany<Post>('mentionedBy'),
.hasMany<Post>('mentionedBy')
.attribute<number>('mentionedByCount'),
new Extend.Model(User) //
.attribute<boolean>('canMentionGroups'),

View File

@@ -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<P extends MentionedByModalParams = MentionedByModalParams> extends PaginatedListState<Post, P> {
constructor(params: P, page: number = 1) {
const limit = 10;
params.page = { ...(params.page || {}), limit };
super(params, page, limit);
}
get type(): string {
return 'posts';
}
}

View File

@@ -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

View File

@@ -0,0 +1,68 @@
<?php
/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/
namespace Flarum\Mentions\Api;
use Flarum\Discussion\Discussion;
use Flarum\Http\RequestUtil;
use Flarum\Post\Post;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Psr\Http\Message\ServerRequestInterface;
/**
* Apply visibility permissions to API data's mentionedBy relationship.
* And limit mentionedBy to 3 posts only for performance reasons.
*/
class LoadMentionedByRelationship
{
public static $maxMentionedBy = 4;
public static function mutateRelation(BelongsToMany $query, ServerRequestInterface $request)
{
$actor = RequestUtil::getActor($request);
return $query
->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);
}
]);
}
}
}

View File

@@ -0,0 +1,31 @@
<?php
/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/
namespace Flarum\Mentions\Filter;
use Flarum\Filter\FilterInterface;
use Flarum\Filter\FilterState;
class MentionedPostFilter implements FilterInterface
{
public function getFilterKey(): string
{
return 'mentionedPost';
}
public function filter(FilterState $filterState, string $filterValue, bool $negate)
{
$mentionedId = trim($filterValue, '"');
$filterState
->getQuery()
->join('post_mentions_post', 'posts.id', '=', 'post_mentions_post.post_id')
->where('post_mentions_post.mentions_post_id', $negate ? '!=' : '=', $mentionedId);
}
}

View File

@@ -1,100 +0,0 @@
<?php
/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/
namespace Flarum\Mentions;
use Flarum\Api\Controller;
use Flarum\Http\RequestUtil;
use Flarum\Post\CommentPost;
use Flarum\Post\PostRepository;
use Illuminate\Database\Eloquent\Collection;
use Psr\Http\Message\ServerRequestInterface;
class FilterVisiblePosts
{
/**
* @var PostRepository
*/
protected $posts;
/**
* @param PostRepository $posts
*/
public function __construct(PostRepository $posts)
{
$this->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;
}));
}
}
}
}

View File

@@ -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']);

View File

@@ -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' => '<t><p>text</p></t>'],
['id' => 102, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 103, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>', 'is_private' => 1],
['id' => 104, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 105, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 106, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 107, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 108, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 109, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 110, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 111, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
['id' => 112, 'discussion_id' => 100, 'created_at' => Carbon::now(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>text</p></t>'],
],
'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']);
}
}