Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mongox): fix pagination #47

Merged
merged 2 commits into from
Aug 9, 2024
Merged

feat(mongox): fix pagination #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a124646
feat(mongox): fix pagination
pyshx Aug 9, 2024
6328ffb
lint
pyshx Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 77 additions & 62 deletions mongox/pagination.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,38 @@ func (c *Collection) Paginate(ctx context.Context, rawFilter any, s *usecasex.So
return nil, nil
}

pFilter, pOpts, err := c.findFilter(ctx, *p, s)
pFilter, err := c.pageFilter(ctx, *p, s)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, err)
}

filter := rawFilter
if pFilter != nil {
filter = And(rawFilter, "", pFilter)
}

cursor, err := c.collection.Find(ctx, filter, append([]*options.FindOptions{pOpts}, opts...)...)
sortKey := idKey
sortOrder := 1
if s != nil && s.Key != "" {
sortKey = s.Key
if s.Reverted {
sortOrder = -1
}
}

if p.Cursor != nil && p.Cursor.Last != nil {
sortOrder *= -1
}

findOpts := options.Find().
SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}).
SetLimit(limit(*p))

if p.Offset != nil {
findOpts.SetSkip(p.Offset.Offset)
}
Comment on lines +43 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize find options configuration.

The findOpts configuration correctly sets sorting and limit options. Consider consolidating the logic for setting options to enhance readability and maintainability.

- findOpts := options.Find().
- 	SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}).
- 	SetLimit(limit(*p))
+ findOpts := options.Find().
+ 	SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}).
+ 	SetLimit(limit(*p))

if p.Offset != nil {
	findOpts.SetSkip(p.Offset.Offset)
}

Committable suggestion was skipped due to low confidence.


cursor, err := c.collection.Find(ctx, filter, append([]*options.FindOptions{findOpts}, opts...)...)
if err != nil {
return nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to find: %w", err))
}
Expand All @@ -46,6 +68,7 @@ func (c *Collection) Paginate(ctx context.Context, rawFilter any, s *usecasex.So

if p.Cursor != nil && p.Cursor.Last != nil {
reverse(items)
startCursor, endCursor = endCursor, startCursor
}

for _, item := range items {
Expand All @@ -59,7 +82,6 @@ func (c *Collection) Paginate(ctx context.Context, rawFilter any, s *usecasex.So
return usecasex.NewPageInfo(count, startCursor, endCursor, hasNextPage, hasPreviousPage), nil
}


func (c *Collection) PaginateAggregation(ctx context.Context, pipeline []any, s *usecasex.Sort, p *usecasex.Pagination, consumer Consumer, opts ...*options.AggregateOptions) (*usecasex.PageInfo, error) {
if p == nil || p.Cursor == nil && p.Offset == nil {
return nil, nil
Expand Down Expand Up @@ -155,19 +177,6 @@ func reverse(items []bson.Raw) {
}
}

func (c *Collection) findFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) (any, *options.FindOptions, error) {
if p.Cursor == nil && p.Offset == nil {
return nil, nil, errors.New("invalid pagination")
}

opts := findOptionsFromPagination(p, s)
if p.Offset != nil {
return nil, opts, nil
}
f, err := c.pageFilter(ctx, p, s)
return f, opts, err
}

func (c *Collection) aggregateFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) ([]any, *options.AggregateOptions, error) {
if p.Cursor == nil && p.Offset == nil {
return nil, nil, errors.New("invalid pagination")
Expand All @@ -189,21 +198,6 @@ func (c *Collection) aggregateFilter(ctx context.Context, p usecasex.Pagination,
return append(stages, bson.M{"$limit": limit(p)}), aggregateOptionsFromPagination(p, s), err
}

func findOptionsFromPagination(p usecasex.Pagination, s *usecasex.Sort) *options.FindOptions {
o := options.Find().SetAllowDiskUse(true).SetLimit(limit(p))

if p.Offset != nil {
o = o.SetSkip(p.Offset.Offset)
}

collation := options.Collation{
Locale: "en",
Strength: 2,
}

return o.SetCollation(&collation).SetSort(sortFilter(p, s))
}

func aggregateOptionsFromPagination(_ usecasex.Pagination, _ *usecasex.Sort) *options.AggregateOptions {
collation := options.Collation{
Locale: "en",
Expand All @@ -212,56 +206,77 @@ func aggregateOptionsFromPagination(_ usecasex.Pagination, _ *usecasex.Sort) *op
return options.Aggregate().SetAllowDiskUse(true).SetCollation(&collation)
}

func (c *Collection) pageFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) (any, error) {
func (c *Collection) pageFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) (bson.M, error) {
if p.Cursor == nil {
return nil, nil
}

var filter bson.M
sortKey := idKey
sortOrder := 1

if s != nil && s.Key != "" {
sortKey = s.Key
if s.Reverted {
sortOrder = -1
}
}

var cursor *usecasex.Cursor
var op string
var cur *usecasex.Cursor

if p.Cursor.First != nil {
if p.Cursor.After != nil {
cursor = p.Cursor.After
op = "$gt"
cur = p.Cursor.After
} else if p.Cursor.Last != nil {
} else if p.Cursor.Before != nil {
cursor = p.Cursor.Before
op = "$lt"
cur = p.Cursor.Before
} else {
return nil, errors.New("neither first nor last are set")
}
if cur == nil {
return nil, nil
}

var sortKey *string
if s != nil {
sortKey = &s.Key
}
var paginationFilter bson.M
if sortKey == nil || *sortKey == "" {
paginationFilter = bson.M{idKey: bson.M{op: *cur}}
} else {
var cursorDoc bson.M
if err := c.collection.FindOne(ctx, bson.M{idKey: *cur}).Decode(&cursorDoc); err != nil {
return nil, fmt.Errorf("failed to find cursor element")
}

if cursorDoc[*sortKey] == nil {
return nil, fmt.Errorf("invalied sort key")
if cursor != nil {
cursorDoc, err := c.getCursorDocument(ctx, *cursor)
if err != nil {
return nil, err
}

paginationFilter = bson.M{
filter = bson.M{
"$or": []bson.M{
{*sortKey: bson.M{op: cursorDoc[*sortKey]}},
{sortKey: bson.M{op: cursorDoc[sortKey]}},
{
*sortKey: cursorDoc[*sortKey],
idKey: bson.M{op: *cur},
sortKey: cursorDoc[sortKey],
idKey: bson.M{op: cursorDoc[idKey]},
},
},
}

if sortOrder == -1 {
if op == "$gt" {
op = "$lt"
} else {
op = "$gt"
}
filter = bson.M{
"$or": []bson.M{
{sortKey: bson.M{op: cursorDoc[sortKey]}},
{
sortKey: cursorDoc[sortKey],
idKey: bson.M{op: cursorDoc[idKey]},
},
},
}
}
}

return paginationFilter, nil
return filter, nil
Comment on lines +209 to +270
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize duplicated filter logic for reversed sort order.

The logic for creating the filter when the sort order is reversed is duplicated. Consider refactoring to reduce redundancy.

if cursor != nil {
    cursorDoc, err := c.getCursorDocument(ctx, *cursor)
    if err != nil {
        return nil, err
    }

    op1, op2 := op, "$lt"
    if sortOrder == -1 {
        op1, op2 = "$lt", "$gt"
    }

    filter = bson.M{
        "$or": []bson.M{
            {sortKey: bson.M{op1: cursorDoc[sortKey]}},
            {
                sortKey: cursorDoc[sortKey],
                idKey:   bson.M{op2: cursorDoc[idKey]},
            },
        },
    }
}

}

func (c *Collection) getCursorDocument(ctx context.Context, cursor usecasex.Cursor) (bson.M, error) {
var cursorDoc bson.M
err := c.collection.FindOne(ctx, bson.M{idKey: cursor}).Decode(&cursorDoc)
if err != nil {
return nil, fmt.Errorf("failed to find cursor element: %w", err)
}
return cursorDoc, nil
}

func sortFilter(p usecasex.Pagination, s *usecasex.Sort) bson.D {
Expand Down
94 changes: 94 additions & 0 deletions mongox/pagination_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,100 @@ func TestClientCollection_PaginateWithUpdatedAtSort(t *testing.T) {
assert.Equal(t, []usecasex.Cursor{"c", "d", "e"}, con.Cursors)
}

func TestClientCollection_DetailedPagination(t *testing.T) {
ctx := context.Background()
initDB := mongotest.Connect(t)
c := NewCollection(initDB(t).Collection("test"))

// Seed data
seeds := []struct {
id string
updatedAt int64
}{
{"a", 1000},
{"b", 2000},
{"c", 3000},
{"d", 4000},
{"e", 5000},
}

_, _ = c.Client().InsertMany(ctx, lo.Map(seeds, func(s struct {
id string
updatedAt int64
}, i int) any {
return bson.M{"id": s.id, "updatedAt": s.updatedAt}
}))

testCases := []struct {
name string
sort *usecasex.Sort
pagination *usecasex.CursorPagination
expected []string
}{
{
name: "First 2, Ascending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: false},
pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2))},
expected: []string{"a", "b"},
},
{
name: "First 2, Descending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: true},
pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2))},
expected: []string{"e", "d"},
},
{
name: "Last 2, Ascending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: false},
pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2))},
expected: []string{"d", "e"},
},
{
name: "Last 2, Descending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: true},
pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2))},
expected: []string{"b", "a"},
},
{
name: "First 2 After 'b', Ascending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: false},
pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2)), After: usecasex.Cursor("b").Ref()},
expected: []string{"c", "d"},
},
{
name: "First 2 After 'd', Descending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: true},
pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2)), After: usecasex.Cursor("d").Ref()},
expected: []string{"c", "b"},
},
{
name: "Last 2 Before 'd', Ascending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: false},
pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2)), Before: usecasex.Cursor("d").Ref()},
expected: []string{"b", "c"},
},
{
name: "Last 2 Before 'b', Descending",
sort: &usecasex.Sort{Key: "updatedAt", Reverted: true},
pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2)), Before: usecasex.Cursor("b").Ref()},
expected: []string{"d", "c"},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
con := &consumer{}
_, err := c.Paginate(ctx, bson.M{}, tc.sort, tc.pagination.Wrap(), con)
assert.NoError(t, err)

gotIDs := lo.Map(con.Cursors, func(c usecasex.Cursor, _ int) string {
return string(c)
})
assert.Equal(t, tc.expected, gotIDs)
})
}
}

type consumer struct {
Cursors []usecasex.Cursor
}
Expand Down
Loading