From 66fdc0bbe0b6497320eef449cc5ed644e91025fd Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Thu, 29 Feb 2024 07:44:15 +0100
Subject: [PATCH] fix(db): Avoid dirty read for collected addresses

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 lib/Db/CollectedAddressMapper.php             | 19 +++++++++++-----
 .../AutoCompletion/AddressCollector.php       | 15 +++++--------
 .../Db/CollectedAddressMapperTest.php         | 12 +++++-----
 .../Autocompletion/AddressCollectorTest.php   | 22 ++++++-------------
 4 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/lib/Db/CollectedAddressMapper.php b/lib/Db/CollectedAddressMapper.php
index ce0d167c4..bf9c58953 100644
--- a/lib/Db/CollectedAddressMapper.php
+++ b/lib/Db/CollectedAddressMapper.php
@@ -60,11 +60,7 @@ class CollectedAddressMapper extends QBMapper {
 		return $this->findEntities($dbQuery);
 	}
 
-	/**
-	 * @param null|string $email
-	 * @return bool
-	 */
-	public function exists(string $userId, ?string $email) {
+	public function insertIfNew(string $userId, string $email, ?string $label): bool {
 		$qb = $this->db->getQueryBuilder();
 		$dbQuery = $qb
 			->select('*')
@@ -72,7 +68,18 @@ class CollectedAddressMapper extends QBMapper {
 			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId)))
 			->andWhere($qb->expr()->iLike('email', $qb->createNamedParameter($email)));
 
-		return count($this->findEntities($dbQuery)) > 0;
+		if (!empty($this->findEntities($dbQuery))) {
+			return false;
+		}
+
+		$entity = new CollectedAddress();
+		$entity->setUserId($userId);
+		if ($label !== $email) {
+			$entity->setDisplayName($label);
+		}
+		$entity->setEmail($email);
+		$this->insert($entity);
+		return true;
 	}
 
 	public function getTotal(): int {
diff --git a/lib/Service/AutoCompletion/AddressCollector.php b/lib/Service/AutoCompletion/AddressCollector.php
index 3cff52b79..fd6e5b122 100644
--- a/lib/Service/AutoCompletion/AddressCollector.php
+++ b/lib/Service/AutoCompletion/AddressCollector.php
@@ -28,9 +28,12 @@ use OCA\Mail\Address;
 use OCA\Mail\AddressList;
 use OCA\Mail\Db\CollectedAddress;
 use OCA\Mail\Db\CollectedAddressMapper;
+use OCP\AppFramework\Db\TTransactional;
 use Psr\Log\LoggerInterface;
 
 class AddressCollector {
+	use TTransactional;
+
 	/** @var CollectedAddressMapper */
 	private $mapper;
 
@@ -78,16 +81,8 @@ class AddressCollector {
 			$this->logger->debug("<" . $address->getEmail() . "> is not a valid RFC822 mail address");
 			return;
 		}
-		if ($address->getEmail() !== null && !$this->mapper->exists($userId, $address->getEmail())) {
-			$this->logger->debug("saving new address <{$address->getEmail()}>");
-
-			$entity = new CollectedAddress();
-			$entity->setUserId($userId);
-			if ($address->getLabel() !== $address->getEmail()) {
-				$entity->setDisplayName($address->getLabel());
-			}
-			$entity->setEmail($address->getEmail());
-			$this->mapper->insert($entity);
+		if ($address->getEmail() !== null && $this->mapper->insertIfNew($userId, $address->getEmail(), $address->getLabel())) {
+			$this->logger->debug("saved new address <{$address->getEmail()}>");
 		}
 	}
 
diff --git a/tests/Integration/Db/CollectedAddressMapperTest.php b/tests/Integration/Db/CollectedAddressMapperTest.php
index dad111f97..cdfdfb64d 100644
--- a/tests/Integration/Db/CollectedAddressMapperTest.php
+++ b/tests/Integration/Db/CollectedAddressMapperTest.php
@@ -125,18 +125,18 @@ class CollectedAddressMapperTest extends TestCase {
 		}
 	}
 
-	public function existsData() {
+	public function insertIfNewData(): array {
 		return [
-			['user1@example.com', true],
-			['user3@example.com', false],
+			['user1@example.com', false],
+			['user3@example.com', true],
 		];
 	}
 
 	/**
-	 * @dataProvider existsData
+	 * @dataProvider insertIfNewData
 	 */
-	public function testExists($email, $expected) {
-		$actual = $this->mapper->exists($this->userId, $email);
+	public function testExists($email, $expected): void {
+		$actual = $this->mapper->insertIfNew($this->userId, $email, null);
 
 		$this->assertSame($expected, $actual);
 	}
diff --git a/tests/Unit/Service/Autocompletion/AddressCollectorTest.php b/tests/Unit/Service/Autocompletion/AddressCollectorTest.php
index 033cab0fd..7b95a9910 100644
--- a/tests/Unit/Service/Autocompletion/AddressCollectorTest.php
+++ b/tests/Unit/Service/Autocompletion/AddressCollectorTest.php
@@ -48,7 +48,7 @@ class AddressCollectorTest extends TestCase {
 		);
 	}
 
-	public function testAddAddresses() {
+	public function testAddAddresses(): void {
 		$addresses = [
 			'"User" <user@example.com>',
 			'Example <example@user.com>',
@@ -64,37 +64,29 @@ class AddressCollectorTest extends TestCase {
 		$address2->setUserId($this->userId);
 
 		$this->mapper->expects($this->exactly(2))
-			->method('exists')
+			->method('insertIfNew')
 			->withConsecutive(
 				[$this->userId, 'user@example.com'],
 				[$this->userId, 'example@user.com']
 			)
 			->willReturnOnConsecutiveCalls(
-				false,
-				false
-			);
-		$this->mapper->expects($this->exactly(2))
-			->method('insert')
-			->withConsecutive(
-				[$address1],
-				[$address2]
+				true,
+				true,
 			);
 
 		$this->collector->addAddresses($this->userId, $addressList);
 	}
 
-	public function testAddDuplicateAddresses() {
+	public function testAddDuplicateAddresses(): void {
 		$addresses = [
 			'user@example.com',
 		];
 		$addressList = AddressList::parse($addresses);
 
 		$this->mapper->expects($this->once())
-			->method('exists')
+			->method('insertIfNew')
 			->with($this->userId, $addresses[0])
-			->will($this->returnValue(true));
-		$this->mapper->expects($this->never())
-			->method('insert');
+			->willReturn(false);
 
 		$this->collector->addAddresses($this->userId, $addressList);
 	}