Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions packages/pagination/pagination.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,10 +328,6 @@ func (r *NextCursorPage[T]) UnmarshalJSON(data []byte) error {
// there is no next page, this function will return a 'nil' for the page value, but
// will not return an error
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 {
return nil, nil
}
Expand Down Expand Up @@ -380,20 +376,22 @@ func NewNextCursorPageAutoPager[T any](page *NextCursorPage[T], err error) *Next
}

func (r *NextCursorPageAutoPager[T]) Next() bool {
if r.page == nil || len(r.page.Data) == 0 {
return false
}
if r.idx >= len(r.page.Data) {
for {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Stop when empty pages repeat a cursor

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 👍 / 👎.

if r.page == nil {
return false
}
if r.idx < len(r.page.Data) {
r.cur = r.page.Data[r.idx]
r.run += 1
r.idx += 1
return true
}
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 || r.page == nil {
return false
}
}
r.cur = r.page.Data[r.idx]
r.run += 1
r.idx += 1
return true
}

func (r *NextCursorPageAutoPager[T]) Current() T {
Expand Down
82 changes: 82 additions & 0 deletions pagination_next_cursor_empty_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package openai_test

import (
"context"
"errors"
"io"
"net/http"
"reflect"
"strings"
"testing"

"github.com/openai/openai-go/v3"
"github.com/openai/openai-go/v3/option"
)

func TestNextCursorPaginationFollowsEmptyPages(t *testing.T) {
tests := []struct {
name string
pages map[string]string
want []string
}{
{
name: "empty first page",
pages: map[string]string{
"": `{"data":[],"has_more":true,"next":"cursor-1"}`,
"cursor-1": `{"data":[{"id":"group-1","created_at":1,"group_type":"group","is_scim_managed":false,"name":"one"}],"has_more":false,"next":null}`,
},
want: []string{"group-1"},
},
{
name: "empty intermediate page",
pages: map[string]string{
"": `{"data":[{"id":"group-1","created_at":1,"group_type":"group","is_scim_managed":false,"name":"one"}],"has_more":true,"next":"cursor-1"}`,
"cursor-1": `{"data":[],"has_more":true,"next":"cursor-2"}`,
"cursor-2": `{"data":[{"id":"group-2","created_at":2,"group_type":"group","is_scim_managed":false,"name":"two"}],"has_more":false,"next":null}`,
},
want: []string{"group-1", "group-2"},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
calls := 0
client := openai.NewClient(
option.WithBaseURL("https://example.com/v1"),
option.WithAdminAPIKey("test-admin-key"),
option.WithMaxRetries(0),
option.WithHTTPClient(paginationHTTPDoerFunc(func(req *http.Request) (*http.Response, error) {
calls++
cursor := req.URL.Query().Get("after")
body, ok := test.pages[cursor]
if !ok {
return nil, errors.New("unexpected pagination cursor")
}
return &http.Response{
StatusCode: http.StatusOK,
Header: http.Header{"Content-Type": {"application/json"}},
Body: io.NopCloser(strings.NewReader(body)),
Request: req,
}, nil
})),
)

pager := client.Admin.Organization.Groups.ListAutoPaging(
context.Background(), openai.AdminOrganizationGroupListParams{},
)
var got []string
for pager.Next() {
got = append(got, pager.Current().ID)
}
if err := pager.Err(); err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(got, test.want) {
t.Fatalf("group IDs = %v, want %v", got, test.want)
}
if calls != len(test.pages) {
t.Fatalf("HTTP calls = %d, want %d", calls, len(test.pages))
}
})
}
}