fix: Optimize repair step performance

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Feb 24, 2025 · d61b527 · d61b527
1 parent 5ec2091
commit d61b527
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 4 deletions.
diff --git a/lib/private/Repair/NC29/ValidateAccountProperties.php b/lib/private/Repair/NC29/ValidateAccountProperties.php
@@ -18,6 +18,13 @@
 
 class ValidateAccountProperties implements IRepairStep {
 
+	private const PROPERTIES_TO_CHECK = [
+		IAccountManager::PROPERTY_PHONE,
+		IAccountManager::PROPERTY_WEBSITE,
+		IAccountManager::PROPERTY_TWITTER,
+		IAccountManager::PROPERTY_FEDIVERSE,
+	];
+
 	public function __construct(
 		private IUserManager $userManager,
 		private IAccountManager $accountManager,
@@ -34,21 +41,32 @@ public function run(IOutput $output): void {
 
 		$this->userManager->callForSeenUsers(function (IUser $user) use (&$numRemoved) {
 			$account = $this->accountManager->getAccount($user);
-			while (true) {
+			$properties = array_keys($account->jsonSerialize());
+
+			// Check if there are some properties we can sanitize - reduces number of db queries
+			if (empty(array_intersect($properties, self::PROPERTIES_TO_CHECK))) {
+				return;
+			}
+
+			// Limit the loop to the properties we check to ensure there are no infinite loops
+			// we add one additional loop (+ 1) as we need 1 loop for checking + 1 for update.
+			$iteration = count(self::PROPERTIES_TO_CHECK) + 1;
+			while ($iteration-- > 0) {
 				try {
 					$this->accountManager->updateAccount($account);
-					break;
+					return;
 				} catch (InvalidArgumentException $e) {
 					if (in_array($e->getMessage(), IAccountManager::ALLOWED_PROPERTIES)) {
 						$numRemoved++;
 						$property = $account->getProperty($e->getMessage());
 						$account->setProperty($property->getName(), '', $property->getScope(), IAccountManager::NOT_VERIFIED);
 					} else {
 						$this->logger->error('Error while sanitizing account property', ['exception' => $e, 'user' => $user->getUID()]);
-						break;
+						return;
 					}
 				}
 			}
+			$this->logger->error('Iteration limit exceeded while cleaning account properties', ['user' => $user->getUID()]);
 		});
 
 		if ($numRemoved > 0) {

diff --git a/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php b/tests/lib/Repair/NC29/ValidateAccountPropertiesTest.php
@@ -43,6 +43,7 @@ public function testRun(): void {
 		$users = [
 			$this->createMock(IUser::class),
 			$this->createMock(IUser::class),
+			$this->createMock(IUser::class),
 		];
 		$this->userManager
 			->expects(self::once())
@@ -61,15 +62,39 @@ public function testRun(): void {
 		$account1->expects(self::once())
 			->method('setProperty')
 			->with(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED);
+		$account1->expects(self::once())
+			->method('jsonSerialize')
+			->willReturn([
+				IAccountManager::PROPERTY_DISPLAYNAME => [],
+				IAccountManager::PROPERTY_PHONE => [],
+			]);
+
 		$account2 = $this->createMock(IAccount::class);
 		$account2->expects(self::never())
 			->method('getProperty');
+		$account2->expects(self::once())
+			->method('jsonSerialize')
+			->willReturn([
+				IAccountManager::PROPERTY_DISPLAYNAME => [],
+				IAccountManager::PROPERTY_PHONE => [],
+			]);
+
+		$account3 = $this->createMock(IAccount::class);
+		$account3->expects(self::never())
+			->method('getProperty');
+		$account3->expects(self::once())
+			->method('jsonSerialize')
+			->willReturn([
+				IAccountManager::PROPERTY_DISPLAYNAME => [],
+			]);
+
 		$this->accountManager
-			->expects(self::exactly(2))
+			->expects(self::exactly(3))
 			->method('getAccount')
 			->willReturnMap([
 				[$users[0], $account1],
 				[$users[1], $account2],
+				[$users[2], $account3],
 			]);
 		$valid = false;
 		$this->accountManager->expects(self::exactly(3))