diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php index f63c28f9b6..1116a75845 100644 --- a/lib/IMAP/MessageMapper.php +++ b/lib/IMAP/MessageMapper.php @@ -890,15 +890,14 @@ public function getBodyStructureData(Horde_Imap_Client_Socket $client, $structures = $client->fetch($mailbox, $structureQuery, [ 'ids' => new Horde_Imap_Client_Ids($uids), ]); - return array_map(function (Horde_Imap_Client_Data_Fetch $fetchData) use ($mailbox, $client, $emailAddress) { + + $perMessage = []; + $bodyPartIds = []; + foreach ($structures as $fetchData) { + /** @var Horde_Imap_Client_Data_Fetch $fetchData */ $hasAttachments = false; - $text = ''; $isImipMessage = false; - $isEncrypted = false; - - if ($this->smimeService->isEncrypted($fetchData)) { - $isEncrypted = true; - } + $isEncrypted = $this->smimeService->isEncrypted($fetchData); $structure = $fetchData->getStructure(); @@ -917,34 +916,64 @@ public function getBodyStructureData(Horde_Imap_Client_Socket $client, $textBodyId = $structure->findBody() ?? $structure->findBody('text'); $htmlBodyId = $structure->findBody('html'); - if ($textBodyId === null && $htmlBodyId === null) { - return new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); - } - $partsQuery = new Horde_Imap_Client_Fetch_Query(); + if ($htmlBodyId !== null) { - $partsQuery->bodyPart($htmlBodyId, [ - 'peek' => true, - ]); - $partsQuery->mimeHeader($htmlBodyId, [ - 'peek' => true - ]); + $bodyPartIds[$htmlBodyId] = true; } if ($textBodyId !== null) { - $partsQuery->bodyPart($textBodyId, [ + $bodyPartIds[$textBodyId] = true; + } + + $perMessage[$fetchData->getUid()] = [ + 'fetchData' => $fetchData, + 'structure' => $structure, + 'hasAttachments' => $hasAttachments, + 'isImipMessage' => $isImipMessage, + 'isEncrypted' => $isEncrypted, + 'textBodyId' => $textBodyId, + 'htmlBodyId' => $htmlBodyId, + ]; + } + + $parts = null; + if ($bodyPartIds !== []) { + $partsQuery = new Horde_Imap_Client_Fetch_Query(); + foreach (array_keys($bodyPartIds) as $bodyPartId) { + $partsQuery->bodyPart($bodyPartId, [ 'peek' => true, ]); - $partsQuery->mimeHeader($textBodyId, [ - 'peek' => true + $partsQuery->mimeHeader($bodyPartId, [ + 'peek' => true, ]); } $parts = $client->fetch($mailbox, $partsQuery, [ - 'ids' => new Horde_Imap_Client_Ids([$fetchData->getUid()]), + 'ids' => new Horde_Imap_Client_Ids($uids), ]); - /** @var Horde_Imap_Client_Data_Fetch $part */ - $part = $parts[$fetchData->getUid()]; + } + + $result = []; + foreach ($perMessage as $uid => $message) { + /** @var Horde_Imap_Client_Data_Fetch $fetchData */ + $fetchData = $message['fetchData']; + $structure = $message['structure']; + $hasAttachments = $message['hasAttachments']; + $isImipMessage = $message['isImipMessage']; + $isEncrypted = $message['isEncrypted']; + $textBodyId = $message['textBodyId']; + $htmlBodyId = $message['htmlBodyId']; + $text = ''; + + if ($textBodyId === null && $htmlBodyId === null) { + $result[$uid] = new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); + continue; + } + + /** @var Horde_Imap_Client_Data_Fetch|null $part */ + $part = $parts !== null ? $parts[$uid] : null; // This is sus - why does this even happen? A delete / move in the middle of this processing? if ($part === null) { - return new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); + $result[$uid] = new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); + continue; } /** Convert a given binary body to utf-8 according to the applicable @@ -986,34 +1015,36 @@ public function getBodyStructureData(Horde_Imap_Client_Socket $client, return $this->converter->convert($structure); }; - $htmlBody = ($htmlBodyId !== null) ? $part->getBodyPart($htmlBodyId) : null; if (!empty($htmlBody)) { $htmlBody = $convertBody($htmlBodyId, $htmlBody); $mentionsUser = $this->checkLinks($htmlBody, $emailAddress); $html = new Html2Text($htmlBody, ['do_links' => 'none','alt_image' => 'hide']); - return new MessageStructureData( + $result[$uid] = new MessageStructureData( $hasAttachments, trim($html->getText()), $isImipMessage, $isEncrypted, $mentionsUser, ); + continue; } - $textBody = $part->getBodyPart($textBodyId); + $textBody = $textBodyId !== null ? $part->getBodyPart($textBodyId) : null; if (!empty($textBody)) { $textBody = $convertBody($textBodyId, $textBody); - return new MessageStructureData( + $result[$uid] = new MessageStructureData( $hasAttachments, $textBody, $isImipMessage, $isEncrypted, false, ); + continue; } - return new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); - }, iterator_to_array($structures->getIterator())); + $result[$uid] = new MessageStructureData($hasAttachments, $text, $isImipMessage, $isEncrypted, false); + } + return $result; } private function checkLinks(string $body, string $mailAddress) : bool { if (empty($body)) { diff --git a/tests/Unit/IMAP/MessageMapperTest.php b/tests/Unit/IMAP/MessageMapperTest.php index 864bcca156..17eec16403 100644 --- a/tests/Unit/IMAP/MessageMapperTest.php +++ b/tests/Unit/IMAP/MessageMapperTest.php @@ -745,6 +745,53 @@ public function testGetBodyStructureUsesMessageHeaderFallback(): void { $this->assertEquals($bodyText, $data[$messageId]->getPreviewText()); } + /** + * The body parts of every message in the batch must be retrieved with a + * single combined fetch instead of one fetch per message (no N+1). For any + * number of messages this means exactly two fetches: one for the + * structures and one for the union of all body parts. + * + * @throws Horde_Imap_Client_Exception + */ + public function testGetBodyStructureDataBatchesBodyPartFetches(): void { + $uids = [100, 200, 300]; + $structureResult = new Horde_Imap_Client_Fetch_Results(); + $partsResult = new Horde_Imap_Client_Fetch_Results(); + foreach ($uids as $uid) { + $part = new Horde_Mime_Part(); + $header = $part->addMimeHeaders(); + $header->addHeader('content-type', 'text/plain; charset=us-ascii'); + + $fetchData = new Horde_Imap_Client_Data_Fetch(); + $fetchData->setStructure($part); + $fetchData->setHeaderText('0', "Content-Transfer-Encoding: base64\r\n"); + $fetchData->setBodyPart('1', base64_encode('preview ' . $uid)); + $fetchData->setUid($uid); + + $structureResult[$uid] = $fetchData; + $partsResult[$uid] = $fetchData; + } + + $imapClient = $this->createMock(Horde_Imap_Client_Socket::class); + $imapClient->expects(self::exactly(2)) + ->method('fetch') + ->willReturnOnConsecutiveCalls($structureResult, $partsResult); + $this->converter->method('convert') + ->willReturnCallback(static fn (Horde_Mime_Part $part): string => $part->getContents()); + + $data = $this->mapper->getBodyStructureData( + $imapClient, + 'INBOX', + $uids, + 'alice@example.org', + ); + + $this->assertCount(3, $data); + $this->assertEquals('preview 100', $data[100]->getPreviewText()); + $this->assertEquals('preview 200', $data[200]->getPreviewText()); + $this->assertEquals('preview 300', $data[300]->getPreviewText()); + } + public function isImipMessageProvider(): array { return [ 'google request' => ['request_google', true],