From 4984816869dc12d82afee5c761c5d34c00c2ffb6 Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Wed, 22 Jul 2026 20:32:03 +0200 Subject: [PATCH] perf(api): avoid up to 200 notification queries when viewing file activities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When viewing the activities of a file, every returned activity was turned into a markProcessed() call, each of which queries and deletes against the notification storage — up to 200 round trips per sidebar request, mostly for notifications that do not exist. Skip activities of one's own actions (they never generate a notification) and bail out with a single COUNT query when the user has no notifications at all, which is the common case. Signed-off-by: Frank Karlitschek Co-Authored-By: Claude Fable 5 --- lib/Controller/APIv2Controller.php | 46 +++++++++--- tests/Controller/APIv2ControllerTest.php | 91 ++++++++++++++++++++++-- 2 files changed, 121 insertions(+), 16 deletions(-) diff --git a/lib/Controller/APIv2Controller.php b/lib/Controller/APIv2Controller.php index 4a41c1c46..6c4832edc 100644 --- a/lib/Controller/APIv2Controller.php +++ b/lib/Controller/APIv2Controller.php @@ -221,22 +221,46 @@ protected function get(string $filter, int $since, int $limit, bool $previews, s // When viewing activities for a specific file, mark corresponding activity // notifications as processed so they clear from the notification bell if ($this->objectType !== '' && $this->objectId !== 0 && !empty($this->user)) { - $deferred = $this->notificationManager->defer(); - foreach ($preparedActivities as $activity) { - $notification = $this->notificationManager->createNotification(); - $notification->setApp($activity['app'] ?? 'activity') - ->setUser($this->user) - ->setObject('activity_notification', (string)$activity['activity_id']); - $this->notificationManager->markProcessed($notification); - } - if ($deferred) { - $this->notificationManager->flush(); - } + $this->markActivityNotificationsProcessed($preparedActivities); } return new DataResponse($preparedActivities, Http::STATUS_OK, $headers); } + /** + * Mark the activity notifications of the given activities as processed + * so they clear from the notification bell. + * + * @param array[] $activities + */ + protected function markActivityNotificationsProcessed(array $activities): void { + // Notifications are never generated for one's own actions + $activities = array_filter($activities, fn (array $activity): bool => ($activity['user'] ?? '') !== $this->user); + if (empty($activities)) { + return; + } + + // Every markProcessed() call queries the notification storage, so + // bail out with a single query when the user has no notifications + $anyNotification = $this->notificationManager->createNotification(); + $anyNotification->setUser($this->user); + if ($this->notificationManager->getCount($anyNotification) === 0) { + return; + } + + $deferred = $this->notificationManager->defer(); + foreach ($activities as $activity) { + $notification = $this->notificationManager->createNotification(); + $notification->setApp($activity['app'] ?? 'activity') + ->setUser($this->user) + ->setObject('activity_notification', (string)$activity['activity_id']); + $this->notificationManager->markProcessed($notification); + } + if ($deferred) { + $this->notificationManager->flush(); + } + } + protected function generateHeaders(array $headers, bool $hasMoreActivities, array $data): array { if ($hasMoreActivities && isset($headers['X-Activity-Last-Given'])) { // Set the "Link" header for the next page diff --git a/tests/Controller/APIv2ControllerTest.php b/tests/Controller/APIv2ControllerTest.php index b133a48db..a54ab3aa9 100644 --- a/tests/Controller/APIv2ControllerTest.php +++ b/tests/Controller/APIv2ControllerTest.php @@ -522,8 +522,8 @@ public function testGetMarksActivityNotificationsProcessedForFile(): void { ->method('get') ->willReturn([ 'data' => [ - ['activity_id' => 11, 'app' => 'files', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], - ['activity_id' => 22, 'app' => 'comments', 'timestamp' => 1234567891, 'object_type' => 'files', 'object_id' => 42], + ['activity_id' => 11, 'app' => 'files', 'user' => 'user2', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], + ['activity_id' => 22, 'app' => 'comments', 'user' => 'user2', 'timestamp' => 1234567891, 'object_type' => 'files', 'object_id' => 42], ], 'headers' => ['ETag' => 'abc'], 'has_more' => false, @@ -532,10 +532,13 @@ public function testGetMarksActivityNotificationsProcessedForFile(): void { $notification = $this->createMock(INotification::class); $notification->method($this->anything())->willReturnSelf(); + $this->notificationManager->expects($this->once()) + ->method('getCount') + ->willReturn(5); $this->notificationManager->expects($this->once()) ->method('defer') ->willReturn(true); - $this->notificationManager->expects($this->exactly(2)) + $this->notificationManager->expects($this->exactly(3)) ->method('createNotification') ->willReturn($notification); $this->notificationManager->expects($this->exactly(2)) @@ -548,6 +551,79 @@ public function testGetMarksActivityNotificationsProcessedForFile(): void { $this->assertSame(Http::STATUS_OK, $result->getStatus()); } + public function testGetSkipsMarkingWhenUserHasNoNotifications(): void { + $controller = $this->getController(['validateParameters', 'generateHeaders']); + $controller->method('generateHeaders')->willReturnArgument(0); + $controller->method('validateParameters'); + + self::invokePrivate($controller, 'objectType', ['files']); + self::invokePrivate($controller, 'objectId', [42]); + self::invokePrivate($controller, 'user', ['user1']); + + $this->data->expects($this->once()) + ->method('get') + ->willReturn([ + 'data' => [ + ['activity_id' => 11, 'app' => 'files', 'user' => 'user2', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], + ], + 'headers' => ['ETag' => 'abc'], + 'has_more' => false, + ]); + + $notification = $this->createMock(INotification::class); + $notification->method($this->anything())->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('getCount') + ->willReturn(0); + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + $this->notificationManager->expects($this->never()) + ->method('defer'); + $this->notificationManager->expects($this->never()) + ->method('markProcessed'); + $this->notificationManager->expects($this->never()) + ->method('flush'); + + $result = self::invokePrivate($controller, 'get', ['all', 0, 50, false, 'files', 42, 'desc']); + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + + public function testGetSkipsMarkingForOwnActivities(): void { + $controller = $this->getController(['validateParameters', 'generateHeaders']); + $controller->method('generateHeaders')->willReturnArgument(0); + $controller->method('validateParameters'); + + self::invokePrivate($controller, 'objectType', ['files']); + self::invokePrivate($controller, 'objectId', [42]); + self::invokePrivate($controller, 'user', ['user1']); + + $this->data->expects($this->once()) + ->method('get') + ->willReturn([ + 'data' => [ + ['activity_id' => 11, 'app' => 'files', 'user' => 'user1', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], + ], + 'headers' => ['ETag' => 'abc'], + 'has_more' => false, + ]); + + $this->notificationManager->expects($this->never()) + ->method('getCount'); + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + $this->notificationManager->expects($this->never()) + ->method('defer'); + $this->notificationManager->expects($this->never()) + ->method('markProcessed'); + $this->notificationManager->expects($this->never()) + ->method('flush'); + + $result = self::invokePrivate($controller, 'get', ['all', 0, 50, false, 'files', 42, 'desc']); + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + public function testGetSkipsFlushWhenAlreadyDeferred(): void { $controller = $this->getController(['validateParameters', 'generateHeaders']); $controller->method('generateHeaders')->willReturnArgument(0); @@ -561,7 +637,7 @@ public function testGetSkipsFlushWhenAlreadyDeferred(): void { ->method('get') ->willReturn([ 'data' => [ - ['activity_id' => 11, 'app' => 'files', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], + ['activity_id' => 11, 'app' => 'files', 'user' => 'user2', 'timestamp' => 1234567890, 'object_type' => 'files', 'object_id' => 42], ], 'headers' => ['ETag' => 'abc'], 'has_more' => false, @@ -570,10 +646,13 @@ public function testGetSkipsFlushWhenAlreadyDeferred(): void { $notification = $this->createMock(INotification::class); $notification->method($this->anything())->willReturnSelf(); + $this->notificationManager->expects($this->once()) + ->method('getCount') + ->willReturn(1); $this->notificationManager->expects($this->once()) ->method('defer') ->willReturn(false); - $this->notificationManager->expects($this->once()) + $this->notificationManager->expects($this->exactly(2)) ->method('createNotification') ->willReturn($notification); $this->notificationManager->expects($this->once()) @@ -606,6 +685,8 @@ public function testGetDoesNotMarkNotificationsForGlobalStream(): void { 'has_more' => false, ]); + $this->notificationManager->expects($this->never()) + ->method('getCount'); $this->notificationManager->expects($this->never()) ->method('defer'); $this->notificationManager->expects($this->never())