fix(gitlab): ensure GetFiles and CreateComment pages over API results

Gitlab's commit-diff API  defaults to return 20 files per page. Without
paging through the files, cel funcs like `files.all` and `pathChanged`
will only match 20 files on push events.

This commits also adds paging logic when listing commits in the gitlab
CreateComment function.

Previously, only the first 100 notes on a MR were compared when finding
the note to update. Paging through the list of merge request notes
ensures that if a MR has more than 100 comments we will still update
the appropriate comment.

This commit also creates default gitlab paging options which are used by
all gitlab API calls that paginate through the response.

Co-authored-by: Cursor <noreply@cursor.com>

Author: 2 people

pkg/provider/gitlab/acl.go

@@ -72,7 +72,7 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e
 	}
 
 	var nextPage int
-	opt := &gitlab.ListMergeRequestDiscussionsOptions{Page: page}
+	opt := &gitlab.ListMergeRequestDiscussionsOptions{Page: page, PerPage: defaultGitlabListOptions.PerPage}
 	discussions, resp, err := v.Client().Discussions.ListMergeRequestDiscussions(v.targetProjectID, event.PullRequestNumber, opt)
 	if err != nil || len(discussions) == 0 {
 		return false, err

pkg/provider/gitlab/gitlab.go

@@ -68,6 +68,10 @@ type Provider struct {
 	cachedChangedFiles *changedfiles.ChangedFiles
 }
 
+var defaultGitlabListOptions = gitlab.ListOptions{
+	PerPage: 100,
+}
+
 func (v *Provider) Client() *gitlab.Client {
 	providerMetrics.RecordAPIUsage(
 		v.Logger,
@@ -99,32 +103,46 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
 
 	// List comments of the merge request
 	if updateMarker != "" {
-		comments, _, err := v.Client().Notes.ListMergeRequestNotes(event.TargetProjectID, event.PullRequestNumber, &gitlab.ListMergeRequestNotesOptions{
-			ListOptions: gitlab.ListOptions{
-				Page:    1,
-				PerPage: 100,
-			},
-		})
-		if err != nil {
-			return err
-		}
+		commentRe := regexp.MustCompile(updateMarker)
+		options := []gitlab.RequestOptionFunc{}
 
-		re := regexp.MustCompile(updateMarker)
-		for _, comment := range comments {
-			if re.MatchString(comment.Body) {
-				_, _, err := v.Client().Notes.UpdateMergeRequestNote(event.TargetProjectID, event.PullRequestNumber, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
-					Body: &commit,
-				})
+		for {
+			comments, resp, err := v.Client().Notes.ListMergeRequestNotes(event.TargetProjectID, event.PullRequestNumber, &gitlab.ListMergeRequestNotesOptions{ListOptions: defaultGitlabListOptions}, options...)
+			if err != nil {
 				return err
 			}
+
+			for _, comment := range comments {
+				if commentRe.MatchString(comment.Body) {
+					_, _, err := v.Client().Notes.UpdateMergeRequestNote(event.TargetProjectID, event.PullRequestNumber, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
+						Body: &commit,
+					})
+					if err != nil {
+						return fmt.Errorf("unable to update merge request note: %w", err)
+					}
+					return nil
+				}
+			}
+
+			// Exit the loop when we've seen all pages.
+			if resp.NextLink == "" {
+				break
+			}
+
+			// Otherwise, set param to query the next page
+			options = []gitlab.RequestOptionFunc{
+				gitlab.WithKeysetPaginationParameters(resp.NextLink),
+			}
 		}
 	}
 
 	_, _, err := v.Client().Notes.CreateMergeRequestNote(event.TargetProjectID, event.PullRequestNumber, &gitlab.CreateMergeRequestNoteOptions{
 		Body: &commit,
 	})
-
-	return err
+	if err != nil {
+		return fmt.Errorf("unable to create merge request note: %w", err)
+	}
+	return nil
 }
 
 // CheckPolicyAllowing TODO: Implement ME.
@@ -386,7 +404,7 @@ func (v *Provider) GetTektonDir(_ context.Context, event *info.Event, path, prov
 		ListOptions: gitlab.ListOptions{
 			OrderBy:    "id",
 			Pagination: "keyset",
-			PerPage:    20,
+			PerPage:    defaultGitlabListOptions.PerPage,
 			Sort:       "asc",
 		},
 	}
@@ -524,7 +542,7 @@ func (v *Provider) fetchChangedFiles(_ context.Context, runevent *info.Event) (c
 			ListOptions: gitlab.ListOptions{
 				OrderBy:    "id",
 				Pagination: "keyset",
-				PerPage:    20,
+				PerPage:    defaultGitlabListOptions.PerPage,
 				Sort:       "asc",
 			},
 		}
@@ -564,23 +582,38 @@ func (v *Provider) fetchChangedFiles(_ context.Context, runevent *info.Event) (c
 			}
 		}
 	case triggertype.Push:
-		pushChanges, _, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &gitlab.GetCommitDiffOptions{})
-		if err != nil {
-			return changedfiles.ChangedFiles{}, err
-		}
-		for _, change := range pushChanges {
-			changedFiles.All = append(changedFiles.All, change.NewPath)
-			if change.NewFile {
-				changedFiles.Added = append(changedFiles.Added, change.NewPath)
+		options := gitlab.GetCommitDiffOptions{ListOptions: defaultGitlabListOptions}
+		pageOpts := []gitlab.RequestOptionFunc{}
+
+		for {
+			pushChanges, resp, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &options, pageOpts...)
+			if err != nil {
+				return changedfiles.ChangedFiles{}, err
 			}
-			if change.DeletedFile {
-				changedFiles.Deleted = append(changedFiles.Deleted, change.NewPath)
+
+			for _, change := range pushChanges {
+				changedFiles.All = append(changedFiles.All, change.NewPath)
+				if change.NewFile {
+					changedFiles.Added = append(changedFiles.Added, change.NewPath)
+				}
+				if change.DeletedFile {
+					changedFiles.Deleted = append(changedFiles.Deleted, change.NewPath)
+				}
+				if !change.RenamedFile && !change.DeletedFile && !change.NewFile {
+					changedFiles.Modified = append(changedFiles.Modified, change.NewPath)
+				}
+				if change.RenamedFile {
+					changedFiles.Renamed = append(changedFiles.Renamed, change.NewPath)
+				}
 			}
-			if !change.RenamedFile && !change.DeletedFile && !change.NewFile {
-				changedFiles.Modified = append(changedFiles.Modified, change.NewPath)
+
+			if resp.NextLink == "" {
+				// Exit the loop when we've seen all pages.
+				break
 			}
-			if change.RenamedFile {
-				changedFiles.Renamed = append(changedFiles.Renamed, change.NewPath)
+			// Otherwise, set param to query the next page
+			pageOpts = []gitlab.RequestOptionFunc{
+				gitlab.WithKeysetPaginationParameters(resp.NextLink),
 			}
 		}
 	default:

pkg/provider/gitlab/gitlab_test.go

@@ -1306,6 +1306,72 @@ func TestGetFiles(t *testing.T) {
 	}
 }
 
+func TestGetFilesPaging(t *testing.T) {
+	tests := []struct {
+		name  string
+		event *info.Event
+	}{
+		{
+			name: "pull-request",
+			event: &info.Event{
+				TriggerTarget: "pull_request",
+				Organization:  "owner",
+				Repository:    "repository",
+			},
+		},
+		{
+			name: "push",
+			event: &info.Event{
+				TriggerTarget: "push",
+				Organization:  "owner",
+				Repository:    "repository",
+				SHA:           "shacommitinfo",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			metrics.ResetMetrics()
+			fakeclient, mux, teardown := thelp.Setup(t)
+			defer teardown()
+
+			if tt.event.TriggerTarget == "pull_request" {
+				mux.HandleFunc(fmt.Sprintf("/projects/0/merge_requests/%d/diffs",
+					tt.event.PullRequestNumber), func(rw http.ResponseWriter, req *http.Request) {
+					pageCount := thelp.SetPagingHeader(t, rw, req, 5)
+					jeez, err := json.Marshal([]*gitlab.MergeRequestDiff{{NewPath: fmt.Sprintf("change-%d.txt", pageCount)}})
+					assert.NilError(t, err)
+					_, _ = rw.Write(jeez)
+				})
+			}
+			if tt.event.TriggerTarget == "push" {
+				mux.HandleFunc(fmt.Sprintf("/projects/0/repository/commits/%s/diff",
+					tt.event.SHA), func(rw http.ResponseWriter, req *http.Request) {
+					pageCount := thelp.SetPagingHeader(t, rw, req, 5)
+					jeez, err := json.Marshal([]*gitlab.Diff{{NewPath: fmt.Sprintf("change-%d.txt", pageCount)}})
+					assert.NilError(t, err)
+					_, _ = rw.Write(jeez)
+				})
+			}
+
+			metricsTags := map[string]string{"provider": "api.gitlab.com", "event-type": string(tt.event.TriggerTarget)}
+			metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count")
+
+			providerInfo := &Provider{gitlabClient: fakeclient, sourceProjectID: 0, targetProjectID: 0, triggerEvent: string(tt.event.TriggerTarget), apiURL: "api.gitlab.com"}
+			changedFiles, err := providerInfo.GetFiles(ctx, tt.event)
+			assert.NilError(t, err, nil)
+			assert.DeepEqual(t, changedFiles.All, []string{"change-1.txt", "change-2.txt", "change-3.txt", "change-4.txt", "change-5.txt"})
+			assert.Equal(t, len(changedFiles.Modified), 5)
+
+			// Check caching
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 5)
+			_, _ = providerInfo.GetFiles(ctx, tt.event)
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 5)
+		})
+	}
+}
+
 func TestIsHeadCommitOfBranch(t *testing.T) {
 	tests := []struct {
 		name          string
@@ -1484,3 +1550,50 @@ func TestGitLabCreateComment(t *testing.T) {
 		})
 	}
 }
+
+func TestGitLabCreateCommentPaging(t *testing.T) {
+	updated := false
+	event := &info.Event{PullRequestNumber: 123, TargetProjectID: 666}
+	commit := "Updated Comment"
+	updateMarker := "MARKER"
+	mockResponses := map[string]func(rw http.ResponseWriter, _ *http.Request){
+		"/projects/666/merge_requests/123/notes": func(rw http.ResponseWriter, r *http.Request) {
+			if r.Method == http.MethodGet {
+				page := thelp.SetPagingHeader(t, rw, r, 100)
+				note := "unrelated"
+				if page == 10 {
+					note = "MARKER"
+				} else if page > 10 {
+					t.Error("notes shouldn't be queries past the expected ID")
+				}
+				fmt.Fprintf(rw, `[{"id": %d, "body": "%s"}]`, page, note)
+			}
+		},
+		"/projects/666/merge_requests/123/notes/{id}": func(rw http.ResponseWriter, r *http.Request) {
+			assert.Equal(t, r.Method, "PUT")
+			if r.PathValue("id") == "10" {
+				rw.WriteHeader(http.StatusOK)
+				updated = true
+			} else {
+				rw.WriteHeader(http.StatusNotFound)
+				t.Errorf("note %s is not intended to be updated", r.PathValue("id"))
+			}
+			fmt.Fprint(rw, `{}`)
+		},
+	}
+
+	fakeclient, mux, teardown := thelp.Setup(t)
+	defer teardown()
+
+	for endpoint, handler := range mockResponses {
+		mux.HandleFunc(endpoint, handler)
+	}
+
+	p := &Provider{
+		sourceProjectID: 666,
+		gitlabClient:    fakeclient,
+	}
+	err := p.CreateComment(context.Background(), event, commit, updateMarker)
+	assert.NilError(t, err)
+	assert.Assert(t, updated == true, "comment update handler has not been called")
+}

pkg/provider/gitlab/test/test.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -176,6 +177,33 @@ func MuxGetFile(mux *http.ServeMux, pid int, fname, content string, wantErr bool
 	})
 }
 
+// SetPagingHeader sets the gitlab paging header "link" to the next page. If the requested page
+// is equal to or higher than maxPage, no next page header is set. The requested page number
+// is returned.
+func SetPagingHeader(t *testing.T, rw http.ResponseWriter, req *http.Request, maxPage int) int {
+	t.Helper()
+	url := req.URL
+	query := url.Query()
+	lastPageStr := query.Get("page")
+	if lastPageStr == "" {
+		lastPageStr = "1"
+	}
+	lastPage, err := strconv.Atoi(lastPageStr)
+	if err != nil {
+		t.Errorf("unable to parse page number %s: %v", lastPageStr, err)
+		return 0
+	}
+
+	if lastPage < maxPage {
+		query.Set("page", strconv.Itoa(lastPage+1))
+		url.RawQuery = query.Encode()
+		header := fmt.Sprintf("<%s>; rel=\"next\",", url.String())
+		rw.Header().Add("link", header)
+	}
+
+	return lastPage
+}
+
 type TEvent struct {
 	Username, DefaultBranch, URL, SHA, SHAurl, SHAtitle, Headbranch, Basebranch, HeadURL, BaseURL string
 	UserID, MRID, TargetProjectID, SourceProjectID, NoteID                                        int