mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 10:04:18 +00:00
perf(mail-list): rows read message fields, not whole messages
Every list page fetch-joined every message of every thread on it, bodies, headers and search vector included, to read a sender and a date off each and a snippet off the last. ThreadRows now fetches only the row fields of all messages plus the two body columns of each row's latest message; the snippet is flattened once per row. Rendering is unchanged. Measured by ThreadListQueryBudgetTest at 50 rows: Message entities hydrated 171 -> 0; queries 24 -> 25 (inbox visit), 13 -> 14 (frame). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
dd6bd5a62b
commit
67935f0043
13 files changed
+510
-119
No files matched your search
@@ -24,6 +24,7 @@ use App\Service\Ai\ThreadTranscript;
|
||||
use App\Service\Mail\NewMailMarkers;
|
||||
use App\Service\Mail\SidebarCounts;
|
||||
use App\Service\Mail\ThreadListRenderer;
|
||||
use App\Service\Mail\ThreadRows;
|
||||
use Doctrine\ORM\EntityManagerInterface;
|
||||
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
|
||||
use Symfony\Component\HttpFoundation\JsonResponse;
|
||||
@@ -46,6 +47,7 @@ final class MailController extends AbstractController
|
||||
private readonly ThreadSummariser $summariser,
|
||||
private readonly ThreadTranscript $transcript,
|
||||
private readonly ThreadSummaryStore $summaries,
|
||||
private readonly ThreadRows $threadRows,
|
||||
)
|
||||
{
|
||||
}
|
||||
@@ -255,7 +257,7 @@ final class MailController extends AbstractController
|
||||
static fn (MessageCategory $case): bool => false === in_array($case, $tabs, true),
|
||||
));
|
||||
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
return $this->renderList($request, 'mail/inbox.html.twig', $threads, [
|
||||
'tab' => $tab,
|
||||
@@ -313,7 +315,7 @@ final class MailController extends AbstractController
|
||||
$sort = $this->listSort($request);
|
||||
$threads = $this->threadRepository->findForLabel($label, $account, $page, self::PER_PAGE, $sort, $unreadOnly);
|
||||
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
return $this->renderList($request, 'mail/label.html.twig', $threads, [
|
||||
'label' => $label,
|
||||
@@ -406,7 +408,7 @@ final class MailController extends AbstractController
|
||||
$sort = $this->listSort($request);
|
||||
$threads = $this->threadRepository->findForAccountInbox($account, $page, self::PER_PAGE, $sort);
|
||||
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
return $this->renderList($request, 'mail/account.html.twig', $threads, [
|
||||
'account' => $account,
|
||||
@@ -432,7 +434,7 @@ final class MailController extends AbstractController
|
||||
$sort = $this->listSort($request);
|
||||
$threads = $this->threadRepository->findForStarred($user, $page, self::PER_PAGE, $sort, $unreadOnly);
|
||||
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
return $this->renderList($request, 'mail/starred.html.twig', $threads, [
|
||||
'page' => $page,
|
||||
@@ -521,7 +523,7 @@ final class MailController extends AbstractController
|
||||
$sort = $this->listSort($request);
|
||||
$threads = $this->threadRepository->findForRole($user, $role, $page, self::PER_PAGE, $sort, $unreadOnly);
|
||||
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
return $this->renderList($request, $template, $threads, [
|
||||
'page' => $page,
|
||||
|
||||
@@ -11,6 +11,7 @@ use App\Domain\Enum\Mail\SearchSortOrder;
|
||||
use App\Entity\User\User;
|
||||
use App\Repository\Mail\MessageThreadRepository;
|
||||
use App\Service\Mail\ThreadListRenderer;
|
||||
use App\Service\Mail\ThreadRows;
|
||||
use App\Service\Search\SearchQueryParser;
|
||||
use App\Service\Search\SearchResultHighlights;
|
||||
use Doctrine\ORM\EntityManagerInterface;
|
||||
@@ -33,6 +34,7 @@ final class SearchController extends AbstractController
|
||||
private readonly SemanticCoverage $coverage,
|
||||
private readonly EmbeddingCatchUp $catchUp,
|
||||
private readonly SearchResultHighlights $highlights,
|
||||
private readonly ThreadRows $threadRows,
|
||||
) {}
|
||||
|
||||
#[Route('', name: '', methods: ['GET'])]
|
||||
@@ -136,7 +138,7 @@ final class SearchController extends AbstractController
|
||||
// why a full page of results measured 167 queries against the inbox's
|
||||
// 120 for the same fifty rows: search paid for the label chips one row
|
||||
// at a time on top of everything the inbox was already paying.
|
||||
$this->threadRepository->preloadForRows($threads);
|
||||
$this->threadRows->preload($threads);
|
||||
|
||||
// WHY THE ROW CAN SAY WHY IT IS HERE. Without this a result shows the
|
||||
// opening line of its newest message, which usually does not contain
|
||||
@@ -145,9 +147,9 @@ final class SearchController extends AbstractController
|
||||
// a window around the match with every occurrence marked.
|
||||
//
|
||||
// AFTER the preload, and that is an ordering constraint rather than a
|
||||
// preference: this reads `thread.messages` for the ids, so run before
|
||||
// it, the page pays a lazy load per row and the search list is back to
|
||||
// the hundred-and-sixty-seven-query shape preloadForRows() exists to
|
||||
// preference: this reads the preloaded row messages for the ids, so run
|
||||
// before it, the page pays a load per row and the search list is back to
|
||||
// the hundred-and-sixty-seven-query shape the preload exists to
|
||||
// prevent. See ThreadListQueryBudgetTest.
|
||||
//
|
||||
// Handed to the template as a map keyed by thread id, next to
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Domain\DTO\Mail;
|
||||
|
||||
/**
|
||||
* What one conversation row needs from the thread's messages, already reduced.
|
||||
*
|
||||
* `latest` and `draft` are the two answers the template used to compute with
|
||||
* `|last` and `|filter(m => m.isDraft)|last` over the whole collection; they are
|
||||
* computed the same way here, over the same order (receivedAt, id — the
|
||||
* association's #[ORM\OrderBy]), so the row cannot tell the difference.
|
||||
*/
|
||||
final readonly class ThreadRow
|
||||
{
|
||||
public ?ThreadRowMessage $latest;
|
||||
public ?ThreadRowMessage $draft;
|
||||
|
||||
/**
|
||||
* @param list<ThreadRowMessage> $messages oldest first
|
||||
* @param string $snippet MessageSnippet of the latest message
|
||||
*/
|
||||
public function __construct(
|
||||
public array $messages,
|
||||
public string $snippet,
|
||||
) {
|
||||
$this->latest = [] === $messages ? null : $messages[count($messages) - 1];
|
||||
|
||||
$draft = null;
|
||||
foreach ($messages as $message) {
|
||||
if ($message->isDraft) {
|
||||
$draft = $message;
|
||||
}
|
||||
}
|
||||
$this->draft = $draft;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,56 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Domain\DTO\Mail;
|
||||
|
||||
use App\Entity\Mail\Message;
|
||||
use DateTimeImmutable;
|
||||
|
||||
/**
|
||||
* The part of one message a conversation ROW reads — and nothing else.
|
||||
*
|
||||
* A list row walks every message of its thread (ThreadParticipants wants the
|
||||
* whole cast), but of each one it wants a sender, the recipients, a few dates
|
||||
* and the draft flag. Hydrating Message for that dragged every body, header
|
||||
* blob and search vector of every message on the page into PHP; a fifty-row
|
||||
* inbox of long threads was megabytes of mail nobody on the page could see.
|
||||
* See App\Service\Mail\ThreadRows.
|
||||
*
|
||||
* The property names are Message's own, deliberately: ThreadParticipants and
|
||||
* the row template read either one without knowing which they were given.
|
||||
*/
|
||||
final readonly class ThreadRowMessage
|
||||
{
|
||||
/**
|
||||
* @param array<int, array{name?: ?string, address?: ?string}>|null $toAddresses
|
||||
*/
|
||||
public function __construct(
|
||||
public ?int $id,
|
||||
public ?string $fromAddress,
|
||||
public ?string $fromName,
|
||||
public ?array $toAddresses,
|
||||
public bool $isDraft,
|
||||
public ?DateTimeImmutable $receivedAt,
|
||||
public ?DateTimeImmutable $sentAt,
|
||||
public DateTimeImmutable $createdAt,
|
||||
public ?DateTimeImmutable $submissionSendAt,
|
||||
) {
|
||||
}
|
||||
|
||||
/** For a thread whose messages are already in memory — the thread view, a sync. */
|
||||
public static function of(Message $message): self
|
||||
{
|
||||
return new self(
|
||||
$message->id,
|
||||
$message->fromAddress,
|
||||
$message->fromName,
|
||||
$message->toAddresses,
|
||||
$message->isDraft(),
|
||||
$message->receivedAt,
|
||||
$message->sentAt,
|
||||
$message->createdAt,
|
||||
$message->submissionSendAt,
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -542,10 +542,10 @@ class MessageRepository extends ServiceEntityRepository
|
||||
*
|
||||
* The companion to findSearchHeadlines() above, and only ever called after
|
||||
* it: SearchHighlighter's fallback is a substring search, so it needs the
|
||||
* field as it was written rather than the fragment Postgres returned. The
|
||||
* web search page needs no such query — `preloadForRows()` has hydrated
|
||||
* every message on the page before the highlighter runs — but a JMAP
|
||||
* SearchSnippet/get holds DBAL rows and nothing else.
|
||||
* field as it was written rather than the fragment Postgres returned. Both
|
||||
* callers hold rows rather than hydrated bodies: a JMAP SearchSnippet/get
|
||||
* has DBAL rows and nothing else, and the web search page's rows carry no
|
||||
* bodies either (see App\Service\Mail\ThreadRows).
|
||||
*
|
||||
* TWO ID LISTS, BECAUSE THE TWO COLUMNS COST DIFFERENT AMOUNTS. A subject
|
||||
* is tens of bytes and a text part is thousands, and the field that is
|
||||
@@ -2244,4 +2244,79 @@ class MessageRepository extends ServiceEntityRepository
|
||||
->getQuery()
|
||||
->getResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* The handful of columns a conversation row reads from each of its
|
||||
* thread's messages, for every thread on a list page in one statement.
|
||||
*
|
||||
* Fields rather than entities, and that is the point: hydrating Message
|
||||
* here pulled every body (text, raw HTML, sanitised HTML), the header blob
|
||||
* and the search vector of every message on the page into PHP, to read a
|
||||
* sender and a date off each. See App\Service\Mail\ThreadRows.
|
||||
*
|
||||
* ORDERED the way the association is (#[ORM\OrderBy(receivedAt, id)]),
|
||||
* because the row's "latest" and "draft" are both "the last one of these".
|
||||
*
|
||||
* @param list<int> $threadIds
|
||||
*
|
||||
* @return list<array<string, mixed>>
|
||||
*/
|
||||
public function findRowFieldsForThreads(array $threadIds): array
|
||||
{
|
||||
if ([] === $threadIds) {
|
||||
return [];
|
||||
}
|
||||
|
||||
/** @var list<array<string, mixed>> */
|
||||
return $this->createQueryBuilder('m')
|
||||
->select(
|
||||
'm.id',
|
||||
'IDENTITY(m.thread) AS threadId',
|
||||
'm.fromAddress',
|
||||
'm.fromName',
|
||||
'm.toAddresses',
|
||||
'm.flags',
|
||||
'm.receivedAt',
|
||||
'm.sentAt',
|
||||
'm.createdAt',
|
||||
'm.submissionSendAt',
|
||||
)
|
||||
->where('m.thread IN (:threads)')
|
||||
->setParameter('threads', $threadIds)
|
||||
->addOrderBy('m.receivedAt', 'ASC')
|
||||
->addOrderBy('m.id', 'ASC')
|
||||
->getQuery()
|
||||
->getArrayResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* The two body columns MessageSnippet reads, for specific messages only —
|
||||
* one per list row, the row's latest.
|
||||
*
|
||||
* @param list<int> $ids
|
||||
*
|
||||
* @return array<int, array{bodyHtmlSafe: ?string, bodyText: ?string}>
|
||||
*/
|
||||
public function findSnippetBodies(array $ids): array
|
||||
{
|
||||
if ([] === $ids) {
|
||||
return [];
|
||||
}
|
||||
|
||||
/** @var list<array{id: int|string, bodyHtmlSafe: ?string, bodyText: ?string}> $rows */
|
||||
$rows = $this->createQueryBuilder('m')
|
||||
->select('m.id', 'm.bodyHtmlSafe', 'm.bodyText')
|
||||
->where('m.id IN (:ids)')
|
||||
->setParameter('ids', $ids)
|
||||
->getQuery()
|
||||
->getArrayResult();
|
||||
|
||||
$bodies = [];
|
||||
|
||||
foreach ($rows as $row) {
|
||||
$bodies[(int) $row['id']] = ['bodyHtmlSafe' => $row['bodyHtmlSafe'], 'bodyText' => $row['bodyText']];
|
||||
}
|
||||
|
||||
return $bodies;
|
||||
}
|
||||
}
|
||||
@@ -1166,48 +1166,6 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
->getResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* The same trick for the messages collection, which one row reads three
|
||||
* times over: thread_participants() walks every sender, `|last` supplies
|
||||
* the snippet and the avatar seed, and `|filter(m => m.isDraft)` decides
|
||||
* whether the row opens the compose dock instead of the thread. Each of
|
||||
* those initialises the collection, so a fifty-row list was fifty
|
||||
* `SELECT … FROM message WHERE thread_id = ?`.
|
||||
*
|
||||
* A batch rather than a denormalised column on MessageThread, and the
|
||||
* participants are why. The row does not want "the latest message" — it
|
||||
* wants the CAST of the conversation, every distinct sender in the order
|
||||
* they joined it (see ThreadParticipants). A lastMessageId stored beside
|
||||
* messageCount would leave that walk exactly where it is, the collection
|
||||
* would hydrate anyway, and the denormalisation would have bought nothing
|
||||
* while adding a second thing that can go stale. One query for the page
|
||||
* settles all three readers at once.
|
||||
*
|
||||
* ORDERED, which the label preload has no need to be: the association
|
||||
* carries #[ORM\OrderBy(receivedAt, id)] and a fetch join does not inherit
|
||||
* it. Without the order spelled here the collection arrives however
|
||||
* Postgres returned it, `|last` stops meaning "newest", and the row's
|
||||
* snippet comes off an arbitrary message.
|
||||
*
|
||||
* @param MessageThread[] $threads
|
||||
*/
|
||||
public function preloadMessages(array $threads): void
|
||||
{
|
||||
if (count($threads) === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->createQueryBuilder('thread')
|
||||
->addSelect('message')
|
||||
->leftJoin('thread.messages', 'message')
|
||||
->where('thread IN (:threads)')
|
||||
->setParameter('threads', $threads)
|
||||
->addOrderBy('message.receivedAt', 'ASC')
|
||||
->addOrderBy('message.id', 'ASC')
|
||||
->getQuery()
|
||||
->getResult();
|
||||
}
|
||||
|
||||
/**
|
||||
* And the account, which the row reaches for twice — once for the corner
|
||||
* wedge that says which mailbox a unified row arrived in, and once inside
|
||||
@@ -1241,14 +1199,11 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
}
|
||||
|
||||
/**
|
||||
* Everything one page of thread ROWS reads, in three queries.
|
||||
* The two entity preloads a page of thread rows needs.
|
||||
*
|
||||
* One entry point rather than three calls at each of the six call sites.
|
||||
* The label preload existed and was called from every list view except
|
||||
* search — which is exactly why search cost fifty queries more than the
|
||||
* same fifty rows in the inbox. A list view that gets one preload and not
|
||||
* another is the failure this replaces; adding a fourth thing the row
|
||||
* needs should mean editing this method, not auditing every controller.
|
||||
* The messages are not among them any more: a row reads a few fields of
|
||||
* each, never the entity, and App\Service\Mail\ThreadRows fetches exactly
|
||||
* those. That service is the list views' one entry point and calls this.
|
||||
*
|
||||
* @param MessageThread[] $threads
|
||||
*/
|
||||
@@ -1256,7 +1211,6 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
{
|
||||
$this->preloadAccounts($threads);
|
||||
$this->preloadLabels($threads);
|
||||
$this->preloadMessages($threads);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -32,8 +32,8 @@ use App\Entity\Mail\Message;
|
||||
* path trusts, and stripping tags off raw sender HTML would put script text and
|
||||
* style rules into a list row.
|
||||
*
|
||||
* Costs no query. The row already hydrates the whole Message to read
|
||||
* `bodyText`, so the HTML is in memory either way.
|
||||
* Costs no query of its own. The list fetches the two body columns of each
|
||||
* row's latest message only — see App\Service\Mail\ThreadRows.
|
||||
*/
|
||||
final class MessageSnippet
|
||||
{
|
||||
@@ -79,7 +79,16 @@ final class MessageSnippet
|
||||
return '';
|
||||
}
|
||||
|
||||
$fromHtml = $this->flatten((string) $message->bodyHtmlSafe);
|
||||
return $this->fromBodies($message->bodyHtmlSafe, $message->bodyText);
|
||||
}
|
||||
|
||||
/**
|
||||
* The same preview from the two columns alone, for a caller that fetched
|
||||
* them without hydrating the Message — see App\Service\Mail\ThreadRows.
|
||||
*/
|
||||
public function fromBodies(?string $bodyHtmlSafe, ?string $bodyText): string
|
||||
{
|
||||
$fromHtml = $this->flatten((string) $bodyHtmlSafe);
|
||||
|
||||
if ('' !== $fromHtml) {
|
||||
return $fromHtml;
|
||||
@@ -89,7 +98,7 @@ final class MessageSnippet
|
||||
// down to nothing. The text part is all there is, and it is still run
|
||||
// through the same flattening, because a sender who put markup in it is
|
||||
// exactly the case that made this necessary.
|
||||
return $this->flatten((string) $message->bodyText);
|
||||
return $this->flatten((string) $bodyText);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -4,6 +4,8 @@ declare(strict_types=1);
|
||||
|
||||
namespace App\Service\Mail;
|
||||
|
||||
use App\Domain\DTO\Mail\ThreadRowMessage;
|
||||
use App\Entity\Mail\Account;
|
||||
use App\Entity\Mail\Message;
|
||||
use App\Entity\Mail\MessageThread;
|
||||
|
||||
@@ -28,14 +30,28 @@ final class ThreadParticipants
|
||||
*/
|
||||
public function forThread(MessageThread $thread, string $me = 'me'): array
|
||||
{
|
||||
$owned = $this->ownedAddresses($thread);
|
||||
$names = $this->senders($thread, $owned, $me);
|
||||
return $this->forMessages($thread->messages, $thread->account, $me);
|
||||
}
|
||||
|
||||
/**
|
||||
* The same answer from the row fields alone — a list page never hydrates
|
||||
* the messages themselves, see App\Service\Mail\ThreadRows.
|
||||
*
|
||||
* @param iterable<Message|ThreadRowMessage> $messages oldest first
|
||||
*
|
||||
* @return list<string>
|
||||
*/
|
||||
public function forMessages(iterable $messages, ?Account $account, string $me = 'me'): array
|
||||
{
|
||||
$messages = is_array($messages) ? $messages : iterator_to_array($messages, false);
|
||||
$owned = $this->ownedAddresses($account);
|
||||
$names = $this->senders($messages, $owned, $me);
|
||||
|
||||
// A thread nobody else has written in — a sent mail, or a draft. Its
|
||||
// recipients are the interesting party; falling back to "me" alone
|
||||
// would make every row in Sent identical.
|
||||
if ([] === $names || [$me] === $names) {
|
||||
$recipients = $this->recipients($thread, $owned);
|
||||
$recipients = $this->recipients($messages, $owned);
|
||||
|
||||
if ([] !== $recipients) {
|
||||
return $this->elide($recipients);
|
||||
@@ -46,15 +62,16 @@ final class ThreadParticipants
|
||||
}
|
||||
|
||||
/**
|
||||
* @param list<string> $owned
|
||||
* @param array<Message|ThreadRowMessage> $messages
|
||||
* @param list<string> $owned
|
||||
*
|
||||
* @return list<string>
|
||||
*/
|
||||
private function senders(MessageThread $thread, array $owned, string $me): array
|
||||
private function senders(array $messages, array $owned, string $me): array
|
||||
{
|
||||
$names = [];
|
||||
|
||||
foreach ($thread->messages as $message) {
|
||||
foreach ($messages as $message) {
|
||||
$address = $this->normalise($message->fromAddress);
|
||||
|
||||
if ('' === $address) {
|
||||
@@ -72,13 +89,14 @@ final class ThreadParticipants
|
||||
/**
|
||||
* Everyone the newest message was addressed to, the reader excluded.
|
||||
*
|
||||
* @param list<string> $owned
|
||||
* @param array<Message|ThreadRowMessage> $messages
|
||||
* @param list<string> $owned
|
||||
*
|
||||
* @return list<string>
|
||||
*/
|
||||
private function recipients(MessageThread $thread, array $owned): array
|
||||
private function recipients(array $messages, array $owned): array
|
||||
{
|
||||
$newest = $this->newest($thread);
|
||||
$newest = $this->newest($messages);
|
||||
|
||||
if (null === $newest) {
|
||||
return [];
|
||||
@@ -121,13 +139,16 @@ final class ThreadParticipants
|
||||
return array_values(array_unique($names));
|
||||
}
|
||||
|
||||
private function newest(MessageThread $thread): ?Message
|
||||
/**
|
||||
* @param array<Message|ThreadRowMessage> $messages
|
||||
*/
|
||||
private function newest(array $messages): Message|ThreadRowMessage|null
|
||||
{
|
||||
$newest = null;
|
||||
|
||||
// The association is ordered by date, but a thread assembled in memory
|
||||
// during a sync is not, so the newest is picked rather than assumed.
|
||||
foreach ($thread->messages as $message) {
|
||||
foreach ($messages as $message) {
|
||||
// createdAt closes the chain and is never null, so $at always has
|
||||
// a value to compare — only the first pass has nothing to compare
|
||||
// it against.
|
||||
@@ -176,10 +197,8 @@ final class ThreadParticipants
|
||||
/**
|
||||
* @return list<string>
|
||||
*/
|
||||
private function ownedAddresses(MessageThread $thread): array
|
||||
private function ownedAddresses(?Account $account): array
|
||||
{
|
||||
$account = $thread->account;
|
||||
|
||||
if (null === $account) {
|
||||
return [];
|
||||
}
|
||||
|
||||
@@ -0,0 +1,190 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Service\Mail;
|
||||
|
||||
use App\Domain\DTO\Mail\ThreadRow;
|
||||
use App\Domain\DTO\Mail\ThreadRowMessage;
|
||||
use App\Domain\Enum\Mail\MessageFlag;
|
||||
use App\Entity\Mail\MessageThread;
|
||||
use App\Repository\Mail\MessageRepository;
|
||||
use App\Repository\Mail\MessageThreadRepository;
|
||||
use DateTimeImmutable;
|
||||
use Doctrine\Common\Collections\AbstractLazyCollection;
|
||||
use Symfony\Contracts\Service\ResetInterface;
|
||||
|
||||
/**
|
||||
* What a page of conversation rows reads from their messages, fetched once.
|
||||
*
|
||||
* ── What this replaces ──────────────────────────────────────────────────────
|
||||
* The row used to walk `thread.messages`, so the list preloaded that whole
|
||||
* collection with a fetch join: every message of every thread on the page,
|
||||
* hydrated in full — body text, raw HTML, sanitised HTML, the header blob —
|
||||
* to read a sender and a date off each and a snippet off the last. On a page of
|
||||
* long newsletter threads that was megabytes per render of mail nobody could
|
||||
* see from the list.
|
||||
*
|
||||
* Now it is two statements for the page: the row fields of every message
|
||||
* (MessageRepository::findRowFieldsForThreads), and the two body columns of
|
||||
* the ONE message per row whose snippet is shown. The snippet is flattened
|
||||
* here, once per row, rather than by the template.
|
||||
*
|
||||
* ── The three ways a row is asked for ───────────────────────────────────────
|
||||
* A list page calls preload() with its rows first. A single row re-rendered on
|
||||
* its own — a status stream after archive or star — was never preloaded and is
|
||||
* loaded alone, which costs what the lazy collection used to. And a thread
|
||||
* whose messages are ALREADY in memory — the thread view, a sync that just
|
||||
* assembled it — is read from the entities, because those are the truth for
|
||||
* this request and may hold a message not yet flushed.
|
||||
*
|
||||
* Resettable because FrankenPHP and the Messenger workers keep this alive
|
||||
* between requests, and a row answered from a previous one is stale.
|
||||
*/
|
||||
final class ThreadRows implements ResetInterface
|
||||
{
|
||||
/** @var array<int, ThreadRow> thread id => its row */
|
||||
private array $rows = [];
|
||||
|
||||
public function __construct(
|
||||
private readonly MessageThreadRepository $threads,
|
||||
private readonly MessageRepository $messages,
|
||||
private readonly MessageSnippet $snippet,
|
||||
) {
|
||||
}
|
||||
|
||||
public function reset(): void
|
||||
{
|
||||
$this->rows = [];
|
||||
}
|
||||
|
||||
/**
|
||||
* Everything one page of thread ROWS reads, in four queries.
|
||||
*
|
||||
* One entry point for the six list views. A list that gets one preload and
|
||||
* not another is the failure this exists to prevent — search once cost
|
||||
* fifty queries more than the inbox for the same rows because it skipped
|
||||
* the label preload — so adding a fifth thing the row needs should mean
|
||||
* editing this method, not auditing every controller.
|
||||
*
|
||||
* @param MessageThread[] $threads
|
||||
*/
|
||||
public function preload(array $threads): void
|
||||
{
|
||||
if ([] === $threads) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->threads->preloadForRows($threads);
|
||||
|
||||
$ids = [];
|
||||
|
||||
foreach ($threads as $thread) {
|
||||
if (null !== $thread->id && !$this->inMemory($thread)) {
|
||||
$ids[] = $thread->id;
|
||||
}
|
||||
}
|
||||
|
||||
$this->load($ids);
|
||||
}
|
||||
|
||||
public function for(MessageThread $thread): ThreadRow
|
||||
{
|
||||
if (null === $thread->id || $this->inMemory($thread)) {
|
||||
return $this->fromEntities($thread);
|
||||
}
|
||||
|
||||
if (!isset($this->rows[$thread->id])) {
|
||||
$this->load([$thread->id]);
|
||||
}
|
||||
|
||||
return $this->rows[$thread->id];
|
||||
}
|
||||
|
||||
/**
|
||||
* An initialised collection is Doctrine's in-memory truth, so it wins over
|
||||
* a fetched copy; an uninitialised one costs nothing to ask about.
|
||||
*/
|
||||
private function inMemory(MessageThread $thread): bool
|
||||
{
|
||||
$messages = $thread->messages;
|
||||
|
||||
return !$messages instanceof AbstractLazyCollection || $messages->isInitialized();
|
||||
}
|
||||
|
||||
private function fromEntities(MessageThread $thread): ThreadRow
|
||||
{
|
||||
$messages = array_values(array_map(
|
||||
ThreadRowMessage::of(...),
|
||||
$thread->messages->toArray(),
|
||||
));
|
||||
|
||||
$latest = $thread->messages->last();
|
||||
|
||||
return new ThreadRow($messages, $this->snippet->of(false === $latest ? null : $latest));
|
||||
}
|
||||
|
||||
/**
|
||||
* @param list<int> $threadIds
|
||||
*/
|
||||
private function load(array $threadIds): void
|
||||
{
|
||||
if ([] === $threadIds) {
|
||||
return;
|
||||
}
|
||||
|
||||
/** @var array<int, list<ThreadRowMessage>> $byThread */
|
||||
$byThread = array_fill_keys($threadIds, []);
|
||||
|
||||
foreach ($this->messages->findRowFieldsForThreads($threadIds) as $row) {
|
||||
$flags = is_array($row['flags']) ? $row['flags'] : [];
|
||||
|
||||
/** @var DateTimeImmutable $createdAt */
|
||||
$createdAt = $row['createdAt'];
|
||||
|
||||
$byThread[(int) $row['threadId']][] = new ThreadRowMessage(
|
||||
(int) $row['id'],
|
||||
self::stringOrNull($row['fromAddress']),
|
||||
self::stringOrNull($row['fromName']),
|
||||
is_array($row['toAddresses']) ? $row['toAddresses'] : null,
|
||||
in_array(MessageFlag::DRAFT->value, $flags, true),
|
||||
self::dateOrNull($row['receivedAt']),
|
||||
self::dateOrNull($row['sentAt']),
|
||||
$createdAt,
|
||||
self::dateOrNull($row['submissionSendAt']),
|
||||
);
|
||||
}
|
||||
|
||||
$latestIds = [];
|
||||
|
||||
foreach ($byThread as $messages) {
|
||||
$latest = $messages[count($messages) - 1] ?? null;
|
||||
|
||||
if (null !== $latest?->id) {
|
||||
$latestIds[] = $latest->id;
|
||||
}
|
||||
}
|
||||
|
||||
$bodies = $this->messages->findSnippetBodies($latestIds);
|
||||
|
||||
foreach ($byThread as $threadId => $messages) {
|
||||
$latestId = ($messages[count($messages) - 1] ?? null)?->id;
|
||||
$body = null !== $latestId ? ($bodies[$latestId] ?? null) : null;
|
||||
|
||||
$this->rows[$threadId] = new ThreadRow(
|
||||
$messages,
|
||||
null === $body ? '' : $this->snippet->fromBodies($body['bodyHtmlSafe'], $body['bodyText']),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
private static function stringOrNull(mixed $value): ?string
|
||||
{
|
||||
return is_string($value) ? $value : null;
|
||||
}
|
||||
|
||||
private static function dateOrNull(mixed $value): ?DateTimeImmutable
|
||||
{
|
||||
return $value instanceof DateTimeImmutable ? $value : null;
|
||||
}
|
||||
}
|
||||
@@ -4,9 +4,10 @@ declare(strict_types=1);
|
||||
|
||||
namespace App\Service\Search;
|
||||
|
||||
use App\Entity\Mail\Message;
|
||||
use App\Domain\DTO\Mail\ThreadRowMessage;
|
||||
use App\Entity\Mail\MessageThread;
|
||||
use App\Repository\Mail\MessageRepository;
|
||||
use App\Service\Mail\ThreadRows;
|
||||
use Twig\Markup;
|
||||
|
||||
/**
|
||||
@@ -32,8 +33,8 @@ use Twig\Markup;
|
||||
* subject and text part it highlights are columns of `message`, and the search
|
||||
* vector it agrees with is generated from those same columns. A result page is
|
||||
* a list of THREADS and spans every account in a unified mailbox, so this walks
|
||||
* the rows the page is already holding — `preloadForRows()` has hydrated their
|
||||
* messages by the time this runs, so the ids cost nothing — groups them by the
|
||||
* the rows the page is already holding — ThreadRows has fetched their
|
||||
* messages' ids by the time this runs, so the ids cost nothing — groups them by the
|
||||
* THREAD's account, and asks for one batch of headlines per account.
|
||||
*
|
||||
* The thread's account rather than each message's: a thread belongs to exactly
|
||||
@@ -85,6 +86,7 @@ final class SearchResultHighlights
|
||||
public function __construct(
|
||||
private readonly MessageRepository $messages,
|
||||
private readonly SearchHighlighter $highlighter,
|
||||
private readonly ThreadRows $rows,
|
||||
) {
|
||||
}
|
||||
|
||||
@@ -119,6 +121,7 @@ final class SearchResultHighlights
|
||||
}
|
||||
|
||||
$headlines = [];
|
||||
$bodies = [];
|
||||
|
||||
foreach ($candidates as $accountId => $messageIds) {
|
||||
$rows = $this->messages->findSearchHeadlines(
|
||||
@@ -128,8 +131,21 @@ final class SearchResultHighlights
|
||||
SearchHighlighter::HEADLINE_OPTIONS,
|
||||
);
|
||||
|
||||
$unmarked = [];
|
||||
|
||||
foreach ($rows as $row) {
|
||||
$headlines[(int) $row['id']] = $row;
|
||||
|
||||
if (!$this->highlighter->isMarked($row['preview'])) {
|
||||
$unmarked[] = (int) $row['id'];
|
||||
}
|
||||
}
|
||||
|
||||
// Only the bodies `ts_headline` declined to mark, which the
|
||||
// fallback has to search itself. Usually none, and then no
|
||||
// statement is issued at all.
|
||||
foreach ($this->messages->findSearchTexts($accountId, $unmarked, $unmarked) as $id => $text) {
|
||||
$bodies[$id] = $text['body_text'];
|
||||
}
|
||||
}
|
||||
|
||||
@@ -142,7 +158,7 @@ final class SearchResultHighlights
|
||||
continue;
|
||||
}
|
||||
|
||||
$highlight = $this->forThread($thread, $headlines, $freeText);
|
||||
$highlight = $this->forThread($thread, $headlines, $bodies, $freeText);
|
||||
|
||||
if (null !== $highlight['subject'] || null !== $highlight['snippet']) {
|
||||
$highlights[$id] = $highlight;
|
||||
@@ -181,18 +197,18 @@ final class SearchResultHighlights
|
||||
/**
|
||||
* The messages of one thread that are worth a headline, newest first.
|
||||
*
|
||||
* The association is ordered OLDEST first — `#[ORM\OrderBy]` on receivedAt,
|
||||
* which preloadMessages() has to repeat because a fetch join does not
|
||||
* inherit it — so newest first is that reversed. Both loops in this class
|
||||
* read the same list, or the one that asks for headlines and the one that
|
||||
* reads them back would disagree about which messages exist.
|
||||
* The row messages are ordered OLDEST first — the association's
|
||||
* `#[ORM\OrderBy]` on receivedAt, which ThreadRows repeats — so newest
|
||||
* first is that reversed. Both loops in this class read the same list, or
|
||||
* the one that asks for headlines and the one that reads them back would
|
||||
* disagree about which messages exist.
|
||||
*
|
||||
* @return list<Message>
|
||||
* @return list<ThreadRowMessage>
|
||||
*/
|
||||
private function newestFirst(MessageThread $thread): array
|
||||
{
|
||||
$messages = array_values(array_filter(
|
||||
array_reverse($thread->messages->toArray()),
|
||||
array_reverse($this->rows->for($thread)->messages),
|
||||
// A thread can hold a message that has never been flushed — the
|
||||
// compose dock persists a draft into a live thread — and an id-less
|
||||
// one has nothing for `WHERE m.id IN (…)` to find.
|
||||
@@ -211,24 +227,20 @@ final class SearchResultHighlights
|
||||
* while previewing the fragment from the one that actually carries it is
|
||||
* the answer that describes the row best.
|
||||
*
|
||||
* WHERE THE FALLBACK'S TEXT COMES FROM, AND WHY IT IS FREE. When
|
||||
* `ts_headline` marked nothing, SearchHighlighter needs the raw field to
|
||||
* search it itself — and `$message->bodyText` is a plain mapped column that
|
||||
* `preloadMessages()` has already hydrated for every row on the page. So
|
||||
* the fallback reads text this request has held in memory since before this
|
||||
* class was called: no lazy load, no second statement, nothing added to the
|
||||
* ceiling ThreadListQueryBudgetTest enforces.
|
||||
*
|
||||
* Fetching it instead — adding `m.body_text` to findSearchHeadlines() —
|
||||
* would cost no queries either and was still the wrong answer: fifty rows
|
||||
* times five messages is 250 whole bodies pulled a SECOND time, once as a
|
||||
* headline and once raw, on every search page. Free in queries is not free.
|
||||
* WHERE THE FALLBACK'S TEXT COMES FROM. When `ts_headline` marked
|
||||
* nothing, SearchHighlighter needs the raw field to search it itself. The
|
||||
* list no longer hydrates message bodies (see ThreadRows), so forPage()
|
||||
* fetches `body_text` for exactly those unmarked messages through
|
||||
* MessageRepository::findSearchTexts() — not by widening
|
||||
* findSearchHeadlines(), which would pull every candidate's body a second
|
||||
* time, marked or not.
|
||||
*
|
||||
* @param array<int, array{id: int|string, subject: mixed, preview: mixed}> $headlines
|
||||
* @param array<int, mixed> $bodies message id => raw body_text
|
||||
*
|
||||
* @return array{subject: ?Markup, snippet: ?Markup}
|
||||
*/
|
||||
private function forThread(MessageThread $thread, array $headlines, string $freeText): array
|
||||
private function forThread(MessageThread $thread, array $headlines, array $bodies, string $freeText): array
|
||||
{
|
||||
$subject = null;
|
||||
$snippet = null;
|
||||
@@ -241,7 +253,7 @@ final class SearchResultHighlights
|
||||
}
|
||||
|
||||
$snippet ??= $this->highlighter->toMarkup(
|
||||
$this->highlighter->headlineOrFallback($row['preview'], $message->bodyText, $freeText),
|
||||
$this->highlighter->headlineOrFallback($row['preview'], $bodies[(int) $message->id] ?? null, $freeText),
|
||||
);
|
||||
$subject ??= $this->subject($thread, $row['subject'], $freeText);
|
||||
|
||||
|
||||
@@ -6,19 +6,22 @@ namespace App\Twig;
|
||||
|
||||
use App\Entity\Mail\MessageThread;
|
||||
use App\Service\Mail\ThreadParticipants;
|
||||
use App\Service\Mail\ThreadRows;
|
||||
use Symfony\Contracts\Translation\TranslatorInterface;
|
||||
use Twig\Extension\AbstractExtension;
|
||||
use Twig\TwigFunction;
|
||||
|
||||
/**
|
||||
* `thread_participants(thread)` — the names for a conversation row's who
|
||||
* column, already localised.
|
||||
* column, already localised — and `thread_row(thread)`, the rest of what that
|
||||
* row reads from the thread's messages (App\Service\Mail\ThreadRows).
|
||||
*/
|
||||
final class ThreadParticipantsExtension extends AbstractExtension
|
||||
{
|
||||
public function __construct(
|
||||
private readonly ThreadParticipants $participants,
|
||||
private readonly TranslatorInterface $translator,
|
||||
private readonly ThreadRows $rows,
|
||||
) {
|
||||
}
|
||||
|
||||
@@ -26,6 +29,7 @@ final class ThreadParticipantsExtension extends AbstractExtension
|
||||
{
|
||||
return [
|
||||
new TwigFunction('thread_participants', $this->forThread(...)),
|
||||
new TwigFunction('thread_row', $this->rows->for(...)),
|
||||
];
|
||||
}
|
||||
|
||||
@@ -34,6 +38,12 @@ final class ThreadParticipantsExtension extends AbstractExtension
|
||||
*/
|
||||
public function forThread(MessageThread $thread): array
|
||||
{
|
||||
return $this->participants->forThread($thread, $this->translator->trans('thread_row.me'));
|
||||
// The row's fields, not $thread->messages: on a list page those were
|
||||
// never hydrated, and walking them would load every body on the page.
|
||||
return $this->participants->forMessages(
|
||||
$this->rows->for($thread)->messages,
|
||||
$thread->account,
|
||||
$this->translator->trans('thread_row.me'),
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -90,7 +90,12 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
|
||||
{% set date = item.lastMessageAt %}
|
||||
{% set count = item.messageCount %}
|
||||
|
||||
{% set latest = item.messages|last %}
|
||||
{# thread_row(), not item.messages: a list page fetches only the fields a
|
||||
row reads (App\Service\Mail\ThreadRows) rather than every message body
|
||||
on the page. `latest` and `draft` are the collection's |last and its last
|
||||
isDraft, computed in the same order. #}
|
||||
{% set row = thread_row(item) %}
|
||||
{% set latest = row.latest %}
|
||||
{# Everyone who has written in the conversation, oldest first — not the
|
||||
newest sender, which made every thread you had answered look like it
|
||||
came from you. #}
|
||||
@@ -103,22 +108,21 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
|
||||
preview, so the list misreported what the message said. Twig's autoescaping
|
||||
is what makes this safe, and it is also what makes it correct — escaped, the
|
||||
angle brackets are shown; stripped, they were obeyed. #}
|
||||
{# mail_snippet, not bodyText. The text part is whatever the sender put in
|
||||
{# MessageSnippet (made by thread_row), not bodyText. The text part is whatever the sender put in
|
||||
the half of the mail most recipients never see — for bulk senders that is
|
||||
routinely a sentence saying the mail is only available in HTML, and for
|
||||
some it is HTML itself, which arrived in the list as visible `<p
|
||||
style=…>`. The preview is made from the rendered body instead, which is
|
||||
what the reader would actually see. See App\Service\Mail\MessageSnippet. #}
|
||||
{% set snippet = mail_snippet(latest ?: null)|slice(0, 100) %}
|
||||
{% set snippet = row.snippet|slice(0, 100) %}
|
||||
|
||||
{# `latest` comes from |last, which yields FALSE (not null) on an empty
|
||||
collection — so `is not null` passes and .isDraft blows up on a bool.
|
||||
A thread can legitimately end up with no messages (every message
|
||||
{# `latest` and `draftMessage` are null on a thread with no messages, and
|
||||
every use below tests them for truth first. A thread can legitimately end up with no messages (every message
|
||||
expunged provider-side), and this row is rendered for the whole inbox,
|
||||
so one such thread would 500 the entire list. #}
|
||||
{# Not `latest`: the collection is not ordered by date, so the draft is
|
||||
looked up by flag rather than assumed to be the last message. #}
|
||||
{% set draftMessage = item.messages|filter(m => m.isDraft)|last %}
|
||||
{% set draftMessage = row.draft %}
|
||||
{% set draft = draftMessage
|
||||
and (draft_scope or item.messageCount == 1) %}
|
||||
{% set draftId = draft ? draftMessage.id : null %}
|
||||
|
||||
@@ -298,10 +298,10 @@ final class ThreadListQueryBudgetTest extends WebTestCase
|
||||
* The preload has to hand the collection over in the association's own
|
||||
* order, and nothing else in the suite would notice if it did not.
|
||||
*
|
||||
* `{% set latest = item.messages|last %}` is only "the newest message"
|
||||
* while the collection is sorted; #[ORM\OrderBy] guarantees that for a
|
||||
* lazy load and a fetch join does NOT inherit it. Drop the addOrderBy in
|
||||
* preloadMessages() and every row in every list quietly starts previewing
|
||||
* The row's `latest` is only "the newest message" while the row fields
|
||||
* are sorted; #[ORM\OrderBy] guarantees that for a lazy load and a
|
||||
* hand-written query does NOT inherit it. Drop the addOrderBy in
|
||||
* MessageRepository::findRowFieldsForThreads() and every row in every list quietly starts previewing
|
||||
* whichever message Postgres happened to return last — a silent, plausible
|
||||
* wrong answer, which is the worst kind.
|
||||
*/
|
||||
@@ -328,6 +328,26 @@ final class ThreadListQueryBudgetTest extends WebTestCase
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Rows, not just queries: a list page reads a few fields of each message
|
||||
* and must not hydrate the messages themselves.
|
||||
*
|
||||
* The query count cannot see this. The old batch preload was ONE query and
|
||||
* still pulled every body, header blob and search vector of every message
|
||||
* on the page — 175 Message entities for these fixtures, to print one
|
||||
* snippet per row. See App\Service\Mail\ThreadRows.
|
||||
*/
|
||||
public function testAListPageHydratesNoMessageEntity(): void
|
||||
{
|
||||
$this->seedPage(MessageCategory::Promotions);
|
||||
|
||||
$this->queriesFor('/mail/inbox?tab=promotions');
|
||||
|
||||
$map = $this->em->getUnitOfWork()->getIdentityMap();
|
||||
|
||||
self::assertCount(0, $map[Message::class] ?? [], 'the list hydrated whole messages again');
|
||||
}
|
||||
|
||||
// ── fixtures ──────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user