diff --git a/backend/plugins/github/e2e/account_test.go b/backend/plugins/github/e2e/account_test.go index 03f71ecb49d..07fddbc20f3 100644 --- a/backend/plugins/github/e2e/account_test.go +++ b/backend/plugins/github/e2e/account_test.go @@ -18,15 +18,20 @@ limitations under the License. package e2e import ( + "encoding/json" + "fmt" "testing" + "time" "github.com/apache/devlake/core/dal" "github.com/apache/devlake/core/models/domainlayer/crossdomain" "github.com/apache/devlake/core/models/domainlayer/didgen" "github.com/apache/devlake/helpers/e2ehelper" + "github.com/apache/devlake/helpers/pluginhelper/api" "github.com/apache/devlake/plugins/github/impl" "github.com/apache/devlake/plugins/github/models" "github.com/apache/devlake/plugins/github/tasks" + graphqlTasks "github.com/apache/devlake/plugins/github_graphql/tasks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -142,3 +147,103 @@ func TestAccountDataFlow(t *testing.T) { } assert.True(t, sawOrphanCase, "fixture should include the non-committer orphan case (milichev)") } + +// Without profile enrichment, bot classification must survive every repository's +// conversion of the shared account, including REST aliases of a GraphQL actor. +func TestAccountDataFlowBotWithoutProfile(t *testing.T) { + t.Setenv("GITHUB_PR_EXCLUDELIST", "") + tasks.ResetExcludedUsernamesForTest() + t.Cleanup(tasks.ResetExcludedUsernamesForTest) + df := e2ehelper.NewDataFlowTester(t, "github", impl.Github{}) + for _, model := range []interface{ TableName() string }{ + &models.GithubAccount{}, &models.GithubAccountOrg{}, &models.GithubRepoAccount{}, + &crossdomain.Account{}, &models.GithubPullRequest{}, &models.GithubPrReview{}, + &models.GithubReviewer{}, &models.GithubPrLabel{}, &models.GithubPrCommit{}, + &models.GithubCommit{}, &models.GithubPrComment{}, + } { + df.FlushTabler(model) + } + graphqlTable := "_raw_" + graphqlTasks.RAW_PRS_TABLE + restTable := "_raw_" + tasks.RAW_PR_REVIEW_COMMENTS_TABLE + df.FlushRawTable(graphqlTable) + df.FlushRawTable(restTable) + + accountIdGen := didgen.NewDomainIdGenerator(&models.GithubAccount{}) + for step, run := range []struct { + repoID int + graphql bool + }{ + {101, true}, {102, false}, {101, false}, {102, true}, + } { + t.Run(fmt.Sprintf("%d/repo-%d/graphql-%t", step, run.repoID, run.graphql), func(t *testing.T) { + name := fmt.Sprintf("owner/repo-%d", run.repoID) + data := &tasks.GithubTaskData{Options: &tasks.GithubOptions{ + ConnectionId: 1, GithubId: run.repoID, Name: name, + }} + params, err := json.Marshal(tasks.GithubApiParams{ConnectionId: 1, Name: name}) + require.NoError(t, err) + wantBotLogin := "Copilot[bot]" + if run.graphql { + // Match the real decoder's overlapping fragment fields and the + // collector's JSON serialization, rather than a Bot-only fragment. + raw := fmt.Sprintf(`{ + "DatabaseId":%d, "Number":1, "State":"OPEN", + "CreatedAt":"2026-01-01T00:00:00Z", "UpdatedAt":"2026-01-02T00:00:00Z", + "Author":{ + "TypeName":"User", "Login":"octocat", "Id":583231, + "Bot":{"Login":"octocat", "Id":583231} + }, + "Reviews":{"Nodes":[{ + "databaseId":%d, "state":"COMMENTED", + "Author":{ + "TypeName":"Bot", "Login":"copilot-pull-request-reviewer", "Id":175728472, + "Bot":{"Login":"copilot-pull-request-reviewer", "Id":175728472} + } + }]} + }`, run.repoID, run.repoID) + var pr graphqlTasks.GraphqlQueryPr + require.NoError(t, json.Unmarshal([]byte(raw), &pr)) + serialized, err := json.Marshal(pr) + require.NoError(t, err) + require.NoError(t, df.Db.Table(graphqlTable).Create(&api.RawData{ + Params: string(params), Data: serialized, CreatedAt: time.Now().UTC(), + }).Error) + df.Subtask(graphqlTasks.ExtractPrsMeta, data) + wantBotLogin = "copilot-pull-request-reviewer[bot]" + } else { + for i, user := range []string{ + `{"login":"Copilot","id":175728472,"type":"Bot"}`, + `{"login":"octocat","id":583231,"type":"User"}`, + } { + raw := fmt.Sprintf(`{"id":%d,"body":"review","user":%s,"pull_request_url":"https://api.github.com/repos/%s/pulls/1","created_at":"2026-01-01T00:00:00Z","updated_at":"2026-01-02T00:00:00Z"}`, run.repoID*10+i, user, name) + require.NoError(t, df.Db.Table(restTable).Create(&api.RawData{ + Params: string(params), Data: []byte(raw), CreatedAt: time.Now().UTC(), + }).Error) + } + df.Subtask(tasks.ExtractApiPrReviewCommentsMeta, data) + } + df.Subtask(tasks.ConvertAccountsMeta, data) + for _, expected := range []struct { + id int + login string + bot bool + }{ + {175728472, wantBotLogin, true}, {583231, "octocat", false}, + } { + var mapping models.GithubRepoAccount + require.NoError(t, df.Dal.First(&mapping, dal.Where("connection_id = ? AND repo_github_id = ? AND account_id = ?", 1, run.repoID, expected.id))) + assert.Equal(t, expected.login, mapping.Login) + var account crossdomain.Account + require.NoError(t, df.Dal.First(&account, dal.Where("id = ?", accountIdGen.Generate(uint64(1), expected.id)))) + assert.Equal(t, expected.login, account.UserName) + assert.Equal(t, expected.bot, account.IsBot) + } + }) + } + count, err := df.Dal.Count(dal.From(&models.GithubAccount{})) + require.NoError(t, err) + assert.Zero(t, count, "profile enrichment must remain absent throughout the test") + count, err = df.Dal.Count(dal.From(&crossdomain.Account{})) + require.NoError(t, err) + assert.Equal(t, int64(2), count, "repositories must share the same numeric account identities") +} diff --git a/backend/plugins/github/tasks/account_pre_extractor.go b/backend/plugins/github/tasks/account_pre_extractor.go index ef60312d34d..cab6ddc1a06 100644 --- a/backend/plugins/github/tasks/account_pre_extractor.go +++ b/backend/plugins/github/tasks/account_pre_extractor.go @@ -18,6 +18,8 @@ limitations under the License. package tasks import ( + "strings" + "github.com/apache/devlake/core/errors" "github.com/apache/devlake/plugins/github/models" ) @@ -44,10 +46,16 @@ type GithubAccountResponse struct { } func convertAccount(res *GithubAccountResponse, repoId int, connId uint64) (*models.GithubRepoAccount, errors.Error) { + login := res.Login + // Some REST endpoints return bot aliases without the usual suffix. Keep + // the explicit type available to conversion even without profile enrichment. + if res.Type == "Bot" && !strings.HasSuffix(login, "[bot]") { + login += "[bot]" + } githubAccount := &models.GithubRepoAccount{ ConnectionId: connId, RepoGithubId: repoId, - Login: res.Login, + Login: login, AccountId: res.Id, } return githubAccount, nil diff --git a/backend/plugins/github/tasks/account_pre_extractor_test.go b/backend/plugins/github/tasks/account_pre_extractor_test.go new file mode 100644 index 00000000000..1617e4d9bf0 --- /dev/null +++ b/backend/plugins/github/tasks/account_pre_extractor_test.go @@ -0,0 +1,48 @@ +/* +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" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConvertAccountBotType(t *testing.T) { + for _, tc := range []struct{ name, raw, wantLogin string }{ + {"bot alias", `{"login":"Copilot","id":175728472,"type":"Bot"}`, "Copilot[bot]"}, + {"suffixed bot", `{"login":"copilot-pull-request-reviewer[bot]","id":175728472,"type":"Bot"}`, "copilot-pull-request-reviewer[bot]"}, + {"human", `{"login":"octocat","id":583231,"type":"User"}`, "octocat"}, + {"no type", `{"login":"legacy","id":1}`, "legacy"}, + } { + t.Run(tc.name, func(t *testing.T) { + var response GithubAccountResponse + require.NoError(t, json.Unmarshal([]byte(tc.raw), &response)) + original := response + account, err := convertAccount(&response, 42, 7) + require.NoError(t, err) + assert.Equal(t, tc.wantLogin, account.Login) + assert.Equal(t, response.Id, account.AccountId) + assert.Equal(t, 42, account.RepoGithubId) + assert.Equal(t, uint64(7), account.ConnectionId) + assert.Equal(t, original, response, "normalization must not mutate the API response") + }) + } +} 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 6e114cb323a..3be41b021c9 100644 --- a/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go +++ b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor.go @@ -70,6 +70,7 @@ func (q *GraphqlInlineAccountQuery) Account() GithubAccountEdge { // `User`-typed field makes GitHub reject the whole query with // "Fragment on Bot can't be spread inside User". type GraphqlInlineActorQuery struct { + TypeName string `graphql:"__typename"` GithubAccountEdge `graphql:"... on User"` Bot GithubBotEdge `graphql:"... on Bot"` } @@ -79,8 +80,21 @@ func (q *GraphqlInlineActorQuery) Account() GithubAccountEdge { if q == nil { return GithubAccountEdge{} } - if q.GithubAccountEdge.Id != 0 { + // The GraphQL decoder populates overlapping fields in both fragments. + // Only __typename identifies which fragment actually resolved. + switch q.TypeName { + case "User": return q.GithubAccountEdge + case "Bot": + // Normalize the bot fragment below. + case "": + // Historical raw records lack a discriminator. Preserve their original + // user-first behavior, including when both fragments are populated. + if q.GithubAccountEdge.Id != 0 { + return q.GithubAccountEdge + } + default: + return GithubAccountEdge{} } if q.Bot.Id == 0 { return GithubAccountEdge{} 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 index 4979e552343..e09ee2423f6 100644 --- a/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor_test.go +++ b/backend/plugins/github_graphql/tasks/account_graphql_pre_extractor_test.go @@ -18,12 +18,16 @@ limitations under the License. package tasks import ( + "context" "encoding/json" + "io" + "net/http" "strings" "testing" "github.com/merico-ai/graphql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestInlineAccountQueryAccount(t *testing.T) { @@ -95,27 +99,139 @@ func TestExtractGraphqlPreAccountSkipsUnresolvedActors(t *testing.T) { 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" +// The client decoder fills overlapping fields in both inline fragments. Exercise +// that decoder, then the same JSON round trip used by collection and extraction. +func TestInlineActorQueryClientRoundTrip(t *testing.T) { + for _, tc := range []struct { + name, actorType, login, wantLogin string + id int + }{ + {"copilot", "Bot", "copilot-pull-request-reviewer", "copilot-pull-request-reviewer[bot]", 175728472}, + {"human", "User", "octocat", "octocat", 583231}, + {"suffixed bot", "Bot", "renovate[bot]", "renovate[bot]", 29139614}, + } { + t.Run(tc.name, func(t *testing.T) { + client := graphql.NewClient("https://github.invalid/graphql", &http.Client{ + Transport: actorRoundTripper(func(req *http.Request) (*http.Response, error) { + var request struct{ Query string } + require.NoError(t, json.NewDecoder(req.Body).Decode(&request)) + assert.Contains(t, request.Query, "__typename") + actor := map[string]interface{}{ + "login": tc.login, "databaseId": tc.id, + "avatarUrl": "https://github.invalid/avatar", "url": "https://github.invalid/profile", + } + // Return only requested fields, as GitHub would. This also lets the + // regression expose the old accessor's incorrect login before the fix. + if strings.Contains(request.Query, "__typename") { + actor["__typename"] = tc.actorType + } + if tc.actorType == "User" { + actor["name"], actor["company"], actor["email"] = "The Octocat", "GitHub", "octocat@github.com" + } + body, err := json.Marshal(map[string]interface{}{"data": map[string]interface{}{"author": actor}}) + require.NoError(t, err) + return &http.Response{StatusCode: http.StatusOK, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(string(body)))}, nil + }), + }) + var query struct{ Author *GraphqlInlineActorQuery } + dataErrors, err := client.Query(context.Background(), &query, nil) + require.NoError(t, err) + require.Empty(t, dataErrors) + require.NotNil(t, query.Author) + // This is the decoder behavior that hand-built fragment tests missed. + assert.Equal(t, tc.id, query.Author.GithubAccountEdge.Id) + assert.Equal(t, tc.id, query.Author.Bot.Id) + raw, err := json.Marshal(query) + require.NoError(t, err) + var extracted struct{ Author *GraphqlInlineActorQuery } + require.NoError(t, json.Unmarshal(raw, &extracted)) + account := extracted.Author.Account() + assert.Equal(t, tc.wantLogin, account.Login) + assert.Equal(t, tc.id, account.Id) + assert.Equal(t, "https://github.invalid/avatar", account.AvatarUrl) + assert.Equal(t, "https://github.invalid/profile", account.HtmlUrl) + if tc.actorType == "User" { + assert.Equal(t, "The Octocat", account.Name) + assert.Equal(t, "octocat@github.com", account.Email) + } else { + assert.Empty(t, account.Name) + assert.Empty(t, account.Email) + } + }) + } +} + +type actorRoundTripper func(*http.Request) (*http.Response, error) + +func (f actorRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +func TestInlineActorQueryRawCompatibility(t *testing.T) { + for _, tc := range []struct { + name, raw string + want GithubAccountEdge + }{ + {"null", `null`, GithubAccountEdge{}}, + {"unresolved", `{}`, GithubAccountEdge{}}, + {"unsupported explicit type", `{"TypeName":"Organization","Login":"org","Id":1,"Bot":{"Login":"org","Id":1}}`, GithubAccountEdge{}}, + {"unresolved explicit bot", `{"TypeName":"Bot","Login":"stale-user","Id":1}`, GithubAccountEdge{}}, + {"explicit user selects user only", `{"TypeName":"User","Bot":{"Login":"stale-bot","Id":1}}`, GithubAccountEdge{}}, + {"legacy user", `{"Login":"octocat","Id":583231}`, GithubAccountEdge{Login: "octocat", Id: 583231}}, + {"legacy bot", `{"Bot":{"Login":"dependabot","Id":49699333}}`, GithubAccountEdge{Login: "dependabot[bot]", Id: 49699333}}, + // Old ambiguous records keep the historical user-first behavior; only a + // fresh collection can supply the missing type discriminator. + {"legacy overlapping fragments", `{"Login":"copilot-pull-request-reviewer","Id":175728472,"Bot":{"Login":"copilot-pull-request-reviewer","Id":175728472}}`, GithubAccountEdge{Login: "copilot-pull-request-reviewer", Id: 175728472}}, + } { + t.Run(tc.name, func(t *testing.T) { + var actor *GraphqlInlineActorQuery + require.NoError(t, json.Unmarshal([]byte(tc.raw), &actor)) + assert.Equal(t, tc.want, actor.Account()) + }) + } +} + +// Bot fragments and actor type discriminators must never reach User-only fields. 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`) + for _, query := range []interface{}{ + &GraphqlQueryPrWrapper{}, &GraphqlQueryPrDetailWrapper{}, + &GraphqlQueryIssueWrapper{}, &GraphqlQueryIssueDetailWrapper{}, + } { + generated, _ := graphql.ConstructQuery(query, map[string]interface{}{ + "pullRequest": []map[string]interface{}{{"number": graphql.Int(1)}}, + "issue": []map[string]interface{}{{"number": graphql.Int(1)}}, + }) + actorSelection := `{__typename,... on User{login,databaseId,name,company,email,avatarUrl,url},... on Bot{login,databaseId,avatarUrl,url}}` + assert.Contains(t, generated, "author"+actorSelection) + if strings.Contains(generated, "mergedBy") { + assert.Contains(t, generated, "mergedBy"+actorSelection) + // PR authors, review authors and mergedBy are all Actor fields. + assert.Equal(t, 3, strings.Count(generated, actorSelection)) + } + for _, prefix := range []string{"assignees(first: 1){nodes", "assignees(first: 100){nodes", "user"} { + if strings.Contains(generated, prefix+"{") { + assert.Contains(t, generated, prefix+`{... on User{login,databaseId,name,company,email,avatarUrl,url}}`) + } } } - 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`) +} + +func TestInlineActorQueryClientUnresolved(t *testing.T) { + for _, actorJSON := range []string{`null`, `{}`, `{"__typename":"Mannequin"}`} { + t.Run(actorJSON, func(t *testing.T) { + client := graphql.NewClient("https://github.invalid/graphql", &http.Client{ + Transport: actorRoundTripper(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`{"data":{"author":` + actorJSON + `}}`))}, nil + }), + }) + var query struct{ Author *GraphqlInlineActorQuery } + dataErrors, err := client.Query(context.Background(), &query, nil) + require.NoError(t, err) + require.Empty(t, dataErrors) + raw, err := json.Marshal(query) + require.NoError(t, err) + var extracted struct{ Author *GraphqlInlineActorQuery } + require.NoError(t, json.Unmarshal(raw, &extracted)) + assert.Equal(t, GithubAccountEdge{}, extracted.Author.Account()) + }) } }