Skip to content

Commit 69d89a6

Browse files
williammartinwilliammartin
authored
Merge pull request cli#12884 from cli/babakks/use-min-discovery-fields-for-issue-create
fix(issue): avoid fetching unnecessary fields for discovery
2 parents 7ef7d7f + 27fb2da commit 69d89a6

7 files changed

Lines changed: 247 additions & 26 deletions

File tree

‎api/queries_repo.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,47 @@ func FetchRepository(client *Client, repo ghrepo.Interface, fields []string) (*R
314314
return InitRepoHostname(result.Repository, repo.RepoHost()), nil
315315
}
316316

317+
// IssueRepoInfo fetches only the repository fields needed for issue operations such as
318+
// issue creation and transfer, avoiding fields like defaultBranchRef that require additional
319+
// token permissions.
320+
func IssueRepoInfo(client *Client, repo ghrepo.Interface) (*Repository, error) {
321+
query := `
322+
query IssueRepositoryInfo($owner: String!, $name: String!) {
323+
repository(owner: $owner, name: $name) {
324+
id
325+
name
326+
owner { login }
327+
hasIssuesEnabled
328+
viewerPermission
329+
}
330+
}`
331+
variables := map[string]interface{}{
332+
"owner": repo.RepoOwner(),
333+
"name": repo.RepoName(),
334+
}
335+
336+
var result struct {
337+
Repository *Repository
338+
}
339+
if err := client.GraphQL(repo.RepoHost(), query, variables, &result); err != nil {
340+
return nil, err
341+
}
342+
// The GraphQL API should have returned an error in case of a missing repository, but this isn't
343+
// guaranteed to happen when an authentication token with insufficient permissions is being used.
344+
if result.Repository == nil {
345+
return nil, GraphQLError{
346+
GraphQLError: &ghAPI.GraphQLError{
347+
Errors: []ghAPI.GraphQLErrorItem{{
348+
Type: "NOT_FOUND",
349+
Message: fmt.Sprintf("Could not resolve to a Repository with the name '%s/%s'.", repo.RepoOwner(), repo.RepoName()),
350+
}},
351+
},
352+
}
353+
}
354+
355+
return InitRepoHostname(result.Repository, repo.RepoHost()), nil
356+
}
357+
317358
func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
318359
query := `
319360
fragment repo on Repository {

‎api/queries_repo_test.go‎

Lines changed: 174 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,181 @@ func TestGitHubRepo_notFound(t *testing.T) {
2626

2727
client := newTestClient(httpReg)
2828
repo, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO"))
29-
if err == nil {
30-
t.Fatal("GitHubRepo did not return an error")
31-
}
32-
if wants := "GraphQL: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
33-
t.Errorf("GitHubRepo error: want %q, got %q", wants, err.Error())
34-
}
35-
if repo != nil {
36-
t.Errorf("GitHubRepo: expected nil repo, got %v", repo)
29+
require.EqualError(t, err, "GraphQL: Could not resolve to a Repository with the name 'OWNER/REPO'.")
30+
assert.Nil(t, repo)
31+
}
32+
33+
func TestGitHubRepo_success(t *testing.T) {
34+
httpReg := &httpmock.Registry{}
35+
defer httpReg.Verify(t)
36+
37+
httpReg.Register(
38+
httpmock.GraphQL(`query RepositoryInfo\b`),
39+
httpmock.StringResponse(`
40+
{ "data": { "repository": {
41+
"id": "REPOID",
42+
"name": "REPO",
43+
"owner": {"login": "OWNER"},
44+
"hasIssuesEnabled": true,
45+
"description": "a cool repo",
46+
"hasWikiEnabled": true,
47+
"viewerPermission": "ADMIN",
48+
"defaultBranchRef": {"name": "main"},
49+
"parent": null,
50+
"mergeCommitAllowed": true,
51+
"rebaseMergeAllowed": true,
52+
"squashMergeAllowed": false
53+
} } }`))
54+
55+
client := newTestClient(httpReg)
56+
repo, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO"))
57+
require.NoError(t, err)
58+
assert.Equal(t, &Repository{
59+
ID: "REPOID",
60+
Name: "REPO",
61+
Owner: RepositoryOwner{Login: "OWNER"},
62+
HasIssuesEnabled: true,
63+
Description: "a cool repo",
64+
HasWikiEnabled: true,
65+
ViewerPermission: "ADMIN",
66+
DefaultBranchRef: BranchRef{Name: "main"},
67+
MergeCommitAllowed: true,
68+
RebaseMergeAllowed: true,
69+
hostname: "github.com",
70+
}, repo)
71+
assert.True(t, repo.ViewerCanPush())
72+
assert.True(t, repo.ViewerCanTriage())
73+
}
74+
75+
func TestGitHubRepo_withParent(t *testing.T) {
76+
httpReg := &httpmock.Registry{}
77+
defer httpReg.Verify(t)
78+
79+
httpReg.Register(
80+
httpmock.GraphQL(`query RepositoryInfo\b`),
81+
httpmock.StringResponse(`
82+
{ "data": { "repository": {
83+
"id": "REPOID",
84+
"name": "REPO",
85+
"owner": {"login": "OWNER"},
86+
"hasIssuesEnabled": true,
87+
"description": "",
88+
"hasWikiEnabled": false,
89+
"viewerPermission": "READ",
90+
"defaultBranchRef": {"name": "main"},
91+
"parent": {
92+
"id": "PARENTID",
93+
"name": "PARENT-REPO",
94+
"owner": {"login": "PARENT-OWNER"},
95+
"hasIssuesEnabled": true,
96+
"description": "parent repo",
97+
"hasWikiEnabled": true,
98+
"viewerPermission": "READ",
99+
"defaultBranchRef": {"name": "develop"}
100+
},
101+
"mergeCommitAllowed": false,
102+
"rebaseMergeAllowed": false,
103+
"squashMergeAllowed": true
104+
} } }`))
105+
106+
client := newTestClient(httpReg)
107+
repo, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO"))
108+
require.NoError(t, err)
109+
wantParent := &Repository{
110+
ID: "PARENTID",
111+
Name: "PARENT-REPO",
112+
Owner: RepositoryOwner{Login: "PARENT-OWNER"},
113+
HasIssuesEnabled: true,
114+
Description: "parent repo",
115+
HasWikiEnabled: true,
116+
ViewerPermission: "READ",
117+
DefaultBranchRef: BranchRef{Name: "develop"},
118+
hostname: "github.com",
37119
}
120+
assert.Equal(t, &Repository{
121+
ID: "REPOID",
122+
Name: "REPO",
123+
Owner: RepositoryOwner{Login: "OWNER"},
124+
HasIssuesEnabled: true,
125+
ViewerPermission: "READ",
126+
DefaultBranchRef: BranchRef{Name: "main"},
127+
Parent: wantParent,
128+
SquashMergeAllowed: true,
129+
hostname: "github.com",
130+
}, repo)
131+
assert.False(t, repo.ViewerCanPush())
132+
assert.False(t, repo.ViewerCanTriage())
133+
}
134+
135+
func TestIssueRepoInfo_notFound(t *testing.T) {
136+
httpReg := &httpmock.Registry{}
137+
defer httpReg.Verify(t)
138+
139+
httpReg.Register(
140+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
141+
httpmock.StringResponse(`{ "data": { "repository": null } }`))
142+
143+
client := newTestClient(httpReg)
144+
repo, err := IssueRepoInfo(client, ghrepo.New("OWNER", "REPO"))
145+
require.EqualError(t, err, "GraphQL: Could not resolve to a Repository with the name 'OWNER/REPO'.")
146+
assert.Nil(t, repo)
147+
}
148+
149+
func TestIssueRepoInfo_success(t *testing.T) {
150+
httpReg := &httpmock.Registry{}
151+
defer httpReg.Verify(t)
152+
153+
httpReg.Register(
154+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
155+
httpmock.StringResponse(`
156+
{ "data": { "repository": {
157+
"id": "REPOID",
158+
"name": "REPO",
159+
"owner": {"login": "OWNER"},
160+
"hasIssuesEnabled": true,
161+
"viewerPermission": "WRITE"
162+
} } }`))
163+
164+
client := newTestClient(httpReg)
165+
repo, err := IssueRepoInfo(client, ghrepo.New("OWNER", "REPO"))
166+
require.NoError(t, err)
167+
assert.Equal(t, &Repository{
168+
ID: "REPOID",
169+
Name: "REPO",
170+
Owner: RepositoryOwner{Login: "OWNER"},
171+
HasIssuesEnabled: true,
172+
ViewerPermission: "WRITE",
173+
hostname: "github.com",
174+
}, repo)
175+
assert.True(t, repo.ViewerCanTriage())
176+
}
177+
178+
func TestIssueRepoInfo_issuesDisabled(t *testing.T) {
179+
httpReg := &httpmock.Registry{}
180+
defer httpReg.Verify(t)
181+
182+
httpReg.Register(
183+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
184+
httpmock.StringResponse(`
185+
{ "data": { "repository": {
186+
"id": "REPOID",
187+
"name": "REPO",
188+
"owner": {"login": "OWNER"},
189+
"hasIssuesEnabled": false,
190+
"viewerPermission": "READ"
191+
} } }`))
192+
193+
client := newTestClient(httpReg)
194+
repo, err := IssueRepoInfo(client, ghrepo.New("OWNER", "REPO"))
195+
require.NoError(t, err)
196+
assert.Equal(t, &Repository{
197+
ID: "REPOID",
198+
Name: "REPO",
199+
Owner: RepositoryOwner{Login: "OWNER"},
200+
ViewerPermission: "READ",
201+
hostname: "github.com",
202+
}, repo)
203+
assert.False(t, repo.ViewerCanTriage())
38204
}
39205

40206
func Test_RepoMetadata(t *testing.T) {

‎pkg/cmd/issue/create/create.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ func createRun(opts *CreateOptions) (err error) {
234234
fmt.Fprintf(opts.IO.ErrOut, "\nCreating issue in %s\n\n", ghrepo.FullName(baseRepo))
235235
}
236236

237-
repo, err := api.GitHubRepo(apiClient, baseRepo)
237+
repo, err := api.IssueRepoInfo(apiClient, baseRepo)
238238
if err != nil {
239239
return
240240
}

‎pkg/cmd/issue/create/create_test.go‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,7 @@ func Test_createRun(t *testing.T) {
411411
name: "editor",
412412
httpStubs: func(r *httpmock.Registry) {
413413
r.Register(
414-
httpmock.GraphQL(`query RepositoryInfo\b`),
414+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
415415
httpmock.StringResponse(`
416416
{ "data": { "repository": {
417417
"id": "REPOID",
@@ -440,7 +440,7 @@ func Test_createRun(t *testing.T) {
440440
name: "editor and template",
441441
httpStubs: func(r *httpmock.Registry) {
442442
r.Register(
443-
httpmock.GraphQL(`query RepositoryInfo\b`),
443+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
444444
httpmock.StringResponse(`
445445
{ "data": { "repository": {
446446
"id": "REPOID",
@@ -522,7 +522,7 @@ func Test_createRun(t *testing.T) {
522522
},
523523
httpStubs: func(r *httpmock.Registry) {
524524
r.Register(
525-
httpmock.GraphQL(`query RepositoryInfo\b`),
525+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
526526
httpmock.StringResponse(`
527527
{ "data": { "repository": {
528528
"id": "REPOID",
@@ -595,7 +595,7 @@ func Test_createRun(t *testing.T) {
595595
},
596596
httpStubs: func(r *httpmock.Registry) {
597597
r.Register(
598-
httpmock.GraphQL(`query RepositoryInfo\b`),
598+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
599599
httpmock.StringResponse(`
600600
{ "data": { "repository": {
601601
"id": "REPOID",
@@ -727,7 +727,7 @@ func TestIssueCreate(t *testing.T) {
727727
defer http.Verify(t)
728728

729729
http.Register(
730-
httpmock.GraphQL(`query RepositoryInfo\b`),
730+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
731731
httpmock.StringResponse(`
732732
{ "data": { "repository": {
733733
"id": "REPOID",
@@ -760,7 +760,7 @@ func TestIssueCreate_recover(t *testing.T) {
760760
defer http.Verify(t)
761761

762762
http.Register(
763-
httpmock.GraphQL(`query RepositoryInfo\b`),
763+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
764764
httpmock.StringResponse(`
765765
{ "data": { "repository": {
766766
"id": "REPOID",
@@ -844,7 +844,7 @@ func TestIssueCreate_nonLegacyTemplate(t *testing.T) {
844844
defer http.Verify(t)
845845

846846
http.Register(
847-
httpmock.GraphQL(`query RepositoryInfo\b`),
847+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
848848
httpmock.StringResponse(`
849849
{ "data": { "repository": {
850850
"id": "REPOID",
@@ -907,7 +907,7 @@ func TestIssueCreate_continueInBrowser(t *testing.T) {
907907
defer http.Verify(t)
908908

909909
http.Register(
910-
httpmock.GraphQL(`query RepositoryInfo\b`),
910+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
911911
httpmock.StringResponse(`
912912
{ "data": { "repository": {
913913
"id": "REPOID",
@@ -953,7 +953,7 @@ func TestIssueCreate_metadata(t *testing.T) {
953953
http := &httpmock.Registry{}
954954
defer http.Verify(t)
955955

956-
http.StubRepoInfoResponse("OWNER", "REPO", "main")
956+
http.StubIssueRepoInfoResponse("OWNER", "REPO")
957957
http.Register(
958958
httpmock.GraphQL(`query RepositoryLabelList\b`),
959959
httpmock.StringResponse(`
@@ -1064,7 +1064,7 @@ func TestIssueCreate_disabledIssues(t *testing.T) {
10641064
defer http.Verify(t)
10651065

10661066
http.Register(
1067-
httpmock.GraphQL(`query RepositoryInfo\b`),
1067+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
10681068
httpmock.StringResponse(`
10691069
{ "data": { "repository": {
10701070
"id": "REPOID",
@@ -1091,7 +1091,7 @@ func TestIssueCreate_AtMeAssignee(t *testing.T) {
10911091
`),
10921092
)
10931093
http.Register(
1094-
httpmock.GraphQL(`query RepositoryInfo\b`),
1094+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
10951095
httpmock.StringResponse(`
10961096
{ "data": { "repository": {
10971097
"id": "REPOID",
@@ -1134,7 +1134,7 @@ func TestIssueCreate_AtCopilotAssignee(t *testing.T) {
11341134
defer http.Verify(t)
11351135

11361136
http.Register(
1137-
httpmock.GraphQL(`query RepositoryInfo\b`),
1137+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
11381138
httpmock.StringResponse(`
11391139
{ "data": { "repository": {
11401140
"id": "REPOID",
@@ -1176,7 +1176,7 @@ func TestIssueCreate_projectsV2(t *testing.T) {
11761176
http := &httpmock.Registry{}
11771177
defer http.Verify(t)
11781178

1179-
http.StubRepoInfoResponse("OWNER", "REPO", "main")
1179+
http.StubIssueRepoInfoResponse("OWNER", "REPO")
11801180
http.Register(
11811181
httpmock.GraphQL(`query RepositoryProjectList\b`),
11821182
httpmock.StringResponse(`
@@ -1269,7 +1269,7 @@ func TestProjectsV1Deprecation(t *testing.T) {
12691269
ios, _, _, _ := iostreams.Test()
12701270

12711271
reg := &httpmock.Registry{}
1272-
reg.StubRepoInfoResponse("OWNER", "REPO", "main")
1272+
reg.StubIssueRepoInfoResponse("OWNER", "REPO")
12731273
reg.Register(
12741274
// ( is required to avoid matching projectsV2
12751275
httpmock.GraphQL(`projects\(`),
@@ -1306,7 +1306,7 @@ func TestProjectsV1Deprecation(t *testing.T) {
13061306
ios, _, _, _ := iostreams.Test()
13071307

13081308
reg := &httpmock.Registry{}
1309-
reg.StubRepoInfoResponse("OWNER", "REPO", "main")
1309+
reg.StubIssueRepoInfoResponse("OWNER", "REPO")
13101310
// ( is required to avoid matching projectsV2
13111311
reg.Exclude(t, httpmock.GraphQL(`projects\(`))
13121312

‎pkg/cmd/issue/transfer/transfer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func issueTransfer(httpClient *http.Client, issueID string, destRepo ghrepo.Inte
105105
destinationRepoID = r.ID
106106
} else {
107107
apiClient := api.NewClientFromHTTP(httpClient)
108-
r, err := api.GitHubRepo(apiClient, destRepo)
108+
r, err := api.IssueRepoInfo(apiClient, destRepo)
109109
if err != nil {
110110
return "", err
111111
}

‎pkg/cmd/issue/transfer/transfer_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ func Test_transferRunSuccessfulIssueTransfer(t *testing.T) {
169169
} } }`))
170170

171171
http.Register(
172-
httpmock.GraphQL(`query RepositoryInfo\b`),
172+
httpmock.GraphQL(`query IssueRepositoryInfo\b`),
173173
httpmock.StringResponse(`
174174
{ "data": { "repository": {
175175
"id": "dest-id",

0 commit comments

Comments
 (0)