From e60bf67c610b68f12240b7fc3fc96754ef1f71a7 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Thu, 25 Mar 2021 15:36:39 +0100 Subject: [PATCH] Eager loading extender (#2724) * Eager loading extender * Add tests for the eager loading extender --- .../AbstractSerializeController.php | 57 +++++++ .../Controller/ListDiscussionsController.php | 4 +- .../Api/Controller/ListGroupsController.php | 6 +- .../ListNotificationsController.php | 8 +- .../Api/Controller/ListPostsController.php | 6 +- .../Api/Controller/ListUsersController.php | 6 +- framework/core/src/Extend/ApiController.php | 24 +++ .../extenders/ApiControllerTest.php | 151 ++++++++++++++++++ 8 files changed, 255 insertions(+), 7 deletions(-) diff --git a/framework/core/src/Api/Controller/AbstractSerializeController.php b/framework/core/src/Api/Controller/AbstractSerializeController.php index e9e05ba29..cdb20b030 100644 --- a/framework/core/src/Api/Controller/AbstractSerializeController.php +++ b/framework/core/src/Api/Controller/AbstractSerializeController.php @@ -11,6 +11,8 @@ namespace Flarum\Api\Controller; use Flarum\Api\JsonApiResponse; use Illuminate\Contracts\Container\Container; +use Illuminate\Database\Eloquent\Collection; +use Illuminate\Support\Str; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; @@ -84,6 +86,11 @@ abstract class AbstractSerializeController implements RequestHandlerInterface */ protected static $beforeSerializationCallbacks = []; + /** + * @var array + */ + protected static $loadRelations = []; + /** * {@inheritdoc} */ @@ -139,6 +146,47 @@ abstract class AbstractSerializeController implements RequestHandlerInterface */ abstract protected function createElement($data, SerializerInterface $serializer); + /** + * Eager loads the required relationships. + * + * @param Collection $models + * @param array $relations + * @return void + */ + protected function loadRelations(Collection $models, array $relations): void + { + $addedRelations = []; + + foreach (array_reverse(array_merge([static::class], class_parents($this))) as $class) { + if (isset(static::$loadRelations[$class])) { + $addedRelations = array_merge($addedRelations, static::$loadRelations[$class]); + } + } + + if (! empty($addedRelations)) { + usort($addedRelations, function ($a, $b) { + return substr_count($a, '.') - substr_count($b, '.'); + }); + + foreach ($addedRelations as $relation) { + if (strpos($relation, '.') !== false) { + $parentRelation = Str::beforeLast($relation, '.'); + + if (! in_array($parentRelation, $relations, true)) { + continue; + } + } + + $relations[] = $relation; + } + } + + if (! empty($relations)) { + $relations = array_unique($relations); + $models->loadMissing($relations); + } + } + /** * @param ServerRequestInterface $request * @return array @@ -348,4 +396,13 @@ abstract class AbstractSerializeController implements RequestHandlerInterface static::$beforeSerializationCallbacks[$controllerClass][] = $callback; } + + public static function setLoadRelations(string $controllerClass, array $relations) + { + if (! isset(static::$loadRelations[$controllerClass])) { + static::$loadRelations[$controllerClass] = []; + } + + static::$loadRelations[$controllerClass] = array_merge(static::$loadRelations[$controllerClass], $relations); + } } diff --git a/framework/core/src/Api/Controller/ListDiscussionsController.php b/framework/core/src/Api/Controller/ListDiscussionsController.php index 495b14357..d3e1e15e0 100644 --- a/framework/core/src/Api/Controller/ListDiscussionsController.php +++ b/framework/core/src/Api/Controller/ListDiscussionsController.php @@ -121,7 +121,9 @@ class ListDiscussionsController extends AbstractListController } } - $results = $results->getResults()->load($include); + $results = $results->getResults(); + + $this->loadRelations($results, $include); if ($relations = array_intersect($include, ['firstPost', 'lastPost', 'mostRelevantPost'])) { foreach ($results as $discussion) { diff --git a/framework/core/src/Api/Controller/ListGroupsController.php b/framework/core/src/Api/Controller/ListGroupsController.php index 098f703d9..f82dfbcae 100644 --- a/framework/core/src/Api/Controller/ListGroupsController.php +++ b/framework/core/src/Api/Controller/ListGroupsController.php @@ -28,6 +28,10 @@ class ListGroupsController extends AbstractListController { $actor = $request->getAttribute('actor'); - return Group::whereVisibleTo($actor)->get(); + $results = Group::whereVisibleTo($actor)->get(); + + $this->loadRelations($results, []); + + return $results; } } diff --git a/framework/core/src/Api/Controller/ListNotificationsController.php b/framework/core/src/Api/Controller/ListNotificationsController.php index 54a0649e0..6700959ac 100644 --- a/framework/core/src/Api/Controller/ListNotificationsController.php +++ b/framework/core/src/Api/Controller/ListNotificationsController.php @@ -76,9 +76,11 @@ class ListNotificationsController extends AbstractListController $include[] = 'subject'; } - $notifications = $this->notifications->findByUser($actor, $limit + 1, $offset) - ->load(array_diff($include, ['subject.discussion'])) - ->all(); + $notifications = $this->notifications->findByUser($actor, $limit + 1, $offset); + + $this->loadRelations($notifications, array_diff($include, ['subject.discussion'])); + + $notifications = $notifications->all(); $areMoreResults = false; diff --git a/framework/core/src/Api/Controller/ListPostsController.php b/framework/core/src/Api/Controller/ListPostsController.php index f034838d7..c7006d9dc 100644 --- a/framework/core/src/Api/Controller/ListPostsController.php +++ b/framework/core/src/Api/Controller/ListPostsController.php @@ -104,7 +104,11 @@ class ListPostsController extends AbstractListController $include[] = 'user.groups'; } - return $results->getResults()->load($include); + $results = $results->getResults(); + + $this->loadRelations($results, $include); + + return $results; } /** diff --git a/framework/core/src/Api/Controller/ListUsersController.php b/framework/core/src/Api/Controller/ListUsersController.php index bc31ef2b0..343a19b10 100644 --- a/framework/core/src/Api/Controller/ListUsersController.php +++ b/framework/core/src/Api/Controller/ListUsersController.php @@ -105,6 +105,10 @@ class ListUsersController extends AbstractListController $results->areMoreResults() ? null : 0 ); - return $results->getResults()->load($include); + $results = $results->getResults(); + + $this->loadRelations($results, $include); + + return $results; } } diff --git a/framework/core/src/Extend/ApiController.php b/framework/core/src/Extend/ApiController.php index 0551bb025..be7848879 100644 --- a/framework/core/src/Extend/ApiController.php +++ b/framework/core/src/Extend/ApiController.php @@ -29,6 +29,7 @@ class ApiController implements ExtenderInterface private $addSortFields = []; private $removeSortFields = []; private $sort; + private $load = []; /** * @param string $controllerClass The ::class attribute of the controller you are modifying. @@ -216,6 +217,27 @@ class ApiController implements ExtenderInterface return $this; } + /** + * Eager loads relationships needed for serializer logic. + * + * First level relationships will be loaded regardless of whether they are included in the response. + * Sublevel relationships will only be loaded if the upper level was included or manually loaded. + * + * @example If a relationship such as: 'relation.subRelation' is specified, + * it will only be loaded if 'relation' is or has been loaded. + * To force load the relationship, both levels have to be specified, + * example: ['relation', 'relation.subRelation']. + * + * @param string|array + * @return self + */ + public function load($relations) + { + $this->load = array_merge($this->load, (array) $relations); + + return $this; + } + public function extend(Container $container, Extension $extension = null) { $this->beforeDataCallbacks[] = function (AbstractSerializeController $controller) use ($container) { @@ -281,6 +303,8 @@ class ApiController implements ExtenderInterface $beforeSerializationCallback = ContainerUtil::wrapCallback($beforeSerializationCallback, $container); AbstractSerializeController::addSerializationPreparationCallback($this->controllerClass, $beforeSerializationCallback); } + + AbstractSerializeController::setLoadRelations($this->controllerClass, $this->load); } /** diff --git a/framework/core/tests/integration/extenders/ApiControllerTest.php b/framework/core/tests/integration/extenders/ApiControllerTest.php index 27323a70d..c23a7f667 100644 --- a/framework/core/tests/integration/extenders/ApiControllerTest.php +++ b/framework/core/tests/integration/extenders/ApiControllerTest.php @@ -12,6 +12,7 @@ namespace Flarum\Tests\integration\extenders; use Carbon\Carbon; use Flarum\Api\Controller\AbstractShowController; use Flarum\Api\Controller\ListDiscussionsController; +use Flarum\Api\Controller\ListUsersController; use Flarum\Api\Controller\ShowDiscussionController; use Flarum\Api\Controller\ShowForumController; use Flarum\Api\Controller\ShowPostController; @@ -22,6 +23,7 @@ use Flarum\Api\Serializer\PostSerializer; use Flarum\Api\Serializer\UserSerializer; use Flarum\Discussion\Discussion; use Flarum\Extend; +use Flarum\Post\Post; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Flarum\User\User; @@ -47,6 +49,11 @@ class ApiControllerTest extends TestCase ['id' => 2, 'title' => 'Custom Discussion Title', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 3, 'first_post_id' => 0, 'comment_count' => 1, 'is_private' => 0], ['id' => 3, 'title' => 'Custom Discussion Title', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'first_post_id' => 0, 'comment_count' => 1, 'is_private' => 0], ], + 'posts' => [ + ['id' => 1, 'discussion_id' => 3, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'discussionRenamed', 'content' => '

can i haz relationz?

'], + ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'discussionRenamed', 'content' => '

can i haz relationz?

'], + ['id' => 3, 'discussion_id' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 1, 'type' => 'discussionRenamed', 'content' => '

can i haz relationz?

'], + ], ]); } @@ -652,6 +659,150 @@ class ApiControllerTest extends TestCase $this->assertEquals(200, $response->getStatusCode()); $this->assertEquals([2, 1, 3], Arr::pluck($payload['data'], 'id')); } + + /** + * @test + */ + public function custom_first_level_relation_is_not_loaded_by_default() + { + $users = null; + + $this->extend( + (new Extend\Model(User::class)) + ->hasOne('firstLevelRelation', Post::class, 'user_id'), + (new Extend\ApiController(ListUsersController::class)) + ->prepareDataForSerialization(function ($controller, $data) use (&$users) { + $users = $data; + + return []; + }) + ); + + $this->send( + $this->request('GET', '/api/users', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertTrue($users->filter->relationLoaded('firstLevelRelation')->isEmpty()); + } + + /** + * @test + */ + public function custom_first_level_relation_is_loaded_if_added() + { + $users = null; + + $this->extend( + (new Extend\Model(User::class)) + ->hasOne('firstLevelRelation', Post::class, 'user_id'), + (new Extend\ApiController(ListUsersController::class)) + ->load('firstLevelRelation') + ->prepareDataForSerialization(function ($controller, $data) use (&$users) { + $users = $data; + + return []; + }) + ); + + $this->send( + $this->request('GET', '/api/users', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertFalse($users->filter->relationLoaded('firstLevelRelation')->isEmpty()); + } + + /** + * @test + */ + public function custom_second_level_relation_is_not_loaded_by_default() + { + $users = null; + + $this->extend( + (new Extend\Model(User::class)) + ->hasOne('firstLevelRelation', Post::class, 'user_id'), + (new Extend\Model(Post::class)) + ->belongsTo('secondLevelRelation', Discussion::class), + (new Extend\ApiController(ListUsersController::class)) + ->prepareDataForSerialization(function ($controller, $data) use (&$users) { + $users = $data; + + return []; + }) + ); + + $this->send( + $this->request('GET', '/api/users', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertTrue($users->pluck('firstLevelRelation')->filter->relationLoaded('secondLevelRelation')->isEmpty()); + } + + /** + * @test + */ + public function custom_second_level_relation_is_loaded_if_added() + { + $users = null; + + $this->extend( + (new Extend\Model(User::class)) + ->hasOne('firstLevelRelation', Post::class, 'user_id'), + (new Extend\Model(Post::class)) + ->belongsTo('secondLevelRelation', Discussion::class), + (new Extend\ApiController(ListUsersController::class)) + ->load(['firstLevelRelation', 'firstLevelRelation.secondLevelRelation']) + ->prepareDataForSerialization(function ($controller, $data) use (&$users) { + $users = $data; + + return []; + }) + ); + + $this->send( + $this->request('GET', '/api/users', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertFalse($users->pluck('firstLevelRelation')->filter->relationLoaded('secondLevelRelation')->isEmpty()); + } + + /** + * @test + */ + public function custom_second_level_relation_is_not_loaded_when_first_level_is_not() + { + $users = null; + + $this->extend( + (new Extend\Model(User::class)) + ->hasOne('firstLevelRelation', Post::class, 'user_id'), + (new Extend\Model(Post::class)) + ->belongsTo('secondLevelRelation', Discussion::class), + (new Extend\ApiController(ListUsersController::class)) + ->load(['firstLevelRelation.secondLevelRelation']) + ->prepareDataForSerialization(function ($controller, $data) use (&$users) { + $users = $data; + + return []; + }) + ); + + $this->send( + $this->request('GET', '/api/users', [ + 'authenticatedAs' => 1, + ]) + ); + + $this->assertTrue($users->pluck('firstLevelRelation')->filter->relationLoaded('secondLevelRelation')->isEmpty()); + } } class CustomDiscussionSerializer extends DiscussionSerializer