Skip to content

Commit

Permalink
Introduce metadata comment and fingerprint to github-pr-review reporter
Browse files Browse the repository at this point in the history 
Now reviwedog include metadata as HTML comment for github-pr-review
reporter.

We can now use calculated fingerprint to identify existing posted comments.

It resolves the problem w.r.t. related location feature.
Since reviewdog include the latest SHA in body of comment for relate
location feature, comparing comment body cannot be used for identifying
existing comments and it leads to post duplicate comments.
We can avoid this problem with fingerprint check.
  • Loading branch information
@haya14busa
haya14busa committed Jun 18, 2024
1 parent 162c74e commit a8144c8
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 15 deletions.
10 changes: 5 additions & 5 deletions service/commentutil/commentutil.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import (
"github.com/reviewdog/reviewdog/proto/rdf"
)

// `path` to `position`(Lnum for new file) to comment `body`s
// `path` to `position`(Lnum for new file) to comment `body` or `finterprint`
type PostedComments map[string]map[int][]string

// IsPosted returns true if a given comment has been posted in code review service already,
// otherwise returns false. It sees comments with same path, same position,
// and same body as same comments.
func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int, body string) bool {
func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int, bodyOrFingerprint string) bool {
path := c.Result.Diagnostic.GetLocation().GetPath()
if _, ok := p[path]; !ok {
return false
Expand All @@ -25,22 +25,22 @@ func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int, body string)
return false
}
for _, b := range bodies {
if b == body {
if b == bodyOrFingerprint {
return true
}
}
return false
}

// AddPostedComment adds a posted comment.
func (p PostedComments) AddPostedComment(path string, lineNum int, body string) {
func (p PostedComments) AddPostedComment(path string, lineNum int, bodyOrFingerprint string) {
if _, ok := p[path]; !ok {
p[path] = make(map[int][]string)
}
if _, ok := p[path][lineNum]; !ok {
p[path][lineNum] = make([]string, 0)
}
p[path][lineNum] = append(p[path][lineNum], body)
p[path][lineNum] = append(p[path][lineNum], bodyOrFingerprint)
}

// DebugLog outputs posted comments as log for debugging.
Expand Down
63 changes: 59 additions & 4 deletions service/github/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package github

import (
"context"
"encoding/base64"
"errors"
"fmt"
"hash/fnv"
"log"
"net/http"
"os"
Expand All @@ -13,10 +15,12 @@ import (
"sync"

"github.com/google/go-github/v60/github"
"google.golang.org/protobuf/proto"

"github.com/reviewdog/reviewdog"
"github.com/reviewdog/reviewdog/cienv"
"github.com/reviewdog/reviewdog/pathutil"
"github.com/reviewdog/reviewdog/proto/metacomment"
"github.com/reviewdog/reviewdog/proto/rdf"
"github.com/reviewdog/reviewdog/service/commentutil"
"github.com/reviewdog/reviewdog/service/github/githubutils"
Expand Down Expand Up @@ -154,8 +158,12 @@ func (g *PullRequest) postAsReviewComment(ctx context.Context) error {
}
repoBaseHTMLURLForRelatedLoc = repo.GetHTMLURL() + "/blob/" + g.sha
}
body := buildBody(c, repoBaseHTMLURLForRelatedLoc, rootPath)
if g.postedcs.IsPosted(c, githubCommentLine(c), body) {
fprint, err := fingerprint(c.Result.Diagnostic)
if err != nil {
return err
}
body := buildBody(c, repoBaseHTMLURLForRelatedLoc, rootPath, fprint)
if g.postedcs.IsPosted(c, githubCommentLine(c), fprint) {
// it's already posted. skip it.
continue
}
Expand Down Expand Up @@ -307,7 +315,31 @@ func (g *PullRequest) setPostedComment(ctx context.Context) error {
if c.GetSubjectType() == "line" {
line = c.GetLine()
}
g.postedcs.AddPostedComment(c.GetPath(), line, c.GetBody())
if meta := extractMetaComment(c.GetBody()); meta != nil {
g.postedcs.AddPostedComment(c.GetPath(), line, meta.GetFingerprint())
}
}
return nil
}

func extractMetaComment(body string) *metacomment.MetaComment {
prefix := "<!-- __reviewdog__:"
for _, line := range strings.Split(body, "\n") {
if after, found := strings.CutPrefix(line, prefix); found {
if metastring, foundSuffix := strings.CutSuffix(after, " -->"); foundSuffix {
b, err := base64.StdEncoding.DecodeString(metastring)
if err != nil {
log.Printf("failed to decode MetaComment: %v", err)
continue
}
meta := &metacomment.MetaComment{}
if err := proto.Unmarshal(b, meta); err != nil {
log.Printf("failed to unmarshal MetaComment: %v", err)
continue
}
return meta
}
}
}
return nil
}
Expand Down Expand Up @@ -403,7 +435,7 @@ func listAllPullRequestsComments(ctx context.Context, cli *github.Client,
return append(comments, restComments...), nil
}

func buildBody(c *reviewdog.Comment, baseRelatedLocURL string, gitRootPath string) string {
func buildBody(c *reviewdog.Comment, baseRelatedLocURL string, gitRootPath string, fprint string) string {
cbody := commentutil.MarkdownComment(c)
if suggestion := buildSuggestions(c); suggestion != "" {
cbody += "\n" + suggestion
Expand All @@ -420,9 +452,20 @@ func buildBody(c *reviewdog.Comment, baseRelatedLocURL string, gitRootPath strin
}
cbody += "\n<hr>\n\n" + relatedLoc.GetMessage() + "\n" + relatedURL
}
cbody += fmt.Sprintf("\n<!-- __reviewdog__:%s -->\n", buildMetaComment(fprint, c.ToolName))
return cbody
}

func buildMetaComment(fprint string, toolName string) string {
b, _ := proto.Marshal(
&metacomment.MetaComment{
Fingerprint: fprint,
SourceName: toolName,
},
)
return base64.StdEncoding.EncodeToString(b)
}

func buildSuggestions(c *reviewdog.Comment) string {
var sb strings.Builder
for _, s := range c.Result.Diagnostic.GetSuggestions() {
Expand Down Expand Up @@ -505,3 +548,15 @@ func getSourceLine(sourceLines map[int]string, line int) (string, error) {
}
return lineContent, nil
}

func fingerprint(d *rdf.Diagnostic) (string, error) {
h := fnv.New64a()
data, err := proto.Marshal(d)
if err != nil {
return "", err
}
if _, err := h.Write(data); err != nil {
return "", err
}
return fmt.Sprintf("%x", h.Sum64()), nil
}
44 changes: 38 additions & 6 deletions service/github/github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
{
Path: github.String("reviewdog.go"),
Line: github.Int(2),
Body: github.String(commentutil.BodyPrefix + "already commented"),
Body: github.String(commentutil.BodyPrefix + "already commented" + "\n<!-- __reviewdog__:ChBmMzg0YTRlZDRkYTViOTZl -->\n"),
SubjectType: github.String("line"),
},
}
Expand All @@ -196,21 +196,21 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
{
Path: github.String("reviewdog.go"),
Line: github.Int(15),
Body: github.String(commentutil.BodyPrefix + "already commented 2"),
Body: github.String(commentutil.BodyPrefix + "already commented 2" + "\n<!-- __reviewdog__:ChAxNDgzY2EyNTY0MjU2NmYx -->\n"),
SubjectType: github.String("line"),
},
{
Path: github.String("reviewdog.go"),
StartLine: github.Int(15),
Line: github.Int(16),
Body: github.String(commentutil.BodyPrefix + "multiline existing comment"),
Body: github.String(commentutil.BodyPrefix + "multiline existing comment" + "\n<!-- __reviewdog__:ChBjNGNiNTRjMDc2YjNhMjcx -->\n"),
SubjectType: github.String("line"),
},
{
Path: github.String("reviewdog.go"),
StartLine: github.Int(15),
Line: github.Int(17),
Body: github.String(commentutil.BodyPrefix + "multiline existing comment (line-break)"),
Body: github.String(commentutil.BodyPrefix + "multiline existing comment (line-break)" + "\n<!-- __reviewdog__:ChA2NjI1ZDI2MGJmNTdhNjUw -->\n"),
SubjectType: github.String("line"),
},
{
Expand Down Expand Up @@ -257,15 +257,15 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
Path: github.String("reviewdog.go"),
Side: github.String("RIGHT"),
Line: github.Int(15),
Body: github.String(commentutil.BodyPrefix + "new comment"),
Body: github.String(commentutil.BodyPrefix + "new comment" + "\n<!-- __reviewdog__:ChA4NjA2Mzk3ZGNhMGViOWMw -->\n"),
},
{
Path: github.String("reviewdog.go"),
Side: github.String("RIGHT"),
StartSide: github.String("RIGHT"),
StartLine: github.Int(15),
Line: github.Int(16),
Body: github.String(commentutil.BodyPrefix + "multiline new comment"),
Body: github.String(commentutil.BodyPrefix + "multiline new comment" + "\n<!-- __reviewdog__:ChBjNGFmMTRlYWJkYjBhNTU1 -->\n"),
},
{
Path: github.String("reviewdog.go"),
Expand All @@ -280,6 +280,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line2",
"line3",
"```",
"",
"<!-- __reviewdog__:ChBlZTZhOWZjZjFkMDQ2OTgx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -292,6 +294,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line1",
"line2",
"```",
"",
"<!-- __reviewdog__:ChBlOGViMDE3YTQ3MmIwMDc0 -->",
}, "\n") + "\n"),
},
{
Expand All @@ -303,6 +307,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
Body: github.String(commentutil.BodyPrefix + strings.Join([]string{
"invalid lines suggestion comment",
invalidSuggestionPre + "GitHub comment range and suggestion line range must be same. L15-L16 v.s. L16-L17" + invalidSuggestionPost,
"",
"<!-- __reviewdog__:ChAyNjJlYTIzNWUxYzgzZjBj -->",
}, "\n") + "\n"),
},
{
Expand All @@ -318,6 +324,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line2",
"line3",
"```",
"",
"<!-- __reviewdog__:ChAyMTYwNmIyODI4MTBkMmUx -->",
}, "\n") + "\n"),
},
{
Expand All @@ -334,6 +342,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"line3",
"```",
invalidSuggestionPre + "GitHub comment range and suggestion line range must be same. L14-L16 v.s. L14-L14" + invalidSuggestionPost,
"",
"<!-- __reviewdog__:ChA3ZDIwNzM1MTFhMmRmMTVh -->",
}, "\n") + "\n"),
},
{
Expand All @@ -345,6 +355,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
Body: github.String(commentutil.BodyPrefix + strings.Join([]string{
"non-line based suggestion comment (no source lines)",
invalidSuggestionPre + "source lines are not available" + invalidSuggestionPost,
"",
"<!-- __reviewdog__:ChA3ODliZTFlZTYzMDM3Njhj -->",
}, "\n") + "\n"),
},
{
Expand All @@ -356,6 +368,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA2NzE4OTdhYjExNzZlZjAw -->",
}, "\n") + "\n"),
},
{
Expand All @@ -369,6 +383,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"haya14busa (multi-line)",
"```",
"",
"<!-- __reviewdog__:ChAzNzYyZTE3N2Q3OWNlYjA3 -->",
}, "\n") + "\n"),
},
{
Expand All @@ -382,6 +398,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"line 15 (content at line 15)",
"```",
"",
"<!-- __reviewdog__:ChBmMTFmMjhjNTEzY2ZkNDMw -->",
}, "\n") + "\n"),
},
{
Expand All @@ -393,6 +411,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChBmNDNmODgyMWMwYTYzNWRl -->",
}, "\n") + "\n"),
},
{
Expand All @@ -410,6 +430,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA1ZjAzYTQ4MzU1MWRhZjYw -->",
}, "\n") + "\n"),
},
{
Expand All @@ -421,6 +443,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```suggestion",
"haya14busa",
"```",
"",
"<!-- __reviewdog__:ChA3YWMyOGVkN2MzN2Y4MTY1 -->",
}, "\n") + "\n"),
},
{
Expand All @@ -436,6 +460,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"some code",
"```",
"````",
"",
"<!-- __reviewdog__:ChA5MWQ0YTY4N2RjZDJlOGZj -->",
}, "\n") + "\n"),
},
{
Expand All @@ -449,6 +475,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"some code",
"```",
"````",
"",
"<!-- __reviewdog__:ChAzNTA4YTViNTc1NjViY2Y0 -->",
}, "\n") + "\n"),
},
{
Expand All @@ -463,6 +491,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"```",
"`````",
"``````",
"",
"<!-- __reviewdog__:ChA3ZGVhN2U3Zjk2NDVmOTVk -->",
}, "\n") + "\n"),
},
{
Expand All @@ -481,6 +511,8 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
"",
"related loc test (2)",
"https://test/repo/path/blob/sha/service/github/reviewdog2.go#L14",
"<!-- __reviewdog__:ChBlMDY2NzMwMmE3ZDFlZTBk -->",
"",
}, "\n")),
},
}
Expand Down

0 comments on commit a8144c8

Please sign in to comment.