Skip to content

Commit 2d6e98d

Browse files
lukaszgryglickilukaszgryglicki
authored
Merge pull request #4827 from linuxfoundation/unicron-implemente-rest-fallback-when-graphql-fails
Unicron implemente rest fallback when graphql fails
2 parents c05daf1 + ef8afdf commit 2d6e98d

File tree

2 files changed

+128
-12
lines changed

2 files changed

+128
-12
lines changed

‎cla-backend-go/github/github_repository.go‎

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1220,6 +1220,90 @@ func GetCommitAuthorsSignedStatuses(
12201220
return signed, unsigned
12211221
}
12221222

1223+
//nolint:gocyclo // complexity is acceptable for now
1224+
func GetPullRequestCommitAuthorsREST(ctx context.Context, usersService users.Service, installationID int64, pullRequestID int, owner, repo string, withCoAuthors bool) ([]*UserCommitSummary, bool, error) {
1225+
f := logrus.Fields{
1226+
"functionName": "github.github_repository.GetPullRequestCommitAuthorsREST",
1227+
"pullRequestID": pullRequestID,
1228+
"withCoAuthors": withCoAuthors,
1229+
}
1230+
var userCommitSummary []*UserCommitSummary
1231+
var mu sync.Mutex
1232+
1233+
client, err := NewGithubAppClient(installationID)
1234+
if err != nil {
1235+
log.WithFields(f).WithError(err).Warn("unable to create Github client")
1236+
return nil, false, err
1237+
}
1238+
1239+
anyMissing := false
1240+
opts := &github.ListOptions{PerPage: 100}
1241+
for {
1242+
commits, resp, comErr := client.PullRequests.ListCommits(ctx, owner, repo, pullRequestID, opts)
1243+
if comErr != nil {
1244+
log.WithFields(f).WithError(comErr).Warnf("problem listing commits for repo: %s/%s pull request: %d", owner, repo, pullRequestID)
1245+
return nil, false, comErr
1246+
}
1247+
if resp.StatusCode != http.StatusOK {
1248+
msg := fmt.Sprintf("unexpected status code: %d - expected: %d", resp.StatusCode, http.StatusOK)
1249+
log.WithFields(f).Warn(msg)
1250+
return nil, false, errors.New(msg)
1251+
}
1252+
1253+
log.WithFields(f).Debugf("found %d commits for pull request: %d", len(commits), pullRequestID)
1254+
for _, commit := range commits {
1255+
log.WithFields(f).Debugf("loaded commit: %+v", commit)
1256+
commitAuthor := ""
1257+
if commit != nil && commit.Commit != nil && commit.Commit.Author != nil && commit.Commit.Author.Login != nil {
1258+
log.WithFields(f).Debugf("commit.Commit.Author.Login: %s", utils.StringValue(commit.Commit.Author.Login))
1259+
commitAuthor = utils.StringValue(commit.Commit.Author.Login)
1260+
} else if commit != nil && commit.Author != nil && commit.Author.Login != nil {
1261+
log.WithFields(f).Debugf("commit.Author.Login: %s", utils.StringValue(commit.Author.Login))
1262+
commitAuthor = utils.StringValue(commit.Author.Login)
1263+
}
1264+
name, email := "", ""
1265+
if commit != nil && commit.Commit != nil && commit.Commit.Author != nil {
1266+
name = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Name))
1267+
email = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Email))
1268+
if (name != "" || email != "") && commit.Author == nil {
1269+
commit.Author = &github.User{}
1270+
}
1271+
if name != "" && commit.Author != nil {
1272+
if commit.Author.Name == nil || strings.TrimSpace(utils.StringValue(commit.Author.Name)) == "" {
1273+
n := name
1274+
commit.Author.Name = &n
1275+
}
1276+
}
1277+
if email != "" && commit.Author != nil {
1278+
if commit.Author.Email == nil || strings.TrimSpace(utils.StringValue(commit.Author.Email)) == "" {
1279+
e := email
1280+
commit.Author.Email = &e
1281+
}
1282+
}
1283+
}
1284+
log.WithFields(f).Debugf("commitAuthor: %s, name: %s, email: %s", commitAuthor, name, email)
1285+
userCommitSummary = append(userCommitSummary, &UserCommitSummary{
1286+
SHA: utils.StringValue(commit.SHA),
1287+
CommitAuthor: commit.Author,
1288+
Affiliated: false,
1289+
Authorized: false,
1290+
})
1291+
if withCoAuthors {
1292+
missing := ExpandWithCoAuthors(ctx, client, usersService, commit, pullRequestID, installationID, &userCommitSummary, &mu)
1293+
if !anyMissing && missing {
1294+
anyMissing = true
1295+
}
1296+
}
1297+
}
1298+
if resp.NextPage == 0 {
1299+
break
1300+
}
1301+
opts.Page = resp.NextPage
1302+
}
1303+
1304+
return userCommitSummary, anyMissing, nil
1305+
}
1306+
12231307
func GetPullRequestCommitAuthors(
12241308
ctx context.Context,
12251309
usersService users.Service,
@@ -1302,7 +1386,7 @@ query($owner:String!, $name:String!, $number:Int!, $pageSize:Int!, $cursor:Strin
13021386
var page prCommitsPage
13031387
if _, err := doGraphQL(ctx, client, query, vars, &page); err != nil {
13041388
log.WithFields(f).WithError(err).Warnf("problem listing commits via GraphQL for %s/%s PR #%d", owner, repo, pullRequestID)
1305-
return nil, false, err
1389+
return GetPullRequestCommitAuthorsREST(ctx, usersService, installationID, pullRequestID, owner, repo, withCoAuthors)
13061390
}
13071391

13081392
c := page.Repository.PullRequest.Commits

‎cla-backend/cla/models/github_models.py‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2210,7 +2210,7 @@ def iter_pr_commits_full(g, owner: str, repo_name: str, number: int, page_size:
22102210
)
22112211
if res is None:
22122212
cla.log.error(f"Failed to fetch commits for {owner}/{repo_name} PR #{number}")
2213-
return
2213+
raise ValueError("failed to fetch commits using GraphQL")
22142214
commits = res["repository"]["pullRequest"]["commits"]
22152215
for n in commits["nodes"]:
22162216
c = n["commit"]
@@ -2341,29 +2341,61 @@ def get_pull_request_commit_authors(client, org, pull_request, installation_id,
23412341

23422342
pr_number = pull_request.number
23432343
repo_name = pull_request.base.repo.name
2344+
gql_ok = True
2345+
pr_commits = None
2346+
23442347
count = get_pr_commit_count_gql(client, org, repo_name, pr_number)
23452348
if count is not None:
2346-
cla.log.debug(f"{fn} - PR: {pr_number}, number of commits: {count}")
2349+
cla.log.debug(f"{fn} - PR: {pr_number}, number of commits (GraphQL): {count}")
23472350
else:
2348-
cla.log.debug(f"{fn} - PR: {pr_number}, number of commits: (unknown)")
2351+
cla.log.debug(f"{fn} - PR: {pr_number}, failed to get commits count using GraphQL, fallback to REST")
2352+
gql_ok = False
2353+
try:
2354+
pr_commits = pull_request.get_commits()
2355+
count = pr_commits.totalCount
2356+
except Exception as exc:
2357+
cla.log.warning(f"{fn} - PR: {pr_number}, get PR commits raised: {exc}")
2358+
raise
2359+
cla.log.debug(f"{fn} - PR: {pr_number}, number of commits (REST API): {count}")
2360+
if count == 250:
2361+
cla.log.warning(f"{fn} - PR: {pr_number}, commit count is 250, which is the max for REST API, there can be more commits")
23492362

23502363
commit_authors = []
23512364
any_missing = False
23522365
max_workers = 16
23532366
submit_chunk = 256
23542367

2355-
try:
2356-
with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor:
2357-
for chunk in iter_chunks(iter_pr_commits_full(client, org, repo_name, pr_number), submit_chunk):
2358-
futures = [executor.submit(get_author_summary_gql, c, pr_number, installation_id, with_co_authors)
2359-
for c in chunk]
2368+
if gql_ok:
2369+
try:
2370+
with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor:
2371+
for chunk in iter_chunks(iter_pr_commits_full(client, org, repo_name, pr_number), submit_chunk):
2372+
futures = [executor.submit(get_author_summary_gql, c, pr_number, installation_id, with_co_authors) for c in chunk]
2373+
for fut in concurrent.futures.as_completed(futures):
2374+
authors, missing = fut.result()
2375+
any_missing = any_missing or missing
2376+
commit_authors.extend(authors)
2377+
except Exception as exc:
2378+
cla.log.warning(f"{fn} - PR: {pr_number}, GraphQL processing failed: {exc}, falling back to REST")
2379+
gql_ok = False
2380+
commit_authors = []
2381+
any_missing = False
2382+
if not gql_ok:
2383+
if pr_commits is None:
2384+
try:
2385+
pr_commits = pull_request.get_commits()
2386+
except Exception as exc:
2387+
cla.log.warning(f"{fn} - PR: {pr_number}, fallback get PR commits raised: {exc}")
2388+
raise
2389+
try:
2390+
with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor:
2391+
futures = [executor.submit(get_author_summary, c, pr_number, installation_id, with_co_authors) for c in pr_commits]
23602392
for fut in concurrent.futures.as_completed(futures):
23612393
authors, missing = fut.result()
23622394
any_missing = any_missing or missing
23632395
commit_authors.extend(authors)
2364-
except Exception as exc:
2365-
cla.log.warning(f"{fn} - PR: {pr_number}, get_author_summary_gql raised: {exc}")
2366-
raise
2396+
except Exception as exc:
2397+
cla.log.warning(f"{fn} - PR: {pr_number}, REST processing failed: {exc}")
2398+
raise
23672399

23682400
return (commit_authors, any_missing)
23692401

0 commit comments

Comments
 (0)