Skip to content

[2.x] N+1 group_user queries when serializing post authors on the forum HTML render path #4724

Description

@imorland

Summary

The N+1 group_user fix from #4696 (shipped in v2.0.0-rc.3) does not cover the forum HTML page render path. When a discussion page is rendered server-side (e.g. for a guest / crawler), serializing post authors still issues one group_user query per serialized author instance via UserPolicy::editCredentials()User::isAdmin()$user->groups.

What #4696 fixed

#4696 made the UserResource groups getter read the eager-loaded relation, and the posts endpoints (Show/Index) ->eagerLoad(['user.groups']). The regression test tests/integration/api/posts/ListGroupsQueryCountTest.php verifies this — but it exercises the direct JSON:API endpoint (GET /api/posts) as an authenticated user:

$this->request('GET', '/api/posts', ['authenticatedAs' => 2])
    ->withQueryParams(['filter' => ['discussion' => 1]])

That path passes: groups are batch-loaded once.

What's still broken

When the forum content is rendered as HTML (Flarum\Forum\Content\Discussion), the same authors are serialized again through additional documents beyond the embedded /posts include (the discussion's own user, firstPost.user, etc.). Those User model instances do not carry the eager-loaded groups relation, so the per-user lazy load fires again.

The trigger is the email field's visible() callback and the canEditCredentials field in UserResource:

// UserResource — email field
->visible(function (User $user, Context $context) {
    return $context->getActor()->id === $user->id
        || $context->getActor()->can('editCredentials', $user); // ← isAdmin() → $user->groups
})

editCredentials reads $user->isAdmin() (→ $user->groups) on a User instance whose groups relation was not eager-loaded on this path.

Trace (from a downstream extension's render test)

Flarum\Api\Endpoint\Index serialize
  → UserResource email field visible()  (UserResource.php:185)
  → $actor->can('editCredentials', $user)
  → UserPolicy::editCredentials → $user->isAdmin() → $user->groups   ← lazy, 1 query per author instance

Query-log classification on an 8-author discussion page rendered as a guest:

Both populations coexist, which proves the eager-loaded instances and the editCredentials-checked instances are not the same objects.

How it surfaced

Discovered downstream in FriendsOfFlarum/seo, whose QAPage structured-data render walks a discussion's posts. A query-count regression guard there fails on rc.3 (growth of 6 vs an expected ≤3) while green on the older CI baseline. Repro is environment-agnostic (reproduces on SQLite, which is in core's own CI matrix).

Suggested fixes (one of)

  1. Reorder UserPolicy::editCredentials to check the cheap $actor permission / $actor->isAdmin() before reading $user->isAdmin() ($user->groups).
  2. Ensure the forum discussion content render eager-loads user.groups on all serialized author instances, not only the embedded /posts include.
  3. Have editCredentials avoid forcing a lazy relation load on unloaded User instances.

Repro environment

Related: #4695, #4696

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions