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

* Don't make AccountManager::deleteUser, delete the user twice

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

Author: CarlSchwan

apps/dav/lib/CardDAV/CardDavBackend.php

@@ -650,21 +650,23 @@ public function getMultipleCards($addressBookId, array $uris) {
 	 * @param string $cardData
 	 * @return string
 	 */
-	public function createCard($addressBookId, $cardUri, $cardData) {
+	public function createCard($addressBookId, $cardUri, $cardData, $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->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.');
+			}
 		}
 
 		$query = $this->db->getQueryBuilder();

apps/dav/lib/CardDAV/SyncService.php

@@ -268,12 +268,12 @@ public function updateUser(IUser $user) {
 		$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

@@ -104,10 +104,6 @@ public function setup() {
 			$this->postDeleteUser(['uid' => $uid]);
 		});
 		\OC::$server->getUserManager()->listen('\OC\User', 'postUnassignedUserId', [$this, 'postUnassignedUserId']);
-		Util::connectHook('OC_User',
-			'changeUser',
-			$this,
-			'changeUser');
 	}
 
 	public function postCreateUser($params) {
@@ -161,11 +157,6 @@ public function postUnassignedUserId($uid) {
 		}
 	}
 
-	public function changeUser($params) {
-		$user = $params['user'];
-		$this->syncService->updateUser($user);
-	}
-
 	public function firstLogin(IUser $user = null) {
 		if (!is_null($user)) {
 			$principal = 'principals/users/' . $user->getUID();

lib/private/Accounts/AccountManager.php

@@ -263,12 +263,15 @@ protected function sanitizeWebsite(IAccountProperty $property, bool $throwOnData
 		}
 	}
 
-	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;
@@ -595,12 +598,9 @@ protected function importFromJson(string $json, string $userId): ?array {
 	}
 
 	/**
-	 * 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();
@@ -809,6 +809,6 @@ public function updateAccount(IAccount $account): void {
 			];
 		}
 
-		$this->updateUser($account->getUser(), $data, true);
+		$this->updateUser($account->getUser(), $data, $oldData, true);
 	}
 }

tests/lib/Accounts/AccountManagerTest.php

@@ -424,7 +424,7 @@ protected function populateOrUpdate(): void {
 			],
 		];
 		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 @@ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting)
 		/** @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 @@ function ($eventName, $event) use ($user, $newData) {
 				);
 		}
 
-		$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],