Merge pull request #1421 from cloudflare/gitlab

Fix comments on renamed files

Author: prymitive

docs/changelog.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## v0.73.6
+
+### Fixed
+
+- Fixed a bug in GitLab integration causing some problems fail to create comments when files were renamed.
+
 ## v0.73.5
 
 ### Fixed

internal/reporter/comments.go

@@ -335,6 +335,10 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any, sho
 		if !c.CanDelete(existing) {
 			goto NEXTDelete
 		}
+		slog.Info("Trying to delete a stale existing comment",
+			slog.String("path", existing.path),
+			slog.Int("line", existing.line),
+		)
 		if err := c.Delete(ctx, dst, existing); err != nil {
 			slog.Error("Failed to delete a stale comment",
 				slog.String("reporter", c.Describe()),

internal/reporter/gitlab.go

@@ -191,7 +191,9 @@ func (gl GitLabReporter) List(_ context.Context, dst any) ([]ExistingComment, er
 			c = gl.noteToExisting(disc.ID, note)
 			break
 		}
-		comments = append(comments, c)
+		if c.path != "" && c.line > 0 && c.meta != nil {
+			comments = append(comments, c)
+		}
 	NEXT:
 	}
 	return comments, nil
@@ -426,8 +428,8 @@ func (gl GitLabReporter) unresolveIfPresent(ctx context.Context, dst any, commen
 }
 
 func reportToGitLabDiscussion(pending PendingComment, diffs []*gitlab.MergeRequestDiff, ver *gitlab.MergeRequestDiffVersion) *gitlab.CreateMergeRequestDiscussionOptions {
-	diff := getDiffForPath(diffs, pending.path)
-	if diff == nil {
+	pathDiffs := getDiffsForPath(diffs, pending.path)
+	if len(pathDiffs) == 0 {
 		slog.Debug("Skipping report for path with no GitLab diff",
 			slog.String("path", pending.path),
 		)
@@ -441,39 +443,51 @@ func reportToGitLabDiscussion(pending PendingComment, diffs []*gitlab.MergeReque
 			BaseSHA:      gitlab.Ptr(ver.BaseCommitSHA),
 			HeadSHA:      gitlab.Ptr(ver.HeadCommitSHA),
 			StartSHA:     gitlab.Ptr(ver.StartCommitSHA),
-			OldPath:      gitlab.Ptr(diff.OldPath),
-			NewPath:      gitlab.Ptr(diff.NewPath),
 		},
 	}
 
-	dl, ok := diffLineFor(parseDiffLines(diff.Diff), pending.line)
-	switch {
-	case !ok:
-		// No diffLine for this line, most likely unmodified ?.
-		d.Position.NewLine = gitlab.Ptr(pending.line)
-		d.Position.OldLine = gitlab.Ptr(pending.line)
-	case pending.anchor == checks.AnchorBefore:
-		// Comment on removed line.
-		d.Position.OldLine = gitlab.Ptr(dl.old)
-	case ok && !dl.wasModified:
-		// Comment on unmodified line.
-		d.Position.NewLine = gitlab.Ptr(dl.new)
-		d.Position.OldLine = gitlab.Ptr(dl.old)
-	default:
-		// Comment on new or modified line.
-		d.Position.NewLine = gitlab.Ptr(dl.new)
+	var renamed bool
+	for _, diff := range pathDiffs {
+		d.Position.OldPath = gitlab.Ptr(diff.OldPath)
+		if d.Position.NewPath == nil {
+			d.Position.NewPath = gitlab.Ptr(diff.NewPath)
+		}
+		if diff.OldPath != diff.NewPath {
+			renamed = true
+		}
+
+		dl, ok := diffLineFor(parseDiffLines(diff.Diff), pending.line)
+		switch {
+		case !ok:
+			// No diffLine for this line, could be a file rename.
+			d.Position.NewLine = gitlab.Ptr(pending.line)
+			d.Position.OldLine = gitlab.Ptr(pending.line)
+		case pending.anchor == checks.AnchorBefore:
+			// Comment on removed line.
+			d.Position.OldLine = gitlab.Ptr(dl.old)
+		case ok && !dl.wasModified:
+			// Comment on unmodified line.
+			d.Position.NewLine = gitlab.Ptr(dl.new)
+			d.Position.OldLine = gitlab.Ptr(dl.old)
+		default:
+			// Comment on new or modified line.
+			d.Position.NewLine = gitlab.Ptr(dl.new)
+		}
+	}
+	if renamed {
+		d.Position.OldLine = nil
 	}
 
 	return &d
 }
 
-func getDiffForPath(diffs []*gitlab.MergeRequestDiff, path string) *gitlab.MergeRequestDiff {
+func getDiffsForPath(diffs []*gitlab.MergeRequestDiff, path string) (changes []*gitlab.MergeRequestDiff) {
 	for _, change := range diffs {
 		if change.NewPath == path {
-			return change
+			changes = append(changes, change)
 		}
 	}
-	return nil
+	return changes
 }
 
 type diffLine struct {

internal/reporter/gitlab_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 	"testing"
@@ -234,6 +235,19 @@ func TestGitLabReporter(t *testing.T) {
 		maxComments    int
 		showDuplicates bool
 	}{
+		{
+			description: "bogus JSON responses",
+			timeout:     time.Minute,
+			maxComments: 1,
+			summary:     reporter.NewSummary([]reporter.Report{fooReport}),
+			mock: httpmock.New(func(s *httpmock.Server) {
+				s.ExpectGet(apiUser).ReturnJSON(gitlab.User{ID: 123})
+				s.ExpectGet(regexp.MustCompile(".+")).Times(4).Return(`[{"iid": 333}]`)
+			}),
+			errorHandler: func(err error) error {
+				return err
+			},
+		},
 		{
 			description: "empty list of merge requests",
 			timeout:     time.Minute,
@@ -878,6 +892,90 @@ Below is the list of checks that were disabled for each Prometheus server define
 				return err
 			},
 		},
+		{
+			description: "rule moved to a different file, old deleted",
+			timeout:     time.Minute,
+			maxComments: 1,
+			summary: reporter.NewSummary([]reporter.Report{
+				{
+					Path: discovery.Path{
+						SymlinkTarget: mockPath,
+						Name:          mockPath,
+					},
+					ModifiedLines: []int{2, 3},
+					Rule:          mockFile.Groups[0].Rules[0],
+					Problem: checks.Problem{
+						Reporter: "a",
+						Summary:  "foo error1",
+						Details:  "foo details",
+						Diagnostics: []diags.Diagnostic{
+							{
+								Message: "Diagnostic message",
+								Pos: diags.PositionRanges{
+									{
+										Line:        2,
+										FirstColumn: 3,
+										LastColumn:  24,
+									},
+									{
+										Line:        3,
+										FirstColumn: 3,
+										LastColumn:  15,
+									},
+								},
+								FirstColumn: 1,
+								LastColumn:  24,
+							},
+						},
+						Lines:    diags.LineRange{First: 1, Last: 3},
+						Severity: checks.Fatal,
+						Anchor:   checks.AnchorAfter,
+					},
+				},
+			}),
+			mock: httpmock.New(func(s *httpmock.Server) {
+				s.ExpectGet(apiUser).ReturnJSON(gitlab.User{ID: 123})
+				s.ExpectGet(apiOpenMergeRequests).ReturnJSON([]gitlab.BasicMergeRequest{
+					{IID: 1},
+				})
+				s.ExpectGet(apiVersions(1)).ReturnJSON([]gitlab.MergeRequestDiffVersion{
+					{ID: 2, HeadCommitSHA: "head", BaseCommitSHA: "base", StartCommitSHA: "start"},
+					{ID: 1, HeadCommitSHA: "head", BaseCommitSHA: "base", StartCommitSHA: "start"},
+				})
+				s.ExpectGet(apiDiffs(1)).ReturnJSON([]gitlab.MergeRequestDiff{
+					{
+						Diff:        "",
+						NewPath:     mockPath,
+						OldPath:     "foo.old",
+						AMode:       "100644",
+						BMode:       "100644",
+						RenamedFile: true,
+					},
+					{
+						Diff:    fooDiff,
+						NewPath: "foo.txt",
+						OldPath: "foo.txt",
+					},
+				})
+				s.ExpectGet(apiDiscussions(1, true)).ReturnJSON([]gitlab.Discussion{})
+				s.ExpectPost(apiDiscussions(1, false)).WithBodyJSON(gitlab.CreateMergeRequestDiscussionOptions{
+					Body: discBodyWithDiag("a", "foo error1", "foo details", "```yaml\n2 | - record: target is down\n3 |   expr: up == 0\n      ^^ \n```", "Diagnostic message"),
+					Position: gitlab.Ptr(gitlab.PositionOptions{
+						BaseSHA:      gitlab.Ptr("base"),
+						StartSHA:     gitlab.Ptr("start"),
+						HeadSHA:      gitlab.Ptr("head"),
+						OldPath:      gitlab.Ptr("foo.old"),
+						NewPath:      gitlab.Ptr(mockPath),
+						PositionType: gitlab.Ptr("text"),
+						NewLine:      gitlab.Ptr(3),
+						// Old file is gone so we don't have OldLine here at all
+					}),
+				}).ReturnJSON(gitlab.Response{})
+			}),
+			errorHandler: func(err error) error {
+				return err
+			},
+		},
 	}
 
 	for _, tc := range testCases {