From e4cc7e5e2643e84aff57ad89da5d38bb9f1acb1b Mon Sep 17 00:00:00 2001 From: tantaka tomokazu Date: Wed, 20 Nov 2024 15:00:35 +0900 Subject: [PATCH] fix(server): sort duplicate key error (#55) * fix sort duplicate key error * add test case * fix test error * fix lint * add Mutex lock --------- Co-authored-by: tomokazu tantaka --- Makefile | 15 ++ account/accountdomain/workspace/member.go | 5 + mongox/pagination.go | 30 ++- mongox/pagination_test.go | 272 ++++++++++++++-------- 4 files changed, 209 insertions(+), 113 deletions(-) create mode 100644 Makefile diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..af3af0d --- /dev/null +++ b/Makefile @@ -0,0 +1,15 @@ +help: + @echo "Usage:" + @echo " make " + @echo "" + @echo "Targets:" + @echo " lint Run golangci-lint with auto-fix" + @echo " test Run unit tests with race detector in short mode" + +lint: + golangci-lint run --fix + +test: + go test -race -short -v ./... + +.PHONY: lint test diff --git a/account/accountdomain/workspace/member.go b/account/accountdomain/workspace/member.go index a1c101e..4978b20 100644 --- a/account/accountdomain/workspace/member.go +++ b/account/accountdomain/workspace/member.go @@ -2,6 +2,7 @@ package workspace import ( "sort" + "sync" "github.com/reearth/reearthx/account/accountdomain/user" "github.com/reearth/reearthx/i18n" @@ -29,6 +30,7 @@ type Members struct { users map[UserID]Member integrations map[IntegrationID]Member fixed bool + mu sync.Mutex } func NewMembers() *Members { @@ -221,6 +223,9 @@ func (m *Members) AddIntegration(iid IntegrationID, role Role, i UserID) error { } func (m *Members) Leave(u UserID) error { + m.mu.Lock() + defer m.mu.Unlock() + if m.fixed { return ErrCannotModifyPersonalWorkspace } diff --git a/mongox/pagination.go b/mongox/pagination.go index c168df0..188f433 100644 --- a/mongox/pagination.go +++ b/mongox/pagination.go @@ -8,6 +8,7 @@ import ( "github.com/reearth/reearthx/rerror" "github.com/reearth/reearthx/usecasex" "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" ) @@ -40,8 +41,15 @@ func (c *Collection) Paginate(ctx context.Context, rawFilter any, s *usecasex.So sortOrder *= -1 } + var sort primitive.D + if sortKey == idKey { + sort = bson.D{{Key: sortKey, Value: sortOrder}} + } else { + sort = bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}} + } + findOpts := options.Find(). - SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}). + SetSort(sort). SetLimit(limit(*p)) if p.Offset != nil { @@ -199,11 +207,11 @@ func (c *Collection) aggregateFilter(ctx context.Context, p usecasex.Pagination, } func aggregateOptionsFromPagination(_ usecasex.Pagination, _ *usecasex.Sort) *options.AggregateOptions { - collation := options.Collation{ - Locale: "en", - Strength: 2, - } - return options.Aggregate().SetAllowDiskUse(true).SetCollation(&collation) + collation := options.Collation{ + Locale: "en", + Strength: 2, + } + return options.Aggregate().SetAllowDiskUse(true).SetCollation(&collation) } func (c *Collection) pageFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) (bson.M, error) { @@ -280,11 +288,11 @@ func (c *Collection) getCursorDocument(ctx context.Context, cursor usecasex.Curs } func sortFilter(p usecasex.Pagination, s *usecasex.Sort) bson.D { - var sortOptions bson.D - if s != nil && s.Key != "" && s.Key != idKey { - sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: sortDirection(p, s)}) - } - return append(sortOptions, bson.E{Key: idKey, Value: sortDirection(p, s)}) + var sortOptions bson.D + if s != nil && s.Key != "" && s.Key != idKey { + sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: sortDirection(p, s)}) + } + return append(sortOptions, bson.E{Key: idKey, Value: sortDirection(p, s)}) } func limit(p usecasex.Pagination) int64 { diff --git a/mongox/pagination_test.go b/mongox/pagination_test.go index b5bd198..3d34bd0 100644 --- a/mongox/pagination_test.go +++ b/mongox/pagination_test.go @@ -246,80 +246,76 @@ func TestClientCollection_PaginateAggregation(t *testing.T) { } func TestClientCollection_PaginateWithUpdatedAtSort(t *testing.T) { - ctx := context.Background() - initDB := mongotest.Connect(t) - c := NewCollection(initDB(t).Collection("test")) - - 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} - })) - - sortOpt := &usecasex.Sort{Key: "updatedAt", Reverted: false} - - p := usecasex.CursorPagination{ - First: lo.ToPtr(int64(2)), - } - - con := &consumer{} - _, err := c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) - assert.NoError(t, err) - assert.Equal(t, []usecasex.Cursor{"a", "b"}, con.Cursors) - - - p = usecasex.CursorPagination{ - Last: lo.ToPtr(int64(2)), - } - - con = &consumer{} - _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) - assert.NoError(t, err) - assert.Equal(t, []usecasex.Cursor{"d", "e"}, con.Cursors) - - - p = usecasex.CursorPagination{ - First: lo.ToPtr(int64(2)), - After: usecasex.Cursor("b").Ref(), - } - - con = &consumer{} - _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) - assert.NoError(t, err) - assert.Equal(t, []usecasex.Cursor{"c", "d"}, con.Cursors) - - - p = usecasex.CursorPagination{ - Last: lo.ToPtr(int64(2)), - Before: usecasex.Cursor("d").Ref(), - } - - con = &consumer{} - _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) - assert.NoError(t, err) - assert.Equal(t, []usecasex.Cursor{"b", "c"}, con.Cursors) - - - p = usecasex.CursorPagination{ - Last: lo.ToPtr(int64(3)), - } - - con = &consumer{} - _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) - assert.NoError(t, err) - assert.Equal(t, []usecasex.Cursor{"c", "d", "e"}, con.Cursors) + ctx := context.Background() + initDB := mongotest.Connect(t) + c := NewCollection(initDB(t).Collection("test")) + + 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} + })) + + sortOpt := &usecasex.Sort{Key: "updatedAt", Reverted: false} + + p := usecasex.CursorPagination{ + First: lo.ToPtr(int64(2)), + } + + con := &consumer{} + _, err := c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, []usecasex.Cursor{"a", "b"}, con.Cursors) + + p = usecasex.CursorPagination{ + Last: lo.ToPtr(int64(2)), + } + + con = &consumer{} + _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, []usecasex.Cursor{"d", "e"}, con.Cursors) + + p = usecasex.CursorPagination{ + First: lo.ToPtr(int64(2)), + After: usecasex.Cursor("b").Ref(), + } + + con = &consumer{} + _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, []usecasex.Cursor{"c", "d"}, con.Cursors) + + p = usecasex.CursorPagination{ + Last: lo.ToPtr(int64(2)), + Before: usecasex.Cursor("d").Ref(), + } + + con = &consumer{} + _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, []usecasex.Cursor{"b", "c"}, con.Cursors) + + p = usecasex.CursorPagination{ + Last: lo.ToPtr(int64(3)), + } + + con = &consumer{} + _, err = c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, []usecasex.Cursor{"c", "d", "e"}, con.Cursors) } func TestClientCollection_DetailedPagination(t *testing.T) { @@ -346,58 +342,58 @@ func TestClientCollection_DetailedPagination(t *testing.T) { })) testCases := []struct { - name string - sort *usecasex.Sort + name string + sort *usecasex.Sort pagination *usecasex.CursorPagination - expected []string + expected []string }{ { - name: "First 2, Ascending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, + name: "First 2, Ascending", + sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2))}, - expected: []string{"a", "b"}, + expected: []string{"a", "b"}, }, { - name: "First 2, Descending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, + name: "First 2, Descending", + sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, pagination: &usecasex.CursorPagination{First: lo.ToPtr(int64(2))}, - expected: []string{"e", "d"}, + expected: []string{"e", "d"}, }, { - name: "Last 2, Ascending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, + name: "Last 2, Ascending", + sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2))}, - expected: []string{"d", "e"}, + expected: []string{"d", "e"}, }, { - name: "Last 2, Descending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, + name: "Last 2, Descending", + sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, pagination: &usecasex.CursorPagination{Last: lo.ToPtr(int64(2))}, - expected: []string{"b", "a"}, + expected: []string{"b", "a"}, }, { - name: "First 2 After 'b', Ascending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, + 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"}, + expected: []string{"c", "d"}, }, { - name: "First 2 After 'd', Descending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, + 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"}, + expected: []string{"c", "b"}, }, { - name: "Last 2 Before 'd', Ascending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: false}, + 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"}, + expected: []string{"b", "c"}, }, { - name: "Last 2 Before 'b', Descending", - sort: &usecasex.Sort{Key: "updatedAt", Reverted: true}, + 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"}, + expected: []string{"d", "c"}, }, } @@ -406,7 +402,7 @@ func TestClientCollection_DetailedPagination(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) }) @@ -423,3 +419,75 @@ func (c *consumer) Consume(b bson.Raw) error { c.Cursors = append(c.Cursors, lo.FromPtr(lo.Must(getCursor(b)))) return nil } + +func TestPaginate_SortLogic(t *testing.T) { + ctx := context.Background() + initDB := mongotest.Connect(t) + c := NewCollection(initDB(t).Collection("test")) + + seeds := []struct { + id string + updatedAt int64 + }{ + {"a", 1000}, + {"b", 2000}, + {"c", 3000}, + } + + _, _ = 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} + })) + + cases := []struct { + name string + sortKey string + sortOrder int + expectedOrder []usecasex.Cursor + }{ + { + name: "Sort by id ascending", + sortKey: "id", + sortOrder: 1, + expectedOrder: []usecasex.Cursor{"a", "b", "c"}, + }, + { + name: "Sort by id descending", + sortKey: "id", + sortOrder: -1, + expectedOrder: []usecasex.Cursor{"c", "b", "a"}, + }, + { + name: "Sort by updatedAt ascending", + sortKey: "updatedAt", + sortOrder: 1, + expectedOrder: []usecasex.Cursor{"a", "b", "c"}, + }, + { + name: "Sort by updatedAt descending", + sortKey: "updatedAt", + sortOrder: -1, + expectedOrder: []usecasex.Cursor{"c", "b", "a"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sortOpt := &usecasex.Sort{ + Key: tc.sortKey, + Reverted: tc.sortOrder == -1, + } + p := usecasex.CursorPagination{ + First: lo.ToPtr(int64(len(seeds))), + } + + con := &consumer{} + _, err := c.Paginate(ctx, bson.M{}, sortOpt, p.Wrap(), con) + assert.NoError(t, err) + assert.Equal(t, tc.expectedOrder, con.Cursors) + + }) + } +}