Skip to content

fix(pagination): iterator paged whichever array the spec declared first - #66

Open
giraffesyo wants to merge 1 commit into
canaryfrom
pagination-items-guess
Open

fix(pagination): iterator paged whichever array the spec declared first#66
giraffesyo wants to merge 1 commit into
canaryfrom
pagination-items-guess

Conversation

@giraffesyo

Copy link
Copy Markdown
Member

Closes #65.

The iterator could page the wrong array

findItemsField fell back to the first array-typed field when no field was named items, data, or results:

EventPage:
  properties:
    cursor:   { type: string }
    warnings: { type: array, items: { type: string } }
    events:   { type: array, items: { $ref: "#/components/schemas/Event" } }
func (c *Client) ListEventsIter(...) *PageIterator[string] {
	return result.Warnings, next, nil     // before
}

It compiled, it ran, and it returned the wrong data. Which array won was decided by property order in the spec.

An array is the page when one of three things holds:

  1. its name is items, data, or results, as before,
  2. it is the only array in the response, or
  3. it is the only array of a type the spec declares, beside arrays of scalars.

Rule 3 is what keeps the case above working, and it now yields PageIterator[Event] over result.Events. Two arrays of records identify nothing, and there the operation gets no iterator at all. That is not a loss: the plain ListEvents method still returns the whole page, and an iterator over the wrong field is worse than no iterator.

The iterator could loop forever

cursorFieldNames includes bare cursor, and in a response that is as often the echo of the request cursor as the next one. Next stopped only on an empty cursor, so an echoing server turned All() into an endless run of identical requests.

if next == "" || next == it.nextCursor {
	it.done = true
}

A cursor that does not advance means the page did not advance, whatever the field is called, so this also covers a server that pins its cursor on the last page rather than clearing it. Bare cursor stays in the name list: APIs do use it correctly, and the guard bounds the damage when they do not.

Tests

  • internal/analyzer/pagination_test.go: conventional name wins, lone array is the page, record array beats scalar arrays, two record arrays get no iterator, no array gets no iterator.
  • internal/generator/e2e_pagination_test.go: compiles and runs the generated iterator against a fetch that always returns the same cursor, asserting it stops after the repeat, and against an advancing cursor, asserting it still pages to the end and sends each cursor in turn.

gofmt, go vet ./..., and go test ./... pass.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Paginated iterator can page the wrong array and loop forever on an echoed cursor

1 participant