fix: request every releases page once and cache release listings - #118
Open
xepozz wants to merge 3 commits into
Open
fix: request every releases page once and cache release listings#118xepozz wants to merge 3 commits into
xepozz wants to merge 3 commits into
Conversation
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The release page loader built a new
Paginatorfor every page:getNextPage()performs the request for the next page, and the next iteration then built a freshpaginator for that same page and requested it again. Every page but the first was fetched twice,
and each duplicate costs a GitHub or GitLab API rate limit unit.
Changes
because asking for the next page IS the request for it. Applies to both
GitHubRepositoryandGitLabRepository.per_page=100. GitHub serves 30 releases per page by default and GitLab 20; both allow 100.A repository with 100 releases now costs one request instead of four (GitHub) or five (GitLab).
ResponseCachewraps the listing requestonly — asset downloads share the same client and must never be written to the cache directory.
Caching is off until
cache-dir/DLOAD_CACHE_DIRis set;cache-ttl/DLOAD_CACHE_TTLdefaults to 600 seconds and
0disables it.Only successful responses are stored, so a rate limit answer is not replayed after the limit is
gone. Entry age comes from the stored timestamp and not the file mtime, because a restored CI cache
writes the files with a fresh mtime and mtime-based entries would never expire. Entries are written
aside and renamed into place, so an interrupted or parallel run cannot leave a half-written file.
Any cache failure is logged and swallowed: the cache is an optimisation and must not turn a working
download into a failed one.
Tests
GitHubRepositoryTest/GitLabRepositoryTestassert the exact pages requested ([1, 2, 3], not[1, 2, 2, 3, 3]), that pages stay lazy, thatper_page=100is sent, and that a second run witha carried-over cache directory sends no requests at all.
FileResponseCacheTestcovers thelinkheader surviving the round trip (pagination depends onit), TTL expiry, mtime independence, unsuccessful responses not being stored, and corrupted
entries being ignored.
ResponseCacheBindingTestresolves the binding through a real bootstrap for the XML attribute,the environment variable, and the
ttl=0case.composer test— 517 passed, 2 skipped.composer psalm— no errors.composer cs:diff— clean.