Skip to content

Commit ecb0b15

Browse files
solracsfsolracsf
committed
fix(expire): delete conditions for activity_expire_exclude_users
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent 3b92c84 commit ecb0b15

6 files changed

Lines changed: 110 additions & 64 deletions

File tree

‎lib/Data.php‎

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -434,15 +434,13 @@ public function expire($expireDays = 365) {
434434
$ttl = (60 * 60 * 24 * max(1, $expireDays));
435435
$timelimit = time() - $ttl;
436436
$conditions = [
437-
'timestamp' => [$timelimit, '<'],
437+
['timestamp', $timelimit, '<'],
438438
];
439439

440440
$excludedUsers = $this->config->getSystemValue('activity_expire_exclude_users', []);
441-
if (!empty($excludedUsers)) {
441+
if (is_array($excludedUsers)) {
442442
foreach ($excludedUsers as $user) {
443-
$conditions[] = [
444-
'affecteduser' => [$user, '!=']
445-
];
443+
$conditions[] = ['affecteduser', $user, '!='];
446444
}
447445
}
448446

@@ -452,11 +450,14 @@ public function expire($expireDays = 365) {
452450
/**
453451
* Delete activities that match certain conditions
454452
*
455-
* @param array $conditions Array with conditions that have to be met
456-
* 'field' => 'value' => `field` = 'value'
457-
* 'field' => array('value', 'operator') => `field` operator 'value'
453+
* @param array $conditions List of conditions that all have to be met (combined with AND).
454+
* Each condition is a [column, value, operator] tuple, where the
455+
* operator is optional and defaults to '=':
456+
* ['field', 'value'] => `field` = 'value'
457+
* ['field', 'value', '!='] => `field` != 'value'
458+
* @psalm-param list<array{0: string, 1: mixed, 2?: string}> $conditions
458459
*/
459-
public function deleteActivities($conditions): void {
460+
public function deleteActivities(array $conditions): void {
460461
$platform = $this->connection->getDatabasePlatform();
461462
if ($platform instanceof MySQLPlatform) {
462463
$this->logger->debug('Choosing chunked activity delete for MySQL/MariaDB', ['app' => 'activity']);
@@ -467,21 +468,30 @@ public function deleteActivities($conditions): void {
467468
$deleteQuery = $this->connection->getQueryBuilder();
468469
$deleteQuery->delete('activity');
469470

470-
foreach ($conditions as $column => $comparison) {
471-
if (is_array($comparison)) {
472-
$operation = $comparison[1] ?? '=';
473-
$value = $comparison[0];
474-
} else {
475-
$operation = '=';
476-
$value = $comparison;
477-
}
478-
479-
$deleteQuery->andWhere($deleteQuery->expr()->comparison($column, $operation, $deleteQuery->createNamedParameter($value)));
480-
}
471+
$this->applyConditions($deleteQuery, $conditions);
481472
// Dont use chunked delete - let the DB handle the large row count natively
482473
$deleteQuery->executeStatement();
483474
}
484475

476+
/**
477+
* Apply a list of conditions to a query, combined with AND.
478+
*
479+
* Using andWhere() for every condition is required: where() would replace any
480+
* previously set restriction, silently dropping all but the last condition.
481+
*
482+
* @param IQueryBuilder $query
483+
* @param array $conditions List of [column, value, operator] tuples; operator defaults to '='
484+
* @psalm-param list<array{0: string, 1: mixed, 2?: string}> $conditions
485+
*/
486+
private function applyConditions(IQueryBuilder $query, array $conditions): void {
487+
foreach ($conditions as $condition) {
488+
$column = $condition[0];
489+
$value = $condition[1];
490+
$operation = $condition[2] ?? '=';
491+
$query->andWhere($query->expr()->comparison($column, $operation, $query->createNamedParameter($value)));
492+
}
493+
}
494+
485495
public function getById(int $activityId): ?IEvent {
486496
$query = $this->connection->getQueryBuilder();
487497
$query->select('*')
@@ -563,16 +573,7 @@ private function deleteActivitiesForMySQL(array $conditions): void {
563573
$query->select('activity_id')
564574
->from('activity');
565575

566-
foreach ($conditions as $column => $comparison) {
567-
if (is_array($comparison)) {
568-
$operation = $comparison[1] ?? '=';
569-
$value = $comparison[0];
570-
} else {
571-
$operation = '=';
572-
$value = $comparison;
573-
}
574-
$query->where($query->expr()->comparison($column, $operation, $query->createNamedParameter($value)));
575-
}
576+
$this->applyConditions($query, $conditions);
576577

577578
$query->setMaxResults(50000);
578579
$result = $query->executeQuery();

‎lib/Listener/UserDeleted.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public function handle(Event $event): void {
4343
}
4444

4545
private function deleteUserStream(IUser $user): void {
46-
$this->data->deleteActivities(['affecteduser' => $user->getUID()]);
46+
$this->data->deleteActivities([['affecteduser', $user->getUID()]]);
4747
}
4848

4949
private function deleteUserMailQueue(IUser $user): void {

‎tests/Controller/APIv1ControllerTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,13 +123,13 @@ protected function cleanUp(): void {
123123
$this->deleteUser($data, 'activity-api-user2');
124124

125125
$data->deleteActivities([
126-
'app' => 'app1',
126+
['app', 'app1'],
127127
]);
128128
}
129129

130130
protected function deleteUser(Data $data, string $uid): void {
131131
$data->deleteActivities([
132-
'affecteduser' => $uid,
132+
['affecteduser', $uid],
133133
]);
134134
$user = Server::get(IUserManager::class)->get($uid);
135135
$user?->delete();

‎tests/DataDeleteActivitiesTest.php‎

Lines changed: 74 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use OCP\AppFramework\Utility\ITimeFactory;
3232
use OCP\BackgroundJob\IJobList;
3333
use OCP\DB\IPreparedStatement;
34+
use OCP\DB\QueryBuilder\IQueryBuilder;
3435
use OCP\IConfig;
3536
use OCP\IDBConnection;
3637
use OCP\Server;
@@ -51,31 +52,11 @@ class DataDeleteActivitiesTest extends TestCase {
5152
protected function setUp(): void {
5253
parent::setUp();
5354

54-
$activities = [
55-
['affectedUser' => 'delete', 'subject' => 'subject', 'time' => 0],
56-
['affectedUser' => 'delete', 'subject' => 'subject2', 'time' => time() - 2 * 365 * 24 * 3600],
57-
['affectedUser' => 'otherUser', 'subject' => 'subject', 'time' => time()],
58-
['affectedUser' => 'otherUser', 'subject' => 'subject2', 'time' => time()],
59-
];
55+
$this->insertActivity('delete', 0, 'subject');
56+
$this->insertActivity('delete', time() - 2 * 365 * 24 * 3600, 'subject2');
57+
$this->insertActivity('otherUser', time(), 'subject');
58+
$this->insertActivity('otherUser', time(), 'subject2');
6059

61-
$queryActivity = Server::get(IDBConnection::class)
62-
->prepare('INSERT INTO `*PREFIX*activity`(`app`, `subject`, `subjectparams`, `message`, `messageparams`, `file`, `link`, `user`, `affecteduser`, `timestamp`, `priority`, `type`)' . ' VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )');
63-
foreach ($activities as $activity) {
64-
$queryActivity->execute([
65-
'app',
66-
$activity['subject'],
67-
json_encode([]),
68-
'',
69-
json_encode([]),
70-
'file',
71-
'link',
72-
'user',
73-
$activity['affectedUser'],
74-
$activity['time'],
75-
IExtension::PRIORITY_MEDIUM,
76-
'test',
77-
]);
78-
}
7960
$this->data = new Data(
8061
$this->createMock(IManager::class),
8162
Server::get(IDBConnection::class),
@@ -84,20 +65,40 @@ protected function setUp(): void {
8465
);
8566
}
8667

68+
private function insertActivity(string $affectedUser, int $time, string $subject = 'subject'): void {
69+
$query = Server::get(IDBConnection::class)->getQueryBuilder();
70+
$query->insert('activity')
71+
->values([
72+
'app' => $query->createNamedParameter('app'),
73+
'subject' => $query->createNamedParameter($subject),
74+
'subjectparams' => $query->createNamedParameter(json_encode([])),
75+
'message' => $query->createNamedParameter(''),
76+
'messageparams' => $query->createNamedParameter(json_encode([])),
77+
'file' => $query->createNamedParameter('file'),
78+
'link' => $query->createNamedParameter('link'),
79+
'user' => $query->createNamedParameter('user'),
80+
'affecteduser' => $query->createNamedParameter($affectedUser),
81+
'timestamp' => $query->createNamedParameter($time, IQueryBuilder::PARAM_INT),
82+
'priority' => $query->createNamedParameter(IExtension::PRIORITY_MEDIUM, IQueryBuilder::PARAM_INT),
83+
'type' => $query->createNamedParameter('test'),
84+
]);
85+
$query->executeStatement();
86+
}
87+
8788
protected function tearDown(): void {
8889
$this->data->deleteActivities([
89-
'type' => 'test',
90+
['type', 'test'],
9091
]);
9192

9293
parent::tearDown();
9394
}
9495

9596
public static function deleteActivitiesData(): array {
9697
return [
97-
[['affecteduser' => 'delete'], ['otherUser']],
98-
[['affecteduser' => ['delete', '=']], ['otherUser']],
99-
[['timestamp' => [time() - 10, '<']], ['otherUser']],
100-
[['timestamp' => [time() - 10, '>']], ['delete']],
98+
[[['affecteduser', 'delete']], ['otherUser']],
99+
[[['affecteduser', 'delete', '=']], ['otherUser']],
100+
[[['timestamp', time() - 10, '<']], ['otherUser']],
101+
[[['timestamp', time() - 10, '>']], ['delete']],
101102
];
102103
}
103104

@@ -125,6 +126,50 @@ public function testExpireActivities(): void {
125126
$this->assertUserActivities(['otherUser']);
126127
}
127128

129+
/**
130+
* Regression test for https://github.com/nextcloud/activity/issues/2647
131+
*
132+
* With 'activity_expire_exclude_users' set, expire() builds more than one
133+
* condition. This must:
134+
* - not crash (the excluded users used to be added in a shape the delete
135+
* query could not parse), and
136+
* - keep ANDing every condition together. On MySQL/MariaDB the chunked
137+
* delete used where() in a loop, which dropped all but the last condition
138+
* and would have deleted recent, non-expired activity.
139+
*/
140+
public function testExpireActivitiesWithExcludedUsers(): void {
141+
// Old activity for a user that is NOT excluded -> must still be deleted.
142+
$this->insertActivity('expired', 0, 'subject');
143+
144+
$config = $this->createMock(IConfig::class);
145+
$config->method('getSystemValue')
146+
->willReturnCallback(static function ($key, $default) {
147+
if ($key === 'activity_expire_exclude_users') {
148+
return ['delete', 'otherUser'];
149+
}
150+
return $default;
151+
});
152+
$data = new Data(
153+
$this->createMock(IManager::class),
154+
Server::get(IDBConnection::class),
155+
$this->createMock(LoggerInterface::class),
156+
$config,
157+
);
158+
159+
$time = $this->createMock(ITimeFactory::class);
160+
$time->method('getTime')
161+
->willReturn(time());
162+
$backgroundjob = new ExpireActivities($time, $data, $config);
163+
$backgroundjob->setId('1');
164+
165+
$this->assertUserActivities(['delete', 'expired', 'otherUser']);
166+
$backgroundjob->start($this->createMock(IJobList::class));
167+
168+
// 'delete' (old) survives because it is excluded; 'otherUser' (recent)
169+
// survives; 'expired' (old, not excluded) is the only one deleted.
170+
$this->assertUserActivities(['delete', 'otherUser']);
171+
}
172+
128173
protected function assertUserActivities(array $expected): void {
129174
$query = Server::get(IDBConnection::class)
130175
->prepare("SELECT `affecteduser` FROM `*PREFIX*activity` WHERE `type` = 'test'");

‎tests/DataTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ public function testDeleteAffectedUserActivities(): void {
341341

342342
$this->assertEquals(1, $this->countActivitiesForAffectedUser($user1));
343343
$this->assertEquals(1, $this->countActivitiesForAffectedUser($user2));
344-
$this->data->deleteActivities(['affecteduser' => $user1]);
344+
$this->data->deleteActivities([['affecteduser', $user1]]);
345345
$this->assertEquals(0, $this->countActivitiesForAffectedUser($user1));
346346
$this->assertEquals(1, $this->countActivitiesForAffectedUser($user2));
347347
$this->deleteTestActivities();

‎tests/Listener/UserDeletedTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public function setUp(): void {
5656
public function testUserDeleted(): void {
5757
$this->data->expects($this->once())
5858
->method('deleteActivities')
59-
->with(['affecteduser' => self::UID]);
59+
->with([['affecteduser', self::UID]]);
6060
$this->mailQueueHandler->expects($this->once())
6161
->method('purgeItemsForUser')
6262
->with(self::UID);

0 commit comments

Comments
 (0)