fix: paginate sem get tasks to return all tasks for a project#249
Open
cchristous wants to merge 3 commits intosemaphoreci:masterfrom
Open
fix: paginate sem get tasks to return all tasks for a project#249cchristous wants to merge 3 commits intosemaphoreci:masterfrom
cchristous wants to merge 3 commits intosemaphoreci:masterfrom
Conversation
cchristous
dismissed
loadez’s stale review
The merge-base changed after approval.
ListTasks previously issued a single GET /api/v1alpha/tasks and returned whatever the first page contained (~30 tasks), silently discarding the rest. Projects with >30 tasks were invisible beyond the first page. Rewrite ListTasks to loop with page_size=200, following the API's Link: rel="next" header until exhausted, and aggregating all pages. Mirrors the pattern established for pipelines in semaphoreci#247, reusing retry.RetryWithMaxFailures, retry.NonRetryable, and hasNextPage. Includes a safety cap of 500 pages (100k tasks) and nil-header guard. No CLI surface changes -- sem get tasks -p <project> just works now. Existing httpmock responders that matched task URLs exactly are switched to regexp matchers to accommodate the new page/page_size query params. A new Test__ListTasks__MultiPage verifies both pages are fetched via Link: rel=next and aggregated into the output. Reported by Kumar Utkarsh against semaphoreci#246.
- maxTaskPages is now a local const inside ListTasks (matches pipelines pattern); the previous package-level var was silently mutable and unnecessary since no test overrides it. - Nil-header guard now fires before the page is appended to allTasks, so on error we don't discard partial state that was already mutated. The error message now includes the accumulated task count for debuggability. - Multi-page test regexps tightened: `page=1.*` would also match page=10, page=11, etc. Replaced with anchored boundary patterns `[?&]page=N(?:&|$)` so the regex cannot accidentally match a later page number if the test is extended.
cchristous
force-pushed
the
feat/task-list-pagination
branch
from
60d627e to
56606da
Compare
Without this assertion, a regression in hasNextPage or the break condition could let the loop run extra requests; the existing page1Received/page2Received flags only verify lower bounds. With GetTotalCallCount we explicitly assert the loop terminated when Link: rel=next was absent on page 2.
Contributor
Author
|
Tests are failing, but I can't see why. Can you take a look? |
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
2 participants
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.
Summary
Fixes a pagination bug in
sem get tasks -p <project>: it was silently truncating results to the first 30 tasks (one page) regardless of how many tasks the project had. A project with more than 30 tasks only showed the first 30; the rest were invisible from the CLI even though the UI showed them all.Root cause
ListTasksinapi/client/tasks_v1_alpha.goissued a singleGET /api/v1alpha/tasks?project_id=…and returned whatever came back, discarding the pagination headers. The v1alpha API paginates viaLink: rel="next"— the headers are returned in every response (e.g.Total: N Per-Page: 30 Link: …?page=2…; rel="next"), just unused.Fix
Rewrite
ListTasksto loop withpage_size=200, followLink: rel="next"until exhausted, and aggregate every page into a singleTaskListV1Alpha.This mirrors the pattern established for
sem get pipelinesin #247 (now merged):retry.RetryWithMaxFailureswith exponential backoff for transient failures (5xx, 429, transport errors)retry.NonRetryablefor 3xx/4xx (except 429) and deserialization failures — no wasted retriesThe CLI surface is unchanged — no new flags, same command, just works.
Why this is safe
DescribeTaskorRunTask; pagination applies only to the list endpoint.hasNextPagehelper andretry.RetryWithMaxFailures/NonRetryableare reused unchanged from feat: paginate sem get pipelines to return all results within age window #247 — they are already unit-tested inapi/client/pipelines_v1_alpha_test.go(TestHasNextPage) andapi/retry/with_max_failures_test.go(TestRetryWithMaxFailures,..._NonRetryable,..._Backoff,..._MixedRetryableAndNon,..._ErrorWrapsAttemptCount).Test__ListTasks__*httpmock responders that matched task URLs exactly are switched to regexp responders that match on the project-id query param. This was needed because requests now also carrypageandpage_sizeparameters; an exact-string responder would no longer match.Test__ListTasks__MultiPagecovers the new behavior. It uses anchored regexp patterns ([?&]page=N(?:&|$)) so the page-1 responder cannot accidentally matchpage=10/page=11/etc. The test asserts both responders fire, the call returns no error, the result has length 2, and both task names from the two pages are present in order.go build ./...passes; allTest__ListTasks__*,Test__DescribeTask__*, andTest__GetTasks__*tests pass.Test plan
go build ./...succeedssem get tasks -p <project>against a project with 100+ tasks confirmed all are returned (locally-built binary returned the full task count matching the APItotalheader; the released CLI returned 30)sem get tasks <task-id>(describe) is byte-identical to the released CLI, andsem run task <task-id>still triggers a workflow successfully