Skip to content

Commit e7a6492

Browse files
miaulalalamiaulalala
authored
Merge pull request #2655 from nextcloud/fix/correct-delete-conditions
fix(expire): delete conditions for activity_expire_exclude_users
2 parents ff3677f + 61423f9 commit e7a6492

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
@@ -475,15 +475,13 @@ public function expire($expireDays = 365) {
475475
$ttl = (60 * 60 * 24 * max(1, $expireDays));
476476
$timelimit = time() - $ttl;
477477
$conditions = [
478-
'timestamp' => [$timelimit, '<'],
478+
['timestamp', $timelimit, '<'],
479479
];
480480

481481
$excludedUsers = $this->config->getSystemValue('activity_expire_exclude_users', []);
482-
if (!empty($excludedUsers)) {
482+
if (is_array($excludedUsers)) {
483483
foreach ($excludedUsers as $user) {
484-
$conditions[] = [
485-
'affecteduser' => [$user, '!=']
486-
];
484+
$conditions[] = ['affecteduser', $user, '!='];
487485
}
488486
}
489487

@@ -493,11 +491,14 @@ public function expire($expireDays = 365) {
493491
/**
494492
* Delete activities that match certain conditions
495493
*
496-
* @param array $conditions Array with conditions that have to be met
497-
* 'field' => 'value' => `field` = 'value'
498-
* 'field' => array('value', 'operator') => `field` operator 'value'
494+
* @param array $conditions List of conditions that all have to be met (combined with AND).
495+
* Each condition is a [column, value, operator] tuple, where the
496+
* operator is optional and defaults to '=':
497+
* ['field', 'value'] => `field` = 'value'
498+
* ['field', 'value', '!='] => `field` != 'value'
499+
* @psalm-param list<array{0: string, 1: mixed, 2?: string}> $conditions
499500
*/
500-
public function deleteActivities($conditions): void {
501+
public function deleteActivities(array $conditions): void {
501502
$platform = $this->connection->getDatabasePlatform();
502503
if ($platform instanceof MySQLPlatform) {
503504
$this->logger->debug('Choosing chunked activity delete for MySQL/MariaDB', ['app' => 'activity']);
@@ -508,21 +509,30 @@ public function deleteActivities($conditions): void {
508509
$deleteQuery = $this->connection->getQueryBuilder();
509510
$deleteQuery->delete('activity');
510511

511-
foreach ($conditions as $column => $comparison) {
512-
if (is_array($comparison)) {
513-
$operation = $comparison[1] ?? '=';
514-
$value = $comparison[0];
515-
} else {
516-
$operation = '=';
517-
$value = $comparison;
518-
}
519-
520-
$deleteQuery->andWhere($deleteQuery->expr()->comparison($column, $operation, $deleteQuery->createNamedParameter($value)));
521-
}
512+
$this->applyConditions($deleteQuery, $conditions);
522513
// Dont use chunked delete - let the DB handle the large row count natively
523514
$deleteQuery->executeStatement();
524515
}
525516

517+
/**
518+
* Apply a list of conditions to a query, combined with AND.
519+
*
520+
* Using andWhere() for every condition is required: where() would replace any
521+
* previously set restriction, silently dropping all but the last condition.
522+
*
523+
* @param IQueryBuilder $query
524+
* @param array $conditions List of [column, value, operator] tuples; operator defaults to '='
525+
* @psalm-param list<array{0: string, 1: mixed, 2?: string}> $conditions
526+
*/
527+
private function applyConditions(IQueryBuilder $query, array $conditions): void {
528+
foreach ($conditions as $condition) {
529+
$column = $condition[0];
530+
$value = $condition[1];
531+
$operation = $condition[2] ?? '=';
532+
$query->andWhere($query->expr()->comparison($column, $operation, $query->createNamedParameter($value)));
533+
}
534+
}
535+
526536
public function getById(int $activityId): ?IEvent {
527537
$query = $this->connection->getQueryBuilder();
528538
$query->select('*')
@@ -604,16 +614,7 @@ private function deleteActivitiesForMySQL(array $conditions): void {
604614
$query->select('activity_id')
605615
->from('activity');
606616

607-
foreach ($conditions as $column => $comparison) {
608-
if (is_array($comparison)) {
609-
$operation = $comparison[1] ?? '=';
610-
$value = $comparison[0];
611-
} else {
612-
$operation = '=';
613-
$value = $comparison;
614-
}
615-
$query->where($query->expr()->comparison($column, $operation, $query->createNamedParameter($value)));
616-
}
617+
$this->applyConditions($query, $conditions);
617618

618619
$query->setMaxResults(50000);
619620
$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)