Remove unneccesary client call from outbox

The  login is slow. In case we don't have to handle
attachments, we should skip calling the client factory entirely to speed up
the processing of outbox messages

This is a mini performance optimisation.

Signed-off-by: Anna Larch <anna@nextcloud.com>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

2 files changed, 78 insertions, 13 deletions

diff --git a/lib/Service/OutboxService.php b/lib/Service/OutboxService.php
index 991f8968b..3b4454073 100644
--- a/lib/Service/OutboxService.php
+++ b/lib/Service/OutboxService.php
@@ -145,6 +145,11 @@ class OutboxService implements ILocalMailboxService {
 		$bccRecipients = self::convertToRecipient($bcc, Recipient::TYPE_BCC);
 		$message = $this->mapper->saveWithRecipients($message, $toRecipients, $ccRecipients, $bccRecipients);
 
+		if (empty($attachments)) {
+			$message->setAttachments($attachments);
+			return $message;
+		}
+
 		$client = $this->clientFactory->getClient($account);
 		try {
 			$attachmentIds = $this->attachmentService->handleAttachments($account, $attachments, $client);
@@ -162,6 +167,11 @@ class OutboxService implements ILocalMailboxService {
 		$bccRecipients = self::convertToRecipient($bcc, Recipient::TYPE_BCC);
 		$message = $this->mapper->updateWithRecipients($message, $toRecipients, $ccRecipients, $bccRecipients);
 
+		if (empty($attachments)) {
+			$message->setAttachments($this->attachmentService->updateLocalMessageAttachments($account->getUserId(), $message, []));
+			return $message;
+		}
+
 		$client = $this->clientFactory->getClient($account);
 		try {
 			$attachmentIds = $this->attachmentService->handleAttachments($account, $attachments, $client);
diff --git a/tests/Unit/Service/OutboxServiceTest.php b/tests/Unit/Service/OutboxServiceTest.php
index f077b9f77..f5262b157 100644
--- a/tests/Unit/Service/OutboxServiceTest.php
+++ b/tests/Unit/Service/OutboxServiceTest.php
@@ -248,6 +248,51 @@ class OutboxServiceTest extends TestCase {
 		$this->outboxService->saveMessage($account, $message, $to, $cc, $bcc, $attachments);
 	}
 
+	public function testSaveMessageNoAttachments(): void {
+		$message = new LocalMessage();
+		$message->setAccountId(1);
+		$message->setSendAt($this->time->getTime());
+		$message->setSubject('Test');
+		$message->setBody('Test Test Test');
+		$message->setHtml(true);
+		$message->setInReplyToMessageId('abcd');
+		$to = [
+			[
+				'label' => 'Lewis',
+				'email' => 'tent-living@startdewvalley.com',
+				'type' => Recipient::TYPE_TO,
+			]
+		];
+		$cc = [];
+		$bcc = [];
+		$attachments = [];
+		$rTo = Recipient::fromParams([
+			'label' => 'Lewis',
+			'email' => 'tent-living@startdewvalley.com',
+			'type' => Recipient::TYPE_TO,
+		]);
+		$message2 = $message;
+		$message2->setId(10);
+		$account = $this->createConfiguredMock(Account::class, [
+			'getUserId' => $this->userId
+		]);
+
+		$this->mapper->expects(self::once())
+			->method('saveWithRecipients')
+			->with($message, [$rTo], $cc, $bcc)
+			->willReturn($message2);
+		$this->clientFactory->expects(self::never())
+			->method('getClient');
+		$this->attachmentService->expects(self::never())
+			->method('handleAttachments');
+		$this->attachmentService->expects(self::never())
+			->method('saveLocalMessageAttachments');
+
+		$result = $this->outboxService->saveMessage($account, $message, $to, $cc, $bcc, $attachments);
+		$this->assertEquals($message2->getId(), $result->getId());
+		$this->assertEmpty($result->getAttachments());
+	}
+
 	public function testUpdateMessage(): void {
 		$message = new LocalMessage();
 		$message->setId(10);
@@ -305,44 +350,54 @@ class OutboxServiceTest extends TestCase {
 		$this->outboxService->updateMessage($account, $message, $to, $cc, $bcc, $attachments);
 	}
 
-
-	public function testSaveMessageNoAttachments(): void {
+	public function testUpdateMessageNoAttachments(): void {
 		$message = new LocalMessage();
+		$message->setId(10);
 		$message->setAccountId(1);
 		$message->setSendAt($this->time->getTime());
 		$message->setSubject('Test');
 		$message->setBody('Test Test Test');
 		$message->setHtml(true);
 		$message->setInReplyToMessageId('abcd');
+		$old = Recipient::fromParams([
+			'label' => 'Pam',
+			'email' => 'BuyMeAnAle@startdewvalley.com',
+			'type' => Recipient::TYPE_TO,
+		]);
+		$message->setRecipients([$old]);
 		$to = [
 			[
-				'label' => 'Pam',
-				'email' => 'BuyMeAnAle@startdewvalley.com',
+				'label' => 'Linus',
+				'email' => 'tent-living@startdewvalley.com',
 				'type' => Recipient::TYPE_TO,
 			]
 		];
 		$cc = [];
 		$bcc = [];
+		$attachments = [];
 		$rTo = Recipient::fromParams([
-			'label' => 'Pam',
-			'email' => 'BuyMeAnAle@startdewvalley.com',
+			'label' => 'Linus',
+			'email' => 'tent-living@startdewvalley.com',
 			'type' => Recipient::TYPE_TO,
 		]);
 		$message2 = $message;
-		$message2->setId(10);
+		$message2->setRecipients([$rTo]);
 		$account = $this->createConfiguredMock(Account::class, [
 			'getUserId' => $this->userId
 		]);
-
 		$this->mapper->expects(self::once())
-			->method('saveWithRecipients')
+			->method('updateWithRecipients')
 			->with($message, [$rTo], $cc, $bcc)
 			->willReturn($message2);
 		$this->attachmentService->expects(self::once())
-			->method('saveLocalMessageAttachments')
-			->with($this->userId, 10, []);
-
-		$this->outboxService->saveMessage($account, $message, $to, $cc, $bcc);
+			->method('updateLocalMessageAttachments')
+			->with($this->userId, $message2, $attachments);
+		$this->clientFactory->expects(self::never())
+			->method('getClient');
+		$this->attachmentService->expects(self::never())
+			->method('handleAttachments');
+		$result = $this->outboxService->updateMessage($account, $message, $to, $cc, $bcc, $attachments);
+		$this->assertEmpty($result->getAttachments());
 	}
 
 	public function testSaveMessageError(): void {