From eef973e85ba5e5cc322079ff85a599d42f1334e6 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 26 Jan 2022 20:58:39 +0100 Subject: [PATCH 1/2] 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 (cherry picked from commit c6fd482edf33214a9ad4787e4cac278f871fa7c8) --- apps/dav/lib/CardDAV/CardDavBackend.php | 27 +++++++++++++---------- apps/dav/lib/CardDAV/SyncService.php | 4 ++-- apps/dav/lib/HookManager.php | 12 +++++----- lib/private/Accounts/AccountManager.php | 18 +++++++-------- tests/lib/Accounts/AccountManagerTest.php | 9 +++----- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index 1c1754ff752f3..ec1fef0b965cb 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -648,23 +648,26 @@ public function getMultipleCards($addressBookId, array $uris) { * @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(); diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index b93fd94f7419c..e1ac3af5cc912 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -265,12 +265,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); diff --git a/apps/dav/lib/HookManager.php b/apps/dav/lib/HookManager.php index f0fdd5cfd4f88..b69d9b0cd7909 100644 --- a/apps/dav/lib/HookManager.php +++ b/apps/dav/lib/HookManager.php @@ -105,10 +105,7 @@ public function setup() { $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 @@ public function postUnassignedUserId($uid) { 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 5792ba1dc5d51..7f79ab46c3746 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -269,12 +269,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; @@ -601,12 +604,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(); @@ -820,7 +820,7 @@ public function updateAccount(IAccount $account): void { ]; } - $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 9d54ef36c80e4..69deaf17d3c33 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/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], From e71db404923f8c8b53e7968f8a10d3e7de0abe2a Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 3 Feb 2022 21:41:17 +0100 Subject: [PATCH 2/2] Wrap multiple inserts inside a transation Signed-off-by: Carl Schwan --- apps/dav/lib/CardDAV/CardDavBackend.php | 34 +++++++++++++++---------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index ec1fef0b965cb..f5ed9a548d165 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -1271,21 +1271,29 @@ protected function updateProperties($addressBookId, $cardUri, $vCardSerialized) ] ); - 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(); } }