STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Improve issue view re: overfetching, PR support

Browse source 
- Supports passing a PR as argument, not just issues
- Makes it non-fatal when project cards were not able to load
- Cleans up legacy method for fetching issues
This commit is contained in:
Mislav Marohnić 2021-11-23 23:07:19 +01:00
parent 3cf3d6d2d4
commit 34fc5fb75c
11 changed files with 240 additions and 183 deletions
Download patch file Download diff file Expand all files Collapse all files

26
api/client.go
View file

@ -146,17 +146,31 @@ type GraphQLErrorResponse struct {
func (gr GraphQLErrorResponse) Error() string {
errorMessages := make([]string, 0, len(gr.Errors))
for _, e := range gr.Errors {
errorMessages = append(errorMessages, e.Message)
msg := e.Message
if p := e.PathString(); p != "" {
msg = fmt.Sprintf("%s (%s)", msg, p)
}
errorMessages = append(errorMessages, msg)
}
return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n"))
return fmt.Sprintf("GraphQL: %s", strings.Join(errorMessages, ", "))
}
// Match checks if this error is only about a specific type on a specific path.
// Match checks if this error is only about a specific type on a specific path. If the path argument ends
// with a ".", it will match all its subpaths as well.
func (gr GraphQLErrorResponse) Match(expectType, expectPath string) bool {
if len(gr.Errors) != 1 {
return false
for _, e := range gr.Errors {
if e.Type != expectType || !matchPath(e.PathString(), expectPath) {
return false
}
}
return gr.Errors[0].Type == expectType && gr.Errors[0].PathString() == expectPath
return true
}
func matchPath(p, expect string) bool {
if strings.HasSuffix(expect, ".") {
return strings.HasPrefix(p, expect) || p == strings.TrimSuffix(expect, ".")
}
return p == expect
}
// HTTPError is an error returned by a failed API call

2
api/client_test.go
View file

@ -66,7 +66,7 @@ func TestGraphQLError(t *testing.T) {
)
err := client.GraphQL("github.com", "", nil, &response)
if err == nil || err.Error() != "GraphQL error: OH NO\nthis is fine" {
if err == nil || err.Error() != "GraphQL: OH NO (repository.issue), this is fine (repository.issues.0.comments)" {
t.Fatalf("got %q", err.Error())
}
}

125
api/queries_issue.go
View file

@ -71,17 +71,19 @@ func (l Labels) Names() []string {
}
type ProjectCards struct {
Nodes []struct {
Project struct {
Name string `json:"name"`
} `json:"project"`
Column struct {
Name string `json:"name"`
} `json:"column"`
}
Nodes []*ProjectInfo
TotalCount int
}
type ProjectInfo struct {
Project struct {
Name string `json:"name"`
} `json:"project"`
Column struct {
Name string `json:"name"`
} `json:"column"`
}
func (p ProjectCards) ProjectNames() []string {
names := make([]string, len(p.Nodes))
for i, c := range p.Nodes {
@ -233,113 +235,6 @@ func IssueStatus(client *Client, repo ghrepo.Interface, options IssueStatusOptio
return &payload, nil
}
func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, error) {
type response struct {
Repository struct {
Issue Issue
HasIssuesEnabled bool
}
}
query := `
query IssueByNumber($owner: String!, $repo: String!, $issue_number: Int!) {
repository(owner: $owner, name: $repo) {
hasIssuesEnabled
issue(number: $issue_number) {
id
title
state
body
author {
login
}
comments(last: 1) {
nodes {
author {
login
}
authorAssociation
body
createdAt
includesCreatedEdit
isMinimized
minimizedReason
reactionGroups {
content
users {
totalCount
}
}
}
totalCount
}
number
url
createdAt
assignees(first: 100) {
nodes {
id
name
login
}
totalCount
}
labels(first: 100) {
nodes {
id
name
description
color
}
totalCount
}
projectCards(first: 100) {
nodes {
project {
name
}
column {
name
}
}
totalCount
}
milestone {
number
title
description
dueOn
}
reactionGroups {
content
users {
totalCount
}
}
}
}
}`
variables := map[string]interface{}{
"owner": repo.RepoOwner(),
"repo": repo.RepoName(),
"issue_number": number,
}
var resp response
err := client.GraphQL(repo.RepoHost(), query, variables, &resp)
if err != nil {
return nil, err
}
if !resp.Repository.HasIssuesEnabled {
return nil, &IssuesDisabledError{fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))}
}
return &resp.Repository.Issue, nil
}
func (i Issue) Link() string {
return i.URL
}

2
api/queries_repo_test.go
View file

@ -23,7 +23,7 @@ func TestGitHubRepo_notFound(t *testing.T) {
if err == nil {
t.Fatal("GitHubRepo did not return an error")
}
if wants := "GraphQL error: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
if wants := "GraphQL: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
t.Errorf("GitHubRepo error: want %q, got %q", wants, err.Error())
}
if repo != nil {

18
api/query_builder.go
View file

@ -35,6 +35,22 @@ var issueComments = shortenQuery(`
}
`)
var issueCommentLast = shortenQuery(`
comments(last: 1) {
nodes {
author{login},
authorAssociation,
body,
createdAt,
includesCreatedEdit,
isMinimized,
minimizedReason,
reactionGroups{content,users{totalCount}}
},
totalCount
}
`)
var prReviewRequests = shortenQuery(`
reviewRequests(first: 100) {
nodes {
@ -206,6 +222,8 @@ func PullRequestGraphQL(fields []string) string {
q = append(q, `potentialMergeCommit{oid}`)
case "comments":
q = append(q, issueComments)
case "lastComment": // pseudo-field
q = append(q, issueCommentLast)
case "reviewRequests":
q = append(q, prReviewRequests)
case "reviews":

2
pkg/cmd/issue/delete/delete_test.go
View file

@ -133,7 +133,7 @@ func TestIssueDelete_doesNotExist(t *testing.T) {
)
_, err := runCommand(httpRegistry, true, "13")
if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 13." {
if err == nil || err.Error() != "GraphQL: Could not resolve to an Issue with the number of 13." {
t.Errorf("error running command `issue delete`: %v", err)
}
}

49
pkg/cmd/issue/shared/lookup.go
View file

@ -13,32 +13,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)
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 := api.IssueByNumber(apiClient, baseRepo, issueNumber)
return issue, baseRepo, err
}
// IssueFromArgWithFields loads an issue or pull request with the specified fields.
// IssueFromArgWithFields loads an issue or pull request with the specified fields. If some of the fields
// could not be fetched by GraphQL, this returns a non-nil issue and a *PartialLoadError.
func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.Interface, error), arg string, fields []string) (*api.Issue, ghrepo.Interface, error) {
issueNumber, baseRepo := issueMetadataFromURL(arg)
@ -84,6 +60,10 @@ func issueMetadataFromURL(s string) (int, ghrepo.Interface) {
return issueNumber, repo
}
type PartialLoadError struct {
error
}
func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
type response struct {
Repository struct {
@ -114,8 +94,21 @@ func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, f
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))
if errors.As(err, &gerr) {
if gerr.Match("NOT_FOUND", "repository.issue") && !resp.Repository.HasIssuesEnabled {
return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo))
} else if gerr.Match("FORBIDDEN", "repository.issue.projectCards.") {
issue := resp.Repository.Issue
// remove nil entries for project cards due to permission issues
projects := make([]*api.ProjectInfo, 0, len(issue.ProjectCards.Nodes))
for _, p := range issue.ProjectCards.Nodes {
if p != nil {
projects = append(projects, p)
}
}
issue.ProjectCards.Nodes = projects
return issue, &PartialLoadError{err}
}
}
return nil, err
}

117
pkg/cmd/issue/shared/lookup_test.go
View file

@ -2,25 +2,26 @@ package shared
import (
"net/http"
"strings"
"testing"
"github.com/cli/cli/v2/api"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/httpmock"
)
func TestIssueFromArg(t *testing.T) {
func TestIssueFromArgWithFields(t *testing.T) {
type args struct {
baseRepoFn func() (ghrepo.Interface, error)
selector string
}
tests := []struct {
name string
args args
httpStub func(*httpmock.Registry)
wantIssue int
wantRepo string
wantErr bool
name string
args args
httpStub func(*httpmock.Registry)
wantIssue int
wantRepo string
wantProjects string
wantErr bool
}{
{
name: "number argument",
@ -77,6 +78,95 @@ func TestIssueFromArg(t *testing.T) {
wantIssue: 13,
wantRepo: "https://example.org/OWNER/REPO",
},
{
name: "project cards permission issue",
args: args{
selector: "https://example.org/OWNER/REPO/issues/13",
baseRepoFn: nil,
},
httpStub: func(r *httpmock.Registry) {
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`
{
"data": {
"repository": {
"hasIssuesEnabled": true,
"issue": {
"number": 13,
"projectCards": {
"nodes": [
null,
{
"project": {"name": "myproject"},
"column": {"name": "To Do"}
},
null,
{
"project": {"name": "other project"},
"column": null
}
]
}
}
}
},
"errors": [
{
"type": "FORBIDDEN",
"message": "Resource not accessible by integration",
"path": ["repository", "issue", "projectCards", "nodes", 0]
},
{
"type": "FORBIDDEN",
"message": "Resource not accessible by integration",
"path": ["repository", "issue", "projectCards", "nodes", 2]
}
]
}`))
},
wantErr: true,
wantIssue: 13,
wantProjects: "myproject, other project",
wantRepo: "https://example.org/OWNER/REPO",
},
{
name: "projects permission issue",
args: args{
selector: "https://example.org/OWNER/REPO/issues/13",
baseRepoFn: nil,
},
httpStub: func(r *httpmock.Registry) {
r.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`
{
"data": {
"repository": {
"hasIssuesEnabled": true,
"issue": {
"number": 13,
"projectCards": {
"nodes": null,
"totalCount": 0
}
}
}
},
"errors": [
{
"type": "FORBIDDEN",
"message": "Resource not accessible by integration",
"path": ["repository", "issue", "projectCards", "nodes"]
}
]
}`))
},
wantErr: true,
wantIssue: 13,
wantProjects: "",
wantRepo: "https://example.org/OWNER/REPO",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -85,14 +175,19 @@ func TestIssueFromArg(t *testing.T) {
tt.httpStub(reg)
}
httpClient := &http.Client{Transport: reg}
issue, repo, err := IssueFromArg(api.NewClientFromHTTP(httpClient), tt.args.baseRepoFn, tt.args.selector)
issue, repo, err := IssueFromArgWithFields(httpClient, tt.args.baseRepoFn, tt.args.selector, []string{"number"})
if (err != nil) != tt.wantErr {
t.Errorf("IssueFromArg() error = %v, wantErr %v", err, tt.wantErr)
return
t.Errorf("IssueFromArgWithFields() error = %v, wantErr %v", err, tt.wantErr)
if issue == nil {
return
}
}
if issue.Number != tt.wantIssue {
t.Errorf("want issue #%d, got #%d", tt.wantIssue, issue.Number)
}
if gotProjects := strings.Join(issue.ProjectCards.ProjectNames(), ", "); gotProjects != tt.wantProjects {
t.Errorf("want projects %q, got %q", tt.wantProjects, gotProjects)
}
repoURL := ghrepo.GenerateRepoURL(repo, "")
if repoURL != tt.wantRepo {
t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL)

16
pkg/cmd/issue/view/http.go
View file

@ -15,8 +15,11 @@ func preloadIssueComments(client *http.Client, repo ghrepo.Interface, issue *api
type response struct {
Node struct {
Issue struct {
Comments api.Comments `graphql:"comments(first: 100, after: $endCursor)"`
Comments *api.Comments `graphql:"comments(first: 100, after: $endCursor)"`
} `graphql:"...on Issue"`
PullRequest struct {
Comments *api.Comments `graphql:"comments(first: 100, after: $endCursor)"`
} `graphql:"...on PullRequest"`
} `graphql:"node(id: $id)"`
}
@ -38,11 +41,16 @@ func preloadIssueComments(client *http.Client, repo ghrepo.Interface, issue *api
return err
}
issue.Comments.Nodes = append(issue.Comments.Nodes, query.Node.Issue.Comments.Nodes...)
if !query.Node.Issue.Comments.PageInfo.HasNextPage {
comments := query.Node.Issue.Comments
if comments == nil {
comments = query.Node.PullRequest.Comments
}
issue.Comments.Nodes = append(issue.Comments.Nodes, comments.Nodes...)
if !comments.PageInfo.HasNextPage {
break
}
variables["endCursor"] = githubv4.String(query.Node.Issue.Comments.PageInfo.EndCursor)
variables["endCursor"] = githubv4.String(comments.PageInfo.EndCursor)
}
issue.Comments.PageInfo.HasNextPage = false

42
pkg/cmd/issue/view/view.go
View file

@ -1,6 +1,7 @@
package view
import (
"errors"
"fmt"
"io"
"net/http"
@ -77,24 +78,40 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
return cmd
}
var defaultFields = []string{
"number", "url", "state", "createdAt", "title", "body", "author", "milestone",
"assignees", "labels", "projectCards", "reactionGroups", "lastComment",
}
func viewRun(opts *ViewOptions) error {
httpClient, err := opts.HttpClient()
if err != nil {
return err
}
loadComments := opts.Comments
if !loadComments && opts.Exporter != nil {
fields := set.NewStringSet()
fields.AddValues(opts.Exporter.Fields())
loadComments = fields.Contains("comments")
lookupFields := set.NewStringSet()
if opts.Exporter != nil {
lookupFields.AddValues(opts.Exporter.Fields())
} else if opts.WebMode {
lookupFields.Add("url")
} else {
lookupFields.AddValues(defaultFields)
}
if opts.Comments {
lookupFields.Add("comments")
lookupFields.Remove("lastComment")
}
opts.IO.StartProgressIndicator()
issue, err := findIssue(httpClient, opts.BaseRepo, opts.SelectorArg, loadComments)
issue, err := findIssue(httpClient, opts.BaseRepo, opts.SelectorArg, lookupFields.ToSlice())
opts.IO.StopProgressIndicator()
if err != nil {
return err
var loadErr *issueShared.PartialLoadError
if opts.Exporter == nil && errors.As(err, &loadErr) {
fmt.Fprintf(opts.IO.ErrOut, "warning: %s\n", loadErr.Error())
} else {
return err
}
}
if opts.WebMode {
@ -127,14 +144,17 @@ func viewRun(opts *ViewOptions) error {
return printRawIssuePreview(opts.IO.Out, issue)
}
func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), selector string, loadComments bool) (*api.Issue, error) {
apiClient := api.NewClientFromHTTP(client)
issue, repo, err := issueShared.IssueFromArg(apiClient, baseRepoFn, selector)
func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), selector string, fields []string) (*api.Issue, error) {
fieldSet := set.NewStringSet()
fieldSet.AddValues(fields)
fieldSet.Add("id")
issue, repo, err := issueShared.IssueFromArgWithFields(client, baseRepoFn, selector, fieldSet.ToSlice())
if err != nil {
return issue, err
}
if loadComments {
if fieldSet.Contains("comments") {
err = preloadIssueComments(client, repo, issue)
}
return issue, err

24
pkg/cmd/issue/view/view_test.go
View file

@ -276,7 +276,7 @@ func TestIssueView_web_notFound(t *testing.T) {
defer cmdTeardown(t)
_, err := runCommand(http, true, "-w 9999")
if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 9999." {
if err == nil || err.Error() != "GraphQL: Could not resolve to an Issue with the number of 9999." {
t.Errorf("error running command `issue view`: %v", err)
}
}
@ -288,10 +288,24 @@ func TestIssueView_disabledIssues(t *testing.T) {
http.Register(
httpmock.GraphQL(`query IssueByNumber\b`),
httpmock.StringResponse(`
{ "data": { "repository": {
"id": "REPOID",
"hasIssuesEnabled": false
} } }
{
"data":
{ "repository": {
"id": "REPOID",
"hasIssuesEnabled": false
}
},
"errors": [
{
"type": "NOT_FOUND",
"path": [
"repository",
"issue"
],
"message": "Could not resolve to an issue or pull request with the number of 6666."
}
]
}
`),
)