From a90b62fb6c497b74826d9961010136dd77f474a7 Mon Sep 17 00:00:00 2001 From: zamasu Date: Sun, 11 Oct 2020 17:42:11 +0900 Subject: [PATCH 1/3] Fix parsing labels in paginated IssueList --- api/queries_issue.go | 7 +++- api/queries_issue_test.go | 82 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index d607901a3..fc88d1c16 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -270,7 +270,7 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str variables["milestone"] = milestoneRESTID } - var response struct { + type responseData struct { Repository struct { Issues struct { TotalCount int @@ -285,10 +285,12 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } var issues []Issue + var totalCount int pageLimit := min(limit, 100) loop: for { + var response responseData variables["limit"] = pageLimit err := client.GraphQL(repo.RepoHost(), query, variables, &response) if err != nil { @@ -297,6 +299,7 @@ loop: if !response.Repository.HasIssuesEnabled { return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo)) } + totalCount = response.Repository.Issues.TotalCount for _, issue := range response.Repository.Issues.Nodes { issues = append(issues, issue) @@ -313,7 +316,7 @@ loop: } } - res := IssuesAndTotalCount{Issues: issues, TotalCount: response.Repository.Issues.TotalCount} + res := IssuesAndTotalCount{Issues: issues, TotalCount: totalCount} return &res, nil } diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index 2ea64f0c6..32e757efd 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "io/ioutil" + "reflect" "testing" "github.com/cli/cli/internal/ghrepo" @@ -68,3 +69,84 @@ func TestIssueList(t *testing.T) { t.Errorf("expected %q, got %q", "ENDCURSOR", endCursor) } } + +func TestIssueList_pagination(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "hasIssuesEnabled": true, + "issues": { + "nodes": [ + { + "title": "issue1", + "labels": { "nodes": [ { "name": "bug" } ], "totalCount": 1 }, + "assignees": { "nodes": [ { "login": "user1" } ], "totalCount": 1 } + } + ], + "pageInfo": { + "hasNextPage": true, + "endCursor": "ENDCURSOR" + }, + "totalCount": 2 + } + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "hasIssuesEnabled": true, + "issues": { + "nodes": [ + { + "title": "issue2", + "labels": { "nodes": [ { "name": "enhancement" } ], "totalCount": 1 }, + "assignees": { "nodes": [ { "login": "user2" } ], "totalCount": 1 } + } + ], + "pageInfo": { + "hasNextPage": false, + "endCursor": "ENDCURSOR" + }, + "totalCount": 2 + } + } } } + `)) + repo := ghrepo.New("OWNER", "REPO") + + want := &IssuesAndTotalCount{ + Issues: []Issue{ + { + Title: "issue1", + Assignees: struct { + Nodes []struct{ Login string } + TotalCount int + }{[]struct{ Login string }{{"user1"}}, 1}, + Labels: struct { + Nodes []struct{ Name string } + TotalCount int + }{[]struct{ Name string }{{"bug"}}, 1}, + }, + { + Title: "issue2", + Assignees: struct { + Nodes []struct{ Login string } + TotalCount int + }{[]struct{ Login string }{{"user2"}}, 1}, + Labels: struct { + Nodes []struct{ Name string } + TotalCount int + }{[]struct{ Name string }{{"enhancement"}}, 1}, + }, + }, + TotalCount: 2, + } + + got, err := IssueList(client, repo, "", nil, "", 0, "", "", "") + if err != nil { + t.Errorf("IssueList() error = %v", err) + return + } + if !reflect.DeepEqual(got, want) { + t.Errorf("IssueList() = %v, want %v", got, want) + } +} From 813fbc9b8da7eca89f931a38b9c4e5b6103b85e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Oct 2020 15:38:21 +0200 Subject: [PATCH 2/3] Ensure that we don't reuse the same deserialization struct over pagination iterations This is to avoid subtle deserialization issues in nested slices. --- api/queries_org.go | 6 ++++-- api/queries_repo.go | 12 ++++++++---- pkg/cmd/release/list/http.go | 3 ++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/api/queries_org.go b/api/queries_org.go index f3744e147..a47963aea 100644 --- a/api/queries_org.go +++ b/api/queries_org.go @@ -9,7 +9,7 @@ import ( // OrganizationProjects fetches all open projects for an organization func OrganizationProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) { - var query struct { + type responseData struct { Organization struct { Projects struct { Nodes []RepoProject @@ -30,6 +30,7 @@ func OrganizationProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, var projects []RepoProject for { + var query responseData err := gql.QueryNamed(context.Background(), "OrganizationProjectList", &query, variables) if err != nil { return nil, err @@ -52,7 +53,7 @@ type OrgTeam struct { // OrganizationTeams fetches all the teams in an organization func OrganizationTeams(client *Client, repo ghrepo.Interface) ([]OrgTeam, error) { - var query struct { + type responseData struct { Organization struct { Teams struct { Nodes []OrgTeam @@ -73,6 +74,7 @@ func OrganizationTeams(client *Client, repo ghrepo.Interface) ([]OrgTeam, error) var teams []OrgTeam for { + var query responseData err := gql.QueryNamed(context.Background(), "OrganizationTeamList", &query, variables) if err != nil { return nil, err diff --git a/api/queries_repo.go b/api/queries_repo.go index fdbd13e92..54194b1bb 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -657,7 +657,7 @@ type RepoProject struct { // RepoProjects fetches all open projects for a repository func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) { - var query struct { + type responseData struct { Repository struct { Projects struct { Nodes []RepoProject @@ -679,6 +679,7 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) var projects []RepoProject for { + var query responseData err := gql.QueryNamed(context.Background(), "RepositoryProjectList", &query, variables) if err != nil { return nil, err @@ -701,7 +702,7 @@ type RepoAssignee struct { // RepoAssignableUsers fetches all the assignable users for a repository func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, error) { - var query struct { + type responseData struct { Repository struct { AssignableUsers struct { Nodes []RepoAssignee @@ -723,6 +724,7 @@ func RepoAssignableUsers(client *Client, repo ghrepo.Interface) ([]RepoAssignee, var users []RepoAssignee for { + var query responseData err := gql.QueryNamed(context.Background(), "RepositoryAssignableUsers", &query, variables) if err != nil { return nil, err @@ -745,7 +747,7 @@ type RepoLabel struct { // RepoLabels fetches all the labels in a repository func RepoLabels(client *Client, repo ghrepo.Interface) ([]RepoLabel, error) { - var query struct { + type responseData struct { Repository struct { Labels struct { Nodes []RepoLabel @@ -767,6 +769,7 @@ func RepoLabels(client *Client, repo ghrepo.Interface) ([]RepoLabel, error) { var labels []RepoLabel for { + var query responseData err := gql.QueryNamed(context.Background(), "RepositoryLabelList", &query, variables) if err != nil { return nil, err @@ -789,7 +792,7 @@ type RepoMilestone struct { // RepoMilestones fetches all open milestones in a repository func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, error) { - var query struct { + type responseData struct { Repository struct { Milestones struct { Nodes []RepoMilestone @@ -811,6 +814,7 @@ func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, err var milestones []RepoMilestone for { + var query responseData err := gql.QueryNamed(context.Background(), "RepositoryMilestoneList", &query, variables) if err != nil { return nil, err diff --git a/pkg/cmd/release/list/http.go b/pkg/cmd/release/list/http.go index 763963ff1..8aae95674 100644 --- a/pkg/cmd/release/list/http.go +++ b/pkg/cmd/release/list/http.go @@ -21,7 +21,7 @@ type Release struct { } func fetchReleases(httpClient *http.Client, repo ghrepo.Interface, limit int) ([]Release, error) { - var query struct { + type responseData struct { Repository struct { Releases struct { Nodes []Release @@ -50,6 +50,7 @@ func fetchReleases(httpClient *http.Client, repo ghrepo.Interface, limit int) ([ var releases []Release loop: for { + var query responseData err := gql.QueryNamed(context.Background(), "RepositoryReleaseList", &query, variables) if err != nil { return nil, err From e270cdb29e171ac9d5441ed616ad669bd5431ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Oct 2020 15:54:42 +0200 Subject: [PATCH 3/3] Simplify test to avoid use of `reflect` --- api/queries_issue_test.go | 63 +++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index 32e757efd..83dee55b7 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -4,9 +4,10 @@ import ( "bytes" "encoding/json" "io/ioutil" - "reflect" "testing" + "github.com/stretchr/testify/assert" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" ) @@ -73,6 +74,7 @@ func TestIssueList(t *testing.T) { func TestIssueList_pagination(t *testing.T) { http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) + http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "hasIssuesEnabled": true, @@ -111,42 +113,33 @@ func TestIssueList_pagination(t *testing.T) { } } } } `)) + repo := ghrepo.New("OWNER", "REPO") - - want := &IssuesAndTotalCount{ - Issues: []Issue{ - { - Title: "issue1", - Assignees: struct { - Nodes []struct{ Login string } - TotalCount int - }{[]struct{ Login string }{{"user1"}}, 1}, - Labels: struct { - Nodes []struct{ Name string } - TotalCount int - }{[]struct{ Name string }{{"bug"}}, 1}, - }, - { - Title: "issue2", - Assignees: struct { - Nodes []struct{ Login string } - TotalCount int - }{[]struct{ Login string }{{"user2"}}, 1}, - Labels: struct { - Nodes []struct{ Name string } - TotalCount int - }{[]struct{ Name string }{{"enhancement"}}, 1}, - }, - }, - TotalCount: 2, - } - - got, err := IssueList(client, repo, "", nil, "", 0, "", "", "") + res, err := IssueList(client, repo, "", nil, "", 0, "", "", "") if err != nil { - t.Errorf("IssueList() error = %v", err) - return + t.Fatalf("IssueList() error = %v", err) } - if !reflect.DeepEqual(got, want) { - t.Errorf("IssueList() = %v, want %v", got, want) + + assert.Equal(t, 2, res.TotalCount) + assert.Equal(t, 2, len(res.Issues)) + + getLabels := func(i Issue) []string { + var labels []string + for _, l := range i.Labels.Nodes { + labels = append(labels, l.Name) + } + return labels } + getAssignees := func(i Issue) []string { + var logins []string + for _, u := range i.Assignees.Nodes { + logins = append(logins, u.Login) + } + return logins + } + + assert.Equal(t, []string{"bug"}, getLabels(res.Issues[0])) + assert.Equal(t, []string{"user1"}, getAssignees(res.Issues[0])) + assert.Equal(t, []string{"enhancement"}, getLabels(res.Issues[1])) + assert.Equal(t, []string{"user2"}, getAssignees(res.Issues[1])) }