From f6cd055dbee736b67c6ac106b252fe9d21235d39 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Mon, 26 Feb 2024 13:47:47 +0100 Subject: [PATCH] fix: regressions --- .../Concerns/ExtractsListingParams.php | 2 +- .../src/Api/Resource/DiscussionResource.php | 66 ++++++++++--------- .../src/Api/Resource/NotificationResource.php | 3 +- .../core/src/Api/Resource/PostResource.php | 4 +- framework/core/src/Http/RequestUtil.php | 14 +++- .../src/Search/Database/AbstractSearcher.php | 4 +- .../integration/api/discussions/ShowTest.php | 8 +-- .../extenders/ApiControllerTest.php | 4 +- 8 files changed, 61 insertions(+), 44 deletions(-) diff --git a/framework/core/src/Api/Endpoint/Concerns/ExtractsListingParams.php b/framework/core/src/Api/Endpoint/Concerns/ExtractsListingParams.php index 48f6a2d01..63b14e527 100644 --- a/framework/core/src/Api/Endpoint/Concerns/ExtractsListingParams.php +++ b/framework/core/src/Api/Endpoint/Concerns/ExtractsListingParams.php @@ -90,7 +90,7 @@ trait ExtractsListingParams return [ 'filter' => RequestUtil::extractFilter($context->request), 'sort' => RequestUtil::extractSort($context->request, $this->defaultSort, $this->getAvailableSorts($context)), - 'limit' => $limit = (RequestUtil::extractLimit($context->request, $this->limit, $this->maxLimit) ?? -1), + 'limit' => $limit = (RequestUtil::extractLimit($context->request, $this->limit, $this->maxLimit) ?? null), 'offset' => RequestUtil::extractOffset($context->request, $limit), ]; } diff --git a/framework/core/src/Api/Resource/DiscussionResource.php b/framework/core/src/Api/Resource/DiscussionResource.php index 3f7218347..c69314542 100644 --- a/framework/core/src/Api/Resource/DiscussionResource.php +++ b/framework/core/src/Api/Resource/DiscussionResource.php @@ -193,42 +193,46 @@ class DiscussionResource extends AbstractDatabaseResource ->includable() ->type('posts'), Schema\Relationship\ToMany::make('posts') - ->withLinkage() + ->withLinkage(function (Context $context) { + return $context->collection instanceof self && $context->endpoint instanceof Endpoint\Show; + }) ->includable() ->get(function (Discussion $discussion, Context $context) { - if ($context->collection instanceof self && $context->endpoint instanceof Endpoint\Show) { - $actor = $context->getActor(); + $showingDiscussion = $context->collection instanceof self && $context->endpoint instanceof Endpoint\Show; - $limit = $context->endpoint->extractLimitValue($context, $context->endpoint->defaultExtracts($context)); - - 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 = $context->endpoint->extractOffsetValue($context, $context->endpoint->defaultExtracts($context)); - } - - $posts = $discussion->posts() - ->whereVisibleTo($actor) - ->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; + if (! $showingDiscussion) { + return fn () => $discussion->posts->all(); } - return []; + $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 = $context->endpoint->extractOffsetValue($context, $context->endpoint->defaultExtracts($context)); + } + + $posts = $discussion->posts() + ->whereVisibleTo($actor) + ->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; }), Schema\Relationship\ToOne::make('mostRelevantPost') ->visible(fn (Discussion $model, Context $context) => $context->endpoint instanceof Endpoint\Index) diff --git a/framework/core/src/Api/Resource/NotificationResource.php b/framework/core/src/Api/Resource/NotificationResource.php index cfa8a66fc..9ea5731d9 100644 --- a/framework/core/src/Api/Resource/NotificationResource.php +++ b/framework/core/src/Api/Resource/NotificationResource.php @@ -10,6 +10,7 @@ use Flarum\Bus\Dispatcher; use Flarum\Notification\Command\ReadNotification; use Flarum\Notification\Notification; use Flarum\Notification\NotificationRepository; +use Illuminate\Database\Eloquent\Builder; use Tobyz\JsonApiServer\Pagination\Pagination; class NotificationResource extends AbstractDatabaseResource @@ -32,7 +33,7 @@ class NotificationResource extends AbstractDatabaseResource public function query(\Tobyz\JsonApiServer\Context $context): object { - if ($context->endpoint instanceof Endpoint\Index) { + if ($context->collection instanceof self && $context->endpoint instanceof Endpoint\Index) { /** @var Pagination $pagination */ $pagination = ($context->endpoint->paginationResolver)($context); diff --git a/framework/core/src/Api/Resource/PostResource.php b/framework/core/src/Api/Resource/PostResource.php index f9f97b771..b196ad932 100644 --- a/framework/core/src/Api/Resource/PostResource.php +++ b/framework/core/src/Api/Resource/PostResource.php @@ -23,6 +23,8 @@ use Tobyz\JsonApiServer\Exception\BadRequestException; class PostResource extends AbstractDatabaseResource { + public static int $defaultLimit = 20; + public function __construct( protected PostRepository $posts, protected TranslatorInterface $translator, @@ -137,7 +139,7 @@ class PostResource extends AbstractDatabaseResource 'hiddenUser', 'discussion' ]) - ->paginate(), + ->paginate(static::$defaultLimit), ]; } diff --git a/framework/core/src/Http/RequestUtil.php b/framework/core/src/Http/RequestUtil.php index a3acc0857..1e5f03e42 100644 --- a/framework/core/src/Http/RequestUtil.php +++ b/framework/core/src/Http/RequestUtil.php @@ -37,7 +37,13 @@ class RequestUtil public static function extractSort(Request $request, ?string $default, array $available = []): ?array { - if (! ($input = $request->getQueryParams()['sort'] ?? $default)) { + $input = $request->getQueryParams()['sort'] ?? null; + + if (is_null($input) || ! filled($input)) { + $input = $default; + } + + if (! $input) { return null; } @@ -71,7 +77,11 @@ class RequestUtil public static function extractLimit(Request $request, ?int $defaultLimit = null, ?int $max = null): ?int { - $limit = (int) ($request->getQueryParams()['page']['limit'] ?? $defaultLimit); + $limit = $request->getQueryParams()['page']['limit'] ?? ''; + + if (is_null($limit) || ! filled($limit)) { + $limit = $defaultLimit; + } if (! $limit) { return null; diff --git a/framework/core/src/Search/Database/AbstractSearcher.php b/framework/core/src/Search/Database/AbstractSearcher.php index dae1ada88..59de0166e 100644 --- a/framework/core/src/Search/Database/AbstractSearcher.php +++ b/framework/core/src/Search/Database/AbstractSearcher.php @@ -39,7 +39,7 @@ abstract class AbstractSearcher implements SearcherInterface $this->applySort($search, $criteria->sort, $criteria->sortIsDefault); $this->applyOffset($search, $criteria->offset); - $this->applyLimit($search, $criteria->limit + 1); + $this->applyLimit($search, $criteria->limit ? $criteria->limit + 1 : null); foreach ($this->mutators as $mutator) { $mutator($search, $criteria); @@ -102,7 +102,7 @@ abstract class AbstractSearcher implements SearcherInterface protected function applyLimit(DatabaseSearchState $state, ?int $limit): void { - if ($limit > 0) { + if ($limit && $limit > 0) { $state->getQuery()->take($limit); } } diff --git a/framework/core/tests/integration/api/discussions/ShowTest.php b/framework/core/tests/integration/api/discussions/ShowTest.php index acdfccaac..a81bcf406 100644 --- a/framework/core/tests/integration/api/discussions/ShowTest.php +++ b/framework/core/tests/integration/api/discussions/ShowTest.php @@ -53,7 +53,7 @@ class ShowTest extends TestCase ]) ); - $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents()); } /** @@ -71,7 +71,7 @@ class ShowTest extends TestCase ]) ); - $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents()); } /** @@ -113,7 +113,7 @@ class ShowTest extends TestCase $json = json_decode($response->getBody()->getContents(), true); - $this->assertEquals(2, Arr::get($json, 'data.relationships.posts.data.0.id')); + $this->assertEquals(2, Arr::get($json, 'data.relationships.posts.data.0.id'), $response->getBody()->getContents()); } /** @@ -125,7 +125,7 @@ class ShowTest extends TestCase $this->request('GET', '/api/discussions/2') ); - $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents()); } /** diff --git a/framework/core/tests/integration/extenders/ApiControllerTest.php b/framework/core/tests/integration/extenders/ApiControllerTest.php index 3d665ed0a..924fb8c2d 100644 --- a/framework/core/tests/integration/extenders/ApiControllerTest.php +++ b/framework/core/tests/integration/extenders/ApiControllerTest.php @@ -78,9 +78,9 @@ class ApiControllerTest extends TestCase ]) ); - $payload = json_decode($response->getBody()->getContents(), true); + $payload = json_decode($body = $response->getBody()->getContents(), true); - $this->assertEquals('dataSerializationPrepCustomTitle', $payload['data']['attributes']['title']); + $this->assertEquals('dataSerializationPrepCustomTitle', $payload['data']['attributes']['title'], $body); } /**