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