fix(pagination): continue after empty cursor pages - #893
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7cb9aac2c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| func (r *NextCursorPage[T]) GetNextPage() (res *NextCursorPage[T], err error) { | ||
| if len(r.Data) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if r.JSON.HasMore.Valid() && r.HasMore == false { |
There was a problem hiding this comment.
This changes Castiron-owned shared pagination scaffolding, but the commit contains neither a generator change nor regenerated metadata, leaving the underlying template to keep emitting the faulty empty-page guard for future generations and sibling SDKs. Fix the shared Castiron template and regenerate this file rather than carrying only a repository-local patch.
AGENTS.md reference: AGENTS.md:L68-L70
Useful? React with 👍 / 👎.
| r.idx = 0 | ||
| r.page, r.err = r.page.GetNextPage() | ||
| if r.err != nil || r.page == nil || len(r.page.Data) == 0 { | ||
| if r.err != nil { |
There was a problem hiding this comment.
When an empty page returns has_more: true and repeats the same nonempty next cursor, this loop issues the identical request indefinitely inside a single Next() call; with a successful server and a context without a deadline, the caller never regains control and the client can continuously hammer the endpoint. Detect a cursor that makes no progress and terminate with an error, with a public-entrypoint regression test covering the malformed response.
AGENTS.md reference: AGENTS.md:L38-L40
Useful? React with 👍 / 👎.
|
Addressed the repeated-cursor feedback in commit a0afa6b: the auto-pager now returns an error instead of issuing the same request indefinitely, with a public HTTP regression test. The pagination implementation is Castiron-generated; this checkout does not contain the Castiron generator/template source or regeneration metadata, so I cannot make the requested upstream generator edit from this repository. The generated scaffolding change remains isolated and the focused -race tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0afa6bbad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if r.page.Next != "" && r.page.Next == r.lastCursor { | ||
| r.err = errors.New("pagination cursor did not advance") | ||
| return false |
There was a problem hiding this comment.
When the final page explicitly returns has_more:false but leaves next set to the cursor used to fetch it, this branch runs after yielding that page's items and before GetNextPage() can apply its terminal has_more check. A subsequent Next() therefore reports pagination cursor did not advance instead of clean exhaustion, regressing the existing explicit-false behavior; short-circuit terminal pages before this comparison and cover this response shape in the regression test.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
| if r.page.Next != "" && r.page.Next == r.lastCursor { | ||
| r.err = errors.New("pagination cursor did not advance") | ||
| return false | ||
| } | ||
| r.lastCursor = r.page.Next |
There was a problem hiding this comment.
When empty pages advertise a cursor cycle longer than one token—for example A, then B, then A—each adjacent token differs, so a single Next() loops indefinitely issuing alternating requests if its context has no deadline. Fresh evidence beyond the prior repeated-cursor comment is that the new implementation retains only lastCursor; track all cursors already requested and fail before reusing any of them, with an A → B → A public-entrypoint regression test.
AGENTS.md reference: AGENTS.md:L38-L40
Useful? React with 👍 / 👎.
|
Addressed the remaining pagination review feedback in the public
Validation: |
|
Correction: the commit was created locally first, but the initial push used the read-only upstream remote and returned 403. It has now been pushed successfully to the fork branch; the PR should update from the new commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1559cdd2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| err: err, | ||
| page: page, | ||
| err: err, | ||
| seenCursors: make(map[string]struct{}), |
There was a problem hiding this comment.
When ListAutoPaging starts with a nonempty After value, this map does not include the cursor used for the initial request. If that response has data, has_more:true, and echoes the same next cursor, the pager returns the first page's items, makes the identical request, and returns those items again before detecting the repetition on a later Next() call. Seed cycle detection from the initial request cursor, or track the cursor used to fetch each page, and add a public-entrypoint regression test for this resume-pagination case.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
| r.err = errors.New("pagination cursor did not advance") | ||
| return false | ||
| } | ||
| r.seenCursors[r.page.Next] = struct{}{} |
There was a problem hiding this comment.
On a successful pagination with unique cursors, every exhausted page adds another string to seenCursors, and nothing removes entries until the entire pager is discarded. This changes auto-pagination from constant auxiliary memory to memory proportional to the total page count; for example, iterating a large group or user collection with a small limit can retain millions of opaque cursors despite processing items incrementally. Restrict history to the consecutive empty-page traversal or use constant-space cycle detection so normal large iterations do not accumulate all prior cursors.
AGENTS.md reference: AGENTS.md:L197-L198
Useful? React with 👍 / 👎.
1 participant
Fixes #890
NextCursorPage stopped pagination whenever a page had an empty data array, even when has_more was true and the server supplied a next cursor. Allow GetNextPage and the auto-pager to traverse empty intermediate pages, and add a regression test using the public Admin Organization Groups endpoint.
Validation: