diff --git a/pull/github.go b/pull/github.go index dea66af3..737d60f7 100644 --- a/pull/github.go +++ b/pull/github.go @@ -22,7 +22,6 @@ import ( "github.com/google/go-github/github" "github.com/pkg/errors" - "github.com/rs/zerolog" "github.com/shurcooL/githubv4" ) @@ -36,6 +35,11 @@ const ( MaxPullRequestCommits = 250 ) +var ( + commitLoadMaxAttempts = 3 + commitLoadBaseDelay = time.Second +) + // Locator identifies a pull request and optionally contains a full or partial // pull request object. type Locator struct { @@ -221,12 +225,16 @@ func (ghc *GitHubContext) ChangedFiles() ([]*File, error) { func (ghc *GitHubContext) Commits() ([]*Commit, error) { if ghc.commits == nil { - if err := ghc.loadPagedData(); err != nil { + commits, err := ghc.loadCommits() + if err != nil { return nil, err } - } - if len(ghc.commits) >= MaxPullRequestCommits { - return nil, errors.Errorf("too many commits in pull request, maximum is %d", MaxPullRequestCommits) + if len(commits) >= MaxPullRequestCommits { + return nil, errors.Errorf("too many commits in pull request, maximum is %d", MaxPullRequestCommits) + } + + backfillPushedAt(commits, ghc.pr.HeadRefOID) + ghc.commits = commits } return ghc.commits, nil } @@ -250,15 +258,10 @@ func (ghc *GitHubContext) Reviews() ([]*Review, error) { } func (ghc *GitHubContext) loadPagedData() error { - // this is a minor optimization: make max(c,m,r) requests instead of c+m+r + // this is a minor optimization: make max(c,r) requests instead of c+r var q struct { Repository struct { PullRequest struct { - Commits struct { - PageInfo v4PageInfo - Nodes []v4PullRequestCommit - } `graphql:"commits(first: 100, after: $commitCursor)"` - Comments struct { PageInfo v4PageInfo Nodes []v4IssueComment @@ -276,12 +279,10 @@ func (ghc *GitHubContext) loadPagedData() error { "name": githubv4.String(ghc.repo), "number": githubv4.Int(ghc.number), - "commitCursor": (*githubv4.String)(nil), "commentCursor": (*githubv4.String)(nil), "reviewCursor": (*githubv4.String)(nil), } - commits := []*Commit{} comments := []*Comment{} reviews := []*Review{} for { @@ -290,13 +291,6 @@ func (ghc *GitHubContext) loadPagedData() error { return errors.Wrap(err, "failed to load pull request data") } - for _, c := range q.Repository.PullRequest.Commits.Nodes { - commits = append(commits, c.Commit.ToCommit()) - } - if !q.Repository.PullRequest.Commits.PageInfo.UpdateCursor(qvars, "commitCursor") { - complete++ - } - for _, c := range q.Repository.PullRequest.Comments.Nodes { comments = append(comments, c.ToComment()) } @@ -311,41 +305,100 @@ func (ghc *GitHubContext) loadPagedData() error { complete++ } - if complete == 3 { + if complete == 2 { break } } - if ghc.pr.IsCrossRepository { - // if the commits are from a fork, the pushed date is not set in the - // query result above; this is a GitHub limitation as of 2019-04-25, - // but their support team has suggested it will be fixed in the future - if err := ghc.loadPushedAt(commits); err != nil { - return err + ghc.comments = comments + ghc.reviews = reviews + return nil +} + +func (ghc *GitHubContext) loadCommits() ([]*Commit, error) { + // github does not always return the latest commit information for a PR + // immediately after it was updated; if we're missing data, try again + attempts := 0 + for { + rawCommits, err := ghc.loadRawCommits() + if err != nil { + return nil, err } + + var head *Commit + commits := make([]*Commit, 0, len(rawCommits)) + + for _, r := range rawCommits { + c := r.Commit.ToCommit() + if c.SHA == ghc.pr.HeadRefOID { + head = c + } + commits = append(commits, c) + } + + if head != nil { + // as of 2019-05-01, the GitHub API does return pushed date for + // commits from forks, so we must load that separately + if ghc.pr.IsCrossRepository && head.PushedAt == nil { + if err := ghc.loadPushedAt(commits); err != nil { + return nil, err + } + } + if head.PushedAt != nil { + return commits, nil + } + } + + attempts++ + if attempts >= commitLoadMaxAttempts { + msg := "missing" + if head != nil { + msg += " pushed date" + } + return nil, errors.Errorf("head commit %.10s is %s; this is probably a bug", ghc.pr.HeadRefOID, msg) + } + + time.Sleep(time.Duration(attempts) * commitLoadBaseDelay) + } +} + +func (ghc *GitHubContext) loadRawCommits() ([]*v4PullRequestCommit, error) { + var q struct { + Repository struct { + PullRequest struct { + Commits struct { + PageInfo v4PageInfo + Nodes []*v4PullRequestCommit + } `graphql:"commits(first: 100, after: $cursor)"` + } `graphql:"pullRequest(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` } - if err := backfillPushedAt(commits, ghc.pr.HeadRefOID); err != nil { - return err + qvars := map[string]interface{}{ + "owner": githubv4.String(ghc.owner), + "name": githubv4.String(ghc.repo), + "number": githubv4.Int(ghc.number), + "cursor": (*githubv4.String)(nil), } - ghc.commits = commits - ghc.comments = comments - ghc.reviews = reviews - return nil + commits := []*v4PullRequestCommit{} + for { + if err := ghc.v4client.Query(ghc.ctx, &q, qvars); err != nil { + return nil, errors.Wrap(err, "failed to load commits") + } + commits = append(commits, q.Repository.PullRequest.Commits.Nodes...) + if !q.Repository.PullRequest.Commits.PageInfo.UpdateCursor(qvars, "cursor") { + break + } + } + return commits, nil } func (ghc *GitHubContext) loadPushedAt(commits []*Commit) error { - commitsBySHA := make(map[string]*Commit) + commitsBySHA := make(map[string]*Commit, len(commits)) for _, c := range commits { commitsBySHA[c.SHA] = c } - if c, ok := commitsBySHA[ghc.pr.HeadRefOID]; ok && c.PushedAt != nil { - // head commit has a pushed date, so assume we don't need to load more - zerolog.Ctx(ghc.ctx).Debug().Msg("Skipping pushed date loading, a date was already present") - return nil - } - var q struct { Repository struct { Object struct { @@ -372,14 +425,12 @@ func (ghc *GitHubContext) loadPushedAt(commits []*Commit) error { if err := ghc.v4client.Query(ghc.ctx, &q, qvars); err != nil { return errors.Wrap(err, "failed to load commit pushed dates") } - for _, n := range q.Repository.Object.Commit.History.Nodes { if c, ok := commitsBySHA[n.OID]; ok { c.PushedAt = n.PushedDate delete(commitsBySHA, n.OID) } } - if !q.Repository.Object.Commit.History.PageInfo.UpdateCursor(qvars, "cursor") { break } @@ -391,24 +442,12 @@ func (ghc *GitHubContext) loadPushedAt(commits []*Commit) error { return nil } -func backfillPushedAt(commits []*Commit, headSHA string) error { - if len(commits) == 0 { - return nil - } - - commitsBySHA := make(map[string]*Commit) +func backfillPushedAt(commits []*Commit, headSHA string) { + commitsBySHA := make(map[string]*Commit, len(commits)) for _, c := range commits { commitsBySHA[c.SHA] = c } - if head, ok := commitsBySHA[headSHA]; !ok || head.PushedAt == nil { - msg := "missing" - if ok { - msg += " a pushed date" - } - return errors.Errorf("head commit %s is %s; this is probably a bug", headSHA, msg) - } - root := headSHA for { c, ok := commitsBySHA[root] @@ -424,9 +463,10 @@ func backfillPushedAt(commits []*Commit, headSHA string) error { if firstParent.PushedAt == nil { firstParent.PushedAt = c.PushedAt } + + delete(commitsBySHA, root) root = firstParent.SHA } - return nil } // if adding new fields to this struct, modify Locator#toV4() as well diff --git a/pull/github_test.go b/pull/github_test.go index edd35d5f..287d097f 100644 --- a/pull/github_test.go +++ b/pull/github_test.go @@ -27,6 +27,12 @@ import ( "github.com/stretchr/testify/require" ) +func init() { + // adjust attempts/delays for faster tests + commitLoadMaxAttempts = 2 + commitLoadBaseDelay = time.Millisecond +} + func TestChangedFiles(t *testing.T) { rp := &ResponsePlayer{} filesRule := rp.AddRule( @@ -66,16 +72,13 @@ func TestCommits(t *testing.T) { "testdata/responses/pull_commits.yml", ) - pr := defaultTestPR() - pr.Commits = github.Int(3) - - ctx := makeContext(t, rp, pr) + ctx := makeContext(t, rp, nil) commits, err := ctx.Commits() require.NoError(t, err) require.Len(t, commits, 3, "incorrect number of commits") - assert.Equal(t, 2, dataRule.Count, "no http request was made") + assert.Equal(t, 2, dataRule.Count, "incorrect number of http requests") expectedTime, err := time.Parse(time.RFC3339, "2018-12-04T12:34:56Z") assert.NoError(t, err) @@ -103,6 +106,40 @@ func TestCommits(t *testing.T) { assert.Equal(t, 2, dataRule.Count, "cached commits were not used") } +func TestCommitsRetry(t *testing.T) { + rp := &ResponsePlayer{} + dataRule := rp.AddRule( + GraphQLNodePrefixMatcher("repository.pullRequest.commits"), + "testdata/responses/pull_commits_retry.yml", + ) + + ctx := makeContext(t, rp, nil) + + commits, err := ctx.Commits() + require.NoError(t, err) + + require.Len(t, commits, 3, "incorrect number of commits") + assert.Equal(t, 2, dataRule.Count, "incorrect number of http requests") + + expectedTime, err := time.Parse(time.RFC3339, "2018-12-04T12:34:56Z") + assert.NoError(t, err) + + assert.Equal(t, "a6f3f69b64eaafece5a0d854eb4af11c0d64394c", commits[0].SHA) + assert.Equal(t, "mhaypenny", commits[0].Author) + assert.Equal(t, "mhaypenny", commits[0].Committer) + assert.Equal(t, newTime(expectedTime), commits[0].PushedAt) + + assert.Equal(t, "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9", commits[1].SHA) + assert.Equal(t, "mhaypenny", commits[1].Author) + assert.Equal(t, "mhaypenny", commits[1].Committer) + assert.Equal(t, newTime(expectedTime), commits[1].PushedAt) + + assert.Equal(t, "e05fcae367230ee709313dd2720da527d178ce43", commits[2].SHA) + assert.Equal(t, "ttest", commits[2].Author) + assert.Equal(t, "mhaypenny", commits[2].Committer) + assert.Equal(t, newTime(expectedTime.Add(48*time.Hour)), commits[2].PushedAt) +} + func TestReviews(t *testing.T) { rp := &ResponsePlayer{} dataRule := rp.AddRule( diff --git a/pull/testdata/responses/pull_commits_retry.yml b/pull/testdata/responses/pull_commits_retry.yml new file mode 100644 index 00000000..e49657a4 --- /dev/null +++ b/pull/testdata/responses/pull_commits_retry.yml @@ -0,0 +1,176 @@ +- status: 200 + body: | + { + "errors": [], + "data": { + "repository": { + "pullRequest": { + "commits": { + "pageInfo": { + "endCursor": "2", + "hasNextPage": false + }, + "nodes": [ + { + "commit": { + "oid": "a6f3f69b64eaafece5a0d854eb4af11c0d64394c", + "pushedDate": null, + "author": { + "user": { + "login": "mhaypenny" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "0492883704e64bb53834c7c5fbd5fc22d44dedda" + } + ] + } + } + }, + { + "commit": { + "oid": "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9", + "pushedDate": "2018-12-04T12:34:56Z", + "author": { + "user": { + "login": "mhaypenny" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "a6f3f69b64eaafece5a0d854eb4af11c0d64394c" + } + ] + } + } + }, + { + "commit": { + "oid": "e05fcae367230ee709313dd2720da527d178ce43", + "pushedDate": null, + "author": { + "user": { + "login": "ttest" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9" + } + ] + } + } + } + ] + } + } + } + } + } +- status: 200 + body: | + { + "errors": [], + "data": { + "repository": { + "pullRequest": { + "commits": { + "pageInfo": { + "endCursor": "2", + "hasNextPage": false + }, + "nodes": [ + { + "commit": { + "oid": "a6f3f69b64eaafece5a0d854eb4af11c0d64394c", + "pushedDate": null, + "author": { + "user": { + "login": "mhaypenny" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "0492883704e64bb53834c7c5fbd5fc22d44dedda" + } + ] + } + } + }, + { + "commit": { + "oid": "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9", + "pushedDate": "2018-12-04T12:34:56Z", + "author": { + "user": { + "login": "mhaypenny" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "a6f3f69b64eaafece5a0d854eb4af11c0d64394c" + } + ] + } + } + }, + { + "commit": { + "oid": "e05fcae367230ee709313dd2720da527d178ce43", + "pushedDate": "2018-12-06T12:34:56Z", + "author": { + "user": { + "login": "ttest" + } + }, + "committer": { + "user": { + "login": "mhaypenny" + } + }, + "parents": { + "nodes": [ + { + "oid": "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9" + } + ] + } + } + } + ] + } + } + } + } + }