From 188874e0f7a7519887bba06d48cebdece7fe5d6d Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 22 Oct 2025 18:43:30 +0200 Subject: [PATCH 1/3] fix(performance): move array_merge outside of loop Signed-off-by: Anna Larch --- apps/provisioning_api/lib/Controller/UsersController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 12ada1c0213bf..59ea79f54f8d1 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -128,8 +128,9 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = $users = []; foreach ($subAdminOfGroups as $group) { - $users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); + $users[] = $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); } + $users = array_merge(...$users); } /** @var list $users */ From 96d7f1778dbad699879a1dbc847ac5e2aa53b9b0 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 28 Oct 2025 16:19:06 +0100 Subject: [PATCH 2/3] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/UsersController.php | 87 +++++++++---------- 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 59ea79f54f8d1..2f6f8757e2812 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -36,6 +36,8 @@ use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; use OCP\HintException; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; @@ -60,6 +62,7 @@ class UsersController extends AUserDataOCSController { private IL10N $l10n; + private ICache $cache; public function __construct( string $appName, @@ -81,6 +84,7 @@ public function __construct( private IEventDispatcher $eventDispatcher, private IPhoneNumberUtil $phoneNumberUtil, private IAppManager $appManager, + private ICacheFactory $cacheFactory, ) { parent::__construct( $appName, @@ -96,6 +100,7 @@ public function __construct( ); $this->l10n = $l10nFactory->get($appName); + $this->cache = $this->cacheFactory->createDistributed('usercache'); } /** @@ -111,31 +116,46 @@ public function __construct( #[NoAdminRequired] public function getUsers(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $user = $this->userSession->getUser(); - $users = []; - - // Admin? Or SubAdmin? $uid = $user->getUID(); - $subAdminManager = $this->groupManager->getSubAdmin(); + + $users = $this->cache->get($uid . '_' . $search); + if (!empty($users)) { + return new DataResponse([ + 'users' => $users + ]); + } + $isAdmin = $this->groupManager->isAdmin($uid); $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($uid); if ($isAdmin || $isDelegatedAdmin) { $users = $this->userManager->search($search, $limit, $offset); - } elseif ($subAdminManager->isSubAdmin($user)) { - $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($user); - foreach ($subAdminOfGroups as $key => $group) { - $subAdminOfGroups[$key] = $group->getGID(); - } + $users = array_keys($users); + $this->cache->set($uid . '_' . $search, $users, 10); + return new DataResponse([ + 'users' => $users + ]); + } - $users = []; - foreach ($subAdminOfGroups as $group) { - $users[] = $this->groupManager->displayNamesInGroup($group, $search, $limit, $offset); - } - $users = array_merge(...$users); + $users = []; + $subAdminManager = $this->groupManager->getSubAdmin(); + if (!$subAdminManager->isSubAdmin($user)) { + return new DataResponse([ + 'users' => $users + ]); + } + + $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($user); + foreach ($subAdminOfGroups as $key => $group) { + $subAdminOfGroups[$key] = $group->getGID(); } - /** @var list $users */ - $users = array_keys($users); + foreach ($subAdminOfGroups as $group) { + foreach ($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset) as $uid => $user) { + $users[] = $uid; + } + } + $this->cache->set($uid . '_' . $search, $users, 10); return new DataResponse([ 'users' => $users ]); @@ -153,29 +173,7 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = */ #[NoAdminRequired] public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { - $currentUser = $this->userSession->getUser(); - $users = []; - - // Admin? Or SubAdmin? - $uid = $currentUser->getUID(); - $subAdminManager = $this->groupManager->getSubAdmin(); - $isAdmin = $this->groupManager->isAdmin($uid); - $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($uid); - if ($isAdmin || $isDelegatedAdmin) { - $users = $this->userManager->search($search, $limit, $offset); - $users = array_keys($users); - } elseif ($subAdminManager->isSubAdmin($currentUser)) { - $subAdminOfGroups = $subAdminManager->getSubAdminsGroups($currentUser); - foreach ($subAdminOfGroups as $key => $group) { - $subAdminOfGroups[$key] = $group->getGID(); - } - - $users = []; - foreach ($subAdminOfGroups as $group) { - $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); - } - $users = array_merge(...$users); - } + $users = $this->getUsers($search, $limit, $offset)->getData()['users']; $usersDetails = []; foreach ($users as $userId) { @@ -188,14 +186,9 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of $userData = null; $this->logger->warning('Found one enabled account that is removed from its backend, but still exists in Nextcloud database', ['accountId' => $userId]); } - // Do not insert empty entry - if ($userData !== null) { - $usersDetails[$userId] = $userData; - } else { - // Logged user does not have permissions to see this user - // only showing its id - $usersDetails[$userId] = ['id' => $userId]; - } + + // $userdata === null means logged user does not have permissions to see this user + $usersDetails[$userId] = $userData ?? ['id' => $userId]; } return new DataResponse([ From d13c31a89cba4c80f50f87aa52462ef74e37c9d1 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 28 Oct 2025 16:43:57 +0100 Subject: [PATCH 3/3] fixup! fix(performance): move array_merge outside of loop --- .../lib/Controller/UsersController.php | 104 +++++++++--------- 1 file changed, 54 insertions(+), 50 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 2f6f8757e2812..cfe86a9946fbb 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -64,6 +64,8 @@ class UsersController extends AUserDataOCSController { private IL10N $l10n; private ICache $cache; + private const CACHE_PREFIX = 'provisioning_usercache'; + public function __construct( string $appName, IRequest $request, @@ -100,7 +102,7 @@ public function __construct( ); $this->l10n = $l10nFactory->get($appName); - $this->cache = $this->cacheFactory->createDistributed('usercache'); + $this->cache = $this->cacheFactory->createDistributed(self::CACHE_PREFIX); } /** @@ -827,6 +829,7 @@ public function editUserMultiValue( throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); $subAdminManager = $this->groupManager->getSubAdmin(); $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID()); $isAdminOrSubadmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()) @@ -925,6 +928,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); $permittedFields = []; if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { $allowDisplayNameChange = $this->config->getSystemValue('allow_user_to_change_display_name', true); @@ -1331,6 +1335,7 @@ public function deleteUser(string $userId): DataResponse { throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); // Go ahead with the delete if ($targetUser->delete()) { return new DataResponse(); @@ -1377,7 +1382,6 @@ public function enableUser(string $userId): DataResponse { */ private function setEnabled(string $userId, bool $value): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); - $targetUser = $this->userManager->get($userId); if ($targetUser === null || $targetUser->getUID() === $currentLoggedInUser->getUID()) { throw new OCSException('', 101); @@ -1391,6 +1395,7 @@ private function setEnabled(string $userId, bool $value): DataResponse { throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $this->cache->clear(self::CACHE_PREFIX); // enable/disable the user now $targetUser->setEnabled($value); return new DataResponse(); @@ -1423,22 +1428,21 @@ public function getUsersGroups(string $userId): DataResponse { return new DataResponse([ 'groups' => $this->groupManager->getUserGroupIds($targetUser) ]); - } else { - $subAdminManager = $this->groupManager->getSubAdmin(); + } - // Looking up someone else - if ($subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { - // Return the group that the method caller is subadmin of for the user in question - $groups = array_values(array_intersect( - array_map(static fn (IGroup $group) => $group->getGID(), $subAdminManager->getSubAdminsGroups($loggedInUser)), - $this->groupManager->getUserGroupIds($targetUser) - )); - return new DataResponse(['groups' => $groups]); - } else { - // Not permitted - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); - } + $subAdminManager = $this->groupManager->getSubAdmin(); + + // Looking up someone else + if (!$subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + + // Return the group that the method caller is subadmin of for the user in question + $groups = array_values(array_intersect( + array_map(static fn (IGroup $group) => $group->getGID(), $subAdminManager->getSubAdminsGroups($loggedInUser)), + $this->groupManager->getUserGroupIds($targetUser) + )); + return new DataResponse(['groups' => $groups]); } /** @@ -1481,41 +1485,41 @@ function (Group $group) { return new DataResponse([ 'groups' => $groups, ]); - } else { - $subAdminManager = $this->groupManager->getSubAdmin(); + } - // Looking up someone else - if ($subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { - // Return the group that the method caller is subadmin of for the user in question - $gids = array_values(array_intersect( - array_map( - static fn (IGroup $group) => $group->getGID(), - $subAdminManager->getSubAdminsGroups($loggedInUser), - ), - $this->groupManager->getUserGroupIds($targetUser) - )); - $groups = array_map( - function (string $gid) { - $group = $this->groupManager->get($gid); - return [ - 'id' => $group->getGID(), - 'displayname' => $group->getDisplayName(), - 'usercount' => $group->count(), - 'disabled' => $group->countDisabled(), - 'canAdd' => $group->canAddUser(), - 'canRemove' => $group->canRemoveUser(), - ]; - }, - $gids, - ); - return new DataResponse([ - 'groups' => $groups, - ]); - } else { - // Not permitted - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); - } + $subAdminManager = $this->groupManager->getSubAdmin(); + + // Looking up someone else + if (!$subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { + // Not permitted + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + + // Return the group that the method caller is subadmin of for the user in question + $gids = array_values(array_intersect( + array_map( + static fn (IGroup $group) => $group->getGID(), + $subAdminManager->getSubAdminsGroups($loggedInUser), + ), + $this->groupManager->getUserGroupIds($targetUser) + )); + $groups = array_map( + function (string $gid) { + $group = $this->groupManager->get($gid); + return [ + 'id' => $group->getGID(), + 'displayname' => $group->getDisplayName(), + 'usercount' => $group->count(), + 'disabled' => $group->countDisabled(), + 'canAdd' => $group->canAddUser(), + 'canRemove' => $group->canRemoveUser(), + ]; + }, + $gids, + ); + return new DataResponse([ + 'groups' => $groups, + ]); } /** @@ -1617,7 +1621,7 @@ public function addToGroup(string $userId, string $groupid = ''): DataResponse { public function removeFromGroup(string $userId, string $groupid): DataResponse { $loggedInUser = $this->userSession->getUser(); - if ($groupid === null || trim($groupid) === '') { + if (trim($groupid) === '') { throw new OCSException('', 101); }