Optimize retrieving display name when searching for users in a group

This is recurrent scenario that we are searching for users and then for
each users we fetch the displayName. This is inefficient, so instead try
to do one query to fetch everything (e.g. Database backend) or use the
already existing DisplayNameCache helper.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

3 files changed, 62 insertions, 63 deletions

diff --git a/tests/lib/Group/GroupTest.php b/tests/lib/Group/GroupTest.php
index 60f15a65732..99389880383 100644
--- a/tests/lib/Group/GroupTest.php
+++ b/tests/lib/Group/GroupTest.php
@@ -303,9 +303,9 @@ class GroupTest extends \Test\TestCase {
 		$group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager);
 
 		$backend->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', '2')
-			->willReturn(['user2']);
+			->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
 
 		$users = $group->searchUsers('2');
 
@@ -325,13 +325,13 @@ class GroupTest extends \Test\TestCase {
 		$group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager);
 
 		$backend1->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', '2')
-			->willReturn(['user2']);
+			->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
 		$backend2->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', '2')
-			->willReturn(['user2']);
+			->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
 
 		$users = $group->searchUsers('2');
 
@@ -348,9 +348,9 @@ class GroupTest extends \Test\TestCase {
 		$group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager);
 
 		$backend->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', 'user', 1, 1)
-			->willReturn(['user2']);
+			->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
 
 		$users = $group->searchUsers('user', 1, 1);
 
@@ -370,13 +370,13 @@ class GroupTest extends \Test\TestCase {
 		$group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager);
 
 		$backend1->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', 'user', 2, 1)
-			->willReturn(['user2']);
+			->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
 		$backend2->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('group1', 'user', 2, 1)
-			->willReturn(['user1']);
+			->willReturn(['user1' => new \OC\User\User('user1', null, $this->dispatcher)]);
 
 		$users = $group->searchUsers('user', 2, 1);
 
diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php
index 5e29f974706..1bdb2219346 100644
--- a/tests/lib/Group/ManagerTest.php
+++ b/tests/lib/Group/ManagerTest.php
@@ -24,6 +24,7 @@
 namespace Test\Group;
 
 use OC\Group\Database;
+use OC\User\User;
 use OC\User\Manager;
 use OCP\GroupInterface;
 use OCP\ICacheFactory;
@@ -91,6 +92,7 @@ class ManagerTest extends TestCase {
 				'createGroup',
 				'addToGroup',
 				'removeFromGroup',
+				'searchInGroup',
 			])
 			->getMock();
 		$backend->expects($this->any())
@@ -724,7 +726,7 @@ class ManagerTest extends TestCase {
 
 	public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() {
 		/**
-		 * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend
+		 * @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend
 		 */
 		$backend = $this->getTestBackend();
 		$backend->expects($this->exactly(1))
@@ -733,22 +735,11 @@ class ManagerTest extends TestCase {
 			->willReturn(true);
 
 		$backend->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('testgroup', '', -1, 0)
-			->willReturn(['user2', 'user33']);
+			->willReturn([$this->getTestUser('user2'), $this->getTestUser('user33')]);
 
-		$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;
-				}
-			});
+		$this->userManager->expects($this->never())->method('get');
 
 		$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
 		$manager->addBackend($backend);
@@ -772,22 +763,11 @@ class ManagerTest extends TestCase {
 			->willReturn(true);
 
 		$backend->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('testgroup', '', 1, 0)
-			->willReturn(['user2']);
+			->willReturn([new User('user2', null, $this->dispatcher)]);
 
-		$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;
-				}
-			});
+		$this->userManager->expects($this->never())->method('get');
 
 		$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
 		$manager->addBackend($backend);
@@ -811,22 +791,11 @@ class ManagerTest extends TestCase {
 			->willReturn(true);
 
 		$backend->expects($this->once())
-			->method('usersInGroup')
+			->method('searchInGroup')
 			->with('testgroup', '', 1, 1)
-			->willReturn(['user33']);
+			->willReturn([$this->getTestUser('user33')]);
 
-		$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;
-				}
-			});
+		$this->userManager->expects($this->never())->method('get');
 
 		$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
 		$manager->addBackend($backend);
diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php
index 3735a5e1167..2d86e45cfc1 100644
--- a/tests/lib/Util/Group/Dummy.php
+++ b/tests/lib/Util/Group/Dummy.php
@@ -29,12 +29,18 @@
 
 namespace Test\Util\Group;
 
-use OC\Group\Backend;
+use Test\Util\User\Dummy as DummyUser;
+use OCP\Group\Backend\ABackend;
+use OCP\Group\Backend\IDeleteGroupBackend;
+use OCP\Group\Backend\IAddToGroupBackend;
+use OCP\Group\Backend\IRemoveFromGroupBackend;
+use OCP\Group\Backend\ICreateGroupBackend;
+use OCP\Group\Backend\ICountUsersBackend;
 
 /**
- * dummy group backend, does not keep state, only for testing use
+ * Dummy group backend, does not keep state, only for testing use
  */
-class Dummy extends Backend {
+class Dummy extends ABackend implements ICreateGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend {
 	private $groups = [];
 	/**
 	 * Try to create a new group
@@ -44,7 +50,7 @@ class Dummy extends Backend {
 	 * Tries to create a new group. If the group name already exists, false will
 	 * be returned.
 	 */
-	public function createGroup($gid) {
+	public function createGroup(string $gid): bool {
 		if (!isset($this->groups[$gid])) {
 			$this->groups[$gid] = [];
 			return true;
@@ -60,7 +66,7 @@ class Dummy extends Backend {
 	 *
 	 * Deletes a group and removes it from the group_user-table
 	 */
-	public function deleteGroup($gid) {
+	public function deleteGroup(string $gid): bool {
 		if (isset($this->groups[$gid])) {
 			unset($this->groups[$gid]);
 			return true;
@@ -93,7 +99,7 @@ class Dummy extends Backend {
 	 *
 	 * Adds a user to a group.
 	 */
-	public function addToGroup($uid, $gid) {
+	public function addToGroup(string $uid, string $gid): bool {
 		if (isset($this->groups[$gid])) {
 			if (array_search($uid, $this->groups[$gid]) === false) {
 				$this->groups[$gid][] = $uid;
@@ -114,7 +120,7 @@ class Dummy extends Backend {
 	 *
 	 * removes the user from a group.
 	 */
-	public function removeFromGroup($uid, $gid) {
+	public function removeFromGroup(string $uid, string $gid): bool {
 		if (isset($this->groups[$gid])) {
 			if (($index = array_search($uid, $this->groups[$gid])) !== false) {
 				unset($this->groups[$gid][$index]);
@@ -192,6 +198,25 @@ class Dummy extends Backend {
 		}
 	}
 
+	public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
+		if (isset($this->groups[$gid])) {
+			if (empty($search)) {
+				$length = $limit < 0 ? null : $limit;
+				$users = array_slice($this->groups[$gid], $offset, $length);
+				return array_map(fn ($user) => new DummyUser($user, ''));
+			}
+			$result = [];
+			foreach ($this->groups[$gid] as $user) {
+				if (stripos($user, $search) !== false) {
+					$result[] = new DummyUser($user, '');
+				}
+			}
+			return $result;
+		} else {
+			return [];
+		}
+	}
+
 	/**
 	 * get the number of all users in a group
 	 * @param string $gid
@@ -200,7 +225,7 @@ class Dummy extends Backend {
 	 * @param int $offset
 	 * @return int
 	 */
-	public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
+	public function countUsersInGroup(string $gid, string $search = ''): int {
 		if (isset($this->groups[$gid])) {
 			if (empty($search)) {
 				return count($this->groups[$gid]);
@@ -213,5 +238,10 @@ class Dummy extends Backend {
 			}
 			return $count;
 		}
+		return 0;
+	}
+
+	public function groupExists($gid) {
+		return isset($this->groups[$gid]);
 	}
 }