Merge pull request #50244 from nextcloud/perf/cache-file-reminders

perf(files_reminders): Reduce db queries on propfind
nextcloud · Feb 21, 2025 · ceb6f56 · ceb6f56
2 parents 6cd3046 + d249b60
commit ceb6f56
Show file tree

Hide file tree

Showing 7 changed files with 137 additions and 48 deletions.
diff --git a/apps/files_reminders/composer/composer/autoload_classmap.php b/apps/files_reminders/composer/composer/autoload_classmap.php
@@ -16,6 +16,7 @@
     'OCA\\FilesReminders\\Db\\Reminder' => $baseDir . '/../lib/Db/Reminder.php',
     'OCA\\FilesReminders\\Db\\ReminderMapper' => $baseDir . '/../lib/Db/ReminderMapper.php',
     'OCA\\FilesReminders\\Exception\\NodeNotFoundException' => $baseDir . '/../lib/Exception/NodeNotFoundException.php',
+    'OCA\\FilesReminders\\Exception\\ReminderNotFoundException' => $baseDir . '/../lib/Exception/ReminderNotFoundException.php',
     'OCA\\FilesReminders\\Exception\\UserNotFoundException' => $baseDir . '/../lib/Exception/UserNotFoundException.php',
     'OCA\\FilesReminders\\Listener\\LoadAdditionalScriptsListener' => $baseDir . '/../lib/Listener/LoadAdditionalScriptsListener.php',
     'OCA\\FilesReminders\\Listener\\NodeDeletedListener' => $baseDir . '/../lib/Listener/NodeDeletedListener.php',

diff --git a/apps/files_reminders/composer/composer/autoload_static.php b/apps/files_reminders/composer/composer/autoload_static.php
@@ -31,6 +31,7 @@ class ComposerStaticInitFilesReminders
         'OCA\\FilesReminders\\Db\\Reminder' => __DIR__ . '/..' . '/../lib/Db/Reminder.php',
         'OCA\\FilesReminders\\Db\\ReminderMapper' => __DIR__ . '/..' . '/../lib/Db/ReminderMapper.php',
         'OCA\\FilesReminders\\Exception\\NodeNotFoundException' => __DIR__ . '/..' . '/../lib/Exception/NodeNotFoundException.php',
+        'OCA\\FilesReminders\\Exception\\ReminderNotFoundException' => __DIR__ . '/..' . '/../lib/Exception/ReminderNotFoundException.php',
         'OCA\\FilesReminders\\Exception\\UserNotFoundException' => __DIR__ . '/..' . '/../lib/Exception/UserNotFoundException.php',
         'OCA\\FilesReminders\\Listener\\LoadAdditionalScriptsListener' => __DIR__ . '/..' . '/../lib/Listener/LoadAdditionalScriptsListener.php',
         'OCA\\FilesReminders\\Listener\\NodeDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/NodeDeletedListener.php',

diff --git a/apps/files_reminders/lib/Controller/ApiController.php b/apps/files_reminders/lib/Controller/ApiController.php
@@ -14,8 +14,8 @@
 use DateTimeZone;
 use Exception;
 use OCA\FilesReminders\Exception\NodeNotFoundException;
+use OCA\FilesReminders\Exception\ReminderNotFoundException;
 use OCA\FilesReminders\Service\ReminderService;
-use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
 use OCP\AppFramework\Http\DataResponse;
@@ -53,15 +53,14 @@ public function get(int $fileId): DataResponse {
 
 		try {
 			$reminder = $this->reminderService->getDueForUser($user, $fileId);
-			$reminderData = [
+			if ($reminder === null) {
+				return new DataResponse(['dueDate' => null], Http::STATUS_OK);
+			}
+			return new DataResponse([
 				'dueDate' => $reminder->getDueDate()->format(DateTimeInterface::ATOM), // ISO 8601
-			];
-			return new DataResponse($reminderData, Http::STATUS_OK);
-		} catch (NodeNotFoundException|DoesNotExistException $e) {
-			$reminderData = [
-				'dueDate' => null,
-			];
-			return new DataResponse($reminderData, Http::STATUS_OK);
+			], Http::STATUS_OK);
+		} catch (NodeNotFoundException $e) {
+			return new DataResponse(['dueDate' => null], Http::STATUS_OK);
 		}
 	}
 
@@ -125,7 +124,7 @@ public function remove(int $fileId): DataResponse {
 		try {
 			$this->reminderService->remove($user, $fileId);
 			return new DataResponse([], Http::STATUS_OK);
-		} catch (NodeNotFoundException|DoesNotExistException $e) {
+		} catch (NodeNotFoundException|ReminderNotFoundException $e) {
 			return new DataResponse([], Http::STATUS_NOT_FOUND);
 		}
 	}

diff --git a/apps/files_reminders/lib/Dav/PropFindPlugin.php b/apps/files_reminders/lib/Dav/PropFindPlugin.php
@@ -10,9 +10,10 @@
 namespace OCA\FilesReminders\Dav;
 
 use DateTimeInterface;
+use OCA\DAV\Connector\Sabre\Directory;
 use OCA\DAV\Connector\Sabre\Node;
 use OCA\FilesReminders\Service\ReminderService;
-use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\Files\Folder;
 use OCP\IUser;
 use OCP\IUserSession;
 use Sabre\DAV\INode;
@@ -43,6 +44,15 @@ public function propFind(PropFind $propFind, INode $node) {
 			return;
 		}
 
+		if (
+			$node instanceof Directory
+			&& $propFind->getDepth() > 0
+			&& $propFind->getStatus(static::REMINDER_DUE_DATE_PROPERTY) !== null
+		) {
+			$folder = $node->getNode();
+			$this->cacheFolder($folder);
+		}
+
 		$propFind->handle(
 			static::REMINDER_DUE_DATE_PROPERTY,
 			function () use ($node) {
@@ -52,14 +62,21 @@ function () use ($node) {
 				}
 
 				$fileId = $node->getId();
-				try {
-					$reminder = $this->reminderService->getDueForUser($user, $fileId);
-				} catch (DoesNotExistException $e) {
+				$reminder = $this->reminderService->getDueForUser($user, $fileId);
+				if ($reminder === null) {
 					return '';
 				}
 
 				return $reminder->getDueDate()->format(DateTimeInterface::ATOM); // ISO 8601
 			},
 		);
 	}
+
+	private function cacheFolder(Folder $folder): void {
+		$user = $this->userSession->getUser();
+		if (!($user instanceof IUser)) {
+			return;
+		}
+		$this->reminderService->cacheFolder($user, $folder);
+	}
 }
diff --git a/apps/files_reminders/lib/Db/ReminderMapper.php b/apps/files_reminders/lib/Db/ReminderMapper.php
@@ -13,6 +13,7 @@
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\QBMapper;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\Files\Folder;
 use OCP\Files\Node;
 use OCP\Files\NotFoundException;
 use OCP\IDBConnection;
@@ -39,16 +40,6 @@ public function markNotified(Reminder $reminder): Reminder {
 		return $this->update($reminderUpdate);
 	}
 
-	public function find(int $id): Reminder {
-		$qb = $this->db->getQueryBuilder();
-
-		$qb->select('id', 'user_id', 'file_id', 'due_date', 'updated_at', 'created_at', 'notified')
-			->from($this->getTableName())
-			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)));
-
-		return $this->findEntity($qb);
-	}
-
 	/**
 	 * @throws DoesNotExistException
 	 */
@@ -141,4 +132,30 @@ public function findNotified(DateTime $buffer, ?int $limit = null) {
 
 		return $this->findEntities($qb);
 	}
+
+	/**
+	 * @return Reminder[]
+	 */
+	public function findAllInFolder(IUser $user, Folder $folder) {
+		$fileIds = array_values(array_filter(array_map(
+			function (Node $node) {
+				try {
+					return $node->getId();
+				} catch (NotFoundException $e) {
+					return null;
+				}
+			},
+			$folder->getDirectoryListing(),
+		)));
+
+		$qb = $this->db->getQueryBuilder();
+
+		$qb->select('id', 'user_id', 'file_id', 'due_date', 'updated_at', 'created_at', 'notified')
+			->from($this->getTableName())
+			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->in('file_id', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)))
+			->orderBy('due_date', 'ASC');
+
+		return $this->findEntities($qb);
+	}
 }
diff --git a/apps/files_reminders/lib/Exception/ReminderNotFoundException.php b/apps/files_reminders/lib/Exception/ReminderNotFoundException.php
@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\FilesReminders\Exception;
+
+use Exception;
+
+class ReminderNotFoundException extends Exception {
+}
diff --git a/apps/files_reminders/lib/Service/ReminderService.php b/apps/files_reminders/lib/Service/ReminderService.php
@@ -15,11 +15,15 @@
 use OCA\FilesReminders\Db\Reminder;
 use OCA\FilesReminders\Db\ReminderMapper;
 use OCA\FilesReminders\Exception\NodeNotFoundException;
+use OCA\FilesReminders\Exception\ReminderNotFoundException;
 use OCA\FilesReminders\Exception\UserNotFoundException;
 use OCA\FilesReminders\Model\RichReminder;
 use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\Files\Node;
+use OCP\ICache;
+use OCP\ICacheFactory;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserManager;
@@ -28,32 +32,57 @@
 use Throwable;
 
 class ReminderService {
+
+	private ICache $cache;
+
 	public function __construct(
 		protected IUserManager $userManager,
 		protected IURLGenerator $urlGenerator,
 		protected INotificationManager $notificationManager,
 		protected ReminderMapper $reminderMapper,
 		protected IRootFolder $root,
 		protected LoggerInterface $logger,
+		protected ICacheFactory $cacheFactory,
 	) {
+		$this->cache = $this->cacheFactory->createInMemory();
 	}
 
-	/**
-	 * @throws DoesNotExistException
-	 */
-	public function get(int $id): RichReminder {
-		$reminder = $this->reminderMapper->find($id);
-		return new RichReminder($reminder, $this->root);
+	public function cacheFolder(IUser $user, Folder $folder): void {
+		$reminders = $this->reminderMapper->findAllInFolder($user, $folder);
+		$reminderMap = [];
+		foreach ($reminders as $reminder) {
+			$reminderMap[$reminder->getFileId()] = $reminder;
+		}
+
+		$nodes = $folder->getDirectoryListing();
+		foreach ($nodes as $node) {
+			$reminder = $reminderMap[$node->getId()] ?? false;
+			$this->cache->set("{$user->getUID()}-{$node->getId()}", $reminder);
+		}
 	}
 
 	/**
 	 * @throws NodeNotFoundException
-	 * @throws DoesNotExistException
 	 */
-	public function getDueForUser(IUser $user, int $fileId): RichReminder {
+	public function getDueForUser(IUser $user, int $fileId): ?RichReminder {
 		$this->checkNode($user, $fileId);
-		$reminder = $this->reminderMapper->findDueForUser($user, $fileId);
-		return new RichReminder($reminder, $this->root);
+		/** @var null|false|Reminder $cachedReminder */
+		$cachedReminder = $this->cache->get("{$user->getUID()}-$fileId");
+		if ($cachedReminder === false) {
+			return null;
+		}
+		if ($cachedReminder instanceof Reminder) {
+			return new RichReminder($cachedReminder, $this->root);
+		}
+
+		try {
+			$reminder = $this->reminderMapper->findDueForUser($user, $fileId);
+			$this->cache->set("{$user->getUID()}-$fileId", $reminder);
+			return new RichReminder($reminder, $this->root);
+		} catch (DoesNotExistException $e) {
+			$this->cache->set("{$user->getUID()}-$fileId", false);
+			return null;
+		}
 	}
 
 	/**
@@ -77,46 +106,49 @@ public function getAll(?IUser $user = null) {
 	public function createOrUpdate(IUser $user, int $fileId, DateTime $dueDate): bool {
 		$now = new DateTime('now', new DateTimeZone('UTC'));
 		$this->checkNode($user, $fileId);
-		try {
-			$reminder = $this->reminderMapper->findDueForUser($user, $fileId);
-			$reminder->setDueDate($dueDate);
-			$reminder->setUpdatedAt($now);
-			$this->reminderMapper->update($reminder);
-			return false;
-		} catch (DoesNotExistException $e) {
-			// Create new reminder if no reminder is found
+		$reminder = $this->getDueForUser($user, $fileId);
+		if ($reminder === null) {
 			$reminder = new Reminder();
 			$reminder->setUserId($user->getUID());
 			$reminder->setFileId($fileId);
 			$reminder->setDueDate($dueDate);
 			$reminder->setUpdatedAt($now);
 			$reminder->setCreatedAt($now);
 			$this->reminderMapper->insert($reminder);
+			$this->cache->set("{$user->getUID()}-$fileId", $reminder);
 			return true;
 		}
+		$reminder->setDueDate($dueDate);
+		$reminder->setUpdatedAt($now);
+		$this->reminderMapper->update($reminder);
+		$this->cache->set("{$user->getUID()}-$fileId", $reminder);
+		return false;
 	}
 
 	/**
 	 * @throws NodeNotFoundException
-	 * @throws DoesNotExistException
+	 * @throws ReminderNotFoundException
 	 */
 	public function remove(IUser $user, int $fileId): void {
 		$this->checkNode($user, $fileId);
-		$reminder = $this->reminderMapper->findDueForUser($user, $fileId);
-		$this->reminderMapper->delete($reminder);
+		$reminder = $this->getDueForUser($user, $fileId);
+		if ($reminder === null) {
+			throw new ReminderNotFoundException();
+		}
+		$this->deleteReminder($reminder);
 	}
 
 	public function removeAllForNode(Node $node): void {
 		$reminders = $this->reminderMapper->findAllForNode($node);
 		foreach ($reminders as $reminder) {
-			$this->reminderMapper->delete($reminder);
+			$this->deleteReminder($reminder);
 		}
 	}
 
 	public function removeAllForUser(IUser $user): void {
 		$reminders = $this->reminderMapper->findAllForUser($user);
 		foreach ($reminders as $reminder) {
-			$this->reminderMapper->delete($reminder);
+			$this->deleteReminder($reminder);
 		}
 	}
 
@@ -148,6 +180,7 @@ public function send(Reminder $reminder): void {
 		try {
 			$this->notificationManager->notify($notification);
 			$this->reminderMapper->markNotified($reminder);
+			$this->cache->set("{$user->getUID()}-{$reminder->getFileId()}", $reminder);
 		} catch (Throwable $th) {
 			$this->logger->error($th->getMessage(), $th->getTrace());
 		}
@@ -159,10 +192,16 @@ public function cleanUp(?int $limit = null): void {
 			->modify('-1 day');
 		$reminders = $this->reminderMapper->findNotified($buffer, $limit);
 		foreach ($reminders as $reminder) {
-			$this->reminderMapper->delete($reminder);
+			$this->deleteReminder($reminder);
 		}
 	}
 
+	private function deleteReminder(Reminder $reminder): void {
+		$this->reminderMapper->delete($reminder);
+		$this->cache->set("{$reminder->getUserId()}-{$reminder->getFileId()}", false);
+	}
+
+
 	/**
 	 * @throws NodeNotFoundException
 	 */