fix(github_graphql): resolve Bot actors so bot-authored PRs keep their author

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

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,
 	})
 }

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`)
+	}
+}

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

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())

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 {

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
 }