refactor: remove listing of posts in the show discussion endpoint (#4067

)
flarum · Oct 16, 2024 · b0e8f5c · b0e8f5c
1 parent 40996de
commit b0e8f5c
Show file tree

Hide file tree

Showing 12 changed files with 84 additions and 121 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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'],

diff --git 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
@@ -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
@@ -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]