fix(api): follow the pagination Link URL without rebuilding the query

Description

glab api --paginate rebuilds the query string on every page, so each page after the first is requested with the caller's fields appended a second time onto the URL the server already built.

httpRequest treats an absolute path as the request URL (internal/commands/api/http.go:26) but still runs the query builder over it (http.go:40), while the pagination loop replaces the path with the Link header's rel="next" target and re-passes the same fields each iteration (api.go:362 and api.go:383). parseQuery concatenates instead of merging by key, so nothing collapses the duplicates.

For an array field the server receives a different parameter list than the caller asked for:

glab api --paginate -X GET -F 'ids=[1,2]' groups/x/issues

Page two goes out as ...?ids[]=1&ids[]=2&page=2&per_page=100&ids[]=1&ids[]=2, four values where the Link header asked for two. Each further page adds two more, because the server echoes what it received and the client appends its own copy again.

A caller-supplied page is worse, because the outcome then depends on how the server resolves a repeated key. The request settles at ?page=2&per_page=100&page=1, with the caller's value last:

glab api --paginate -X GET -f 'page=1' projects

Where a repeated key is last-wins, which is what Rack's parse_nested_query does, the server answers page one, its Link header points at page two again, and the loop does not terminate. gitlab.com does not resolve duplicates that way, so that particular command advances there. The caller's value still wins when it outranks the one the Link header asked for: -f 'page=5' answered x-page: 5 while the Link header asked for page two, so pages are skipped rather than repeated.

The fix follows the rel="next" target as given, which is what the REST documentation asks for: "Be sure to use these links instead of generating your own URLs" for offset pagination, and "You should use only the given link to retrieve the next page instead of building your own URL" for keyset. Once the loop adopts a Link URL it stops reapplying fields, and it tests the method to decide that: only a query method puts fields in the query, so only a query method has anything to stop reapplying. The one method that reaches the loop carrying fields in its body, an inferred POST, has to keep sending them on every page.

Scope beyond the minimum fix, in one line: the GET/DELETE test that decides whether fields become a query is now isQueryMethod, shared by httpRequest, the pagination loop and warnOnLegacyRawArrays so the three cannot drift apart.

GraphQL pagination is unaffected. Its path stays graphql and never becomes absolute, and its cursor is carried inside the fields map.

Reaching the duplicate-query bug at all needs an explicit -X GET. Fields with no method flag turn the request into a POST (api.go:302) and travel in the body, and validate rejects --paginate with any explicit non-GET method (api.go:266), which Test_NewCmdApi/POST pagination already pins. -X DELETE --paginate and -X POST --paginate both exit with "the '--paginate' option is not supported for non-GET requests". So a query method inside the loop means GET; isQueryMethod names DELETE for httpRequest, which serves -X DELETE without --paginate.

The method test in the guard is not redundant with that. validate runs before the method is inferred, so a --paginate call carrying fields and no method flag passes validation on the "GET" flag default, reaches the loop as a POST, and must keep re-sending its body. Dropping that body would break glab api --paginate -f a=b projects silently, so the fifth test case below pins it.

The guard tests the method and nothing else, if isQueryMethod(method). It does not also test hasNextPage: when there is no next page the loop exits before another request, so clearing the body there is unobservable and no test could pin the difference. There is no io.Reader body for the guard to protect either: --input is mutually exclusive with --paginate (api.go:247), so an input file never reaches the loop, and --form with no -X is inferred to POST (api.go:327), which the method test already covers. The one combination left is an explicit -X GET --form, and its body is the io.Pipe from http.go:183, which the first request has already drained; page two sends no payload whether the loop keeps the reader or drops it, and dropping it also stops sending the stale multipart Content-Type the drained pipe still carried.

A nearby defect is out of scope here. -f 'ids[]=1' -f 'ids[]=2' sends only the last value, which is field parsing rather than pagination, and !3776 fixes it. The behaviours are independent, but both changes declare a new symbol in the same part of http.go, so whichever lands second needs a small conflict resolution there. !3776 also carries its own GET/DELETE check that isQueryMethod would replace, so the second to land can fold it onto the helper.

Related Issues

None filed. This surfaced while working on !3776, whose description records the behaviour as observed and out of scope.

How has this been tested?

Test_apiRun_paginationREST_followsLinkURLVerbatim covers five cases: an array field, the same with ndjson output, a scalar field, a caller-supplied page, and a field with no method flag. Each asserts the exact page-two and page-three request URIs against the Link header the mock served, so the expectation is identity with the URL the server advertised rather than a value read back out of the client. Each also asserts the method, the body and the Content-Type of all three requests rather than only the first.

The two halves of the guard are pinned by different assertions, and it is worth being exact about which. The four query cases pin the query half through their URL assertions alone: because httpRequest routes a map into the query for a query method (http.go:39-41), their bodies are empty whether the guard runs or not, so their body and Content-Type assertions document the wire rather than pin the guard. The POST half is pinned by the fifth case, whose body and Content-Type assertions on pages two and three are the only assertions in the test that a dropped body can fail.

The fifth case is the inferred-POST path. It runs with the "GET" flag default and requestMethodPassed false, which is what glab api --paginate -f a=b projects produces, and Test_NewCmdApi/pagination with fields pins that validate accepts that command so the case is a path the CLI can reach.

Every page serves a distinct payload and each case asserts stdout exactly, so the test covers pages arriving at the caller in order and not only pages being requested. The ndjson case asserts {"id":1}\n{"id":2}\n{"id":3}\n where the JSON cases assert [{"id":1}][{"id":2}][{"id":3}].

Mutation results, measured on this branch with go test ./internal/commands/api/...:

Change	Failing subtests
api.go and http.go reverted to main, new test kept	4 of 5: array field, array field with ndjson output, scalar field, caller-supplied page. The fifth passes, because main never drops a body.
requestBody = nil deleted, so the body is never dropped	4: array field, array field with ndjson output, scalar field, caller-supplied page. Each fails only at its two rel="next" URL assertions.
isQueryMethod(method) removed, so the body is always dropped	1: field with no method flag, at its page-two and page-three body and Content-Type assertions.
ndjson write path at api.go:433 disabled	1 here: array field with ndjson output. The pre-existing Test_apiRun_ndjson and Test_apiRun_ndjson_pagination fail too.

The two guard mutations are complementary rather than overlapping: never dropping the body fails the four query cases and leaves the POST case passing, always dropping it fails the POST case and leaves the four query cases passing, and neither alone fails all five. The ndjson mutation is there because that subtest otherwise differed from the case above it only in a field the request path never reads.

The page case is bounded by an error response past the last canned page, so a regression shows up as a failure rather than a hang.

Test_httpRequest/DELETE with params pins isQueryMethod's DELETE arm at the httpRequest seam, where fields on a DELETE are still reachable: reducing the helper to its GET arm fails that case alone.

go test ./internal/commands/api/... passes with 0 failures and make lint reports 0 issues.

Checked against gitlab.com as well as the mocks, with read-only GETs. Duplicate keys are honoured non-trivially there, page=5&page=2 answering x-page: 5, so a stale caller page can win over the one the Link header asked for. gitlab.com resolves duplicates by sorting rather than by plain last occurrence, which is why the non-termination above is described as depending on the server.

Screenshots (if appropriate):

Not applicable.