From 07cad386a559ebc7b268dc2cd196a925bcdaee78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Nov 2021 17:18:10 +0100 Subject: [PATCH] Improve `issue close` re: overfetching, handling PRs - `issue close` no longer fetches all issue fields and thus avoids the problem when loading failed due to token not having access to projects - `issue close` now accepts either issue or pull number as argument. --- api/client.go | 24 +++++++++++++++++++++-- api/client_test.go | 12 ++++++++++-- api/queries_issue.go | 30 +++++------------------------ api/queries_pr.go | 10 ++++------ pkg/cmd/issue/close/close.go | 32 ++++++++++++++++++++++++++++--- pkg/cmd/issue/close/close_test.go | 21 +++++++++++++++++--- pkg/cmd/issue/shared/lookup.go | 14 +++++++++++++- pkg/cmd/pr/close/close.go | 4 ++-- 8 files changed, 103 insertions(+), 44 deletions(-) diff --git a/api/client.go b/api/client.go index d6eb3a81b93..94f396e6ac4 100644 --- a/api/client.go +++ b/api/client.go @@ -124,7 +124,18 @@ type graphQLResponse struct { type GraphQLError struct { Type string Message string - // Path []interface // mixed strings and numbers + Path []interface{} // mixed strings and numbers +} + +func (ge GraphQLError) PathString() string { + var res strings.Builder + for i, v := range ge.Path { + if i > 0 { + res.WriteRune('.') + } + fmt.Fprintf(&res, "%v", v) + } + return res.String() } // GraphQLErrorResponse contains errors returned in a GraphQL response @@ -140,6 +151,14 @@ func (gr GraphQLErrorResponse) Error() string { return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n")) } +// Match checks if this error is only about a specific type on a specific path. +func (gr GraphQLErrorResponse) Match(expectType, expectPath string) bool { + if len(gr.Errors) != 1 { + return false + } + return gr.Errors[0].Type == expectType && gr.Errors[0].PathString() == expectPath +} + // HTTPError is an error returned by a failed API call type HTTPError struct { StatusCode int @@ -221,7 +240,8 @@ func EndpointNeedsScopes(resp *http.Response, s string) *http.Response { return resp } -// GraphQL performs a GraphQL request and parses the response +// GraphQL performs a GraphQL request and parses the response. If there are errors in the response, +// *GraphQLErrorResponse will be returned, but the data will also be parsed into the receiver. func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { reqBody, err := json.Marshal(map[string]interface{}{"query": query, "variables": variables}) if err != nil { diff --git a/api/client_test.go b/api/client_test.go index c7848d24250..63d21b776d7 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -50,8 +50,16 @@ func TestGraphQLError(t *testing.T) { httpmock.GraphQL(""), httpmock.StringResponse(` { "errors": [ - {"message":"OH NO"}, - {"message":"this is fine"} + { + "type": "NOT_FOUND", + "message": "OH NO", + "path": ["repository", "issue"] + }, + { + "type": "ACTUALLY_ITS_FINE", + "message": "this is fine", + "path": ["repository", "issues", 0, "comments"] + } ] } `), diff --git a/api/queries_issue.go b/api/queries_issue.go index d09497569e6..66c09ff0ea0 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -22,6 +22,7 @@ type IssuesAndTotalCount struct { } type Issue struct { + Typename string `json:"__typename"` ID string Number int Title string @@ -41,6 +42,10 @@ type Issue struct { ReactionGroups ReactionGroups } +func (i Issue) IsPullRequest() bool { + return i.Typename == "PullRequest" +} + type Assignees struct { Nodes []GitHubUser TotalCount int @@ -337,31 +342,6 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e return &resp.Repository.Issue, nil } -func IssueClose(client *Client, repo ghrepo.Interface, issue Issue) error { - var mutation struct { - CloseIssue struct { - Issue struct { - ID githubv4.ID - } - } `graphql:"closeIssue(input: $input)"` - } - - variables := map[string]interface{}{ - "input": githubv4.CloseIssueInput{ - IssueID: issue.ID, - }, - } - - gql := graphQLClient(client.http, repo.RepoHost()) - err := gql.MutateNamed(context.Background(), "IssueClose", &mutation, variables) - - if err != nil { - return err - } - - return nil -} - func IssueReopen(client *Client, repo ghrepo.Interface, issue Issue) error { var mutation struct { ReopenIssue struct { diff --git a/api/queries_pr.go b/api/queries_pr.go index 744b4c9e291..149724fcbe7 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -660,7 +660,7 @@ func isBlank(v interface{}) bool { } } -func PullRequestClose(client *Client, repo ghrepo.Interface, pr *PullRequest) error { +func PullRequestClose(httpClient *http.Client, repo ghrepo.Interface, prID string) error { var mutation struct { ClosePullRequest struct { PullRequest struct { @@ -671,14 +671,12 @@ func PullRequestClose(client *Client, repo ghrepo.Interface, pr *PullRequest) er variables := map[string]interface{}{ "input": githubv4.ClosePullRequestInput{ - PullRequestID: pr.ID, + PullRequestID: prID, }, } - gql := graphQLClient(client.http, repo.RepoHost()) - err := gql.MutateNamed(context.Background(), "PullRequestClose", &mutation, variables) - - return err + gql := graphQLClient(httpClient, repo.RepoHost()) + return gql.MutateNamed(context.Background(), "PullRequestClose", &mutation, variables) } func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) error { diff --git a/pkg/cmd/issue/close/close.go b/pkg/cmd/issue/close/close.go index 95d56b51d31..eabdc3ab7fa 100644 --- a/pkg/cmd/issue/close/close.go +++ b/pkg/cmd/issue/close/close.go @@ -1,15 +1,19 @@ package close import ( + "context" "fmt" "net/http" "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" "github.com/cli/cli/v2/pkg/cmd/issue/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + graphql "github.com/cli/shurcooL-graphql" + "github.com/shurcooL/githubv4" "github.com/spf13/cobra" ) @@ -58,9 +62,8 @@ func closeRun(opts *CloseOptions) error { if err != nil { return err } - apiClient := api.NewClientFromHTTP(httpClient) - issue, baseRepo, err := shared.IssueFromArg(apiClient, opts.BaseRepo, opts.SelectorArg) + issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"}) if err != nil { return err } @@ -70,7 +73,7 @@ func closeRun(opts *CloseOptions) error { return nil } - err = api.IssueClose(apiClient, baseRepo, *issue) + err = apiClose(httpClient, baseRepo, issue) if err != nil { return err } @@ -79,3 +82,26 @@ func closeRun(opts *CloseOptions) error { return nil } + +func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue) error { + if issue.IsPullRequest() { + return api.PullRequestClose(httpClient, repo, issue.ID) + } + + var mutation struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + } + } `graphql:"closeIssue(input: $input)"` + } + + variables := map[string]interface{}{ + "input": githubv4.CloseIssueInput{ + IssueID: issue.ID, + }, + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), httpClient) + return gql.MutateNamed(context.Background(), "IssueClose", &mutation, variables) +} diff --git a/pkg/cmd/issue/close/close_test.go b/pkg/cmd/issue/close/close_test.go index 80d1e5366f3..54bf79fd0d2 100644 --- a/pkg/cmd/issue/close/close_test.go +++ b/pkg/cmd/issue/close/close_test.go @@ -119,9 +119,24 @@ func TestIssueClose_issuesDisabled(t *testing.T) { http.Register( httpmock.GraphQL(`query IssueByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "hasIssuesEnabled": false - } } }`), + { + "data": { + "repository": { + "hasIssuesEnabled": false, + "issue": null + } + }, + "errors": [ + { + "type": "NOT_FOUND", + "path": [ + "repository", + "issue" + ], + "message": "Could not resolve to an issue or pull request with the number of 13." + } + ] + }`), ) _, err := runCommand(http, true, "13") diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 288ee3c03b1..875a43c49d0 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -1,6 +1,7 @@ package shared import ( + "errors" "fmt" "net/http" "net/url" @@ -86,14 +87,17 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) { func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { type response struct { Repository struct { - Issue *api.Issue + HasIssuesEnabled bool + Issue *api.Issue } } query := fmt.Sprintf(` query IssueByNumber($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { + hasIssuesEnabled issue: issueOrPullRequest(number: $number) { + __typename ...on Issue{%[1]s} ...on PullRequest{%[1]s} } @@ -109,8 +113,16 @@ func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, f var resp response client := api.NewClientFromHTTP(httpClient) if err := client.GraphQL(repo.RepoHost(), query, variables, &resp); err != nil { + var gerr *api.GraphQLErrorResponse + if errors.As(err, &gerr) && gerr.Match("NOT_FOUND", "repository.issue") && !resp.Repository.HasIssuesEnabled { + return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo)) + } return nil, err } + if resp.Repository.Issue == nil { + return nil, errors.New("issue was not found but GraphQL reported no error") + } + return resp.Repository.Issue, nil } diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go index 1e3b1333d3e..fc954b19e60 100644 --- a/pkg/cmd/pr/close/close.go +++ b/pkg/cmd/pr/close/close.go @@ -79,9 +79,8 @@ func closeRun(opts *CloseOptions) error { if err != nil { return err } - apiClient := api.NewClientFromHTTP(httpClient) - err = api.PullRequestClose(apiClient, baseRepo, pr) + err = api.PullRequestClose(httpClient, baseRepo, pr.ID) if err != nil { return fmt.Errorf("API call failed: %w", err) } @@ -90,6 +89,7 @@ func closeRun(opts *CloseOptions) error { if opts.DeleteBranch { branchSwitchString := "" + apiClient := api.NewClientFromHTTP(httpClient) if opts.DeleteLocalBranch { currentBranch, err := opts.Branch()