From 23958c0e9a662fb36bc65eb88eb4e3fa3a635284 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Sat, 12 Oct 2024 11:20:51 +0100 Subject: [PATCH] refactor: remove preloading/listing of posts in the show discussion endpoint --- extensions/flags/extend.php | 5 -- extensions/likes/extend.php | 5 -- .../tests/integration/api/ListPostsTest.php | 8 +-- extensions/mentions/extend.php | 12 ---- .../tests/integration/api/ListPostsTest.php | 8 +-- extensions/tags/extend.php | 6 -- .../src/Api/Resource/DiscussionResource.php | 67 ++++--------------- .../core/src/Api/Resource/PostResource.php | 1 - framework/core/src/Api/Serializer.php | 23 +++---- .../core/src/Forum/Content/Discussion.php | 52 +++++++++++--- .../integration/api/discussions/ShowTest.php | 13 ++-- .../extenders/ModelVisibilityTest.php | 7 +- 12 files changed, 85 insertions(+), 122 deletions(-) diff --git a/extensions/flags/extend.php b/extensions/flags/extend.php index 71c93643f4..b9245096ff 100644 --- a/extensions/flags/extend.php +++ b/extensions/flags/extend.php @@ -52,11 +52,6 @@ (new Extend\ApiResource(Resource\ForumResource::class)) ->fields(ForumResourceFields::class), - (new Extend\ApiResource(Resource\DiscussionResource::class)) - ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint) { - return $endpoint->addDefaultInclude(['posts.flags', 'posts.flags.user']); - }), - (new Extend\ApiResource(Resource\PostResource::class)) ->endpoint([Endpoint\Index::class, Endpoint\Show::class], function (Endpoint\Index|Endpoint\Show $endpoint) { return $endpoint->addDefaultInclude(['flags', 'flags.user']); diff --git a/extensions/likes/extend.php b/extensions/likes/extend.php index e84f0c5a1b..eb12697c17 100644 --- a/extensions/likes/extend.php +++ b/extensions/likes/extend.php @@ -50,11 +50,6 @@ function (Endpoint\Index|Endpoint\Show|Endpoint\Create|Endpoint\Update $endpoint } ), - (new Extend\ApiResource(Resource\DiscussionResource::class)) - ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Endpoint { - return $endpoint->addDefaultInclude(['posts.likes']); - }), - (new Extend\Event()) ->listen(PostWasLiked::class, Listener\SendNotificationWhenPostIsLiked::class) ->listen(PostWasUnliked::class, Listener\SendNotificationWhenPostIsUnliked::class) diff --git a/extensions/likes/tests/integration/api/ListPostsTest.php b/extensions/likes/tests/integration/api/ListPostsTest.php index 31639daedd..6b853f2694 100644 --- a/extensions/likes/tests/integration/api/ListPostsTest.php +++ b/extensions/likes/tests/integration/api/ListPostsTest.php @@ -173,9 +173,10 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po { // Show discussion endpoint $response = $this->send( - $this->request('GET', '/api/discussions/100', [ + $this->request('GET', '/api/posts', [ 'authenticatedAs' => 2, ])->withQueryParams([ + 'filter' => ['discussion' => 100], 'include' => $include, ]) ); @@ -184,7 +185,7 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po $this->assertEquals(200, $response->getStatusCode(), $body); - $included = json_decode($body, true)['included'] ?? []; + $included = json_decode($body, true)['data'] ?? []; $likes = collect($included) ->where('type', 'posts') @@ -206,8 +207,7 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po public static function likesIncludeProvider(): array { return [ - ['posts,posts.likes'], - ['posts.likes'], + ['likes'], [null], ]; } diff --git a/extensions/mentions/extend.php b/extensions/mentions/extend.php index 7312c7312b..f5768bc612 100644 --- a/extensions/mentions/extend.php +++ b/extensions/mentions/extend.php @@ -81,15 +81,6 @@ 'lastPost.mentionsPosts.user', 'lastPost.mentionsPosts.discussion', 'lastPost.mentionsGroups', ], ]); - }) - ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Show { - return $endpoint->addDefaultInclude(['posts.mentionedBy', 'posts.mentionedBy.user', 'posts.mentionedBy.discussion']) - ->eagerLoadWhenIncluded([ - 'posts' => [ - 'posts.mentionsUsers', 'posts.mentionsPosts', 'posts.mentionsPosts.user', - 'posts.mentionsPosts.discussion', 'posts.mentionsGroups' - ], - ]); }), (new Extend\ApiResource(Resource\UserResource::class)) @@ -128,9 +119,6 @@ ]), (new Extend\ApiResource(Resource\DiscussionResource::class)) - ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Show { - return $endpoint->eagerLoadWhenIncluded(['posts' => ['posts.mentionsTags']]); - }) ->endpoint(Endpoint\Index::class, function (Endpoint\Index $endpoint): Endpoint\Index { return $endpoint->eagerLoadWhenIncluded(['firstPost' => ['firstPost.mentionsTags'], 'lastPost' => ['lastPost.mentionsTags']]); }), diff --git a/extensions/mentions/tests/integration/api/ListPostsTest.php b/extensions/mentions/tests/integration/api/ListPostsTest.php index d45bfd892d..1a9670dbc6 100644 --- a/extensions/mentions/tests/integration/api/ListPostsTest.php +++ b/extensions/mentions/tests/integration/api/ListPostsTest.php @@ -207,14 +207,15 @@ public function mentioned_by_relation_returns_limited_results_and_shows_only_vis // Show discussion endpoint $response = $this->send( - $this->request('GET', '/api/discussions/100', [ + $this->request('GET', '/api/posts', [ 'authenticatedAs' => 2, ])->withQueryParams([ + 'filter' => ['discussion' => 100], 'include' => $include, ]) ); - $included = json_decode($body = $response->getBody()->getContents(), true)['included'] ?? []; + $included = json_decode($body = $response->getBody()->getContents(), true)['data'] ?? []; $this->assertEquals(200, $response->getStatusCode(), $body); @@ -233,8 +234,7 @@ public function mentioned_by_relation_returns_limited_results_and_shows_only_vis public static function mentionedByIncludeProvider(): array { return [ - ['posts,posts.mentionedBy'], - ['posts.mentionedBy'], + ['mentionedBy'], [null], ]; } diff --git a/extensions/tags/extend.php b/extensions/tags/extend.php index b656961dfb..f256721f3d 100644 --- a/extensions/tags/extend.php +++ b/extensions/tags/extend.php @@ -178,10 +178,4 @@ function (Endpoint\Index|Endpoint\Show|Endpoint\Create $endpoint) { return $endpoint ->addDefaultInclude(['eventPostMentionsTags']); }), - - (new Extend\ApiResource(Resource\DiscussionResource::class)) - ->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint) { - return $endpoint - ->addDefaultInclude(['posts.eventPostMentionsTags']); - }), ]; diff --git a/framework/core/src/Api/Resource/DiscussionResource.php b/framework/core/src/Api/Resource/DiscussionResource.php index cc6ba1580f..f61780fad4 100644 --- a/framework/core/src/Api/Resource/DiscussionResource.php +++ b/framework/core/src/Api/Resource/DiscussionResource.php @@ -75,7 +75,6 @@ public function endpoints(): array ->authenticated() ->can('startDiscussion') ->defaultInclude([ - 'posts', 'user', 'lastPostedUser', 'firstPost', @@ -89,12 +88,14 @@ public function endpoints(): array Endpoint\Show::make() ->defaultInclude([ 'user', - 'posts', - 'posts.discussion', - 'posts.user', - 'posts.user.groups', - 'posts.editedUser', - 'posts.hiddenUser' + 'lastPostedUser', + 'firstPost', + 'firstPost.discussion', + 'firstPost.user', + 'firstPost.user.groups', + 'firstPost.editedUser', + 'firstPost.hiddenUser', + 'lastPost' ]), Endpoint\Index::make() ->defaultInclude([ @@ -206,54 +207,14 @@ public function fields(): array ->type('posts'), Schema\Relationship\ToMany::make('posts') ->withLinkage(function (Context $context) { - return $context->showing(self::class); + return $context->showing(self::class) + || $context->creating(self::class) + || $context->creating(PostResource::class); }) - ->includable() - // @todo: remove this, and send a second request from the frontend to /posts instead. Revert Serializer::addIncluded while you're at it. ->get(function (Discussion $discussion, Context $context) { - $showingDiscussion = $context->showing(self::class); - - if (! $showingDiscussion) { - return fn () => $discussion->posts->all(); - } - - /** @var Endpoint\Show $endpoint */ - $endpoint = $context->endpoint; - - $actor = $context->getActor(); - - $limit = PostResource::$defaultLimit; - - if (($near = Arr::get($context->request->getQueryParams(), 'page.near')) > 1) { - $offset = $this->posts->getIndexForNumber($discussion->id, $near, $actor); - $offset = max(0, $offset - $limit / 2); - } else { - $offset = $endpoint->extractOffsetValue($context, $endpoint->defaultExtracts($context)); - } - - /** @var Endpoint\Endpoint $endpoint */ - $endpoint = $context->endpoint; - - $posts = $discussion->posts() - ->whereVisibleTo($actor) - ->with($endpoint->getEagerLoadsFor('posts', $context)) - ->with($endpoint->getWhereEagerLoadsFor('posts', $context)) - ->orderBy('number') - ->skip($offset) - ->take($limit) - ->get(); - - /** @var Post $post */ - foreach ($posts as $post) { - $post->setRelation('discussion', $discussion); - } - - $allPosts = $discussion->posts()->whereVisibleTo($actor)->orderBy('number')->pluck('id')->all(); - $loadedPosts = $posts->all(); - - array_splice($allPosts, $offset, $limit, $loadedPosts); - - return $allPosts; + // @todo: is it possible to refactor the frontend to not need all post IDs? + // some kind of percentage-based stream scrubber? + return $discussion->posts()->whereVisibleTo($context->getActor())->select('id')->get()->all(); }), Schema\Relationship\ToOne::make('mostRelevantPost') ->visible(fn (Discussion $model, Context $context) => $context->listing()) diff --git a/framework/core/src/Api/Resource/PostResource.php b/framework/core/src/Api/Resource/PostResource.php index 988bd21652..824162b070 100644 --- a/framework/core/src/Api/Resource/PostResource.php +++ b/framework/core/src/Api/Resource/PostResource.php @@ -101,7 +101,6 @@ public function endpoints(): array ->defaultInclude([ 'user', 'discussion', - 'discussion.posts', 'discussion.lastPostedUser' ]), Endpoint\Update::make() diff --git a/framework/core/src/Api/Serializer.php b/framework/core/src/Api/Serializer.php index 96308d9ae4..7f12f5a4ff 100644 --- a/framework/core/src/Api/Serializer.php +++ b/framework/core/src/Api/Serializer.php @@ -146,24 +146,17 @@ private function whenResolved($value, $callback, bool $prepend = false): void */ public function addIncluded(Relationship $field, $model, ?array $include): array { - if (is_object($model)) { - $relatedResource = $this->resourceForModel($field, $model); - - if ($include === null) { - return [ - 'type' => $relatedResource->type(), - 'id' => $relatedResource->getId($model, $this->context), - ]; - } + $relatedResource = $this->resourceForModel($field, $model); - $data = $this->addToMap($relatedResource, $model, $include); - } else { - $data = [ - 'type' => $field->collections[0], - 'id' => (string) $model, + if ($include === null) { + return [ + 'type' => $relatedResource->type(), + 'id' => $relatedResource->getId($model, $this->context), ]; } + $data = $this->addToMap($relatedResource, $model, $include); + return [ 'type' => $data['type'], 'id' => $data['id'], @@ -181,7 +174,7 @@ private function resourceForModel(Relationship $field, $model): Resource } throw new RuntimeException( - 'No resource type defined to represent model '.get_class($model), + 'No resource type defined to represent model ' . get_class($model), ); } diff --git a/framework/core/src/Forum/Content/Discussion.php b/framework/core/src/Forum/Content/Discussion.php index b35b95125c..3214043d1f 100644 --- a/framework/core/src/Forum/Content/Discussion.php +++ b/framework/core/src/Forum/Content/Discussion.php @@ -33,15 +33,7 @@ public function __invoke(Document $document, Request $request): Document $near = intval(Arr::get($queryParams, 'near')); $page = max(1, intval(Arr::get($queryParams, 'page')), 1 + intdiv($near, 20)); - $params = [ - 'page' => [ - 'near' => $near, - 'offset' => ($page - 1) * 20, - 'limit' => 20 - ] - ]; - - $apiDocument = $this->getApiDocument($request, $id, $params); + $apiDocument = $this->getApiDocument($request, $id); $getResource = function ($link) use ($apiDocument) { return Arr::first($apiDocument->included, function ($value) use ($link) { @@ -64,9 +56,21 @@ public function __invoke(Document $document, Request $request): Document ($queryString ? '?'.$queryString : ''); }; + $params = [ + 'filter' => [ + 'discussion' => intval($id), + ], + 'page' => [ + 'near' => $near, + 'offset' => ($page - 1) * 20, + 'limit' => 20, + ], + ]; + + $postsApiDocument = $this->getPostsApiDocument($request, $params); $posts = []; - foreach ($apiDocument->included as $resource) { + foreach ($postsApiDocument->data as $resource) { if ($resource->type === 'posts' && isset($resource->relationships->discussion) && isset($resource->attributes->contentHtml)) { $posts[] = $resource; } @@ -77,6 +81,15 @@ public function __invoke(Document $document, Request $request): Document $document->title = $apiDocument->data->attributes->title; $document->content = $this->view->make('flarum.forum::frontend.content.discussion', compact('apiDocument', 'page', 'hasPrevPage', 'hasNextPage', 'getResource', 'posts', 'url')); + + $apiDocument->included = array_values(array_filter($apiDocument->included, function ($value) { + return $value->type !== 'posts'; + })); + $apiDocument->included = array_merge($apiDocument->included, $postsApiDocument->data, $postsApiDocument->included); + $apiDocument->included = array_values(array_filter($apiDocument->included, function ($value) use ($apiDocument) { + return $value->type !== 'discussions' || $value->id !== $apiDocument->data->id; + })); + $document->payload['apiDocument'] = $apiDocument; $document->canonicalUrl = $url([]); @@ -91,7 +104,7 @@ public function __invoke(Document $document, Request $request): Document * * @throws RouteNotFoundException */ - protected function getApiDocument(Request $request, string $id, array $params): object + protected function getApiDocument(Request $request, string $id, array $params = []): object { $params['bySlug'] = true; @@ -104,4 +117,21 @@ protected function getApiDocument(Request $request, string $id, array $params): ->getBody() ); } + + /** + * Get the result of an API request to list the posts of a discussion. + * + * @throws RouteNotFoundException + */ + protected function getPostsApiDocument(Request $request, array $params): object + { + return json_decode( + $this->api + ->withoutErrorHandling() + ->withParentRequest($request) + ->withQueryParams($params) + ->get("/posts") + ->getBody() + ); + } } diff --git a/framework/core/tests/integration/api/discussions/ShowTest.php b/framework/core/tests/integration/api/discussions/ShowTest.php index fbda8a44b3..305a26b22d 100644 --- a/framework/core/tests/integration/api/discussions/ShowTest.php +++ b/framework/core/tests/integration/api/discussions/ShowTest.php @@ -88,26 +88,31 @@ public function guest_cannot_see_empty_discussion() public function guest_cannot_see_hidden_posts() { $response = $this->send( - $this->request('GET', '/api/discussions/4') + $this->request('GET', '/api/posts') + ->withQueryParams([ + 'filter' => ['discussion' => 4] + ]) ); $json = json_decode($response->getBody()->getContents(), true); - $this->assertEmpty(Arr::get($json, 'data.relationships.posts.data')); + $this->assertEmpty(Arr::get($json, 'data')); } #[Test] public function author_can_see_hidden_posts() { $response = $this->send( - $this->request('GET', '/api/discussions/4', [ + $this->request('GET', '/api/posts', [ 'authenticatedAs' => 2, + ])->withQueryParams([ + 'filter' => ['discussion' => 4] ]) ); $json = json_decode($response->getBody()->getContents(), true); - $this->assertEquals(2, Arr::get($json, 'data.relationships.posts.data.0.id'), $response->getBody()->getContents()); + $this->assertEquals(2, Arr::get($json, 'data.0.id'), $response->getBody()->getContents()); } #[Test] diff --git a/framework/core/tests/integration/extenders/ModelVisibilityTest.php b/framework/core/tests/integration/extenders/ModelVisibilityTest.php index 333c4c42d6..50baf9a2a9 100644 --- a/framework/core/tests/integration/extenders/ModelVisibilityTest.php +++ b/framework/core/tests/integration/extenders/ModelVisibilityTest.php @@ -59,12 +59,15 @@ public function when_allowed_guests_can_see_hidden_posts() ); $response = $this->send( - $this->request('GET', '/api/discussions/2') + $this->request('GET', '/api/posts') + ->withQueryParams([ + 'filter' => ['discussion' => 2] + ]) ); $json = json_decode($response->getBody()->getContents(), true); - $this->assertEquals(1, Arr::get($json, 'data.relationships.posts.data.0.id')); + $this->assertEquals(1, Arr::get($json, 'data.0.id')); } #[Test]