Skip to content

Commit

Permalink
Improve issue close re: overfetching, handling PRs
Browse files Browse the repository at this point in the history
- `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.
  • Loading branch information
mislav committed Nov 23, 2021
1 parent 1eb790c commit 07cad38
Show file tree
Hide file tree
Showing 8 changed files with 103 additions and 44 deletions.
24 changes: 22 additions & 2 deletions api/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
12 changes: 10 additions & 2 deletions api/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
}
]
}
`),
Expand Down
30 changes: 5 additions & 25 deletions api/queries_issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type IssuesAndTotalCount struct {
}

type Issue struct {
Typename string `json:"__typename"`
ID string
Number int
Title string
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 4 additions & 6 deletions api/queries_pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
32 changes: 29 additions & 3 deletions pkg/cmd/issue/close/close.go
Original file line number Diff line number Diff line change
@@ -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"
)

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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)
}
21 changes: 18 additions & 3 deletions pkg/cmd/issue/close/close_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
14 changes: 13 additions & 1 deletion pkg/cmd/issue/shared/lookup.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package shared

import (
"errors"
"fmt"
"net/http"
"net/url"
Expand Down Expand Up @@ -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}
}
Expand All @@ -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
}
4 changes: 2 additions & 2 deletions pkg/cmd/pr/close/close.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -90,6 +89,7 @@ func closeRun(opts *CloseOptions) error {

if opts.DeleteBranch {
branchSwitchString := ""
apiClient := api.NewClientFromHTTP(httpClient)

if opts.DeleteLocalBranch {
currentBranch, err := opts.Branch()
Expand Down

0 comments on commit 07cad38

Please sign in to comment.