Merge pull request #9423 from nextcloud/fix/db/dirty-read-message-del…

…ete-by-uid fix(db): Avoid dirty reads while deleting messages by uid
nextcloud · Mar 5, 2024 · 99409c6 · 99409c6
2 parents 4c7f8a3 + e92114e
commit 99409c6
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 18 deletions.
diff --git a/lib/Db/MessageMapper.php b/lib/Db/MessageMapper.php
@@ -705,39 +705,32 @@ public function deleteAll(Mailbox $mailbox): void {
 	}
 
 	public function deleteByUid(Mailbox $mailbox, int ...$uids): void {
-		$messageIdQuery = $this->db->getQueryBuilder();
+		$messageIdSubQuery = $this->db->getQueryBuilder();
 		$deleteRecipientsQuery = $this->db->getQueryBuilder();
 		$deleteMessagesQuery = $this->db->getQueryBuilder();
 
-		// Get all message ids query
-		$messageIdQuery->select('id')
+		$messageIdSubQuery->select('id')
 			->from($this->getTableName())
 			->where(
-				$messageIdQuery->expr()->eq('mailbox_id', $messageIdQuery->createNamedParameter($mailbox->getId())),
-				$messageIdQuery->expr()->in('uid', $messageIdQuery->createParameter('uids'))
+				$messageIdSubQuery->expr()->eq('mailbox_id', $deleteRecipientsQuery->createNamedParameter($mailbox->getId())),
+				$messageIdSubQuery->expr()->in('uid', $deleteRecipientsQuery->createParameter('uids'))
 			);
-
 		$deleteRecipientsQuery->delete('mail_recipients')
-			->where($deleteRecipientsQuery->expr()->in('message_id', $deleteRecipientsQuery->createParameter('messageIds')));
+			->where($deleteRecipientsQuery->expr()->in('message_id', $deleteMessagesQuery->createFunction($messageIdSubQuery->getSQL())));
 
 		$deleteMessagesQuery->delete($this->getTableName())
-			->where($deleteMessagesQuery->expr()->in('id', $deleteMessagesQuery->createParameter('messageIds')));
+			->where(
+				$deleteMessagesQuery->expr()->eq('mailbox_id', $deleteMessagesQuery->createNamedParameter($mailbox->getId())),
+				$deleteMessagesQuery->expr()->in('uid', $deleteMessagesQuery->createParameter('uids')),
+			);
 
 		foreach (array_chunk($uids, 1000) as $chunk) {
-			$messageIdQuery->setParameter('uids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
-			$cursor = $messageIdQuery->executeQuery();
-
-			$messageIds = array_map(static function (array $message) {
-				return $message['id'];
-			}, $cursor->fetchAll());
-			$cursor->closeCursor();
-
 			// delete all related recipient entries
-			$deleteRecipientsQuery->setParameter('messageIds', $messageIds, IQueryBuilder::PARAM_INT_ARRAY);
+			$deleteRecipientsQuery->setParameter('uids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
 			$deleteRecipientsQuery->executeStatement();
 
 			// delete all messages
-			$deleteMessagesQuery->setParameter('messageIds', $messageIds, IQueryBuilder::PARAM_INT_ARRAY);
+			$deleteMessagesQuery->setParameter('uids', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
 			$deleteMessagesQuery->executeStatement();
 		}
 	}

diff --git a/tests/Integration/Db/MessageMapperTest.php b/tests/Integration/Db/MessageMapperTest.php
@@ -36,6 +36,8 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
+use function array_map;
+use function range;
 use function time;
 
 class MessageMapperTest extends TestCase {
@@ -186,4 +188,27 @@ public function testFindIdsByQuery(): void {
 
 		self::assertEquals([3,2,1], $result);
 	}
+
+	public function testDeleteByUid(): void {
+		$mailbox = new Mailbox();
+		$mailbox->setId(1);
+		array_map(function ($i) {
+			$qb = $this->db->getQueryBuilder();
+			$insert = $qb->insert($this->mapper->getTableName())
+				->values([
+					'uid' => $qb->createNamedParameter($i, IQueryBuilder::PARAM_INT),
+					'message_id' => $qb->createNamedParameter('<abc' . $i . '@123.com>'),
+					'mailbox_id' => $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT),
+					'subject' => $qb->createNamedParameter('TEST'),
+					'sent_at' => $qb->createNamedParameter(time(), IQueryBuilder::PARAM_INT),
+					'in_reply_to' => $qb->createNamedParameter('<>')
+				]);
+			$insert->executeStatement();
+		}, range(1, 10));
+
+		$this->mapper->deleteByUid($mailbox, 1, 5);
+
+		$messages = $this->mapper->findByUids($mailbox, range(1, 10));
+		self::assertCount(8, $messages);
+	}
 }