Skip to content

perf(api): avoid up to 200 notification queries when viewing file act… #2816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
karlitschek wants to merge 1 commit into
base: master
Choose a base branch
from
+121 −16
Open

perf(api): avoid up to 200 notification queries when viewing file act… #2816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions lib/Controller/APIv2Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment on lines +230 to +236
// 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
Expand Down
91 changes: 86 additions & 5 deletions tests/Controller/APIv2ControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))
Expand All @@ -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');
Comment on lines +576 to +587

$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);
Expand All @@ -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,
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand Down
Loading