Merge pull request #30863 from nextcloud/performance/saving-user-profile-info

Minor optimizations for saving user personal information

5 files changed, 57 insertions, 47 deletions

diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php
index 1c1754ff752..f5ed9a548d1 100644
--- a/apps/dav/lib/CardDAV/CardDavBackend.php
+++ b/apps/dav/lib/CardDAV/CardDavBackend.php
@@ -648,23 +648,26 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 	 * @param mixed $addressBookId
 	 * @param string $cardUri
 	 * @param string $cardData
+	 * @param bool $checkAlreadyExists
 	 * @return string
 	 */
-	public function createCard($addressBookId, $cardUri, $cardData) {
+	public function createCard($addressBookId, $cardUri, $cardData, bool $checkAlreadyExists = true) {
 		$etag = md5($cardData);
 		$uid = $this->getUID($cardData);
 
-		$q = $this->db->getQueryBuilder();
-		$q->select('uid')
-			->from($this->dbCardsTable)
-			->where($q->expr()->eq('addressbookid', $q->createNamedParameter($addressBookId)))
-			->andWhere($q->expr()->eq('uid', $q->createNamedParameter($uid)))
-			->setMaxResults(1);
-		$result = $q->execute();
-		$count = (bool)$result->fetchOne();
-		$result->closeCursor();
-		if ($count) {
-			throw new \Sabre\DAV\Exception\BadRequest('VCard object with uid already exists in this addressbook collection.');
+		if ($checkAlreadyExists) {
+			$q = $this->db->getQueryBuilder();
+			$q->select('uid')
+				->from($this->dbCardsTable)
+				->where($q->expr()->eq('addressbookid', $q->createNamedParameter($addressBookId)))
+				->andWhere($q->expr()->eq('uid', $q->createNamedParameter($uid)))
+				->setMaxResults(1);
+			$result = $q->executeQuery();
+			$count = (bool)$result->fetchOne();
+			$result->closeCursor();
+			if ($count) {
+				throw new \Sabre\DAV\Exception\BadRequest('VCard object with uid already exists in this addressbook collection.');
+			}
 		}
 
 		$query = $this->db->getQueryBuilder();
@@ -1268,21 +1271,29 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 				]
 			);
 
-		foreach ($vCard->children() as $property) {
-			if (!in_array($property->name, self::$indexProperties)) {
-				continue;
-			}
-			$preferred = 0;
-			foreach ($property->parameters as $parameter) {
-				if ($parameter->name === 'TYPE' && strtoupper($parameter->getValue()) === 'PREF') {
-					$preferred = 1;
-					break;
+
+		$this->db->beginTransaction();
+
+		try {
+			foreach ($vCard->children() as $property) {
+				if (!in_array($property->name, self::$indexProperties)) {
+					continue;
+				}
+				$preferred = 0;
+				foreach ($property->parameters as $parameter) {
+					if ($parameter->name === 'TYPE' && strtoupper($parameter->getValue()) === 'PREF') {
+						$preferred = 1;
+						break;
+					}
 				}
+				$query->setParameter('name', $property->name);
+				$query->setParameter('value', mb_strcut($property->getValue(), 0, 254));
+				$query->setParameter('preferred', $preferred);
+				$query->execute();
 			}
-			$query->setParameter('name', $property->name);
-			$query->setParameter('value', mb_strcut($property->getValue(), 0, 254));
-			$query->setParameter('preferred', $preferred);
-			$query->execute();
+			$this->db->commit();
+		} catch (\Exception $e) {
+			$this->db->rollBack();
 		}
 	}
 
diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
index b93fd94f741..e1ac3af5cc9 100644
--- a/apps/dav/lib/CardDAV/SyncService.php
+++ b/apps/dav/lib/CardDAV/SyncService.php
@@ -265,12 +265,12 @@ class SyncService {
 		$userId = $user->getUID();
 
 		$cardId = "$name:$userId.vcf";
-		$card = $this->backend->getCard($addressBookId, $cardId);
 		if ($user->isEnabled()) {
+			$card = $this->backend->getCard($addressBookId, $cardId);
 			if ($card === false) {
 				$vCard = $this->converter->createCardFromUser($user);
 				if ($vCard !== null) {
-					$this->backend->createCard($addressBookId, $cardId, $vCard->serialize());
+					$this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false);
 				}
 			} else {
 				$vCard = $this->converter->createCardFromUser($user);
diff --git a/apps/dav/lib/HookManager.php b/apps/dav/lib/HookManager.php
index f0fdd5cfd4f..b69d9b0cd79 100644
--- a/apps/dav/lib/HookManager.php
+++ b/apps/dav/lib/HookManager.php
@@ -105,10 +105,7 @@ class HookManager {
 			$this->postDeleteUser(['uid' => $uid]);
 		});
 		\OC::$server->getUserManager()->listen('\OC\User', 'postUnassignedUserId', [$this, 'postUnassignedUserId']);
-		Util::connectHook('OC_User',
-			'changeUser',
-			$this,
-			'changeUser');
+		Util::connectHook('OC_User', 'changeUser', $this, 'changeUser');
 	}
 
 	public function postCreateUser($params) {
@@ -164,7 +161,12 @@ class HookManager {
 
 	public function changeUser($params) {
 		$user = $params['user'];
-		$this->syncService->updateUser($user);
+		$feature = $params['feature'];
+		// This case is already covered by the account manager firing up a signal
+		// later on
+		if ($feature !== 'eMailAddress' && $feature !== 'displayName') {
+			$this->syncService->updateUser($user);
+		}
 	}
 
 	public function firstLogin(IUser $user = null) {
diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php
index 5792ba1dc5d..7f79ab46c37 100644
--- a/lib/private/Accounts/AccountManager.php
+++ b/lib/private/Accounts/AccountManager.php
@@ -269,12 +269,15 @@ class AccountManager implements IAccountManager {
 		}
 	}
 
-	protected function updateUser(IUser $user, array $data, bool $throwOnData = false): array {
-		$oldUserData = $this->getUser($user, false);
+	protected function updateUser(IUser $user, array $data, ?array $oldUserData, bool $throwOnData = false): array {
+		if ($oldUserData === null) {
+			$oldUserData = $this->getUser($user, false);
+		}
+
 		$updated = true;
 
 		if ($oldUserData !== $data) {
-			$this->updateExistingUser($user, $data);
+			$this->updateExistingUser($user, $data, $oldUserData);
 		} else {
 			// nothing needs to be done if new and old data set are the same
 			$updated = false;
@@ -601,12 +604,9 @@ class AccountManager implements IAccountManager {
 	}
 
 	/**
-	 * update existing user in accounts table
-	 *
-	 * @param IUser $user
-	 * @param array $data
+	 * Update existing user in accounts table
 	 */
-	protected function updateExistingUser(IUser $user, array $data): void {
+	protected function updateExistingUser(IUser $user, array $data, array $oldData): void {
 		$uid = $user->getUID();
 		$jsonEncodedData = $this->prepareJson($data);
 		$query = $this->connection->getQueryBuilder();
@@ -820,7 +820,7 @@ class AccountManager implements IAccountManager {
 			];
 		}
 
-		$this->updateUser($account->getUser(), $data, true);
+		$this->updateUser($account->getUser(), $data, $oldData, true);
 		$this->internalCache->set($account->getUser()->getUID(), $account);
 	}
 }
diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php
index 9d54ef36c80..69deaf17d3c 100644
--- a/tests/lib/Accounts/AccountManagerTest.php
+++ b/tests/lib/Accounts/AccountManagerTest.php
@@ -424,7 +424,7 @@ class AccountManagerTest extends TestCase {
 			],
 		];
 		foreach ($users as $userInfo) {
-			$this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], false]);
+			$this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], null, false]);
 		}
 	}
 
@@ -466,9 +466,6 @@ class AccountManagerTest extends TestCase {
 		/** @var IUser $user */
 		$user = $this->createMock(IUser::class);
 
-		// FIXME: should be an integration test instead of this abomination
-		$accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($oldData);
-
 		if ($updateExisting) {
 			$accountManager->expects($this->once())->method('updateExistingUser')
 				->with($user, $newData);
@@ -497,10 +494,10 @@ class AccountManagerTest extends TestCase {
 				);
 		}
 
-		$this->invokePrivate($accountManager, 'updateUser', [$user, $newData]);
+		$this->invokePrivate($accountManager, 'updateUser', [$user, $newData, $oldData]);
 	}
 
-	public function dataTrueFalse() {
+	public function dataTrueFalse(): array {
 		return [
 			#$newData | $oldData | $insertNew | $updateExisting
 			[['myProperty' => ['value' => 'newData']], ['myProperty' => ['value' => 'oldData']], false, true],