From eeecbb39902b72ae771f80b03dfe99532de6bef1 Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Fri, 25 Nov 2022 13:42:08 +0100 Subject: [PATCH 01/10] feat: add additional column ordering to keysetpagination --- pagination/keysetpagination/paginator.go | 60 +++++++++++++++++-- pagination/keysetpagination/paginator_test.go | 21 ++++++- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/pagination/keysetpagination/paginator.go b/pagination/keysetpagination/paginator.go index d8483871..e3bc6b53 100644 --- a/pagination/keysetpagination/paginator.go +++ b/pagination/keysetpagination/paginator.go @@ -5,16 +5,23 @@ package keysetpagination import ( "fmt" + "strings" "github.com/gobuffalo/pop/v6" ) type ( - Item interface{ PageToken() string } + Item interface{ PageToken() string } + + columnOrdering struct { + name string + order string + } Paginator struct { token, defaultToken string size, defaultSize, maxSize int isLast bool + additionalColumn columnOrdering } Option func(*Paginator) *Paginator ) @@ -40,6 +47,25 @@ func (p *Paginator) Size() int { return size } +func parseToken(idField string, s string) map[string]string { + tokens := strings.Split(s, "/") + if len(tokens) != 2 { + return map[string]string{idField: s} + } + + r := map[string]string{} + + for _, p := range tokens { + parts := strings.Split(p, "=") + if len(parts) != 2 { + continue + } + r[parts[0]] = parts[1] + } + + return r +} + func (p *Paginator) IsLast() bool { return p.isLast } @@ -61,12 +87,28 @@ func (p *Paginator) ToOptions() []Option { // q := c.Where("foo = ?", foo).Scope(keysetpagination.Paginate[Item](paginator)) func Paginate[I Item](p *Paginator) pop.ScopeFunc { var item I - id := (&pop.Model{Value: item}).IDField() + model := &pop.Model{Value: item} + id := model.IDField() return func(q *pop.Query) *pop.Query { eid := q.Connection.Dialect.Quote(id) + + tokenParts := parseToken(id, p.Token()) + idValue := tokenParts[id] + if column, ok := model.Columns().Cols[p.additionalColumn.name]; ok { + quoteName := q.Connection.Dialect.Quote(column.Name) + + value := tokenParts[column.Name] + + q = q. + Where(fmt.Sprintf("%s > ? OR (%s = ? AND %s > ?)", quoteName, quoteName, eid), value, value, idValue). + Order(fmt.Sprintf("%s %s", quoteName, p.additionalColumn.order)) + } else { + q = q. + Where(fmt.Sprintf(`%s > ?`, eid), idValue) + } return q. - Limit(p.Size()+1). - Where(fmt.Sprintf(`%s > ?`, eid), p.Token()). + Limit(p.Size() + 1). + // we always need to order by the id field last Order(fmt.Sprintf(`%s ASC`, eid)) } } @@ -127,6 +169,16 @@ func WithSize(size int) Option { } } +func WithColumn(name string, order string) Option { + return func(opts *Paginator) *Paginator { + opts.additionalColumn = columnOrdering{ + name: name, + order: order, + } + return opts + } +} + func withIsLast(isLast bool) Option { return func(opts *Paginator) *Paginator { opts.isLast = isLast diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index 3b63af43..6b85dd9f 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -14,7 +14,8 @@ import ( ) type testItem struct { - ID string `db:"pk"` + ID string `db:"pk"` + CreatedAt string `db:"created_at"` } func (t testItem) PageToken() string { @@ -32,7 +33,7 @@ func TestPaginator(t *testing.T) { q = q.Scope(Paginate[testItem](paginator)) sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) - assert.Equal(t, "SELECT test_items.pk FROM test_items AS test_items WHERE \"pk\" > $1 ORDER BY \"pk\" ASC LIMIT 11", sql) + assert.Equal(t, "SELECT test_items.created_at, test_items.pk FROM test_items AS test_items WHERE \"pk\" > $1 ORDER BY \"pk\" ASC LIMIT 11", sql) assert.Equal(t, []interface{}{"token"}, args) }) @@ -46,7 +47,7 @@ func TestPaginator(t *testing.T) { q = q.Scope(Paginate[testItem](paginator)) sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) - assert.Equal(t, "SELECT test_items.pk FROM test_items AS test_items WHERE `pk` > ? ORDER BY `pk` ASC LIMIT 11", sql) + assert.Equal(t, "SELECT test_items.created_at, test_items.pk FROM test_items AS test_items WHERE `pk` > ? ORDER BY `pk` ASC LIMIT 11", sql) assert.Equal(t, []interface{}{"token"}, args) }) @@ -156,3 +157,17 @@ func TestParse(t *testing.T) { require.ErrorIs(t, err, strconv.ErrSyntax) }) } + +func TestPaginateWithAdditionalColumn(t *testing.T) { + c, err := pop.NewConnection(&pop.ConnectionDetails{ + URL: "postgres://foo.bar", + }) + require.NoError(t, err) + q := pop.Q(c) + paginator := GetPaginator(WithSize(10), WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "DESC")) + q = q.Scope(Paginate[testItem](paginator)) + + sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) + assert.Equal(t, "SELECT test_items.created_at, test_items.pk FROM test_items AS test_items WHERE \"created_at\" > $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" DESC, \"pk\" ASC LIMIT 11", sql) + assert.Equal(t, []interface{}{"timestamp", "timestamp", "token_value"}, args) +} From 551ffb438eebacb7f670be70aa566879bcd6924a Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Mon, 28 Nov 2022 10:17:52 +0100 Subject: [PATCH 02/10] chore: add tests --- pagination/keysetpagination/paginator.go | 79 ++++++++++++++----- pagination/keysetpagination/paginator_test.go | 54 +++++++++++-- 2 files changed, 108 insertions(+), 25 deletions(-) diff --git a/pagination/keysetpagination/paginator.go b/pagination/keysetpagination/paginator.go index e3bc6b53..f5a3885c 100644 --- a/pagination/keysetpagination/paginator.go +++ b/pagination/keysetpagination/paginator.go @@ -4,18 +4,22 @@ package keysetpagination import ( + "errors" "fmt" "strings" "github.com/gobuffalo/pop/v6" + "github.com/gobuffalo/pop/v6/columns" ) type ( Item interface{ PageToken() string } + Order string + columnOrdering struct { name string - order string + order Order } Paginator struct { token, defaultToken string @@ -26,6 +30,24 @@ type ( Option func(*Paginator) *Paginator ) +var ErrUnknownOrder = errors.New("unknown order") + +const ( + OrderDescending Order = "DESC" + OrderAscending Order = "ASC" +) + +func (o Order) extract() (string, string, error) { + switch o { + case OrderAscending: + return ">", string(o), nil + case OrderDescending: + return "<", string(o), nil + default: + return "", "", ErrUnknownOrder + } +} + func (p *Paginator) Token() string { if p.token == "" { return p.defaultToken @@ -56,11 +78,9 @@ func parseToken(idField string, s string) map[string]string { r := map[string]string{} for _, p := range tokens { - parts := strings.Split(p, "=") - if len(parts) != 2 { - continue + if columnName, value, found := strings.Cut(p, "="); found { + r[columnName] = value } - r[parts[0]] = parts[1] } return r @@ -77,10 +97,40 @@ func (p *Paginator) ToOptions() []Option { WithDefaultToken(p.defaultToken), WithDefaultSize(p.defaultSize), WithMaxSize(p.maxSize), + WithColumn(p.additionalColumn.name, p.additionalColumn.order), withIsLast(p.isLast), } } +func (p *Paginator) multipleOrderFieldsQuery(q *pop.Query, idField string, cols map[string]*columns.Column, quote func(string) string) error { + column, ok := cols[p.additionalColumn.name] + if !ok { + return errors.New("column not supported") + } + + tokenParts := parseToken(idField, p.Token()) + idValue := tokenParts[idField] + + quoteName := quote(column.Name) + + value, ok := tokenParts[column.Name] + + if !ok { + return errors.New("no value provided for " + column.Name) + } + + sign, keyword, err := p.additionalColumn.order.extract() + if err != nil { + return err + } + + q. + Where(fmt.Sprintf("%s %s ? OR (%s = ? AND %s > ?)", quoteName, sign, quoteName, quote(idField)), value, value, idValue). + Order(fmt.Sprintf("%s %s", quoteName, keyword)) + + return nil +} + // Paginate returns a function that paginates a pop.Query. // Usage: // @@ -92,20 +142,11 @@ func Paginate[I Item](p *Paginator) pop.ScopeFunc { return func(q *pop.Query) *pop.Query { eid := q.Connection.Dialect.Quote(id) - tokenParts := parseToken(id, p.Token()) - idValue := tokenParts[id] - if column, ok := model.Columns().Cols[p.additionalColumn.name]; ok { - quoteName := q.Connection.Dialect.Quote(column.Name) - - value := tokenParts[column.Name] - - q = q. - Where(fmt.Sprintf("%s > ? OR (%s = ? AND %s > ?)", quoteName, quoteName, eid), value, value, idValue). - Order(fmt.Sprintf("%s %s", quoteName, p.additionalColumn.order)) - } else { - q = q. - Where(fmt.Sprintf(`%s > ?`, eid), idValue) + if err := p.multipleOrderFieldsQuery(q, id, model.Columns().Cols, q.Connection.Dialect.Quote); err != nil { + // silently ignore the error, and fall back to the "default" behavior of just ordering by the token + q.Where(fmt.Sprintf(`%s > ?`, eid), p.Token()) } + return q. Limit(p.Size() + 1). // we always need to order by the id field last @@ -169,7 +210,7 @@ func WithSize(size int) Option { } } -func WithColumn(name string, order string) Option { +func WithColumn(name string, order Order) Option { return func(opts *Paginator) *Paginator { opts.additionalColumn = columnOrdering{ name: name, diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index 6b85dd9f..a3bb7bff 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -163,11 +163,53 @@ func TestPaginateWithAdditionalColumn(t *testing.T) { URL: "postgres://foo.bar", }) require.NoError(t, err) - q := pop.Q(c) - paginator := GetPaginator(WithSize(10), WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "DESC")) - q = q.Scope(Paginate[testItem](paginator)) - sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) - assert.Equal(t, "SELECT test_items.created_at, test_items.pk FROM test_items AS test_items WHERE \"created_at\" > $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" DESC, \"pk\" ASC LIMIT 11", sql) - assert.Equal(t, []interface{}{"timestamp", "timestamp", "token_value"}, args) + for _, tc := range []struct { + d string + opts []Option + e string + args []interface{} + }{ + { + d: "with sort by created_at DESC", + opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "DESC")}, + e: "created_at\" < $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" DESC, \"pk\" ASC", + args: []interface{}{"timestamp", "timestamp", "token_value"}, + }, + { + d: "with sort by created_at ASC", + opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "ASC")}, + e: "created_at\" > $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" ASC, \"pk\" ASC", + args: []interface{}{"timestamp", "timestamp", "token_value"}, + }, + { + d: "with malformed token", + opts: []Option{WithToken("some/random/token"), WithColumn("created_at", "ASC")}, + e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + args: []interface{}{"some/random/token"}, + }, + { + d: "with unknown column", + opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("unknown_column", "ASC")}, + e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + args: []interface{}{"pk=token_value/created_at=timestamp"}, + }, + { + d: "with unknown order", + opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", Order("unknown order"))}, + e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + args: []interface{}{"pk=token_value/created_at=timestamp"}, + }, + } { + t.Run("case="+tc.d, func(t *testing.T) { + opts := append(tc.opts, WithSize(10)) + paginator := GetPaginator(opts...) + sql, args := pop.Q(c). + Scope(Paginate[testItem](paginator)). + ToSQL(&pop.Model{Value: new(testItem)}) + assert.Contains(t, sql, tc.e) + assert.Contains(t, sql, "LIMIT 11") + assert.Equal(t, tc.args, args) + }) + } } From e2df1f5d26ba74cd6ec73a7bd7518f3260ad317f Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Mon, 28 Nov 2022 11:04:59 +0100 Subject: [PATCH 03/10] chore: unescape page token param --- pagination/keysetpagination/header.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pagination/keysetpagination/header.go b/pagination/keysetpagination/header.go index 30480a6c..d084364a 100644 --- a/pagination/keysetpagination/header.go +++ b/pagination/keysetpagination/header.go @@ -94,7 +94,11 @@ func Header(w http.ResponseWriter, u *url.URL, p *Paginator) { func Parse(q url.Values) ([]Option, error) { var opts []Option if q.Has("page_token") { - opts = append(opts, WithToken(q.Get("page_token"))) + pageToken, err := url.QueryUnescape(q.Get("page_token")) + if err != nil { + return nil, errors.WithStack(err) + } + opts = append(opts, WithToken(pageToken)) } if q.Has("page_size") { size, err := strconv.Atoi(q.Get("page_size")) From a6593f344b51aeb1234b1a5b4a127392cd0668fa Mon Sep 17 00:00:00 2001 From: Patrik Date: Tue, 29 Nov 2022 17:56:10 +0100 Subject: [PATCH 04/10] chore: reformat strings in test --- pagination/keysetpagination/paginator_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index a3bb7bff..aa51ee70 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -173,31 +173,31 @@ func TestPaginateWithAdditionalColumn(t *testing.T) { { d: "with sort by created_at DESC", opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "DESC")}, - e: "created_at\" < $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" DESC, \"pk\" ASC", + e: `WHERE "created_at" < $1 OR ("created_at" = $2 AND "pk" > $3) ORDER BY "created_at" DESC, "pk" ASC`, args: []interface{}{"timestamp", "timestamp", "token_value"}, }, { d: "with sort by created_at ASC", opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "ASC")}, - e: "created_at\" > $1 OR (\"created_at\" = $2 AND \"pk\" > $3) ORDER BY \"created_at\" ASC, \"pk\" ASC", + e: `WHERE "created_at" > $1 OR ("created_at" = $2 AND "pk" > $3) ORDER BY "created_at" ASC, "pk" ASC`, args: []interface{}{"timestamp", "timestamp", "token_value"}, }, { d: "with malformed token", opts: []Option{WithToken("some/random/token"), WithColumn("created_at", "ASC")}, - e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + e: `WHERE "pk" > $1 ORDER BY "pk"`, args: []interface{}{"some/random/token"}, }, { d: "with unknown column", opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("unknown_column", "ASC")}, - e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + e: `WHERE "pk" > $1 ORDER BY "pk"`, args: []interface{}{"pk=token_value/created_at=timestamp"}, }, { d: "with unknown order", opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", Order("unknown order"))}, - e: "WHERE \"pk\" > $1 ORDER BY \"pk\"", + e: `WHERE "pk" > $1 ORDER BY "pk"`, args: []interface{}{"pk=token_value/created_at=timestamp"}, }, } { From aed37eef63277e20bd6b1870cb624ed64fa36d5b Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 11:41:37 +0100 Subject: [PATCH 05/10] feat: add page token interface --- pagination/keysetpagination/header.go | 12 ++- pagination/keysetpagination/header_test.go | 52 ++++++++++++ pagination/keysetpagination/page_token.go | 69 ++++++++++++++++ pagination/keysetpagination/paginator.go | 35 ++++---- pagination/keysetpagination/paginator_test.go | 79 +++++++++++-------- 5 files changed, 191 insertions(+), 56 deletions(-) create mode 100644 pagination/keysetpagination/header_test.go create mode 100644 pagination/keysetpagination/page_token.go diff --git a/pagination/keysetpagination/header.go b/pagination/keysetpagination/header.go index d084364a..48214148 100644 --- a/pagination/keysetpagination/header.go +++ b/pagination/keysetpagination/header.go @@ -83,22 +83,26 @@ func header(u *url.URL, rel, token string, size int) string { // It contains links to the first and next page, if one exists. func Header(w http.ResponseWriter, u *url.URL, p *Paginator) { size := p.Size() - w.Header().Set("Link", header(u, "first", p.defaultToken, size)) + w.Header().Set("Link", header(u, "first", p.defaultToken.Encode(), size)) if !p.IsLast() { - w.Header().Add("Link", header(u, "next", p.Token(), size)) + w.Header().Add("Link", header(u, "next", p.Token().Encode(), size)) } } // Parse returns the pagination options from the URL query. -func Parse(q url.Values) ([]Option, error) { +func Parse(q url.Values, p PageTokenConstructor) ([]Option, error) { var opts []Option if q.Has("page_token") { pageToken, err := url.QueryUnescape(q.Get("page_token")) if err != nil { return nil, errors.WithStack(err) } - opts = append(opts, WithToken(pageToken)) + parsed, err := p(pageToken) + if err != nil { + return nil, errors.WithStack(err) + } + opts = append(opts, WithToken(parsed)) } if q.Has("page_size") { size, err := strconv.Atoi(q.Get("page_size")) diff --git a/pagination/keysetpagination/header_test.go b/pagination/keysetpagination/header_test.go new file mode 100644 index 00000000..41d8659e --- /dev/null +++ b/pagination/keysetpagination/header_test.go @@ -0,0 +1,52 @@ +package keysetpagination + +import ( + "net/http/httptest" + "net/url" + "testing" + + "github.com/instana/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHeader(t *testing.T) { + p := &Paginator{ + defaultToken: StringPageToken("default"), + token: StringPageToken("next"), + size: 2, + } + + u, err := url.Parse("http://ory.sh/") + require.NoError(t, err) + + r := httptest.NewRecorder() + + Header(r, u, p) + + links := r.HeaderMap["Link"] + assert.Len(t, links, 2) + assert.Contains(t, links[0], "page_token=default") + assert.Contains(t, links[1], "page_token=next") +} + +func TestHeader_WithIsLast(t *testing.T) { + p := &Paginator{ + defaultToken: StringPageToken("default"), + token: StringPageToken("next"), + size: 2, + isLast: true, + } + + u, err := url.Parse("http://ory.sh/") + require.NoError(t, err) + + r := httptest.NewRecorder() + + Header(r, u, p) + + links := r.HeaderMap["Link"] + assert.Len(t, links, 1) + assert.Contains(t, links[0], "page_token=default") + + t.Logf("%v", url.QueryEscape("pk=token/created_at=123")) +} diff --git a/pagination/keysetpagination/page_token.go b/pagination/keysetpagination/page_token.go new file mode 100644 index 00000000..e86f4b99 --- /dev/null +++ b/pagination/keysetpagination/page_token.go @@ -0,0 +1,69 @@ +package keysetpagination + +import ( + "fmt" + "net/url" + "strings" +) + +type PageToken interface { + Parse(string) map[string]string + Encode() string +} + +var _ PageToken = new(StringPageToken) +var _ PageToken = new(MapPageToken) + +type StringPageToken string + +func (s StringPageToken) Parse(idField string) map[string]string { + return map[string]string{idField: string(s)} +} + +func (s StringPageToken) Encode() string { + return string(s) +} + +func NewStringPageToken(s string) (PageToken, error) { + return StringPageToken(s), nil +} + +type MapPageToken map[string]string + +func (m MapPageToken) Parse(_ string) map[string]string { + return map[string]string(m) +} + +const pageTokenColumnDelim = "/" + +func (m MapPageToken) Encode() string { + elems := []string{} + for k, v := range m { + elems = append(elems, fmt.Sprintf("%s=%s", k, v)) + } + + return url.QueryEscape(strings.Join(elems, pageTokenColumnDelim)) +} + +func NewMapPageToken(s string) (PageToken, error) { + s, err := url.QueryUnescape(s) + if err != nil { + return nil, err + } + tokens := strings.Split(s, pageTokenColumnDelim) + + r := map[string]string{} + + for _, p := range tokens { + if columnName, value, found := strings.Cut(p, "="); found { + r[columnName] = value + } + } + + return MapPageToken(r), nil +} + +var _ PageTokenConstructor = NewMapPageToken +var _ PageTokenConstructor = NewStringPageToken + +type PageTokenConstructor func(string) (PageToken, error) diff --git a/pagination/keysetpagination/paginator.go b/pagination/keysetpagination/paginator.go index f5a3885c..0e8b4686 100644 --- a/pagination/keysetpagination/paginator.go +++ b/pagination/keysetpagination/paginator.go @@ -13,7 +13,7 @@ import ( ) type ( - Item interface{ PageToken() string } + Item interface{ PageToken() PageToken } Order string @@ -22,7 +22,7 @@ type ( order Order } Paginator struct { - token, defaultToken string + token, defaultToken PageToken size, defaultSize, maxSize int isLast bool additionalColumn columnOrdering @@ -48,8 +48,8 @@ func (o Order) extract() (string, string, error) { } } -func (p *Paginator) Token() string { - if p.token == "" { +func (p *Paginator) Token() PageToken { + if p.token == nil { return p.defaultToken } return p.token @@ -102,33 +102,35 @@ func (p *Paginator) ToOptions() []Option { } } -func (p *Paginator) multipleOrderFieldsQuery(q *pop.Query, idField string, cols map[string]*columns.Column, quote func(string) string) error { +func (p *Paginator) multipleOrderFieldsQuery(q *pop.Query, idField string, cols map[string]*columns.Column, quote func(string) string) { + tokenParts := p.Token().Parse(idField) + idValue := tokenParts[idField] + column, ok := cols[p.additionalColumn.name] if !ok { - return errors.New("column not supported") + q.Where(fmt.Sprintf(`%s > ?`, quote(idField)), idValue) + return } - tokenParts := parseToken(idField, p.Token()) - idValue := tokenParts[idField] - quoteName := quote(column.Name) value, ok := tokenParts[column.Name] if !ok { - return errors.New("no value provided for " + column.Name) + q.Where(fmt.Sprintf(`%s > ?`, quote(idField)), idValue) + return } sign, keyword, err := p.additionalColumn.order.extract() if err != nil { - return err + q.Where(fmt.Sprintf(`%s > ?`, quote(idField)), idValue) + return } q. Where(fmt.Sprintf("%s %s ? OR (%s = ? AND %s > ?)", quoteName, sign, quoteName, quote(idField)), value, value, idValue). Order(fmt.Sprintf("%s %s", quoteName, keyword)) - return nil } // Paginate returns a function that paginates a pop.Query. @@ -142,10 +144,7 @@ func Paginate[I Item](p *Paginator) pop.ScopeFunc { return func(q *pop.Query) *pop.Query { eid := q.Connection.Dialect.Quote(id) - if err := p.multipleOrderFieldsQuery(q, id, model.Columns().Cols, q.Connection.Dialect.Quote); err != nil { - // silently ignore the error, and fall back to the "default" behavior of just ordering by the token - q.Where(fmt.Sprintf(`%s > ?`, eid), p.Token()) - } + p.multipleOrderFieldsQuery(q, id, model.Columns().Cols, q.Connection.Dialect.Quote) return q. Limit(p.Size() + 1). @@ -175,7 +174,7 @@ func Result[I Item](items []I, p *Paginator) ([]I, *Paginator) { } } -func WithDefaultToken(t string) Option { +func WithDefaultToken(t PageToken) Option { return func(opts *Paginator) *Paginator { opts.defaultToken = t return opts @@ -196,7 +195,7 @@ func WithMaxSize(size int) Option { } } -func WithToken(t string) Option { +func WithToken(t PageToken) Option { return func(opts *Paginator) *Paginator { opts.token = t return opts diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index aa51ee70..da675244 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -18,8 +18,8 @@ type testItem struct { CreatedAt string `db:"created_at"` } -func (t testItem) PageToken() string { - return t.ID +func (t testItem) PageToken() PageToken { + return StringPageToken(t.ID) } func TestPaginator(t *testing.T) { @@ -29,7 +29,7 @@ func TestPaginator(t *testing.T) { }) require.NoError(t, err) q := pop.Q(c) - paginator := GetPaginator(WithSize(10), WithToken("token")) + paginator := GetPaginator(WithSize(10), WithToken(StringPageToken("token"))) q = q.Scope(Paginate[testItem](paginator)) sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) @@ -43,7 +43,7 @@ func TestPaginator(t *testing.T) { }) require.NoError(t, err) q := pop.Q(c) - paginator := GetPaginator(WithSize(10), WithToken("token")) + paginator := GetPaginator(WithSize(10), WithToken(StringPageToken("token"))) q = q.Scope(Paginate[testItem](paginator)) sql, args := q.ToSQL(&pop.Model{Value: new(testItem)}) @@ -65,10 +65,10 @@ func TestPaginator(t *testing.T) { {ID: "10"}, {ID: "11"}, } - paginator := GetPaginator(WithDefaultSize(10), WithToken("token")) + paginator := GetPaginator(WithDefaultSize(10), WithToken(StringPageToken("token"))) items, nextPage := Result(items, paginator) assert.Len(t, items, 10) - assert.Equal(t, "10", nextPage.Token()) + assert.Equal(t, StringPageToken("10"), nextPage.Token()) assert.Equal(t, 10, nextPage.Size()) }) @@ -77,31 +77,30 @@ func TestPaginator(t *testing.T) { name string opts []Option expectedSize int - expectedToken string + expectedToken PageToken }{ { - name: "default", - opts: nil, - expectedSize: 100, - expectedToken: "", + name: "default", + opts: nil, + expectedSize: 100, }, { name: "with size and token", - opts: []Option{WithSize(10), WithToken("token")}, + opts: []Option{WithSize(10), WithToken(StringPageToken("token"))}, expectedSize: 10, - expectedToken: "token", + expectedToken: StringPageToken("token"), }, { name: "with custom defaults", - opts: []Option{WithDefaultSize(10), WithDefaultToken("token")}, + opts: []Option{WithDefaultSize(10), WithDefaultToken(StringPageToken("token"))}, expectedSize: 10, - expectedToken: "token", + expectedToken: StringPageToken("token"), }, { name: "with custom defaults and size and token", - opts: []Option{WithDefaultSize(10), WithDefaultToken("token"), WithSize(20), WithToken("token2")}, + opts: []Option{WithDefaultSize(10), WithDefaultToken(StringPageToken("token")), WithSize(20), WithToken(StringPageToken("token2"))}, expectedSize: 20, - expectedToken: "token2", + expectedToken: StringPageToken("token2"), }, { name: "with size and custom default and max size", @@ -123,28 +122,46 @@ func TestParse(t *testing.T) { name string q url.Values expectedSize int - expectedToken string + expectedToken PageToken + f PageTokenConstructor }{ { name: "with page token", q: url.Values{"page_token": {"token3"}}, expectedSize: 100, - expectedToken: "token3", + expectedToken: StringPageToken("token3"), + f: NewStringPageToken, }, { name: "with page size", q: url.Values{"page_size": {"123"}}, expectedSize: 123, + f: NewStringPageToken, }, { name: "with page size and page token", q: url.Values{"page_size": {"123"}, "page_token": {"token5"}}, expectedSize: 123, - expectedToken: "token5", + expectedToken: StringPageToken("token5"), + f: NewStringPageToken, + }, + { + name: "with page size and page token", + q: url.Values{"page_size": {"123"}, "page_token": {"pk=token5"}}, + expectedSize: 123, + expectedToken: MapPageToken{"pk": "token5"}, + f: NewMapPageToken, + }, + { + name: "with page size and page token", + q: url.Values{"page_size": {"123"}, "page_token": {"pk%3Dtoken%2Fcreated_at%3D111"}}, + expectedSize: 123, + expectedToken: MapPageToken{"created_at": "111", "pk": "token"}, + f: NewMapPageToken, }, } { t.Run(tc.name, func(t *testing.T) { - opts, err := Parse(tc.q) + opts, err := Parse(tc.q, tc.f) require.NoError(t, err) paginator := GetPaginator(opts...) assert.Equal(t, tc.expectedSize, paginator.Size()) @@ -153,7 +170,7 @@ func TestParse(t *testing.T) { } t.Run("invalid page size leads to err", func(t *testing.T) { - _, err := Parse(url.Values{"page_size": {"invalid-int"}}) + _, err := Parse(url.Values{"page_size": {"invalid-int"}}, NewStringPageToken) require.ErrorIs(t, err, strconv.ErrSyntax) }) } @@ -172,33 +189,27 @@ func TestPaginateWithAdditionalColumn(t *testing.T) { }{ { d: "with sort by created_at DESC", - opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "DESC")}, + opts: []Option{WithToken(MapPageToken{"pk": "token_value", "created_at": "timestamp"}), WithColumn("created_at", "DESC")}, e: `WHERE "created_at" < $1 OR ("created_at" = $2 AND "pk" > $3) ORDER BY "created_at" DESC, "pk" ASC`, args: []interface{}{"timestamp", "timestamp", "token_value"}, }, { d: "with sort by created_at ASC", - opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", "ASC")}, + opts: []Option{WithToken(MapPageToken{"pk": "token_value", "created_at": "timestamp"}), WithColumn("created_at", "ASC")}, e: `WHERE "created_at" > $1 OR ("created_at" = $2 AND "pk" > $3) ORDER BY "created_at" ASC, "pk" ASC`, args: []interface{}{"timestamp", "timestamp", "token_value"}, }, - { - d: "with malformed token", - opts: []Option{WithToken("some/random/token"), WithColumn("created_at", "ASC")}, - e: `WHERE "pk" > $1 ORDER BY "pk"`, - args: []interface{}{"some/random/token"}, - }, { d: "with unknown column", - opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("unknown_column", "ASC")}, + opts: []Option{WithToken(MapPageToken{"pk": "token_value", "created_at": "timestamp"}), WithColumn("unknown_column", "ASC")}, e: `WHERE "pk" > $1 ORDER BY "pk"`, - args: []interface{}{"pk=token_value/created_at=timestamp"}, + args: []interface{}{"token_value"}, }, { d: "with unknown order", - opts: []Option{WithToken("pk=token_value/created_at=timestamp"), WithColumn("created_at", Order("unknown order"))}, + opts: []Option{WithToken(MapPageToken{"pk": "token_value", "created_at": "timestamp"}), WithColumn("created_at", Order("unknown order"))}, e: `WHERE "pk" > $1 ORDER BY "pk"`, - args: []interface{}{"pk=token_value/created_at=timestamp"}, + args: []interface{}{"token_value"}, }, } { t.Run("case="+tc.d, func(t *testing.T) { From b94c6f320cce68f8fe96e63b2e1382c5f7bd64bd Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 11:45:42 +0100 Subject: [PATCH 06/10] chore: u --- pagination/keysetpagination/header_test.go | 3 +++ pagination/keysetpagination/page_token.go | 3 +++ pagination/keysetpagination/paginator.go | 18 ------------------ pagination/keysetpagination/paginator_test.go | 6 ++++++ 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/pagination/keysetpagination/header_test.go b/pagination/keysetpagination/header_test.go index 41d8659e..5ffd4da5 100644 --- a/pagination/keysetpagination/header_test.go +++ b/pagination/keysetpagination/header_test.go @@ -1,3 +1,6 @@ +// Copyright © 2022 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + package keysetpagination import ( diff --git a/pagination/keysetpagination/page_token.go b/pagination/keysetpagination/page_token.go index e86f4b99..358a896a 100644 --- a/pagination/keysetpagination/page_token.go +++ b/pagination/keysetpagination/page_token.go @@ -1,3 +1,6 @@ +// Copyright © 2022 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + package keysetpagination import ( diff --git a/pagination/keysetpagination/paginator.go b/pagination/keysetpagination/paginator.go index 0e8b4686..20675ba7 100644 --- a/pagination/keysetpagination/paginator.go +++ b/pagination/keysetpagination/paginator.go @@ -6,7 +6,6 @@ package keysetpagination import ( "errors" "fmt" - "strings" "github.com/gobuffalo/pop/v6" "github.com/gobuffalo/pop/v6/columns" @@ -69,23 +68,6 @@ func (p *Paginator) Size() int { return size } -func parseToken(idField string, s string) map[string]string { - tokens := strings.Split(s, "/") - if len(tokens) != 2 { - return map[string]string{idField: s} - } - - r := map[string]string{} - - for _, p := range tokens { - if columnName, value, found := strings.Cut(p, "="); found { - r[columnName] = value - } - } - - return r -} - func (p *Paginator) IsLast() bool { return p.isLast } diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index da675244..93a52b72 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -205,6 +205,12 @@ func TestPaginateWithAdditionalColumn(t *testing.T) { e: `WHERE "pk" > $1 ORDER BY "pk"`, args: []interface{}{"token_value"}, }, + { + d: "with no token value", + opts: []Option{WithToken(MapPageToken{"pk": "token_value"}), WithColumn("created_at", "ASC")}, + e: `WHERE "pk" > $1 ORDER BY "pk"`, + args: []interface{}{"token_value"}, + }, { d: "with unknown order", opts: []Option{WithToken(MapPageToken{"pk": "token_value", "created_at": "timestamp"}), WithColumn("created_at", Order("unknown order"))}, From 21fbab232855d8a9f03e9c2527538884ceaa1912 Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 11:48:55 +0100 Subject: [PATCH 07/10] chore: improve header test --- pagination/keysetpagination/header_test.go | 25 ++++++---------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/pagination/keysetpagination/header_test.go b/pagination/keysetpagination/header_test.go index 5ffd4da5..ebbe489d 100644 --- a/pagination/keysetpagination/header_test.go +++ b/pagination/keysetpagination/header_test.go @@ -30,26 +30,15 @@ func TestHeader(t *testing.T) { assert.Len(t, links, 2) assert.Contains(t, links[0], "page_token=default") assert.Contains(t, links[1], "page_token=next") -} - -func TestHeader_WithIsLast(t *testing.T) { - p := &Paginator{ - defaultToken: StringPageToken("default"), - token: StringPageToken("next"), - size: 2, - isLast: true, - } - u, err := url.Parse("http://ory.sh/") - require.NoError(t, err) + t.Run("with isLast", func(t *testing.T) { + p.isLast = true - r := httptest.NewRecorder() + Header(r, u, p) - Header(r, u, p) - - links := r.HeaderMap["Link"] - assert.Len(t, links, 1) - assert.Contains(t, links[0], "page_token=default") + links := r.HeaderMap["Link"] + assert.Len(t, links, 1) + assert.Contains(t, links[0], "page_token=default") + }) - t.Logf("%v", url.QueryEscape("pk=token/created_at=123")) } From 031a59af19255b2d719af10b3559e32176deab7b Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 13:56:43 +0100 Subject: [PATCH 08/10] chore: use base64 --- pagination/keysetpagination/page_token.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pagination/keysetpagination/page_token.go b/pagination/keysetpagination/page_token.go index 358a896a..942aaf7b 100644 --- a/pagination/keysetpagination/page_token.go +++ b/pagination/keysetpagination/page_token.go @@ -4,8 +4,8 @@ package keysetpagination import ( + "encoding/base64" "fmt" - "net/url" "strings" ) @@ -45,15 +45,15 @@ func (m MapPageToken) Encode() string { elems = append(elems, fmt.Sprintf("%s=%s", k, v)) } - return url.QueryEscape(strings.Join(elems, pageTokenColumnDelim)) + return base64.RawStdEncoding.EncodeToString([]byte(strings.Join(elems, pageTokenColumnDelim))) } func NewMapPageToken(s string) (PageToken, error) { - s, err := url.QueryUnescape(s) + b, err := base64.RawStdEncoding.DecodeString(s) if err != nil { return nil, err } - tokens := strings.Split(s, pageTokenColumnDelim) + tokens := strings.Split(string(b), pageTokenColumnDelim) r := map[string]string{} From ee5838144bce6eccb3ac7df3fcdcbec35e506556 Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 14:07:26 +0100 Subject: [PATCH 09/10] chore: u --- pagination/keysetpagination/page_token.go | 2 ++ pagination/keysetpagination/paginator_test.go | 9 +-------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/pagination/keysetpagination/page_token.go b/pagination/keysetpagination/page_token.go index 942aaf7b..231cc20b 100644 --- a/pagination/keysetpagination/page_token.go +++ b/pagination/keysetpagination/page_token.go @@ -45,6 +45,8 @@ func (m MapPageToken) Encode() string { elems = append(elems, fmt.Sprintf("%s=%s", k, v)) } + // For now: use Base64 instead of URL escaping, as the Timestamp format we need to use can contain a `+` sign, + // which represents a space in URLs, so it's not properly encoded by the Go library. return base64.RawStdEncoding.EncodeToString([]byte(strings.Join(elems, pageTokenColumnDelim))) } diff --git a/pagination/keysetpagination/paginator_test.go b/pagination/keysetpagination/paginator_test.go index 93a52b72..e57f8740 100644 --- a/pagination/keysetpagination/paginator_test.go +++ b/pagination/keysetpagination/paginator_test.go @@ -147,18 +147,11 @@ func TestParse(t *testing.T) { }, { name: "with page size and page token", - q: url.Values{"page_size": {"123"}, "page_token": {"pk=token5"}}, + q: url.Values{"page_size": {"123"}, "page_token": {"cGs9dG9rZW41"}}, expectedSize: 123, expectedToken: MapPageToken{"pk": "token5"}, f: NewMapPageToken, }, - { - name: "with page size and page token", - q: url.Values{"page_size": {"123"}, "page_token": {"pk%3Dtoken%2Fcreated_at%3D111"}}, - expectedSize: 123, - expectedToken: MapPageToken{"created_at": "111", "pk": "token"}, - f: NewMapPageToken, - }, } { t.Run(tc.name, func(t *testing.T) { opts, err := Parse(tc.q, tc.f) From bc6e2a24d9c8ee2ab775e1a7f88f40d6d427a5dc Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Wed, 30 Nov 2022 15:45:08 +0100 Subject: [PATCH 10/10] chore: review --- pagination/keysetpagination/header_test.go | 4 ++-- pagination/keysetpagination/page_token.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pagination/keysetpagination/header_test.go b/pagination/keysetpagination/header_test.go index ebbe489d..df4503fb 100644 --- a/pagination/keysetpagination/header_test.go +++ b/pagination/keysetpagination/header_test.go @@ -27,7 +27,7 @@ func TestHeader(t *testing.T) { Header(r, u, p) links := r.HeaderMap["Link"] - assert.Len(t, links, 2) + require.Len(t, links, 2) assert.Contains(t, links[0], "page_token=default") assert.Contains(t, links[1], "page_token=next") @@ -37,7 +37,7 @@ func TestHeader(t *testing.T) { Header(r, u, p) links := r.HeaderMap["Link"] - assert.Len(t, links, 1) + require.Len(t, links, 1) assert.Contains(t, links[0], "page_token=default") }) diff --git a/pagination/keysetpagination/page_token.go b/pagination/keysetpagination/page_token.go index 231cc20b..1beb3755 100644 --- a/pagination/keysetpagination/page_token.go +++ b/pagination/keysetpagination/page_token.go @@ -40,7 +40,7 @@ func (m MapPageToken) Parse(_ string) map[string]string { const pageTokenColumnDelim = "/" func (m MapPageToken) Encode() string { - elems := []string{} + elems := make([]string, 0, len(m)) for k, v := range m { elems = append(elems, fmt.Sprintf("%s=%s", k, v)) } @@ -71,4 +71,4 @@ func NewMapPageToken(s string) (PageToken, error) { var _ PageTokenConstructor = NewMapPageToken var _ PageTokenConstructor = NewStringPageToken -type PageTokenConstructor func(string) (PageToken, error) +type PageTokenConstructor = func(string) (PageToken, error)