From b3aa7465fae6d486115b305209cd178a5bc836b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= <14975046+skjnldsv@users.noreply.github.com> Date: Thu, 24 Sep 2026 03:03:47 +0200 Subject: [PATCH] fix(files_reminders): keep folder preload for folders over 512 entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReminderService::cacheFolder() stored one entry per child in the in-memory cache, which is capped at 512 entries. Listing a folder with 512 or more children evicted entries before the DAV plugin read them, so each evicted file fell back to its own query. Keep the folder preload in a separate per-request map and drop the entry whenever a reminder is written, so later changes in the same request win. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com> --- .../lib/Service/ReminderService.php | 34 +++- .../tests/Service/ReminderServiceTest.php | 191 ++++++++++++++++++ 2 files changed, 215 insertions(+), 10 deletions(-) create mode 100644 apps/files_reminders/tests/Service/ReminderServiceTest.php diff --git a/apps/files_reminders/lib/Service/ReminderService.php b/apps/files_reminders/lib/Service/ReminderService.php index 56fc8389257a4..4452b9b67d7be 100644 --- a/apps/files_reminders/lib/Service/ReminderService.php +++ b/apps/files_reminders/lib/Service/ReminderService.php @@ -35,6 +35,14 @@ class ReminderService { private ICache $cache; + /** + * Reminders preloaded for whole folders, not capped like $cache + * so listings of large folders don't fall back to one query per file. + * + * @var array + */ + private array $folderCache = []; + public function __construct( protected IUserManager $userManager, protected IURLGenerator $urlGenerator, @@ -56,8 +64,7 @@ public function cacheFolder(IUser $user, Folder $folder): void { $nodes = $folder->getDirectoryListing(); foreach ($nodes as $node) { - $reminder = $reminderMap[$node->getId()] ?? false; - $this->cache->set("{$user->getUID()}-{$node->getId()}", $reminder); + $this->folderCache["{$user->getUID()}-{$node->getId()}"] = $reminderMap[$node->getId()] ?? false; } } @@ -68,14 +75,16 @@ public function getDueForUser(IUser $user, int $fileId, bool $checkNode = true): if ($checkNode) { $this->checkNode($user, $fileId); } + $cacheKey = "{$user->getUID()}-$fileId"; /** @var null|false|Reminder $cachedReminder */ - $cachedReminder = $this->cache->get("{$user->getUID()}-$fileId"); + $cachedReminder = $this->folderCache[$cacheKey] ?? $this->cache->get($cacheKey); if ($cachedReminder === false) { return null; } if ($cachedReminder instanceof Reminder) { if ($cachedReminder->getDueDate() < new DateTime()) { - $this->cache->remove("{$user->getUID()}-$fileId"); + $this->cache->remove($cacheKey); + unset($this->folderCache[$cacheKey]); return null; } return new RichReminder($cachedReminder, $this->root); @@ -88,10 +97,10 @@ public function getDueForUser(IUser $user, int $fileId, bool $checkNode = true): return null; } - $this->cache->set("{$user->getUID()}-$fileId", $reminder); + $this->setCached($user->getUID(), $fileId, $reminder); return new RichReminder($reminder, $this->root); } catch (DoesNotExistException $e) { - $this->cache->set("{$user->getUID()}-$fileId", false); + $this->setCached($user->getUID(), $fileId, false); return null; } } @@ -126,13 +135,13 @@ public function createOrUpdate(IUser $user, int $fileId, DateTime $dueDate): boo $reminder->setUpdatedAt($now); $reminder->setCreatedAt($now); $this->reminderMapper->insert($reminder); - $this->cache->set("{$user->getUID()}-$fileId", $reminder); + $this->setCached($user->getUID(), $fileId, $reminder); return true; } $reminder->setDueDate($dueDate); $reminder->setUpdatedAt($now); $this->reminderMapper->update($reminder); - $this->cache->set("{$user->getUID()}-$fileId", $reminder); + $this->setCached($user->getUID(), $fileId, $reminder); return false; } @@ -191,7 +200,7 @@ public function send(Reminder $reminder): void { try { $this->notificationManager->notify($notification); $this->reminderMapper->markNotified($reminder); - $this->cache->set("{$user->getUID()}-{$reminder->getFileId()}", $reminder); + $this->setCached($user->getUID(), $reminder->getFileId(), $reminder); } catch (Throwable $th) { $this->logger->error($th->getMessage(), $th->getTrace()); } @@ -209,7 +218,12 @@ public function cleanUp(?int $limit = null): void { private function deleteReminder(Reminder $reminder): void { $this->reminderMapper->delete($reminder); - $this->cache->set("{$reminder->getUserId()}-{$reminder->getFileId()}", false); + $this->setCached($reminder->getUserId(), $reminder->getFileId(), false); + } + + private function setCached(string $userId, int $fileId, Reminder|false $reminder): void { + $this->cache->set("$userId-$fileId", $reminder); + unset($this->folderCache["$userId-$fileId"]); } /** diff --git a/apps/files_reminders/tests/Service/ReminderServiceTest.php b/apps/files_reminders/tests/Service/ReminderServiceTest.php new file mode 100644 index 0000000000000..db5ae32d7f4a9 --- /dev/null +++ b/apps/files_reminders/tests/Service/ReminderServiceTest.php @@ -0,0 +1,191 @@ +reminderMapper = $this->createMock(ReminderMapper::class); + $this->root = $this->createMock(IRootFolder::class); + $this->user = $this->createMock(IUser::class); + $this->user->method('getUID')->willReturn('alice'); + + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createInMemory')->willReturnCallback(fn (int $capacity = 512) => new CappedMemoryCache($capacity)); + + $this->service = new ReminderService( + $this->createMock(IUserManager::class), + $this->createMock(IURLGenerator::class), + $this->createMock(INotificationManager::class), + $this->reminderMapper, + $this->root, + $this->createMock(LoggerInterface::class), + $cacheFactory, + ); + } + + private function createReminder(int $fileId, DateTime $dueDate): Reminder { + $reminder = new Reminder(); + $reminder->setUserId('alice'); + $reminder->setFileId($fileId); + $reminder->setDueDate($dueDate); + return $reminder; + } + + /** + * @param Reminder[] $reminders + */ + private function preloadFolder(int $childCount, array $reminders): void { + $children = array_map(function (int $fileId): Node { + $node = $this->createMock(Node::class); + $node->method('getId')->willReturn($fileId); + return $node; + }, range(1, $childCount)); + + $folder = $this->createMock(Folder::class); + $folder->method('getDirectoryListing')->willReturn($children); + $this->reminderMapper->method('findAllInFolder')->willReturn($reminders); + + $this->service->cacheFolder($this->user, $folder); + } + + private function allowNodeAccess(): void { + $userFolder = $this->createMock(IUserFolder::class); + $userFolder->method('getFirstNodeById')->willReturn($this->createMock(Node::class)); + $this->root->method('getUserFolder')->willReturn($userFolder); + } + + /** + * A DAV listing preloads the reminders of the whole folder, then asks for + * each child. The per-file cache only holds 512 entries, so the preload of + * a larger folder must not depend on it: no child may fall back to a query. + */ + public function testCacheFolderCoversFoldersLargerThanTheMemoryCache(): void { + $this->preloadFolder(2000, [$this->createReminder(1500, new DateTime('+1 day'))]); + + $this->reminderMapper->expects($this->never())->method('findDueForUser'); + + $found = []; + for ($fileId = 1; $fileId <= 2000; $fileId++) { + if ($this->service->getDueForUser($this->user, $fileId, false) !== null) { + $found[] = $fileId; + } + } + $this->assertSame([1500], $found); + } + + /** + * Files outside any preloaded folder are still looked up one by one, + * and a missing reminder is cached so the query only runs once. + */ + public function testUncachedFileFallsBackToTheMapper(): void { + $this->preloadFolder(10, []); + + $this->reminderMapper->expects($this->once()) + ->method('findDueForUser') + ->with($this->user, 42) + ->willThrowException(new DoesNotExistException('')); + + $this->assertNull($this->service->getDueForUser($this->user, 42, false)); + // The miss is cached too + $this->assertNull($this->service->getDueForUser($this->user, 42, false)); + } + + /** + * A preloaded reminder can already be past due. It must not be returned, + * and dropping it from the preload means the next lookup asks the database. + */ + public function testExpiredPreloadedReminderIsNotReturned(): void { + $this->preloadFolder(600, [$this->createReminder(7, new DateTime('-1 hour'))]); + + $this->reminderMapper->expects($this->once()) + ->method('findDueForUser') + ->with($this->user, 7) + ->willThrowException(new DoesNotExistException('')); + + $this->assertNull($this->service->getDueForUser($this->user, 7, false)); + $this->assertNull($this->service->getDueForUser($this->user, 7, false)); + } + + /** + * The preload says "no reminder" for every child without one. Creating a + * reminder later in the same request must replace that answer. + */ + public function testCreateAfterPreloadIsReturned(): void { + $this->allowNodeAccess(); + $this->preloadFolder(600, []); + $this->reminderMapper->expects($this->once())->method('insert'); + + $dueDate = new DateTime('+2 days'); + $this->assertTrue($this->service->createOrUpdate($this->user, 300, $dueDate)); + + $reminder = $this->service->getDueForUser($this->user, 300, false); + $this->assertNotNull($reminder); + $this->assertEquals($dueDate, $reminder->getDueDate()); + } + + /** + * Updating a preloaded reminder must return the new due date, + * not the one loaded with the folder. + */ + public function testUpdateAfterPreloadIsReturned(): void { + $this->allowNodeAccess(); + $this->preloadFolder(600, [$this->createReminder(300, new DateTime('+1 day'))]); + $this->reminderMapper->expects($this->once())->method('update'); + + $dueDate = new DateTime('+5 days'); + $this->assertFalse($this->service->createOrUpdate($this->user, 300, $dueDate)); + + $reminder = $this->service->getDueForUser($this->user, 300, false); + $this->assertNotNull($reminder); + $this->assertEquals($dueDate, $reminder->getDueDate()); + } + + /** + * Removing a preloaded reminder must hide it for the rest of the request, + * without querying the database again. + */ + public function testRemoveAfterPreloadIsNotReturned(): void { + $this->allowNodeAccess(); + $this->preloadFolder(600, [$this->createReminder(300, new DateTime('+1 day'))]); + $this->reminderMapper->expects($this->once())->method('delete'); + $this->reminderMapper->expects($this->never())->method('findDueForUser'); + + $this->service->remove($this->user, 300); + + $this->assertNull($this->service->getDueForUser($this->user, 300, false)); + } +}