fix(gitlab): workaround the gitlab diff API limit when necessary

When the Gitlab Merge Request exceeds the Diff API limit[1] the merge
request diff API will not return the full list of changed files. To
circumvent this limitation, we can use the Get Merge Request API to
detect that the diff exceeds the limit and then switch to the Compare
API to get the full diff. We do not use the Compare API in all instances,
since it does not support paging and would increase the memory used by
PaC during merge request event processing; by only using the Compare API
when we know the limit is exceeded, PaC's memory usage will be more
consistent and only spike when such a merge request is made.

[1] - https://docs.gitlab.com/administration/instance_limits/#diff-limits

Author: aThorp96

pkg/provider/gitlab/gitlab.go

@@ -9,6 +9,7 @@ import (
 	"path"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
@@ -596,24 +597,22 @@ func (v *Provider) fetchChangedFiles(_ context.Context, runevent *info.Event) (c
 
 	switch runevent.TriggerTarget {
 	case triggertype.PullRequest:
-		opt := &gitlab.ListMergeRequestDiffsOptions{
-			ListOptions: gitlab.ListOptions{
-				OrderBy:    "id",
-				Pagination: "keyset",
-				PerPage:    defaultGitlabListOptions.PerPage,
-				Sort:       "asc",
-			},
+		var err error
+		changedFiles, err = v.mergeRequestFilesChanged(runevent)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
 		}
-		options := []gitlab.RequestOptionFunc{}
+	case triggertype.Push:
+		options := gitlab.GetCommitDiffOptions{ListOptions: defaultGitlabListOptions}
+		pageOpts := []gitlab.RequestOptionFunc{}
 
 		for {
-			mrchanges, resp, err := v.Client().MergeRequests.ListMergeRequestDiffs(v.targetProjectID, int64(runevent.PullRequestNumber), opt, options...)
+			pushChanges, resp, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &options, pageOpts...)
 			if err != nil {
-				// TODO: Should this return the files found so far?
 				return changedfiles.ChangedFiles{}, err
 			}
 
-			for _, change := range mrchanges {
+			for _, change := range pushChanges {
 				changedFiles.All = append(changedFiles.All, change.NewPath)
 				if change.NewFile {
 					changedFiles.Added = append(changedFiles.Added, change.NewPath)
@@ -629,27 +628,81 @@ func (v *Provider) fetchChangedFiles(_ context.Context, runevent *info.Event) (c
 				}
 			}
 
-			// Exit the loop when we've seen all pages.
 			if resp.NextLink == "" {
+				// Exit the loop when we've seen all pages.
 				break
 			}
-
 			// Otherwise, set param to query the next page
-			options = []gitlab.RequestOptionFunc{
+			pageOpts = []gitlab.RequestOptionFunc{
 				gitlab.WithKeysetPaginationParameters(resp.NextLink),
 			}
 		}
-	case triggertype.Push:
-		options := gitlab.GetCommitDiffOptions{ListOptions: defaultGitlabListOptions}
-		pageOpts := []gitlab.RequestOptionFunc{}
+	default:
+		// No action necessary
+	}
+	return changedFiles, nil
+}
 
+func (v *Provider) mergeRequestFilesChanged(runevent *info.Event) (changedfiles.ChangedFiles, error) {
+	diffTruncated, changeCount, err := v.isMergeRequestDiffTruncated(v.targetProjectID, int64(runevent.PullRequestNumber))
+	if err != nil {
+		return changedfiles.ChangedFiles{}, err
+	}
+
+	changedFiles := changedfiles.ChangedFiles{
+		All:     make([]string, 0, changeCount),
+		Added:   []string{},
+		Deleted: []string{},
+		Renamed: []string{},
+	}
+
+	options := []gitlab.RequestOptionFunc{}
+
+	// Only use the repository/compare API if the standard merge_request/diff API endpoint will
+	// return a truncated set of changes. The repository/compare API returns the entire set of
+	// changes without paging, so it can have a significantly heavier memory footprint if used
+	// in all cases.
+	if diffTruncated {
+		compareOpts := &gitlab.CompareOptions{
+			From: &runevent.BaseBranch,
+			To:   &runevent.SHA,
+		}
+		comparison, _, err := v.Client().Repositories.Compare(v.targetProjectID, compareOpts, options...)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
+		}
+
+		for _, change := range comparison.Diffs {
+			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)
+			}
+		}
+	} else {
+		diffOpts := &gitlab.ListMergeRequestDiffsOptions{
+			ListOptions: gitlab.ListOptions{
+				OrderBy:    "id",
+				Pagination: "keyset",
+				PerPage:    defaultGitlabListOptions.PerPage,
+				Sort:       "asc",
+			},
+		}
 		for {
-			pushChanges, resp, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &options, pageOpts...)
+			mrchanges, resp, err := v.Client().MergeRequests.ListMergeRequestDiffs(v.targetProjectID, int64(runevent.PullRequestNumber), diffOpts, options...)
 			if err != nil {
+				// TODO: Should this return the files found so far?
 				return changedfiles.ChangedFiles{}, err
 			}
-
-			for _, change := range pushChanges {
+			for _, change := range mrchanges {
 				changedFiles.All = append(changedFiles.All, change.NewPath)
 				if change.NewFile {
 					changedFiles.Added = append(changedFiles.Added, change.NewPath)
@@ -665,21 +718,41 @@ func (v *Provider) fetchChangedFiles(_ context.Context, runevent *info.Event) (c
 				}
 			}
 
+			// Exit the loop when we've seen all pages.
 			if resp.NextLink == "" {
-				// Exit the loop when we've seen all pages.
 				break
 			}
+
 			// Otherwise, set param to query the next page
-			pageOpts = []gitlab.RequestOptionFunc{
+			options = []gitlab.RequestOptionFunc{
 				gitlab.WithKeysetPaginationParameters(resp.NextLink),
 			}
 		}
-	default:
-		// No action necessary
 	}
 	return changedFiles, nil
 }
 
+// isMergeRequestDiffTruncated checks if the merge request is affected by the Gitlab API's Diff Limits.
+// This is determined by the Get Merge Request API's returning a ChangeCount number with a "+" suffix.
+// See also: https://docs.gitlab.com/administration/diff_limits/
+// Returns (bool: isTruncated, int: changeCount, err: error).
+func (v *Provider) isMergeRequestDiffTruncated(projectID, mergeRequestID int64) (bool, int, error) {
+	out, _, err := v.Client().MergeRequests.GetMergeRequest(projectID, mergeRequestID, &gitlab.GetMergeRequestsOptions{})
+	if err != nil {
+		return false, 0, fmt.Errorf("error getting merge request %d: %w", mergeRequestID, err)
+	}
+	fileCount := 0
+	truncated := strings.HasSuffix(out.ChangesCount, "+")
+	if out.ChangesCount != "" {
+		countStr := strings.TrimSuffix(out.ChangesCount, "+")
+		fileCount, err = strconv.Atoi(countStr)
+		if err != nil {
+			return false, 0, err
+		}
+	}
+	return truncated, fileCount, nil
+}
+
 func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) {
 	return "", nil
 }

pkg/provider/gitlab/gitlab_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -1234,13 +1235,15 @@ func TestGetFiles(t *testing.T) {
 		name                             string
 		event                            *info.Event
 		mrchanges                        []*gitlab.MergeRequestDiff
+		diffAPILimitExceeded             bool
 		pushChanges                      []*gitlab.Diff
 		wantAddedFilesCount              int
 		wantDeletedFilesCount            int
 		wantModifiedFilesCount           int
 		wantRenamedFilesCount            int
 		sourceProjectID, targetProjectID int
 		wantError                        bool
+		apiCallCount                     int64
 	}{
 		{
 			name: "pull-request",
@@ -1249,6 +1252,9 @@ func TestGetFiles(t *testing.T) {
 				Organization:      "pullrequestowner",
 				Repository:        "pullrequestrepository",
 				PullRequestNumber: 10,
+				BaseBranch:        "main",
+				HeadBranch:        "feature",
+				SHA:               "abc123",
 			},
 			mrchanges: []*gitlab.MergeRequestDiff{
 				{
@@ -1272,6 +1278,44 @@ func TestGetFiles(t *testing.T) {
 			wantModifiedFilesCount: 1,
 			wantRenamedFilesCount:  1,
 			targetProjectID:        10,
+			apiCallCount:           2,
+		},
+		{
+			name: "merge request exceeding gitlab Diff API",
+			event: &info.Event{
+				TriggerTarget:     "pull_request",
+				Organization:      "pullrequestowner",
+				Repository:        "pullrequestrepository",
+				PullRequestNumber: 10,
+				BaseBranch:        "main",
+				HeadBranch:        "feature",
+				SHA:               "abc123",
+			},
+			mrchanges: []*gitlab.MergeRequestDiff{
+				{
+					NewPath: "modified.yaml",
+				},
+				{
+					NewPath: "added.doc",
+					NewFile: true,
+				},
+				{
+					NewPath:     "removed.yaml",
+					DeletedFile: true,
+				},
+				{
+					NewPath:     "renamed.doc",
+					RenamedFile: true,
+				},
+			},
+			wantAddedFilesCount:    1,
+			wantDeletedFilesCount:  1,
+			wantModifiedFilesCount: 1,
+			wantRenamedFilesCount:  1,
+			targetProjectID:        10,
+			wantError:              false,
+			apiCallCount:           2,
+			diffAPILimitExceeded:   true,
 		},
 		{
 			name: "pull-request with wrong project ID",
@@ -1304,6 +1348,7 @@ func TestGetFiles(t *testing.T) {
 			wantRenamedFilesCount:  0,
 			targetProjectID:        12,
 			wantError:              true,
+			apiCallCount:           1,
 		},
 		{
 			name: "push",
@@ -1335,6 +1380,7 @@ func TestGetFiles(t *testing.T) {
 			wantModifiedFilesCount: 1,
 			wantRenamedFilesCount:  1,
 			sourceProjectID:        0,
+			apiCallCount:           1,
 		},
 	}
 	for _, tt := range tests {
@@ -1343,10 +1389,43 @@ func TestGetFiles(t *testing.T) {
 			metricsutils.ResetMetrics()
 			fakeclient, mux, teardown := thelp.Setup(t)
 			defer teardown()
+
 			if tt.event.TriggerTarget == "pull_request" {
+				mux.HandleFunc(fmt.Sprintf("/projects/10/merge_requests/%d", tt.event.PullRequestNumber),
+					func(rw http.ResponseWriter, _ *http.Request) {
+						resp := gitlab.MergeRequest{
+							ChangesCount: strconv.Itoa(len(tt.mrchanges)),
+						}
+						if tt.diffAPILimitExceeded {
+							resp.ChangesCount = strconv.Itoa(len(tt.mrchanges)-1) + "+"
+						}
+						jeez, err := json.Marshal(resp)
+						assert.NilError(t, err)
+						_, _ = rw.Write(jeez)
+					})
 				mux.HandleFunc(fmt.Sprintf("/projects/10/merge_requests/%d/diffs",
 					tt.event.PullRequestNumber), func(rw http.ResponseWriter, _ *http.Request) {
-					jeez, err := json.Marshal(tt.mrchanges)
+					diffAPIChanges := tt.mrchanges
+					if tt.diffAPILimitExceeded {
+						// The Diff API will not return the full list of files
+						diffAPIChanges = diffAPIChanges[:len(tt.mrchanges)-1]
+					}
+
+					jeez, err := json.Marshal(diffAPIChanges)
+					assert.NilError(t, err)
+					_, _ = rw.Write(jeez)
+				})
+				mux.HandleFunc("/projects/10/repository/compare", func(rw http.ResponseWriter, req *http.Request) {
+					if req.URL.Query().Get("from") != tt.event.BaseBranch || req.URL.Query().Get("to") != tt.event.SHA {
+						t.Errorf("expecting compare API call with 'from=%s&to=%s', got %s", tt.event.BaseBranch, tt.event.SHA, req.URL.Query().Encode())
+					}
+
+					// always return the full list, not subject to the Diff API size limitations
+					diffs := []*gitlab.Diff{}
+					for _, d := range tt.mrchanges {
+						diffs = append(diffs, &gitlab.Diff{NewPath: d.NewPath, NewFile: d.NewFile, RenamedFile: d.RenamedFile, DeletedFile: d.DeletedFile})
+					}
+					jeez, err := json.Marshal(gitlab.Compare{Diffs: diffs})
 					assert.NilError(t, err)
 					_, _ = rw.Write(jeez)
 				})
@@ -1385,14 +1464,14 @@ func TestGetFiles(t *testing.T) {
 			}
 
 			// Check caching
-			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, tt.apiCallCount)
 			_, _ = providerInfo.GetFiles(ctx, tt.event)
 			if tt.wantError {
 				// No caching on error
-				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 2)
+				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, tt.apiCallCount*2)
 			} else {
 				// Cache API results on success
-				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
+				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, tt.apiCallCount)
 			}
 		})
 	}
@@ -1428,10 +1507,23 @@ func TestGetFilesPaging(t *testing.T) {
 			fakeclient, mux, teardown := thelp.Setup(t)
 			defer teardown()
 
+			changeCount := 5
+			apiCallCount := changeCount
 			if tt.event.TriggerTarget == "pull_request" {
+				// Extra request made to check the diff API limit
+				apiCallCount++
+			}
+
+			if tt.event.TriggerTarget == "pull_request" {
+				mux.HandleFunc(fmt.Sprintf("/projects/0/merge_requests/%d",
+					tt.event.PullRequestNumber), func(rw http.ResponseWriter, _ *http.Request) {
+					jeez, err := json.Marshal(gitlab.MergeRequest{ChangesCount: strconv.Itoa(changeCount)})
+					assert.NilError(t, err)
+					_, _ = rw.Write(jeez)
+				})
 				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)
+					pageCount := thelp.SetPagingHeader(t, rw, req, changeCount)
 					jeez, err := json.Marshal([]*gitlab.MergeRequestDiff{{NewPath: fmt.Sprintf("change-%d.txt", pageCount)}})
 					assert.NilError(t, err)
 					_, _ = rw.Write(jeez)
@@ -1440,7 +1532,7 @@ func TestGetFilesPaging(t *testing.T) {
 			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)
+					pageCount := thelp.SetPagingHeader(t, rw, req, changeCount)
 					jeez, err := json.Marshal([]*gitlab.Diff{{NewPath: fmt.Sprintf("change-%d.txt", pageCount)}})
 					assert.NilError(t, err)
 					_, _ = rw.Write(jeez)
@@ -1454,12 +1546,12 @@ func TestGetFilesPaging(t *testing.T) {
 			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)
+			assert.Equal(t, len(changedFiles.Modified), changeCount)
 
 			// Check caching
-			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 5)
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, int64(apiCallCount))
 			_, _ = providerInfo.GetFiles(ctx, tt.event)
-			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 5)
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, int64(apiCallCount))
 		})
 	}
 }