From 1eb790cacd67a7860e248688c6f7e4773f3510d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Nov 2021 16:40:14 +0100 Subject: [PATCH] Improve `issue comment` re: overfetching, handling PRs - `issue comment` no longer fetches all issue fields and thus avoids the problem when loading failed due to token not having access to projects - `issue comment` now accepts either issue or pull number as argument. --- api/query_builder.go | 2 + pkg/cmd/issue/comment/comment.go | 30 +++---------- pkg/cmd/issue/comment/comment_test.go | 38 ++++++++--------- pkg/cmd/issue/shared/lookup.go | 61 +++++++++++++++++++++++++-- 4 files changed, 84 insertions(+), 47 deletions(-) diff --git a/api/query_builder.go b/api/query_builder.go index c9ab62d13..7dc9a423a 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -176,6 +176,8 @@ var PullRequestFields = append(IssueFields, "statusCheckRollup", ) +// PullRequestGraphQL constructs a GraphQL query fragment for a set of pull request fields. Since GitHub +// pull requests are also technically issues, this function can be used to query issues as well. func PullRequestGraphQL(fields []string) string { var q []string for _, field := range fields { diff --git a/pkg/cmd/issue/comment/comment.go b/pkg/cmd/issue/comment/comment.go index 3e73a1d79..41c76eeea 100644 --- a/pkg/cmd/issue/comment/comment.go +++ b/pkg/cmd/issue/comment/comment.go @@ -1,10 +1,7 @@ package comment import ( - "net/http" - "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" issueShared "github.com/cli/cli/v2/pkg/cmd/issue/shared" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" @@ -32,7 +29,13 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*prShared.CommentableOptions) e `), Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - opts.RetrieveCommentable = retrieveIssue(f.HttpClient, f.BaseRepo, args[0]) + opts.RetrieveCommentable = func() (prShared.Commentable, ghrepo.Interface, error) { + httpClient, err := f.HttpClient() + if err != nil { + return nil, nil, err + } + return issueShared.IssueFromArgWithFields(httpClient, f.BaseRepo, args[0], []string{"id", "url"}) + } return prShared.CommentablePreRun(cmd, opts) }, RunE: func(_ *cobra.Command, args []string) error { @@ -58,22 +61,3 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*prShared.CommentableOptions) e return cmd } - -func retrieveIssue(httpClient func() (*http.Client, error), - baseRepo func() (ghrepo.Interface, error), - selector string) func() (prShared.Commentable, ghrepo.Interface, error) { - return func() (prShared.Commentable, ghrepo.Interface, error) { - httpClient, err := httpClient() - if err != nil { - return nil, nil, err - } - apiClient := api.NewClientFromHTTP(httpClient) - - issue, repo, err := issueShared.IssueFromArg(apiClient, baseRepo, selector) - if err != nil { - return nil, nil, err - } - - return issue, repo, nil - } -} diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go index 066130024..a82a0e405 100644 --- a/pkg/cmd/issue/comment/comment_test.go +++ b/pkg/cmd/issue/comment/comment_test.go @@ -203,7 +203,6 @@ func Test_commentRun(t *testing.T) { ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockIssueFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", @@ -217,9 +216,6 @@ func Test_commentRun(t *testing.T) { OpenInBrowser: func(string) error { return nil }, }, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockIssueFromNumber(t, reg) - }, stderr: "Opening github.com/OWNER/REPO/issues/123 in your browser.\n", }, { @@ -232,7 +228,6 @@ func Test_commentRun(t *testing.T) { EditSurvey: func() (string, error) { return "comment body", nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockIssueFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", @@ -245,7 +240,6 @@ func Test_commentRun(t *testing.T) { Body: "comment body", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockIssueFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/issues/123#issuecomment-456\n", @@ -259,14 +253,17 @@ func Test_commentRun(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - tt.httpStubs(t, reg) - - httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } + if tt.httpStubs != nil { + tt.httpStubs(t, reg) + } tt.input.IO = io - tt.input.HttpClient = httpClient - tt.input.RetrieveCommentable = retrieveIssue(tt.input.HttpClient, baseRepo, "123") + tt.input.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + tt.input.RetrieveCommentable = func() (shared.Commentable, ghrepo.Interface, error) { + return &mockCommentable{}, ghrepo.New("OWNER", "REPO"), nil + } t.Run(tt.name, func(t *testing.T) { err := shared.CommentableRun(tt.input) @@ -277,15 +274,13 @@ func Test_commentRun(t *testing.T) { } } -func mockIssueFromNumber(_ *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query IssueByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "hasIssuesEnabled": true, "issue": { - "number": 123, - "url": "https://github.com/OWNER/REPO/issues/123" - } } } }`), - ) +type mockCommentable struct{} + +func (c mockCommentable) Identifier() string { + return "ISSUE-ID" +} +func (c mockCommentable) Link() string { + return "https://github.com/OWNER/REPO/issues/123" } func mockCommentCreate(t *testing.T, reg *httpmock.Registry) { @@ -296,6 +291,7 @@ func mockCommentCreate(t *testing.T, reg *httpmock.Registry) { "url": "https://github.com/OWNER/REPO/issues/123#issuecomment-456" } } } } }`, func(inputs map[string]interface{}) { + assert.Equal(t, "ISSUE-ID", inputs["subjectId"]) assert.Equal(t, "comment body", inputs["body"]) }), ) diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index d0c1126da..288ee3c03 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -2,6 +2,7 @@ package shared import ( "fmt" + "net/http" "net/url" "regexp" "strconv" @@ -11,6 +12,8 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" ) +// IssueFromArg loads an issue with all its fields. +// Deprecated: use IssueFromArgWithFields instead. func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), arg string) (*api.Issue, ghrepo.Interface, error) { issueNumber, baseRepo := issueMetadataFromURL(arg) @@ -30,7 +33,31 @@ func IssueFromArg(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, er } } - issue, err := issueFromNumber(apiClient, baseRepo, issueNumber) + issue, err := api.IssueByNumber(apiClient, baseRepo, issueNumber) + return issue, baseRepo, err +} + +// IssueFromArgWithFields loads an issue or pull request with the specified fields. +func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, fields []string) (*api.Issue, ghrepo.Interface, error) { + issueNumber, baseRepo := issueMetadataFromURL(arg) + + if issueNumber == 0 { + var err error + issueNumber, err = strconv.Atoi(strings.TrimPrefix(arg, "#")) + if err != nil { + return nil, nil, fmt.Errorf("invalid issue format: %q", arg) + } + } + + if baseRepo == nil { + var err error + baseRepo, err = baseRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + } + + issue, err := findIssueOrPR(httpClient, baseRepo, issueNumber, fields) return issue, baseRepo, err } @@ -56,6 +83,34 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) { return issueNumber, repo } -func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber int) (*api.Issue, error) { - return api.IssueByNumber(apiClient, repo, issueNumber) +func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { + type response struct { + Repository struct { + Issue *api.Issue + } + } + + query := fmt.Sprintf(` + query IssueByNumber($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + issue: issueOrPullRequest(number: $number) { + ...on Issue{%[1]s} + ...on PullRequest{%[1]s} + } + } + }`, api.PullRequestGraphQL(fields)) + + variables := map[string]interface{}{ + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), + "number": number, + } + + var resp response + client := api.NewClientFromHTTP(httpClient) + if err := client.GraphQL(repo.RepoHost(), query, variables, &resp); err != nil { + return nil, err + } + + return resp.Repository.Issue, nil }