diff --git a/src/VCS/Adapter/Git/Bitbucket.php b/src/VCS/Adapter/Git/Bitbucket.php index 95f2eff8..6e28ad09 100644 --- a/src/VCS/Adapter/Git/Bitbucket.php +++ b/src/VCS/Adapter/Git/Bitbucket.php @@ -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) { diff --git a/src/VCS/Adapter/Git/GitHub.php b/src/VCS/Adapter/Git/GitHub.php index 826b92ee..c19e772d 100644 --- a/src/VCS/Adapter/Git/GitHub.php +++ b/src/VCS/Adapter/Git/GitHub.php @@ -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) { diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 6d25871b..f6649087 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -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; } diff --git a/src/VCS/Adapter/Git/Gitea.php b/src/VCS/Adapter/Git/Gitea.php index d5d7ab18..719ef2db 100644 --- a/src/VCS/Adapter/Git/Gitea.php +++ b/src/VCS/Adapter/Git/Gitea.php @@ -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) { diff --git a/tests/VCS/Adapter/BitbucketTest.php b/tests/VCS/Adapter/BitbucketTest.php index 3bb9cffb..cbb2e236 100644 --- a/tests/VCS/Adapter/BitbucketTest.php +++ b/tests/VCS/Adapter/BitbucketTest.php @@ -50,6 +50,29 @@ protected function signWebhookPayload(string $payload, string $secret): string return 'sha256=' . hash_hmac('sha256', $payload, $secret); } + /** + * @param array $filenames + * @return array|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> + */ + protected function malformedPullRequestFilesBodies(): array + { + return ['an HTML error page' => '502 Bad Gateway']; + } + protected function setupAdapter(): void { if (empty(static::$accessToken)) { diff --git a/tests/VCS/Adapter/GitHubTest.php b/tests/VCS/Adapter/GitHubTest.php index a31f5851..9d12acb0 100644 --- a/tests/VCS/Adapter/GitHubTest.php +++ b/tests/VCS/Adapter/GitHubTest.php @@ -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 { diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index 13c4704b..4c1a9ac3 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -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 @@ -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 */ protected static array $pullRequestOpenedActions = ['opened', 'synchronize']; @@ -36,6 +38,26 @@ protected function signWebhookPayload(string $payload, string $secret): string return $secret; } + /** + * @param array $filenames + * @return array|string + */ + protected function pullRequestFilesPage(array $filenames, bool $last = true): array|string + { + return \array_map(fn (string $filename) => ['new_path' => $filename], $filenames); + } + + /** + * @param array> $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)) { diff --git a/tests/VCS/Base.php b/tests/VCS/Base.php index ab571542..c68ffb45 100644 --- a/tests/VCS/Base.php +++ b/tests/VCS/Base.php @@ -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; @@ -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; @@ -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 $filenames + * @return array|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> + */ + protected function malformedPullRequestFilesBodies(): array + { + return [ + 'an error payload' => ['message' => 'Bad credentials'], + 'an HTML error page' => '502 Bad Gateway', + ]; + } + protected function setUp(): void { $this->setupAdapter(); @@ -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> $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|string $body + * @return array + */ + 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');