Skip to content

perf: reduce number of avatar requests #10486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/Contracts/IAvatarService.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,6 @@ public function getAvatar(string $email, string $uid): ?Avatar;
* @return array|null image data
*/
public function getAvatarImage(string $email, string $uid);

public function getCachedAvatar(string $email, string $uid): Avatar|false|null;
}
2 changes: 2 additions & 0 deletions lib/Contracts/IMailSearch.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public function findMessage(Account $account,
* @param string|null $filter
* @param int|null $cursor
* @param int|null $limit
* @param string|null $userId
* @param string|null $view
*
* @return Message[]
Expand All @@ -52,6 +53,7 @@ public function findMessages(Account $account,
?string $filter,
?int $cursor,
?int $limit,
?string $userId,
?string $view): array;

/**
Expand Down
1 change: 1 addition & 0 deletions lib/Controller/MessagesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
$filter === '' ? null : $filter,
$cursor,
$limit,
$this->currentUserId,

Check warning on line 159 in lib/Controller/MessagesController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/MessagesController.php#L159

Added line #L159 was not covered by tests
$view
);
return new JSONResponse(
Expand Down
27 changes: 27 additions & 0 deletions lib/Db/Message.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Horde_Mail_Rfc822_Identification;
use JsonSerializable;
use OCA\Mail\AddressList;
use OCA\Mail\Service\Avatar\Avatar;
use OCP\AppFramework\Db\Entity;
use ReturnTypeWillChange;
use function in_array;
Expand Down Expand Up @@ -136,6 +137,12 @@ class Message extends Entity implements JsonSerializable {
/** @var Tag[] */
private $tags = [];

/** @var Avatar|null */
private $avatar;

/** @var bool */
private $fetchAvatarFromClient = false;

public function __construct() {
$this->from = new AddressList([]);
$this->to = new AddressList([]);
Expand Down Expand Up @@ -286,6 +293,24 @@ public function setFlag(string $flag, bool $value = true) {
);
}
}
/**
* @param Avatar|null $avatar
* @return void
*/
public function setAvatar(?Avatar $avatar): void {
$this->avatar = $avatar;
}

public function setFetchAvatarFromClient(bool $fetchAvatarFromClient): void {
$this->fetchAvatarFromClient = $fetchAvatarFromClient;
}

/**
* @return ?Avatar
*/
public function getAvatar(): ?Avatar {
return $this->avatar;
}

#[\Override]
#[ReturnTypeWillChange]
Expand Down Expand Up @@ -332,6 +357,8 @@ static function (Tag $tag) {
'summary' => $this->getSummary(),
'encrypted' => ($this->isEncrypted() === true),
'mentionsMe' => $this->getMentionsMe(),
'avatar' => $this->avatar?->jsonSerialize(),
'fetchAvatarFromClient' => $this->fetchAvatarFromClient,
];
}
}
27 changes: 25 additions & 2 deletions lib/IMAP/PreviewEnhancer.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
use OCA\Mail\Db\Message;
use OCA\Mail\Db\MessageMapper as DbMapper;
use OCA\Mail\IMAP\MessageMapper as ImapMapper;
use OCA\Mail\Service\Avatar\Avatar;
use OCA\Mail\Service\AvatarService;
use Psr\Log\LoggerInterface;
use function array_key_exists;
use function array_map;
Expand All @@ -34,22 +36,27 @@ class PreviewEnhancer {
/** @var LoggerInterface */
private $logger;

/** @var AvatarService */
private $avatarService;

public function __construct(IMAPClientFactory $clientFactory,
ImapMapper $imapMapper,
DbMapper $dbMapper,
LoggerInterface $logger) {
LoggerInterface $logger,
AvatarService $avatarService) {
$this->clientFactory = $clientFactory;
$this->imapMapper = $imapMapper;
$this->mapper = $dbMapper;
$this->logger = $logger;
$this->avatarService = $avatarService;
}

/**
* @param Message[] $messages
*
* @return Message[]
*/
public function process(Account $account, Mailbox $mailbox, array $messages): array {
public function process(Account $account, Mailbox $mailbox, array $messages, bool $preLoadAvatars = false, ?string $userId = null): array {
$needAnalyze = array_reduce($messages, static function (array $carry, Message $message) {
if ($message->getStructureAnalyzed()) {
// Nothing to do
Expand All @@ -59,6 +66,22 @@ public function process(Account $account, Mailbox $mailbox, array $messages): ar
return array_merge($carry, [$message->getUid()]);
}, []);

if ($preLoadAvatars) {
foreach ($messages as $message) {
$from = $message->getFrom()->first();
if ($message->getAvatar() === null && $from !== null && $from->getEmail() !== null && $userId !== null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: negate the condition and use continue to skip one loop iteration. That avoids at least one level of indentation in this deep indentation tree.

$avatar = $this->avatarService->getCachedAvatar($from->getEmail(), $userId);
if ($avatar === null) {
$message->setFetchAvatarFromClient(true);
}
if ($avatar instanceof Avatar) {
$message->setAvatar($avatar);
}

}
}
}

if ($needAnalyze === []) {
// Nothing to enhance
return $messages;
Expand Down
4 changes: 4 additions & 0 deletions lib/Service/AvatarService.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@
}
}

public function getCachedAvatar(string $email, string $uid): Avatar|false|null {
return $this->cache->get($email, $uid);

Check warning on line 90 in lib/Service/AvatarService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/AvatarService.php#L89-L90

Added lines #L89 - L90 were not covered by tests
}

/**
* @param string $email
* @param string $uid
Expand Down
5 changes: 4 additions & 1 deletion lib/Service/Search/MailSearch.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public function findMessages(Account $account,
?string $filter,
?int $cursor,
?int $limit,
?string $userId,
?string $view): array {
if ($mailbox->hasLocks($this->timeFactory->getTime())) {
throw MailboxLockedException::from($mailbox);
Expand Down Expand Up @@ -125,7 +126,9 @@ public function findMessages(Account $account,
$this->messageMapper->findByIds($account->getUserId(),
$this->getIdsLocally($account, $mailbox, $query, $sortOrder, $limit),
$sortOrder,
)
),
true,
$userId
);
}

Expand Down
28 changes: 22 additions & 6 deletions src/components/Avatar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

<script>
import NcAvatar from '@nextcloud/vue/components/NcAvatar'
import { generateUrl } from '@nextcloud/router'
import { fetchAvatarUrlMemoized } from '../service/AvatarService.js'
import logger from '../logger.js'

Expand All @@ -30,6 +31,14 @@ export default {
type: String,
required: true,
},
avatar: {
type: Object,
default: null,
},
fetchAvatar: {
type: Boolean,
default: false,
},
email: {
type: String,
required: true,
Expand All @@ -55,14 +64,21 @@ export default {
},
},
async mounted() {
if (this.email !== '') {
try {
this.avatarUrl = await fetchAvatarUrlMemoized(this.email)
} catch {
logger.debug('Could not fetch avatar', { email: this.email })
if (this.avatar) {
this.avatarUrl = this.avatar.isExternal
? generateUrl('/apps/mail/api/avatars/image/{email}', {
email: this.email,
})
: this.avatar.url
} else if (this.fetchAvatar) {
if (this.email !== '') {
try {
this.avatarUrl = await fetchAvatarUrlMemoized(this.email)
} catch {
logger.debug('Could not fetch avatar', { email: this.email })
}
}
}

this.loading = false
},
}
Expand Down
5 changes: 4 additions & 1 deletion src/components/Envelope.vue
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
:class="{ 'important-one-line': oneLineLayout, 'icon-important': !oneLineLayout }"
:data-starred="isImportant ? 'true' : 'false'"
@click.prevent="hasWriteAcl ? onToggleImportant() : false"
v-html="importantSvg" />

Check warning on line 39 in src/components/Envelope.vue

View workflow job for this annotation

GitHub Actions / NPM lint

'v-html' directive can lead to XSS attack
<JunkIcon v-if="data.flags.$junk"
:size="18"
class="app-content-list-item-star junk-icon-style"
Expand All @@ -53,7 +53,10 @@
<CheckIcon :size="40" class="check-icon" :class="{ 'app-content-list-item-avatar-selected': selected }" />
</template>
<template v-else>
<Avatar :display-name="addresses" :email="avatarEmail" />
<Avatar :display-name="addresses"
:email="avatarEmail"
:fetch-avatar="data.fetchAvatarFromClient"
:avatar="data.avatar" />
</template>
</div>
</template>
Expand Down Expand Up @@ -497,7 +500,7 @@

window.addEventListener('resize', this.onWindowResize)
},
computed: {

Check warning on line 503 in src/components/Envelope.vue

View workflow job for this annotation

GitHub Actions / NPM lint

The "computed" property should be above the "mounted" property on line 498
...mapStores(useMainStore),
...mapState(useMainStore, [
'isSnoozeDisabled',
Expand Down
5 changes: 4 additions & 1 deletion src/components/OutboxMessageListItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
:details="details"
@click="openModal">
<template #icon>
<Avatar :display-name="avatarDisplayName" :email="avatarEmail" />
<Avatar :display-name="avatarDisplayName"
:email="avatarEmail"
:fetch-avatar="data.fetchAvatarFromClient"
:avatar="message.avatar" />
</template>
<template #subname>
{{ subjectForSubtitle }}
Expand Down
2 changes: 2 additions & 0 deletions src/components/ThreadEnvelope.vue
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
:display-name="envelope.from[0].label"
:disable-tooltip="true"
:size="40"
:fetch-avatar="envelope.fetchAvatarFromClient"
:avatar="envelope.avatar"
class="envelope__header__avatar-avatar" />
<div v-if="isImportant"
class="app-content-list-item-star icon-important"
Expand Down
22 changes: 22 additions & 0 deletions tests/Integration/Db/MessageTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use ChristophWurst\Nextcloud\Testing\TestCase;
use OCA\Mail\Db\Message;
use OCA\Mail\Service\Avatar\Avatar;

class MessageTest extends TestCase {
protected function setUp(): void {
Expand Down Expand Up @@ -109,4 +110,25 @@ public function testSetInReplyToEmpty(): void {
$this->assertEquals($expected, $message->getThreadRootId());
$this->assertNull($message->getInReplyTo());
}

public function testSetAvatar(): void {
$expected = new Avatar(
'http://example.com/avatar.png',
'image/png',
true
);
$message = new Message();

$message->setAvatar($expected);

$this->assertEquals($expected, $message->getAvatar());
}

public function testSetFetchAvatarFromClient(): void {
$message = new Message();

$message->setFetchAvatarFromClient(true);

$this->assertTrue($message->jsonSerialize()['fetchAvatarFromClient']);
}
}
93 changes: 93 additions & 0 deletions tests/Unit/IMAP/PreviewEnhancerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace Unit\IMAP;

use ChristophWurst\Nextcloud\Testing\TestCase;
use OCA\Mail\Address;
use OCA\Mail\AddressList;
use OCA\Mail\Db\Message;
use OCA\Mail\Db\MessageMapper as DbMapper;
use OCA\Mail\IMAP\IMAPClientFactory;
use OCA\Mail\IMAP\MessageMapper as ImapMapper;
use OCA\Mail\IMAP\PreviewEnhancer;
use OCA\Mail\Service\Avatar\Avatar;
use OCA\Mail\Service\AvatarService;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;

class PreviewEnhancerTest extends TestCase {


/** @var IMAPClientFactory|MockObject */
private $imapClientFactory;
/** @var ImapMapper|MockObject */
private $imapMapper;
/** @var DbMapper|MockObject */
private $dbMapper;
/** @var LoggerInterface|MockObject */
private $logger;
/** @var AvatarService|MockObject */
private $avatarService;
/** @var PreviewEnhancer */
private $previewEnhancer;

protected function setUp(): void {
parent::setUp();

$this->imapClientFactory = $this->createMock(IMAPClientFactory::class);
$this->imapMapper = $this->createMock(ImapMapper::class);
$this->dbMapper = $this->createMock(DbMapper::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->avatarService = $this->createMock(AvatarService::class);

$this->previewEnhancer = new previewEnhancer($this->imapClientFactory,
$this->imapMapper,
$this->dbMapper,
$this->logger,
$this->avatarService);
}

public function testAvatars(): void {

$message1 = new Message();
$message1->setId(1);
$message1->setStructureAnalyzed(true);
$message1->setFrom(new AddressList([Address::fromRaw('Alice', 'alice@example.com')]));
$message2 = new Message();
$message2->setId(2);
$message2->setStructureAnalyzed(true);
$message2->setFrom(new AddressList([Address::fromRaw('Bob', 'bob@example.com')]));
$messages = [$message1, $message2];
$message2Avatar = new Avatar('example.com', 'image/png', true);
$this->avatarService->expects($this->exactly(2))
->method('getCachedAvatar')
->withConsecutive(
['alice@example.com', 'testuser'],
['bob@example.com', 'testuser']
)
->willReturnOnConsecutiveCalls(
null,
$message2Avatar
);
$this->previewEnhancer->process(
$this->createMock(\OCA\Mail\Account::class),
$this->createMock(\OCA\Mail\Db\Mailbox::class),
$messages,
true,
'testuser'
);
$this->assertTrue($message1->jsonSerialize()['fetchAvatarFromClient']);
$this->assertFalse($message2->jsonSerialize()['fetchAvatarFromClient']);
$this->assertNull($message1->getAvatar());
$this->assertSame($message2Avatar, $message2->getAvatar());
}


}
Loading
Loading