From d23460a590aabcf5033fa0b18cff337523187f8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 14 Jan 2022 15:06:33 +0100 Subject: [PATCH] Fix filtering issues by milestone The milestone filter in the `Repository.issues` GraphQL connection is broken, so switch to the Search API for any milestone filtering. Previously, we used to work around this by obtaining the milestone database ID from decoding the GraphQL ID, but that no longer works since the GraphQL ID format has changed. --- api/queries_repo.go | 40 ----------- pkg/cmd/issue/list/http.go | 46 ++----------- pkg/cmd/issue/list/list.go | 32 ++++++++- pkg/cmd/issue/list/list_test.go | 118 +++++--------------------------- 4 files changed, 52 insertions(+), 184 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 796c2ed45..1c8a210fb 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1137,46 +1137,6 @@ func RepoMilestones(client *Client, repo ghrepo.Interface, state string) ([]Repo return milestones, nil } -func MilestoneByTitle(client *Client, repo ghrepo.Interface, state, title string) (*RepoMilestone, error) { - milestones, err := RepoMilestones(client, repo, state) - if err != nil { - return nil, err - } - - for i := range milestones { - if strings.EqualFold(milestones[i].Title, title) { - return &milestones[i], nil - } - } - return nil, fmt.Errorf("no milestone found with title %q", title) -} - -func MilestoneByNumber(client *Client, repo ghrepo.Interface, number int32) (*RepoMilestone, error) { - var query struct { - Repository struct { - Milestone *RepoMilestone `graphql:"milestone(number: $number)"` - } `graphql:"repository(owner: $owner, name: $name)"` - } - - variables := map[string]interface{}{ - "owner": githubv4.String(repo.RepoOwner()), - "name": githubv4.String(repo.RepoName()), - "number": githubv4.Int(number), - } - - gql := graphQLClient(client.http, repo.RepoHost()) - - err := gql.QueryNamed(context.Background(), "RepositoryMilestoneByNumber", &query, variables) - if err != nil { - return nil, err - } - if query.Repository.Milestone == nil { - return nil, fmt.Errorf("no milestone found with number '%d'", number) - } - - return query.Repository.Milestone, nil -} - func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []string) ([]string, error) { var paths []string projects, err := RepoAndOrgProjects(client, repo) diff --git a/pkg/cmd/issue/list/http.go b/pkg/cmd/issue/list/http.go index a992b553d..40a32d748 100644 --- a/pkg/cmd/issue/list/http.go +++ b/pkg/cmd/issue/list/http.go @@ -1,10 +1,7 @@ package list import ( - "encoding/base64" "fmt" - "strconv" - "strings" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" @@ -26,10 +23,10 @@ func listIssues(client *api.Client, repo ghrepo.Interface, filters prShared.Filt fragments := fmt.Sprintf("fragment issue on Issue {%s}", api.PullRequestGraphQL(filters.Fields)) query := fragments + ` - query IssueList($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $assignee: String, $author: String, $mention: String, $milestone: String) { + query IssueList($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $assignee: String, $author: String, $mention: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled - issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mention, milestone: $milestone}) { + issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mention}) { totalCount nodes { ...issue @@ -59,24 +56,10 @@ func listIssues(client *api.Client, repo ghrepo.Interface, filters prShared.Filt } if filters.Milestone != "" { - var milestone *api.RepoMilestone - if milestoneNumber, err := strconv.ParseInt(filters.Milestone, 10, 32); err == nil { - milestone, err = api.MilestoneByNumber(client, repo, int32(milestoneNumber)) - if err != nil { - return nil, err - } - } else { - milestone, err = api.MilestoneByTitle(client, repo, "all", filters.Milestone) - if err != nil { - return nil, err - } - } - - milestoneRESTID, err := milestoneNodeIdToDatabaseId(milestone.ID) - if err != nil { - return nil, err - } - variables["milestone"] = milestoneRESTID + // The "milestone" filter in the GraphQL connection doesn't work as documented and accepts neither a + // milestone number nor a title. It does accept a numeric database ID, but we cannot obtain one + // using the GraphQL API. + return nil, fmt.Errorf("cannot filter by milestone using the `Repository.issues` GraphQL connection") } type responseData struct { @@ -204,23 +187,6 @@ loop: return &ic, nil } -// milestoneNodeIdToDatabaseId extracts the REST Database ID from the GraphQL Node ID -// This conversion is necessary since the GraphQL API requires the use of the milestone's database ID -// for querying the related issues. -func milestoneNodeIdToDatabaseId(nodeId string) (string, error) { - // The Node ID is Base64 obfuscated, with an underlying pattern: - // "09:Milestone12345", where "12345" is the database ID - decoded, err := base64.StdEncoding.DecodeString(nodeId) - if err != nil { - return "", err - } - splitted := strings.Split(string(decoded), "Milestone") - if len(splitted) != 2 { - return "", fmt.Errorf("couldn't get database id from node id") - } - return splitted[1], nil -} - func min(a, b int) int { if a < b { return a diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index 8e26a8afe..bf0e9bae6 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -1,6 +1,7 @@ package list import ( + "context" "fmt" "net/http" "strconv" @@ -9,6 +10,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" issueShared "github.com/cli/cli/v2/pkg/cmd/issue/shared" "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -16,6 +18,8 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" + graphql "github.com/cli/shurcooL-graphql" + "github.com/shurcooL/githubv4" "github.com/spf13/cobra" ) @@ -182,9 +186,9 @@ func listRun(opts *ListOptions) error { func issueList(client *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.IssuesAndTotalCount, error) { apiClient := api.NewClientFromHTTP(client) - if filters.Search != "" || len(filters.Labels) > 0 { + if filters.Search != "" || len(filters.Labels) > 0 || filters.Milestone != "" { if milestoneNumber, err := strconv.ParseInt(filters.Milestone, 10, 32); err == nil { - milestone, err := api.MilestoneByNumber(apiClient, repo, int32(milestoneNumber)) + milestone, err := milestoneByNumber(client, repo, int32(milestoneNumber)) if err != nil { return nil, err } @@ -211,3 +215,27 @@ func issueList(client *http.Client, repo ghrepo.Interface, filters prShared.Filt return listIssues(apiClient, repo, filters, limit) } + +func milestoneByNumber(client *http.Client, repo ghrepo.Interface, number int32) (*api.RepoMilestone, error) { + var query struct { + Repository struct { + Milestone *api.RepoMilestone `graphql:"milestone(number: $number)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "number": githubv4.Int(number), + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) + if err := gql.QueryNamed(context.Background(), "RepositoryMilestoneByNumber", &query, variables); err != nil { + return nil, err + } + if query.Repository.Milestone == nil { + return nil, fmt.Errorf("no milestone found with number '%d'", number) + } + + return query.Repository.Milestone, nil +} diff --git a/pkg/cmd/issue/list/list_test.go b/pkg/cmd/issue/list/list_test.go index cce2849f9..732db3488 100644 --- a/pkg/cmd/issue/list/list_test.go +++ b/pkg/cmd/issue/list/list_test.go @@ -121,20 +121,10 @@ func TestIssueList_tty_withFlags(t *testing.T) { assert.Equal(t, "probablyCher", params["assignee"].(string)) assert.Equal(t, "foo", params["author"].(string)) assert.Equal(t, "me", params["mention"].(string)) - assert.Equal(t, "12345", params["milestone"].(string)) assert.Equal(t, []interface{}{"OPEN"}, params["states"].([]interface{})) })) - http.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [{ "title":"1.x", "id": "MDk6TWlsZXN0b25lMTIzNDU=" }], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - - output, err := runCommand(http, true, "-a probablyCher -s open -A foo --mention me --milestone 1.x") + output, err := runCommand(http, true, "-a probablyCher -s open -A foo --mention me") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -269,45 +259,7 @@ func Test_issueList(t *testing.T) { httpmock.GraphQL(`query RepositoryMilestoneByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "milestone": { - "id": "MDk6TWlsZXN0b25lMTIzNDU=" - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query IssueList\b`), - httpmock.GraphQLQuery(` - { "data": { "repository": { - "hasIssuesEnabled": true, - "issues": { "nodes": [] } - } } }`, func(_ string, params map[string]interface{}) { - assert.Equal(t, map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "limit": float64(30), - "states": []interface{}{"OPEN"}, - "milestone": "12345", - }, params) - })) - }, - }, - { - name: "milestone by number with search", - args: args{ - limit: 30, - repo: ghrepo.New("OWNER", "REPO"), - filters: prShared.FilterOptions{ - Entity: "issue", - State: "open", - Milestone: "13", - Search: "auth bug", - }, - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestone": { - "title": "Big 1.0", - "id": "MDk6TWlsZXN0b25lMTIzNDU=" + "title": "1.x" } } } } `)) reg.Register( @@ -324,40 +276,7 @@ func Test_issueList(t *testing.T) { "owner": "OWNER", "repo": "REPO", "limit": float64(30), - "query": "repo:OWNER/REPO is:issue is:open milestone:\"Big 1.0\" auth bug", - "type": "ISSUE", - }, params) - })) - }, - }, - { - name: "milestone by title with search", - args: args{ - limit: 30, - repo: ghrepo.New("OWNER", "REPO"), - filters: prShared.FilterOptions{ - Entity: "issue", - State: "open", - Milestone: "Big 1.0", - Search: "auth bug", - }, - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query IssueSearch\b`), - httpmock.GraphQLQuery(` - { "data": { - "repository": { "hasIssuesEnabled": true }, - "search": { - "issueCount": 0, - "nodes": [] - } - } }`, func(_ string, params map[string]interface{}) { - assert.Equal(t, map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "limit": float64(30), - "query": "repo:OWNER/REPO is:issue is:open milestone:\"Big 1.0\" auth bug", + "query": "repo:OWNER/REPO is:issue is:open milestone:1.x", "type": "ISSUE", }, params) })) @@ -376,26 +295,21 @@ func Test_issueList(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [{ "title":"1.x", "id": "MDk6TWlsZXN0b25lMTIzNDU=" }], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query IssueList\b`), + httpmock.GraphQL(`query IssueSearch\b`), httpmock.GraphQLQuery(` - { "data": { "repository": { - "hasIssuesEnabled": true, - "issues": { "nodes": [] } - } } }`, func(_ string, params map[string]interface{}) { + { "data": { + "repository": { "hasIssuesEnabled": true }, + "search": { + "issueCount": 0, + "nodes": [] + } + } }`, func(_ string, params map[string]interface{}) { assert.Equal(t, map[string]interface{}{ - "owner": "OWNER", - "repo": "REPO", - "limit": float64(30), - "states": []interface{}{"OPEN"}, - "milestone": "12345", + "owner": "OWNER", + "repo": "REPO", + "limit": float64(30), + "query": "repo:OWNER/REPO is:issue is:open milestone:1.x", + "type": "ISSUE", }, params) })) },