diff --git a/lib/Data.php b/lib/Data.php index 06e7867af..318799be2 100644 --- a/lib/Data.php +++ b/lib/Data.php @@ -434,15 +434,13 @@ public function expire($expireDays = 365) { $ttl = (60 * 60 * 24 * max(1, $expireDays)); $timelimit = time() - $ttl; $conditions = [ - 'timestamp' => [$timelimit, '<'], + ['timestamp', $timelimit, '<'], ]; $excludedUsers = $this->config->getSystemValue('activity_expire_exclude_users', []); - if (!empty($excludedUsers)) { + if (is_array($excludedUsers)) { foreach ($excludedUsers as $user) { - $conditions[] = [ - 'affecteduser' => [$user, '!='] - ]; + $conditions[] = ['affecteduser', $user, '!=']; } } @@ -452,11 +450,14 @@ public function expire($expireDays = 365) { /** * Delete activities that match certain conditions * - * @param array $conditions Array with conditions that have to be met - * 'field' => 'value' => `field` = 'value' - * 'field' => array('value', 'operator') => `field` operator 'value' + * @param array $conditions List of conditions that all have to be met (combined with AND). + * Each condition is a [column, value, operator] tuple, where the + * operator is optional and defaults to '=': + * ['field', 'value'] => `field` = 'value' + * ['field', 'value', '!='] => `field` != 'value' + * @psalm-param list $conditions */ - public function deleteActivities($conditions): void { + public function deleteActivities(array $conditions): void { $platform = $this->connection->getDatabasePlatform(); if ($platform instanceof MySQLPlatform) { $this->logger->debug('Choosing chunked activity delete for MySQL/MariaDB', ['app' => 'activity']); @@ -467,21 +468,30 @@ public function deleteActivities($conditions): void { $deleteQuery = $this->connection->getQueryBuilder(); $deleteQuery->delete('activity'); - foreach ($conditions as $column => $comparison) { - if (is_array($comparison)) { - $operation = $comparison[1] ?? '='; - $value = $comparison[0]; - } else { - $operation = '='; - $value = $comparison; - } - - $deleteQuery->andWhere($deleteQuery->expr()->comparison($column, $operation, $deleteQuery->createNamedParameter($value))); - } + $this->applyConditions($deleteQuery, $conditions); // Dont use chunked delete - let the DB handle the large row count natively $deleteQuery->executeStatement(); } + /** + * Apply a list of conditions to a query, combined with AND. + * + * Using andWhere() for every condition is required: where() would replace any + * previously set restriction, silently dropping all but the last condition. + * + * @param IQueryBuilder $query + * @param array $conditions List of [column, value, operator] tuples; operator defaults to '=' + * @psalm-param list $conditions + */ + private function applyConditions(IQueryBuilder $query, array $conditions): void { + foreach ($conditions as $condition) { + $column = $condition[0]; + $value = $condition[1]; + $operation = $condition[2] ?? '='; + $query->andWhere($query->expr()->comparison($column, $operation, $query->createNamedParameter($value))); + } + } + public function getById(int $activityId): ?IEvent { $query = $this->connection->getQueryBuilder(); $query->select('*') @@ -563,16 +573,7 @@ private function deleteActivitiesForMySQL(array $conditions): void { $query->select('activity_id') ->from('activity'); - foreach ($conditions as $column => $comparison) { - if (is_array($comparison)) { - $operation = $comparison[1] ?? '='; - $value = $comparison[0]; - } else { - $operation = '='; - $value = $comparison; - } - $query->where($query->expr()->comparison($column, $operation, $query->createNamedParameter($value))); - } + $this->applyConditions($query, $conditions); $query->setMaxResults(50000); $result = $query->executeQuery(); diff --git a/lib/Listener/UserDeleted.php b/lib/Listener/UserDeleted.php index 47bb2adbf..25c1bf3af 100644 --- a/lib/Listener/UserDeleted.php +++ b/lib/Listener/UserDeleted.php @@ -43,7 +43,7 @@ public function handle(Event $event): void { } private function deleteUserStream(IUser $user): void { - $this->data->deleteActivities(['affecteduser' => $user->getUID()]); + $this->data->deleteActivities([['affecteduser', $user->getUID()]]); } private function deleteUserMailQueue(IUser $user): void { diff --git a/tests/Controller/APIv1ControllerTest.php b/tests/Controller/APIv1ControllerTest.php index 53ea144e9..163866360 100644 --- a/tests/Controller/APIv1ControllerTest.php +++ b/tests/Controller/APIv1ControllerTest.php @@ -123,13 +123,13 @@ protected function cleanUp(): void { $this->deleteUser($data, 'activity-api-user2'); $data->deleteActivities([ - 'app' => 'app1', + ['app', 'app1'], ]); } protected function deleteUser(Data $data, string $uid): void { $data->deleteActivities([ - 'affecteduser' => $uid, + ['affecteduser', $uid], ]); $user = Server::get(IUserManager::class)->get($uid); $user?->delete(); diff --git a/tests/DataDeleteActivitiesTest.php b/tests/DataDeleteActivitiesTest.php index 715eb01ac..d4da51954 100644 --- a/tests/DataDeleteActivitiesTest.php +++ b/tests/DataDeleteActivitiesTest.php @@ -31,6 +31,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJobList; use OCP\DB\IPreparedStatement; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IConfig; use OCP\IDBConnection; use OCP\Server; @@ -51,31 +52,11 @@ class DataDeleteActivitiesTest extends TestCase { protected function setUp(): void { parent::setUp(); - $activities = [ - ['affectedUser' => 'delete', 'subject' => 'subject', 'time' => 0], - ['affectedUser' => 'delete', 'subject' => 'subject2', 'time' => time() - 2 * 365 * 24 * 3600], - ['affectedUser' => 'otherUser', 'subject' => 'subject', 'time' => time()], - ['affectedUser' => 'otherUser', 'subject' => 'subject2', 'time' => time()], - ]; + $this->insertActivity('delete', 0, 'subject'); + $this->insertActivity('delete', time() - 2 * 365 * 24 * 3600, 'subject2'); + $this->insertActivity('otherUser', time(), 'subject'); + $this->insertActivity('otherUser', time(), 'subject2'); - $queryActivity = Server::get(IDBConnection::class) - ->prepare('INSERT INTO `*PREFIX*activity`(`app`, `subject`, `subjectparams`, `message`, `messageparams`, `file`, `link`, `user`, `affecteduser`, `timestamp`, `priority`, `type`)' . ' VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )'); - foreach ($activities as $activity) { - $queryActivity->execute([ - 'app', - $activity['subject'], - json_encode([]), - '', - json_encode([]), - 'file', - 'link', - 'user', - $activity['affectedUser'], - $activity['time'], - IExtension::PRIORITY_MEDIUM, - 'test', - ]); - } $this->data = new Data( $this->createMock(IManager::class), Server::get(IDBConnection::class), @@ -84,9 +65,29 @@ protected function setUp(): void { ); } + private function insertActivity(string $affectedUser, int $time, string $subject = 'subject'): void { + $query = Server::get(IDBConnection::class)->getQueryBuilder(); + $query->insert('activity') + ->values([ + 'app' => $query->createNamedParameter('app'), + 'subject' => $query->createNamedParameter($subject), + 'subjectparams' => $query->createNamedParameter(json_encode([])), + 'message' => $query->createNamedParameter(''), + 'messageparams' => $query->createNamedParameter(json_encode([])), + 'file' => $query->createNamedParameter('file'), + 'link' => $query->createNamedParameter('link'), + 'user' => $query->createNamedParameter('user'), + 'affecteduser' => $query->createNamedParameter($affectedUser), + 'timestamp' => $query->createNamedParameter($time, IQueryBuilder::PARAM_INT), + 'priority' => $query->createNamedParameter(IExtension::PRIORITY_MEDIUM, IQueryBuilder::PARAM_INT), + 'type' => $query->createNamedParameter('test'), + ]); + $query->executeStatement(); + } + protected function tearDown(): void { $this->data->deleteActivities([ - 'type' => 'test', + ['type', 'test'], ]); parent::tearDown(); @@ -94,10 +95,10 @@ protected function tearDown(): void { public static function deleteActivitiesData(): array { return [ - [['affecteduser' => 'delete'], ['otherUser']], - [['affecteduser' => ['delete', '=']], ['otherUser']], - [['timestamp' => [time() - 10, '<']], ['otherUser']], - [['timestamp' => [time() - 10, '>']], ['delete']], + [[['affecteduser', 'delete']], ['otherUser']], + [[['affecteduser', 'delete', '=']], ['otherUser']], + [[['timestamp', time() - 10, '<']], ['otherUser']], + [[['timestamp', time() - 10, '>']], ['delete']], ]; } @@ -125,6 +126,50 @@ public function testExpireActivities(): void { $this->assertUserActivities(['otherUser']); } + /** + * Regression test for https://github.com/nextcloud/activity/issues/2647 + * + * With 'activity_expire_exclude_users' set, expire() builds more than one + * condition. This must: + * - not crash (the excluded users used to be added in a shape the delete + * query could not parse), and + * - keep ANDing every condition together. On MySQL/MariaDB the chunked + * delete used where() in a loop, which dropped all but the last condition + * and would have deleted recent, non-expired activity. + */ + public function testExpireActivitiesWithExcludedUsers(): void { + // Old activity for a user that is NOT excluded -> must still be deleted. + $this->insertActivity('expired', 0, 'subject'); + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue') + ->willReturnCallback(static function ($key, $default) { + if ($key === 'activity_expire_exclude_users') { + return ['delete', 'otherUser']; + } + return $default; + }); + $data = new Data( + $this->createMock(IManager::class), + Server::get(IDBConnection::class), + $this->createMock(LoggerInterface::class), + $config, + ); + + $time = $this->createMock(ITimeFactory::class); + $time->method('getTime') + ->willReturn(time()); + $backgroundjob = new ExpireActivities($time, $data, $config); + $backgroundjob->setId('1'); + + $this->assertUserActivities(['delete', 'expired', 'otherUser']); + $backgroundjob->start($this->createMock(IJobList::class)); + + // 'delete' (old) survives because it is excluded; 'otherUser' (recent) + // survives; 'expired' (old, not excluded) is the only one deleted. + $this->assertUserActivities(['delete', 'otherUser']); + } + protected function assertUserActivities(array $expected): void { $query = Server::get(IDBConnection::class) ->prepare("SELECT `affecteduser` FROM `*PREFIX*activity` WHERE `type` = 'test'"); diff --git a/tests/DataTest.php b/tests/DataTest.php index 1ff577a49..505ebace9 100644 --- a/tests/DataTest.php +++ b/tests/DataTest.php @@ -341,7 +341,7 @@ public function testDeleteAffectedUserActivities(): void { $this->assertEquals(1, $this->countActivitiesForAffectedUser($user1)); $this->assertEquals(1, $this->countActivitiesForAffectedUser($user2)); - $this->data->deleteActivities(['affecteduser' => $user1]); + $this->data->deleteActivities([['affecteduser', $user1]]); $this->assertEquals(0, $this->countActivitiesForAffectedUser($user1)); $this->assertEquals(1, $this->countActivitiesForAffectedUser($user2)); $this->deleteTestActivities(); diff --git a/tests/Listener/UserDeletedTest.php b/tests/Listener/UserDeletedTest.php index 3e6c36455..eea832455 100644 --- a/tests/Listener/UserDeletedTest.php +++ b/tests/Listener/UserDeletedTest.php @@ -56,7 +56,7 @@ public function setUp(): void { public function testUserDeleted(): void { $this->data->expects($this->once()) ->method('deleteActivities') - ->with(['affecteduser' => self::UID]); + ->with([['affecteduser', self::UID]]); $this->mailQueueHandler->expects($this->once()) ->method('purgeItemsForUser') ->with(self::UID);