fix: comment list pagination (#21)

* fix: comment list pagination * update aqua * fix lint * fix conflict * fix review work * add test case for pagination sentinel --------- Co-authored-by: hirosassa <hiro.sassa@gmail.com>
hirosassa · Apr 11, 2024 · 0cb63c9 · 0cb63c9
1 parent 2b267e0
commit 0cb63c9
Show file tree

Hide file tree

Showing 2 changed files with 122 additions and 19 deletions.
diff --git a/pkg/notifier/gitlab/comment.go b/pkg/notifier/gitlab/comment.go
@@ -3,9 +3,15 @@ package gitlab
 import (
 	"fmt"
 
+	"github.com/sirupsen/logrus"
 	gitlab "github.com/xanzy/go-gitlab"
 )
 
+const (
+	listPerPage = 100
+	maxPages    = 100
+)
+
 // CommentService handles communication with the comment related
 // methods of GitLab API
 type CommentService service
@@ -59,9 +65,40 @@ func (g *CommentService) Patch(note int, body string, opt PostOptions) error {
 
 // List lists comments on GitLab merge requests
 func (g *CommentService) List(number int) ([]*gitlab.Note, error) {
-	comments, _, err := g.client.API.ListMergeRequestNotes(
-		number,
-		&gitlab.ListMergeRequestNotesOptions{},
-	)
-	return comments, err
+	allComments := make([]*gitlab.Note, 0)
+
+	opt := &gitlab.ListMergeRequestNotesOptions{
+		ListOptions: gitlab.ListOptions{
+			Page:    1,
+			PerPage: listPerPage,
+		},
+	}
+
+	for sentinel := 1; ; sentinel++ {
+		comments, resp, err := g.client.API.ListMergeRequestNotes(
+			number,
+			opt,
+		)
+		if err != nil {
+			return nil, err
+		}
+
+		allComments = append(allComments, comments...)
+
+		if resp.NextPage == 0 {
+			break
+		}
+
+		if sentinel >= maxPages {
+			logE := logrus.WithFields(logrus.Fields{
+				"program": "tfcmt",
+			})
+			logE.WithField("maxPages", maxPages).Debug("gitlab.comment.list: too many pages, something went wrong")
+			break
+		}
+
+		opt.Page = resp.NextPage
+	}
+
+	return allComments, nil
 }
diff --git a/pkg/notifier/gitlab/comment_test.go b/pkg/notifier/gitlab/comment_test.go
@@ -124,16 +124,6 @@ func TestCommentPost(t *testing.T) {
 
 func TestCommentList(t *testing.T) {
 	t.Parallel()
-	comments := []*gitlab.Note{
-		{
-			ID:   371748792,
-			Body: "comment 1",
-		},
-		{
-			ID:   371765743,
-			Body: "comment 2",
-		},
-	}
 	testCases := []struct {
 		name                string
 		config              Config
@@ -147,12 +137,56 @@ func TestCommentList(t *testing.T) {
 			config: newFakeConfig(),
 			createMockGitLabAPI: func(ctrl *gomock.Controller) *gitlabmock.MockAPI {
 				api := gitlabmock.NewMockAPI(ctrl)
-				api.EXPECT().ListMergeRequestNotes(1, gomock.Any()).Return(comments, &gitlab.Response{TotalPages: 1, NextPage: 1}, nil)
+				api.EXPECT().ListMergeRequestNotes(1, gomock.Any()).MaxTimes(2).DoAndReturn(
+					func(mergeRequest int, opt *gitlab.ListMergeRequestNotesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Note, *gitlab.Response, error) {
+						res := []*gitlab.Note{
+							// same response to any page for now
+							{
+								ID:   371748792,
+								Body: "comment 1",
+							},
+							{
+								ID:   371765743,
+								Body: "comment 2",
+							},
+						}
+
+						// fake pagination with 2 pages
+						resp := &gitlab.Response{
+							NextPage: 0,
+						}
+						if opt.Page == 1 {
+							resp.NextPage = 2
+						}
+
+						return res, resp, nil
+					},
+				)
+
 				return api
 			},
-			number:   1,
-			ok:       true,
-			comments: comments,
+			number: 1,
+			ok:     true,
+			comments: []*gitlab.Note{
+				// page1
+				{
+					ID:   371748792,
+					Body: "comment 1",
+				},
+				{
+					ID:   371765743,
+					Body: "comment 2",
+				},
+				// page2
+				{
+					ID:   371748792,
+					Body: "comment 1",
+				},
+				{
+					ID:   371765743,
+					Body: "comment 2",
+				},
+			},
 		},
 	}
 
@@ -180,3 +214,35 @@ func TestCommentList(t *testing.T) {
 		})
 	}
 }
+
+func TestCommentListSentinel(t *testing.T) {
+	t.Parallel()
+
+	client, err := NewClient(newFakeConfig())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	createMockGitLabAPI := func(ctrl *gomock.Controller) *gitlabmock.MockAPI {
+		api := gitlabmock.NewMockAPI(ctrl)
+		api.EXPECT().ListMergeRequestNotes(1, gomock.Any()).MaxTimes(100).Return(
+			[]*gitlab.Note{},
+			&gitlab.Response{
+				NextPage: 1, // just cause infinite loop
+			},
+			nil,
+		)
+
+		return api
+	}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	client.API = createMockGitLabAPI(mockCtrl)
+
+	_, err = client.Comment.List(1) // no assert res, only assert `.MaxTimes(100)`
+	if err != nil {
+		t.Errorf("got error %q", err)
+	}
+}