Skip to content

Commit e20db62

Browse files
SebastianKrupinskiSebastianKrupinski
committed
fixup! feat: jmap support - part 2
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>
1 parent 445ea19 commit e20db62

2 files changed

Lines changed: 246 additions & 51 deletions

File tree

‎lib/IMAP/ImapMessageConnector.php‎

Lines changed: 63 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -186,38 +186,40 @@ public function moveMessages(Account $account, Mailbox $targetMailbox, Mailbox $
186186
$client = $this->protocolFactory->imapClient($account);
187187

188188
$mutatedMessages = [];
189-
foreach ($messages as $message) {
190-
try {
191-
$newUid = $this->imapMessageMapper->move($client, $sourceMailbox->getName(), $message->getUid(), $targetMailbox->getName());
192-
if ($newUid === null) {
193-
// The IMAP server does not support UIDPLUS and the message has no Message-ID
194-
// header, so the new UID is unknown. It will be reconciled on the next sync.
195-
$this->logger->debug('Moved message but could not determine its new UID', [
189+
try {
190+
foreach ($messages as $message) {
191+
try {
192+
$newUid = $this->imapMessageMapper->move($client, $sourceMailbox->getName(), $message->getUid(), $targetMailbox->getName());
193+
if ($newUid === null) {
194+
// The IMAP server does not support UIDPLUS and the message has no Message-ID
195+
// header, so the new UID is unknown. It will be reconciled on the next sync.
196+
$this->logger->debug('Moved message but could not determine its new UID', [
197+
'userId' => $account->getUserId(),
198+
'accountId' => $account->getId(),
199+
'sourceMailboxId' => $sourceMailbox->getId(),
200+
'targetMailboxId' => $targetMailbox->getId(),
201+
'messageUid' => $message->getUid(),
202+
]);
203+
continue;
204+
}
205+
$message->setUid($newUid);
206+
$message->setMailboxId($targetMailbox->getId());
207+
$mutatedMessages[] = $message;
208+
} catch (Horde_Imap_Client_Exception $e) {
209+
$this->logger->error('Could not move message on remote IMAP server', [
210+
'exception' => $e,
196211
'userId' => $account->getUserId(),
197212
'accountId' => $account->getId(),
198213
'sourceMailboxId' => $sourceMailbox->getId(),
199214
'targetMailboxId' => $targetMailbox->getId(),
200215
'messageUid' => $message->getUid(),
201216
]);
202-
continue;
203217
}
204-
$message->setUid($newUid);
205-
$message->setMailboxId($targetMailbox->getId());
206-
$mutatedMessages[] = $message;
207-
} catch (Horde_Imap_Client_Exception $e) {
208-
$this->logger->error('Could not move message on remote IMAP server', [
209-
'exception' => $e,
210-
'userId' => $account->getUserId(),
211-
'accountId' => $account->getId(),
212-
'sourceMailboxId' => $sourceMailbox->getId(),
213-
'targetMailboxId' => $targetMailbox->getId(),
214-
'messageUid' => $message->getUid(),
215-
]);
216218
}
219+
} finally {
220+
$client->logout();
217221
}
218222

219-
$client->logout();
220-
221223
return $mutatedMessages;
222224
}
223225

@@ -229,23 +231,25 @@ public function deleteMessages(Account $account, Mailbox $mailbox, Message ...$m
229231
$client = $this->protocolFactory->imapClient($account);
230232

231233
$mutatedMessages = [];
232-
foreach ($messages as $message) {
233-
try {
234-
$this->imapMessageMapper->expunge($client, $mailbox->getName(), $message->getUid());
235-
$mutatedMessages[] = $message;
236-
} catch (Horde_Imap_Client_Exception $e) {
237-
$this->logger->error('Could not delete message on remote IMAP server', [
238-
'exception' => $e,
239-
'userId' => $account->getUserId(),
240-
'accountId' => $account->getId(),
241-
'mailboxId' => $mailbox->getId(),
242-
'messageUid' => $message->getUid(),
243-
]);
234+
try {
235+
foreach ($messages as $message) {
236+
try {
237+
$this->imapMessageMapper->expunge($client, $mailbox->getName(), $message->getUid());
238+
$mutatedMessages[] = $message;
239+
} catch (Horde_Imap_Client_Exception $e) {
240+
$this->logger->error('Could not delete message on remote IMAP server', [
241+
'exception' => $e,
242+
'userId' => $account->getUserId(),
243+
'accountId' => $account->getId(),
244+
'mailboxId' => $mailbox->getId(),
245+
'messageUid' => $message->getUid(),
246+
]);
247+
}
244248
}
249+
} finally {
250+
$client->logout();
245251
}
246252

247-
$client->logout();
248-
249253
return $mutatedMessages;
250254
}
251255

@@ -276,10 +280,10 @@ public function flagMessages(Account $account, Mailbox $mailbox, string $flag, b
276280
}
277281
} catch (Horde_Imap_Client_Exception $e) {
278282
throw new ServiceException('Could not set message flag on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e);
283+
} finally {
284+
$client->logout();
279285
}
280286

281-
$client->logout();
282-
283287
return $messages;
284288
}
285289

@@ -290,20 +294,24 @@ public function tagMessages(Account $account, Mailbox $mailbox, Tag $tag, bool $
290294
}
291295
$client = $this->protocolFactory->imapClient($account);
292296

293-
if ($this->isPermflagsEnabledWithClient($client, $mailbox->getName()) === false) {
294-
$this->logger->error('Cannot set message keyword, server does not support permanent flags', ['tag' => $tag->getDisplayName()]);
295-
return [];
296-
}
297-
298-
$uids = array_map(static fn (Message $message) => $message->getUid(), $messages);
299297
try {
300-
if ($value) {
301-
$this->imapMessageMapper->addFlag($client, $mailbox, $uids, $tag->getImapLabel());
302-
} else {
303-
$this->imapMessageMapper->removeFlag($client, $mailbox, $uids, $tag->getImapLabel());
298+
if ($this->isPermflagsEnabledWithClient($client, $mailbox->getName()) === false) {
299+
$this->logger->error('Cannot set message keyword, server does not support permanent flags', ['tag' => $tag->getDisplayName()]);
300+
return [];
304301
}
305-
} catch (Horde_Imap_Client_Exception $e) {
306-
throw new ServiceException('Could not set message keyword on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e);
302+
303+
$uids = array_map(static fn (Message $message) => $message->getUid(), $messages);
304+
try {
305+
if ($value) {
306+
$this->imapMessageMapper->addFlag($client, $mailbox, $uids, $tag->getImapLabel());
307+
} else {
308+
$this->imapMessageMapper->removeFlag($client, $mailbox, $uids, $tag->getImapLabel());
309+
}
310+
} catch (Horde_Imap_Client_Exception $e) {
311+
throw new ServiceException('Could not set message keyword on remote IMAP server: ' . $e->getMessage(), $e->getCode(), $e);
312+
}
313+
} finally {
314+
$client->logout();
307315
}
308316

309317
foreach ($messages as $message) {
@@ -359,7 +367,11 @@ public function repairSync(Account $account, Mailbox $mailbox): void {
359367
#[\Override]
360368
public function isPermflagsEnabled(Account $account, Mailbox $mailbox): bool {
361369
$client = $this->protocolFactory->imapClient($account);
362-
return $this->isPermflagsEnabledWithClient($client, $mailbox->getName());
370+
try {
371+
return $this->isPermflagsEnabledWithClient($client, $mailbox->getName());
372+
} finally {
373+
$client->logout();
374+
}
363375
}
364376

365377
private function isPermflagsEnabledWithClient($client, string $mailbox): bool {

‎tests/Unit/IMAP/ImapMessageConnectorTest.php‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Mail\Tests\Unit\IMAP;
11+
12+
use ChristophWurst\Nextcloud\Testing\TestCase;
13+
use Horde_Imap_Client_Exception;
14+
use Horde_Imap_Client_Socket;
15+
use OCA\Mail\Account;
16+
use OCA\Mail\Db\Mailbox;
17+
use OCA\Mail\Db\Message;
18+
use OCA\Mail\Db\Tag;
19+
use OCA\Mail\Exception\ServiceException;
20+
use OCA\Mail\IMAP\FolderMapper;
21+
use OCA\Mail\IMAP\ImapMessageConnector;
22+
use OCA\Mail\IMAP\MessageMapper;
23+
use OCA\Mail\Protocol\ProtocolFactory;
24+
use OCA\Mail\Service\Sync\ImapToDbSynchronizer;
25+
use PHPUnit\Framework\MockObject\MockObject;
26+
use Psr\Log\LoggerInterface;
27+
28+
/**
29+
* Every connector method that opens an IMAP client via ProtocolFactory::imapClient()
30+
* must log it out again even when the underlying operation fails, otherwise the
31+
* connection to the mail server is leaked (see moveMessages/deleteMessages/flagMessages/
32+
* tagMessages/isPermflagsEnabled).
33+
*/
34+
class ImapMessageConnectorTest extends TestCase {
35+
private ProtocolFactory&MockObject $protocolFactory;
36+
private ImapToDbSynchronizer&MockObject $synchronizer;
37+
private FolderMapper&MockObject $imapMailboxMapper;
38+
private MessageMapper&MockObject $imapMessageMapper;
39+
private LoggerInterface&MockObject $logger;
40+
private ImapMessageConnector $connector;
41+
private Account&MockObject $account;
42+
private Horde_Imap_Client_Socket&MockObject $client;
43+
44+
protected function setUp(): void {
45+
parent::setUp();
46+
47+
$this->protocolFactory = $this->createMock(ProtocolFactory::class);
48+
$this->synchronizer = $this->createMock(ImapToDbSynchronizer::class);
49+
$this->imapMailboxMapper = $this->createMock(FolderMapper::class);
50+
$this->imapMessageMapper = $this->createMock(MessageMapper::class);
51+
$this->logger = $this->createMock(LoggerInterface::class);
52+
53+
$this->connector = new ImapMessageConnector(
54+
$this->protocolFactory,
55+
$this->synchronizer,
56+
$this->imapMailboxMapper,
57+
$this->imapMessageMapper,
58+
$this->logger,
59+
);
60+
61+
$this->account = $this->createMock(Account::class);
62+
$this->client = $this->createMock(Horde_Imap_Client_Socket::class);
63+
$this->protocolFactory->method('imapClient')
64+
->with($this->account)
65+
->willReturn($this->client);
66+
}
67+
68+
public function testMoveMessagesLogsOutClientWhenMapperThrows(): void {
69+
$sourceMailbox = new Mailbox();
70+
$sourceMailbox->setName('INBOX');
71+
$targetMailbox = new Mailbox();
72+
$targetMailbox->setName('Archive');
73+
$message = new Message();
74+
$message->setUid(1);
75+
76+
$this->imapMessageMapper->method('move')
77+
->willThrowException(new ServiceException('could not move'));
78+
$this->client->expects(self::once())
79+
->method('logout');
80+
81+
$this->expectException(ServiceException::class);
82+
83+
$this->connector->moveMessages($this->account, $targetMailbox, $sourceMailbox, $message);
84+
}
85+
86+
public function testDeleteMessagesLogsOutClientWhenMapperThrows(): void {
87+
$mailbox = new Mailbox();
88+
$mailbox->setName('INBOX');
89+
$message = new Message();
90+
$message->setUid(1);
91+
92+
$this->imapMessageMapper->method('expunge')
93+
->willThrowException(new ServiceException('could not expunge'));
94+
$this->client->expects(self::once())
95+
->method('logout');
96+
97+
$this->expectException(ServiceException::class);
98+
99+
$this->connector->deleteMessages($this->account, $mailbox, $message);
100+
}
101+
102+
public function testFlagMessagesLogsOutClientWhenMapperThrows(): void {
103+
$mailbox = new Mailbox();
104+
$mailbox->setName('INBOX');
105+
$message = new Message();
106+
$message->setUid(1);
107+
108+
$this->imapMessageMapper->method('addFlag')
109+
->willThrowException(new Horde_Imap_Client_Exception('store failed'));
110+
$this->client->expects(self::once())
111+
->method('logout');
112+
113+
$this->expectException(ServiceException::class);
114+
115+
$this->connector->flagMessages($this->account, $mailbox, 'seen', true, $message);
116+
}
117+
118+
public function testTagMessagesLogsOutClientWhenPermflagsCheckThrows(): void {
119+
$mailbox = new Mailbox();
120+
$mailbox->setName('INBOX');
121+
$tag = new Tag();
122+
$tag->setDisplayName('Important');
123+
$tag->setImapLabel('$important');
124+
$message = new Message();
125+
$message->setUid(1);
126+
127+
$this->client->method('status')
128+
->willThrowException(new Horde_Imap_Client_Exception('status failed'));
129+
$this->client->expects(self::once())
130+
->method('logout');
131+
132+
$this->expectException(ServiceException::class);
133+
134+
$this->connector->tagMessages($this->account, $mailbox, $tag, true, $message);
135+
}
136+
137+
public function testTagMessagesLogsOutClientWhenPermflagsNotSupported(): void {
138+
$mailbox = new Mailbox();
139+
$mailbox->setName('INBOX');
140+
$tag = new Tag();
141+
$tag->setDisplayName('Important');
142+
$tag->setImapLabel('$important');
143+
$message = new Message();
144+
$message->setUid(1);
145+
146+
$this->client->method('status')
147+
->willReturn(['permflags' => []]);
148+
$this->client->expects(self::once())
149+
->method('logout');
150+
151+
$result = $this->connector->tagMessages($this->account, $mailbox, $tag, true, $message);
152+
153+
self::assertSame([], $result);
154+
}
155+
156+
public function testIsPermflagsEnabledLogsOutClientWhenStatusThrows(): void {
157+
$mailbox = new Mailbox();
158+
$mailbox->setName('INBOX');
159+
160+
$this->client->method('status')
161+
->willThrowException(new Horde_Imap_Client_Exception('status failed'));
162+
$this->client->expects(self::once())
163+
->method('logout');
164+
165+
$this->expectException(ServiceException::class);
166+
167+
$this->connector->isPermflagsEnabled($this->account, $mailbox);
168+
}
169+
170+
public function testIsPermflagsEnabledLogsOutClientOnSuccess(): void {
171+
$mailbox = new Mailbox();
172+
$mailbox->setName('INBOX');
173+
174+
$this->client->method('status')
175+
->willReturn(['permflags' => ['\\*']]);
176+
$this->client->expects(self::once())
177+
->method('logout');
178+
179+
$result = $this->connector->isPermflagsEnabled($this->account, $mailbox);
180+
181+
self::assertTrue($result);
182+
}
183+
}

0 commit comments

Comments
 (0)