From a0cc4ef5ef07bbc119016ffbc71ea2255010b548 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 12:33:37 +0530 Subject: [PATCH 1/6] Add GitLab::listNamespaces() to list a user's personal namespace + groups GitLab's OAuth2 grant covers the user's personal namespace and every group they belong to in one authorization -- unlike GitHub's per- installation org picker, there's no step in the flow where the user chooses one. Appwrite needs this to build its own namespace/group picker after the OAuth redirect, similar to how Vercel/Netlify let you switch between orgs post-connect. --- src/VCS/Adapter/Git/GitLab.php | 73 ++++++++++++++++++++++++++++++++ tests/VCS/Adapter/GitLabTest.php | 38 +++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 0cadaeeb..6a417dc4 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -255,6 +255,79 @@ public function getInstallationRepository(string $repositoryName): array throw new Exception("getInstallationRepository is not applicable for this adapter"); } + /** + * List namespaces the current user can browse: their personal namespace + * plus every group they belong to. GitLab's OAuth grant covers all of + * them in a single authorization -- unlike GitHub's per-installation org + * picker, there is no step in the OAuth flow itself where the user + * chooses one, so callers need this to build their own picker. + * + * @return array{items: array, total: int} + */ + public function listNamespaces(int $page = 1, int $per_page = 20, string $search = ''): array + { + $namespaces = []; + $includedPersonalNamespace = false; + + if ($page === 1) { + $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); + $userHeaders = $userResponse['headers'] ?? []; + $userStatusCode = $userHeaders['status-code'] ?? 0; + if ($userStatusCode >= 400) { + throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); + } + $user = $userResponse['body'] ?? []; + $username = $user['username'] ?? ''; + + if (empty($search) || stripos($username, $search) !== false) { + $namespaces[] = [ + 'id' => (string) ($user['id'] ?? ''), + 'name' => $user['name'] ?? $username, + 'path' => $username, + 'kind' => 'user', + 'avatarUrl' => $user['avatar_url'] ?? '', + ]; + $includedPersonalNamespace = true; + } + } + + $url = "/groups?page={$page}&per_page={$per_page}&min_access_level=10"; + if (!empty($search)) { + $url .= '&search=' . urlencode($search); + } + + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken]); + $responseHeaders = $response['headers'] ?? []; + $statusCode = $responseHeaders['status-code'] ?? 0; + if ($statusCode >= 400) { + throw new Exception("Failed to list groups: HTTP {$statusCode}", $statusCode); + } + + $groups = $response['body'] ?? []; + $groups = is_array($groups) ? $groups : []; + foreach ($groups as $group) { + $namespaces[] = [ + 'id' => (string) ($group['id'] ?? ''), + 'name' => $group['name'] ?? ($group['path'] ?? ''), + 'path' => $group['full_path'] ?? ($group['path'] ?? ''), + 'kind' => 'group', + 'avatarUrl' => $group['avatar_url'] ?? '', + ]; + } + + // GitLab reports the group total via X-Total; the personal + // namespace isn't part of that pagination, so add it in separately. + $total = (int) ($responseHeaders['x-total'] ?? \count($groups)); + if ($includedPersonalNamespace) { + $total += 1; + } + + return [ + 'items' => $namespaces, + 'total' => $total, + ]; + } + public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { $ownerPath = $this->getOwnerPath($owner); diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index 3eba2ad5..f9504765 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -208,6 +208,44 @@ public function testGetOwnerNameWithRepositoryId(): void } } + public function testListNamespaces(): void + { + /** @var GitLab $adapter */ + $adapter = $this->vcsAdapter; + + $result = $adapter->listNamespaces(1, 20); + + $this->assertIsArray($result); + $this->assertArrayHasKey('items', $result); + $this->assertArrayHasKey('total', $result); + $this->assertNotEmpty($result['items']); + + $kinds = array_column($result['items'], 'kind'); + $this->assertContains('user', $kinds); + $this->assertContains('group', $kinds); + + foreach ($result['items'] as $namespace) { + $this->assertArrayHasKey('id', $namespace); + $this->assertArrayHasKey('name', $namespace); + $this->assertArrayHasKey('path', $namespace); + $this->assertArrayHasKey('kind', $namespace); + $this->assertNotEmpty($namespace['path']); + } + } + + public function testListNamespacesWithSearch(): void + { + /** @var GitLab $adapter */ + $adapter = $this->vcsAdapter; + $ownerPath = explode(':', static::$owner)[1] ?? static::$owner; + + $result = $adapter->listNamespaces(1, 20, $ownerPath); + + $this->assertNotEmpty($result['items']); + $paths = array_column($result['items'], 'path'); + $this->assertContains($ownerPath, $paths); + } + public function testSearchRepositories(): void { $repositoryName = 'test-search-repositories-' . \uniqid(); From 2844867f7dbdc62290bc0869630e3a86eeeac2a7 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 12:45:58 +0530 Subject: [PATCH 2/6] Fix listNamespaces() total being inconsistent across pages Total only included the personal namespace on page 1 (it was gated behind the same page===1 check as the item itself), so a caller paginating would see a different total depending on which page they requested first. Fetch /user on every page to decide the search match consistently, and drop the redundant min_access_level filter -- /groups already scopes to groups the user belongs to. --- src/VCS/Adapter/Git/GitLab.php | 51 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 6a417dc4..94232dd1 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -266,32 +266,35 @@ public function getInstallationRepository(string $repositoryName): array */ public function listNamespaces(int $page = 1, int $per_page = 20, string $search = ''): array { + // Fetched on every page (not just page 1): the personal namespace + // always contributes to the total, so pagination stays consistent + // across pages even though it's only ever placed in page 1's items. + $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); + $userHeaders = $userResponse['headers'] ?? []; + $userStatusCode = $userHeaders['status-code'] ?? 0; + if ($userStatusCode >= 400) { + throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); + } + $user = $userResponse['body'] ?? []; + $username = $user['username'] ?? ''; + $name = $user['name'] ?? ''; + + $matchesSearch = empty($search) + || stripos($username, $search) !== false + || stripos($name, $search) !== false; + $namespaces = []; - $includedPersonalNamespace = false; - - if ($page === 1) { - $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); - $userHeaders = $userResponse['headers'] ?? []; - $userStatusCode = $userHeaders['status-code'] ?? 0; - if ($userStatusCode >= 400) { - throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); - } - $user = $userResponse['body'] ?? []; - $username = $user['username'] ?? ''; - - if (empty($search) || stripos($username, $search) !== false) { - $namespaces[] = [ - 'id' => (string) ($user['id'] ?? ''), - 'name' => $user['name'] ?? $username, - 'path' => $username, - 'kind' => 'user', - 'avatarUrl' => $user['avatar_url'] ?? '', - ]; - $includedPersonalNamespace = true; - } + if ($page === 1 && $matchesSearch) { + $namespaces[] = [ + 'id' => (string) ($user['id'] ?? ''), + 'name' => $name ?: $username, + 'path' => $username, + 'kind' => 'user', + 'avatarUrl' => $user['avatar_url'] ?? '', + ]; } - $url = "/groups?page={$page}&per_page={$per_page}&min_access_level=10"; + $url = "/groups?page={$page}&per_page={$per_page}"; if (!empty($search)) { $url .= '&search=' . urlencode($search); } @@ -318,7 +321,7 @@ public function listNamespaces(int $page = 1, int $per_page = 20, string $search // GitLab reports the group total via X-Total; the personal // namespace isn't part of that pagination, so add it in separately. $total = (int) ($responseHeaders['x-total'] ?? \count($groups)); - if ($includedPersonalNamespace) { + if ($matchesSearch) { $total += 1; } From 8bd586e9eae0f622b1a728c5b0269369b3efdde2 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 12:47:43 +0530 Subject: [PATCH 3/6] Avoid an unnecessary /user call on every paginated request The previous fix fetched /user on every page to keep the total consistent, but that's wasteful: without a search term the personal namespace always counts, so only page 1 (to build the item) or an active search (to resolve the match) actually need the call. --- src/VCS/Adapter/Git/GitLab.php | 55 ++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 94232dd1..de474d16 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -266,32 +266,37 @@ public function getInstallationRepository(string $repositoryName): array */ public function listNamespaces(int $page = 1, int $per_page = 20, string $search = ''): array { - // Fetched on every page (not just page 1): the personal namespace - // always contributes to the total, so pagination stays consistent - // across pages even though it's only ever placed in page 1's items. - $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); - $userHeaders = $userResponse['headers'] ?? []; - $userStatusCode = $userHeaders['status-code'] ?? 0; - if ($userStatusCode >= 400) { - throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); - } - $user = $userResponse['body'] ?? []; - $username = $user['username'] ?? ''; - $name = $user['name'] ?? ''; - - $matchesSearch = empty($search) - || stripos($username, $search) !== false - || stripos($name, $search) !== false; - + // /user is only needed to build page 1's item, or -- on any page -- + // to resolve whether the personal namespace matches a search term. + // With no search it always counts, so skip the call entirely for + // page > 1 rather than paying for it on every paginated request. + $matchesSearch = true; $namespaces = []; - if ($page === 1 && $matchesSearch) { - $namespaces[] = [ - 'id' => (string) ($user['id'] ?? ''), - 'name' => $name ?: $username, - 'path' => $username, - 'kind' => 'user', - 'avatarUrl' => $user['avatar_url'] ?? '', - ]; + + if ($page === 1 || !empty($search)) { + $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); + $userHeaders = $userResponse['headers'] ?? []; + $userStatusCode = $userHeaders['status-code'] ?? 0; + if ($userStatusCode >= 400) { + throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); + } + $user = $userResponse['body'] ?? []; + $username = $user['username'] ?? ''; + $name = $user['name'] ?? ''; + + $matchesSearch = empty($search) + || stripos($username, $search) !== false + || stripos($name, $search) !== false; + + if ($page === 1 && $matchesSearch) { + $namespaces[] = [ + 'id' => (string) ($user['id'] ?? ''), + 'name' => $name ?: $username, + 'path' => $username, + 'kind' => 'user', + 'avatarUrl' => $user['avatar_url'] ?? '', + ]; + } } $url = "/groups?page={$page}&per_page={$per_page}"; From 289453b404d3d5b5837cc5c9302c3c11f6af8f12 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 17:42:36 +0530 Subject: [PATCH 4/6] Fix combined pagination overflowing page 1 and advertising an empty last page Page 1 always requested a full per_page groups before prepending the personal namespace, so a full group page pushed it to per_page+1 items -- and when the group count was exactly divisible by per_page, the incremented total advertised a final page whose group request returned nothing (Greptile P1). Reserves one slot for the personal namespace on page 1 only, and fetches groups via a 0-based offset into the full group list (shifted by 1 whenever the personal namespace occupies a slot), walking GitLab's own page/per_page boundaries since that offset rarely aligns with them. Adds a pure-unit test faking the HTTP layer to cover the exact boundary case a live-only test can't reach without seeding 20+ real groups. Also drops the page/per_page defaults on listNamespaces() to match searchRepositories()' pattern of requiring callers to pass them explicitly. --- src/VCS/Adapter/Git/GitLab.php | 95 +++++++++--- .../GitLabNamespacesPaginationTest.php | 139 ++++++++++++++++++ 2 files changed, 215 insertions(+), 19 deletions(-) create mode 100644 tests/VCS/Adapter/GitLabNamespacesPaginationTest.php diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index de474d16..19cb367b 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -264,7 +264,7 @@ public function getInstallationRepository(string $repositoryName): array * * @return array{items: array, total: int} */ - public function listNamespaces(int $page = 1, int $per_page = 20, string $search = ''): array + public function listNamespaces(int $page, int $per_page, string $search = ''): array { // /user is only needed to build page 1's item, or -- on any page -- // to resolve whether the personal namespace matches a search term. @@ -299,20 +299,18 @@ public function listNamespaces(int $page = 1, int $per_page = 20, string $search } } - $url = "/groups?page={$page}&per_page={$per_page}"; - if (!empty($search)) { - $url .= '&search=' . urlencode($search); - } + // Page 1 reserves one slot for the personal namespace (if it's + // included), so it only asks GitLab for per_page-1 groups; every + // other page asks for a full per_page. Without this, a full page of + // groups plus the personal namespace would overflow page 1 to + // per_page+1 items, and the combined total would advertise a final + // page whose group request returns nothing. + $includesPersonal = $page === 1 && $matchesSearch; + $groupsNeeded = $per_page - ($includesPersonal ? 1 : 0); + $groupOffset = \max(($page - 1) * $per_page - ($matchesSearch ? 1 : 0), 0); - $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken]); - $responseHeaders = $response['headers'] ?? []; - $statusCode = $responseHeaders['status-code'] ?? 0; - if ($statusCode >= 400) { - throw new Exception("Failed to list groups: HTTP {$statusCode}", $statusCode); - } + ['items' => $groups, 'total' => $groupTotal] = $this->fetchGroupsSlice($groupOffset, $groupsNeeded, $per_page, $search); - $groups = $response['body'] ?? []; - $groups = is_array($groups) ? $groups : []; foreach ($groups as $group) { $namespaces[] = [ 'id' => (string) ($group['id'] ?? ''), @@ -323,12 +321,9 @@ public function listNamespaces(int $page = 1, int $per_page = 20, string $search ]; } - // GitLab reports the group total via X-Total; the personal - // namespace isn't part of that pagination, so add it in separately. - $total = (int) ($responseHeaders['x-total'] ?? \count($groups)); - if ($matchesSearch) { - $total += 1; - } + // The personal namespace isn't part of GitLab's group pagination, + // so it's added in separately once here. + $total = $groupTotal + ($matchesSearch ? 1 : 0); return [ 'items' => $namespaces, @@ -336,6 +331,68 @@ public function listNamespaces(int $page = 1, int $per_page = 20, string $search ]; } + /** + * Fetch exactly $limit groups starting at 0-based $offset into the full + * (search-filtered) groups list, by walking GitLab's own page/per_page + * pagination. $offset rarely lands on a GitLab page boundary (it's + * shifted by one whenever the personal namespace occupies a slot), so + * this may span more than one GitLab request. + * + * @return array{items: array>, total: int} + */ + private function fetchGroupsSlice(int $offset, int $limit, int $per_page, string $search): array + { + if ($limit <= 0) { + $probe = $this->fetchGroupsPage(1, max($per_page, 1), $search); + return ['items' => [], 'total' => $probe['total']]; + } + + $gitlabPage = intdiv($offset, $per_page) + 1; + $skip = $offset % $per_page; + + $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); + $collected = array_slice($result['items'], $skip); + + // Keep pulling subsequent GitLab pages until either enough items + // are collected or GitLab returns a short page (its real end). + while (count($collected) < $limit && count($result['items']) === $per_page) { + $gitlabPage++; + $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); + $collected = array_merge($collected, $result['items']); + } + + return [ + 'items' => array_slice($collected, 0, $limit), + 'total' => $result['total'], + ]; + } + + /** + * @return array{items: array>, total: int} + */ + private function fetchGroupsPage(int $page, int $per_page, string $search): array + { + $url = "/groups?page={$page}&per_page={$per_page}"; + if (!empty($search)) { + $url .= '&search=' . urlencode($search); + } + + $response = $this->call(self::METHOD_GET, $url, ['Authorization' => 'Bearer ' . $this->accessToken]); + $responseHeaders = $response['headers'] ?? []; + $statusCode = $responseHeaders['status-code'] ?? 0; + if ($statusCode >= 400) { + throw new Exception("Failed to list groups: HTTP {$statusCode}", $statusCode); + } + + $items = $response['body'] ?? []; + $items = is_array($items) ? $items : []; + + return [ + 'items' => $items, + 'total' => (int) ($responseHeaders['x-total'] ?? \count($items)), + ]; + } + public function searchRepositories(string $owner, int $page, int $per_page, string $search = ''): array { $ownerPath = $this->getOwnerPath($owner); diff --git a/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php b/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php new file mode 100644 index 00000000..0a87d900 --- /dev/null +++ b/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php @@ -0,0 +1,139 @@ +> $groups + */ + private function makeAdapter(array $groups): GitLab + { + return new class ($groups) extends GitLab { + /** @var array> */ + private array $groups; + + /** + * @param array> $groups + */ + public function __construct(array $groups) + { + parent::__construct(new Cache(new None())); + $this->groups = $groups; + $this->accessToken = 'fake-token'; + } + + /** + * @param array $headers + * @param array $params + * @return array + */ + protected function call(string $method, string $path = '', array $headers = [], array $params = [], bool $decode = true, bool $followRedirects = true) + { + if (str_starts_with($path, '/user')) { + return [ + 'headers' => ['status-code' => 200], + 'body' => [ + 'id' => 1, + 'username' => 'harsh173', + 'name' => 'Harsh', + 'avatar_url' => '', + ], + ]; + } + + parse_str(explode('?', $path)[1] ?? '', $query); + $page = (int) ($query['page'] ?? 1); + $perPage = (int) ($query['per_page'] ?? 20); + + $slice = array_slice($this->groups, ($page - 1) * $perPage, $perPage); + + return [ + 'headers' => [ + 'status-code' => 200, + 'x-total' => (string) count($this->groups), + ], + 'body' => $slice, + ]; + } + }; + } + + /** + * @return array> + */ + private function makeGroups(int $count): array + { + $groups = []; + for ($i = 1; $i <= $count; $i++) { + $groups[] = [ + 'id' => $i, + 'name' => "group-{$i}", + 'full_path' => "group-{$i}", + 'avatar_url' => '', + ]; + } + return $groups; + } + + public function testPageOneDoesNotOverflowWhenGroupCountFillsAPage(): void + { + $adapter = $this->makeAdapter($this->makeGroups(20)); + + $result = $adapter->listNamespaces(1, 10); + + $this->assertCount(10, $result['items']); + $this->assertSame('user', $result['items'][0]['kind']); + $this->assertSame(21, $result['total']); + } + + public function testLastPageIsNotEmptyAndTotalStaysConsistent(): void + { + $adapter = $this->makeAdapter($this->makeGroups(20)); + + $page1 = $adapter->listNamespaces(1, 10); + $page2 = $adapter->listNamespaces(2, 10); + $page3 = $adapter->listNamespaces(3, 10); + + $this->assertCount(10, $page1['items']); + $this->assertCount(10, $page2['items']); + $this->assertCount(1, $page3['items']); + $this->assertNotEmpty($page3['items']); + + $this->assertSame(21, $page1['total']); + $this->assertSame(21, $page2['total']); + $this->assertSame(21, $page3['total']); + + $totalItemsAcrossPages = count($page1['items']) + count($page2['items']) + count($page3['items']); + $this->assertSame($page1['total'], $totalItemsAcrossPages); + } + + public function testGroupsAreNotDroppedOrDuplicatedAcrossPageBoundary(): void + { + $adapter = $this->makeAdapter($this->makeGroups(20)); + + $page1 = $adapter->listNamespaces(1, 10); + $page2 = $adapter->listNamespaces(2, 10); + $page3 = $adapter->listNamespaces(3, 10); + + $groupPaths = array_column( + array_filter([...$page1['items'], ...$page2['items'], ...$page3['items']], fn ($n) => $n['kind'] === 'group'), + 'path', + ); + + $this->assertCount(20, $groupPaths); + $this->assertCount(20, array_unique($groupPaths)); + } +} From 0c7d9c4b6f4d3f17e1632237e30751d3c9978458 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 17:46:20 +0530 Subject: [PATCH 5/6] Trim comments and drop the separate pagination test --- src/VCS/Adapter/Git/GitLab.php | 30 +--- .../GitLabNamespacesPaginationTest.php | 139 ------------------ 2 files changed, 8 insertions(+), 161 deletions(-) delete mode 100644 tests/VCS/Adapter/GitLabNamespacesPaginationTest.php diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 19cb367b..efd174e2 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -258,18 +258,14 @@ public function getInstallationRepository(string $repositoryName): array /** * List namespaces the current user can browse: their personal namespace * plus every group they belong to. GitLab's OAuth grant covers all of - * them in a single authorization -- unlike GitHub's per-installation org - * picker, there is no step in the OAuth flow itself where the user - * chooses one, so callers need this to build their own picker. + * them in one authorization, unlike GitHub's per-installation org picker. * * @return array{items: array, total: int} */ public function listNamespaces(int $page, int $per_page, string $search = ''): array { - // /user is only needed to build page 1's item, or -- on any page -- - // to resolve whether the personal namespace matches a search term. - // With no search it always counts, so skip the call entirely for - // page > 1 rather than paying for it on every paginated request. + // /user is skippable past page 1 when there's no search, since the + // personal namespace always counts in that case. $matchesSearch = true; $namespaces = []; @@ -299,12 +295,8 @@ public function listNamespaces(int $page, int $per_page, string $search = ''): a } } - // Page 1 reserves one slot for the personal namespace (if it's - // included), so it only asks GitLab for per_page-1 groups; every - // other page asks for a full per_page. Without this, a full page of - // groups plus the personal namespace would overflow page 1 to - // per_page+1 items, and the combined total would advertise a final - // page whose group request returns nothing. + // Page 1 reserves a slot for the personal namespace, so it fetches + // one fewer group -- otherwise page 1 overflows to per_page+1 items. $includesPersonal = $page === 1 && $matchesSearch; $groupsNeeded = $per_page - ($includesPersonal ? 1 : 0); $groupOffset = \max(($page - 1) * $per_page - ($matchesSearch ? 1 : 0), 0); @@ -321,8 +313,6 @@ public function listNamespaces(int $page, int $per_page, string $search = ''): a ]; } - // The personal namespace isn't part of GitLab's group pagination, - // so it's added in separately once here. $total = $groupTotal + ($matchesSearch ? 1 : 0); return [ @@ -332,11 +322,9 @@ public function listNamespaces(int $page, int $per_page, string $search = ''): a } /** - * Fetch exactly $limit groups starting at 0-based $offset into the full - * (search-filtered) groups list, by walking GitLab's own page/per_page - * pagination. $offset rarely lands on a GitLab page boundary (it's - * shifted by one whenever the personal namespace occupies a slot), so - * this may span more than one GitLab request. + * Fetch $limit groups starting at 0-based $offset, walking GitLab's own + * page/per_page pagination since $offset is shifted by one (and won't + * align with a GitLab page) whenever the personal namespace takes a slot. * * @return array{items: array>, total: int} */ @@ -353,8 +341,6 @@ private function fetchGroupsSlice(int $offset, int $limit, int $per_page, string $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); $collected = array_slice($result['items'], $skip); - // Keep pulling subsequent GitLab pages until either enough items - // are collected or GitLab returns a short page (its real end). while (count($collected) < $limit && count($result['items']) === $per_page) { $gitlabPage++; $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); diff --git a/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php b/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php deleted file mode 100644 index 0a87d900..00000000 --- a/tests/VCS/Adapter/GitLabNamespacesPaginationTest.php +++ /dev/null @@ -1,139 +0,0 @@ -> $groups - */ - private function makeAdapter(array $groups): GitLab - { - return new class ($groups) extends GitLab { - /** @var array> */ - private array $groups; - - /** - * @param array> $groups - */ - public function __construct(array $groups) - { - parent::__construct(new Cache(new None())); - $this->groups = $groups; - $this->accessToken = 'fake-token'; - } - - /** - * @param array $headers - * @param array $params - * @return array - */ - protected function call(string $method, string $path = '', array $headers = [], array $params = [], bool $decode = true, bool $followRedirects = true) - { - if (str_starts_with($path, '/user')) { - return [ - 'headers' => ['status-code' => 200], - 'body' => [ - 'id' => 1, - 'username' => 'harsh173', - 'name' => 'Harsh', - 'avatar_url' => '', - ], - ]; - } - - parse_str(explode('?', $path)[1] ?? '', $query); - $page = (int) ($query['page'] ?? 1); - $perPage = (int) ($query['per_page'] ?? 20); - - $slice = array_slice($this->groups, ($page - 1) * $perPage, $perPage); - - return [ - 'headers' => [ - 'status-code' => 200, - 'x-total' => (string) count($this->groups), - ], - 'body' => $slice, - ]; - } - }; - } - - /** - * @return array> - */ - private function makeGroups(int $count): array - { - $groups = []; - for ($i = 1; $i <= $count; $i++) { - $groups[] = [ - 'id' => $i, - 'name' => "group-{$i}", - 'full_path' => "group-{$i}", - 'avatar_url' => '', - ]; - } - return $groups; - } - - public function testPageOneDoesNotOverflowWhenGroupCountFillsAPage(): void - { - $adapter = $this->makeAdapter($this->makeGroups(20)); - - $result = $adapter->listNamespaces(1, 10); - - $this->assertCount(10, $result['items']); - $this->assertSame('user', $result['items'][0]['kind']); - $this->assertSame(21, $result['total']); - } - - public function testLastPageIsNotEmptyAndTotalStaysConsistent(): void - { - $adapter = $this->makeAdapter($this->makeGroups(20)); - - $page1 = $adapter->listNamespaces(1, 10); - $page2 = $adapter->listNamespaces(2, 10); - $page3 = $adapter->listNamespaces(3, 10); - - $this->assertCount(10, $page1['items']); - $this->assertCount(10, $page2['items']); - $this->assertCount(1, $page3['items']); - $this->assertNotEmpty($page3['items']); - - $this->assertSame(21, $page1['total']); - $this->assertSame(21, $page2['total']); - $this->assertSame(21, $page3['total']); - - $totalItemsAcrossPages = count($page1['items']) + count($page2['items']) + count($page3['items']); - $this->assertSame($page1['total'], $totalItemsAcrossPages); - } - - public function testGroupsAreNotDroppedOrDuplicatedAcrossPageBoundary(): void - { - $adapter = $this->makeAdapter($this->makeGroups(20)); - - $page1 = $adapter->listNamespaces(1, 10); - $page2 = $adapter->listNamespaces(2, 10); - $page3 = $adapter->listNamespaces(3, 10); - - $groupPaths = array_column( - array_filter([...$page1['items'], ...$page2['items'], ...$page3['items']], fn ($n) => $n['kind'] === 'group'), - 'path', - ); - - $this->assertCount(20, $groupPaths); - $this->assertCount(20, array_unique($groupPaths)); - } -} From 2423618e32a1b3d0dd3e8e4327b5f85561d90cf3 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Thu, 23 Jul 2026 17:53:30 +0530 Subject: [PATCH 6/6] Use GitLab's native /namespaces endpoint instead of hand-rolling /user + /groups GitLab already exposes GET /namespaces, which combines the personal namespace and every group into one correctly-paginated list with the exact fields needed (id, name, path, kind, avatar_url) and its own search param. The previous implementation manually merged /user with /groups and shifted pagination offsets to account for the personal namespace taking a slot -- solving a problem GitLab's own API already solves, and the source of the P1 pagination bug found in review. This also settles review naming questions: "namespaces" (not "groups") and "kind" (not "type") both mirror GitLab's own /namespaces API terminology directly, since this is an adapter for that API. --- src/VCS/Adapter/Git/GitLab.php | 115 +++++---------------------------- 1 file changed, 15 insertions(+), 100 deletions(-) diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index efd174e2..0a133d79 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -257,108 +257,15 @@ public function getInstallationRepository(string $repositoryName): array /** * List namespaces the current user can browse: their personal namespace - * plus every group they belong to. GitLab's OAuth grant covers all of - * them in one authorization, unlike GitHub's per-installation org picker. + * plus every group they belong to. GitLab's own /namespaces endpoint + * already combines both with correct pagination, so this is a thin + * passthrough rather than a hand-rolled merge of /user + /groups. * * @return array{items: array, total: int} */ public function listNamespaces(int $page, int $per_page, string $search = ''): array { - // /user is skippable past page 1 when there's no search, since the - // personal namespace always counts in that case. - $matchesSearch = true; - $namespaces = []; - - if ($page === 1 || !empty($search)) { - $userResponse = $this->call(self::METHOD_GET, '/user', ['Authorization' => 'Bearer ' . $this->accessToken]); - $userHeaders = $userResponse['headers'] ?? []; - $userStatusCode = $userHeaders['status-code'] ?? 0; - if ($userStatusCode >= 400) { - throw new Exception("Failed to get current user: HTTP {$userStatusCode}", $userStatusCode); - } - $user = $userResponse['body'] ?? []; - $username = $user['username'] ?? ''; - $name = $user['name'] ?? ''; - - $matchesSearch = empty($search) - || stripos($username, $search) !== false - || stripos($name, $search) !== false; - - if ($page === 1 && $matchesSearch) { - $namespaces[] = [ - 'id' => (string) ($user['id'] ?? ''), - 'name' => $name ?: $username, - 'path' => $username, - 'kind' => 'user', - 'avatarUrl' => $user['avatar_url'] ?? '', - ]; - } - } - - // Page 1 reserves a slot for the personal namespace, so it fetches - // one fewer group -- otherwise page 1 overflows to per_page+1 items. - $includesPersonal = $page === 1 && $matchesSearch; - $groupsNeeded = $per_page - ($includesPersonal ? 1 : 0); - $groupOffset = \max(($page - 1) * $per_page - ($matchesSearch ? 1 : 0), 0); - - ['items' => $groups, 'total' => $groupTotal] = $this->fetchGroupsSlice($groupOffset, $groupsNeeded, $per_page, $search); - - foreach ($groups as $group) { - $namespaces[] = [ - 'id' => (string) ($group['id'] ?? ''), - 'name' => $group['name'] ?? ($group['path'] ?? ''), - 'path' => $group['full_path'] ?? ($group['path'] ?? ''), - 'kind' => 'group', - 'avatarUrl' => $group['avatar_url'] ?? '', - ]; - } - - $total = $groupTotal + ($matchesSearch ? 1 : 0); - - return [ - 'items' => $namespaces, - 'total' => $total, - ]; - } - - /** - * Fetch $limit groups starting at 0-based $offset, walking GitLab's own - * page/per_page pagination since $offset is shifted by one (and won't - * align with a GitLab page) whenever the personal namespace takes a slot. - * - * @return array{items: array>, total: int} - */ - private function fetchGroupsSlice(int $offset, int $limit, int $per_page, string $search): array - { - if ($limit <= 0) { - $probe = $this->fetchGroupsPage(1, max($per_page, 1), $search); - return ['items' => [], 'total' => $probe['total']]; - } - - $gitlabPage = intdiv($offset, $per_page) + 1; - $skip = $offset % $per_page; - - $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); - $collected = array_slice($result['items'], $skip); - - while (count($collected) < $limit && count($result['items']) === $per_page) { - $gitlabPage++; - $result = $this->fetchGroupsPage($gitlabPage, $per_page, $search); - $collected = array_merge($collected, $result['items']); - } - - return [ - 'items' => array_slice($collected, 0, $limit), - 'total' => $result['total'], - ]; - } - - /** - * @return array{items: array>, total: int} - */ - private function fetchGroupsPage(int $page, int $per_page, string $search): array - { - $url = "/groups?page={$page}&per_page={$per_page}"; + $url = "/namespaces?page={$page}&per_page={$per_page}"; if (!empty($search)) { $url .= '&search=' . urlencode($search); } @@ -367,15 +274,23 @@ private function fetchGroupsPage(int $page, int $per_page, string $search): arra $responseHeaders = $response['headers'] ?? []; $statusCode = $responseHeaders['status-code'] ?? 0; if ($statusCode >= 400) { - throw new Exception("Failed to list groups: HTTP {$statusCode}", $statusCode); + throw new Exception("Failed to list namespaces: HTTP {$statusCode}", $statusCode); } $items = $response['body'] ?? []; $items = is_array($items) ? $items : []; + $namespaces = array_map(fn (array $namespace) => [ + 'id' => (string) ($namespace['id'] ?? ''), + 'name' => $namespace['name'] ?? ($namespace['path'] ?? ''), + 'path' => $namespace['full_path'] ?? ($namespace['path'] ?? ''), + 'kind' => $namespace['kind'] ?? 'group', + 'avatarUrl' => $namespace['avatar_url'] ?? '', + ], $items); + return [ - 'items' => $items, - 'total' => (int) ($responseHeaders['x-total'] ?? \count($items)), + 'items' => $namespaces, + 'total' => (int) ($responseHeaders['x-total'] ?? \count($namespaces)), ]; }