From 8f40dfd7c8bb9525286589bcff90f3a4c3b1350d Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Tue, 11 Aug 2026 14:26:41 +0400 Subject: [PATCH 1/2] fix(get): return a non-zero exit code when a download fails feat(repository): turn unsuccessful API responses into actionable errors feat(downloader): report why every download attempt failed fix(downloader): fail when nothing matches the extraction rules docs(readme): document failure reporting and GITHUB_TOKEN limits in CI A failed run used to be indistinguishable from a successful one: the command always exited with 0, and any API failure (invalid token, exhausted rate limit, missing repository) was swallowed by `catch (\Throwable) { return; }` in the repository page loaders, surfacing only as "0 releases found". Responses are now validated centrally per provider, so the message names the HTTP status, whether a token was configured and what to do about it. Repository fallback is preserved: the reasons are collected per attempt and printed as a single report with matched releases, release assets and the filters that rejected them. Assisted-By: Claude Opus 5 (1M context) --- README.md | 26 ++ psalm-baseline.xml | 4 + src/Command/Get.php | 64 ++- src/DLoad.php | 29 ++ src/Module/Downloader/Downloader.php | 144 +++++- .../Downloader/Exception/DownloadFailed.php | 31 ++ .../Downloader/Exception/NothingExtracted.php | 45 ++ .../Diagnostics/DownloadDiagnostics.php | 97 ++++ .../Internal/Diagnostics/ReleaseAttempt.php | 87 ++++ .../Diagnostics/RepositoryAttempt.php | 108 +++++ .../Downloader/Internal/DownloadContext.php | 11 + .../Exception/AccessDeniedException.php | 12 + .../Repository/Exception/ApiException.php | 12 + .../Exception/AuthenticationException.php | 12 + .../Exception/RateLimitException.php | 28 ++ .../Exception/RepositoryException.php | 28 ++ .../Exception/RepositoryNotFoundException.php | 12 + .../Repository/Internal/GitHub/Api/Client.php | 54 +-- .../Internal/GitHub/Api/RepositoryApi.php | 146 ++++--- .../Internal/GitHub/Api/ResponseValidator.php | 64 +++ .../Exception/GitHubRateLimitException.php | 29 +- .../Internal/GitHub/GitHubAsset.php | 4 +- .../Internal/GitHub/GitHubRepository.php | 14 +- .../Repository/Internal/GitLab/Api/Client.php | 29 +- .../Internal/GitLab/Api/RepositoryApi.php | 152 ++++--- .../Internal/GitLab/Api/ResponseValidator.php | 69 +++ .../Exception/GitLabRateLimitException.php | 14 +- .../Internal/GitLab/GitLabAsset.php | 4 +- .../Internal/GitLab/GitLabRepository.php | 13 +- .../Repository/Internal/ResponseValidator.php | 413 ++++++++++++++++++ src/Module/Task/Manager.php | 4 +- .../Exception/NothingExtractedTest.php | 50 +++ .../Diagnostics/DownloadDiagnosticsTest.php | 161 +++++++ .../Internal/GitHub/Api/ClientTest.php | 220 +++++----- .../GitHub/Api/ResponseValidatorTest.php | 180 ++++++++ .../Internal/GitHub/Stub/ClientStub.php | 1 + .../GitLab/Api/ResponseValidatorTest.php | 55 +++ .../GitHub => }/Stub/ResponseStub.php | 4 +- 38 files changed, 2117 insertions(+), 313 deletions(-) create mode 100644 src/Module/Downloader/Exception/DownloadFailed.php create mode 100644 src/Module/Downloader/Exception/NothingExtracted.php create mode 100644 src/Module/Downloader/Internal/Diagnostics/DownloadDiagnostics.php create mode 100644 src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php create mode 100644 src/Module/Downloader/Internal/Diagnostics/RepositoryAttempt.php create mode 100644 src/Module/Repository/Exception/AccessDeniedException.php create mode 100644 src/Module/Repository/Exception/ApiException.php create mode 100644 src/Module/Repository/Exception/AuthenticationException.php create mode 100644 src/Module/Repository/Exception/RateLimitException.php create mode 100644 src/Module/Repository/Exception/RepositoryException.php create mode 100644 src/Module/Repository/Exception/RepositoryNotFoundException.php create mode 100644 src/Module/Repository/Internal/GitHub/Api/ResponseValidator.php create mode 100644 src/Module/Repository/Internal/GitLab/Api/ResponseValidator.php create mode 100644 src/Module/Repository/Internal/ResponseValidator.php create mode 100644 tests/Unit/Module/Downloader/Exception/NothingExtractedTest.php create mode 100644 tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php create mode 100644 tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php create mode 100644 tests/Unit/Module/Repository/Internal/GitLab/Api/ResponseValidatorTest.php rename tests/Unit/Module/Repository/{Internal/GitHub => }/Stub/ResponseStub.php (97%) diff --git a/README.md b/README.md index 987e1cd..6abf007 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,7 @@ With DLoad, you can: - [PHAR Tools Management](#phar-tools-management) - [Frontend Asset Distribution](#frontend-asset-distribution) - [API Rate Limits](#api-rate-limits) +- [Failure Reporting](#failure-reporting) - [Gitlab CI configuration](#gitlab-ci-configuration) - [Contributing](#contributing) @@ -574,6 +575,31 @@ GITLAB_TOKEN=your_token_here ./vendor/bin/dload get Add to CI/CD environment variables for automated downloads. +> [!NOTE] +> In GitHub Actions, `secrets.GITHUB_TOKEN` is scoped to the current repository and shares a limit of +> 1,000 requests per hour across all jobs of the repository. With a large job matrix the limit may run out, +> and downloads from other repositories may be rejected. Use a personal access token if that happens. + +## Failure Reporting + +`dload get` exits with a non-zero code when at least one requested package was not installed, and prints +the reason for every failed download: the API error (invalid token, exhausted rate limit, missing repository), +the number of matched releases, the assets each checked release contains, and the filters that rejected them. + +``` + Failed to download `rr` +Requested: version `any`, OS `linux`, architecture `amd64`, minimum stability `stable`, asset type `any`. +Tried 1 repository(ies): + 1) github `roadrunner-server/roadrunner` + GitHub API rejected the credentials (HTTP 401: Bad credentials). + The API token from the GITHUB_TOKEN environment variable is invalid, expired or revoked. Provide a valid + token or unset the variable to use anonymous access. + +1 of 1 download(s) failed. +``` + +Run with `-vvv` to also get stack traces and the full request log. + ## Gitlab CI configuration When you make a release in Gitlab, make sure to upload your assets to the release page via diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 0f1aded..ca11921 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -198,6 +198,8 @@ + release->getAssets()]]> + @@ -220,7 +222,9 @@ + + diff --git a/src/Command/Get.php b/src/Command/Get.php index a165b35..1c7e24a 100644 --- a/src/Command/Get.php +++ b/src/Command/Get.php @@ -10,10 +10,12 @@ use Internal\DLoad\Module\Common\Stability; use Internal\DLoad\Module\Config\Schema\Action\Download as DownloadConfig; use Internal\DLoad\Module\Config\Schema\Actions; +use Internal\DLoad\Module\Downloader\Exception\DownloadFailed; use Internal\DLoad\Service\Container; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Exception\InvalidArgumentException; +use Symfony\Component\Console\Formatter\OutputFormatter; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -81,7 +83,8 @@ public function configure(): void * @param InputInterface $input Command input * @param OutputInterface $output Command output * - * @return int Command result code + * @return int `Command::SUCCESS` when every requested package is in place, + * `Command::FAILURE` when at least one download failed * * @throws \RuntimeException When no software is specified to download */ @@ -104,14 +107,43 @@ protected function execute(InputInterface $input, OutputInterface $output): int /** @var DLoad $dload */ $dload = $container->get(DLoad::class); - $forceDownload = $input->getOption('force'); + $forceDownload = (bool) $input->getOption('force'); + /** @var list $failures */ + $failures = []; foreach ($actions as $action) { - $dload->addTask($action, $forceDownload); + try { + $dload->addTask($action, $forceDownload)->then( + null, + static function (\Throwable $e) use (&$failures, $action): void { + $failures[] = [$action->software, $e]; + }, + ); + } catch (\Throwable $e) { + // A task may fail before it is even scheduled, e.g. when the software is unknown + $failures[] = [$action->software, $e]; + } } $dload->run(); - return Command::SUCCESS; + return $failures === [] + ? Command::SUCCESS + : $this->reportFailures($output, $failures, \count($actions)); + } + + /** + * Builds a user-facing explanation of a failure. + */ + private static function describeFailure(\Throwable $error): string + { + $message = $error->getMessage(); + + return match (true) { + // The report already describes the whole context of the failure + $error instanceof DownloadFailed => $error->report, + $message === '' => \sprintf('Unexpected %s without a message.', $error::class), + default => $message, + }; } /** @@ -201,4 +233,28 @@ private static function applyFlags(InputInterface $input, Container $container): "Unknown architecture: {$arch}.", )); } + + /** + * Prints the reason of every failed download and returns a failure exit code. + * + * @param list $failures Software identifier with its failure + * @param int<1, max> $total Total number of requested downloads + */ + private function reportFailures(OutputInterface $output, array $failures, int $total): int + { + foreach ($failures as [$software, $error]) { + $output->writeln(''); + $output->writeln(\sprintf(' Failed to download `%s` ', $software)); + $output->writeln(OutputFormatter::escape(self::describeFailure($error))); + + $output->isVeryVerbose() and $this->logger->exception($error, important: true); + } + + $output->writeln(''); + $output->writeln( + \sprintf('%d of %d download(s) failed.', \count($failures), $total), + ); + + return Command::FAILURE; + } } diff --git a/src/DLoad.php b/src/DLoad.php index 5bb9e55..96218c8 100644 --- a/src/DLoad.php +++ b/src/DLoad.php @@ -16,6 +16,7 @@ use Internal\DLoad\Module\Config\Schema\Embed\File; use Internal\DLoad\Module\Config\Schema\Embed\Software; use Internal\DLoad\Module\Downloader\Downloader; +use Internal\DLoad\Module\Downloader\Exception\NothingExtracted; use Internal\DLoad\Module\Downloader\SoftwareCollection; use Internal\DLoad\Module\Downloader\Task\DownloadResult; use Internal\DLoad\Module\Downloader\Task\DownloadTask; @@ -234,11 +235,14 @@ private function prepareExtractTask( $extractor = $archive->extract(); $this->logger->info('Extracting %s', $fileInfo->getFilename()); $binaryPattern = $this->generateBinaryExtractionConfig($software->binary); + $extractionRules = $this->describeExtractionRules($software, $binaryPattern); + $archiveFiles = []; while ($extractor->valid()) { $to = $rule = null; $file = $extractor->current(); \assert($file instanceof \SplFileInfo); + $archiveFiles[] = $file->getFilename(); # Check if it's binary and should be extracted $isBinary = false; @@ -283,6 +287,13 @@ private function prepareExtractTask( } } + # A downloaded asset without a single matching file means nothing was installed + $resultFiles === [] and throw new NothingExtracted( + assetName: $fileInfo->getFilename(), + rules: $extractionRules, + files: $archiveFiles, + ); + return new DloadResult($resultFiles, $resultBinary); } finally { // Cleanup: Delete the temporary downloaded file @@ -336,6 +347,24 @@ private function getDestinationPath(DownloadConfig $action): Path return Path::create($this->configDestination->path ?? $action->extractPath ?? (string) \getcwd()); } + /** + * Lists the patterns applied to archive entries, to explain why nothing was extracted. + * + * @param File|null $binaryPattern Generated binary extraction rule + * @return list + */ + private function describeExtractionRules(Software $software, ?File $binaryPattern): array + { + $rules = []; + $binaryPattern === null or $rules[] = \sprintf('binary `%s`', $binaryPattern->pattern); + + foreach ($software->files as $file) { + $rules[] = \sprintf('file `%s`', $file->pattern); + } + + return $rules; + } + /** * Generates a binary extraction configuration based on the provided binary configuration. * diff --git a/src/Module/Downloader/Downloader.php b/src/Module/Downloader/Downloader.php index 68f828b..09d2274 100644 --- a/src/Module/Downloader/Downloader.php +++ b/src/Module/Downloader/Downloader.php @@ -15,12 +15,17 @@ use Internal\DLoad\Module\Config\Schema\Action\Type; use Internal\DLoad\Module\Config\Schema\Downloader as DownloaderConfig; use Internal\DLoad\Module\Config\Schema\Embed\Software; +use Internal\DLoad\Module\Downloader\Exception\DownloadFailed; use Internal\DLoad\Module\Downloader\Exception\NotFound; +use Internal\DLoad\Module\Downloader\Internal\Diagnostics\DownloadDiagnostics; use Internal\DLoad\Module\Downloader\Internal\DownloadContext; use Internal\DLoad\Module\Downloader\Task\DownloadResult; use Internal\DLoad\Module\Downloader\Task\DownloadTask; use Internal\DLoad\Module\Repository\AssetInterface; use Internal\DLoad\Module\Repository\Collection\AssetsCollection; +use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; +use Internal\DLoad\Module\Repository\Exception\RateLimitException; +use Internal\DLoad\Module\Repository\Exception\RepositoryException; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Module\Repository\Repository; use Internal\DLoad\Module\Repository\RepositoryProvider; @@ -51,6 +56,9 @@ */ final class Downloader { + /** Number of release names collected for the failure report. */ + private const FETCHED_RELEASES_LIMIT = 10; + public function __construct( private readonly DownloaderConfig $config, private readonly Logger $logger, @@ -83,6 +91,13 @@ public function download( onProgress: $onProgress, actionConfig: $actionConfig, tempDir: $this->getTempDirectory(), + diagnostics: new DownloadDiagnostics( + software: $software, + actionConfig: $actionConfig, + operatingSystem: $this->operatingSystem, + architecture: $this->architecture, + stability: $this->stability, + ), ); $repositories = $software->repositories; @@ -90,9 +105,17 @@ public function download( return coroutine(function () use ($repositories, $context) { // Try every repo to load software. start: - $repositories === [] and throw new NotFound('No relevant repository found.'); + $repositories === [] and throw new DownloadFailed( + software: $context->software->getId(), + report: $context->diagnostics->render(), + ); $context->repoConfig = \array_shift($repositories); $repository = $this->repositoryProvider->getByConfig($context->repoConfig); + $context->repositoryAttempt = $context->diagnostics->addRepository( + type: $context->repoConfig->type, + name: $repository->getName(), + assetPattern: $context->repoConfig->assetPattern, + ); $this->logger->debug('Trying to load from repo `%s`', $repository->getName()); @@ -104,11 +127,18 @@ public function download( version: $context->release->getVersion(), ); } catch (NotFound $e) { + // Nothing suitable in this repository: the reason is already in the diagnostics + $this->logger->debug($e->getMessage()); + goto start; + } catch (RepositoryException $e) { + // The repository is unusable (API error, invalid token, rate limit, etc.): + // remember the reason and fall back to the next repository. + $context->repositoryAttempt->error = $e; $this->logger->debug($e->getMessage()); + $this->logger->exception($e, important: false); goto start; } catch (\Throwable $e) { - $this->logger->error($e->getMessage()); - $this->logger->exception($e); + $this->logger->exception($e, important: false); throw $e; } finally { $repository instanceof Destroyable and $repository->destroy(); @@ -141,14 +171,15 @@ private function processRepository(Repository $repository, DownloadContext $cont $repository->getName(), ); + $allReleases = $repository->getReleases(); if ($context->actionConfig->version !== null) { $constraint = Constraint::fromConstraintString($context->actionConfig->version); // Filter by version if specified - $releasesCollection = $repository->getReleases() + $releasesCollection = $allReleases ->minimumStability($constraint->minimumStability) ->satisfies($constraint); } else { - $releasesCollection = $repository->getReleases() + $releasesCollection = $allReleases ->minimumStability($this->stability); } @@ -160,18 +191,29 @@ private function processRepository(Repository $repository, DownloadContext $cont // Try without limit $releases === [] and $releases = $releasesCollection->limit(0)->toArray(); + $context->repositoryAttempt->matchedReleases = \count($releases); + + if ($releases === []) { + // Show what the repository actually offers: it explains version and stability mismatches + $context->repositoryAttempt->registerFetchedReleases($this->fetchReleaseNames($allReleases)); + + throw new NotFound('No relevant release found.'); + } + process_release: $releases === [] and throw new NotFound('No relevant release found.'); $context->release = \array_shift($releases); + $context->releaseAttempt = $context->repositoryAttempt->addRelease($context->release->getName()); - $this->logger->info('Loading release `%s`', $context->release->getName()); + $this->logger->debug('Loading release `%s`', $context->release->getName()); try { await(coroutine($this->processRelease($context))); return $context->release; } catch (NotFound $e) { + $context->releaseAttempt->reason ??= $e->getMessage(); $this->logger->debug($e->getMessage()); - $this->logger->exception($e); + $this->logger->exception($e, important: false); goto process_release; } }; @@ -188,13 +230,23 @@ private function processRepository(Repository $repository, DownloadContext $cont */ private function processRelease(DownloadContext $context): \Closure { - return fn(): AssetInterface => match (true) { - // Phar assets usually don't depend on OS or architecture, so we can use gradual filtering - $context->actionConfig->type === Type::Phar => $this->findAssetWithGradualFiltering($context), - // Use strict filtering when binary configuration exists - $context->software->binary !== null => $this->findAssetWithStrictFiltering($context), - // Use gradual filtering when no binary configuration exists - default => $this->findAssetWithGradualFiltering($context), + return function () use ($context): AssetInterface { + // Remember all the release assets: it makes a "nothing matched" report meaningful + $names = []; + foreach ($context->release->getAssets() as $asset) { + $names[] = $asset->getName(); + } + + $context->releaseAttempt->registerAssets($names); + + return match (true) { + // Phar assets usually don't depend on OS or architecture, so we can use gradual filtering + $context->actionConfig->type === Type::Phar => $this->findAssetWithGradualFiltering($context), + // Use strict filtering when binary configuration exists + $context->software->binary !== null => $this->findAssetWithStrictFiltering($context), + // Use gradual filtering when no binary configuration exists + default => $this->findAssetWithGradualFiltering($context), + }; }; } @@ -217,7 +269,15 @@ private function findAssetWithStrictFiltering(DownloadContext $context): AssetIn $allAssets = $this->addFormatFilter($assetsCollection, $context->actionConfig)->toArray(); $this->logger->debug('%d matching assets found.', \count($allAssets)); - $allAssets === [] and throw new NotFound('No relevant assets found.'); + $allAssets === [] and throw new NotFound( + \sprintf( + 'no asset matches OS `%s`, architecture `%s`, name pattern `%s`%s', + $this->operatingSystem->value, + $this->architecture->value, + $context->repoConfig->assetPattern, + $this->describeFormatFilter($context->actionConfig), + ), + ); // Sort assets by priority and try to process them $sortedAssets = $this->sortAssetsByPriority($allAssets, $this->archiveService->getSupportedExtensions()); @@ -241,7 +301,13 @@ private function findAssetWithGradualFiltering(DownloadContext $context): AssetI $supportedExtensions = $this->archiveService->getSupportedExtensions(); // If we got here, no assets were found with any filter combination - \count($assetsCollection) === 0 and throw new NotFound('No relevant assets found.'); + \count($assetsCollection) === 0 and throw new NotFound( + \sprintf( + 'no asset matches name pattern `%s`%s', + $context->repoConfig->assetPattern, + $this->describeFormatFilter($context->actionConfig), + ), + ); // Try #1: Filter by both OS and architecture (most specific) $filteredAssets = $assetsCollection @@ -279,7 +345,7 @@ private function findAssetWithGradualFiltering(DownloadContext $context): AssetI $sortedAssets = $this->sortAssetsByPriority($filteredAssets, $supportedExtensions); try { return $this->tryProcessAssets($sortedAssets, $context); - } catch (\RuntimeException $e) { + } catch (NotFound $e) { $this->logger->debug('Failed to process assets with OS-only filtering: %s', $e->getMessage()); // Continue to next filter strategy } @@ -299,7 +365,7 @@ private function findAssetWithGradualFiltering(DownloadContext $context): AssetI $sortedAssets = $this->sortAssetsByPriority($filteredAssets, $supportedExtensions); try { return $this->tryProcessAssets($sortedAssets, $context); - } catch (\RuntimeException $e) { + } catch (NotFound $e) { $this->logger->debug('Failed to process assets with architecture-only filtering: %s', $e->getMessage()); // Continue to next filter strategy } @@ -327,18 +393,56 @@ private function findAssetWithGradualFiltering(DownloadContext $context): AssetI private function tryProcessAssets(array $assets, DownloadContext $context): AssetInterface { process_asset: - $assets === [] and throw new NotFound('No relevant asset found.'); + $assets === [] and throw new NotFound('none of the matching assets could be downloaded'); $context->asset = \array_shift($assets); $this->logger->debug('Trying to load asset `%s`', $context->asset->getName()); try { await(coroutine($this->processAsset($context))); return $context->asset; + } catch (RateLimitException $e) { + // Retrying other assets makes the situation worse: report the limit immediately + throw $e; } catch (\Throwable $e) { - $this->logger->exception($e); + $context->releaseAttempt->addFailure($context->asset->getName(), $e); + $this->logger->exception($e, important: false); goto process_asset; } } + /** + * Collects names of the first releases available in the repository for a failure report. + * + * @return list + */ + private function fetchReleaseNames(ReleasesCollection $releases): array + { + $names = []; + foreach ($releases as $release) { + $names[] = $release->getName(); + + if (\count($names) >= self::FETCHED_RELEASES_LIMIT) { + break; + } + } + + return $names; + } + + /** + * Describes the asset format restriction for failure reports. + */ + private function describeFormatFilter(Download $actionOptions): string + { + return match ($actionOptions->type) { + Type::Phar => ' and the `phar` extension', + Type::Archive => \sprintf( + ' and one of the archive extensions: %s', + \implode(', ', $this->archiveService->getSupportedExtensions()), + ), + default => '', + }; + } + /** * Sorts assets by priority with supported archives first, then other files. * diff --git a/src/Module/Downloader/Exception/DownloadFailed.php b/src/Module/Downloader/Exception/DownloadFailed.php new file mode 100644 index 0000000..2d53d21 --- /dev/null +++ b/src/Module/Downloader/Exception/DownloadFailed.php @@ -0,0 +1,31 @@ + $rules Patterns that were applied to the archive entries + * @param list $files Names of the files found in the archive + */ + public function __construct( + string $assetName, + array $rules, + array $files, + ) { + $listed = \array_slice($files, 0, self::FILES_LIMIT); + $hidden = \count($files) - \count($listed); + + parent::__construct( + \sprintf( + "Nothing was extracted from `%s`: none of the %d file(s) inside matches the extraction rules.\n" + . "Extraction rules: %s\n" + . 'Files in the asset: %s%s', + $assetName, + \count($files), + $rules === [] ? 'none' : \implode(', ', $rules), + $listed === [] ? 'none' : \implode(', ', $listed), + $hidden > 0 ? \sprintf(' and %d more', $hidden) : '', + ), + ); + } +} diff --git a/src/Module/Downloader/Internal/Diagnostics/DownloadDiagnostics.php b/src/Module/Downloader/Internal/Diagnostics/DownloadDiagnostics.php new file mode 100644 index 0000000..7161e0f --- /dev/null +++ b/src/Module/Downloader/Internal/Diagnostics/DownloadDiagnostics.php @@ -0,0 +1,97 @@ +addRepository('github', 'owner/repo', '/^.*$/'); + * $repository->matchedReleases = 0; + * throw new DownloadFailed($diagnostics->render()); + * ``` + * + * @internal + */ +final class DownloadDiagnostics +{ + /** @var list */ + private array $repositories = []; + + public function __construct( + private readonly Software $software, + private readonly DownloadConfig $actionConfig, + private readonly OperatingSystem $operatingSystem, + private readonly Architecture $architecture, + private readonly Stability $stability, + ) {} + + /** + * @param non-empty-string $type Repository type, e.g. `github` + * @param non-empty-string $name Repository name, e.g. `owner/repo` + * @param non-empty-string $assetPattern Asset name pattern from the configuration + */ + public function addRepository(string $type, string $name, string $assetPattern): RepositoryAttempt + { + return $this->repositories[] = new RepositoryAttempt($type, $name, $assetPattern); + } + + /** + * Builds the human-readable report about all the attempts. + * + * @return non-empty-string + */ + public function render(): string + { + $lines = [$this->renderRequest()]; + + if ($this->repositories === []) { + $lines[] = \sprintf( + 'No repositories are configured for `%s`. Add a `repository` entry to the software definition.', + $this->software->getId(), + ); + + return \implode("\n", $lines); + } + + $lines[] = \sprintf('Tried %d repository(ies):', \count($this->repositories)); + + foreach ($this->repositories as $index => $repository) { + foreach ($repository->describe() as $lineIndex => $line) { + $lines[] = $lineIndex === 0 + ? \sprintf(' %d) %s', $index + 1, $line) + : ' ' . $line; + } + } + + return \implode("\n", $lines); + } + + /** + * @return non-empty-string + */ + private function renderRequest(): string + { + return \sprintf( + 'Requested: version `%s`, OS `%s`, architecture `%s`, minimum stability `%s`, asset type `%s`.', + $this->actionConfig->version ?? 'any', + $this->operatingSystem->value, + $this->architecture->value, + $this->stability->value, + $this->actionConfig->type?->value ?? 'any', + ); + } +} diff --git a/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php b/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php new file mode 100644 index 0000000..0be6e8d --- /dev/null +++ b/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php @@ -0,0 +1,87 @@ + Total number of assets in the release */ + public int $assetsTotal = 0; + + /** @var string|null Why the release was rejected */ + public ?string $reason = null; + + /** @var list Names of all assets in the release */ + private array $assetNames = []; + + /** @var list Errors occurred while downloading matched assets */ + private array $failures = []; + + /** + * @param non-empty-string $name Release name or tag + */ + public function __construct( + public readonly string $name, + ) {} + + /** + * @param list $names Names of all assets available in the release + */ + public function registerAssets(array $names): void + { + $this->assetsTotal = \count($names); + $this->assetNames = $names; + } + + /** + * Registers a failure of a matched asset, e.g. a broken download. + * + * @param non-empty-string $assetName + */ + public function addFailure(string $assetName, \Throwable $error): void + { + $this->failures[] = \sprintf('`%s`: %s', $assetName, $error->getMessage()); + } + + /** + * Renders release details as report lines. + * + * @return list + */ + public function describe(): array + { + $lines = [ + \sprintf( + '%s: %d asset(s)%s', + $this->name, + $this->assetsTotal, + $this->reason === null ? '' : ', ' . $this->reason, + ), + ]; + + if ($this->assetNames !== []) { + $listed = \array_slice($this->assetNames, 0, self::ASSETS_LIMIT); + $hidden = \count($this->assetNames) - \count($listed); + $lines[] = \sprintf( + ' Assets: %s%s', + \implode(', ', $listed), + $hidden > 0 ? \sprintf(' and %d more', $hidden) : '', + ); + } + + foreach ($this->failures as $failure) { + $lines[] = ' Failed asset ' . $failure; + } + + return $lines; + } +} diff --git a/src/Module/Downloader/Internal/Diagnostics/RepositoryAttempt.php b/src/Module/Downloader/Internal/Diagnostics/RepositoryAttempt.php new file mode 100644 index 0000000..0d7a135 --- /dev/null +++ b/src/Module/Downloader/Internal/Diagnostics/RepositoryAttempt.php @@ -0,0 +1,108 @@ +|null Number of releases that match the version constraint and stability */ + public ?int $matchedReleases = null; + + /** @var list Release names fetched from the repository */ + private array $fetchedReleases = []; + + /** @var list */ + private array $releases = []; + + /** @var int<0, max> Number of releases processed but not included into the report */ + private int $hiddenReleases = 0; + + /** + * @param non-empty-string $type Repository type, e.g. `github` + * @param non-empty-string $name Repository name, e.g. `owner/repo` + * @param non-empty-string $assetPattern Asset name pattern from the configuration + */ + public function __construct( + public readonly string $type, + public readonly string $name, + public readonly string $assetPattern, + ) {} + + /** + * @param list $names Release names available in the repository + */ + public function registerFetchedReleases(array $names): void + { + $this->fetchedReleases = $names; + } + + /** + * @param non-empty-string $name Release name or tag + */ + public function addRelease(string $name): ReleaseAttempt + { + $attempt = new ReleaseAttempt($name); + + if (\count($this->releases) < self::RELEASES_LIMIT) { + $this->releases[] = $attempt; + } else { + ++$this->hiddenReleases; + } + + return $attempt; + } + + /** + * Renders repository details as report lines. + * + * @return list + */ + public function describe(): array + { + $lines = [\sprintf('%s `%s`', $this->type, $this->name)]; + + if ($this->error !== null) { + foreach (\explode("\n", $this->error->getMessage()) as $line) { + $lines[] = ' ' . $line; + } + } + + $this->matchedReleases === null or $lines[] = \sprintf( + ' %d release(s) match the requested version and stability.', + $this->matchedReleases, + ); + + if ($this->matchedReleases === 0 && $this->fetchedReleases !== []) { + $listed = \array_slice($this->fetchedReleases, 0, self::FETCHED_RELEASES_LIMIT); + $lines[] = \sprintf(' Releases available in the repository: %s', \implode(', ', $listed)); + } + + if ($this->releases !== []) { + $lines[] = ' Checked releases:'; + foreach ($this->releases as $release) { + foreach ($release->describe() as $index => $line) { + $lines[] = $index === 0 ? ' - ' . $line : ' ' . $line; + } + } + + $this->hiddenReleases === 0 or $lines[] = \sprintf(' and %d more release(s)', $this->hiddenReleases); + } + + return $lines; + } +} diff --git a/src/Module/Downloader/Internal/DownloadContext.php b/src/Module/Downloader/Internal/DownloadContext.php index cedaf14..9db0fc3 100644 --- a/src/Module/Downloader/Internal/DownloadContext.php +++ b/src/Module/Downloader/Internal/DownloadContext.php @@ -7,6 +7,9 @@ use Internal\DLoad\Module\Config\Schema\Action\Download as DownloadConfig; use Internal\DLoad\Module\Config\Schema\Embed\Repository; use Internal\DLoad\Module\Config\Schema\Embed\Software; +use Internal\DLoad\Module\Downloader\Internal\Diagnostics\DownloadDiagnostics; +use Internal\DLoad\Module\Downloader\Internal\Diagnostics\ReleaseAttempt; +use Internal\DLoad\Module\Downloader\Internal\Diagnostics\RepositoryAttempt; use Internal\DLoad\Module\Repository\AssetInterface; use Internal\DLoad\Module\Repository\ReleaseInterface; use Internal\DLoad\Module\Task\Progress; @@ -34,6 +37,12 @@ final class DownloadContext /** @var ReleaseInterface Current release being processed */ public ReleaseInterface $release; + /** @var RepositoryAttempt Diagnostics of the repository being processed */ + public RepositoryAttempt $repositoryAttempt; + + /** @var ReleaseAttempt Diagnostics of the release being processed */ + public ReleaseAttempt $releaseAttempt; + /** * Creates a new download context. * @@ -42,11 +51,13 @@ final class DownloadContext * Exception thrown in this callback will stop and revert the task. * @param DownloadConfig $actionConfig Download action configuration * @param Path $tempDir Temporary directory for downloads + * @param DownloadDiagnostics $diagnostics Collector of failure reasons for the final report */ public function __construct( public readonly Software $software, public readonly \Closure $onProgress, public readonly DownloadConfig $actionConfig, public readonly Path $tempDir, + public readonly DownloadDiagnostics $diagnostics, ) {} } diff --git a/src/Module/Repository/Exception/AccessDeniedException.php b/src/Module/Repository/Exception/AccessDeniedException.php new file mode 100644 index 0000000..d322206 --- /dev/null +++ b/src/Module/Repository/Exception/AccessDeniedException.php @@ -0,0 +1,12 @@ + 'application/vnd.github.v3+json', ]; + private readonly ResponseValidator $validator; + public function __construct( private readonly HttpFactory $httpFactory, private readonly ClientInterface $client, @@ -39,13 +41,14 @@ public function __construct( ) { // Add authorization header if token is available $this->gitHubConfig->token !== null and $this->defaultHeaders['authorization'] = 'Bearer ' . $this->gitHubConfig->token; + + $this->validator = new ResponseValidator(authenticated: $this->gitHubConfig->token !== null); } /** * @param Method|non-empty-string $method * @param array $headers - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function request(Method|string $method, string|UriInterface $uri, array $headers = []): ResponseInterface { @@ -55,45 +58,18 @@ public function request(Method|string $method, string|UriInterface $uri, array $ } /** - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function sendRequest(RequestInterface $request): ResponseInterface { - $response = $this->client->sendRequest($request); - - $this->checkForRateLimit($response); - - return $response; - } - - /** - * @throws GitHubRateLimitException - */ - private function checkForRateLimit(ResponseInterface $response): void - { - // GitHub rate limit responses typically have 403 status - if ($response->getStatusCode() !== 403) { - return; + try { + $response = $this->client->sendRequest($request); + } catch (ClientExceptionInterface $e) { + throw $this->validator->transportFailure($request, $e); } - $body = $response->getBody()->__toString(); + $this->validator->validate($request, $response); - try { - /** @var mixed $decoded */ - $decoded = \json_decode($body, true, 512, JSON_THROW_ON_ERROR); - - // GitHub rate limit responses have format: ["API rate limit ...", "https://docs.github.com/..."] - if (\is_array($decoded) - && \count($decoded) === 2 - && \is_string(\reset($decoded)) - && \is_string(\next($decoded)) - && \str_contains(\reset($decoded), 'API rate limit') - ) { - throw GitHubRateLimitException::fromApiResponse($decoded); - } - } catch (\JsonException) { - // Not a JSON response, continue without rate limit check - } + return $response; } } diff --git a/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php index 943d5aa..c3ce8fb 100644 --- a/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php @@ -6,12 +6,12 @@ use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; use Internal\DLoad\Module\HttpClient\Method; +use Internal\DLoad\Module\Repository\Exception\ApiException; +use Internal\DLoad\Module\Repository\Exception\RepositoryException; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Response\ReleaseInfo; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Response\RepositoryInfo; -use Internal\DLoad\Module\Repository\Internal\GitHub\Exception\GitHubRateLimitException; use Internal\DLoad\Module\Repository\Internal\Paginator; use Internal\DLoad\Service\Logger; -use Psr\Http\Client\ClientExceptionInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; @@ -50,8 +50,7 @@ public function __construct( /** * @param Method|non-empty-string $method * @param array $headers - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function request(Method|string $method, string|UriInterface $uri, array $headers = []): ResponseInterface { @@ -59,8 +58,7 @@ public function request(Method|string $method, string|UriInterface $uri, array $ } /** - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function getRepository(): RepositoryInfo { @@ -83,8 +81,7 @@ public function getRepository(): RepositoryInfo /** * @param int<1, max> $page * @return Paginator - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function getReleases(int $page = 1): Paginator { @@ -92,59 +89,110 @@ public function getReleases(int $page = 1): Paginator $currentPage = $page; do { - try { - $response = $this->releasesRequest($currentPage); - - /** @var array, - * prerelease: bool, - * draft: bool - * }> $data */ - $data = \json_decode($response->getBody()->__toString(), true, 512, JSON_THROW_ON_ERROR); - - // If empty response, no more pages - if ($data === []) { - return; - } + $response = $this->releasesRequest($currentPage); + + /** @var list, + * prerelease: bool, + * draft: bool + * }> $data */ + $data = $this->decodeReleasesResponse($response); + + // If empty response, no more pages + if ($data === []) { + return; + } - $releases = []; - foreach ($data as $releaseData) { - try { - $releases[] = ReleaseInfo::fromApiResponse($releaseData); - } catch (\Throwable $e) { - $this->logger->exception($e, important: false); - // Skip invalid releases - continue; - } + $releases = []; + $failure = null; + foreach ($data as $releaseData) { + try { + $releases[] = ReleaseInfo::fromApiResponse($releaseData); + } catch (\Throwable $e) { + $failure ??= $e; + $this->logger->exception($e, important: false); + // Skip invalid releases + continue; } + } - yield $releases; - - // Check if there are more pages - $hasMorePages = $this->hasNextPage($response); - $currentPage++; - } catch (ClientExceptionInterface $e) { - $this->logger->exception($e, important: false); - return; + // The whole page is unreadable: the response structure is not what we expect + if ($releases === [] && $failure !== null) { + throw new ApiException( + \sprintf( + 'GitHub API returned %d release(s) for repository `%s`, but none of them could be read: %s', + \count($data), + $this->repositoryPath, + $failure->getMessage(), + ), + $this->repositoryPath, + $failure, + ); } + + yield $releases; + + // Check if there are more pages + $hasMorePages = $this->hasNextPage($response); + $currentPage++; } while ($hasMorePages); }; return Paginator::createFromGenerator($pageLoader(), null); } + /** + * Decodes a releases list response and validates its shape. + * + * @return list> + * @throws ApiException When the response is not a list of releases. + */ + private function decodeReleasesResponse(ResponseInterface $response): array + { + $body = $response->getBody()->__toString(); + + try { + /** @var mixed $data */ + $data = \json_decode($body, true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + throw new ApiException( + \sprintf( + 'GitHub API returned a malformed response for repository `%s`: %s', + $this->repositoryPath, + $e->getMessage(), + ), + $this->repositoryPath, + $e, + ); + } + + if (!\is_array($data) || !\array_is_list($data)) { + throw new ApiException( + \sprintf( + 'GitHub API returned an unexpected response for repository `%s`: ' + . 'a list of releases is expected, got %s.', + $this->repositoryPath, + \is_array($data) ? 'an object: ' . \substr($body, 0, 200) : \get_debug_type($data), + ), + $this->repositoryPath, + ); + } + + /** @var list> */ + return $data; + } + /** * @param positive-int $page - * @throws GitHubRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ private function releasesRequest(int $page): ResponseInterface { diff --git a/src/Module/Repository/Internal/GitHub/Api/ResponseValidator.php b/src/Module/Repository/Internal/GitHub/Api/ResponseValidator.php new file mode 100644 index 0000000..01ca4fb --- /dev/null +++ b/src/Module/Repository/Internal/GitHub/Api/ResponseValidator.php @@ -0,0 +1,64 @@ + - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function download(?\Closure $progress = null): \Generator { diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index ee0ecdd..f24b5e0 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -7,7 +7,6 @@ use Internal\Destroy\Destroyable; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; -use Internal\DLoad\Module\Repository\Internal\GitHub\Exception\GitHubRateLimitException; use Internal\DLoad\Module\Repository\Repository; use Internal\DLoad\Service\Logger; @@ -54,6 +53,7 @@ public function getReleases(): ReleasesCollection // Create a generator function for lazy loading release pages $pageLoader = function (): \Generator { $page = 0; + $anyPageLoaded = false; do { try { @@ -74,12 +74,18 @@ public function getReleases(): ReleasesCollection } } yield $toYield; + $anyPageLoaded = true; // Check if there are more pages by getting next page $hasMorePages = $paginator->getNextPage() !== null; - } catch (GitHubRateLimitException $e) { - throw $e; - } catch (\Throwable) { + } catch (\Throwable $e) { + # The first page is mandatory: when it fails, there is nothing to download and the reason + # (invalid token, rate limit, missing repository, etc.) must reach the user. + $anyPageLoaded or throw $e; + + # Already loaded releases are enough to continue, so a failure of a subsequent page + # only stops the pagination. + $this->logger->exception($e, important: false); return; } } while ($hasMorePages); diff --git a/src/Module/Repository/Internal/GitLab/Api/Client.php b/src/Module/Repository/Internal/GitLab/Api/Client.php index 634408e..2306957 100644 --- a/src/Module/Repository/Internal/GitLab/Api/Client.php +++ b/src/Module/Repository/Internal/GitLab/Api/Client.php @@ -7,7 +7,7 @@ use Internal\DLoad\Module\Config\Schema\GitLab; use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; use Internal\DLoad\Module\HttpClient\Method; -use Internal\DLoad\Module\Repository\Internal\GitLab\Exception\GitLabRateLimitException; +use Internal\DLoad\Module\Repository\Exception\RepositoryException; use Psr\Http\Client\ClientExceptionInterface; use Psr\Http\Client\ClientInterface; use Psr\Http\Message\RequestInterface; @@ -17,8 +17,8 @@ /** * HTTP client wrapper with GitLab-specific error handling and authentication. * - * Detects and handles GitLab Rate Limit responses automatically. - * Adds GitLab API token authentication when available. + * Converts unsuccessful responses (rate limits, invalid token, missing project, etc.) + * into exceptions with actionable messages. Adds GitLab API token authentication when available. * * @internal * @psalm-internal Internal\DLoad\Module\Repository\Internal\GitLab @@ -32,6 +32,8 @@ final class Client 'accept' => 'application/json', ]; + private readonly ResponseValidator $validator; + public function __construct( private readonly HttpFactory $httpFactory, private readonly ClientInterface $client, @@ -39,11 +41,12 @@ public function __construct( ) { // Add authorization header if token is available $this->gitLabConfig->token !== null and $this->defaultHeaders['authorization'] = 'Bearer ' . $this->gitLabConfig->token; + + $this->validator = new ResponseValidator(authenticated: $this->gitLabConfig->token !== null); } /** - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function downloadArtifact(string|UriInterface $uri): ResponseInterface { @@ -62,8 +65,7 @@ public function downloadArtifact(string|UriInterface $uri): ResponseInterface /** * @param Method|non-empty-string $method * @param array $headers - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function request(Method|string $method, string|UriInterface $uri, array $headers = []): ResponseInterface { @@ -73,17 +75,18 @@ public function request(Method|string $method, string|UriInterface $uri, array $ } /** - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function sendRequest(RequestInterface $request): ResponseInterface { - $response = $this->client->sendRequest($request); - - if ($response->getStatusCode() === 429) { - throw new GitLabRateLimitException(); + try { + $response = $this->client->sendRequest($request); + } catch (ClientExceptionInterface $e) { + throw $this->validator->transportFailure($request, $e); } + $this->validator->validate($request, $response); + return $response; } } diff --git a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php index 020aeb1..fab47a3 100644 --- a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php @@ -6,11 +6,11 @@ use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; use Internal\DLoad\Module\HttpClient\Method; +use Internal\DLoad\Module\Repository\Exception\ApiException; +use Internal\DLoad\Module\Repository\Exception\RepositoryException; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\Response\ReleaseInfo; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\Response\RepositoryInfo; -use Internal\DLoad\Module\Repository\Internal\GitLab\Exception\GitLabRateLimitException; use Internal\DLoad\Module\Repository\Internal\Paginator; -use Psr\Http\Client\ClientExceptionInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; @@ -45,8 +45,7 @@ public function __construct( * @param non-empty-string $repositoryPath * @param non-empty-string $releaseName * @param non-empty-string $fileName - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function downloadArtifact(string $repositoryPath, string $releaseName, string $fileName): ResponseInterface { @@ -57,8 +56,7 @@ public function downloadArtifact(string $repositoryPath, string $releaseName, st /** * @param Method|non-empty-string $method * @param array $headers - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function request(Method|string $method, string|UriInterface $uri, array $headers = []): ResponseInterface { @@ -66,8 +64,7 @@ public function request(Method|string $method, string|UriInterface $uri, array $ } /** - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function getRepository(): RepositoryInfo { @@ -90,8 +87,7 @@ public function getRepository(): RepositoryInfo /** * @param int<1, max> $page * @return Paginator - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function getReleases(int $page = 1): Paginator { @@ -99,60 +95,112 @@ public function getReleases(int $page = 1): Paginator $currentPage = $page; do { - try { - $response = $this->releasesRequest($currentPage); - - /** @var array - * }, - * upcoming_release: bool - * }> $data */ - $data = \json_decode($response->getBody()->__toString(), true, 512, JSON_THROW_ON_ERROR); - - // If empty response, no more pages - if ($data === []) { - return; - } + $response = $this->releasesRequest($currentPage); + + /** @var list + * }, + * upcoming_release: bool + * }> $data */ + $data = $this->decodeReleasesResponse($response); + + // If empty response, no more pages + if ($data === []) { + return; + } - $releases = []; - foreach ($data as $releaseData) { - try { - $releases[] = ReleaseInfo::fromApiResponse($releaseData); - } catch (\Throwable) { - // Skip invalid releases - continue; - } + $releases = []; + $failure = null; + foreach ($data as $releaseData) { + try { + $releases[] = ReleaseInfo::fromApiResponse($releaseData); + } catch (\Throwable $e) { + $failure ??= $e; + // Skip invalid releases + continue; } + } - yield $releases; - - // Check if there are more pages - $hasMorePages = $this->hasNextPage($response); - $currentPage++; - } catch (ClientExceptionInterface) { - return; + // The whole page is unreadable: the response structure is not what we expect + if ($releases === [] && $failure !== null) { + throw new ApiException( + \sprintf( + 'GitLab API returned %d release(s) for project `%s`, but none of them could be read: %s', + \count($data), + $this->repositoryPath, + $failure->getMessage(), + ), + $this->repositoryPath, + $failure, + ); } + + yield $releases; + + // Check if there are more pages + $hasMorePages = $this->hasNextPage($response); + $currentPage++; } while ($hasMorePages); }; return Paginator::createFromGenerator($pageLoader(), null); } + /** + * Decodes a releases list response and validates its shape. + * + * @return list> + * @throws ApiException When the response is not a list of releases. + */ + private function decodeReleasesResponse(ResponseInterface $response): array + { + $body = $response->getBody()->__toString(); + + try { + /** @var mixed $data */ + $data = \json_decode($body, true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + throw new ApiException( + \sprintf( + 'GitLab API returned a malformed response for project `%s`: %s', + $this->repositoryPath, + $e->getMessage(), + ), + $this->repositoryPath, + $e, + ); + } + + if (!\is_array($data) || !\array_is_list($data)) { + throw new ApiException( + \sprintf( + 'GitLab API returned an unexpected response for project `%s`: ' + . 'a list of releases is expected, got %s.', + $this->repositoryPath, + \is_array($data) ? 'an object: ' . \substr($body, 0, 200) : \get_debug_type($data), + ), + $this->repositoryPath, + ); + } + + /** @var list> */ + return $data; + } + /** * @param positive-int $page - * @throws GitLabRateLimitException - * @throws ClientExceptionInterface + * @throws RepositoryException */ private function releasesRequest(int $page): ResponseInterface { diff --git a/src/Module/Repository/Internal/GitLab/Api/ResponseValidator.php b/src/Module/Repository/Internal/GitLab/Api/ResponseValidator.php new file mode 100644 index 0000000..6e135ce --- /dev/null +++ b/src/Module/Repository/Internal/GitLab/Api/ResponseValidator.php @@ -0,0 +1,69 @@ + - * @throws ClientExceptionInterface + * @throws RepositoryException */ public function download(?\Closure $progress = null): \Generator { diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index 8e1c9c4..dffa7bc 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -7,7 +7,6 @@ use Internal\Destroy\Destroyable; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; -use Internal\DLoad\Module\Repository\Internal\GitLab\Exception\GitLabRateLimitException; use Internal\DLoad\Module\Repository\Repository; /** @@ -50,6 +49,7 @@ public function getReleases(): ReleasesCollection // Create a generator function for lazy loading release pages $pageLoader = function (): \Generator { $page = 0; + $anyPageLoaded = false; do { try { @@ -69,12 +69,17 @@ public function getReleases(): ReleasesCollection } } yield $toYield; + $anyPageLoaded = true; // Check if there are more pages by getting next page $hasMorePages = $paginator->getNextPage() !== null; - } catch (GitLabRateLimitException $e) { - throw $e; - } catch (\Throwable) { + } catch (\Throwable $e) { + # The first page is mandatory: when it fails, there is nothing to download and the reason + # (invalid token, rate limit, missing project, etc.) must reach the user. + $anyPageLoaded or throw $e; + + # Already loaded releases are enough to continue, so a failure of a subsequent page + # only stops the pagination. return; } } while ($hasMorePages); diff --git a/src/Module/Repository/Internal/ResponseValidator.php b/src/Module/Repository/Internal/ResponseValidator.php new file mode 100644 index 0000000..6e893c9 --- /dev/null +++ b/src/Module/Repository/Internal/ResponseValidator.php @@ -0,0 +1,413 @@ +getStatusCode(); + if ($status < 400) { + return; + } + + $apiMessage = self::extractApiMessage($response); + $repository = $this->repositoryFromUri((string) $request->getUri()); + $endpoint = \sprintf('%s %s', $request->getMethod(), $request->getUri()); + + if ($this->isRateLimited($status, $response, $apiMessage)) { + throw $this->createRateLimitException($response, $apiMessage, $repository); + } + + throw match (true) { + $status === 401 => new AuthenticationException( + $this->authenticationMessage($apiMessage), + $repository, + ), + $status === 403 => new AccessDeniedException( + $this->accessDeniedMessage($apiMessage, $repository), + $repository, + ), + $status === 404 => new RepositoryNotFoundException( + $this->notFoundMessage($apiMessage, $repository, $endpoint), + $repository, + ), + $status >= 500 => new ApiException( + \sprintf( + '%s API is unavailable: HTTP %d %s (%s).%s Try again later.', + $this->providerName(), + $status, + $response->getReasonPhrase(), + $endpoint, + $apiMessage === null ? '' : ' ' . $apiMessage, + ), + $repository, + ), + default => new ApiException( + \sprintf( + '%s API request failed with HTTP %d %s (%s).%s', + $this->providerName(), + $status, + $response->getReasonPhrase(), + $endpoint, + $apiMessage === null ? '' : ' ' . $apiMessage, + ), + $repository, + ), + }; + } + + /** + * Wraps a transport-level failure (DNS, TLS, timeout, etc.) into a readable exception. + */ + public function transportFailure(RequestInterface $request, \Throwable $e): ApiException + { + return new ApiException( + \sprintf( + 'Failed to reach %s API (%s %s): %s', + $this->providerName(), + $request->getMethod(), + $request->getUri(), + $e->getMessage(), + ), + $this->repositoryFromUri((string) $request->getUri()), + $e, + ); + } + + /** + * @return non-empty-string Provider display name, e.g. `GitHub`. + */ + abstract protected function providerName(): string; + + /** + * @return non-empty-string Environment variable that holds the API token. + */ + abstract protected function tokenEnvVariable(): string; + + /** + * @return non-empty-string How the provider calls a repository, e.g. `repository` or `project`. + */ + protected function repositoryTerm(): string + { + return 'repository'; + } + + /** + * Extracts a repository identifier from an API URL for better error messages. + * + * @return non-empty-string|null + */ + abstract protected function repositoryFromUri(string $uri): ?string; + + /** + * @return positive-int|null Requests per hour allowed without a token. + */ + abstract protected function anonymousRateLimit(): ?int; + + /** + * @return positive-int|null Requests per hour allowed with a token. + */ + abstract protected function authenticatedRateLimit(): ?int; + + /** + * Creates a provider specific rate limit exception. + * + * @param non-empty-string $message + * @param non-empty-string|null $repository + */ + abstract protected function instantiateRateLimitException( + string $message, + ?string $repository, + ?\DateTimeImmutable $resetAt, + ): RateLimitException; + + /** + * Detects a rate limit response. + * + * Rate limiting is reported inconsistently: HTTP 429, or HTTP 403 with an exhausted + * `x-ratelimit-remaining` header, or HTTP 403 with a message about primary/secondary limits. + */ + protected function isRateLimited(int $status, ResponseInterface $response, ?string $apiMessage): bool + { + if ($status === 429) { + return true; + } + + if ($status !== 403) { + return false; + } + + return $response->getHeaderLine('x-ratelimit-remaining') === '0' + || $response->getHeaderLine('ratelimit-remaining') === '0' + || ($apiMessage !== null && \str_contains(\strtolower($apiMessage), 'rate limit')); + } + + /** + * @param non-empty-string|null $repository + */ + protected function createRateLimitException( + ResponseInterface $response, + ?string $apiMessage, + ?string $repository, + ): RateLimitException { + $resetAt = self::resetTime($response); + $isSecondary = $apiMessage !== null && \str_contains(\strtolower($apiMessage), 'secondary rate limit'); + $authenticatedLimit = $this->authenticatedRateLimit(); + $limit = $response->getHeaderLine('x-ratelimit-limit'); + $limit === '' and $limit = (string) ($this->authenticated + ? $authenticatedLimit + : $this->anonymousRateLimit()); + + $message = \sprintf( + '%s API %srate limit exceeded%s.', + $this->providerName(), + $isSecondary ? 'secondary ' : '', + $limit === '' ? '' : \sprintf(' (limit: %s requests per hour)', $limit), + ); + + $resetAt === null or $message .= \sprintf( + ' The limit resets at %s (in %s).', + $resetAt->format('Y-m-d H:i:s T'), + self::humanizeInterval($resetAt), + ); + + $apiMessage === null or $message .= \sprintf(' API message: %s', $apiMessage); + + $message .= "\n" . ($this->authenticated + ? \sprintf( + 'The API token from the %s environment variable has spent its quota: wait for the reset or use another token.', + $this->tokenEnvVariable(), + ) + : \sprintf( + 'No API token is configured. Set the %s environment variable to raise the limit%s.', + $this->tokenEnvVariable(), + $authenticatedLimit === null + ? '' + : \sprintf(' up to %d requests per hour', $authenticatedLimit), + )); + + return $this->instantiateRateLimitException($message, $repository, $resetAt); + } + + /** + * @return non-empty-string + */ + protected function authenticationMessage(?string $apiMessage): string + { + return \sprintf( + "%s API rejected the credentials (HTTP 401%s).\n%s", + $this->providerName(), + $apiMessage === null ? '' : ': ' . $apiMessage, + $this->authenticated + ? \sprintf( + 'The API token from the %s environment variable is invalid, expired or revoked. ' + . 'Provide a valid token or unset the variable to use anonymous access.', + $this->tokenEnvVariable(), + ) + : \sprintf( + 'No API token is configured, so the request was anonymous. ' + . 'Set the %s environment variable with a valid token.', + $this->tokenEnvVariable(), + ), + ); + } + + /** + * @param non-empty-string|null $repository + * @return non-empty-string + */ + protected function accessDeniedMessage(?string $apiMessage, ?string $repository): string + { + return \sprintf( + "%s API denied access%s (HTTP 403%s).\n%s", + $this->providerName(), + $repository === null ? '' : \sprintf(' to `%s`', $repository), + $apiMessage === null ? '' : ': ' . $apiMessage, + $this->authenticated + ? \sprintf( + 'The API token from the %s environment variable has no read access to this %s. ' + . 'Use a token with read permissions for it.', + $this->tokenEnvVariable(), + $this->repositoryTerm(), + ) + : \sprintf( + 'No API token is configured. Set the %s environment variable with a token ' + . 'that has read access to this %s.', + $this->tokenEnvVariable(), + $this->repositoryTerm(), + ), + ); + } + + /** + * @param non-empty-string|null $repository + * @return non-empty-string + */ + protected function notFoundMessage(?string $apiMessage, ?string $repository, string $endpoint): string + { + return \sprintf( + "%s API returned HTTP 404 for %s%s.\n%s", + $this->providerName(), + $repository === null ? $endpoint : \sprintf('%s `%s`', $this->repositoryTerm(), $repository), + $apiMessage === null ? '' : ': ' . $apiMessage, + $this->authenticated + ? \sprintf( + 'Check the %1$s address in the configuration. If the %1$s is private, ' + . 'make sure the token from the %2$s environment variable has read access to it ' + . '(a 404 is also returned instead of 403 when access is missing).', + $this->repositoryTerm(), + $this->tokenEnvVariable(), + ) + : \sprintf( + 'Check the %1$s address in the configuration. If the %1$s is private, ' + . 'set the %2$s environment variable with a token that has read access to it.', + $this->repositoryTerm(), + $this->tokenEnvVariable(), + ), + ); + } + + /** + * Reads a human-readable message from an API error response. + */ + private static function extractApiMessage(ResponseInterface $response): ?string + { + $body = \trim($response->getBody()->__toString()); + if ($body === '') { + return null; + } + + try { + /** @var mixed $decoded */ + $decoded = \json_decode($body, true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException) { + // Not a JSON response: quote the raw body + return self::truncate($body); + } + + $message = match (true) { + \is_string($decoded) => $decoded, + \is_array($decoded) => self::messageFromArray($decoded), + default => null, + }; + + return $message === null ? null : self::truncate($message); + } + + /** + * @param array $decoded + */ + private static function messageFromArray(array $decoded): ?string + { + // GitHub: {"message": "...", "documentation_url": "..."} + // GitLab: {"message": "404 Project Not Found"} or {"error": "..."} + foreach (['message', 'error', 'error_description'] as $key) { + /** @var mixed $value */ + $value = $decoded[$key] ?? null; + + if (\is_string($value) && $value !== '') { + return $value; + } + + if (!\is_array($value)) { + continue; + } + + // GitLab may report a list of messages + $parts = []; + /** @var mixed $item */ + foreach ($value as $item) { + \is_string($item) and $parts[] = $item; + } + + if ($parts !== []) { + return \implode(' ', $parts); + } + } + + // Older GitHub responses use a plain list: ["API rate limit exceeded...", "https://docs..."] + /** @var mixed $first */ + $first = $decoded[0] ?? null; + + return \is_string($first) && $first !== '' ? $first : null; + } + + /** + * Resolves the moment when a rate limit is reset from response headers. + */ + private static function resetTime(ResponseInterface $response): ?\DateTimeImmutable + { + $reset = $response->getHeaderLine('x-ratelimit-reset'); + if (\preg_match('/^\d+$/', $reset) === 1) { + return (new \DateTimeImmutable('@' . $reset))->setTimezone(new \DateTimeZone(\date_default_timezone_get())); + } + + $retryAfter = $response->getHeaderLine('retry-after'); + if (\preg_match('/^\d+$/', $retryAfter) === 1) { + return new \DateTimeImmutable(\sprintf('+%d seconds', (int) $retryAfter)); + } + + return null; + } + + /** + * @return non-empty-string Time left until the given moment, e.g. `42 min 5 sec`. + */ + private static function humanizeInterval(\DateTimeImmutable $until): string + { + $seconds = $until->getTimestamp() - \time(); + if ($seconds <= 0) { + return 'a moment'; + } + + $minutes = \intdiv($seconds, 60); + return $minutes === 0 + ? \sprintf('%d sec', $seconds) + : \sprintf('%d min %d sec', $minutes, $seconds % 60); + } + + private static function truncate(string $message): string + { + $message = \trim(\preg_replace('/\s+/', ' ', $message) ?? $message); + + return \strlen($message) > self::MESSAGE_MAX_LENGTH + ? \substr($message, 0, self::MESSAGE_MAX_LENGTH) . '…' + : $message; + } +} diff --git a/src/Module/Task/Manager.php b/src/Module/Task/Manager.php index 2be8c98..a73434b 100644 --- a/src/Module/Task/Manager.php +++ b/src/Module/Task/Manager.php @@ -93,8 +93,8 @@ public function getProcessor(): \Generator yield $task->resume(); } catch (\Throwable $e) { - $this->logger->error($e->getMessage()); - $this->logger->exception($e); + # The failure is delivered via the promise, so the caller decides how to report it + $this->logger->exception($e, important: false); unset($this->tasks[$key]); $deferred->reject($e); yield $e; diff --git a/tests/Unit/Module/Downloader/Exception/NothingExtractedTest.php b/tests/Unit/Module/Downloader/Exception/NothingExtractedTest.php new file mode 100644 index 0000000..448399c --- /dev/null +++ b/tests/Unit/Module/Downloader/Exception/NothingExtractedTest.php @@ -0,0 +1,50 @@ +getMessage(); + + // Assert + self::assertStringContainsString( + 'Nothing was extracted from `roadrunner-2025.1.15-windows-amd64.zip`: ' + . 'none of the 3 file(s) inside matches the extraction rules.', + $message, + ); + self::assertStringContainsString('Extraction rules: binary `/^roadrunner-.*/`', $message); + self::assertStringContainsString('Files in the asset: CHANGELOG.md, LICENSE, rr.exe', $message); + } + + public function testMessageTruncatesLongFileList(): void + { + // Arrange + $files = \array_map(static fn(int $i): string => "file-{$i}.txt", \range(1, 25)); + + // Act + $message = (new NothingExtracted('archive.tar.gz', [], $files))->getMessage(); + + // Assert + self::assertStringContainsString('Extraction rules: none', $message); + self::assertStringContainsString('file-20.txt', $message); + self::assertStringNotContainsString('file-21.txt', $message); + self::assertStringContainsString('and 5 more', $message); + } +} diff --git a/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php b/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php new file mode 100644 index 0000000..0f1c8c5 --- /dev/null +++ b/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php @@ -0,0 +1,161 @@ +render(); + + // Assert + self::assertStringContainsString( + 'Requested: version `^2.0`, OS `linux`, architecture `amd64`, ' + . 'minimum stability `stable`, asset type `binary`.', + $report, + ); + } + + public function testReportMentionsMissingRepositoryConfiguration(): void + { + // Arrange + $diagnostics = self::diagnostics(); + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('No repositories are configured for `app`', $report); + } + + public function testReportListsAvailableReleasesWhenNothingMatches(): void + { + // Arrange + $diagnostics = self::diagnostics(version: '^5.0'); + $repository = $diagnostics->addRepository('github', 'owner/repo', '/^app-.*/'); + $repository->matchedReleases = 0; + $repository->registerFetchedReleases(['v1.2.0', 'v1.1.0']); + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('Tried 1 repository(ies):', $report); + self::assertStringContainsString('1) github `owner/repo`', $report); + self::assertStringContainsString('0 release(s) match the requested version and stability.', $report); + self::assertStringContainsString('Releases available in the repository: v1.2.0, v1.1.0', $report); + } + + public function testReportListsCheckedReleasesWithTheirAssets(): void + { + // Arrange + $diagnostics = self::diagnostics(); + $repository = $diagnostics->addRepository('github', 'owner/repo', '/^app-.*/'); + $repository->matchedReleases = 2; + + $release = $repository->addRelease('v1.2.0'); + $release->registerAssets(['app-1.2.0-darwin-arm64.tar.gz', 'app-1.2.0-windows-amd64.zip']); + $release->reason = 'no asset matches OS `linux`, architecture `amd64`, name pattern `/^app-.*/`'; + + $failed = $repository->addRelease('v1.1.0'); + $failed->registerAssets(['app-1.1.0-linux-amd64.tar.gz']); + $failed->addFailure('app-1.1.0-linux-amd64.tar.gz', new \RuntimeException('Broken archive')); + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('2 release(s) match the requested version and stability.', $report); + self::assertStringContainsString('Checked releases:', $report); + self::assertStringContainsString('- v1.2.0: 2 asset(s), no asset matches OS `linux`', $report); + self::assertStringContainsString('Assets: app-1.2.0-darwin-arm64.tar.gz, app-1.2.0-windows-amd64.zip', $report); + self::assertStringContainsString( + 'Failed asset `app-1.1.0-linux-amd64.tar.gz`: Broken archive', + $report, + ); + } + + public function testReportContainsRepositoryLevelError(): void + { + // Arrange + $diagnostics = self::diagnostics(); + $repository = $diagnostics->addRepository('github', 'owner/repo', '/^app-.*/'); + $repository->error = new ApiException( + "GitHub API rate limit exceeded.\nSet the GITHUB_TOKEN environment variable.", + 'owner/repo', + ); + + $fallback = $diagnostics->addRepository('gitlab', 'group/app', '/^app-.*/'); + $fallback->matchedReleases = 0; + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('Tried 2 repository(ies):', $report); + self::assertStringContainsString('GitHub API rate limit exceeded.', $report); + self::assertStringContainsString('Set the GITHUB_TOKEN environment variable.', $report); + self::assertStringContainsString('2) gitlab `group/app`', $report); + } + + public function testReportLimitsTheNumberOfDescribedReleases(): void + { + // Arrange + $diagnostics = self::diagnostics(); + $repository = $diagnostics->addRepository('github', 'owner/repo', '/^app-.*/'); + $repository->matchedReleases = 8; + + for ($i = 8; $i > 0; --$i) { + $repository->addRelease("v1.0.{$i}")->registerAssets(["app-1.0.{$i}-linux-amd64.tar.gz"]); + } + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('- v1.0.8:', $report); + self::assertStringContainsString('- v1.0.6:', $report); + self::assertStringNotContainsString('- v1.0.5:', $report); + self::assertStringContainsString('and 5 more release(s)', $report); + } + + /** + * @param non-empty-string|null $version + */ + private static function diagnostics(?string $version = null, ?Type $type = null): DownloadDiagnostics + { + $software = Software::fromArray(['name' => 'App', 'alias' => 'app']); + + $config = DownloadConfig::fromSoftwareId('app'); + $config->version = $version; + $config->type = $type; + + return new DownloadDiagnostics( + software: $software, + actionConfig: $config, + operatingSystem: OperatingSystem::Linux, + architecture: Architecture::X86_64, + stability: Stability::Stable, + ); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Api/ClientTest.php b/tests/Unit/Module/Repository/Internal/GitHub/Api/ClientTest.php index aee24cc..5ff50d2 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Api/ClientTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Api/ClientTest.php @@ -5,12 +5,16 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Api; use Internal\DLoad\Module\Config\Schema\GitHub; +use Internal\DLoad\Module\Repository\Exception\AccessDeniedException; +use Internal\DLoad\Module\Repository\Exception\ApiException; +use Internal\DLoad\Module\Repository\Exception\AuthenticationException; +use Internal\DLoad\Module\Repository\Exception\RateLimitException; +use Internal\DLoad\Module\Repository\Exception\RepositoryNotFoundException; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\Client; -use Internal\DLoad\Module\Repository\Internal\GitHub\Exception\GitHubRateLimitException; use Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub\ClientStub; use Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub\GitHubConfigStub; use Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub\HttpFactoryStub; -use Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub\ResponseStub; +use Internal\DLoad\Tests\Unit\Module\Repository\Stub\ResponseStub; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -33,51 +37,96 @@ public static function provideRequestHeaders(): \Generator yield 'override default headers' => [['accept' => 'application/json']]; } - public static function provideRateLimitScenarios(): \Generator + /** + * @return \Generator, class-string<\Throwable>|null}> + */ + public static function provideErrorScenarios(): \Generator { - yield 'valid rate limit response' => [ + yield 'legacy rate limit response' => [ 403, \json_encode([ - 'API rate limit exceeded for user ID 1234. Check the hourly limit for your plan at https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting', + 'API rate limit exceeded for user ID 1234.', 'https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting', ]), - true, + [], + RateLimitException::class, ]; - yield 'non-403 status code' => [ + yield 'rate limit reported with 429' => [ 429, - \json_encode(['API rate limit exceeded', 'https://docs.github.com']), - false, + \json_encode(['message' => 'API rate limit exceeded', 'documentation_url' => 'https://docs.github.com']), + [], + RateLimitException::class, ]; - yield '403 with invalid JSON' => [ + yield 'rate limit detected by header' => [ 403, - 'invalid json response', - false, + \json_encode(['message' => 'Request forbidden', 'documentation_url' => 'https://docs.github.com']), + ['x-ratelimit-remaining' => ['0']], + RateLimitException::class, ]; - yield '403 with wrong array structure' => [ + yield 'secondary rate limit' => [ 403, - \json_encode(['message' => 'Forbidden']), - false, + \json_encode(['message' => 'You have exceeded a secondary rate limit.']), + [], + RateLimitException::class, ]; - yield '403 with wrong array count' => [ - 403, - \json_encode(['API rate limit exceeded']), - false, + yield 'invalid token' => [ + 401, + \json_encode(['message' => 'Bad credentials']), + [], + AuthenticationException::class, ]; - yield '403 with non-string elements' => [ + yield 'forbidden without rate limit' => [ 403, - \json_encode([123, 456]), - false, + \json_encode(['message' => 'Resource not accessible by integration']), + [], + AccessDeniedException::class, ]; - yield '403 without rate limit text' => [ + yield 'non-JSON forbidden body' => [ 403, - \json_encode(['Something else', 'https://docs.github.com']), - false, + 'invalid json response', + [], + AccessDeniedException::class, + ]; + + yield 'missing repository' => [ + 404, + \json_encode(['message' => 'Not Found']), + [], + RepositoryNotFoundException::class, + ]; + + yield 'server error' => [ + 502, + 'Bad Gateway', + [], + ApiException::class, + ]; + + yield 'unprocessable entity' => [ + 422, + \json_encode(['message' => 'Validation Failed']), + [], + ApiException::class, + ]; + + yield 'successful response' => [ + 200, + '[]', + [], + null, + ]; + + yield 'redirect is not an error' => [ + 302, + '', + [], + null, ]; } @@ -151,68 +200,49 @@ public function testDetectsRateLimitResponseAndThrowsException(): void $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); // Assert (before Act for exceptions) - $this->expectException(GitHubRateLimitException::class); - $this->expectExceptionMessage('API rate limit exceeded for user ID 1234'); + $this->expectException(RateLimitException::class); + $this->expectExceptionMessage('rate limit exceeded'); // Act $this->client->request($method, $uri); } - public function testDoesNotThrowExceptionForNon403Response(): void + public function testRateLimitMessageSuggestsTokenWhenThereIsNoToken(): void { // Arrange - $method = 'GET'; - $uri = $this->createMock(UriInterface::class); $request = $this->createMock(RequestInterface::class); - $response = ResponseStub::ok(); - - $this->httpFactory = $this->httpFactory->withRequest($method, $uri, $request); - $this->httpClient = $this->httpClient->withResponse($request, $response); - $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); - - // Act - $result = $this->client->request($method, $uri); - - // Assert - self::assertSame($response, $result); - } - - public function testDoesNotThrowExceptionForNonRateLimitError(): void - { - // Arrange - $method = 'GET'; - $uri = $this->createMock(UriInterface::class); - $request = $this->createMock(RequestInterface::class); - $forbiddenResponse = ResponseStub::githubForbidden(); - - $this->httpFactory = $this->httpFactory->withRequest($method, $uri, $request); - $this->httpClient = $this->httpClient->withResponse($request, $forbiddenResponse); + $this->httpClient = $this->httpClient->withResponse($request, ResponseStub::githubRateLimit()); $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); // Act - $result = $this->client->request($method, $uri); - - // Assert - self::assertSame($forbiddenResponse, $result); + try { + $this->client->sendRequest($request); + self::fail('RateLimitException is expected.'); + } catch (RateLimitException $e) { + // Assert + self::assertStringContainsString('GITHUB_TOKEN', $e->getMessage()); + self::assertStringContainsString('No API token is configured', $e->getMessage()); + } } - public function testDoesNotThrowExceptionForInvalidJsonResponse(): void + public function testAuthenticationMessageMentionsConfiguredToken(): void { // Arrange - $method = 'GET'; - $uri = $this->createMock(UriInterface::class); $request = $this->createMock(RequestInterface::class); - $invalidJsonResponse = ResponseStub::invalidJson(); + $response = new ResponseStub(401, [], \json_encode(['message' => 'Bad credentials'])); - $this->httpFactory = $this->httpFactory->withRequest($method, $uri, $request); - $this->httpClient = $this->httpClient->withResponse($request, $invalidJsonResponse); - $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); + $this->httpClient = $this->httpClient->withResponse($request, $response); + $client = new Client($this->httpFactory, $this->httpClient, GitHubConfigStub::withToken('invalid-token')); // Act - $result = $this->client->request($method, $uri); - - // Assert - self::assertSame($invalidJsonResponse, $result); + try { + $client->sendRequest($request); + self::fail('AuthenticationException is expected.'); + } catch (AuthenticationException $e) { + // Assert + self::assertStringContainsString('Bad credentials', $e->getMessage()); + self::assertStringContainsString('invalid, expired or revoked', $e->getMessage()); + } } public function testSendRequestDelegatesToHttpClient(): void @@ -231,23 +261,7 @@ public function testSendRequestDelegatesToHttpClient(): void self::assertSame($response, $result); } - public function testSendRequestThrowsRateLimitExceptionOn403WithRateLimitJson(): void - { - // Arrange - $request = $this->createMock(RequestInterface::class); - $rateLimitResponse = ResponseStub::githubRateLimit(); - - $this->httpClient = $this->httpClient->withResponse($request, $rateLimitResponse); - $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); - - // Assert (before Act for exceptions) - $this->expectException(GitHubRateLimitException::class); - - // Act - $this->client->sendRequest($request); - } - - public function testSendRequestPropagatesClientExceptions(): void + public function testSendRequestWrapsClientExceptionsIntoApiException(): void { // Arrange $request = $this->createMock(RequestInterface::class); @@ -256,11 +270,15 @@ public function testSendRequestPropagatesClientExceptions(): void $this->httpClient = $this->httpClient->withException($request, $clientException); $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); - // Assert (before Act for exceptions) - $this->expectException(ClientExceptionInterface::class); - // Act - $this->client->sendRequest($request); + try { + $this->client->sendRequest($request); + self::fail('ApiException is expected.'); + } catch (ApiException $e) { + // Assert + self::assertStringContainsString('Failed to reach GitHub API', $e->getMessage()); + self::assertSame($clientException, $e->getPrevious()); + } } #[DataProvider('provideRequestHeaders')] @@ -283,29 +301,31 @@ public function testRequestMergesHeadersCorrectly(array $additionalHeaders): voi self::assertSame($response, $result); } - #[DataProvider('provideRateLimitScenarios')] - public function testRateLimitDetectionScenarios( + /** + * @param array $headers + * @param class-string<\Throwable>|null $expectedException + */ + #[DataProvider('provideErrorScenarios')] + public function testUnsuccessfulResponsesAreConvertedIntoExceptions( int $statusCode, string $responseBody, - bool $shouldThrowException, + array $headers, + ?string $expectedException, ): void { // Arrange $request = $this->createMock(RequestInterface::class); - $response = new ResponseStub($statusCode, [], $responseBody); + $response = new ResponseStub($statusCode, $headers, $responseBody); $this->httpClient = $this->httpClient->withResponse($request, $response); $this->client = new Client($this->httpFactory, $this->httpClient, $this->gitHubConfig); - if ($shouldThrowException) { - $this->expectException(GitHubRateLimitException::class); - } + $expectedException === null or $this->expectException($expectedException); - // Act & Assert + // Act $result = $this->client->sendRequest($request); - if (!$shouldThrowException) { - self::assertSame($response, $result); - } + // Assert + self::assertSame($response, $result); } protected function setUp(): void diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php b/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php new file mode 100644 index 0000000..c33459d --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php @@ -0,0 +1,180 @@ +validate(self::releasesRequest(), ResponseStub::ok('[]')); + + // Assert + self::assertTrue(true, 'Successful responses must not throw.'); + } + + public function testRateLimitWithoutTokenExplainsAnonymousLimit(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $response = new ResponseStub( + 403, + ['x-ratelimit-remaining' => ['0'], 'x-ratelimit-limit' => ['60']], + \json_encode(['message' => 'API rate limit exceeded for 1.2.3.4.']), + ); + + // Assert (before Act for exceptions) + $this->expectException(RateLimitException::class); + $this->expectExceptionMessage('60 requests per hour'); + + // Act + $validator->validate(self::releasesRequest(), $response); + } + + public function testRateLimitWithTokenReportsSpentQuotaAndResetTime(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: true); + $resetsAt = \time() + 600; + $response = new ResponseStub( + 429, + ['x-ratelimit-remaining' => ['0'], 'x-ratelimit-reset' => [(string) $resetsAt]], + \json_encode(['message' => 'API rate limit exceeded']), + ); + + // Act + try { + $validator->validate(self::releasesRequest(), $response); + self::fail('RateLimitException is expected.'); + } catch (RateLimitException $e) { + // Assert + self::assertStringContainsString('spent its quota', $e->getMessage()); + self::assertStringContainsString('GITHUB_TOKEN', $e->getMessage()); + self::assertNotNull($e->resetAt); + self::assertSame($resetsAt, $e->resetAt->getTimestamp()); + } + } + + public function testSecondaryRateLimitIsRecognized(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: true); + $response = new ResponseStub( + 403, + ['retry-after' => ['60']], + \json_encode(['message' => 'You have exceeded a secondary rate limit. Please wait a few minutes.']), + ); + + // Assert (before Act for exceptions) + $this->expectException(RateLimitException::class); + $this->expectExceptionMessage('secondary rate limit exceeded'); + + // Act + $validator->validate(self::releasesRequest(), $response); + } + + public function testForbiddenResponseMentionsRepositoryAndToken(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: true); + $response = new ResponseStub(403, [], \json_encode(['message' => 'Resource not accessible by integration'])); + + // Act + try { + $validator->validate(self::releasesRequest(), $response); + self::fail('AccessDeniedException is expected.'); + } catch (AccessDeniedException $e) { + // Assert + self::assertSame('owner/repo', $e->repository); + self::assertStringContainsString('Resource not accessible by integration', $e->getMessage()); + self::assertStringContainsString('no read access to this repository', $e->getMessage()); + } + } + + public function testNotFoundResponseSuggestsCheckingRepositoryAddress(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $response = new ResponseStub(404, [], \json_encode(['message' => 'Not Found'])); + + // Act + try { + $validator->validate(self::releasesRequest(), $response); + self::fail('RepositoryNotFoundException is expected.'); + } catch (RepositoryNotFoundException $e) { + // Assert + self::assertStringContainsString('repository `owner/repo`', $e->getMessage()); + self::assertStringContainsString('GITHUB_TOKEN', $e->getMessage()); + } + } + + public function testServerErrorIsReportedAsTemporaryFailure(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $response = new ResponseStub(503, [], 'Service Unavailable', 'Service Unavailable'); + + // Assert (before Act for exceptions) + $this->expectException(ApiException::class); + $this->expectExceptionMessage('GitHub API is unavailable: HTTP 503'); + + // Act + $validator->validate(self::releasesRequest(), $response); + } + + public function testRepositoryIsResolvedFromAssetDownloadUrl(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $request = new Request('GET', 'https://github.com/owner/repo/releases/download/v1.0.0/asset.zip'); + $response = new ResponseStub(404, [], \json_encode(['message' => 'Not Found'])); + + // Act + try { + $validator->validate($request, $response); + self::fail('RepositoryNotFoundException is expected.'); + } catch (RepositoryNotFoundException $e) { + // Assert + self::assertSame('owner/repo', $e->repository); + } + } + + public function testTransportFailureKeepsTheOriginalError(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $original = new \RuntimeException('Could not resolve host: api.github.com'); + + // Act + $exception = $validator->transportFailure(self::releasesRequest(), $original); + + // Assert + self::assertStringContainsString('Failed to reach GitHub API', $exception->getMessage()); + self::assertStringContainsString('Could not resolve host', $exception->getMessage()); + self::assertSame($original, $exception->getPrevious()); + self::assertSame('owner/repo', $exception->repository); + } + + private static function releasesRequest(): RequestInterface + { + return new Request('GET', 'https://api.github.com/repos/owner/repo/releases?page=1'); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/ClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/ClientStub.php index 4d6aceb..0d211fb 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/ClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/ClientStub.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub; +use Internal\DLoad\Tests\Unit\Module\Repository\Stub\ResponseStub; use Psr\Http\Client\ClientExceptionInterface; use Psr\Http\Client\ClientInterface; use Psr\Http\Message\RequestInterface; diff --git a/tests/Unit/Module/Repository/Internal/GitLab/Api/ResponseValidatorTest.php b/tests/Unit/Module/Repository/Internal/GitLab/Api/ResponseValidatorTest.php new file mode 100644 index 0000000..2be6501 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitLab/Api/ResponseValidatorTest.php @@ -0,0 +1,55 @@ + '404 Project Not Found'])); + + // Act + try { + $validator->validate($request, $response); + self::fail('RepositoryNotFoundException is expected.'); + } catch (RepositoryNotFoundException $e) { + // Assert + self::assertSame('group/project', $e->repository); + self::assertStringContainsString('project `group/project`', $e->getMessage()); + self::assertStringContainsString('GITLAB_TOKEN', $e->getMessage()); + } + } + + public function testTooManyRequestsIsReportedAsRateLimit(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + $request = new Request('GET', 'https://gitlab.com/api/v4/projects/group%2Fproject/releases'); + $response = new ResponseStub(429, ['retry-after' => ['30']], ''); + + // Act + try { + $validator->validate($request, $response); + self::fail('RateLimitException is expected.'); + } catch (RateLimitException $e) { + // Assert + self::assertStringContainsString('GitLab API rate limit exceeded', $e->getMessage()); + self::assertStringContainsString('GITLAB_TOKEN', $e->getMessage()); + self::assertNotNull($e->resetAt); + } + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/ResponseStub.php b/tests/Unit/Module/Repository/Stub/ResponseStub.php similarity index 97% rename from tests/Unit/Module/Repository/Internal/GitHub/Stub/ResponseStub.php rename to tests/Unit/Module/Repository/Stub/ResponseStub.php index 78b88c7..3da8e71 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/ResponseStub.php +++ b/tests/Unit/Module/Repository/Stub/ResponseStub.php @@ -2,13 +2,13 @@ declare(strict_types=1); -namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitHub\Stub; +namespace Internal\DLoad\Tests\Unit\Module\Repository\Stub; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\StreamInterface; /** - * HTTP Response stub for GitHub API tests. + * HTTP Response stub for repository API tests. * * Provides controllable response data for testing. */ From a9339a809b021d8579c9d5a2fe243d40446473bb Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Tue, 11 Aug 2026 14:51:16 +0400 Subject: [PATCH 2/2] fix(repository): report rate limits that interrupt release pagination fix(downloader): distinguish an unfetched asset list from an empty one in reports fix(repository): keep truncated API messages valid UTF-8 fix(get): escape the software name in the failure header A rate limit hit on a follow-up page used to stop pagination silently, so the failure report claimed the repository had nothing more to offer. GitLab pagination failures are now logged the same way as GitHub ones. Assisted-By: Claude Fable 5 --- src/Command/Get.php | 2 +- src/Module/Downloader/Downloader.php | 7 +++---- .../Internal/Diagnostics/ReleaseAttempt.php | 9 +++++---- .../Internal/GitHub/GitHubRepository.php | 5 +++++ .../Repository/Internal/GitLab/Factory.php | 4 +++- .../Internal/GitLab/GitLabRepository.php | 8 ++++++++ .../Repository/Internal/ResponseValidator.php | 15 +++++++++++--- .../Diagnostics/DownloadDiagnosticsTest.php | 18 +++++++++++++++++ .../GitHub/Api/ResponseValidatorTest.php | 20 +++++++++++++++++++ 9 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/Command/Get.php b/src/Command/Get.php index 1c7e24a..7653508 100644 --- a/src/Command/Get.php +++ b/src/Command/Get.php @@ -244,7 +244,7 @@ private function reportFailures(OutputInterface $output, array $failures, int $t { foreach ($failures as [$software, $error]) { $output->writeln(''); - $output->writeln(\sprintf(' Failed to download `%s` ', $software)); + $output->writeln(\sprintf(' Failed to download `%s` ', OutputFormatter::escape($software))); $output->writeln(OutputFormatter::escape(self::describeFailure($error))); $output->isVeryVerbose() and $this->logger->exception($error, important: true); diff --git a/src/Module/Downloader/Downloader.php b/src/Module/Downloader/Downloader.php index 09d2274..2b88e49 100644 --- a/src/Module/Downloader/Downloader.php +++ b/src/Module/Downloader/Downloader.php @@ -10,7 +10,6 @@ use Internal\DLoad\Module\Common\FileSystem\FS; use Internal\DLoad\Module\Common\OperatingSystem; use Internal\DLoad\Module\Common\Stability; -use Internal\DLoad\Module\Config\Schema\Action\Download; use Internal\DLoad\Module\Config\Schema\Action\Download as DownloadConfig; use Internal\DLoad\Module\Config\Schema\Action\Type; use Internal\DLoad\Module\Config\Schema\Downloader as DownloaderConfig; @@ -431,7 +430,7 @@ private function fetchReleaseNames(ReleasesCollection $releases): array /** * Describes the asset format restriction for failure reports. */ - private function describeFormatFilter(Download $actionOptions): string + private function describeFormatFilter(DownloadConfig $actionOptions): string { return match ($actionOptions->type) { Type::Phar => ' and the `phar` extension', @@ -538,10 +537,10 @@ private function getTempDirectory(): Path * Adds format filter to the assets collection if specified in action options. * * @param AssetsCollection $collection Collection of assets to filter - * @param Download $actionOptions Download action options + * @param DownloadConfig $actionOptions Download action options * @return AssetsCollection Filtered collection */ - private function addFormatFilter(AssetsCollection $collection, Download $actionOptions): AssetsCollection + private function addFormatFilter(AssetsCollection $collection, DownloadConfig $actionOptions): AssetsCollection { return match ($actionOptions->type) { Type::Phar => $collection->whereFileExtensions(['phar']), diff --git a/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php b/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php index 0be6e8d..ec0c57e 100644 --- a/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php +++ b/src/Module/Downloader/Internal/Diagnostics/ReleaseAttempt.php @@ -14,8 +14,8 @@ final class ReleaseAttempt /** Maximum number of asset names listed in the report. */ private const ASSETS_LIMIT = 15; - /** @var int<0, max> Total number of assets in the release */ - public int $assetsTotal = 0; + /** @var int<0, max>|null Total number of assets in the release, or null when the list was not fetched */ + public ?int $assetsTotal = null; /** @var string|null Why the release was rejected */ public ?string $reason = null; @@ -61,9 +61,10 @@ public function describe(): array { $lines = [ \sprintf( - '%s: %d asset(s)%s', + '%s: %s%s', $this->name, - $this->assetsTotal, + // A missing asset list (an error before it was fetched) is not the same as an empty one + $this->assetsTotal === null ? 'asset list not loaded' : \sprintf('%d asset(s)', $this->assetsTotal), $this->reason === null ? '' : ', ' . $this->reason, ), ]; diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index f24b5e0..2c4d9be 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -6,6 +6,7 @@ use Internal\Destroy\Destroyable; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; +use Internal\DLoad\Module\Repository\Exception\RateLimitException; use Internal\DLoad\Module\Repository\Internal\GitHub\Api\RepositoryApi; use Internal\DLoad\Module\Repository\Repository; use Internal\DLoad\Service\Logger; @@ -83,6 +84,10 @@ public function getReleases(): ReleasesCollection # (invalid token, rate limit, missing repository, etc.) must reach the user. $anyPageLoaded or throw $e; + # A rate limit leaves the release list incomplete: hiding it would produce a report + # that claims the repository has nothing more, so it must reach the user as well. + $e instanceof RateLimitException and throw $e; + # Already loaded releases are enough to continue, so a failure of a subsequent page # only stops the pagination. $this->logger->exception($e, important: false); diff --git a/src/Module/Repository/Internal/GitLab/Factory.php b/src/Module/Repository/Internal/GitLab/Factory.php index 0cda2e8..e34ffe8 100644 --- a/src/Module/Repository/Internal/GitLab/Factory.php +++ b/src/Module/Repository/Internal/GitLab/Factory.php @@ -10,6 +10,7 @@ use Internal\DLoad\Module\Repository\Internal\GitLab\Api\Client; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; use Internal\DLoad\Module\Repository\RepositoryFactory; +use Internal\DLoad\Service\Logger; /** * Factory for creating GitLab repository instances. @@ -28,6 +29,7 @@ final class Factory implements RepositoryFactory public function __construct( private readonly HttpFactory $httpFactory, GitLab $gitLabConfig, + private readonly Logger $logger, ) { $this->gitLabClient = new Client( $httpFactory, @@ -46,7 +48,7 @@ public function create(RepositoryConfig $config): GitLabRepository $uri = \parse_url($config->uri, PHP_URL_PATH) ?? $config->uri; $api = $this->createRepositoryApi($uri); - return new GitLabRepository($api, $uri); + return new GitLabRepository($api, $uri, $this->logger); } /** diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index dffa7bc..94f3dff 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -6,8 +6,10 @@ use Internal\Destroy\Destroyable; use Internal\DLoad\Module\Repository\Collection\ReleasesCollection; +use Internal\DLoad\Module\Repository\Exception\RateLimitException; use Internal\DLoad\Module\Repository\Internal\GitLab\Api\RepositoryApi; use Internal\DLoad\Module\Repository\Repository; +use Internal\DLoad\Service\Logger; /** * GitLab Repository class representing a GitLab repository. @@ -32,6 +34,7 @@ final class GitLabRepository implements Repository, Destroyable public function __construct( private readonly RepositoryApi $api, string $projectPath, + private readonly Logger $logger, ) { $this->name = $projectPath; } @@ -78,8 +81,13 @@ public function getReleases(): ReleasesCollection # (invalid token, rate limit, missing project, etc.) must reach the user. $anyPageLoaded or throw $e; + # A rate limit leaves the release list incomplete: hiding it would produce a report + # that claims the project has nothing more, so it must reach the user as well. + $e instanceof RateLimitException and throw $e; + # Already loaded releases are enough to continue, so a failure of a subsequent page # only stops the pagination. + $this->logger->exception($e, important: false); return; } } while ($hasMorePages); diff --git a/src/Module/Repository/Internal/ResponseValidator.php b/src/Module/Repository/Internal/ResponseValidator.php index 6e893c9..608a089 100644 --- a/src/Module/Repository/Internal/ResponseValidator.php +++ b/src/Module/Repository/Internal/ResponseValidator.php @@ -406,8 +406,17 @@ private static function truncate(string $message): string { $message = \trim(\preg_replace('/\s+/', ' ', $message) ?? $message); - return \strlen($message) > self::MESSAGE_MAX_LENGTH - ? \substr($message, 0, self::MESSAGE_MAX_LENGTH) . '…' - : $message; + if (\strlen($message) <= self::MESSAGE_MAX_LENGTH) { + return $message; + } + + $cut = \substr($message, 0, self::MESSAGE_MAX_LENGTH); + + // A byte-based cut may split a multibyte UTF-8 character: drop its leftover bytes + for ($i = 0; $i < 3 && $cut !== '' && \preg_match('//u', $cut) !== 1; ++$i) { + $cut = \substr($cut, 0, -1); + } + + return $cut . '…'; } } diff --git a/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php b/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php index 0f1c8c5..174229d 100644 --- a/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php +++ b/tests/Unit/Module/Downloader/Internal/Diagnostics/DownloadDiagnosticsTest.php @@ -118,6 +118,24 @@ public function testReportContainsRepositoryLevelError(): void self::assertStringContainsString('2) gitlab `group/app`', $report); } + public function testReleaseWithoutFetchedAssetListIsNotReportedAsEmpty(): void + { + // Arrange + $diagnostics = self::diagnostics(); + $repository = $diagnostics->addRepository('github', 'owner/repo', '/^app-.*/'); + $repository->matchedReleases = 1; + + // An error interrupted the attempt before the asset list was fetched + $repository->addRelease('v1.2.0')->reason = 'GitHub API is unavailable: HTTP 502 Bad Gateway'; + + // Act + $report = $diagnostics->render(); + + // Assert + self::assertStringContainsString('- v1.2.0: asset list not loaded, GitHub API is unavailable', $report); + self::assertStringNotContainsString('0 asset(s)', $report); + } + public function testReportLimitsTheNumberOfDescribedReleases(): void { // Arrange diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php b/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php index c33459d..d095510 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Api/ResponseValidatorTest.php @@ -173,6 +173,26 @@ public function testTransportFailureKeepsTheOriginalError(): void self::assertSame('owner/repo', $exception->repository); } + public function testLongApiMessageIsTruncatedWithoutBreakingUtf8(): void + { + // Arrange + $validator = new ResponseValidator(authenticated: false); + // An ASCII prefix shifts the byte-based cut into the middle of a multibyte character + $apiMessage = 'x' . \str_repeat('я', 400); + $response = new ResponseStub(422, [], \json_encode(['message' => $apiMessage])); + + // Act + try { + $validator->validate(self::releasesRequest(), $response); + self::fail('ApiException is expected.'); + } catch (ApiException $e) { + // Assert + $message = $e->getMessage(); + self::assertSame(1, \preg_match('//u', $message), 'The message must stay valid UTF-8.'); + self::assertStringContainsString('…', $message); + } + } + private static function releasesRequest(): RequestInterface { return new Request('GET', 'https://api.github.com/repos/owner/repo/releases?page=1');