X Tutup
Skip to content

Commit 07cad38

Browse files
mislavmislav
committed
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.
1 parent 1eb790c commit 07cad38

File tree

8 files changed

+103
-44
lines changed

8 files changed

+103
-44
lines changed

‎api/client.go‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,18 @@ type graphQLResponse struct {
124124
type GraphQLError struct {
125125
Type string
126126
Message string
127-
// Path []interface // mixed strings and numbers
127+
Path []interface{} // mixed strings and numbers
128+
}
129+
130+
func (ge GraphQLError) PathString() string {
131+
var res strings.Builder
132+
for i, v := range ge.Path {
133+
if i > 0 {
134+
res.WriteRune('.')
135+
}
136+
fmt.Fprintf(&res, "%v", v)
137+
}
138+
return res.String()
128139
}
129140

130141
// GraphQLErrorResponse contains errors returned in a GraphQL response
@@ -140,6 +151,14 @@ func (gr GraphQLErrorResponse) Error() string {
140151
return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n"))
141152
}
142153

154+
// Match checks if this error is only about a specific type on a specific path.
155+
func (gr GraphQLErrorResponse) Match(expectType, expectPath string) bool {
156+
if len(gr.Errors) != 1 {
157+
return false
158+
}
159+
return gr.Errors[0].Type == expectType && gr.Errors[0].PathString() == expectPath
160+
}
161+
143162
// HTTPError is an error returned by a failed API call
144163
type HTTPError struct {
145164
StatusCode int
@@ -221,7 +240,8 @@ func EndpointNeedsScopes(resp *http.Response, s string) *http.Response {
221240
return resp
222241
}
223242

224-
// GraphQL performs a GraphQL request and parses the response
243+
// GraphQL performs a GraphQL request and parses the response. If there are errors in the response,
244+
// *GraphQLErrorResponse will be returned, but the data will also be parsed into the receiver.
225245
func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error {
226246
reqBody, err := json.Marshal(map[string]interface{}{"query": query, "variables": variables})
227247
if err != nil {

‎api/client_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,16 @@ func TestGraphQLError(t *testing.T) {
5050
httpmock.GraphQL(""),
5151
httpmock.StringResponse(`
5252
{ "errors": [
53-
{"message":"OH NO"},
54-
{"message":"this is fine"}
53+
{
54+
"type": "NOT_FOUND",
55+
"message": "OH NO",
56+
"path": ["repository", "issue"]
57+
},
58+
{
59+
"type": "ACTUALLY_ITS_FINE",
60+
"message": "this is fine",
61+
"path": ["repository", "issues", 0, "comments"]
62+
}
5563
]
5664
}
5765
`),

‎api/queries_issue.go‎

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ type IssuesAndTotalCount struct {
2222
}
2323

2424
type Issue struct {
25+
Typename string `json:"__typename"`
2526
ID string
2627
Number int
2728
Title string
@@ -41,6 +42,10 @@ type Issue struct {
4142
ReactionGroups ReactionGroups
4243
}
4344

45+
func (i Issue) IsPullRequest() bool {
46+
return i.Typename == "PullRequest"
47+
}
48+
4449
type Assignees struct {
4550
Nodes []GitHubUser
4651
TotalCount int
@@ -337,31 +342,6 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e
337342
return &resp.Repository.Issue, nil
338343
}
339344

340-
func IssueClose(client *Client, repo ghrepo.Interface, issue Issue) error {
341-
var mutation struct {
342-
CloseIssue struct {
343-
Issue struct {
344-
ID githubv4.ID
345-
}
346-
} `graphql:"closeIssue(input: $input)"`
347-
}
348-
349-
variables := map[string]interface{}{
350-
"input": githubv4.CloseIssueInput{
351-
IssueID: issue.ID,
352-
},
353-
}
354-
355-
gql := graphQLClient(client.http, repo.RepoHost())
356-
err := gql.MutateNamed(context.Background(), "IssueClose", &mutation, variables)
357-
358-
if err != nil {
359-
return err
360-
}
361-
362-
return nil
363-
}
364-
365345
func IssueReopen(client *Client, repo ghrepo.Interface, issue Issue) error {
366346
var mutation struct {
367347
ReopenIssue struct {

‎api/queries_pr.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,7 @@ func isBlank(v interface{}) bool {
660660
}
661661
}
662662

663-
func PullRequestClose(client *Client, repo ghrepo.Interface, pr *PullRequest) error {
663+
func PullRequestClose(httpClient *http.Client, repo ghrepo.Interface, prID string) error {
664664
var mutation struct {
665665
ClosePullRequest struct {
666666
PullRequest struct {
@@ -671,14 +671,12 @@ func PullRequestClose(client *Client, repo ghrepo.Interface, pr *PullRequest) er
671671

672672
variables := map[string]interface{}{
673673
"input": githubv4.ClosePullRequestInput{
674-
PullRequestID: pr.ID,
674+
PullRequestID: prID,
675675
},
676676
}
677677

678-
gql := graphQLClient(client.http, repo.RepoHost())
679-
err := gql.MutateNamed(context.Background(), "PullRequestClose", &mutation, variables)
680-
681-
return err
678+
gql := graphQLClient(httpClient, repo.RepoHost())
679+
return gql.MutateNamed(context.Background(), "PullRequestClose", &mutation, variables)
682680
}
683681

684682
func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) error {

‎pkg/cmd/issue/close/close.go‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
11
package close
22

33
import (
4+
"context"
45
"fmt"
56
"net/http"
67

78
"github.com/cli/cli/v2/api"
89
"github.com/cli/cli/v2/internal/config"
10+
"github.com/cli/cli/v2/internal/ghinstance"
911
"github.com/cli/cli/v2/internal/ghrepo"
1012
"github.com/cli/cli/v2/pkg/cmd/issue/shared"
1113
"github.com/cli/cli/v2/pkg/cmdutil"
1214
"github.com/cli/cli/v2/pkg/iostreams"
15+
graphql "github.com/cli/shurcooL-graphql"
16+
"github.com/shurcooL/githubv4"
1317
"github.com/spf13/cobra"
1418
)
1519

@@ -58,9 +62,8 @@ func closeRun(opts *CloseOptions) error {
5862
if err != nil {
5963
return err
6064
}
61-
apiClient := api.NewClientFromHTTP(httpClient)
6265

63-
issue, baseRepo, err := shared.IssueFromArg(apiClient, opts.BaseRepo, opts.SelectorArg)
66+
issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"})
6467
if err != nil {
6568
return err
6669
}
@@ -70,7 +73,7 @@ func closeRun(opts *CloseOptions) error {
7073
return nil
7174
}
7275

73-
err = api.IssueClose(apiClient, baseRepo, *issue)
76+
err = apiClose(httpClient, baseRepo, issue)
7477
if err != nil {
7578
return err
7679
}
@@ -79,3 +82,26 @@ func closeRun(opts *CloseOptions) error {
7982

8083
return nil
8184
}
85+
86+
func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue) error {
87+
if issue.IsPullRequest() {
88+
return api.PullRequestClose(httpClient, repo, issue.ID)
89+
}
90+
91+
var mutation struct {
92+
CloseIssue struct {
93+
Issue struct {
94+
ID githubv4.ID
95+
}
96+
} `graphql:"closeIssue(input: $input)"`
97+
}
98+
99+
variables := map[string]interface{}{
100+
"input": githubv4.CloseIssueInput{
101+
IssueID: issue.ID,
102+
},
103+
}
104+
105+
gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), httpClient)
106+
return gql.MutateNamed(context.Background(), "IssueClose", &mutation, variables)
107+
}

‎pkg/cmd/issue/close/close_test.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,24 @@ func TestIssueClose_issuesDisabled(t *testing.T) {
119119
http.Register(
120120
httpmock.GraphQL(`query IssueByNumber\b`),
121121
httpmock.StringResponse(`
122-
{ "data": { "repository": {
123-
"hasIssuesEnabled": false
124-
} } }`),
122+
{
123+
"data": {
124+
"repository": {
125+
"hasIssuesEnabled": false,
126+
"issue": null
127+
}
128+
},
129+
"errors": [
130+
{
131+
"type": "NOT_FOUND",
132+
"path": [
133+
"repository",
134+
"issue"
135+
],
136+
"message": "Could not resolve to an issue or pull request with the number of 13."
137+
}
138+
]
139+
}`),
125140
)
126141

127142
_, err := runCommand(http, true, "13")

‎pkg/cmd/issue/shared/lookup.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package shared
22

33
import (
4+
"errors"
45
"fmt"
56
"net/http"
67
"net/url"
@@ -86,14 +87,17 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) {
8687
func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
8788
type response struct {
8889
Repository struct {
89-
Issue *api.Issue
90+
HasIssuesEnabled bool
91+
Issue *api.Issue
9092
}
9193
}
9294

9395
query := fmt.Sprintf(`
9496
query IssueByNumber($owner: String!, $repo: String!, $number: Int!) {
9597
repository(owner: $owner, name: $repo) {
98+
hasIssuesEnabled
9699
issue: issueOrPullRequest(number: $number) {
100+
__typename
97101
...on Issue{%[1]s}
98102
...on PullRequest{%[1]s}
99103
}
@@ -109,8 +113,16 @@ func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, f
109113
var resp response
110114
client := api.NewClientFromHTTP(httpClient)
111115
if err := client.GraphQL(repo.RepoHost(), query, variables, &resp); err != nil {
116+
var gerr *api.GraphQLErrorResponse
117+
if errors.As(err, &gerr) && gerr.Match("NOT_FOUND", "repository.issue") && !resp.Repository.HasIssuesEnabled {
118+
return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))
119+
}
112120
return nil, err
113121
}
114122

123+
if resp.Repository.Issue == nil {
124+
return nil, errors.New("issue was not found but GraphQL reported no error")
125+
}
126+
115127
return resp.Repository.Issue, nil
116128
}

‎pkg/cmd/pr/close/close.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,8 @@ func closeRun(opts *CloseOptions) error {
7979
if err != nil {
8080
return err
8181
}
82-
apiClient := api.NewClientFromHTTP(httpClient)
8382

84-
err = api.PullRequestClose(apiClient, baseRepo, pr)
83+
err = api.PullRequestClose(httpClient, baseRepo, pr.ID)
8584
if err != nil {
8685
return fmt.Errorf("API call failed: %w", err)
8786
}
@@ -90,6 +89,7 @@ func closeRun(opts *CloseOptions) error {
9089

9190
if opts.DeleteBranch {
9291
branchSwitchString := ""
92+
apiClient := api.NewClientFromHTTP(httpClient)
9393

9494
if opts.DeleteLocalBranch {
9595
currentBranch, err := opts.Branch()

0 commit comments

Comments
 (0)
X Tutup