From 265cf1b6aabc510af9303d0cb92282cbfdb5bec5 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Wed, 19 Aug 2026 19:07:51 -0500 Subject: [PATCH] fix(pagination): iterator paged whichever array the spec declared first A page type with no field named items, data, or results fell back to the first array-typed field, so a response holding warnings alongside events produced an iterator over the warnings. It compiled, it ran, and it returned the wrong data, decided by property order in the spec. An array is now the page when its name says so, when it is the only array, or when it is the only array of a declared type beside arrays of scalars. Two arrays of records identify nothing, and the operation gets no iterator rather than a guess: the plain method still returns the whole page. The iterator also stops when the cursor comes back unchanged. A response field named cursor is as often the echo of the request cursor as the next one, and All() turned that into an endless run of identical requests. --- internal/analyzer/pagination.go | 54 ++++++++++---- internal/analyzer/pagination_test.go | 89 ++++++++++++++++++++++ internal/generator/e2e_pagination_test.go | 90 +++++++++++++++++++++++ internal/templates/pagination.go.tmpl | 5 +- 4 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 internal/generator/e2e_pagination_test.go diff --git a/internal/analyzer/pagination.go b/internal/analyzer/pagination.go index 8f35eef..c9477ee 100644 --- a/internal/analyzer/pagination.go +++ b/internal/analyzer/pagination.go @@ -66,8 +66,10 @@ func (a *Analyzer) detectCursorPagination(op *ir.OperationDef, pkg *ir.Package) return nil } - // Find the items field. - items := findItemsField(respType) + items := findItemsField(ir.TypesByName(pkg.Types), respType) + if items.name == "" { + return nil + } return &ir.PaginationDef{ Style: ir.PaginationStyleCursor, @@ -120,9 +122,12 @@ func (a *Analyzer) detectOffsetPagination(op *ir.OperationDef, pkg *ir.Package) // buildOffsetPagination builds an offset-style PaginationDef. func (a *Analyzer) buildOffsetPagination(op *ir.OperationDef, pkg *ir.Package, offsetParam, limitParam string) *ir.PaginationDef { respType := a.findSuccessResponseType(op, pkg) - var items itemsFieldInfo - if respType != nil { - items = findItemsField(respType) + if respType == nil { + return nil + } + items := findItemsField(ir.TypesByName(pkg.Types), respType) + if items.name == "" { + return nil } return &ir.PaginationDef{ @@ -154,22 +159,45 @@ type itemsFieldInfo struct { } // findItemsField finds the name and element type of the array-typed field in a -// response type that contains the paginated items. -func findItemsField(td *ir.TypeDef) itemsFieldInfo { +// response type that contains the paginated items. It returns the zero value +// when nothing identifies one, which leaves the operation without an iterator +// rather than paging over whichever array the spec happens to declare first. +func findItemsField(byName map[string]*ir.TypeDef, td *ir.TypeDef) itemsFieldInfo { + var arrays []*ir.Field for _, f := range td.Fields { - if containsCI(itemsFieldNames, f.JSONName) && strings.HasPrefix(f.Type, "[]") { - return itemsFieldInfo{name: f.JSONName, elemType: f.Type[2:]} + if !strings.HasPrefix(f.Type, "[]") { + continue } - } - // Fallback: find any array-typed field. - for _, f := range td.Fields { - if strings.HasPrefix(f.Type, "[]") { + if containsCI(itemsFieldNames, f.JSONName) { return itemsFieldInfo{name: f.JSONName, elemType: f.Type[2:]} } + arrays = append(arrays, f) + } + // One array is the page by elimination. + if len(arrays) == 1 { + return itemsFieldInfo{name: arrays[0].JSONName, elemType: arrays[0].Type[2:]} + } + // A page holds records, so an array of a declared type beside arrays of + // scalars is still unambiguous. Two arrays of records are not, and guessing + // there returns the wrong data instead of failing. + if records := recordArrays(byName, arrays); len(records) == 1 { + return itemsFieldInfo{name: records[0].JSONName, elemType: records[0].Type[2:]} } return itemsFieldInfo{} } +// recordArrays returns the array fields whose element type is a type the spec +// declares rather than a builtin. +func recordArrays(byName map[string]*ir.TypeDef, arrays []*ir.Field) []*ir.Field { + var records []*ir.Field + for _, f := range arrays { + if byName[ir.NamedType(f.Type[2:])] != nil { + records = append(records, f) + } + } + return records +} + // containsCI checks if any element in the list matches the target (case-insensitive). func containsCI(list []string, target string) bool { lower := strings.ToLower(target) diff --git a/internal/analyzer/pagination_test.go b/internal/analyzer/pagination_test.go index 1907efb..007108a 100644 --- a/internal/analyzer/pagination_test.go +++ b/internal/analyzer/pagination_test.go @@ -200,3 +200,92 @@ func TestContainsCI(t *testing.T) { } } } + +// itemsSpec builds a cursor-paginated operation whose page type declares the +// given properties, so each case differs only in what the response holds. +func itemsSpec(properties string) string { + return `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /events: + get: + operationId: listEvents + parameters: + - { name: cursor, in: query, schema: { type: string } } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/EventPage" } +components: + schemas: + EventPage: + type: object + properties: + nextCursor: { type: string } +` + properties + ` + Event: + type: object + properties: { id: { type: string } } +` +} + +func paginationOf(t *testing.T, spec string) *ir.PaginationDef { + t.Helper() + pkg, _ := analyzeSpec(t, spec) + for _, op := range pkg.Operations { + if op.Name == "ListEvents" { + return op.Pagination + } + } + t.Fatal("ListEvents operation not found") + return nil +} + +func TestPaginationItems_ConventionalNameWins(t *testing.T) { + pd := paginationOf(t, itemsSpec(` data: { type: array, items: { $ref: "#/components/schemas/Event" } } + warnings: { type: array, items: { type: string } }`)) + + if pd == nil || pd.ItemsField != "data" { + t.Errorf("items field = %+v, want data", pd) + } +} + +func TestPaginationItems_LoneArrayIsThePage(t *testing.T) { + pd := paginationOf(t, itemsSpec(` events: { type: array, items: { $ref: "#/components/schemas/Event" } }`)) + + if pd == nil || pd.ItemsField != "events" || pd.ItemsType != "Event" { + t.Errorf("items field = %+v, want events of Event", pd) + } +} + +// A page holds records, so an array of a declared type beside an array of +// scalars still identifies itself. +func TestPaginationItems_RecordArrayBeatsScalarArrays(t *testing.T) { + pd := paginationOf(t, itemsSpec(` warnings: { type: array, items: { type: string } } + events: { type: array, items: { $ref: "#/components/schemas/Event" } }`)) + + if pd == nil || pd.ItemsField != "events" { + t.Errorf("items field = %+v, want events rather than the first array declared", pd) + } +} + +// Two arrays of records identify nothing. Paging over whichever the spec +// declares first returns the wrong data, so the operation gets no iterator. +func TestPaginationItems_AmbiguousArraysGetNoIterator(t *testing.T) { + pd := paginationOf(t, itemsSpec(` warnings: { type: array, items: { $ref: "#/components/schemas/Event" } } + events: { type: array, items: { $ref: "#/components/schemas/Event" } }`)) + + if pd != nil { + t.Errorf("pagination = %+v, want none: neither array identifies the page", pd) + } +} + +func TestPaginationItems_NoArrayGetsNoIterator(t *testing.T) { + pd := paginationOf(t, itemsSpec(` total: { type: integer }`)) + + if pd != nil { + t.Errorf("pagination = %+v, want none: the response holds no page", pd) + } +} diff --git a/internal/generator/e2e_pagination_test.go b/internal/generator/e2e_pagination_test.go new file mode 100644 index 0000000..3216459 --- /dev/null +++ b/internal/generator/e2e_pagination_test.go @@ -0,0 +1,90 @@ +package generator + +import "testing" + +const cursorPageSpec = `openapi: 3.1.0 +info: { title: events, version: "1" } +paths: + /events: + get: + operationId: listEvents + parameters: + - { name: cursor, in: query, schema: { type: string } } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/EventPage" } +components: + schemas: + EventPage: + type: object + properties: + cursor: { type: string } + events: { type: array, items: { $ref: "#/components/schemas/Event" } } + Event: + type: object + properties: + id: { type: string } +` + +// TestE2E_IteratorStopsOnANonAdvancingCursor covers a server that echoes the +// cursor it was given, which a response field named `cursor` commonly is. The +// iterator stopped only on an empty cursor, so All() ran identical requests +// forever. +func TestE2E_IteratorStopsOnANonAdvancingCursor(t *testing.T) { + files, _ := generateFromSpec(t, cursorPageSpec, "eventsapi") + + runGeneratedWireTest(t, files, "cursorpage", `package eventsapi + +import "testing" + +func TestEchoedCursorTerminates(t *testing.T) { + calls := 0 + it := &PageIterator[Event]{ + fetch: func(cursor string) ([]Event, string, error) { + calls++ + if calls > 100 { + t.Fatal("iterator never stopped on a cursor that does not advance") + } + return []Event{{}}, "same-cursor", nil + }, + } + + items, err := it.All() + if err != nil { + t.Fatalf("All: %v", err) + } + if calls != 2 { + t.Errorf("fetched %d pages, want 2: one page, then the repeat that ends it", calls) + } + if len(items) != 2 { + t.Errorf("items = %d, want the 2 pages it did fetch", len(items)) + } +} + +func TestAdvancingCursorPagesToTheEnd(t *testing.T) { + pages := map[string]string{"": "b", "b": "c", "c": ""} + var seen []string + + it := &PageIterator[Event]{ + fetch: func(cursor string) ([]Event, string, error) { + seen = append(seen, cursor) + return []Event{{}}, pages[cursor], nil + }, + } + + items, err := it.All() + if err != nil { + t.Fatalf("All: %v", err) + } + if len(items) != 3 { + t.Errorf("items = %d, want 3", len(items)) + } + if len(seen) != 3 || seen[1] != "b" || seen[2] != "c" { + t.Errorf("cursors sent = %v, want the empty first page then b and c", seen) + } +} +`) +} diff --git a/internal/templates/pagination.go.tmpl b/internal/templates/pagination.go.tmpl index 45afab2..5260c74 100644 --- a/internal/templates/pagination.go.tmpl +++ b/internal/templates/pagination.go.tmpl @@ -21,7 +21,10 @@ func (it *PageIterator[T]) Next() ([]T, error) { if err != nil { return nil, err } - if next == "" { + // A cursor that comes back unchanged means the page did not advance, which + // an echoed request cursor would otherwise turn into an endless run of + // identical requests. + if next == "" || next == it.nextCursor { it.done = true } it.nextCursor = next