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 1c1754ff752..ec1fef0b965 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(); 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], 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 ec1fef0b965..f5ed9a548d1 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -1271,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(); } }