Skip to content

fix: report a provider failure when reading pull request files #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
HarshMN2345 wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix: report a provider failure when reading pull request files #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b0fcf9
fix: report a provider failure when reading pull request files
HarshMN2345 Aug 17, 2026
61b92d1
style: keep the pull request files comments to what the code cannot say
HarshMN2345 Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/VCS/Adapter/Git/Bitbucket.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,12 +1154,12 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $

$responseBody = $response['body'] ?? [];
if (!is_array($responseBody)) {
break;
throw new Exception('Pull request files response is not an object.');
}

$values = $responseBody['values'] ?? [];
if (!is_array($values)) {
break;
throw new Exception('Pull request files response is not a list of diffs.');
}

foreach ($values as $diff) {
Expand Down
10 changes: 10 additions & 0 deletions src/VCS/Adapter/Git/GitHub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,17 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $
'page' => $currentPage,
]);

$responseHeaders = $response['headers'] ?? [];
$statusCode = $responseHeaders['status-code'] ?? 0;
if ($statusCode >= 400) {
throw new Exception("Failed to get pull request files: HTTP {$statusCode}", $statusCode);
}

$files = $response['body'] ?? [];
if (!\is_array($files) || !\array_is_list($files)) {
throw new Exception('Pull request files response is not a list of files.');
}

$allFiles = array_merge($allFiles, $files);

if (\count($files) < $perPage) {
Expand Down
6 changes: 5 additions & 1 deletion src/VCS/Adapter/Git/GitLab.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,11 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $
}

$files = $response['body'] ?? [];
if (!is_array($files) || empty($files)) {
if (!is_array($files) || !\array_is_list($files)) {
throw new Exception('Merge request files response is not a list of diffs.');
}

if (empty($files)) {
break;
}

Expand Down
4 changes: 4 additions & 0 deletions src/VCS/Adapter/Git/Gitea.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,10 @@ public function getPullRequestFiles(string $owner, string $repositoryName, int $
}

$files = $response['body'] ?? [];
if (!\is_array($files) || !\array_is_list($files)) {
throw new Exception('Pull request files response is not a list of files.');
}

$allFiles = array_merge($allFiles, $files);

if (\count($files) < $limit) {
Expand Down
23 changes: 23 additions & 0 deletions tests/VCS/Adapter/BitbucketTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,29 @@ protected function signWebhookPayload(string $payload, string $secret): string
return 'sha256=' . hash_hmac('sha256', $payload, $secret);
}

/**
* @param array<string> $filenames
* @return array<mixed>|string
*/
protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string
{
return [
'values' => \array_map(fn (string $filename) => ['new' => ['path' => $filename]], $filenames),
'next' => $last ? null : 'https://api.bitbucket.org/2.0/next',
];
}

/**
* A page is an object here, so an error payload carrying no 'values' is
* indistinguishable from a page listing no files.
*
* @return array<string, array<mixed>|string>
*/
protected function malformedPullRequestFilesBodies(): array
{
return ['an HTML error page' => '<html>502 Bad Gateway</html>'];
}

protected function setupAdapter(): void
{
if (empty(static::$accessToken)) {
Expand Down
2 changes: 2 additions & 0 deletions tests/VCS/Adapter/GitHubTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ protected function signWebhookPayload(string $payload, string $secret): string
{
return 'sha256=' . hash_hmac('sha256', $payload, $secret);
}

protected static string $eventHeader = 'x-github-event';
protected static string $signatureHeader = 'x-hub-signature-256';
protected static bool $replayAdapterNeedsToken = false;

protected function setupAdapter(): void
{
Expand Down
22 changes: 22 additions & 0 deletions tests/VCS/Adapter/GitLabTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Utopia\Cache\Cache;
use Utopia\System\System;
use Utopia\Tests\Base;
use Utopia\VCS\Adapter\Git;
use Utopia\VCS\Adapter\Git\GitLab;

class GitLabTest extends Base
Expand All @@ -18,6 +19,7 @@ class GitLabTest extends Base
protected static string $signatureHeader = 'x-gitlab-token';
protected static string $pushEventName = 'Push Hook';
protected static string $pullRequestEventName = 'Merge Request Hook';
protected static int $pullRequestFilesPageSize = 100;

/** @var array<string> */
protected static array $pullRequestOpenedActions = ['opened', 'synchronize'];
Expand All @@ -36,6 +38,26 @@ protected function signWebhookPayload(string $payload, string $secret): string
return $secret;
}

/**
* @param array<string> $filenames
* @return array<mixed>|string
*/
protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string
{
return \array_map(fn (string $filename) => ['new_path' => $filename], $filenames);
}

/**
* @param array<int, array<mixed>> $responses
*/
protected function replayAdapter(array $responses): Git
{
// GitLab waits for the merge request's diff to be ready before paging.
\array_unshift($responses, $this->providerResponse(['patch_id_sha' => 'abc123']));

return parent::replayAdapter($responses);
}

protected function setupAdapter(): void
{
if (empty(static::$accessToken)) {
Expand Down
151 changes: 151 additions & 0 deletions tests/VCS/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

use Exception;
use PHPUnit\Framework\TestCase;
use Utopia\Cache\Adapter\None;
use Utopia\Cache\Cache;
use Utopia\Fetch\Client;
use Utopia\System\System;
use Utopia\VCS\Adapter\Git;
Expand Down Expand Up @@ -113,6 +115,16 @@ abstract class Base extends TestCase

protected static bool $supportsPullRequestLookup = true;

/**
* Files per page the adapter asks this provider for.
*/
protected static int $pullRequestFilesPageSize = 30;

/**
* GitHub mints its own token from an app, which cannot be done offline.
*/
protected static bool $replayAdapterNeedsToken = true;

protected static bool $supportsCommitStatuses = true;

protected static bool $supportsCommitStatusLookup = true;
Expand Down Expand Up @@ -204,6 +216,32 @@ abstract protected function pushPayload(string $branch, array $added = [], array
*/
abstract protected function pullRequestPayload(bool $external = false): string;

/**
* One page of a pull request's files. $last is only read by providers that
* page by cursor rather than by count.
*
* @param array<string> $filenames
* @return array<mixed>|string
*/
protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string
{
return \array_map(fn (string $filename) => ['filename' => $filename, 'status' => 'added'], $filenames);
}

/**
* Bodies this provider can return with a success status where a page of
* files was expected.
*
* @return array<string, array<mixed>|string>
*/
protected function malformedPullRequestFilesBodies(): array
{
return [
'an error payload' => ['message' => 'Bad credentials'],
'an HTML error page' => '<html>502 Bad Gateway</html>',
];
}

protected function setUp(): void
{
$this->setupAdapter();
Expand Down Expand Up @@ -1305,6 +1343,119 @@ public function testGetPullRequestFiles(): void
}
}

/**
* The adapter under test, replying from $responses in order. A live forge
* cannot be made to page or fail on demand.
*
* @param array<int, array<mixed>> $responses
*/
protected function replayAdapter(array $responses): Git
{
$adapter = $this->getMockBuilder($this->vcsAdapter::class)
->setConstructorArgs([new Cache(new None())])
->onlyMethods(['call'])
->getMock();

$adapter->method('call')->willReturnCallback(
function () use (&$responses): array {
return \array_shift($responses) ?? $this->providerResponse([]);
}
);

if (static::$replayAdapterNeedsToken) {
$adapter->initializeVariables('1', '', null, 'token', null);
}

return $adapter;
}

/**
* @param array<mixed>|string $body
* @return array<mixed>
*/
protected function providerResponse(array|string $body, int $statusCode = 200): array
{
return [
'headers' => ['status-code' => $statusCode],
'body' => $body,
];
}

public function testGetPullRequestFilesPaginated(): void
{
$this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests');

$pageSize = static::$pullRequestFilesPageSize;
$full = \array_map(fn (int $i) => "first-{$i}.txt", \range(0, $pageSize - 1));
$adapter = $this->replayAdapter([
$this->providerResponse($this->pullRequestFilesPage($full, false)),
$this->providerResponse($this->pullRequestFilesPage(['last.txt'])),
]);

$result = $adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1);

$filenames = array_column($result, 'filename');
$this->assertCount($pageSize + 1, $filenames);
$this->assertSame('last.txt', $filenames[$pageSize]);
}

public function testGetPullRequestFilesProviderFailure(): void
{
$this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests');

foreach ([401, 403, 404, 500] as $statusCode) {
$adapter = $this->replayAdapter([
$this->providerResponse(['message' => 'Bad credentials'], $statusCode),
]);

$thrown = null;

try {
$adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1);
} catch (Exception $e) {
$thrown = $e;
}

$this->assertNotNull($thrown, "HTTP {$statusCode} was not reported as a failure");
$this->assertSame($statusCode, $thrown->getCode());
}
}

public function testGetPullRequestFilesProviderFailureOnLaterPage(): void
{
$this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests');

$full = \array_map(fn (int $i) => "first-{$i}.txt", \range(0, static::$pullRequestFilesPageSize - 1));
$adapter = $this->replayAdapter([
$this->providerResponse($this->pullRequestFilesPage($full, false)),
$this->providerResponse(['message' => 'API rate limit exceeded'], 403),
]);

$this->expectException(Exception::class);
$this->expectExceptionCode(403);

$adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1);
}

public function testGetPullRequestFilesMalformedBody(): void
{
$this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests');

foreach ($this->malformedPullRequestFilesBodies() as $description => $body) {
$adapter = $this->replayAdapter([$this->providerResponse($body)]);

$thrown = null;

try {
$adapter->getPullRequestFiles(static::$owner, static::EVENT_REPOSITORY_NAME, 1);
} catch (Exception $e) {
$thrown = $e;
}

$this->assertNotNull($thrown, "{$description} was not reported as a failure");
}
}

public function testGetPullRequestWithInvalidNumber(): void
{
$this->skipUnlessSupported(static::$supportsPullRequestLookup, 'looking up pull requests');
Expand Down
Loading