From 46978cb8201b50d25be92dda5d9bc9e35146b683 Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Sat, 12 Sep 2026 19:30:14 +0400 Subject: [PATCH 1/3] fix: request every releases page once and cache release listings The page loader built a new paginator per page, so every page but the first was requested twice. Ask the existing paginator for the next page instead. Releases are now requested 100 per page, and listings can be cached in a directory between runs through the cache-dir and cache-ttl settings. --- README.md | 48 +++++ dload.xsd | 10 + src/Bootstrap.php | 15 ++ .../Cache/Internal/FileResponseCache.php | 158 ++++++++++++++++ .../Cache/Internal/NullResponseCache.php | 25 +++ src/Module/Cache/ResponseCache.php | 26 +++ src/Module/Config/Schema/Cache.php | 32 ++++ .../Internal/GitHub/Api/RepositoryApi.php | 21 +- .../Repository/Internal/GitHub/Factory.php | 11 +- .../Internal/GitHub/GitHubRepository.php | 19 +- .../Internal/GitLab/Api/RepositoryApi.php | 21 +- .../Repository/Internal/GitLab/Factory.php | 4 +- .../Internal/GitLab/GitLabRepository.php | 19 +- .../Module/Cache/ResponseCacheBindingTest.php | 73 +++++++ .../Module/Cache/FileResponseCacheTest.php | 179 ++++++++++++++++++ .../Internal/GitHub/GitHubRepositoryTest.php | 124 ++++++++++++ .../Internal/GitHub/Stub/PagedClientStub.php | 94 +++++++++ .../Internal/GitLab/FactoryTest.php | 8 +- .../Internal/GitLab/GitLabRepositoryTest.php | 58 ++++++ .../Internal/GitLab/Stub/PagedClientStub.php | 95 ++++++++++ 20 files changed, 1009 insertions(+), 31 deletions(-) create mode 100644 src/Module/Cache/Internal/FileResponseCache.php create mode 100644 src/Module/Cache/Internal/NullResponseCache.php create mode 100644 src/Module/Cache/ResponseCache.php create mode 100644 src/Module/Config/Schema/Cache.php create mode 100644 tests/Integration/Module/Cache/ResponseCacheBindingTest.php create mode 100644 tests/Unit/Module/Cache/FileResponseCacheTest.php create mode 100644 tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php create mode 100644 tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php create mode 100644 tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php create mode 100644 tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php diff --git a/README.md b/README.md index 628c879..03a250c 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ With DLoad, you can: - [Download Types](#download-types) - [Version Constraints](#version-constraints) - [Advanced Configuration Options](#advanced-configuration-options) + - [Caching Release Lists](#caching-release-lists) - [Building Custom RoadRunner](#building-custom-roadrunner) - [Build Action Configuration](#build-action-configuration) - [Velox Action Attributes](#velox-action-attributes) @@ -350,6 +351,50 @@ Use Composer-style version constraints: ``` +### Caching Release Lists + +Resolving a version means asking GitHub or GitLab for the repository's release list. DLoad can keep +those listings in a directory and reuse them, so repeated runs resolve the same versions without +spending the API rate limit: + +```xml + + + + + +``` + +| Attribute | Environment variable | Default | Meaning | +|-------------|----------------------|---------|---------------------------------------------------------------| +| `cache-dir` | `DLOAD_CACHE_DIR` | not set | Directory to store cached release listings in. Caching is off until it is set. | +| `cache-ttl` | `DLOAD_CACHE_TTL` | `600` | Seconds a cached listing stays usable. `0` disables caching. | + +> [!NOTE] +> Only successful release listings are cached. Failed requests are never stored, so a rate limit +> answer is not replayed after the limit is gone, and asset downloads do not go through the cache: +> the directory holds listings only, never the downloaded binaries. + +In GitHub Actions the directory can be carried between jobs, so only the first job of a workflow run +spends any rate limit on listings: + +```yaml +- name: Cache DLoad release lists + uses: actions/cache@v4 + with: + path: ./runtime/dload-cache + key: dload-cache-${{ github.run_id }} + restore-keys: dload-cache- + +- run: ./vendor/bin/dload get + env: + DLOAD_CACHE_DIR: ./runtime/dload-cache +``` + +The `github.run_id` in the key makes every workflow run write a fresh entry, while `restore-keys` +lets the remaining jobs of that run restore it. A static key would never be written again and the +cached listings would stay stale forever. + ## Building Custom RoadRunner DLoad supports building custom RoadRunner binaries using the Velox build tool. This is useful when you need RoadRunner with custom plugin combinations that aren't available in pre-built releases. @@ -600,6 +645,9 @@ Add to CI/CD environment variables for automated downloads. > 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. +Release listings can also be cached between runs, which removes them from the rate limit budget +entirely: see [Caching Release Lists](#caching-release-lists). + ## Failure Reporting `dload get` exits with a non-zero code when at least one requested package was not installed, and prints diff --git a/dload.xsd b/dload.xsd index 793255e..5fbf7ae 100644 --- a/dload.xsd +++ b/dload.xsd @@ -249,6 +249,16 @@ Temporary directory for downloads + + + Directory to cache release listings in; caching is disabled when not set + + + + + Number of seconds a cached release listing stays usable; 0 disables caching + + diff --git a/src/Bootstrap.php b/src/Bootstrap.php index 887153a..a795003 100644 --- a/src/Bootstrap.php +++ b/src/Bootstrap.php @@ -8,10 +8,14 @@ use Internal\Container\ObjectContainer; use Internal\DLoad\Module\Binary\BinaryProvider; use Internal\DLoad\Module\Binary\Internal\BinaryProviderImpl; +use Internal\DLoad\Module\Cache\Internal\FileResponseCache; +use Internal\DLoad\Module\Cache\Internal\NullResponseCache; +use Internal\DLoad\Module\Cache\ResponseCache; use Internal\DLoad\Module\Common\Architecture; use Internal\DLoad\Module\Common\Internal\Injection\ConfigInflector; use Internal\DLoad\Module\Common\OperatingSystem; use Internal\DLoad\Module\Common\Stability; +use Internal\DLoad\Module\Config\Schema\Cache as CacheConfig; use Internal\DLoad\Module\HttpClient\Factory; use Internal\DLoad\Module\HttpClient\Internal\NyholmFactoryImpl; use Internal\DLoad\Module\Repository\Internal\GitHub\Factory as GithubRepositoryFactory; @@ -21,6 +25,7 @@ use Internal\DLoad\Module\Velox\Builder; use Internal\DLoad\Module\Velox\Internal\Client\BuildRoadRunner; use Internal\DLoad\Module\Velox\Internal\VeloxBuilder; +use Internal\DLoad\Service\Logger; /** * Bootstraps the application by configuring the dependency container. @@ -113,6 +118,16 @@ public function withConfig( ->addRepositoryFactory($container->get(GithubRepositoryFactory::class)) ->addRepositoryFactory($container->get(GitLabRepositoryFactory::class)), ); + $this->container->bind( + ResponseCache::class, + static function (Container $container): ResponseCache { + $config = $container->get(CacheConfig::class); + + return $config->dir === null || $config->ttl <= 0 + ? new NullResponseCache() + : new FileResponseCache($config->dir, $config->ttl, $container->get(Logger::class)); + }, + ); $this->container->bind(BinaryProvider::class, BinaryProviderImpl::class); $this->container->bind(Factory::class, NyholmFactoryImpl::class); $this->container->bind(Builder::class, VeloxBuilder::class); diff --git a/src/Module/Cache/Internal/FileResponseCache.php b/src/Module/Cache/Internal/FileResponseCache.php new file mode 100644 index 0000000..3ee3314 --- /dev/null +++ b/src/Module/Cache/Internal/FileResponseCache.php @@ -0,0 +1,158 @@ + $ttl Number of seconds an entry stays usable. + */ + public function __construct( + private readonly string $directory, + private readonly int $ttl, + private readonly Logger $logger, + ) {} + + public function remember(string $key, \Closure $fetch): ResponseInterface + { + $file = $this->fileOf($key); + + $cached = $this->read($file); + if ($cached !== null) { + return $cached; + } + + $response = $fetch(); + $this->write($file, $response); + + return $response; + } + + /** + * Reads the body without consuming it for the caller. + */ + private static function readBody(ResponseInterface $response): string + { + $stream = $response->getBody(); + + $stream->isSeekable() and $stream->rewind(); + $body = $stream->getContents(); + $stream->isSeekable() and $stream->rewind(); + + return $body; + } + + /** + * Reads an entry, or returns `null` when there is none, it expired, or it cannot be used. + */ + private function read(string $file): ?ResponseInterface + { + if (!\is_file($file)) { + return null; + } + + $content = @\file_get_contents($file); + if ($content === false) { + $this->discard($file); + return null; + } + + try { + /** @var mixed $payload */ + $payload = \json_decode($content, true, 512, JSON_THROW_ON_ERROR); + + \is_array($payload) + && \is_int($payload['created_at'] ?? null) + && \is_int($payload['status'] ?? null) + && \is_array($payload['headers'] ?? null) + && \is_string($payload['body'] ?? null) + or throw new \UnexpectedValueException('Unexpected cache entry structure.'); + } catch (\Throwable) { + # A half-written or hand-edited entry is not worth a failed download: drop it and let + # the caller fetch the response again. + $this->discard($file); + return null; + } + + /** @var array{created_at: int, status: int, headers: array>, body: string} $payload */ + + # The age comes from the payload and not from the file mtime: a restored CI cache writes the + # files anew, and an mtime-based entry would then never expire. + if (\time() - $payload['created_at'] > $this->ttl) { + return null; + } + + return new Response($payload['status'], $payload['headers'], $payload['body']); + } + + /** + * Stores a response. Any failure is reported and swallowed: the cache is an optimisation and + * must never turn a working download into a failed one. + */ + private function write(string $file, ResponseInterface $response): void + { + $status = $response->getStatusCode(); + + # Only successful responses are worth keeping: a cached rate limit answer would keep being + # served for the whole TTL, long after the limit is gone. + if ($status < 200 || $status > 299) { + return; + } + + try { + $payload = \json_encode([ + 'created_at' => \time(), + 'status' => $status, + 'headers' => $response->getHeaders(), + 'body' => self::readBody($response), + ], JSON_THROW_ON_ERROR); + + if (!\is_dir($this->directory) && !@\mkdir($this->directory, 0777, true) && !\is_dir($this->directory)) { + throw new \RuntimeException(\sprintf('Failed to create cache directory `%s`.', $this->directory)); + } + + # Written aside and moved into place, so a run interrupted mid-write and a parallel run + # writing the same entry cannot leave a half-written file for anyone to read. + $temp = $file . '.' . (string) \getmypid() . '.tmp'; + + if (@\file_put_contents($temp, $payload) === false) { + throw new \RuntimeException(\sprintf('Failed to write cache entry `%s`.', $temp)); + } + + if (!@\rename($temp, $file)) { + @\unlink($temp); + throw new \RuntimeException(\sprintf('Failed to store cache entry `%s`.', $file)); + } + } catch (\Throwable $e) { + $this->logger->exception($e, important: false); + } + } + + private function discard(string $file): void + { + @\unlink($file); + } + + private function fileOf(string $key): string + { + return $this->directory . \DIRECTORY_SEPARATOR . \hash('xxh128', $key) . '.json'; + } +} diff --git a/src/Module/Cache/Internal/NullResponseCache.php b/src/Module/Cache/Internal/NullResponseCache.php new file mode 100644 index 0000000..058ddae --- /dev/null +++ b/src/Module/Cache/Internal/NullResponseCache.php @@ -0,0 +1,25 @@ +repositoryPath = $owner . '/' . $repo; } @@ -196,13 +204,14 @@ private function decodeReleasesResponse(ResponseInterface $response): array */ private function releasesRequest(int $page): ResponseInterface { - return $this->request( - Method::Get, - $this->httpFactory->uri( - \sprintf(self::URL_RELEASES, $this->repositoryPath), - ['page' => $page], - ), + $uri = $this->httpFactory->uri( + \sprintf(self::URL_RELEASES, $this->repositoryPath), + ['page' => $page, 'per_page' => self::RELEASES_PER_PAGE], ); + + # Only the listing goes through the cache: asset downloads share the same client, and + # caching at that level would write every downloaded binary to the cache directory. + return $this->cache->remember((string) $uri, fn(): ResponseInterface => $this->request(Method::Get, $uri)); } private function hasNextPage(ResponseInterface $response): bool diff --git a/src/Module/Repository/Internal/GitHub/Factory.php b/src/Module/Repository/Internal/GitHub/Factory.php index 77fd1c4..8f74d95 100644 --- a/src/Module/Repository/Internal/GitHub/Factory.php +++ b/src/Module/Repository/Internal/GitHub/Factory.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Module\Repository\Internal\GitHub; +use Internal\DLoad\Module\Cache\ResponseCache; use Internal\DLoad\Module\Config\Schema\Embed\Repository as RepositoryConfig; use Internal\DLoad\Module\Config\Schema\GitHub; use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; @@ -30,6 +31,7 @@ public function __construct( private readonly HttpFactory $httpFactory, GitHub $gitHubConfig, private readonly Logger $logger, + private readonly ResponseCache $cache, ) { $this->gitHubClient = new Client( $httpFactory, @@ -59,6 +61,13 @@ public function create(RepositoryConfig $config): GitHubRepository */ private function createRepositoryApi(string $owner, string $repo): RepositoryApi { - return new RepositoryApi($this->gitHubClient, $this->httpFactory, $owner, $repo, $this->logger); + return new RepositoryApi( + $this->gitHubClient, + $this->httpFactory, + $owner, + $repo, + $this->logger, + $this->cache, + ); } } diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index 2c4d9be..2dde52d 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -53,7 +53,8 @@ public function getReleases(): ReleasesCollection // Create a generator function for lazy loading release pages $pageLoader = function (): \Generator { - $page = 0; + /** @var \Internal\DLoad\Module\Repository\Internal\Paginator|null $page */ + $page = null; $anyPageLoaded = false; do { @@ -61,11 +62,16 @@ public function getReleases(): ReleasesCollection // to avoid first eager loading because of generator yield []; - $paginator = $this->api->getReleases(++$page); - $releases = $paginator->getPageItems(); + # Asking the paginator for the next page IS the request for it: building a new + # paginator per page instead would send every page but the first one twice. + $page = $page === null ? $this->api->getReleases() : $page->getNextPage(); + + if ($page === null) { + return; + } $toYield = []; - foreach ($releases as $releaseDTO) { + foreach ($page->getPageItems() as $releaseDTO) { try { $toYield[] = GitHubRelease::fromDTO($this->api, $this, $releaseDTO); } catch (\Throwable $e) { @@ -76,9 +82,6 @@ public function getReleases(): ReleasesCollection } yield $toYield; $anyPageLoaded = true; - - // Check if there are more pages by getting next page - $hasMorePages = $paginator->getNextPage() !== null; } 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. @@ -93,7 +96,7 @@ public function getReleases(): ReleasesCollection $this->logger->exception($e, important: false); return; } - } while ($hasMorePages); + } while (true); }; // Create paginator diff --git a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php index 6af6c71..190a46b 100644 --- a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Module\Repository\Internal\GitLab\Api; +use Internal\DLoad\Module\Cache\ResponseCache; use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; use Internal\DLoad\Module\HttpClient\Method; use Internal\DLoad\Module\Repository\Exception\ApiException; @@ -28,6 +29,12 @@ final class RepositoryApi private const URL_RELEASES = 'https://gitlab.com/api/v4/projects/%s/releases'; private const URL_RELEASE_ASSET = 'https://gitlab.com/api/v4/projects/%s/releases/%s/downloads/%s'; + /** + * Number of releases to ask for in a single page. GitLab serves 20 by default and allows up to + * 100, so the maximum keeps the release list within as few requests as the API permits. + */ + private const RELEASES_PER_PAGE = 100; + /** * @var non-empty-string */ @@ -40,6 +47,7 @@ public function __construct( private readonly Client $client, private readonly HttpFactory $httpFactory, string $projectPath, + private readonly ResponseCache $cache, ) { $this->repositoryPath = $projectPath; } @@ -207,13 +215,14 @@ private function decodeReleasesResponse(ResponseInterface $response): array */ private function releasesRequest(int $page): ResponseInterface { - return $this->request( - Method::Get, - $this->httpFactory->uri( - \sprintf(self::URL_RELEASES, \urlencode($this->repositoryPath)), - ['page' => $page], - ), + $uri = $this->httpFactory->uri( + \sprintf(self::URL_RELEASES, \urlencode($this->repositoryPath)), + ['page' => $page, 'per_page' => self::RELEASES_PER_PAGE], ); + + # Only the listing goes through the cache: asset downloads share the same client, and + # caching at that level would write every downloaded binary to the cache directory. + return $this->cache->remember((string) $uri, fn(): ResponseInterface => $this->request(Method::Get, $uri)); } private function hasNextPage(ResponseInterface $response): bool diff --git a/src/Module/Repository/Internal/GitLab/Factory.php b/src/Module/Repository/Internal/GitLab/Factory.php index 68d601d..ef4263c 100644 --- a/src/Module/Repository/Internal/GitLab/Factory.php +++ b/src/Module/Repository/Internal/GitLab/Factory.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Module\Repository\Internal\GitLab; +use Internal\DLoad\Module\Cache\ResponseCache; use Internal\DLoad\Module\Config\Schema\Embed\Repository as RepositoryConfig; use Internal\DLoad\Module\Config\Schema\GitLab; use Internal\DLoad\Module\HttpClient\Factory as HttpFactory; @@ -30,6 +31,7 @@ public function __construct( private readonly HttpFactory $httpFactory, GitLab $gitLabConfig, private readonly Logger $logger, + private readonly ResponseCache $cache, ) { $this->gitLabClient = new Client( $httpFactory, @@ -57,6 +59,6 @@ public function create(RepositoryConfig $config): GitLabRepository */ private function createRepositoryApi(string $projectPath): RepositoryApi { - return new RepositoryApi($this->gitLabClient, $this->httpFactory, $projectPath); + return new RepositoryApi($this->gitLabClient, $this->httpFactory, $projectPath, $this->cache); } } diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index 94f3dff..53f3b8b 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -51,7 +51,8 @@ public function getReleases(): ReleasesCollection // Create a generator function for lazy loading release pages $pageLoader = function (): \Generator { - $page = 0; + /** @var \Internal\DLoad\Module\Repository\Internal\Paginator|null $page */ + $page = null; $anyPageLoaded = false; do { @@ -59,11 +60,16 @@ public function getReleases(): ReleasesCollection // to avoid first eager loading because of generator yield []; - $paginator = $this->api->getReleases(++$page); - $releases = $paginator->getPageItems(); + # Asking the paginator for the next page IS the request for it: building a new + # paginator per page instead would send every page but the first one twice. + $page = $page === null ? $this->api->getReleases() : $page->getNextPage(); + + if ($page === null) { + return; + } $toYield = []; - foreach ($releases as $releaseDTO) { + foreach ($page->getPageItems() as $releaseDTO) { try { $toYield[] = GitLabRelease::fromDTO($this->api, $this, $releaseDTO); } catch (\Throwable) { @@ -73,9 +79,6 @@ public function getReleases(): ReleasesCollection } yield $toYield; $anyPageLoaded = true; - - // Check if there are more pages by getting next page - $hasMorePages = $paginator->getNextPage() !== null; } 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. @@ -90,7 +93,7 @@ public function getReleases(): ReleasesCollection $this->logger->exception($e, important: false); return; } - } while ($hasMorePages); + } while (true); }; // Create paginator diff --git a/tests/Integration/Module/Cache/ResponseCacheBindingTest.php b/tests/Integration/Module/Cache/ResponseCacheBindingTest.php new file mode 100644 index 0000000..750db16 --- /dev/null +++ b/tests/Integration/Module/Cache/ResponseCacheBindingTest.php @@ -0,0 +1,73 @@ + \sys_get_temp_dir() . '/dload-cache-binding']), + FileResponseCache::class, + ); + } + + #[Test] + public function xmlAttributeEnablesTheFileCache(): void + { + Assert::instanceOf( + self::resolve(xml: \sprintf( + '', + \sys_get_temp_dir() . '/dload-cache-binding', + )), + FileResponseCache::class, + ); + } + + #[Test] + public function zeroTtlDisablesTheCache(): void + { + Assert::instanceOf( + self::resolve(environment: [ + 'DLOAD_CACHE_DIR' => \sys_get_temp_dir() . '/dload-cache-binding', + 'DLOAD_CACHE_TTL' => '0', + ]), + NullResponseCache::class, + ); + } + + /** + * @param array $environment + */ + private static function resolve(?string $xml = null, array $environment = []): ResponseCache + { + return Bootstrap::init() + ->withConfig(xml: $xml, environment: $environment) + ->finish() + ->get(ResponseCache::class); + } +} diff --git a/tests/Unit/Module/Cache/FileResponseCacheTest.php b/tests/Unit/Module/Cache/FileResponseCacheTest.php new file mode 100644 index 0000000..17caf43 --- /dev/null +++ b/tests/Unit/Module/Cache/FileResponseCacheTest.php @@ -0,0 +1,179 @@ +cache(); + $calls = 0; + $fetch = static function () use (&$calls): ResponseInterface { + ++$calls; + return new Response(200, ['link' => '; rel="next"'], 'first'); + }; + + $first = $cache->remember('https://api.github.com/x', $fetch); + $second = $cache->remember('https://api.github.com/x', $fetch); + + Assert::same($calls, 1); + Assert::same((string) $second->getBody(), (string) $first->getBody()); + Assert::same($second->getStatusCode(), 200); + + # The `link` header drives pagination: a cached response without it would look like a + # single-page listing and the remaining releases would silently disappear. + Assert::same($second->getHeaderLine('link'), '; rel="next"'); + } + + #[Test] + public function differentKeysAreCachedApart(): void + { + $cache = $this->cache(); + + $first = $cache->remember('https://api.github.com/a', static fn(): ResponseInterface => new Response(200, [], 'a')); + $second = $cache->remember('https://api.github.com/b', static fn(): ResponseInterface => new Response(200, [], 'b')); + + Assert::same((string) $first->getBody(), 'a'); + Assert::same((string) $second->getBody(), 'b'); + Assert::same((string) $cache->remember('https://api.github.com/a', self::unexpectedFetch(...))->getBody(), 'a'); + } + + #[Test] + public function entryOlderThanTheTtlIsFetchedAgain(): void + { + $cache = $this->cache(ttl: 60); + $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'stale')); + + $this->ageStoredEntries(120); + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'fresh')); + + Assert::same((string) $response->getBody(), 'fresh'); + } + + #[Test] + public function entryWithinTheTtlIsKept(): void + { + $cache = $this->cache(ttl: 600); + $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'cached')); + + # Freshness must come from the stored timestamp and not from the file mtime: a CI cache + # restores files with a fresh mtime, which would make every entry look brand new. + $this->ageStoredEntries(60, touchFiles: true); + + $response = $cache->remember('https://api.github.com/x', self::unexpectedFetch(...)); + + Assert::same((string) $response->getBody(), 'cached'); + } + + #[Test] + public function unsuccessfulResponseIsNotCached(): void + { + $cache = $this->cache(); + $calls = 0; + $fetch = static function () use (&$calls): ResponseInterface { + ++$calls; + return new Response(403, [], 'rate limit exceeded'); + }; + + $cache->remember('https://api.github.com/x', $fetch); + $cache->remember('https://api.github.com/x', $fetch); + + Assert::same($calls, 2); + Assert::same(\glob($this->directory . '/*.json'), []); + } + + #[Test] + public function corruptedEntryIsIgnored(): void + { + $cache = $this->cache(); + $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'cached')); + + foreach (\glob($this->directory . '/*.json') as $file) { + \file_put_contents($file, 'not a json payload'); + } + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'refetched')); + + Assert::same((string) $response->getBody(), 'refetched'); + } + + #[Test] + public function nullCacheAlwaysFetches(): void + { + $cache = new NullResponseCache(); + $calls = 0; + $fetch = static function () use (&$calls): ResponseInterface { + ++$calls; + return new Response(200, [], 'body'); + }; + + $cache->remember('https://api.github.com/x', $fetch); + $response = $cache->remember('https://api.github.com/x', $fetch); + + Assert::same($calls, 2); + Assert::same((string) $response->getBody(), 'body'); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->directory = \sys_get_temp_dir() . '/dload-response-cache-' . \bin2hex(\random_bytes(6)); + } + + #[AfterTest] + protected function cleanup(): void + { + if (!\is_dir($this->directory)) { + return; + } + + foreach (\glob($this->directory . '/*') as $file) { + \is_file($file) and \unlink($file); + } + + \rmdir($this->directory); + } + + private static function unexpectedFetch(): ResponseInterface + { + throw new \LogicException('The cached entry must be used instead of fetching again.'); + } + + private function cache(int $ttl = 600): FileResponseCache + { + return new FileResponseCache($this->directory, $ttl, new Logger()); + } + + /** + * Moves the stored creation timestamps back in time, so expiry can be tested without sleeping. + */ + private function ageStoredEntries(int $seconds, bool $touchFiles = false): void + { + foreach (\glob($this->directory . '/*.json') as $file) { + $payload = \json_decode(\file_get_contents($file), true, 512, JSON_THROW_ON_ERROR); + $payload['created_at'] -= $seconds; + \file_put_contents($file, \json_encode($payload, JSON_THROW_ON_ERROR)); + + $touchFiles and \touch($file); + } + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php new file mode 100644 index 0000000..88ee5f4 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -0,0 +1,124 @@ +getReleases(), false); + + Assert::same(\count($releases), 6); + Assert::same($client->requestedPages(), [1, 2, 3]); + } + + #[Test] + public function pagesAreLoadedOnlyWhenNeeded(): void + { + $client = new PagedClientStub(pages: 3, releasesPerPage: 2); + $repository = self::createRepository($client); + + foreach ($repository->getReleases() as $release) { + unset($release); + break; + } + + Assert::same($client->requestedPages(), [1]); + } + + #[Test] + public function releasesAreRequestedAHundredPerPage(): void + { + $client = new PagedClientStub(pages: 1, releasesPerPage: 2); + $repository = self::createRepository($client); + + \iterator_to_array($repository->getReleases(), false); + + Assert::same($client->requests, ['page=1&per_page=100']); + } + + #[Test] + public function cachedListingsCostNoRequestsOnTheNextRun(): void + { + $firstClient = new PagedClientStub(pages: 2, releasesPerPage: 2); + $firstRun = \iterator_to_array(self::createRepository($firstClient, $this->cache())->getReleases(), false); + + # A second run in a fresh process with the cache directory carried over: every listing is + # answered from disk, so the rate limit is left untouched. + $secondClient = new PagedClientStub(pages: 2, releasesPerPage: 2); + $secondRun = \iterator_to_array(self::createRepository($secondClient, $this->cache())->getReleases(), false); + + Assert::same($firstClient->requestedPages(), [1, 2]); + Assert::same($secondClient->requestedPages(), []); + Assert::same(\count($firstRun), 4); + Assert::same(\count($secondRun), 4); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->cacheDirectory = \sys_get_temp_dir() . '/dload-github-cache-' . \bin2hex(\random_bytes(6)); + } + + #[AfterTest] + protected function cleanup(): void + { + if (!\is_dir($this->cacheDirectory)) { + return; + } + + foreach (\glob($this->cacheDirectory . '/*') as $file) { + \is_file($file) and \unlink($file); + } + + \rmdir($this->cacheDirectory); + } + + private static function createRepository( + PagedClientStub $client, + ResponseCache $cache = new NullResponseCache(), + ): GitHubRepository { + $logger = new Logger(); + $httpFactory = new NyholmFactoryImpl($logger); + $api = new RepositoryApi( + new Client($httpFactory, $client, new GitHubConfig()), + $httpFactory, + 'owner', + 'repo', + $logger, + $cache, + ); + + return new GitHubRepository($api, 'owner', 'repo', $logger); + } + + private function cache(): FileResponseCache + { + return new FileResponseCache($this->cacheDirectory, 600, new Logger()); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php new file mode 100644 index 0000000..39dcd79 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php @@ -0,0 +1,94 @@ + + */ + public array $requests = []; + + /** + * @param int<1, max> $pages Number of pages the list is split into. + * @param int<1, max> $releasesPerPage Number of releases on every page. + */ + public function __construct( + private readonly int $pages = 1, + private readonly int $releasesPerPage = 2, + ) {} + + public function sendRequest(RequestInterface $request): ResponseInterface + { + $query = $request->getUri()->getQuery(); + $this->requests[] = $query; + + $page = self::pageOf($query); + + if ($page > $this->pages) { + return new ResponseStub(200, [], '[]'); + } + + $headers = $page < $this->pages + ? ['link' => [\sprintf('; rel="next"', $page + 1)]] + : []; + + return new ResponseStub(200, $headers, \json_encode($this->releasesOfPage($page))); + } + + /** + * Page number of every received request, in order. + * + * @return list + */ + public function requestedPages(): array + { + return \array_map(self::pageOf(...), $this->requests); + } + + private static function pageOf(string $query): int + { + \parse_str($query, $params); + + return (int) ($params['page'] ?? 1); + } + + /** + * @param int<1, max> $page + * @return list> + */ + private function releasesOfPage(int $page): array + { + $releases = []; + $offset = ($page - 1) * $this->releasesPerPage; + + for ($i = 1; $i <= $this->releasesPerPage; $i++) { + $tag = \sprintf('v1.0.%d', $offset + $i); + $releases[] = [ + 'name' => $tag, + 'tag_name' => $tag, + 'published_at' => '2024-01-01T00:00:00Z', + 'assets' => [], + 'prerelease' => false, + 'draft' => false, + ]; + } + + return $releases; + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php index 96b9527..74f5e40 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/FactoryTest.php @@ -4,6 +4,7 @@ namespace Internal\DLoad\Tests\Unit\Module\Repository\Internal\GitLab; +use Internal\DLoad\Module\Cache\Internal\NullResponseCache; use Internal\DLoad\Module\Config\Schema\Embed\Repository as RepositoryConfig; use Internal\DLoad\Module\Config\Schema\GitLab as GitLabConfig; use Internal\DLoad\Module\Repository\Internal\GitLab\Factory; @@ -69,6 +70,11 @@ public function createDerivesTheProjectPathFromTheUri(string $uri, string $expec #[BeforeTest] protected function prepare(): void { - $this->factory = new Factory(new HttpFactoryStub(), new GitLabConfig(), new Logger()); + $this->factory = new Factory( + new HttpFactoryStub(), + new GitLabConfig(), + new Logger(), + new NullResponseCache(), + ); } } diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php new file mode 100644 index 0000000..5d2381b --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -0,0 +1,58 @@ +getReleases(), false); + + Assert::same(\count($releases), 6); + Assert::same($client->requestedPages(), [1, 2, 3]); + } + + #[Test] + public function releasesAreRequestedAHundredPerPage(): void + { + $client = new PagedClientStub(pages: 1, releasesPerPage: 2); + $repository = self::createRepository($client); + + \iterator_to_array($repository->getReleases(), false); + + Assert::same($client->requests, ['page=1&per_page=100']); + } + + private static function createRepository(PagedClientStub $client): GitLabRepository + { + $logger = new Logger(); + $httpFactory = new NyholmFactoryImpl($logger); + $api = new RepositoryApi( + new Client($httpFactory, $client, new GitLabConfig()), + $httpFactory, + 'group/project', + new NullResponseCache(), + ); + + return new GitLabRepository($api, 'group/project', $logger); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php new file mode 100644 index 0000000..41997de --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php @@ -0,0 +1,95 @@ + + */ + public array $requests = []; + + /** + * @param int<1, max> $pages Number of pages the list is split into. + * @param int<1, max> $releasesPerPage Number of releases on every page. + */ + public function __construct( + private readonly int $pages = 1, + private readonly int $releasesPerPage = 2, + ) {} + + public function sendRequest(RequestInterface $request): ResponseInterface + { + $query = $request->getUri()->getQuery(); + $this->requests[] = $query; + + $page = self::pageOf($query); + + if ($page > $this->pages) { + return new ResponseStub(200, [], '[]'); + } + + $headers = $page < $this->pages + ? ['link' => [\sprintf('; rel="next"', $page + 1)]] + : []; + + return new ResponseStub(200, $headers, \json_encode($this->releasesOfPage($page))); + } + + /** + * Page number of every received request, in order. + * + * @return list + */ + public function requestedPages(): array + { + return \array_map(self::pageOf(...), $this->requests); + } + + private static function pageOf(string $query): int + { + \parse_str($query, $params); + + return (int) ($params['page'] ?? 1); + } + + /** + * @param int<1, max> $page + * @return list> + */ + private function releasesOfPage(int $page): array + { + $releases = []; + $offset = ($page - 1) * $this->releasesPerPage; + + for ($i = 1; $i <= $this->releasesPerPage; $i++) { + $tag = \sprintf('v1.0.%d', $offset + $i); + $releases[] = [ + 'name' => $tag, + 'tag_name' => $tag, + 'description' => 'Release ' . $tag, + 'created_at' => '2024-01-01T00:00:00Z', + 'released_at' => '2024-01-01T00:00:00Z', + 'assets' => ['links' => []], + 'upcoming_release' => false, + ]; + } + + return $releases; + } +} From c74d9f41243c568039761efe365d4ee64916eb9e Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Sat, 12 Sep 2026 20:42:24 +0400 Subject: [PATCH 2/3] test: cover every line the release listing cache adds Codecov reported 80% patch coverage. The store failure paths were never run, and the release listing requests were attributed to no covering class, so RepositoryApi showed 0%. Add tests for an unreadable entry and for each of the three store failures, declare RepositoryApi as covered by the repository tests, and add the GitHub factory test that its GitLab counterpart already had. --- .../Module/Cache/FileResponseCacheTest.php | 97 +++++++++++++++++-- .../Internal/GitHub/FactoryTest.php | 79 +++++++++++++++ .../Internal/GitHub/GitHubRepositoryTest.php | 1 + .../Internal/GitLab/GitLabRepositoryTest.php | 1 + 4 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/Module/Repository/Internal/GitHub/FactoryTest.php diff --git a/tests/Unit/Module/Cache/FileResponseCacheTest.php b/tests/Unit/Module/Cache/FileResponseCacheTest.php index 17caf43..c7caf89 100644 --- a/tests/Unit/Module/Cache/FileResponseCacheTest.php +++ b/tests/Unit/Module/Cache/FileResponseCacheTest.php @@ -11,6 +11,7 @@ use Psr\Http\Message\ResponseInterface; use Testo\Assert; use Testo\Codecov\Covers; +use Testo\Core\Exception\SkipTest; use Testo\Lifecycle\AfterTest; use Testo\Lifecycle\BeforeTest; use Testo\Test; @@ -133,6 +134,63 @@ public function nullCacheAlwaysFetches(): void Assert::same((string) $response->getBody(), 'body'); } + #[Test] + public function unreadableEntryIsFetchedAgain(): void + { + $cache = $this->cache(); + $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'cached')); + + foreach (\glob($this->directory . '/*.json') as $file) { + \chmod($file, 0000); + \is_readable($file) and throw new SkipTest('The entry stays readable for this user.'); + } + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'refetched')); + + Assert::same((string) $response->getBody(), 'refetched'); + } + + #[Test] + public function responseIsReturnedWhenTheDirectoryCannotBeCreated(): void + { + \file_put_contents($this->directory, 'a file where the cache directory should be'); + $cache = new FileResponseCache($this->directory . '/entries', 600, new Logger()); + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'body')); + + Assert::same((string) $response->getBody(), 'body'); + } + + #[Test] + public function responseIsReturnedWhenTheEntryCannotBeWritten(): void + { + $cache = $this->cache(); + $file = $this->storeAndForget($cache); + + # The entry is written aside first, so an unwritable temporary path is what makes the store fail. + \mkdir($file . '.' . \getmypid() . '.tmp'); + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'body')); + + Assert::same((string) $response->getBody(), 'body'); + } + + #[Test] + public function responseIsReturnedWhenTheEntryCannotBeMovedIntoPlace(): void + { + $cache = $this->cache(); + $file = $this->storeAndForget($cache); + + # A non-empty directory in place of the entry cannot be replaced by a rename. + \mkdir($file); + \file_put_contents($file . '/occupied', 'x'); + + $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'body')); + + Assert::same((string) $response->getBody(), 'body'); + Assert::same(\file_get_contents($file . '/occupied'), 'x'); + } + #[BeforeTest] protected function prepare(): void { @@ -142,15 +200,24 @@ protected function prepare(): void #[AfterTest] protected function cleanup(): void { - if (!\is_dir($this->directory)) { - return; - } + self::erase($this->directory); + } - foreach (\glob($this->directory . '/*') as $file) { - \is_file($file) and \unlink($file); + /** + * Removes a file or a directory with everything below it. + */ + private static function erase(string $path): void + { + if (\is_dir($path)) { + foreach (\glob($path . '/*') as $child) { + self::erase($child); + } + + \rmdir($path); + return; } - \rmdir($this->directory); + \is_file($path) and \unlink($path); } private static function unexpectedFetch(): ResponseInterface @@ -163,6 +230,24 @@ private function cache(int $ttl = 600): FileResponseCache return new FileResponseCache($this->directory, $ttl, new Logger()); } + /** + * Stores an entry to learn the path the cache picks for the key, then removes it again, so a + * test can put an obstacle exactly where the next store writes. + * + * @return non-empty-string + */ + private function storeAndForget(FileResponseCache $cache): string + { + $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'stored')); + + $files = \glob($this->directory . '/*.json'); + Assert::same(\count($files), 1); + + \unlink($files[0]); + + return $files[0]; + } + /** * Moves the stored creation timestamps back in time, so expiry can be tested without sleeping. */ diff --git a/tests/Unit/Module/Repository/Internal/GitHub/FactoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/FactoryTest.php new file mode 100644 index 0000000..1f39be2 --- /dev/null +++ b/tests/Unit/Module/Repository/Internal/GitHub/FactoryTest.php @@ -0,0 +1,79 @@ + ['github', true]; + yield 'mixed case' => ['GitHub', true]; + yield 'uppercase' => ['GITHUB', true]; + yield 'gitlab' => ['gitlab', false]; + yield 'unknown' => ['custom', false]; + } + + public static function provideRepositoryUris(): \Generator + { + yield 'bare repository path' => ['owner/repo', 'owner/repo']; + yield 'full url' => ['https://github.com/owner/repo', 'owner/repo']; + } + + #[DataProvider('provideSupportedTypes')] + #[Test] + public function supportsOnlyGithubRegardlessOfCase(string $type, bool $expected): void + { + $config = new RepositoryConfig(); + $config->type = $type; + $config->uri = 'owner/repo'; + + Assert::same($this->factory->supports($config), $expected); + } + + #[DataProvider('provideRepositoryUris')] + #[Test] + public function createDerivesTheRepositoryNameFromTheUri(string $uri, string $expectedName): void + { + $config = new RepositoryConfig(); + $config->type = 'github'; + $config->uri = $uri; + + Assert::same($this->factory->create($config)->getName(), $expectedName); + } + + #[BeforeTest] + protected function prepare(): void + { + $this->factory = new Factory( + new HttpFactoryStub( + static fn(): UriInterface&MockInterface => \Mockery::mock(UriInterface::class)->shouldIgnoreMissing(), + static fn(): RequestInterface&MockInterface => \Mockery::mock(RequestInterface::class)->shouldIgnoreMissing(), + static fn(): ClientInterface&MockInterface => \Mockery::mock(ClientInterface::class)->shouldIgnoreMissing(), + ), + new GitHubConfig(), + new Logger(), + new NullResponseCache(), + ); + } +} diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index 88ee5f4..ac27556 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -21,6 +21,7 @@ use Testo\Test; #[Covers(GitHubRepository::class)] +#[Covers(RepositoryApi::class)] final class GitHubRepositoryTest { private string $cacheDirectory; diff --git a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php index 5d2381b..4bb885d 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/GitLabRepositoryTest.php @@ -17,6 +17,7 @@ use Testo\Test; #[Covers(GitLabRepository::class)] +#[Covers(RepositoryApi::class)] final class GitLabRepositoryTest { #[Test] From 79b13f625e88ed657f4fa4fa7ce780d626492071 Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Sat, 12 Sep 2026 20:45:07 +0400 Subject: [PATCH 3/3] style: remove comments from the release listing cache code --- .../Cache/Internal/FileResponseCache.php | 28 ++----------------- .../Cache/Internal/NullResponseCache.php | 5 ---- src/Module/Cache/ResponseCache.php | 10 +------ src/Module/Config/Schema/Cache.php | 9 +----- .../Internal/GitHub/Api/RepositoryApi.php | 7 ----- .../Internal/GitHub/GitHubRepository.php | 2 -- .../Internal/GitLab/Api/RepositoryApi.php | 7 ----- .../Internal/GitLab/GitLabRepository.php | 2 -- .../Module/Cache/ResponseCacheBindingTest.php | 4 --- .../Module/Cache/FileResponseCacheTest.php | 15 ---------- .../Internal/GitHub/GitHubRepositoryTest.php | 2 -- .../Internal/GitHub/Stub/PagedClientStub.php | 14 ++-------- .../Internal/GitLab/Stub/PagedClientStub.php | 14 ++-------- 13 files changed, 8 insertions(+), 111 deletions(-) diff --git a/src/Module/Cache/Internal/FileResponseCache.php b/src/Module/Cache/Internal/FileResponseCache.php index 3ee3314..8e5ccbd 100644 --- a/src/Module/Cache/Internal/FileResponseCache.php +++ b/src/Module/Cache/Internal/FileResponseCache.php @@ -10,20 +10,14 @@ use Psr\Http\Message\ResponseInterface; /** - * Cache that keeps responses as JSON files in a directory. - * - * The directory is meant to be carried between runs (a CI cache action, for example), so the - * entries are self-contained: status, headers and body are all stored, and the age of an entry is - * read from the payload rather than from the file system. - * * @internal * @psalm-internal Internal\DLoad */ final class FileResponseCache implements ResponseCache { /** - * @param non-empty-string $directory Directory the entries are stored in. - * @param int<1, max> $ttl Number of seconds an entry stays usable. + * @param non-empty-string $directory + * @param int<1, max> $ttl */ public function __construct( private readonly string $directory, @@ -46,9 +40,6 @@ public function remember(string $key, \Closure $fetch): ResponseInterface return $response; } - /** - * Reads the body without consuming it for the caller. - */ private static function readBody(ResponseInterface $response): string { $stream = $response->getBody(); @@ -60,9 +51,6 @@ private static function readBody(ResponseInterface $response): string return $body; } - /** - * Reads an entry, or returns `null` when there is none, it expired, or it cannot be used. - */ private function read(string $file): ?ResponseInterface { if (!\is_file($file)) { @@ -86,16 +74,12 @@ private function read(string $file): ?ResponseInterface && \is_string($payload['body'] ?? null) or throw new \UnexpectedValueException('Unexpected cache entry structure.'); } catch (\Throwable) { - # A half-written or hand-edited entry is not worth a failed download: drop it and let - # the caller fetch the response again. $this->discard($file); return null; } /** @var array{created_at: int, status: int, headers: array>, body: string} $payload */ - # The age comes from the payload and not from the file mtime: a restored CI cache writes the - # files anew, and an mtime-based entry would then never expire. if (\time() - $payload['created_at'] > $this->ttl) { return null; } @@ -103,16 +87,10 @@ private function read(string $file): ?ResponseInterface return new Response($payload['status'], $payload['headers'], $payload['body']); } - /** - * Stores a response. Any failure is reported and swallowed: the cache is an optimisation and - * must never turn a working download into a failed one. - */ private function write(string $file, ResponseInterface $response): void { $status = $response->getStatusCode(); - # Only successful responses are worth keeping: a cached rate limit answer would keep being - # served for the whole TTL, long after the limit is gone. if ($status < 200 || $status > 299) { return; } @@ -129,8 +107,6 @@ private function write(string $file, ResponseInterface $response): void throw new \RuntimeException(\sprintf('Failed to create cache directory `%s`.', $this->directory)); } - # Written aside and moved into place, so a run interrupted mid-write and a parallel run - # writing the same entry cannot leave a half-written file for anyone to read. $temp = $file . '.' . (string) \getmypid() . '.tmp'; if (@\file_put_contents($temp, $payload) === false) { diff --git a/src/Module/Cache/Internal/NullResponseCache.php b/src/Module/Cache/Internal/NullResponseCache.php index 058ddae..6f1cecf 100644 --- a/src/Module/Cache/Internal/NullResponseCache.php +++ b/src/Module/Cache/Internal/NullResponseCache.php @@ -8,11 +8,6 @@ use Psr\Http\Message\ResponseInterface; /** - * Cache that stores nothing: every call performs the request. - * - * Used when no cache directory is configured, so callers never have to check whether caching - * is enabled. - * * @internal * @psalm-internal Internal\DLoad */ diff --git a/src/Module/Cache/ResponseCache.php b/src/Module/Cache/ResponseCache.php index 442ff9a..9252e34 100644 --- a/src/Module/Cache/ResponseCache.php +++ b/src/Module/Cache/ResponseCache.php @@ -7,20 +7,12 @@ use Psr\Http\Message\ResponseInterface; /** - * Cache for HTTP responses that may be reused between runs. - * - * Intended for API listings that are cheap to serve from disk and expensive in terms of API rate - * limit: a cached listing costs no request at all. - * * @internal */ interface ResponseCache { /** - * Returns the cached response for the key, or the result of `$fetch` when there is none. - * - * @param string $key Identifies the response; the request URI in practice. - * @param \Closure(): ResponseInterface $fetch Performs the request when the cache cannot answer. + * @param \Closure(): ResponseInterface $fetch */ public function remember(string $key, \Closure $fetch): ResponseInterface; } diff --git a/src/Module/Config/Schema/Cache.php b/src/Module/Config/Schema/Cache.php index 33581e4..aa61e9c 100644 --- a/src/Module/Config/Schema/Cache.php +++ b/src/Module/Config/Schema/Cache.php @@ -9,23 +9,16 @@ use Internal\DLoad\Module\Common\Internal\Attribute\XPath; /** - * Release listings cache configuration. - * - * Caching is off until a directory is configured. A directory that survives between runs (a CI - * cache, for example) lets repeated runs read release listings from disk instead of spending the - * API rate limit on them. - * * @internal */ #[InflectableConfig] final class Cache { - /** @var non-empty-string|null $dir Directory to store cached release listings in */ + /** @var non-empty-string|null $dir */ #[XPath('/dload/@cache-dir')] #[Env('DLOAD_CACHE_DIR')] public ?string $dir = null; - /** @var int $ttl Number of seconds a cached release listing stays usable */ #[XPath('/dload/@cache-ttl')] #[Env('DLOAD_CACHE_TTL')] public int $ttl = 600; diff --git a/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php index 554ca3f..eb04dec 100644 --- a/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitHub/Api/RepositoryApi.php @@ -28,11 +28,6 @@ final class RepositoryApi { private const URL_REPOSITORY = 'https://api.github.com/repos/%s'; private const URL_RELEASES = 'https://api.github.com/repos/%s/releases'; - - /** - * Number of releases to ask for in a single page. GitHub serves 30 by default and allows up to - * 100, so the maximum keeps the release list within as few requests as the API permits. - */ private const RELEASES_PER_PAGE = 100; /** @@ -209,8 +204,6 @@ private function releasesRequest(int $page): ResponseInterface ['page' => $page, 'per_page' => self::RELEASES_PER_PAGE], ); - # Only the listing goes through the cache: asset downloads share the same client, and - # caching at that level would write every downloaded binary to the cache directory. return $this->cache->remember((string) $uri, fn(): ResponseInterface => $this->request(Method::Get, $uri)); } diff --git a/src/Module/Repository/Internal/GitHub/GitHubRepository.php b/src/Module/Repository/Internal/GitHub/GitHubRepository.php index 2dde52d..a56a211 100644 --- a/src/Module/Repository/Internal/GitHub/GitHubRepository.php +++ b/src/Module/Repository/Internal/GitHub/GitHubRepository.php @@ -62,8 +62,6 @@ public function getReleases(): ReleasesCollection // to avoid first eager loading because of generator yield []; - # Asking the paginator for the next page IS the request for it: building a new - # paginator per page instead would send every page but the first one twice. $page = $page === null ? $this->api->getReleases() : $page->getNextPage(); if ($page === null) { diff --git a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php index 190a46b..93395e6 100644 --- a/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php +++ b/src/Module/Repository/Internal/GitLab/Api/RepositoryApi.php @@ -28,11 +28,6 @@ final class RepositoryApi private const URL_REPOSITORY = 'https://gitlab.com/api/v4/projects/%s'; private const URL_RELEASES = 'https://gitlab.com/api/v4/projects/%s/releases'; private const URL_RELEASE_ASSET = 'https://gitlab.com/api/v4/projects/%s/releases/%s/downloads/%s'; - - /** - * Number of releases to ask for in a single page. GitLab serves 20 by default and allows up to - * 100, so the maximum keeps the release list within as few requests as the API permits. - */ private const RELEASES_PER_PAGE = 100; /** @@ -220,8 +215,6 @@ private function releasesRequest(int $page): ResponseInterface ['page' => $page, 'per_page' => self::RELEASES_PER_PAGE], ); - # Only the listing goes through the cache: asset downloads share the same client, and - # caching at that level would write every downloaded binary to the cache directory. return $this->cache->remember((string) $uri, fn(): ResponseInterface => $this->request(Method::Get, $uri)); } diff --git a/src/Module/Repository/Internal/GitLab/GitLabRepository.php b/src/Module/Repository/Internal/GitLab/GitLabRepository.php index 53f3b8b..066ca1a 100644 --- a/src/Module/Repository/Internal/GitLab/GitLabRepository.php +++ b/src/Module/Repository/Internal/GitLab/GitLabRepository.php @@ -60,8 +60,6 @@ public function getReleases(): ReleasesCollection // to avoid first eager loading because of generator yield []; - # Asking the paginator for the next page IS the request for it: building a new - # paginator per page instead would send every page but the first one twice. $page = $page === null ? $this->api->getReleases() : $page->getNextPage(); if ($page === null) { diff --git a/tests/Integration/Module/Cache/ResponseCacheBindingTest.php b/tests/Integration/Module/Cache/ResponseCacheBindingTest.php index 750db16..9b89d82 100644 --- a/tests/Integration/Module/Cache/ResponseCacheBindingTest.php +++ b/tests/Integration/Module/Cache/ResponseCacheBindingTest.php @@ -13,10 +13,6 @@ use Testo\Filter\Group; use Testo\Test; -/** - * The cache is only useful when the container actually hands out the configured implementation, - * so the binding is verified through a real bootstrap rather than by constructing it by hand. - */ #[Group('integration')] #[Covers(Bootstrap::class)] final class ResponseCacheBindingTest diff --git a/tests/Unit/Module/Cache/FileResponseCacheTest.php b/tests/Unit/Module/Cache/FileResponseCacheTest.php index c7caf89..4a4b579 100644 --- a/tests/Unit/Module/Cache/FileResponseCacheTest.php +++ b/tests/Unit/Module/Cache/FileResponseCacheTest.php @@ -39,8 +39,6 @@ public function secondCallIsServedFromTheCacheWithItsHeaders(): void Assert::same((string) $second->getBody(), (string) $first->getBody()); Assert::same($second->getStatusCode(), 200); - # The `link` header drives pagination: a cached response without it would look like a - # single-page listing and the remaining releases would silently disappear. Assert::same($second->getHeaderLine('link'), '; rel="next"'); } @@ -76,8 +74,6 @@ public function entryWithinTheTtlIsKept(): void $cache = $this->cache(ttl: 600); $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'cached')); - # Freshness must come from the stored timestamp and not from the file mtime: a CI cache - # restores files with a fresh mtime, which would make every entry look brand new. $this->ageStoredEntries(60, touchFiles: true); $response = $cache->remember('https://api.github.com/x', self::unexpectedFetch(...)); @@ -167,7 +163,6 @@ public function responseIsReturnedWhenTheEntryCannotBeWritten(): void $cache = $this->cache(); $file = $this->storeAndForget($cache); - # The entry is written aside first, so an unwritable temporary path is what makes the store fail. \mkdir($file . '.' . \getmypid() . '.tmp'); $response = $cache->remember('https://api.github.com/x', static fn(): ResponseInterface => new Response(200, [], 'body')); @@ -181,7 +176,6 @@ public function responseIsReturnedWhenTheEntryCannotBeMovedIntoPlace(): void $cache = $this->cache(); $file = $this->storeAndForget($cache); - # A non-empty directory in place of the entry cannot be replaced by a rename. \mkdir($file); \file_put_contents($file . '/occupied', 'x'); @@ -203,9 +197,6 @@ protected function cleanup(): void self::erase($this->directory); } - /** - * Removes a file or a directory with everything below it. - */ private static function erase(string $path): void { if (\is_dir($path)) { @@ -231,9 +222,6 @@ private function cache(int $ttl = 600): FileResponseCache } /** - * Stores an entry to learn the path the cache picks for the key, then removes it again, so a - * test can put an obstacle exactly where the next store writes. - * * @return non-empty-string */ private function storeAndForget(FileResponseCache $cache): string @@ -248,9 +236,6 @@ private function storeAndForget(FileResponseCache $cache): string return $files[0]; } - /** - * Moves the stored creation timestamps back in time, so expiry can be tested without sleeping. - */ private function ageStoredEntries(int $seconds, bool $touchFiles = false): void { foreach (\glob($this->directory . '/*.json') as $file) { diff --git a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php index ac27556..d1b1956 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/GitHubRepositoryTest.php @@ -69,8 +69,6 @@ public function cachedListingsCostNoRequestsOnTheNextRun(): void $firstClient = new PagedClientStub(pages: 2, releasesPerPage: 2); $firstRun = \iterator_to_array(self::createRepository($firstClient, $this->cache())->getReleases(), false); - # A second run in a fresh process with the cache directory carried over: every listing is - # answered from disk, so the rate limit is left untouched. $secondClient = new PagedClientStub(pages: 2, releasesPerPage: 2); $secondRun = \iterator_to_array(self::createRepository($secondClient, $this->cache())->getReleases(), false); diff --git a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php index 39dcd79..4102336 100644 --- a/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitHub/Stub/PagedClientStub.php @@ -9,24 +9,16 @@ use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; -/** - * HTTP client stub that serves a paginated GitHub releases list. - * - * Records every request it receives, so a test can assert not only what the caller got back, - * but how many requests it took to get there. - */ final class PagedClientStub implements ClientInterface { /** - * Query string of every received request, in order. - * * @var list */ public array $requests = []; /** - * @param int<1, max> $pages Number of pages the list is split into. - * @param int<1, max> $releasesPerPage Number of releases on every page. + * @param int<1, max> $pages + * @param int<1, max> $releasesPerPage */ public function __construct( private readonly int $pages = 1, @@ -52,8 +44,6 @@ public function sendRequest(RequestInterface $request): ResponseInterface } /** - * Page number of every received request, in order. - * * @return list */ public function requestedPages(): array diff --git a/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php index 41997de..01fcb9b 100644 --- a/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php +++ b/tests/Unit/Module/Repository/Internal/GitLab/Stub/PagedClientStub.php @@ -9,24 +9,16 @@ use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; -/** - * HTTP client stub that serves a paginated GitLab releases list. - * - * Records every request it receives, so a test can assert not only what the caller got back, - * but how many requests it took to get there. - */ final class PagedClientStub implements ClientInterface { /** - * Query string of every received request, in order. - * * @var list */ public array $requests = []; /** - * @param int<1, max> $pages Number of pages the list is split into. - * @param int<1, max> $releasesPerPage Number of releases on every page. + * @param int<1, max> $pages + * @param int<1, max> $releasesPerPage */ public function __construct( private readonly int $pages = 1, @@ -52,8 +44,6 @@ public function sendRequest(RequestInterface $request): ResponseInterface } /** - * Page number of every received request, in order. - * * @return list */ public function requestedPages(): array