From 14823d607608ddb0ac513d81eecba102dd5ae071 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 7 Jul 2026 13:29:29 +0200 Subject: [PATCH 1/3] perf: Introduce LazyGroup Similar to LazyUser, this allow to get a IGroup object where properties are lazyly evaluated only when needed. This also reuse the DisplayNameCache from the IGroupManager. Signed-off-by: Carl Schwan --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Group/Group.php | 6 + lib/private/Group/LazyGroup.php | 132 ++++++++++++++++++++ lib/private/Group/Manager.php | 19 +-- lib/public/IGroup.php | 7 ++ tests/lib/Group/ManagerTest.php | 30 ++--- 7 files changed, 157 insertions(+), 39 deletions(-) create mode 100644 lib/private/Group/LazyGroup.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 83b5dbeb66a9e..c658abe5eada9 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1880,6 +1880,7 @@ 'OC\\Group\\Database' => $baseDir . '/lib/private/Group/Database.php', 'OC\\Group\\DisplayNameCache' => $baseDir . '/lib/private/Group/DisplayNameCache.php', 'OC\\Group\\Group' => $baseDir . '/lib/private/Group/Group.php', + 'OC\\Group\\LazyGroup' => $baseDir . '/lib/private/Group/LazyGroup.php', 'OC\\Group\\Manager' => $baseDir . '/lib/private/Group/Manager.php', 'OC\\Group\\MetaData' => $baseDir . '/lib/private/Group/MetaData.php', 'OC\\HintException' => $baseDir . '/lib/private/HintException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 1b0383a23142e..4391bcd9f9418 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1921,6 +1921,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Group\\Database' => __DIR__ . '/../../..' . '/lib/private/Group/Database.php', 'OC\\Group\\DisplayNameCache' => __DIR__ . '/../../..' . '/lib/private/Group/DisplayNameCache.php', 'OC\\Group\\Group' => __DIR__ . '/../../..' . '/lib/private/Group/Group.php', + 'OC\\Group\\LazyGroup' => __DIR__ . '/../../..' . '/lib/private/Group/LazyGroup.php', 'OC\\Group\\Manager' => __DIR__ . '/../../..' . '/lib/private/Group/Manager.php', 'OC\\Group\\MetaData' => __DIR__ . '/../../..' . '/lib/private/Group/MetaData.php', 'OC\\HintException' => __DIR__ . '/../../..' . '/lib/private/HintException.php', diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index 65aff5d225b4a..a6924be6e03dc 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -40,6 +40,7 @@ class Group implements IGroup { /** @var User[] */ private array $users = []; private bool $usersLoaded = false; + private bool $isDeleted = false; public function __construct( private string $gid, @@ -336,6 +337,7 @@ public function delete(): bool { $this->emitter->emit('\OC\Group', 'postDelete', [$this]); } } + $this->isDeleted = $result; return $result; } @@ -393,4 +395,8 @@ public function hideFromCollaboration(): bool { return $hide || ($backend instanceof IHideFromCollaborationBackend && $backend->hideGroup($this->gid)); }, false); } + + public function isDeleted(): bool { + return $this->isDeleted; + } } diff --git a/lib/private/Group/LazyGroup.php b/lib/private/Group/LazyGroup.php new file mode 100644 index 0000000000000..0a6a1e831046c --- /dev/null +++ b/lib/private/Group/LazyGroup.php @@ -0,0 +1,132 @@ +gid; + } + + private function getGroup(): IGroup { + if ($this->group === null) { + $this->group = $this->groupManager->get($this->gid); + ; + } + if ($this->group === null) { + Server::get(LoggerInterface::class)->debug('Trying to use the deleted group: "' . $this->gid . '"', ['app' => 'core']); + $this->isDeleted = true; + $this->group = new Group($this->gid, [], Server::get(IEventDispatcher::class), Server::get(IUserManager::class)); + } else { + $this->isDeleted = false; + } + return $this->group; + } + + #[\Override] + public function getDisplayName(): string { + // Use display name cache from IGroupManager + return $this->groupManager->getDisplayName($this->gid); + } + + #[\Override] + public function setDisplayName(string $displayName): bool { + return $this->group->setDisplayName($displayName); + } + + #[\Override] + public function getUsers(): array { + return $this->getGroup()->getUsers(); + } + + #[\Override] + public function inGroup(IUser $user): bool { + return $this->getGroup()->inGroup($user); + } + + #[\Override] + public function addUser(IUser $user): void { + $this->getGroup()->addUser($user); + } + + #[\Override] + public function removeUser(IUser $user): void { + $this->getGroup()->removeUser($user); + } + + #[\Override] + public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array { + return $this->getGroup()->searchUsers($search, $limit, $offset); + } + + #[\Override] + public function count($search = ''): int|bool { + return $this->getGroup()->count($search); + } + + #[\Override] + public function countDisabled(): int|bool { + return $this->getGroup()->countDisabled(); + } + + #[\Override] + public function searchDisplayName(string $search, ?int $limit = null, ?int $offset = null): array { + return $this->getGroup()->searchDisplayName($search, $limit, $offset); + } + + #[\Override] + public function getBackendNames(): array { + return $this->getGroup()->getBackendNames(); + } + + #[\Override] + public function delete(): bool { + return $this->getGroup()->delete(); + } + + #[\Override] + public function canRemoveUser(): bool { + return $this->getGroup()->canRemoveUser(); + } + + #[\Override] + public function canAddUser(): bool { + return $this->getGroup()->canAddUser(); + } + + #[\Override] + public function hideFromCollaboration(): bool { + return $this->getGroup()->hideFromCollaboration(); + } + + #[\Override] + public function isDeleted(): bool { + if ($this->isDeleted === null) { + $this->getGroup(); + } + return $this->isDeleted === true ? true : $this->getGroup()->isDeleted(); + } +} diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 4dddfb3ed3779..6a46a61a92a23 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -313,12 +313,7 @@ public function getUserIdGroups(string $uid): array { $groups = []; foreach ($this->getUserIdGroupIds($uid) as $groupId) { - $aGroup = $this->get($groupId); - if ($aGroup instanceof IGroup) { - $groups[$groupId] = $aGroup; - } else { - $this->logger->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', ['app' => 'core']); - } + $groups[$groupId] = new LazyGroup($groupId, $this); } return $groups; @@ -401,18 +396,6 @@ public function getDisplayName(string $groupId): ?string { return $this->displayNameCache->getDisplayName($groupId); } - /** - * get an array of groupid and displayName for a user - * - * @param IUser $user - * @return array ['displayName' => displayname] - */ - public function getUserGroupNames(IUser $user) { - return array_map(function ($group) { - return ['displayName' => $this->displayNameCache->getDisplayName($group->getGID())]; - }, $this->getUserGroups($user)); - } - #[\Override] public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0) { $group = $this->get($gid); diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php index ff98cfc8d4c07..5f26d9715c161 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -148,4 +148,11 @@ public function canAddUser(): bool; * @since 16.0.0 */ public function hideFromCollaboration(): bool; + + /** + * Return whether the group is deleted. + * + * @since 35.0.0 + */ + public function isDeleted(): bool; } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index cd4eed5180149..a4d9faf73ad08 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -31,16 +31,11 @@ abstract class TestBackend extends ABackend implements ISearchableGroupBackend, } class ManagerTest extends TestCase { - /** @var Manager|MockObject */ - protected $userManager; - /** @var IEventDispatcher|MockObject */ - protected $dispatcher; - /** @var LoggerInterface|MockObject */ - protected $logger; - /** @var ICacheFactory|MockObject */ - private $cache; - /** @var IRemoteAddress|MockObject */ - private $remoteIpAddress; + protected Manager&MockObject $userManager; + protected IEventDispatcher&MockObject $dispatcher; + protected LoggerInterface&MockObject $logger; + private ICacheFactory&MockObject $cache; + private IRemoteAddress&MockObject $remoteIpAddress; #[\Override] protected function setUp(): void { @@ -55,7 +50,7 @@ protected function setUp(): void { $this->remoteIpAddress->method('allowsAdminActions')->willReturn(true); } - private function getTestUser($userId) { + private function getTestUser(string $userId): IUser&MockObject { $mockUser = $this->createMock(IUser::class); $mockUser->expects($this->any()) ->method('getUID') @@ -68,9 +63,8 @@ private function getTestUser($userId) { /** * @param null|int $implementedActions - * @return \PHPUnit\Framework\MockObject\MockObject */ - private function getTestBackend($implementedActions = null) { + private function getTestBackend(?int $implementedActions = null): MockObject&TestBackend { if ($implementedActions === null) { $implementedActions = GroupInterface::ADD_TO_GROUP @@ -467,9 +461,6 @@ public function testGetUserGroupIds(): void { } public function testGetUserGroupsWithDeletedGroup(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getUserGroups') @@ -483,20 +474,17 @@ public function testGetUserGroupsWithDeletedGroup(): void { $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); - /** @var User|\PHPUnit\Framework\MockObject\MockObject $user */ $user = $this->createMock(IUser::class); $user->expects($this->atLeastOnce()) ->method('getUID') ->willReturn('user1'); $groups = $manager->getUserGroups($user); - $this->assertEmpty($groups); + $this->assertCount(1, $groups); + $this->assertTrue($groups['group1']->isDeleted()); } public function testInGroup(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') From e564f8313e489951ce6ed136202687594df81a44 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 7 Jul 2026 14:12:06 +0200 Subject: [PATCH 2/3] refactor: cleanup Group ManagerTest Signed-off-by: Carl Schwan --- tests/lib/Group/ManagerTest.php | 225 +++++++++----------------------- tests/lib/Util/Group/Dummy.php | 23 ++-- 2 files changed, 71 insertions(+), 177 deletions(-) diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index a4d9faf73ad08..094918731ffcb 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -14,7 +14,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IAddToGroupBackend; -use OCP\Group\Backend\ICreateGroupBackend; +use OCP\Group\Backend\ICreateNamedGroupBackend; use OCP\Group\Backend\IGroupDetailsBackend; use OCP\Group\Backend\IRemoveFromGroupBackend; use OCP\Group\Backend\ISearchableGroupBackend; @@ -26,7 +26,7 @@ use Psr\Log\LoggerInterface; use Test\TestCase; -abstract class TestBackend extends ABackend implements ISearchableGroupBackend, IAddToGroupBackend, ICreateGroupBackend, IGroupDetailsBackend, IRemoveFromGroupBackend, GroupInterface { +abstract class TestBackend extends ABackend implements ISearchableGroupBackend, IAddToGroupBackend, ICreateNamedGroupBackend, IGroupDetailsBackend, IRemoveFromGroupBackend, GroupInterface { } @@ -101,9 +101,6 @@ private function getTestBackend(?int $implementedActions = null): MockObject&Tes } public function testGet(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') @@ -125,9 +122,6 @@ public function testGetNoBackend(): void { } public function testGetNotExists(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('groupExists') @@ -153,18 +147,12 @@ public function testGetDeleted(): void { } public function testGetMultipleBackends(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend1 - */ $backend1 = $this->getTestBackend(); $backend1->expects($this->any()) ->method('groupExists') ->with('group1') ->willReturn(false); - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend2 - */ $backend2 = $this->getTestBackend(); $backend2->expects($this->any()) ->method('groupExists') @@ -181,7 +169,6 @@ public function testGetMultipleBackends(): void { } public function testCreate(): void { - /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ $backendGroupCreated = false; $backend = $this->getTestBackend(); $backend->expects($this->any()) @@ -192,9 +179,9 @@ public function testCreate(): void { }); $backend->expects($this->once()) ->method('createGroup') - ->willReturnCallback(function () use (&$backendGroupCreated) { + ->willReturnCallback(function ($groupName) use (&$backendGroupCreated) { $backendGroupCreated = true; - return true; + return $groupName; }); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); @@ -205,8 +192,6 @@ public function testCreate(): void { } public function testCreateFailure(): void { - /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ - $backendGroupCreated = false; $backend = $this->getTestBackend( GroupInterface::ADD_TO_GROUP | GroupInterface::REMOVE_FROM_GOUP @@ -221,7 +206,7 @@ public function testCreateFailure(): void { ->willReturn(false); $backend->expects($this->once()) ->method('createGroup') - ->willReturn(false); + ->willReturn(null); $backend->expects($this->once()) ->method('getGroupDetails') ->willReturn([]); @@ -234,8 +219,6 @@ public function testCreateFailure(): void { } public function testCreateTooLong(): void { - /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ - $backendGroupCreated = false; $backend = $this->getTestBackend( GroupInterface::ADD_TO_GROUP | GroupInterface::REMOVE_FROM_GOUP @@ -258,7 +241,6 @@ public function testCreateTooLong(): void { } public function testCreateExists(): void { - /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') @@ -275,9 +257,6 @@ public function testCreateExists(): void { } public function testSearch(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getGroups') @@ -299,9 +278,6 @@ public function testSearch(): void { } public function testSearchMultipleBackends(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend1 - */ $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') @@ -314,9 +290,6 @@ public function testSearchMultipleBackends(): void { ['group12', []], ]); - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend2 - */ $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') @@ -342,9 +315,6 @@ public function testSearchMultipleBackends(): void { } public function testSearchMultipleBackendsLimitAndOffset(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend1 - */ $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') @@ -359,9 +329,6 @@ public function testSearchMultipleBackendsLimitAndOffset(): void { ['group12', []], ]); - /** - * @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend2 - */ $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') @@ -389,7 +356,6 @@ public function testSearchMultipleBackendsLimitAndOffset(): void { } public function testSearchResultExistsButGroupDoesNot(): void { - /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getGroups') @@ -402,7 +368,6 @@ public function testSearchResultExistsButGroupDoesNot(): void { ->with(['group1']) ->willReturn([]); - /** @var \OC\User\Manager $userManager */ $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); @@ -413,9 +378,6 @@ public function testSearchResultExistsButGroupDoesNot(): void { } public function testGetUserGroups(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') @@ -436,9 +398,6 @@ public function testGetUserGroups(): void { } public function testGetUserGroupIds(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->method('getUserGroups') ->with('myUID') @@ -447,7 +406,6 @@ public function testGetUserGroupIds(): void { $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); - /** @var User|\PHPUnit\Framework\MockObject\MockObject $user */ $user = $this->createMock(IUser::class); $user->method('getUID') ->willReturn('myUID'); @@ -501,9 +459,6 @@ public function testInGroup(): void { } public function testIsAdmin(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') @@ -520,9 +475,6 @@ public function testIsAdmin(): void { } public function testNotAdmin(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') @@ -539,9 +491,6 @@ public function testNotAdmin(): void { } public function testGetUserGroupsMultipleBackends(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend1 - */ $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getUserGroups') @@ -551,9 +500,6 @@ public function testGetUserGroupsMultipleBackends(): void { ->method('groupExists') ->willReturn(true); - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend2 - */ $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getUserGroups') @@ -576,9 +522,6 @@ public function testGetUserGroupsMultipleBackends(): void { } public function testDisplayNamesInGroupWithOneUserBackend(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -588,38 +531,34 @@ public function testDisplayNamesInGroupWithOneUserBackend(): void { $backend->expects($this->any()) ->method('inGroup') ->willReturnCallback(function ($uid, $gid) { - switch ($uid) { - case 'user1': return false; - case 'user2': return true; - case 'user3': return false; - case 'user33': return true; - default: - return null; - } + return match ($uid) { + 'user1' | 'user3' => false, + 'user2' | 'user33' => true, + default => null, + }; }); $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') ->willReturnCallback(function ($search, $limit, $offset) { - switch ($offset) { - case 0: return ['user3' => $this->getTestUser('user3'), - 'user33' => $this->getTestUser('user33')]; - case 2: return []; - } - return null; + return match ($offset) { + 0 => ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')], + 2 => [], + default => null, + }; }); $this->userManager->expects($this->any()) ->method('get') ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - default: - return null; - } + return match ($uid) { + 'user1' => $this->getTestUser('user1'), + 'user2' => $this->getTestUser('user2'), + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + default => null, + }; }); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); @@ -634,9 +573,6 @@ public function testDisplayNamesInGroupWithOneUserBackend(): void { } public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -646,40 +582,35 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified(): v $backend->expects($this->any()) ->method('inGroup') ->willReturnCallback(function ($uid, $gid) { - switch ($uid) { - case 'user1': return false; - case 'user2': return true; - case 'user3': return false; - case 'user33': return true; - case 'user333': return true; - default: - return null; - } + return match ($uid) { + 'user3', 'user1' => false, + 'user33', 'user333', 'user2' => true, + default => null, + }; }); $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') ->willReturnCallback(function ($search, $limit, $offset) { - switch ($offset) { - case 0: return ['user3' => $this->getTestUser('user3'), - 'user33' => $this->getTestUser('user33')]; - case 2: return ['user333' => $this->getTestUser('user333')]; - } - return null; + return match ($offset) { + 0 => ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')], + 2 => ['user333' => $this->getTestUser('user333')], + default => null, + }; }); $this->userManager->expects($this->any()) ->method('get') ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - case 'user333': return $this->getTestUser('user333'); - default: - return null; - } + return match ($uid) { + 'user1' => $this->getTestUser('user1'), + 'user2' => $this->getTestUser('user2'), + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333'), + default => null, + }; }); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); @@ -695,9 +626,6 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified(): v } public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpecified(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -707,43 +635,37 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci $backend->expects($this->any()) ->method('inGroup') ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return false; - case 'user2': return true; - case 'user3': return false; - case 'user33': return true; - case 'user333': return true; - default: - return null; - } + return match ($uid) { + 'user1', 'user3' => false, + 'user2', 'user333', 'user33' => true, + default => null, + }; }); $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') ->willReturnCallback(function ($search, $limit, $offset) { - switch ($offset) { - case 0: - return [ - 'user3' => $this->getTestUser('user3'), - 'user33' => $this->getTestUser('user33'), - 'user333' => $this->getTestUser('user333') - ]; - } - return null; + return match ($offset) { + 0 => [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ], + default => null, + }; }); $this->userManager->expects($this->any()) ->method('get') ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - case 'user333': return $this->getTestUser('user333'); - default: - return null; - } + return match ($uid) { + 'user1' => $this->getTestUser('user1'), + 'user2' => $this->getTestUser('user2'), + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333'), + default => null, + }; }); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); @@ -759,9 +681,6 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci } public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -787,9 +706,6 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty(): void } public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -815,9 +731,6 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS } public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') @@ -843,9 +756,6 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA } public function testGetUserGroupsWithAddUser(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $expectedGroups = []; $backend->expects($this->any()) @@ -880,9 +790,6 @@ public function testGetUserGroupsWithAddUser(): void { } public function testGetUserGroupsWithRemoveUser(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $expectedGroups = ['group1']; $backend->expects($this->any()) @@ -923,9 +830,6 @@ public function testGetUserGroupsWithRemoveUser(): void { } public function testGetUserIdGroups(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('getUserGroups') @@ -940,9 +844,6 @@ public function testGetUserIdGroups(): void { } public function testGroupDisplayName(): void { - /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend - */ $backend = $this->getTestBackend( GroupInterface::ADD_TO_GROUP | GroupInterface::REMOVE_FROM_GOUP diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php index 0680bfc262bad..ca77be13f6e0d 100644 --- a/tests/lib/Util/Group/Dummy.php +++ b/tests/lib/Util/Group/Dummy.php @@ -11,7 +11,7 @@ use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IAddToGroupBackend; use OCP\Group\Backend\ICountUsersBackend; -use OCP\Group\Backend\ICreateGroupBackend; +use OCP\Group\Backend\ICreateNamedGroupBackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\Group\Backend\IRemoveFromGroupBackend; use Test\Util\User\Dummy as DummyUser; @@ -19,23 +19,16 @@ /** * Dummy group backend, does not keep state, only for testing use */ -class Dummy extends ABackend implements ICreateGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend { +class Dummy extends ABackend implements ICreateNamedGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend { private $groups = []; - /** - * Try to create a new group - * @param string $gid The name of the group to create - * @return bool - * - * Tries to create a new group. If the group name already exists, false will - * be returned. - */ + #[\Override] - public function createGroup(string $gid): bool { - if (!isset($this->groups[$gid])) { - $this->groups[$gid] = []; - return true; + public function createGroup(string $name): ?string { + if (!isset($this->groups[$name])) { + $this->groups[$name] = []; + return $name; } else { - return false; + return null; } } From 92f2555a29e6a1d82d1ed3e768aadcfd7055a14c Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 8 Jul 2026 14:10:25 +0200 Subject: [PATCH 3/3] refactor: Remove isDeleted method Not used Signed-off-by: Carl Schwan --- lib/private/Group/Group.php | 6 ------ lib/private/Group/LazyGroup.php | 21 +++++---------------- lib/public/IGroup.php | 7 ------- tests/lib/Group/ManagerTest.php | 24 ------------------------ 4 files changed, 5 insertions(+), 53 deletions(-) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index a6924be6e03dc..65aff5d225b4a 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -40,7 +40,6 @@ class Group implements IGroup { /** @var User[] */ private array $users = []; private bool $usersLoaded = false; - private bool $isDeleted = false; public function __construct( private string $gid, @@ -337,7 +336,6 @@ public function delete(): bool { $this->emitter->emit('\OC\Group', 'postDelete', [$this]); } } - $this->isDeleted = $result; return $result; } @@ -395,8 +393,4 @@ public function hideFromCollaboration(): bool { return $hide || ($backend instanceof IHideFromCollaborationBackend && $backend->hideGroup($this->gid)); }, false); } - - public function isDeleted(): bool { - return $this->isDeleted; - } } diff --git a/lib/private/Group/LazyGroup.php b/lib/private/Group/LazyGroup.php index 0a6a1e831046c..e39017addfb41 100644 --- a/lib/private/Group/LazyGroup.php +++ b/lib/private/Group/LazyGroup.php @@ -1,5 +1,7 @@ group === null) { $this->group = $this->groupManager->get($this->gid); - ; } if ($this->group === null) { - Server::get(LoggerInterface::class)->debug('Trying to use the deleted group: "' . $this->gid . '"', ['app' => 'core']); - $this->isDeleted = true; + Server::get(LoggerInterface::class)->error('Trying to use the deleted group: "' . $this->gid . '"', ['app' => 'core']); $this->group = new Group($this->gid, [], Server::get(IEventDispatcher::class), Server::get(IUserManager::class)); - } else { - $this->isDeleted = false; } return $this->group; } @@ -49,12 +46,12 @@ private function getGroup(): IGroup { #[\Override] public function getDisplayName(): string { // Use display name cache from IGroupManager - return $this->groupManager->getDisplayName($this->gid); + return $this->groupManager->getDisplayName($this->gid) ?? $this->gid; } #[\Override] public function setDisplayName(string $displayName): bool { - return $this->group->setDisplayName($displayName); + return $this->getGroup()->setDisplayName($displayName); } #[\Override] @@ -121,12 +118,4 @@ public function canAddUser(): bool { public function hideFromCollaboration(): bool { return $this->getGroup()->hideFromCollaboration(); } - - #[\Override] - public function isDeleted(): bool { - if ($this->isDeleted === null) { - $this->getGroup(); - } - return $this->isDeleted === true ? true : $this->getGroup()->isDeleted(); - } } diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php index 5f26d9715c161..ff98cfc8d4c07 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -148,11 +148,4 @@ public function canAddUser(): bool; * @since 16.0.0 */ public function hideFromCollaboration(): bool; - - /** - * Return whether the group is deleted. - * - * @since 35.0.0 - */ - public function isDeleted(): bool; } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 094918731ffcb..94773a3dbe903 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -418,30 +418,6 @@ public function testGetUserGroupIds(): void { } } - public function testGetUserGroupsWithDeletedGroup(): void { - $backend = $this->createMock(Database::class); - $backend->expects($this->once()) - ->method('getUserGroups') - ->with('user1') - ->willReturn(['group1']); - $backend->expects($this->any()) - ->method('groupExists') - ->with('group1') - ->willReturn(false); - - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); - $manager->addBackend($backend); - - $user = $this->createMock(IUser::class); - $user->expects($this->atLeastOnce()) - ->method('getUID') - ->willReturn('user1'); - - $groups = $manager->getUserGroups($user); - $this->assertCount(1, $groups); - $this->assertTrue($groups['group1']->isDeleted()); - } - public function testInGroup(): void { $backend = $this->getTestBackend(); $backend->expects($this->once())