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)

diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php
index 1c1754ff752f35c50f640a46119e1025fd3727d0..ec1fef0b965cb753f4b39b6917e5eb67882846ed 100644 (file)
--- 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 b93fd94f7419c355a1e305e1c9985624e1cf70fd..e1ac3af5cc912daf7238a555d2a78cf598c37a14 100644 (file)
--- 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 f0fdd5cfd4f88cc5e073eaf72ef9a8566ad0f5a8..b69d9b0cd790944fe7d29af70200c3860360a024 100644 (file)
--- 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 5792ba1dc5d513b6f9ced315dd03ffa271c1c584..7f79ab46c37466048dde8bccdc71d50e015d1b0a 100644 (file)
--- 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 9d54ef36c80e405c60b4b9fc915d4127d93d5132..69deaf17d3c338ffd5699ab416ea83bf6ff627bd 100644 (file)
--- 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],