From c1296ce9f2c56cf7fb5ad3060cf0054c34286ef5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 20:56:02 +0000 Subject: [PATCH] fix(notification): discard notifications pointing to deleted files The notifications app re-renders every stored notification on each poll of /ocs/v2.php/apps/notifications/api/v2/notifications. If the file a workflow_ocr notification belongs to no longer exists for the user, the notifier logged a warning and fell back to a generic subject, leaving the row in oc_notifications. The warning was therefore emitted again on every single poll and never went away by itself. Throw AlreadyProcessedException instead so Nextcloud removes the obsolete notification, and log the missing file at debug level: a user deleting a file (or moving it to the trashbin, which is outside of //files and hence not resolvable via the user folder either) after OCR has run is a normal situation, not something worth a warning. Unexpected errors while resolving the file still get logged as an error and keep the previous fallback to the generic subject without a file link. Also switches from getById()/array_shift() to getFirstNodeById(). Fixes #382 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M3aC9zhcgYNxtr2n4pZUaE --- lib/Notification/Notifier.php | 53 ++++++++++++++++-------- tests/Unit/Notification/NotifierTest.php | 46 ++++++++++---------- 2 files changed, 60 insertions(+), 39 deletions(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 3d12aaa8..055f9315 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -103,18 +103,20 @@ public function prepare(INotification $notification, string $languageCode): INot } // Only add file info if we have some ... - $richParams = false; + $richParams = null; if ($notification->getObjectType() === 'file' && ($fileId = $notification->getObjectId()) && ($uid = $notification->getUser())) { + // Note:: This might throw an AlreadyProcessedException if the file doesn't exist anymore. + // It has to be thrown before any call to $notification->set... otherwise the notification + // won't be removed from the database. $richParams = $this->tryGetRichParamForFile($uid, intval($fileId)); - if ($richParams !== false) { - $notification->setRichSubject($richSubject, $richParams); - } } - // Fallback to generic error message without file link - if ($richParams === false) { + if ($richParams !== null) { + $notification->setRichSubject($richSubject, $richParams); + } else { + // Fallback to generic error message without file link $notification->setParsedSubject($parsedSubject); } @@ -129,21 +131,38 @@ public function prepare(INotification $notification, string $languageCode): INot return $notification; } - private function tryGetRichParamForFile(string $uid, int $fileId) : array|bool { + /** + * Tries to build the rich notification parameters pointing to the file the + * notification was created for. + * + * @return array|null The rich parameters or `null` if they could not be determined + * because of an unexpected error. + * @throws AlreadyProcessedException If the file cannot be found for the given user + * anymore. In that case the notification is obsolete + * and gets removed from the database instead of being + * re-rendered on every notification poll (see #382). + */ + private function tryGetRichParamForFile(string $uid, int $fileId) : ?array { try { $userFolder = $this->rootFolder->getUserFolder($uid); - /** @var File[] */ - $files = $userFolder->getById($fileId); - /** @var File $file */ - $file = array_shift($files); - if ($file === null) { - $this->logger->warning('Could not find file with id {fileId} for user {uid}', ['fileId' => $fileId, 'uid' => $uid]); - return false; - } - $relativePath = $userFolder->getRelativePath($file->getPath()); + /** @var File|null $file */ + $file = $userFolder->getFirstNodeById($fileId); + $relativePath = $file !== null ? $userFolder->getRelativePath($file->getPath()) : null; } catch (\Throwable $th) { $this->logger->error($th->getMessage(), ['exception' => $th]); - return false; + return null; + } + + if ($file === null) { + // Nothing unusual: the user might have deleted the file (or moved it to the + // trashbin) after the OCR process has finished. Since we cannot render a link + // to the file anymore, the notification is dropped. This also prevents the + // message from being logged over and over again, because the notifications app + // re-renders every stored notification on each poll. + $this->logger->debug('Could not find file with id {fileId} for user {uid}, discarding obsolete notification', ['fileId' => $fileId, 'uid' => $uid]); + // Note:: AlreadyProcessedException has to be thrown before any call to $notification->set... + // otherwise notification won't be removed from the database + throw new AlreadyProcessedException(); } return [ diff --git a/tests/Unit/Notification/NotifierTest.php b/tests/Unit/Notification/NotifierTest.php index 644ca43f..73f48de5 100644 --- a/tests/Unit/Notification/NotifierTest.php +++ b/tests/Unit/Notification/NotifierTest.php @@ -156,9 +156,9 @@ public function testPrepareConstructsOcrErrorCorrectlyWithFileId() { /** @var Folder|MockObject */ $userFolder = $this->createMock(Folder::class); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') - ->willReturn(['file' => $file]); + ->method('getFirstNodeById') + ->with(123) + ->willReturn($file); $userFolder->expects($this->once()) ->method('getRelativePath') ->with('admin/files/file.txt') @@ -256,8 +256,8 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThr $userFolder = $this->createMock(Folder::class); $ex = new \OCP\Files\NotFoundException('nope ... sorry'); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') + ->method('getFirstNodeById') + ->with(123) ->willThrowException($ex); // This is what we want to test ... $userFolder->expects($this->never()) ->method('getRelativePath'); @@ -283,7 +283,10 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThr $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); } - public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayWasEmpty() { + /** + * @see https://github.com/R0Wi-DEV/workflow_ocr/issues/382 + */ + public function testThrowsAlreadyProcessedExceptionIfFileCannotBeFoundAnymore() { /** @var IValidator|MockObject */ $validator = $this->createMock(IValidator::class); /** @var IRichTextFormatter|MockObject */ @@ -305,31 +308,30 @@ public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayW /** @var Folder|MockObject */ $userFolder = $this->createMock(Folder::class); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') - ->willReturn([]); // This is what we want to test ... + ->method('getFirstNodeById') + ->with(123) + ->willReturn(null); // This is what we want to test ... $userFolder->expects($this->never()) ->method('getRelativePath'); $this->rootFolder->expects($this->once()) ->method('getUserFolder') ->with('user') ->willReturn($userFolder); - $this->urlGenerator->expects($this->once()) - ->method('imagePath') - ->with('workflow_ocr', 'app-dark.svg') - ->willReturn('apps/workflow_ocr/app-dark.svg'); - $this->urlGenerator->expects($this->once()) - ->method('getAbsoluteURL') - ->with('apps/workflow_ocr/app-dark.svg') - ->willReturn('http://localhost/index.php/apps/workflow_ocr/app-dark.svg'); + // The notification is dropped, so it's never rendered + $this->urlGenerator->expects($this->never()) + ->method('imagePath'); + $this->urlGenerator->expects($this->never()) + ->method('linkToRouteAbsolute'); + // A missing file is expected (e.g. user deleted it), so no warning should be logged + $this->logger->expects($this->never()) + ->method('warning'); $this->logger->expects($this->once()) - ->method('warning') - ->with('Could not find file with id {fileId} for user {uid}', ['fileId' => '123', 'uid' => 'user']); + ->method('debug') + ->with('Could not find file with id {fileId} for user {uid}, discarding obsolete notification', ['fileId' => 123, 'uid' => 'user']); - $notification = $this->notifier->prepare($notification, 'en'); + $this->expectException(AlreadyProcessedException::class); - $this->assertEmpty($notification->getRichSubject()); - $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); + $this->notifier->prepare($notification, 'en'); } public function testFallbackToParsedSubjectIfMessageIsEmpty() {