Minor optimizations for saving user personal information

* Remove double hook: the OC_User::changeUser triggers an
OC\AccountManager::userUpdated and the app is already listening to this
signal in its Application definition

* Make createCard not check if an card exists if we already checked
  previously. We also don't try to get the card if the user is disabled
  as we don't use the card in this case

We this change we go from 100 DB requests to 80 DB requests when saving
an user email address.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
(cherry picked from commit c6fd482edf33214a9ad4787e4cac278f871fa7c8)

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

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

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

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

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],