diff --git a/backend/plugins/github_graphql/tasks/account_collector.go b/backend/plugins/github_graphql/tasks/account_collector.go index a75013d0d1d..37ecb9716cc 100644 --- a/backend/plugins/github_graphql/tasks/account_collector.go +++ b/backend/plugins/github_graphql/tasks/account_collector.go @@ -75,10 +75,17 @@ func CollectAccount(taskCtx plugin.SubTaskContext) errors.Error { db := taskCtx.GetDal() data := taskCtx.GetData().(*githubTasks.GithubTaskData) + // `user(login:)` resolves Users only, a bot login always comes back as + // "Could not resolve to a User", so bots are left out of this lookup. They are + // still converted to accounts: ConvertAccounts reads _tool_github_repo_accounts + // and only enriches it from _tool_github_accounts when a row is there. cursor, err := db.Cursor( dal.Select("login"), dal.From(models.GithubRepoAccount{}.TableName()), - dal.Where("repo_github_id = ? and connection_id=?", data.Options.GithubId, data.Options.ConnectionId), + dal.Where( + "repo_github_id = ? and connection_id = ? and login not like ?", + data.Options.GithubId, data.Options.ConnectionId, "%"+botLoginSuffix, + ), ) if err != nil { return err diff --git a/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go index e4b47e4756d..54bc44a707e 100644 --- a/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go +++ b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go @@ -18,9 +18,16 @@ limitations under the License. package tasks import ( + "strings" + "github.com/apache/incubator-devlake/plugins/github/models" ) +// botLoginSuffix is how the REST API reports a bot login: `dependabot[bot]`. +// GraphQL's `Bot.login` omits it, so it is appended back to give a bot the same +// identity no matter which collector produced it. +const botLoginSuffix = `[bot]` + type GithubAccountEdge struct { Login string Id int `graphql:"databaseId"` @@ -30,18 +37,76 @@ type GithubAccountEdge struct { AvatarUrl string HtmlUrl string `graphql:"url"` } + +// GithubBotEdge holds what a `Bot` actor exposes. The GitHub schema gives `Bot` +// a narrower field set than `User`: no name, company or email. +type GithubBotEdge struct { + Login string + Id int `graphql:"databaseId"` + AvatarUrl string + HtmlUrl string `graphql:"url"` +} + +// GraphqlInlineAccountQuery is for fields the schema types as `User`, such as +// assignees and commit authors. type GraphqlInlineAccountQuery struct { GithubAccountEdge `graphql:"... on User"` } -func extractGraphqlPreAccount(result *[]interface{}, res *GraphqlInlineAccountQuery, repoId int, connId uint64) { - if res == nil || res.Id == 0 { +// Account returns the account this query resolved to, or a zero value. +func (q *GraphqlInlineAccountQuery) Account() GithubAccountEdge { + if q == nil { + return GithubAccountEdge{} + } + return q.GithubAccountEdge +} + +// GraphqlInlineActorQuery is for fields the schema types as `Actor`, such as a +// pull request author or the user who merged it. `Actor` is implemented by +// `User` and `Bot` among others, so spreading only `... on User` leaves every +// GitHub App, Dependabot or Renovate author empty. +// +// The two inline queries cannot be merged into one: spreading `... on Bot` on a +// `User`-typed field makes GitHub reject the whole query with +// "Fragment on Bot can't be spread inside User". +type GraphqlInlineActorQuery struct { + GithubAccountEdge `graphql:"... on User"` + Bot GithubBotEdge `graphql:"... on Bot"` +} + +// Account returns the actor as an account, whichever type it resolved to. +func (q *GraphqlInlineActorQuery) Account() GithubAccountEdge { + if q == nil { + return GithubAccountEdge{} + } + if q.GithubAccountEdge.Id != 0 { + return q.GithubAccountEdge + } + if q.Bot.Id == 0 { + return GithubAccountEdge{} + } + login := q.Bot.Login + if !strings.HasSuffix(login, botLoginSuffix) { + login += botLoginSuffix + } + // Name, Company and Email stay empty: that is what the REST collector stores + // for a bot as well. + return GithubAccountEdge{ + Login: login, + Id: q.Bot.Id, + AvatarUrl: q.Bot.AvatarUrl, + HtmlUrl: q.Bot.HtmlUrl, + } +} + +func extractGraphqlPreAccount(result *[]interface{}, account GithubAccountEdge, repoId int, connId uint64) { + if account.Id == 0 { return } *result = append(*result, &models.GithubRepoAccount{ ConnectionId: connId, RepoGithubId: repoId, - Login: res.Login, - AccountId: res.Id, + Login: account.Login, + AccountId: account.Id, }) } diff --git a/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor_test.go b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor_test.go new file mode 100644 index 00000000000..4979e552343 --- /dev/null +++ b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor_test.go @@ -0,0 +1,121 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tasks + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/merico-ai/graphql" + "github.com/stretchr/testify/assert" +) + +func TestInlineAccountQueryAccount(t *testing.T) { + var nilQuery *GraphqlInlineAccountQuery + assert.Equal(t, 0, nilQuery.Account().Id) + + user := &GraphqlInlineAccountQuery{ + GithubAccountEdge: GithubAccountEdge{Login: `octocat`, Id: 583231, Name: `The Octocat`}, + } + assert.Equal(t, `octocat`, user.Account().Login) + assert.Equal(t, 583231, user.Account().Id) +} + +func TestInlineActorQueryAccount(t *testing.T) { + var nilQuery *GraphqlInlineActorQuery + assert.Equal(t, GithubAccountEdge{}, nilQuery.Account()) + + // an actor that resolved to neither User nor Bot, e.g. a deleted account + empty := &GraphqlInlineActorQuery{} + assert.Equal(t, GithubAccountEdge{}, empty.Account()) + + user := &GraphqlInlineActorQuery{ + GithubAccountEdge: GithubAccountEdge{Login: `octocat`, Id: 583231, Email: `octocat@github.com`}, + } + assert.Equal(t, `octocat`, user.Account().Login) + assert.Equal(t, `octocat@github.com`, user.Account().Email) + + // Bot.login has no `[bot]` suffix, the REST collector stores one + bot := &GraphqlInlineActorQuery{ + Bot: GithubBotEdge{Login: `dependabot`, Id: 49699333, HtmlUrl: `https://github.com/apps/dependabot`}, + } + assert.Equal(t, `dependabot[bot]`, bot.Account().Login) + assert.Equal(t, 49699333, bot.Account().Id) + assert.Equal(t, `https://github.com/apps/dependabot`, bot.Account().HtmlUrl) + assert.Empty(t, bot.Account().Name) + + // a login that already carries the suffix is left alone + suffixed := &GraphqlInlineActorQuery{ + Bot: GithubBotEdge{Login: `renovate[bot]`, Id: 29139614}, + } + assert.Equal(t, `renovate[bot]`, suffixed.Account().Login) +} + +// the raw layer holds the marshalled query result, so a bot author has to survive +// the round trip the extractors read it back through +func TestInlineActorQueryUnmarshalBotAuthor(t *testing.T) { + raw := `{"Login":"","Id":0,"Name":"","Company":"","Email":"","AvatarUrl":"","HtmlUrl":"", + "Bot":{"Login":"dependabot","Id":49699333,"AvatarUrl":"","HtmlUrl":""}}` + actor := &GraphqlInlineActorQuery{} + assert.Nil(t, json.Unmarshal([]byte(raw), actor)) + assert.Equal(t, `dependabot[bot]`, actor.Account().Login) + assert.Equal(t, 49699333, actor.Account().Id) + + // rows collected before the Bot fragment existed still extract as before + legacy := `{"Login":"octocat","Id":583231,"Name":"The Octocat","Company":"","Email":"","AvatarUrl":"","HtmlUrl":""}` + actor = &GraphqlInlineActorQuery{} + assert.Nil(t, json.Unmarshal([]byte(legacy), actor)) + assert.Equal(t, `octocat`, actor.Account().Login) +} + +func TestExtractGraphqlPreAccountSkipsUnresolvedActors(t *testing.T) { + var results []interface{} + var missing *GraphqlInlineActorQuery + extractGraphqlPreAccount(&results, missing.Account(), 1, 1) + assert.Empty(t, results) + + bot := &GraphqlInlineActorQuery{Bot: GithubBotEdge{Login: `dependabot`, Id: 49699333}} + extractGraphqlPreAccount(&results, bot.Account(), 1, 1) + assert.Len(t, results, 1) +} + +// `... on Bot` may only be spread where the schema says `Actor`; on a `User` field +// GitHub rejects the whole query with "Fragment on Bot can't be spread inside User" +func TestBotFragmentOnlySpreadOnActorFields(t *testing.T) { + prQuery, _ := graphql.ConstructQuery(&GraphqlQueryPrWrapper{}, map[string]interface{}{}) + issueQuery, _ := graphql.ConstructQuery(&GraphqlQueryIssueWrapper{}, map[string]interface{}{}) + + assert.Contains(t, prQuery, `author{... on User{`) + assert.Contains(t, prQuery, `... on Bot{`) + assert.Contains(t, issueQuery, `... on Bot{`) + + // assignees and commit authors are typed `User`, they must stay User-only + for _, userField := range []string{`assignees(first: 1){nodes{`, `assignees(first: 100){nodes{`} { + if idx := strings.Index(prQuery+issueQuery, userField); idx >= 0 { + tail := (prQuery + issueQuery)[idx : idx+120] + assert.NotContains(t, tail, `... on Bot`) + } + } + commitAuthor := `commits(first: 100){` + if idx := strings.Index(prQuery, commitAuthor); idx >= 0 { + tail := prQuery[idx : idx+400] + assert.Contains(t, tail, `user{... on User{`) + assert.NotContains(t, tail, `user{... on User{login,databaseId,name,company,email,avatarUrl,url},... on Bot`) + } +} diff --git a/backend/plugins/github_graphql/tasks/issue_collector.go b/backend/plugins/github_graphql/tasks/issue_collector.go index 36c33584c8a..9069a1c720d 100644 --- a/backend/plugins/github_graphql/tasks/issue_collector.go +++ b/backend/plugins/github_graphql/tasks/issue_collector.go @@ -68,7 +68,7 @@ type GraphqlQueryIssue struct { StateReason string Title string Body string - Author *GraphqlInlineAccountQuery + Author *GraphqlInlineActorQuery Url string ClosedAt *time.Time CreatedAt time.Time diff --git a/backend/plugins/github_graphql/tasks/issue_extractor.go b/backend/plugins/github_graphql/tasks/issue_extractor.go index e27679c83e9..a825c6a89c2 100644 --- a/backend/plugins/github_graphql/tasks/issue_extractor.go +++ b/backend/plugins/github_graphql/tasks/issue_extractor.go @@ -83,9 +83,9 @@ func ExtractIssues(taskCtx plugin.SubTaskContext) errors.Error { results = append(results, githubLabels...) results = append(results, githubIssue) if len(issue.AssigneeList.Assignees) > 0 { - extractGraphqlPreAccount(&results, &issue.AssigneeList.Assignees[0], data.Options.GithubId, data.Options.ConnectionId) + extractGraphqlPreAccount(&results, issue.AssigneeList.Assignees[0].Account(), data.Options.GithubId, data.Options.ConnectionId) } - extractGraphqlPreAccount(&results, issue.Author, data.Options.GithubId, data.Options.ConnectionId) + extractGraphqlPreAccount(&results, issue.Author.Account(), data.Options.GithubId, data.Options.ConnectionId) for _, assignee := range issue.AssigneeList.Assignees { issueAssignee := &models.GithubIssueAssignee{ ConnectionId: githubIssue.ConnectionId, @@ -147,9 +147,9 @@ func convertGithubIssue(milestoneMap map[int]int, issue *GraphqlQueryIssue, conn githubIssue.AssigneeId = issue.AssigneeList.Assignees[0].Id githubIssue.AssigneeName = issue.AssigneeList.Assignees[0].Login } - if issue.Author != nil { - githubIssue.AuthorId = issue.Author.Id - githubIssue.AuthorName = issue.Author.Login + if author := issue.Author.Account(); author.Id != 0 { + githubIssue.AuthorId = author.Id + githubIssue.AuthorName = author.Login } if issue.ClosedAt != nil { temp := uint(issue.ClosedAt.Sub(issue.CreatedAt).Minutes()) diff --git a/backend/plugins/github_graphql/tasks/pr_collector.go b/backend/plugins/github_graphql/tasks/pr_collector.go index 669839385d4..c05cac2efa7 100644 --- a/backend/plugins/github_graphql/tasks/pr_collector.go +++ b/backend/plugins/github_graphql/tasks/pr_collector.go @@ -72,7 +72,7 @@ type GraphqlQueryPr struct { Name string } } `graphql:"labels(first: 100)"` - Author *GraphqlInlineAccountQuery + Author *GraphqlInlineActorQuery Assignees struct { // FIXME now domain layer just support one assignee Assignees []GraphqlInlineAccountQuery `graphql:"nodes"` @@ -99,7 +99,7 @@ type GraphqlQueryPr struct { } `graphql:"reviews(first: 100)"` Additions int Deletions int - MergedBy *GraphqlInlineAccountQuery + MergedBy *GraphqlInlineActorQuery ReviewRequests struct { Nodes []ReviewRequestNode `graphql:"nodes"` } `graphql:"reviewRequests(first: 10)"` @@ -128,7 +128,7 @@ type Team struct { type GraphqlQueryReview struct { Body string - Author *GraphqlInlineAccountQuery + Author *GraphqlInlineActorQuery State string `json:"state"` DatabaseId int `json:"databaseId"` Commit struct { diff --git a/backend/plugins/github_graphql/tasks/pr_extractor.go b/backend/plugins/github_graphql/tasks/pr_extractor.go index 59bc72cdb3f..8d6fd863bc4 100644 --- a/backend/plugins/github_graphql/tasks/pr_extractor.go +++ b/backend/plugins/github_graphql/tasks/pr_extractor.go @@ -78,7 +78,7 @@ func ExtractPrs(taskCtx plugin.SubTaskContext) errors.Error { if err != nil { return nil, err } - extractGraphqlPreAccount(&results, rawL.Author, data.Options.GithubId, data.Options.ConnectionId) + extractGraphqlPreAccount(&results, rawL.Author.Account(), data.Options.GithubId, data.Options.ConnectionId) for _, label := range rawL.Labels.Nodes { results = append(results, &models.GithubPrLabel{ ConnectionId: data.Options.ConnectionId, @@ -110,10 +110,10 @@ func ExtractPrs(taskCtx plugin.SubTaskContext) errors.Error { PullRequestId: githubPr.GithubId, } - if apiPullRequestReview.Author != nil { - githubPrReview.AuthorUserId = apiPullRequestReview.Author.Id - githubPrReview.AuthorUsername = apiPullRequestReview.Author.Login - extractGraphqlPreAccount(&results, apiPullRequestReview.Author, data.Options.GithubId, data.Options.ConnectionId) + if author := apiPullRequestReview.Author.Account(); author.Id != 0 { + githubPrReview.AuthorUserId = author.Id + githubPrReview.AuthorUsername = author.Login + extractGraphqlPreAccount(&results, author, data.Options.GithubId, data.Options.ConnectionId) } results = append(results, githubPrReview) @@ -145,7 +145,7 @@ func ExtractPrs(taskCtx plugin.SubTaskContext) errors.Error { CommitAuthoredDate: githubCommit.AuthoredDate, } results = append(results, githubPullRequestCommit) - extractGraphqlPreAccount(&results, apiPullRequestCommit.Commit.Author.User, data.Options.GithubId, data.Options.ConnectionId) + extractGraphqlPreAccount(&results, apiPullRequestCommit.Commit.Author.User.Account(), data.Options.GithubId, data.Options.ConnectionId) } return results, nil }, @@ -180,16 +180,16 @@ func convertGithubPullRequest(pull *GraphqlQueryPr, connId uint64, repoId int) ( Deletions: pull.Deletions, IsDraft: pull.IsDraft, } - if pull.MergedBy != nil { - githubPull.MergedByName = pull.MergedBy.Login - githubPull.MergedById = pull.MergedBy.Id + if mergedBy := pull.MergedBy.Account(); mergedBy.Id != 0 { + githubPull.MergedByName = mergedBy.Login + githubPull.MergedById = mergedBy.Id } if pull.MergeCommit != nil { githubPull.MergeCommitSha = pull.MergeCommit.Oid } - if pull.Author != nil { - githubPull.AuthorName = pull.Author.Login - githubPull.AuthorId = pull.Author.Id + if author := pull.Author.Account(); author.Id != 0 { + githubPull.AuthorName = author.Login + githubPull.AuthorId = author.Id } return githubPull, nil } @@ -206,8 +206,8 @@ func convertPullRequestCommit(prCommit GraphqlQueryCommit) (*models.GithubCommit CommittedDate: prCommit.Commit.Committer.Date, Url: prCommit.Url, } - if prCommit.Commit.Author.User != nil { - githubCommit.AuthorId = prCommit.Commit.Author.User.Id + if user := prCommit.Commit.Author.User.Account(); user.Id != 0 { + githubCommit.AuthorId = user.Id } return githubCommit, nil }