Skip to content

Commit fa31702

Browse files
imorlandimorland
authored
perf: point endpoint eager loads back at their parent models (#4877)
Relations pre-loaded by an endpoint eager load arrive through loadMissing(), which wires nothing back to the models they were loaded for. The relationship buffer would set the declared inverse, but it skips relations that are already loaded — so nothing did. Serializing an included post then re-fetched its discussion one row at a time for every visibility check (canEdit, canHide, canFlag), even though that discussion is the very model being listed. Installs running flarum/likes never saw this: likes carries a hand-written workaround that eager loads firstPost.discussion and its tags precisely "to avoid N+1s in DiscussionPolicy::can()". Any leaner install paid one discussion fetch per included post — a pure-core reproduction with a single eagerLoadWhenIncluded extender shows 10 identical single-row fetches on a 10-discussion page. loadRelations() now points each loaded relation back at its parent, using the relationship's declared inverse and falling back to the same class-name derivation EloquentBuffer::load() already uses for buffered loads. The parent is already in memory, so the wiring costs no queries. Likes' workaround is retired in the same change: the parent discussions it re-fetched (with their tags) are exactly what the inverse now points at, and the tags extension already eager loads tags on those parents. On a 74-extension install, the discussions index with firstPost and lastPost included drops from 43 to 39 queries and the index document from 41 to 39, with responses byte-identical — the removed queries are likes' now-redundant batched re-fetches.
1 parent 9793bec commit fa31702

3 files changed

Lines changed: 232 additions & 17 deletions

File tree

‎extensions/likes/extend.php‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -76,22 +76,6 @@ function (Endpoint\Index $endpoint): Endpoint\Index {
7676
}
7777
),
7878

79-
// When tags is enabled, also pre-load the back-reference to discussion and its tags,
80-
// needed to avoid N+1s in DiscussionPolicy::can() which accesses $discussion->tags.
81-
(new Extend\Conditional())
82-
->whenExtensionEnabled('flarum-tags', fn () => [
83-
(new Extend\ApiResource(Resource\DiscussionResource::class))
84-
->endpoint(
85-
Endpoint\Index::class,
86-
function (Endpoint\Index $endpoint): Endpoint\Index {
87-
return $endpoint->eagerLoadWhenIncluded([
88-
'firstPost' => ['firstPost.discussion', 'firstPost.discussion.tags'],
89-
'lastPost' => ['lastPost.discussion', 'lastPost.discussion.tags'],
90-
]);
91-
}
92-
),
93-
]),
94-
9579
(new Extend\Event())
9680
->listen(PostWasLiked::class, Listener\SendNotificationWhenPostIsLiked::class)
9781
->listen(PostWasUnliked::class, Listener\SendNotificationWhenPostIsUnliked::class)

‎framework/core/src/Api/Endpoint/Concerns/HasEagerLoading.php‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@
1111

1212
use Closure;
1313
use Flarum\Api\Resource\AbstractDatabaseResource;
14+
use Flarum\Api\Schema\Relationship\ToMany;
15+
use Flarum\Api\Schema\Relationship\ToOne;
1416
use Illuminate\Database\Eloquent\Collection;
17+
use Illuminate\Database\Eloquent\Model;
1518
use Illuminate\Support\Str;
1619
use Tobyz\JsonApiServer\Context;
1720

@@ -89,7 +92,9 @@ public function eagerLoadWhere(string $relation, callable $callback): static
8992
*/
9093
protected function loadRelations(Collection $models, Context $context, array $included = []): void
9194
{
92-
if (! $context->collection instanceof AbstractDatabaseResource) {
95+
$resource = $context->collection;
96+
97+
if (! $resource instanceof AbstractDatabaseResource) {
9398
return;
9499
}
95100

@@ -124,6 +129,74 @@ protected function loadRelations(Collection $models, Context $context, array $in
124129
if (! empty($simpleRelations)) {
125130
$models->loadMissing($simpleRelations);
126131
}
132+
133+
$this->setInverseRelations(
134+
$models,
135+
$context,
136+
$resource,
137+
array_merge(array_keys($whereRelations), $simpleRelations)
138+
);
139+
}
140+
141+
/**
142+
* Point relations loaded above back at the models they were loaded for.
143+
*
144+
* The relationship buffer does this for the relations it loads (see
145+
* EloquentBuffer::load()), but relations pre-loaded here arrive through
146+
* loadMissing(), which wires nothing back — and the buffer then skips
147+
* them because they are already loaded. Serializing such a related model
148+
* re-fetched its parent one row at a time: every visibility check on an
149+
* included firstPost read $post->discussion, which IS the discussion
150+
* being listed.
151+
*
152+
* @param string[] $relations
153+
*/
154+
private function setInverseRelations(Collection $models, Context $context, AbstractDatabaseResource $resource, array $relations): void
155+
{
156+
if ($models->isEmpty() || empty($relations)) {
157+
return;
158+
}
159+
160+
/** @var array<string, ToOne|ToMany> $fields */
161+
$fields = array_filter(
162+
$context->fields($resource),
163+
fn ($field) => $field instanceof ToOne || $field instanceof ToMany
164+
);
165+
166+
// Only the first segment of each loaded path is a relation of the
167+
// models at hand; deeper segments belong to other parents.
168+
$segments = array_unique(array_map(
169+
fn (string $relation) => explode('.', $relation)[0],
170+
$relations
171+
));
172+
173+
foreach ($segments as $segment) {
174+
// A relationship field may expose the relation under a different
175+
// name than the Eloquent relation used in eager load paths.
176+
$field = collect($fields)->first(
177+
fn (ToOne|ToMany $field) => ($field->property ?? $field->name) === $segment || $field->name === $segment
178+
);
179+
180+
foreach ($models as $model) {
181+
if (! $model->relationLoaded($segment)) {
182+
continue;
183+
}
184+
185+
$related = $model->getRelation($segment);
186+
187+
if (! $related) {
188+
continue;
189+
}
190+
191+
$inverse = $field->inverse ?? Str::camel(class_basename($model));
192+
193+
foreach ($related instanceof Collection ? $related : [$related] as $rel) {
194+
if ($rel instanceof Model && $rel->isRelation($inverse)) {
195+
$rel->setRelation($inverse, $model);
196+
}
197+
}
198+
}
199+
}
127200
}
128201

129202
protected function compileSimpleEagerLoads(Context $context, array $included): array

‎framework/core/tests/integration/api/discussions/ListWithIncludedPostsQueryCountTest.php‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
<?php
2+
3+
/*
4+
* This file is part of Flarum.
5+
*
6+
* For detailed copyright and license information, please view the
7+
* LICENSE file that was distributed with this source code.
8+
*/
9+
10+
namespace Flarum\Tests\integration\api\discussions;
11+
12+
use Carbon\Carbon;
13+
use Flarum\Api\Endpoint;
14+
use Flarum\Api\Resource;
15+
use Flarum\Discussion\Discussion;
16+
use Flarum\Extend;
17+
use Flarum\Post\Post;
18+
use Flarum\Testing\integration\RetrievesAuthorizedUsers;
19+
use Flarum\Testing\integration\TestCase;
20+
use Flarum\User\User;
21+
use Illuminate\Database\ConnectionInterface;
22+
use PHPUnit\Framework\Attributes\Test;
23+
24+
class ListWithIncludedPostsQueryCountTest extends TestCase
25+
{
26+
use RetrievesAuthorizedUsers;
27+
28+
private const DISCUSSION_COUNT = 10;
29+
30+
protected function setUp(): void
31+
{
32+
parent::setUp();
33+
34+
$discussions = [];
35+
$posts = [];
36+
$now = Carbon::now();
37+
38+
for ($i = 1; $i <= self::DISCUSSION_COUNT; $i++) {
39+
$discussions[] = [
40+
'id' => $i, 'title' => "Discussion $i", 'created_at' => $now,
41+
'user_id' => 1, 'first_post_id' => $i, 'comment_count' => 1,
42+
];
43+
$posts[] = [
44+
'id' => $i, 'discussion_id' => $i, 'created_at' => $now,
45+
'user_id' => 1, 'type' => 'comment', 'number' => 1,
46+
'content' => '<t><p>first post</p></t>',
47+
];
48+
}
49+
50+
$this->prepareDatabase([
51+
Discussion::class => $discussions,
52+
Post::class => $posts,
53+
User::class => [$this->normalUser()],
54+
]);
55+
}
56+
57+
#[Test]
58+
public function included_first_posts_reuse_the_discussion_being_listed(): void
59+
{
60+
$db = $this->database();
61+
$db->enableQueryLog();
62+
63+
$response = $this->send(
64+
$this->request('GET', '/api/discussions', ['authenticatedAs' => 2])
65+
->withQueryParams(['include' => 'firstPost'])
66+
);
67+
68+
$this->assertEquals(200, $response->getStatusCode());
69+
70+
// Serializing each included first post runs visibility checks
71+
// (canEdit, canHide, ...) that read the post's discussion. The
72+
// discussion is the very model being listed, so it must be reused,
73+
// not fetched again one row at a time per post.
74+
$singleFetches = array_filter($db->getQueryLog(), function (array $query) {
75+
$sql = str_replace(['`', '"', '[', ']'], '"', $query['query']);
76+
77+
return str_contains($sql, 'from "discussions" where "discussions"."id" = ');
78+
});
79+
80+
$this->assertCount(
81+
0,
82+
$singleFetches,
83+
'Included posts should reuse the listed discussion instead of re-fetching it per post.'
84+
);
85+
}
86+
87+
#[Test]
88+
public function posts_preloaded_by_endpoint_eager_loads_reuse_the_listed_discussion(): void
89+
{
90+
// Unlike the buffer-loaded case above, a relation PRE-loaded by an
91+
// endpoint eager load arrives via loadMissing() — a path that
92+
// historically set no inverse. The relationship buffer then sees the
93+
// relation as loaded and skips it entirely, so nothing wired the post
94+
// back to its discussion and every visibility check re-fetched it,
95+
// one row per post. This extender reproduces what sticky+geoip,
96+
// mentions and likes all do: eager load something UNDER an included
97+
// post relation.
98+
$this->extend(
99+
(new Extend\ApiResource(Resource\DiscussionResource::class))
100+
->endpoint(Endpoint\Index::class, function (Endpoint\Index $endpoint): Endpoint\Index {
101+
return $endpoint->eagerLoadWhenIncluded(['firstPost' => ['firstPost.user']]);
102+
})
103+
);
104+
105+
$db = $this->database();
106+
$db->enableQueryLog();
107+
108+
$response = $this->send(
109+
$this->request('GET', '/api/discussions', ['authenticatedAs' => 2])
110+
->withQueryParams(['include' => 'firstPost'])
111+
);
112+
113+
$this->assertEquals(200, $response->getStatusCode());
114+
115+
$singleFetches = array_filter($db->getQueryLog(), function (array $query) {
116+
$sql = str_replace(['`', '"', '[', ']'], '"', $query['query']);
117+
118+
return str_contains($sql, 'from "discussions" where "discussions"."id" = ');
119+
});
120+
121+
$this->assertCount(
122+
0,
123+
$singleFetches,
124+
'Pre-loaded included posts should reuse the listed discussion instead of re-fetching it per post.'
125+
);
126+
}
127+
128+
#[Test]
129+
public function included_first_posts_are_still_serialized(): void
130+
{
131+
$response = $this->send(
132+
$this->request('GET', '/api/discussions', ['authenticatedAs' => 2])
133+
->withQueryParams(['include' => 'firstPost'])
134+
);
135+
136+
$this->assertEquals(200, $response->getStatusCode());
137+
138+
$body = json_decode($response->getBody()->getContents(), true);
139+
140+
$includedPosts = array_filter($body['included'] ?? [], fn (array $r) => $r['type'] === 'posts');
141+
142+
$this->assertCount(self::DISCUSSION_COUNT, $includedPosts);
143+
144+
// And each discussion still links to its own first post.
145+
foreach ($body['data'] as $discussion) {
146+
$this->assertSame(
147+
$discussion['id'],
148+
$discussion['relationships']['firstPost']['data']['id'],
149+
'Fixture links discussion id N to post id N; the inverse wiring must not change linkage.'
150+
);
151+
}
152+
}
153+
154+
protected function database(): ConnectionInterface
155+
{
156+
return parent::database();
157+
}
158+
}

0 commit comments

Comments
 (0)