From e454cc676568314a004dc667ad9b4faa6ba6dd21 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Thu, 12 Feb 2026 12:24:33 +0100 Subject: [PATCH] refactor(settings): Modernize authorized group classes Signed-off-by: provokateurin --- .../lib/Service/AuthorizedGroupService.php | 53 +++++++++++-------- build/rector-strict.php | 3 ++ lib/private/Settings/AuthorizedGroup.php | 18 ++++--- .../Settings/AuthorizedGroupMapper.php | 42 +++++++-------- psalm-strict.xml | 4 ++ 5 files changed, 68 insertions(+), 52 deletions(-) diff --git a/apps/settings/lib/Service/AuthorizedGroupService.php b/apps/settings/lib/Service/AuthorizedGroupService.php index 3189c504de2..223bf8ee19e 100644 --- a/apps/settings/lib/Service/AuthorizedGroupService.php +++ b/apps/settings/lib/Service/AuthorizedGroupService.php @@ -1,5 +1,7 @@ mapper->findAll(); @@ -30,43 +33,40 @@ class AuthorizedGroupService { /** * Find AuthorizedGroup by id. - * - * @param int $id + * @throws DoesNotExistException + * @throws Exception + * @throws MultipleObjectsReturnedException */ public function find(int $id): ?AuthorizedGroup { return $this->mapper->find($id); } /** - * @param $e * @throws NotFoundException + * @throws Throwable */ - private function handleException(\Exception $e): void { + private function handleException(Throwable $e): void { if ($e instanceof DoesNotExistException || $e instanceof MultipleObjectsReturnedException) { throw new NotFoundException('AuthorizedGroup not found'); - } else { - throw $e; } + + throw $e; } /** * Create a new AuthorizedGroup * - * @param string $groupId - * @param string $class - * @return AuthorizedGroup * @throws Exception * @throws ConflictException + * @throws MultipleObjectsReturnedException */ public function create(string $groupId, string $class): AuthorizedGroup { // Check if the group is already assigned to this class try { - $existing = $this->mapper->findByGroupIdAndClass($groupId, $class); - if ($existing) { - throw new ConflictException('Group is already assigned to this class'); - } - } catch (DoesNotExistException $e) { + $this->mapper->findByGroupIdAndClass($groupId, $class); + throw new ConflictException('Group is already assigned to this class'); + } catch (DoesNotExistException) { // This is expected when no duplicate exists, continue with creation } @@ -78,30 +78,37 @@ class AuthorizedGroupService { /** * @throws NotFoundException + * @throws Throwable */ public function delete(int $id): void { try { $authorizedGroup = $this->mapper->find($id); $this->mapper->delete($authorizedGroup); - } catch (\Exception $e) { - $this->handleException($e); + } catch (\Exception $exception) { + $this->handleException($exception); } } + /** + * @return list + */ public function findExistingGroupsForClass(string $class): array { try { - $authorizedGroup = $this->mapper->findExistingGroupsForClass($class); - return $authorizedGroup; - } catch (\Exception $e) { + return $this->mapper->findExistingGroupsForClass($class); + } catch (\Exception) { return []; } } + /** + * @throws Throwable + * @throws NotFoundException + */ public function removeAuthorizationAssociatedTo(IGroup $group): void { try { $this->mapper->removeGroup($group->getGID()); - } catch (\Exception $e) { - $this->handleException($e); + } catch (\Exception $exception) { + $this->handleException($exception); } } } diff --git a/build/rector-strict.php b/build/rector-strict.php index fa26a99a6cb..6490cf73d41 100644 --- a/build/rector-strict.php +++ b/build/rector-strict.php @@ -14,6 +14,9 @@ return (require __DIR__ . '/rector-shared.php') $nextcloudDir . '/lib/public/IContainer.php', $nextcloudDir . '/apps/dav/lib/Connector/Sabre/Node.php', $nextcloudDir . '/apps/files_versions/lib/Versions/IMetadataVersion.php', + $nextcloudDir . '/lib/private/Settings/AuthorizedGroup.php', + $nextcloudDir . '/lib/private/Settings/AuthorizedGroupMapper.php', + $nextcloudDir . '/apps/settings/lib/Service/AuthorizedGroupService.php', ]) ->withPreparedSets( deadCode: true, diff --git a/lib/private/Settings/AuthorizedGroup.php b/lib/private/Settings/AuthorizedGroup.php index 6949c0649aa..4ad9b228b5c 100644 --- a/lib/private/Settings/AuthorizedGroup.php +++ b/lib/private/Settings/AuthorizedGroup.php @@ -8,21 +8,25 @@ declare(strict_types=1); */ namespace OC\Settings; +use JsonSerializable; use OCP\AppFramework\Db\Entity; /** * @method setGroupId(string $groupId) * @method setClass(string $class) - * @method getGroupId(): string - * @method getClass(): string + * @method string getGroupId() + * @method string getClass() */ -class AuthorizedGroup extends Entity implements \JsonSerializable { - /** @var string $group_id */ - protected $groupId; +class AuthorizedGroup extends Entity implements JsonSerializable { + public $id; - /** @var string $class */ - protected $class; + protected ?string $groupId = null; + protected ?string $class = null; + + /** + * @return array + */ public function jsonSerialize(): array { return [ 'id' => $this->getId(), diff --git a/lib/private/Settings/AuthorizedGroupMapper.php b/lib/private/Settings/AuthorizedGroupMapper.php index f59d3fd7fd9..800dc5b8077 100644 --- a/lib/private/Settings/AuthorizedGroupMapper.php +++ b/lib/private/Settings/AuthorizedGroupMapper.php @@ -1,5 +1,7 @@ db->getQueryBuilder(); - /** @var IGroupManager $groupManager */ $groupManager = Server::get(IGroupManager::class); $groups = $groupManager->getUserGroups($user); if (count($groups) === 0) { return []; } - $result = $qb->select('class') + /** @var list $rows */ + $rows = $qb->select('class') ->from($this->getTableName(), 'auth') - ->where($qb->expr()->in('group_id', array_map(function (IGroup $group) use ($qb) { - return $qb->createNamedParameter($group->getGID()); - }, $groups), IQueryBuilder::PARAM_STR)) - ->executeQuery(); + ->where($qb->expr()->in('group_id', array_map(static fn (IGroup $group) => $qb->createNamedParameter($group->getGID()), $groups), IQueryBuilder::PARAM_STR)) + ->executeQuery() + ->fetchFirstColumn(); - $classes = []; - while ($row = $result->fetch()) { - $classes[] = $row['class']; - } - $result->closeCursor(); - return $classes; + return $rows; } /** * @throws DoesNotExistException * @throws MultipleObjectsReturnedException - * @throws \OCP\DB\Exception + * @throws Exception */ public function find(int $id): AuthorizedGroup { $queryBuilder = $this->db->getQueryBuilder(); $queryBuilder->select('*') ->from($this->getTableName()) ->where($queryBuilder->expr()->eq('id', $queryBuilder->createNamedParameter($id))); - /** @var AuthorizedGroup $authorizedGroup */ - $authorizedGroup = $this->findEntity($queryBuilder); - return $authorizedGroup; + return $this->findEntity($queryBuilder); } /** * Get all the authorizations stored in the database. * * @return AuthorizedGroup[] - * @throws \OCP\DB\Exception + * @throws Exception */ public function findAll(): array { $qb = $this->db->getQueryBuilder(); @@ -81,7 +74,12 @@ class AuthorizedGroupMapper extends QBMapper { return $this->findEntities($qb); } - public function findByGroupIdAndClass(string $groupId, string $class) { + /** + * @throws DoesNotExistException + * @throws Exception + * @throws MultipleObjectsReturnedException + */ + public function findByGroupIdAndClass(string $groupId, string $class): AuthorizedGroup { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) @@ -91,8 +89,8 @@ class AuthorizedGroupMapper extends QBMapper { } /** - * @return Entity[] - * @throws \OCP\DB\Exception + * @return list + * @throws Exception */ public function findExistingGroupsForClass(string $class): array { $qb = $this->db->getQueryBuilder(); @@ -105,7 +103,7 @@ class AuthorizedGroupMapper extends QBMapper { /** * @throws Exception */ - public function removeGroup(string $gid) { + public function removeGroup(string $gid): void { $qb = $this->db->getQueryBuilder(); $qb->delete($this->getTableName()) ->where($qb->expr()->eq('group_id', $qb->createNamedParameter($gid))) diff --git a/psalm-strict.xml b/psalm-strict.xml index 9a396165786..13e1aae88d5 100644 --- a/psalm-strict.xml +++ b/psalm-strict.xml @@ -20,6 +20,9 @@ + + + @@ -30,6 +33,7 @@ +