fix: eliminate N+1 group_user queries when serializing users

imorland · imorland · commit 79810f63fdb1 · 2026-06-07T22:38:58.000+01:00
The UserResource `groups` relationship getter issued a fresh `group_user` query for every serialized user, so the query count grew linearly with the number of users in a payload (e.g. one per post author on the posts endpoint). The getter now reads the (eager-)loaded `groups` relation and filters hidden groups in PHP, and the relevant endpoints eager-load `groups` so it is batched into a single query. This also shares the relation that `User::isAdmin()` reads (via the `email` field's `editCredentials` check), removing the redundant per-user load, and the email/isEmailConfirmed visibility checks now short-circuit on the cheap self comparison before invoking the policy. Fixes #4695
diff --git a/framework/core/src/Api/Resource/DiscussionResource.php b/framework/core/src/Api/Resource/DiscussionResource.php
@@ -94,7 +94,8 @@ public function endpoints(): array
                     'firstPost.editedUser',
                     'firstPost.hiddenUser',
                     'lastPost'
-                ]),
+                ])
+                ->eagerLoad(['state', 'user.groups', 'lastPostedUser.groups', 'firstPost.user.groups']),
             Endpoint\Index::make()
                 ->defaultInclude([
                     'user',
diff --git a/framework/core/src/Api/Resource/PostResource.php b/framework/core/src/Api/Resource/PostResource.php
@@ -119,7 +119,8 @@ public function endpoints(): array
                     'editedUser',
                     'hiddenUser',
                     'discussion'
-                ]),
+                ])
+                ->eagerLoad(['user.groups']),
             Endpoint\Index::make()
                 ->extractOffset(function (Context $context, array $defaultExtracts): int {
                     $queryParams = $context->request->getQueryParams();
@@ -150,6 +151,7 @@ public function endpoints(): array
                     'hiddenUser',
                     'discussion'
                 ])
+                ->eagerLoad(['user.groups'])
                 ->defaultSort('number')
                 ->paginate(static::$defaultLimit),
         ];
diff --git a/framework/core/src/Api/Resource/UserResource.php b/framework/core/src/Api/Resource/UserResource.php
@@ -15,6 +15,7 @@
 use Flarum\Api\Sort\SortColumn;
 use Flarum\Bus\Dispatcher;
 use Flarum\Foundation\ValidationException;
+use Flarum\Group\Group;
 use Flarum\Http\SlugManager;
 use Flarum\Locale\TranslatorInterface;
 use Flarum\Settings\SettingsRepositoryInterface;
@@ -111,15 +112,18 @@ public function endpoints(): array
 
                     return true;
                 })
-                ->defaultInclude(['groups']),
+                ->defaultInclude(['groups'])
+                ->eagerLoad(['groups']),
             Endpoint\Delete::make()
                 ->authenticated()
                 ->can('delete'),
             Endpoint\Show::make()
-                ->defaultInclude(['groups']),
+                ->defaultInclude(['groups'])
+                ->eagerLoad(['groups']),
             Endpoint\Index::make()
                 ->can('searchUsers')
                 ->defaultInclude(['groups'])
+                ->eagerLoad(['groups'])
                 ->paginate(),
             Endpoint\Endpoint::make('avatar.upload')
                 ->route('POST', '/{id}/avatar')
@@ -174,13 +178,16 @@ public function fields(): array
                 ->email(['filter'])
                 ->unique('users', 'email', true)
                 ->visible(function (User $user, Context $context) {
-                    return $context->getActor()->can('editCredentials', $user)
-                        || $context->getActor()->id === $user->id;
+                    // Check the cheap self comparison before the editCredentials
+                    // policy, which would otherwise read $user->groups (isAdmin) for
+                    // every serialized user.
+                    return $context->getActor()->id === $user->id
+                        || $context->getActor()->can('editCredentials', $user);
                 })
                 ->writable(function (User $user, Context $context) {
                     return $context->creating()
-                        || $context->getActor()->can('editCredentials', $user)
-                        || $context->getActor()->id === $user->id;
+                        || $context->getActor()->id === $user->id
+                        || $context->getActor()->can('editCredentials', $user);
                 })
                 ->set(function (User $user, string $value, Context $context) {
                     if ($user->exists) {
@@ -198,8 +205,8 @@ public function fields(): array
                 }),
             Schema\Boolean::make('isEmailConfirmed')
                 ->visible(function (User $user, Context $context) {
-                    return $context->getActor()->can('editCredentials', $user)
-                        || $context->getActor()->id === $user->id;
+                    return $context->getActor()->id === $user->id
+                        || $context->getActor()->can('editCredentials', $user);
                 })
                 ->writable(fn (User $user, Context $context) => $context->getActor()->isAdmin())
                 ->set(function (User $user, $value, Context $context) {
@@ -311,9 +318,21 @@ public function fields(): array
 
             Schema\Relationship\ToMany::make('groups')
                 ->get(function (User $user, Context $context) {
-                    return $context->getActor()->can('viewHiddenGroups')
-                        ? $user->groups()->get()->all()
-                        : $user->visibleGroups()->get()->all();
+                    // Read the (eager-)loaded relation instead of issuing a fresh
+                    // query per serialized user. Hidden groups are filtered in PHP
+                    // for actors that cannot view them, so the relation can be
+                    // eager-loaded once for the whole payload (see the endpoints'
+                    // eagerLoad of `groups`). Note: isAdmin() also reads $user->groups,
+                    // so the relation must contain all groups (filtering happens here).
+                    $groups = $user->groups;
+
+                    if (! $context->getActor()->can('viewHiddenGroups')) {
+                        // values() re-indexes after filtering so the result stays a
+                        // sequential list (JSON array) rather than a keyed object.
+                        $groups = $groups->filter(fn (Group $group) => ! $group->is_hidden)->values();
+                    }
+
+                    return $groups->all();
                 })
                 ->writable(fn (User $user, Context $context) => $context->updating() && $context->getActor()->can('editGroups', $user))
                 ->includable()
diff --git a/framework/core/tests/integration/api/discussions/ShowGroupsQueryCountTest.php b/framework/core/tests/integration/api/discussions/ShowGroupsQueryCountTest.php
@@ -0,0 +1,105 @@
+<?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\Tests\integration\api\discussions;
+
+use Carbon\Carbon;
+use Flarum\Discussion\Discussion;
+use Flarum\Group\Group;
+use Flarum\Post\Post;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+use PHPUnit\Framework\Attributes\Test;
+
+/**
+ * Regression test for part (B) of https://github.com/flarum/framework/issues/4695:
+ * DiscussionResource::Show did not eager-load groups for the `user` and
+ * `lastPostedUser` it serialises, so the `email` field's
+ * `editCredentials`/`isAdmin()` check lazy-loaded each user's groups in its own
+ * `group_user` query.
+ */
+class ShowGroupsQueryCountTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->prepareDatabase([
+            Discussion::class => [
+                ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 20, 'last_posted_user_id' => 21, 'first_post_id' => 1, 'comment_count' => 1],
+            ],
+            Post::class => [
+                ['id' => 1, 'number' => 1, 'discussion_id' => 1, 'created_at' => Carbon::now(), 'user_id' => 22, 'type' => 'comment', 'content' => '<t><p>first post</p></t>'],
+            ],
+            User::class => [
+                $this->normalUser(),
+                ['id' => 20, 'username' => 'author', 'email' => 'author@machine.local', 'is_email_confirmed' => 1, 'password' => 'foobar'],
+                ['id' => 21, 'username' => 'lastposter', 'email' => 'lastposter@machine.local', 'is_email_confirmed' => 1, 'password' => 'foobar'],
+                ['id' => 22, 'username' => 'firstposter', 'email' => 'firstposter@machine.local', 'is_email_confirmed' => 1, 'password' => 'foobar'],
+            ],
+            Group::class => [
+                ['id' => 100, 'name_singular' => 'Visible', 'name_plural' => 'Visible', 'is_hidden' => 0],
+            ],
+            'group_user' => [
+                ['user_id' => 20, 'group_id' => 100],
+                ['user_id' => 21, 'group_id' => 100],
+                ['user_id' => 22, 'group_id' => 100],
+            ],
+        ]);
+    }
+
+    private function countGroupUserQueries(): int
+    {
+        $db = $this->database();
+        $db->flushQueryLog();
+        $db->enableQueryLog();
+
+        // Authenticate as the normal user (id 2): not the author, last poster or
+        // first poster, so the email field's editCredentials/isAdmin check runs
+        // against each of those users.
+        $response = $this->send(
+            $this->request('GET', '/api/discussions/1', ['authenticatedAs' => 2])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents());
+
+        $count = 0;
+        foreach ($db->getQueryLog() as $query) {
+            if (stripos($query['query'], 'group_user') !== false) {
+                $count++;
+            }
+        }
+
+        $db->disableQueryLog();
+
+        return $count;
+    }
+
+    #[Test]
+    public function user_groups_are_eager_loaded_on_discussion_show()
+    {
+        $this->app();
+
+        $count = $this->countGroupUserQueries();
+
+        // The discussion serialises three distinct users with groups (author,
+        // last poster, first-post author) plus the actor, so each user's groups
+        // are loaded exactly once: 3 + 1 = 4. Previously firstPost.user's groups
+        // were loaded twice – once by the relationship getter (visibleGroups) and
+        // again by the email field's isAdmin() check (groups) – for 5 queries.
+        $this->assertLessThanOrEqual(
+            4,
+            $count,
+            "Discussion show issued $count `group_user` queries; expected at most 4. The extra query is the redundant per-user groups load from issue #4695."
+        );
+    }
+}
diff --git a/framework/core/tests/integration/api/posts/ListGroupsQueryCountTest.php b/framework/core/tests/integration/api/posts/ListGroupsQueryCountTest.php
@@ -0,0 +1,167 @@
+<?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\Tests\integration\api\posts;
+
+use Carbon\Carbon;
+use Flarum\Discussion\Discussion;
+use Flarum\Group\Group;
+use Flarum\Post\Post;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+use PHPUnit\Framework\Attributes\Test;
+
+/**
+ * Regression test for the N+1 `group_user` queries described in
+ * https://github.com/flarum/framework/issues/4695.
+ *
+ * The posts index includes `user.groups`. Before the fix, the UserResource
+ * `groups` getter issued a fresh `group_user` query for every serialized post
+ * author, so the query count grew linearly with the number of distinct authors.
+ * After the fix the relation is eager-loaded once for the whole payload.
+ */
+class ListGroupsQueryCountTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    /**
+     * Number of distinct post authors. Deliberately large so that an N+1 would
+     * produce many more `group_user` queries than the single batched load.
+     */
+    private const AUTHOR_COUNT = 8;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $users = [];
+        $posts = [];
+        $groupUser = [];
+
+        for ($i = 0; $i < self::AUTHOR_COUNT; $i++) {
+            $userId = 10 + $i;
+            $users[] = [
+                'id' => $userId,
+                'username' => 'author'.$userId,
+                'email' => 'author'.$userId.'@machine.local',
+                'is_email_confirmed' => 1,
+                'password' => 'foobar',
+            ];
+            $posts[] = [
+                'id' => 100 + $i,
+                'number' => $i + 1,
+                'discussion_id' => 1,
+                'created_at' => Carbon::now(),
+                'user_id' => $userId,
+                'type' => 'comment',
+                'content' => '<t><p>post by '.$userId.'</p></t>',
+            ];
+            // Put every author in both a visible and a hidden group, so we also
+            // exercise hidden-group filtering across many distinct users.
+            $groupUser[] = ['user_id' => $userId, 'group_id' => 100];
+            $groupUser[] = ['user_id' => $userId, 'group_id' => 101];
+        }
+
+        $this->prepareDatabase([
+            Discussion::class => [
+                ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 10, 'first_post_id' => 100, 'comment_count' => self::AUTHOR_COUNT],
+            ],
+            Post::class => $posts,
+            User::class => array_merge([$this->normalUser()], $users),
+            Group::class => [
+                ['id' => 100, 'name_singular' => 'Visible', 'name_plural' => 'Visible', 'is_hidden' => 0],
+                ['id' => 101, 'name_singular' => 'Hidden', 'name_plural' => 'Hidden', 'is_hidden' => 1],
+            ],
+            'group_user' => $groupUser,
+        ]);
+    }
+
+    private function listPostsForDiscussion(): array
+    {
+        // Authenticate as the normal user (id 2), a non-admin who cannot view
+        // hidden groups – this exercises both the groups relationship getter and
+        // the email-field `editCredentials`/`isAdmin` path for every author.
+        $response = $this->send(
+            $this->request('GET', '/api/posts', ['authenticatedAs' => 2])
+                ->withQueryParams(['filter' => ['discussion' => 1]])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        return json_decode($response->getBody()->getContents(), true);
+    }
+
+    #[Test]
+    public function groups_are_batch_loaded_without_an_n_plus_one()
+    {
+        // Boot the app and populate the database before we start counting.
+        $this->app();
+
+        $db = $this->database();
+        $db->flushQueryLog();
+        $db->enableQueryLog();
+
+        $this->listPostsForDiscussion();
+
+        // Classify every `group_user` query. The post authors' groups must be
+        // loaded in one batched `where user_id in (...)` query – never one query
+        // per author (which was the N+1). Loading the actor's own groups for
+        // permission checks is constant overhead and unrelated.
+        $batchedAuthorLoads = 0;
+        $individualAuthorLoads = 0;
+
+        foreach ($db->getQueryLog() as $query) {
+            if (stripos($query['query'], 'group_user') === false) {
+                continue;
+            }
+
+            if (stripos($query['query'], ' in (') !== false) {
+                $batchedAuthorLoads++;
+                continue;
+            }
+
+            // A single-row `where user_id = ?` load – flag it if it targets one
+            // of the post authors (ids >= 10) rather than the actor.
+            foreach ($query['bindings'] as $binding) {
+                if ((int) $binding >= 10) {
+                    $individualAuthorLoads++;
+                }
+            }
+        }
+
+        $db->disableQueryLog();
+
+        $this->assertSame(
+            0,
+            $individualAuthorLoads,
+            "Post authors' groups were loaded individually ($individualAuthorLoads times) instead of in a single batched query – this is the N+1 from issue #4695."
+        );
+        $this->assertGreaterThanOrEqual(
+            1,
+            $batchedAuthorLoads,
+            'Expected the authors\' groups to be eager-loaded in a single batched query.'
+        );
+    }
+
+    #[Test]
+    public function hidden_groups_are_still_filtered_for_many_users()
+    {
+        $body = $this->listPostsForDiscussion();
+
+        $groupIds = array_values(array_unique(array_map(
+            fn (array $resource) => $resource['id'],
+            array_filter($body['included'] ?? [], fn (array $r) => ($r['type'] ?? null) === 'groups')
+        )));
+
+        // Group 101 is hidden and the actor (normal user, id 1) cannot view it.
+        $this->assertContains('100', $groupIds);
+        $this->assertNotContains('101', $groupIds);
+    }
+}
diff --git a/framework/core/tests/integration/api/users/ShowGroupsRelationshipTest.php b/framework/core/tests/integration/api/users/ShowGroupsRelationshipTest.php
