fix(pagination): follow next cursors through empty pages - #891
fix(pagination): follow next cursors through empty pages#891sylvesterkaczmarek wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 19f35b11d9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false | ||
| } | ||
| if r.idx >= len(r.page.Data) { | ||
| for { |
There was a problem hiding this comment.
When the server, including a custom endpoint, returns an empty page with has_more: true and repeatedly supplies the same non-empty next cursor, this loop never returns from a single Next() call: every GetNextPage() succeeds with another empty page and the loop immediately requests it again. The removed empty-page guard previously bounded this case, whereas the new behavior can hang and issue unbounded HTTP requests when the context has no deadline; track visited cursors, or at least stop on a non-advancing cursor, while continuing through distinct empty pages.
Useful? React with 👍 / 👎.
|
Addressed the repeated-cursor case. The auto-pager now tracks cursors it has already followed and stops before issuing another request when a cursor repeats, while still traversing distinct empty pages. Added a regression with an empty page that returns the same next cursor. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0eddbd65c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| idx int | ||
| run int | ||
| err error | ||
| seenCursors map[string]struct{} |
There was a problem hiding this comment.
When downstream code instantiates this public generic with a comparable item type and compares concrete pager values or uses them as map keys, adding the map field makes NextCursorPageAutoPager[T] non-comparable, so code that compiled against the previous release now fails to compile. Keep the cursor set behind a comparable indirection (for example, a pointer to the map) so cycle detection does not change the exported type's comparability.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
|
Addressed the comparability regression. Cursor tracking now sits behind a pointer to the private map, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348cafaeb7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| idx int | ||
| run int | ||
| err error | ||
| seenCursors *map[string]struct{} |
There was a problem hiding this comment.
This patches Castiron-owned pagination scaffolding only in the generated SDK, so Castiron's canonical output still contains the empty-page defect and future fresh generations or other generated SDKs will continue to emit it; the local repository must also carry this as a permanent custom patch. Fix the shared pagination template and regenerate this file rather than addressing the recurring defect solely in generated output.
AGENTS.md reference: AGENTS.md:L68-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. packages/pagination/pagination.go is Castiron-owned, and the canonical pagination template is not present in this repository or in any Castiron repository accessible through this account. The SDK branch therefore cannot honestly claim a generator-complete fix. I’m leaving the current patch and regressions as the concrete reproducer/SDK-side implementation; a durable fix requires the same change in Castiron followed by regeneration. Maintainers, please advise whether you want this PR retained for that port or closed in favor of a generator-side change.
1 participant
Summary
Make
NextCursorPagepagination follow explicit server-providednextcursors through empty first or intermediate pages.Previously, both
NextCursorPage.GetNextPage()andNextCursorPageAutoPager.Next()stopped solely because the currentdataarray was empty, before consultinghas_moreornext. Public auto-pagination such asAdmin.Organization.Groups.ListAutoPagingcould therefore silently miss later results.GetNextPage()now lets the explicit cursor metadata decide whether another page exists, and the auto-pager loops across empty pages until it either finds data, encounters an error, or reaches a terminal page. Populated-page behavior is unchanged.Testing
Groups.ListAutoPagingcoverage for empty first and intermediate pagesSKIP_MOCK_TESTS=true go test ./...— passed on Go 1.26.7./scripts/check-go-mod— all four modules tidy./scripts/lint— all analyzer passes reported 0 issuesgit diff --check— passedFixes #890.