perf: materialize post visibility once; flat grouped relation aggregates (#4866)

imorland · web-flow · commit c50c9ec349ee · 2026-07-31T11:54:33.000+01:00
Two structural fixes to how post visibility interacts with the query
shapes the API layer generates:

ScopePostVisibility embedded its discussion-level checks as per-row
correlated EXISTS subqueries. Many posts share few discussions, so the
compounded discussion visibility conditions were re-evaluated for every
candidate row — on a post mentioned 1.4k times, 1,403 evaluations to
check 2 discussions. Both checks are now uncorrelated IN-subqueries the
database can materialize once per query. Extension scopers compose
exactly as before: their logic lives inside Discussion::whereVisibleTo,
which is unchanged.

EloquentBuffer loaded relation aggregates (countRelation etc. — used by
mentions, likes and messages) through Laravel's loadAggregate, which
emits a correlated scalar subquery per model and forces the aggregate's
constraints — typically the full visibility scope — to re-run per
related row PER MODEL, defeating the materialization above. Aggregates
for all buffered models are now computed by ONE flat grouped query
built from the relation's own eager constraints, with loadAggregate as
the fallback for relation types without well-known key semantics.

The mentionedByCount batch on a post mentioned 1,403 times drops from
5.5ms to 1.75ms; the discussion-level visibility semantics are pinned
by new characterization tests green on both implementations.
diff --git a/framework/core/src/Api/Resource/EloquentBuffer.php b/framework/core/src/Api/Resource/EloquentBuffer.php
@@ -14,6 +14,8 @@
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Database\Eloquent\Model;
+use Illuminate\Database\Eloquent\Relations\BelongsToMany;
+use Illuminate\Database\Eloquent\Relations\HasOneOrMany;
 use Illuminate\Database\Eloquent\Relations\MorphTo;
 use Illuminate\Database\Eloquent\Relations\Relation;
 use Illuminate\Support\Str;
@@ -148,8 +150,77 @@ public static function load(
                 }
             });
         } else {
-            $alias = Str::snake($aggregate['name']);
+            self::loadAggregate($collection, $relationName, $aggregate, $loader);
+        }
+    }
+
+    /**
+     * Load a relation aggregate for all buffered models with ONE flat grouped
+     * query.
+     *
+     * Laravel's loadAggregate() emits a correlated scalar subquery per model
+     * (`select id, (select count(*) ... where outer.id = ...)`), which forces
+     * the database to re-evaluate the aggregate's constraints — for API
+     * resources, typically the full visibility scope — once per related row
+     * PER MODEL, with no chance to materialize shared subqueries. A grouped
+     * join over the relation's eager constraints computes every model's
+     * aggregate in a single flat pass instead.
+     *
+     * @param array{name: string, relation: string, column: string, function: string, constrain: callable|null} $aggregate
+     */
+    protected static function loadAggregate(Collection $collection, string $relationName, array $aggregate, callable $loader): void
+    {
+        $alias = Str::snake($aggregate['name']);
+
+        /** @var Model $parent */
+        $parent = $collection->first();
+
+        /** @var Relation $relation */
+        $relation = Relation::noConstraints(fn () => $parent->newModelInstance()->{$relationName}());
+
+        // The key the related rows are grouped and matched back to parents
+        // by, exactly as eager loading would match them.
+        [$groupColumn, $parentKeyName] = match (true) {
+            $relation instanceof BelongsToMany => [$relation->getQualifiedForeignPivotKeyName(), $relation->getParentKeyName()],
+            $relation instanceof HasOneOrMany => [$relation->getQualifiedForeignKeyName(), $relation->getLocalKeyName()],
+            default => [null, null],
+        };
+
+        if ($groupColumn === null) {
+            // Unsupported relation type: fall back to Laravel's per-model
+            // correlated subqueries rather than guessing at key semantics.
             $collection->loadAggregate(["$relationName as $alias" => $loader], $aggregate['column'], $aggregate['function']);
+
+            return;
+        }
+
+        $relation->addEagerConstraints($collection->all());
+
+        // Applies the aggregate's constrain callback (e.g. visibility
+        // scoping) to the relation's underlying Eloquent builder.
+        $loader($relation);
+
+        $query = $relation->getQuery();
+        $grammar = $query->getQuery()->getGrammar();
+
+        $column = $aggregate['column'] === '*'
+            ? '*'
+            : $grammar->wrap($relation->getRelated()->qualifyColumn($aggregate['column']));
+
+        $results = $query
+            ->toBase()
+            ->select($groupColumn)
+            ->selectRaw("{$aggregate['function']}({$column}) as {$grammar->wrap($alias)}")
+            ->groupBy($groupColumn)
+            ->get()
+            ->keyBy(Str::afterLast($groupColumn, '.'));
+
+        $default = $aggregate['function'] === 'count' ? 0 : null;
+
+        foreach ($collection as $model) {
+            $value = $results[$model->getAttribute($parentKeyName)]->{$alias} ?? $default;
+
+            $model->setAttribute($alias, $value);
         }
     }
 }
diff --git a/framework/core/src/Post/Access/ScopePostVisibility.php b/framework/core/src/Post/Access/ScopePostVisibility.php
@@ -47,11 +47,15 @@ public function withinDiscussion(User $actor, Builder $query, Discussion $discus
 
     public function __invoke(User $actor, Builder $query): void
     {
-        // Make sure the post's discussion is visible as well.
-        $query->whereExists(function ($query) use ($actor) {
-            $query->selectRaw('1')
-                ->from('discussions')
-                ->whereColumn('discussions.id', 'posts.discussion_id');
+        // Make sure the post's discussion is visible as well. Shaped as an
+        // uncorrelated IN-subquery rather than a per-row EXISTS: the visible
+        // discussions can then be materialized once per query, instead of the
+        // discussion visibility conditions (which extensions compound) being
+        // re-evaluated for every candidate post row — many posts share few
+        // discussions, so on mention counts, post windows and search results
+        // the EXISTS form did the same work hundreds of times over.
+        $query->whereIn('posts.discussion_id', function ($query) use ($actor) {
+            $query->select('discussions.id')->from('discussions');
             Discussion::query()->setQuery($query)->whereVisibleTo($actor);
         });
 
@@ -69,11 +73,12 @@ public function __invoke(User $actor, Builder $query): void
         if (! $actor->hasPermission('discussion.hidePosts')) {
             $query->where(function ($query) use ($actor) {
                 $query->whereNull('posts.hidden_at')
-                ->orWhere('posts.user_id', $actor->id)
-                    ->orWhereExists(function ($query) use ($actor) {
-                        $query->selectRaw('1')
+                    ->orWhere('posts.user_id', $actor->id)
+                    ->orWhereIn('posts.discussion_id', function ($query) use ($actor) {
+                        $query->select('discussions.id')
                             ->from('discussions')
-                            ->whereColumn('discussions.id', 'posts.discussion_id')
+                            // The 1=0 seed keeps this subquery empty when no
+                            // scoper grants the hidePosts ability at all.
                             ->where(function ($query) use ($actor) {
                                 $query
                                     ->whereRaw('1=0')
diff --git a/framework/core/tests/integration/api/posts/PostVisibilityTest.php b/framework/core/tests/integration/api/posts/PostVisibilityTest.php
@@ -0,0 +1,135 @@
+<?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\Extend;
+use Flarum\Post\Post;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Support\Arr;
+use PHPUnit\Framework\Attributes\Test;
+
+/**
+ * Characterizes ScopePostVisibility's discussion-level branches — the
+ * discussion must be visible, and hidden posts show for actors granted the
+ * hidePosts ability on the post's discussion — so the SQL shape (per-row
+ * EXISTS vs materialized IN-subquery) can change without the semantics
+ * moving. The post-local branches (own hidden posts, private posts) are
+ * covered by ShowTest and PostIdsVisibilityTest.
+ */
+class PostVisibilityTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->prepareDatabase([
+            Discussion::class => [
+                ['id' => 1, 'title' => 'Public', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 10, 'comment_count' => 2, 'is_private' => 0],
+                ['id' => 2, 'title' => 'Private', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 20, 'comment_count' => 1, 'is_private' => 1],
+                ['id' => 3, 'title' => 'Soft-deleted', 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'first_post_id' => 30, 'comment_count' => 1, 'is_private' => 0, 'hidden_at' => Carbon::now()->toDateTimeString()],
+            ],
+            Post::class => [
+                ['id' => 10, 'discussion_id' => 1, 'number' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>public post</p></t>'],
+                ['id' => 11, 'discussion_id' => 1, 'number' => 2, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>hidden post</p></t>', 'hidden_at' => Carbon::now()->toDateTimeString()],
+                ['id' => 20, 'discussion_id' => 2, 'number' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>post in private discussion</p></t>'],
+                ['id' => 30, 'discussion_id' => 3, 'number' => 1, 'created_at' => Carbon::now()->toDateTimeString(), 'user_id' => 2, 'type' => 'comment', 'content' => '<t><p>post in soft-deleted discussion</p></t>'],
+            ],
+            User::class => [
+                $this->normalUser(),
+                ['id' => 3, 'username' => 'onlooker', 'email' => 'onlooker@machine.local', 'is_email_confirmed' => 1],
+            ],
+        ]);
+    }
+
+    protected function tearDown(): void
+    {
+        // Scopers registered through test extenders live in static model
+        // state and would leak into later tests in this process.
+        (function () {
+            self::$visibilityScopers = [];
+        })->bindTo(null, Discussion::class)();
+
+        parent::tearDown();
+    }
+
+    private function visiblePostIds(?int $actorId = null): array
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/posts', $actorId ? ['authenticatedAs' => $actorId] : [])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $json = json_decode($response->getBody()->getContents(), true);
+
+        $ids = array_map('intval', array_column(Arr::get($json, 'data', []), 'id'));
+        sort($ids);
+
+        return $ids;
+    }
+
+    #[Test]
+    public function posts_of_invisible_discussions_are_excluded_for_guests()
+    {
+        // Private (2) and soft-deleted (3) discussions are invisible, so their
+        // posts never appear; the hidden post (11) is excluded post-locally.
+        $this->assertSame([10], $this->visiblePostIds());
+    }
+
+    #[Test]
+    public function discussion_author_sees_posts_of_their_soft_deleted_discussion()
+    {
+        // ScopeDiscussionVisibility lets authors see their own hidden
+        // discussions, and post visibility must follow it.
+        $this->assertSame([10, 11, 30], $this->visiblePostIds(2));
+    }
+
+    #[Test]
+    public function unrelated_user_sees_only_public_discussion_posts()
+    {
+        $this->assertSame([10], $this->visiblePostIds(3));
+    }
+
+    #[Test]
+    public function hidden_posts_appear_when_a_scoper_grants_hide_posts_on_the_discussion()
+    {
+        // Grants the hidePosts ability on discussion 1 only — like a per-tag
+        // moderator. User 3 gains the hidden post there, and nothing else.
+        $this->extend(
+            (new Extend\ModelVisibility(Discussion::class))
+                ->scope(GrantHidePostsOnDiscussionOne::class, 'hidePosts')
+        );
+
+        $this->assertSame([10, 11], $this->visiblePostIds(3));
+    }
+
+    #[Test]
+    public function without_any_hide_posts_scoper_no_discussion_grants_hidden_posts()
+    {
+        // The ability subquery must stay EMPTY when nothing scopes it — a
+        // seeded 1=0 guards against an unconstrained "all discussions" set.
+        $this->assertSame([10], $this->visiblePostIds(3));
+    }
+}
+
+class GrantHidePostsOnDiscussionOne
+{
+    public function __invoke(User $actor, Builder $query): void
+    {
+        $query->orWhere('discussions.id', 1);
+    }
+}
