Skip to content

Commit fab3ac3

Browse files
raghavkaulbalteravishay
raghavkaul
authored and
balteravishay
committed
✨ GitLab: Code Review check (ossf#2764)
* Add GitLab support for Code-Review check Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Remove spurious printf Signed-off-by: Raghav Kaul <raghavkaul@google.com> * Working commit Signed-off-by: Raghav Kaul <raghavkaul@google.com> * update Signed-off-by: Raghav Kaul <raghavkaul@google.com> * update Signed-off-by: Raghav Kaul <raghavkaul@google.com> * e2e test Signed-off-by: Raghav Kaul <raghavkaul@google.com> * update: test coverage Signed-off-by: Raghav Kaul <raghavkaul@google.com> --------- Signed-off-by: Raghav Kaul <raghavkaul@google.com> Signed-off-by: Avishay <avishay.balter@gmail.com>
1 parent 1ff4fac commit fab3ac3

11 files changed

Lines changed: 531 additions & 109 deletions

File tree

‎checks/evaluation/code_review.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,10 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
105105
}
106106
return checker.CreateResultWithScore(
107107
name,
108-
fmt.Sprintf("found %v unreviewed human changesets", nUnreviewedHumanChanges),
108+
fmt.Sprintf(
109+
"found %v unreviewed human changesets (%d total)",
110+
nUnreviewedHumanChanges, len(r.DefaultBranchChangesets),
111+
),
109112
score,
110113
)
111114
}

‎clients/gitlabrepo/client.go‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ type Client struct {
5252
languages *languagesHandler
5353
licenses *licensesHandler
5454
tarball *tarballHandler
55+
graphql *graphqlHandler
5556
ctx context.Context
5657
commitDepth int
5758
}
@@ -78,7 +79,9 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD
7879
client.repourl = &repoURL{
7980
scheme: glRepo.scheme,
8081
host: glRepo.host,
81-
project: fmt.Sprint(repo.ID),
82+
owner: glRepo.owner,
83+
project: glRepo.project,
84+
projectID: fmt.Sprint(repo.ID),
8285
defaultBranch: repo.DefaultBranch,
8386
commitSHA: commitSHA,
8487
}
@@ -132,6 +135,9 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD
132135
// Init tarballHandler
133136
client.tarball.init(client.ctx, client.repourl, repo, commitSHA)
134137

138+
// Init graphqlHandler
139+
client.graphql.init(client.ctx, client.repourl)
140+
135141
return nil
136142
}
137143

@@ -152,7 +158,22 @@ func (client *Client) GetFileContent(filename string) ([]byte, error) {
152158
}
153159

154160
func (client *Client) ListCommits() ([]clients.Commit, error) {
155-
return client.commits.listCommits()
161+
// Get commits from REST API
162+
commitsRaw, err := client.commits.listRawCommits()
163+
if err != nil {
164+
return []clients.Commit{}, err
165+
}
166+
167+
// Get merge request details from GraphQL
168+
// GitLab REST API doesn't provide a way to link Merge Requests and Commits that
169+
// are within them without making a REST call for each commit (~30 by default)
170+
// Making 1 GraphQL query to combine the results of 2 REST calls, we avoid this
171+
mrDetails, err := client.graphql.getMergeRequestsDetail()
172+
if err != nil {
173+
return []clients.Commit{}, err
174+
}
175+
176+
return client.commits.zip(commitsRaw, mrDetails), nil
156177
}
157178

158179
func (client *Client) ListIssues() ([]clients.Issue, error) {
@@ -278,6 +299,7 @@ func CreateGitlabClientWithToken(ctx context.Context, token string, repo clients
278299
},
279300
licenses: &licensesHandler{},
280301
tarball: &tarballHandler{},
302+
graphql: &graphqlHandler{},
281303
}, nil
282304
}
283305

‎clients/gitlabrepo/client_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Copyright 2023 OpenSSF Scorecard Authors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package gitlabrepo
16+
17+
import (
18+
"context"
19+
"testing"
20+
)
21+
22+
func Test_InitRepo(t *testing.T) {
23+
t.Parallel()
24+
tcs := []struct {
25+
name string
26+
repourl string
27+
commit string
28+
depth int
29+
}{
30+
{
31+
repourl: "https://gitlab.com/fdroid/fdroidclient",
32+
commit: "a4bbef5c70fd2ac7c15437a22ef0f9ef0b447d08",
33+
depth: 20,
34+
},
35+
}
36+
37+
for _, tt := range tcs {
38+
t.Run(tt.name, func(t *testing.T) {
39+
repo, err := MakeGitlabRepo(tt.repourl)
40+
if err != nil {
41+
t.Error("couldn't make gitlab repo", err)
42+
}
43+
44+
client, err := CreateGitlabClientWithToken(context.Background(), "", repo)
45+
if err != nil {
46+
t.Error("couldn't make gitlab client", err)
47+
}
48+
49+
err = client.InitRepo(repo, tt.commit, tt.depth)
50+
if err != nil {
51+
t.Error("couldn't init gitlab repo", err)
52+
}
53+
})
54+
}
55+
}

‎clients/gitlabrepo/commits.go‎

Lines changed: 98 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package gitlabrepo
1616

1717
import (
1818
"fmt"
19+
"strconv"
1920
"strings"
2021
"sync"
2122

@@ -25,11 +26,11 @@ import (
2526
)
2627

2728
type commitsHandler struct {
28-
glClient *gitlab.Client
29-
once *sync.Once
30-
errSetup error
31-
repourl *repoURL
32-
commits []clients.Commit
29+
glClient *gitlab.Client
30+
once *sync.Once
31+
errSetup error
32+
repourl *repoURL
33+
commitsRaw []*gitlab.Commit
3334
}
3435

3536
func (handler *commitsHandler) init(repourl *repoURL) {
@@ -38,123 +39,118 @@ func (handler *commitsHandler) init(repourl *repoURL) {
3839
handler.once = new(sync.Once)
3940
}
4041

41-
// nolint: gocognit
4242
func (handler *commitsHandler) setup() error {
4343
handler.once.Do(func() {
44-
commits, _, err := handler.glClient.Commits.ListCommits(handler.repourl.project, &gitlab.ListCommitsOptions{})
44+
commits, _, err := handler.glClient.Commits.ListCommits(handler.repourl.projectID, &gitlab.ListCommitsOptions{})
4545
if err != nil {
4646
handler.errSetup = fmt.Errorf("request for commits failed with %w", err)
4747
return
4848
}
49+
handler.commitsRaw = commits
50+
})
4951

50-
// To limit the number of user requests we are going to map every committer email
51-
// to a user.
52-
userToEmail := make(map[string]*gitlab.User)
53-
for _, commit := range commits {
54-
user, ok := userToEmail[commit.AuthorEmail]
55-
if !ok {
56-
users, _, err := handler.glClient.Search.Users(commit.CommitterName, &gitlab.SearchOptions{})
57-
if err != nil {
58-
// Possibility this shouldn't be an issue as individuals can leave organizations
59-
// (possibly taking their account with them)
60-
handler.errSetup = fmt.Errorf("unable to find user associated with commit: %w", err)
61-
return
62-
}
63-
64-
// For some reason some users have unknown names, so below we are going to parse their email into pieces.
65-
// i.e. (firstname.lastname@domain.com) -> "firstname lastname".
66-
if len(users) == 0 {
67-
users, _, err = handler.glClient.Search.Users(parseEmailToName(commit.CommitterEmail), &gitlab.SearchOptions{})
68-
if err != nil {
69-
handler.errSetup = fmt.Errorf("unable to find user associated with commit: %w", err)
70-
return
71-
}
72-
}
73-
userToEmail[commit.AuthorEmail] = users[0]
74-
user = users[0]
75-
}
52+
return handler.errSetup
53+
}
7654

77-
// Commits are able to be a part of multiple merge requests, but the only one that will be important
78-
// here is the earliest one.
79-
mergeRequests, _, err := handler.glClient.Commits.ListMergeRequestsByCommit(handler.repourl.project, commit.ID)
80-
if err != nil {
81-
handler.errSetup = fmt.Errorf("unable to find merge requests associated with commit: %w", err)
82-
return
83-
}
84-
var mergeRequest *gitlab.MergeRequest
85-
if len(mergeRequests) > 0 {
86-
mergeRequest = mergeRequests[0]
87-
for i := range mergeRequests {
88-
if mergeRequests[i] == nil || mergeRequests[i].MergedAt == nil {
89-
continue
90-
}
91-
if mergeRequests[i].CreatedAt.Before(*mergeRequest.CreatedAt) {
92-
mergeRequest = mergeRequests[i]
93-
}
94-
}
95-
} else {
96-
handler.commits = append(handler.commits, clients.Commit{
97-
CommittedDate: *commit.CommittedDate,
98-
Message: commit.Message,
99-
SHA: commit.ID,
100-
})
101-
continue
102-
}
55+
func (handler *commitsHandler) listRawCommits() ([]*gitlab.Commit, error) {
56+
if err := handler.setup(); err != nil {
57+
return nil, fmt.Errorf("error during commitsHandler.setup: %w", err)
58+
}
59+
60+
return handler.commitsRaw, nil
61+
}
10362

104-
if mergeRequest == nil || mergeRequest.MergedAt == nil {
105-
handler.commits = append(handler.commits, clients.Commit{
106-
CommittedDate: *commit.CommittedDate,
107-
Message: commit.Message,
108-
SHA: commit.ID,
109-
})
63+
// zip combines Commit and MergeRequest information from the GitLab REST API with
64+
// information from the GitLab GraphQL API. The REST API doesn't provide any way to
65+
// get from Commits -> MRs that they were part of or vice-versa (MRs -> commits they
66+
// contain), except through a separate API call. Instead of calling the REST API
67+
// len(commits) times to get the associated MR, we make 3 calls (2 REST, 1 GraphQL).
68+
func (handler *commitsHandler) zip(commitsRaw []*gitlab.Commit, data graphqlData) []clients.Commit {
69+
commitToMRIID := make(map[string]string) // which mr does a commit belong to?
70+
for i := range data.Project.MergeRequests.Nodes {
71+
mr := data.Project.MergeRequests.Nodes[i]
72+
for _, commit := range mr.Commits.Nodes {
73+
commitToMRIID[commit.SHA] = mr.IID
74+
}
75+
commitToMRIID[mr.MergeCommitSHA] = mr.IID
76+
}
77+
78+
iidToMr := make(map[string]clients.PullRequest)
79+
for i := range data.Project.MergeRequests.Nodes {
80+
mr := data.Project.MergeRequests.Nodes[i]
81+
// Two GitLab APIs for reviews (reviews vs. approvals)
82+
// Use a map to consolidate results from both APIs by the user ID who performed review
83+
reviews := make(map[string]clients.Review)
84+
for _, reviewer := range mr.Reviewers.Nodes {
85+
reviews[reviewer.Username] = clients.Review{
86+
Author: &clients.User{Login: reviewer.Username},
87+
State: "COMMENTED",
11088
}
89+
}
11190

112-
// Casting the Reviewers into clients.Review.
113-
var reviews []clients.Review
114-
for _, reviewer := range mergeRequest.Reviewers {
115-
reviews = append(reviews, clients.Review{
116-
Author: &clients.User{ID: int64(reviewer.ID)},
117-
State: "",
118-
})
91+
if fmt.Sprintf("%v", mr.IID) != mr.IID {
92+
continue
93+
}
94+
95+
// Check approvers
96+
for _, approver := range mr.Approvers.Nodes {
97+
reviews[approver.Username] = clients.Review{
98+
Author: &clients.User{Login: approver.Username},
99+
State: "APPROVED",
119100
}
101+
break
102+
}
120103

121-
// Casting the Labels into []clients.Label.
122-
var labels []clients.Label
123-
for _, label := range mergeRequest.Labels {
124-
labels = append(labels, clients.Label{
125-
Name: label,
126-
})
104+
// Check reviewers (sometimes unofficial approvals end up here)
105+
for _, reviewer := range mr.Reviewers.Nodes {
106+
if reviewer.MergeRequestInteraction.ReviewState != "REVIEWED" {
107+
continue
127108
}
109+
reviews[reviewer.Username] = clients.Review{
110+
Author: &clients.User{Login: reviewer.Username},
111+
State: "APPROVED",
112+
}
113+
break
114+
}
128115

129-
// append the commits to the handler.
130-
handler.commits = append(handler.commits,
131-
clients.Commit{
132-
CommittedDate: *commit.CommittedDate,
133-
Message: commit.Message,
134-
SHA: commit.ID,
135-
AssociatedMergeRequest: clients.PullRequest{
136-
Number: mergeRequest.ID,
137-
MergedAt: *mergeRequest.MergedAt,
138-
HeadSHA: mergeRequest.SHA,
139-
Author: clients.User{ID: int64(mergeRequest.Author.ID)},
140-
Labels: labels,
141-
Reviews: reviews,
142-
MergedBy: clients.User{ID: int64(mergeRequest.MergedBy.ID)},
143-
},
144-
Committer: clients.User{ID: int64(user.ID)},
145-
})
116+
vals := []clients.Review{}
117+
for _, v := range reviews {
118+
vals = append(vals, v)
146119
}
147-
})
148120

149-
return handler.errSetup
150-
}
121+
var mrno int
122+
mrno, err := strconv.Atoi(mr.IID)
123+
if err != nil {
124+
mrno = mr.ID.ID
125+
}
151126

152-
func (handler *commitsHandler) listCommits() ([]clients.Commit, error) {
153-
if err := handler.setup(); err != nil {
154-
return nil, fmt.Errorf("error during commitsHandler.setup: %w", err)
127+
iidToMr[mr.IID] = clients.PullRequest{
128+
Number: mrno,
129+
MergedAt: mr.MergedAt,
130+
HeadSHA: mr.MergeCommitSHA,
131+
Author: clients.User{Login: mr.Author.Username, ID: int64(mr.Author.ID.ID)},
132+
Reviews: vals,
133+
MergedBy: clients.User{Login: mr.MergedBy.Username, ID: int64(mr.MergedBy.ID.ID)},
134+
}
135+
}
136+
137+
// Associate Merge Requests with Commits based on the GitLab Merge Request IID
138+
commits := []clients.Commit{}
139+
for _, cRaw := range commitsRaw {
140+
// Get IID of Merge Request that this commit was merged as part of
141+
mrIID := commitToMRIID[cRaw.ID]
142+
associatedMr := iidToMr[mrIID]
143+
144+
commits = append(commits,
145+
clients.Commit{
146+
CommittedDate: *cRaw.CommittedDate,
147+
Message: cRaw.Message,
148+
SHA: cRaw.ID,
149+
AssociatedMergeRequest: associatedMr,
150+
})
155151
}
156152

157-
return handler.commits, nil
153+
return commits
158154
}
159155

160156
// Expected email form: <firstname>.<lastname>@<namespace>.com.

0 commit comments

Comments
 (0)