feat(table): cursor keyset continuation in ListPaged#56
Conversation
1332d0a to
f8f081c
Compare
| // direction (empty means Asc; any other non-Asc/Desc value is an error). | ||
| // Forward-only and stable under concurrent writes, unlike ListPaged; IDColumn | ||
| // must be unique for the ordering to be stable. | ||
| func (t *Table[T, P, I]) ListCursor(ctx context.Context, where sq.Sqlizer, page *Page, order Order) ([]P, *Page, error) { |
There was a problem hiding this comment.
- Why do we need additional method?
- Why can't we have
ListPagedhandle page differently depending onPaginator? (page based / cursor based) - What happens if you pass cursor page to ListPaged? What happens if you pass page based page to ListCursor?
I believe the complexity may be hidden here. We do not need to expose user to this decision making and costly mistakes.
There was a problem hiding this comment.
Reworked: ListCursor is gone and ListPaged now handles both modes (c0cc5f5). Taking the questions in order:
1 + 2. Agreed on one method, with one change to the mechanism: dispatch keys on the request (the Page), not on the table's Paginator config. Config-based dispatch makes the same Page mean different things on differently-configured tables — Page: 5 against a cursor-configured table either silently becomes forward-only or errors at runtime — which is exactly the costly-mistake class this should kill. It also doesn't compose: CursorPaginator's cursor type can't see instance state like t.IDColumn. So: a Page carrying a Cursor continues a keyset walk over IDColumn; anything else is offset pagination, and id-ordered offset pages (the default order) now return NextCursor. Callers start with a plain Page{Size: n} and continue by cursor — no mode to pick, no new method.
3. Both crossings are now well-defined: a cursor page just works; a page-number page works and also hands back NextCursor. The ambiguous combination (Cursor + Page > 1) returns ErrCursorPaged instead of guessing, an order that contradicts the cursor returns ErrCursorPageOrdered, and the cursor encodes its own direction — validated strictly on decode, so a forged or corrupted token ("order":"sideways") returns ErrInvalidCursor rather than silently walking Asc. For reference, master has the silent case today: ListPaged ignores Page.Cursor and offset-paginates it.
PR description is updated to match.
f8f081c to
f753317
Compare
f753317 to
574c38c
Compare
- Order.IsValid() - strict check (exactly Asc/Desc) - Order.Sanitize() - lenient normalize (case + whitespace, unknown -> Asc); the Order type owns its own normalization rules - Sort.sanitize delegates direction handling to Order.Sanitize (was an inline switch); behavior identical - Page.SetDefaults zero-checks collapse to cmp.Or
- a Page carrying a Cursor continues a keyset walk over IDColumn: forward-only, stable under concurrent writes; anything else is offset-paginated as before - offset pages ordered exactly by IDColumn (the default) populate NextCursor, so callers start with a plain Page and continue by cursor - the cursor encodes its direction; a forged or missing direction in the token returns ErrInvalidCursor (cursors are minted, not user input), a conflicting page order returns ErrCursorPageOrdered, and cursor + page number > 1 returns ErrCursorPaged - PrepareResult owns resetting NextCursor (both paginators), so round-tripping the returned Page object never leaks a stale cursor; fixes the same leak in CursorPaginator.PrepareResult from #55
574c38c to
c0cc5f5
Compare
marino39
left a comment
There was a problem hiding this comment.
Review: feat(table): cursor keyset continuation in ListPaged
1. The default paginator is implicit — make it explicit on Table
ListPaged decides the pagination mode by inspecting page.Cursor at call time. This means the
default (offset pagination) is encoded as an undocumented fallback inside a method, not as a
first-class property of Table. A nil or empty Page will always silently offset-paginate, even
on a table where the caller expects keyset behaviour. The mode should live on the table, not inside
a runtime if-branch.
Suggested fix: add a DefaultPaginator field to Table and set it to PageBased out of the box.
The method then reads Table.DefaultPaginator when the page gives no signal.
type PaginatorKind int
const (
PageBased PaginatorKind = iota // default
CursorBased
)
type Table[T, P, I any] struct {
// ...existing fields...
DefaultPaginator PaginatorKind
IDColumn string
}
// caller opts into keyset-first behaviour:
table := pgkit.NewTable[Widget, *Widget, int64](db, pgkit.TableConfig{
DefaultPaginator: pgkit.CursorBased,
IDColumn: "id",
})When page is nil or carries neither Cursor nor Page > 1, ListPaged falls back to
table.DefaultPaginator instead of hardcoding offset. This makes the table self-documenting and
removes the hidden assumption.
2. Page mode is inferred ad-hoc — centralise it in a Type() method
Page carries fields for both pagination modes and ListPaged currently infers which mode is
active by inspecting page.Cursor inline. That inference is scattered: every caller, every
paginator, and every test has to repeat the same field-presence check. An "impossible" combination
like {Page: 3, Cursor: "..."} has to be caught at runtime with ErrCursorPaged because there
is no single place that owns the decision.
Suggested fix: add a Type() method to Page that derives the mode from the populated fields.
No new discriminant field needed — the fields themselves are the signal.
type PaginatorKind int
const (
Unknown PaginatorKind = iota
PageBased
CursorBased
)
func (p *Page) Type() PaginatorKind {
if p == nil || (p.Cursor == "" && p.Number == 0) {
return Unknown // caller must fall back to table's DefaultPaginator
}
if p.Cursor != "" {
return CursorBased
}
return PageBased
}SetDefaults then takes the table's DefaultPaginator as a fallback and applies the right
field defaults for the resolved kind:
func (p *Page) SetDefaults(defaultKind PaginatorKind) {
kind := p.Type()
if kind == PageBased && defaultKind == CursorBased {
kind = CursorBased
}
if p.Size == 0 {
p.Size = defaultPageSize
}
if kind == PageBased && p.Number == 0 {
p.Number = 1
}
}ListPaged then calls page.Type() once for dispatch — no inline field inspection anywhere else.
3. ListPaged reimplements cursor logic — add WithCursorFactory option and implement TableCursor
The keyset query-building (WHERE id > ? / WHERE id < ?, LIMIT, NextCursor minting) is
inlined directly in Table.ListPaged. The root cause is that CursorPaginator[T, C, PC]
constructs its cursor internally from the type parameter PC with no way to supply runtime
state like IDColumn. The fix is a WithCursorFactory constructor option that lets callers
provide a func() PC — defaulting to new(C) when absent so existing callers are unaffected —
plus a TableCursor implementation in the table package.
Suggested fix:
// paginator/cursor.go
// WithCursorFactory overrides the default new(C) cursor construction.
// Use when the cursor type needs runtime state that cannot come from zero-value init.
func WithCursorFactory[T any, C any, PC Cursor[C, T]](f func() PC) PaginatorOption {
return func(s *PaginatorSettings) {
s.CursorFactory = func() any { return f() }
}
}
// Inside CursorPaginator, cursor construction becomes:
func (p *CursorPaginator[T, C, PC]) newCursor() PC {
if p.settings.CursorFactory != nil {
return p.settings.CursorFactory().(PC)
}
return new(C) // existing default behaviour
}// table/table_cursor.go
// TableCursor implements Cursor[I, T] for a table's IDColumn.
// Apply and OrderBy are the only ID-column-specific pieces;
// everything else is handled by CursorPaginator.
type TableCursor[I any] struct {
IDColumn string
id I
order paginator.Order
}
func (c *TableCursor[I]) Apply(q sq.SelectBuilder) sq.SelectBuilder {
if c.order == paginator.Desc {
return q.Where(sq.Lt{c.IDColumn: c.id})
}
return q.Where(sq.Gt{c.IDColumn: c.id})
}
func (c *TableCursor[I]) OrderBy() []string {
return []string{fmt.Sprintf("%s %s", c.IDColumn, c.order)}
}
func (c *TableCursor[I]) From(row T) (string, error) { /* encode c.id + c.order */ }
func (c *TableCursor[I]) Decode(encoded string) error { /* populate c.id + c.order */ }// table/table.go
type Table[T, P, I any] struct {
// ...existing fields...
Paginator Paginator
cursorPaginator paginator.CursorPaginator[T, I, *TableCursor[I]] // created once in constructor
DefaultPaginator PaginatorKind
IDColumn string
}
func NewTable[T, P, I any](db DB, cfg TableConfig) *Table[T, P, I] {
t := &Table[T, P, I]{
IDColumn: cfg.IDColumn,
DefaultPaginator: cfg.DefaultPaginator,
// ...existing init...
}
t.cursorPaginator = paginator.NewCursorPaginator[T, I, *TableCursor[I]](
paginator.WithCursorFactory(func() *TableCursor[I] {
return &TableCursor[I]{IDColumn: t.IDColumn}
}),
)
return t
}
func (t *Table[T, P, I]) ListPaged(ctx context.Context, where sq.Sqlizer, page *Page) ([]P, *Page, error) {
page = t.prepPage(page) // apply defaults using page.Type() + t.DefaultPaginator
var p Paginator
switch page.Type() {
case CursorBased:
p = t.cursorPaginator
default:
p = t.Paginator
}
return p.Paginate(ctx, t.db, where, page)
}TableCursor owns the four ID-column-specific methods. WithCursorFactory is the only addition
to CursorPaginator — a nil-safe option that existing callers never need to set. Table wires
them in NewTable with no intermediate type. ListPaged is a pure dispatcher with no inlined
query logic and no per-call allocations.
CursorPaginator(#55) gives keyset pagination but makes every consumer hand-write aCursortype —Apply/From/OrderBy— even for the overwhelmingly common case of "page by the table's id." This PR folds that case intoListPageditself, so callers never pick a pagination mode: aPagecarrying aCursorcontinues a keyset walk over the table'sIDColumn, anything else is offset pagination, and id-ordered offset pages hand back aNextCursorto opt in.(Earlier revision exposed this as a separate
ListCursormethod — reworked per review so the method choice, and the mixed-Pagemistakes it invited, no longer exist.)refactor(page)—Order.IsValid/Order.SanitizeOrder.IsValid()— strict check (exactlyAsc/Desc).Order.Sanitize()— lenient normalize (case + whitespace, unknown →Asc); theOrdertype now owns its own normalization rules.Sort.sanitizedelegates direction handling to it (was an inlineswitch); behavior-identical, still case-insensitive and defaulting.Page.SetDefaultszero-checks collapse tocmp.Or.feat(table)— cursor keyset continuation inListPagedSignature is unchanged:
Semantics:
IDColumn(the default when no sort is given), pages with more rows populatepage.NextCursor.Pagecarrying thatCursorcontinues as a keyset walk overIDColumn: forward-only, no random page access, but pages never skip or duplicate rows under concurrent writes. The first page of a walk is identical in both modes, so callers start with a plainPage{Size: n}and continue withPage{Size: n, Cursor: prev.NextCursor}— or just round-trip the returned*Page(More/NextCursorare reset every call, so reuse is safe). ResettingNextCursorlives inPrepareResult— the method that owns the page's output fields — which also fixes a latent leak inCursorPaginator.PrepareResultfrom feat: CursorPaginator for keyset pagination #55: it previously left a reused page's staleNextCursorin place on the final page.{"id": ..., "order": "DESC"}, opaque base64-JSON viaEncodeCursor), so a bare{Cursor: next}page cannot silently flip a Desc walk to Asc. Direction is chosen on the first page through the normal order vocabulary (Sort: [{id DESC}]/Column: "-id").ErrCursorPageOrdered(from feat: CursorPaginator for keyset pagination #55)Page > 1→ErrCursorPaged(new sentinel;Page == 1stays allowed becausePrepareResultwrites it into round-tripped pages)ASC/DESC→ErrInvalidCursor. Cursors are minted byListPaged, not user input — a forged"order":"sideways"is rejected, not coerced toAscthe waySortinput is.Notes
ListPaged: it previously ignoredPage.Cursor; now it acts on it, and it populatesNextCursorit never set before. The old behavior was itself a hazard — aPagecarrying a cursor was silently offset-paginated.Tableis markedNOTICE: Experimental, so no compatibility ceremony.Pageand not on the table'sPaginatorconfig: config-based dispatch makes the samePagemean different things on differently-configured tables (Page: 5against a cursor-configured table must either silently become forward-only or error at runtime), andCursorPaginator's cursor type can't see instance state liket.IDColumnanyway. Keying on the request keeps it self-describing.IDColumnto be unique — true for a primary key. Keyset by non-id columns stays out of scope (needs a forced tiebreaker);CursorPaginatorcovers that case.Test plan
TestTableListPagedCursor(tests/cursor_test.go):Lt/Gtbranches plus the offset→keysetNextCursorhandoff).*Pageobject across the whole walk and asserts the final page leaks no stale cursor;TestCursorPaginatorPaginateReturnsPagegot the same final-page assertion for theCursorPaginatorfix (verified red without the fix, green with it).ErrCursorPageOrdered, both wrong direction and wrong column), cursor + page number (ErrCursorPaged), undecodable cursor and forged cursor order —"sideways", lowercase, empty — (ErrInvalidCursor, verified red without the validation); non-id ordering emits no cursor.make test-all, 6 packages ok, 0 failures);go build/go vet/gofmt -lclean.