fix(expire): delete conditions for activity_expire_exclude_users

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<array{0: string, 1: mixed, 2?: string}> $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<array{0: string, 1: mixed, 2?: string}> $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();

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 {

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();

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,20 +65,40 @@ 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();
 	}
 
 	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'");

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();

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);