diff --git a/api/client.go b/api/client.go index 575da6a6c..0447051cc 100644 --- a/api/client.go +++ b/api/client.go @@ -88,6 +88,18 @@ func (c Client) Query(hostname, name string, query interface{}, variables map[st return handleResponse(gqlClient.Query(name, query, variables)) } +// QueryWithContext performs a GraphQL query based on a struct and parses the response with the same struct as the receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. +func (c Client) QueryWithContext(ctx context.Context, hostname, name string, query interface{}, variables map[string]interface{}) error { + opts := clientOptions(hostname, c.http.Transport) + opts.Headers[graphqlFeatures] = features + gqlClient, err := gh.GQLClient(&opts) + if err != nil { + return err + } + return handleResponse(gqlClient.QueryWithContext(ctx, name, query, variables)) +} + // REST performs a REST request and parses the response. func (c Client) REST(hostname string, method string, p string, body io.Reader, data interface{}) error { opts := clientOptions(hostname, c.http.Transport) diff --git a/pkg/cmd/release/delete-asset/delete_asset.go b/pkg/cmd/release/delete-asset/delete_asset.go index 126397321..b2e1f22fe 100644 --- a/pkg/cmd/release/delete-asset/delete_asset.go +++ b/pkg/cmd/release/delete-asset/delete_asset.go @@ -1,6 +1,7 @@ package deleteasset import ( + "context" "fmt" "net/http" @@ -66,7 +67,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/delete-asset/delete_asset_test.go b/pkg/cmd/release/delete-asset/delete_asset_test.go index 11f7fbe84..f28f493d0 100644 --- a/pkg/cmd/release/delete-asset/delete_asset_test.go +++ b/pkg/cmd/release/delete-asset/delete_asset_test.go @@ -8,6 +8,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -156,18 +157,18 @@ func Test_deleteAssetRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "tag_name": "v1.2.3", "draft": false, "url": "https://api.github.com/repos/OWNER/REPO/releases/23456", "assets": [ - { - "url": "https://api.github.com/repos/OWNER/REPO/releases/assets/1", - "id": 1, - "name": "test-asset" - } + { + "url": "https://api.github.com/repos/OWNER/REPO/releases/assets/1", + "id": 1, + "name": "test-asset" + } ] - }`)) + }`) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/releases/assets/1"), httpmock.StatusStringResponse(204, "")) pm := &prompter.PrompterMock{} diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index 3a89c6c2a..94b09cfcb 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -1,6 +1,7 @@ package delete import ( + "context" "fmt" "net/http" @@ -69,7 +70,7 @@ func deleteRun(opts *DeleteOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/delete/delete_test.go b/pkg/cmd/release/delete/delete_test.go index 906cb057a..c0c43f990 100644 --- a/pkg/cmd/release/delete/delete_test.go +++ b/pkg/cmd/release/delete/delete_test.go @@ -9,6 +9,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -193,11 +194,11 @@ func Test_deleteRun(t *testing.T) { } fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "tag_name": "v1.2.3", "draft": false, "url": "https://api.github.com/repos/OWNER/REPO/releases/23456" - }`)) + }`) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/releases/23456"), httpmock.StatusStringResponse(204, "")) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/tags/v1.2.3"), httpmock.StatusStringResponse(204, "")) diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index a07226b2a..d595dca4a 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -1,6 +1,7 @@ package download import ( + "context" "errors" "fmt" "io" @@ -142,14 +143,16 @@ func downloadRun(opts *DownloadOptions) error { opts.IO.StartProgressIndicator() defer opts.IO.StopProgressIndicator() + ctx := context.Background() + var release *shared.Release if opts.TagName == "" { - release, err = shared.FetchLatestRelease(httpClient, baseRepo) + release, err = shared.FetchLatestRelease(ctx, httpClient, baseRepo) if err != nil { return err } } else { - release, err = shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err = shared.FetchRelease(ctx, httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index 4e6ca193f..c2260568a 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -322,7 +323,7 @@ func Test_downloadRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "assets": [ { "name": "windows-32bit.zip", "size": 12, "url": "https://api.github.com/assets/1234" }, @@ -333,7 +334,7 @@ func Test_downloadRun(t *testing.T) { ], "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" - }`)) + }`) fakeHTTP.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) fakeHTTP.Register(httpmock.REST("GET", "assets/3456"), httpmock.StringResponse(`3456`)) fakeHTTP.Register(httpmock.REST("GET", "assets/5678"), httpmock.StringResponse(`5678`)) @@ -378,7 +379,13 @@ func Test_downloadRun(t *testing.T) { expectedAcceptHeader = "application/octet-stream, application/json" } - assert.Equal(t, expectedAcceptHeader, fakeHTTP.Requests[1].Header.Get("Accept")) + for _, req := range fakeHTTP.Requests { + if req.Method != "GET" || req.URL.Path != "repos/OWNER/REPO/releases/tags/v1.2.3" { + // skip non-asset download requests + continue + } + assert.Equal(t, expectedAcceptHeader, req.Header.Get("Accept"), "for request %s", req.URL) + } assert.Equal(t, tt.wantStdout, stdout.String()) assert.Equal(t, tt.wantStderr, stderr.String()) @@ -509,15 +516,15 @@ func Test_downloadRun_cloberAndSkip(t *testing.T) { tt.opts.IO = ios reg := &httpmock.Registry{} - defer reg.Verify(t) - reg.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + // defer reg.Verify(t) // FIXME: intermittetly fails due to StubFetchRelease internals + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ "assets": [ { "name": "windows-64bit.zip", "size": 34, "url": "https://api.github.com/assets/3456" } ], "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" - }`)) + }`) if tt.httpStubs != nil { tt.httpStubs(reg) } diff --git a/pkg/cmd/release/edit/edit.go b/pkg/cmd/release/edit/edit.go index e3af2d714..89d365a9a 100644 --- a/pkg/cmd/release/edit/edit.go +++ b/pkg/cmd/release/edit/edit.go @@ -1,6 +1,7 @@ package edit import ( + "context" "fmt" "net/http" @@ -95,7 +96,7 @@ func editRun(tag string, opts *EditOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, tag) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, tag) if err != nil { return err } diff --git a/pkg/cmd/release/edit/edit_test.go b/pkg/cmd/release/edit/edit_test.go index c6bc31aef..9c7085b11 100644 --- a/pkg/cmd/release/edit/edit_test.go +++ b/pkg/cmd/release/edit/edit_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -415,14 +416,14 @@ func Test_editRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.JSONResponse(map[string]interface{}{ - "id": 12345, - "tag_name": "v1.2.3", - })) + // defer reg.Verify(t) // FIXME: intermittetly fails due to StubFetchRelease internals + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", "v1.2.3", `{ + "id": 12345, + "tag_name": "v1.2.3" + }`) if tt.httpStubs != nil { tt.httpStubs(t, fakeHTTP) } - defer fakeHTTP.Verify(t) tt.opts.IO = ios tt.opts.HttpClient = func() (*http.Client, error) { diff --git a/pkg/cmd/release/shared/fetch.go b/pkg/cmd/release/shared/fetch.go index 29e9c3ecf..c8182cc83 100644 --- a/pkg/cmd/release/shared/fetch.go +++ b/pkg/cmd/release/shared/fetch.go @@ -1,6 +1,7 @@ package shared import ( + "context" "encoding/json" "errors" "fmt" @@ -13,6 +14,9 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" ) var ReleaseFields = []string{ @@ -118,11 +122,83 @@ func (rel *Release) ExportData(fields []string) map[string]interface{} { return data } -// FetchRelease finds a repository release by its tagName. -func FetchRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases/tags/%s", baseRepo.RepoOwner(), baseRepo.RepoName(), tagName) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - req, err := http.NewRequest("GET", url, nil) +var errNotFound = errors.New("release not found") + +type fetchResult struct { + release *Release + error error +} + +// FetchRelease finds a published repository release by its tagName, or a draft release by its pending tag name. +func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { + cc, cancel := context.WithCancel(ctx) + results := make(chan fetchResult, 2) + + // published release lookup + go func() { + path := fmt.Sprintf("repos/%s/%s/releases/tags/%s", repo.RepoOwner(), repo.RepoName(), tagName) + release, err := fetchReleasePath(cc, httpClient, repo.RepoHost(), path) + results <- fetchResult{release: release, error: err} + }() + + // draft release lookup + go func() { + release, err := fetchDraftRelease(cc, httpClient, repo, tagName) + results <- fetchResult{release: release, error: err} + }() + + res := <-results + if errors.Is(res.error, errNotFound) { + res = <-results + cancel() // satisfy the linter even though no goroutines are running anymore + } else { + cancel() + <-results // drain the channel + } + return res.release, res.error +} + +// FetchLatestRelease finds the latest published release for a repository. +func FetchLatestRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) (*Release, error) { + path := fmt.Sprintf("repos/%s/%s/releases/latest", repo.RepoOwner(), repo.RepoName()) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) +} + +// fetchDraftRelease returns the first draft release that has tagName as its pending tag. +func fetchDraftRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { + // First use GraphQL to find a draft release by pending tag name, since REST doesn't have this ability. + var query struct { + Repository struct { + Release *struct { + DatabaseID int64 + IsDraft bool + } `graphql:"release(tagName: $tagName)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "tagName": githubv4.String(tagName), + } + + gql := api.NewClientFromHTTP(httpClient) + if err := gql.QueryWithContext(ctx, repo.RepoHost(), "RepositoryReleaseByTag", &query, variables); err != nil { + return nil, err + } + + if query.Repository.Release == nil || !query.Repository.Release.IsDraft { + return nil, errNotFound + } + + // Then, use REST to get information about the draft release. In theory, we could have fetched + // all the necessary information via GraphQL, but REST is safer for backwards compatibility. + path := fmt.Sprintf("repos/%s/%s/releases/%d", repo.RepoOwner(), repo.RepoName(), query.Repository.Release.DatabaseID) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) +} + +func fetchReleasePath(ctx context.Context, httpClient *http.Client, host string, p string) (*Release, error) { + req, err := http.NewRequestWithContext(ctx, "GET", ghinstance.RESTPrefix(host)+p, nil) if err != nil { return nil, err } @@ -134,102 +210,39 @@ func FetchRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName st defer resp.Body.Close() if resp.StatusCode == 404 { - return FindDraftRelease(httpClient, baseRepo, tagName) - } - - if resp.StatusCode > 299 { + _, _ = io.Copy(io.Discard, resp.Body) + return nil, errNotFound + } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var release Release - err = json.Unmarshal(b, &release) - if err != nil { + if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { return nil, err } return &release, nil } -// FetchLatestRelease finds the latest published release for a repository. -func FetchLatestRelease(httpClient *http.Client, baseRepo ghrepo.Interface) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases/latest", baseRepo.RepoOwner(), baseRepo.RepoName()) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - req, err := http.NewRequest("GET", url, nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - - var release Release - err = json.Unmarshal(b, &release) - if err != nil { - return nil, err - } - - return &release, nil +type testingT interface { + Errorf(format string, args ...interface{}) } -// FindDraftRelease returns the latest draft release that matches tagName. -func FindDraftRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases", baseRepo.RepoOwner(), baseRepo.RepoName()) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - - perPage := 100 - page := 1 - for { - req, err := http.NewRequest("GET", fmt.Sprintf("%s?per_page=%d&page=%d", url, perPage, page), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - - var releases []Release - err = json.Unmarshal(b, &releases) - if err != nil { - return nil, err - } - - for _, r := range releases { - if r.IsDraft && r.TagName == tagName { - return &r, nil - } - } - //nolint:staticcheck - break +func StubFetchRelease(t testingT, reg *httpmock.Registry, owner, repoName, tagName, responseBody string) { + path := "repos/OWNER/REPO/releases/tags/v1.2.3" + if tagName == "" { + path = "repos/OWNER/REPO/releases/latest" } - return nil, errors.New("release not found") + reg.Register(httpmock.REST("GET", path), httpmock.StringResponse(responseBody)) + + if tagName != "" { + reg.Register( + httpmock.GraphQL(`query RepositoryReleaseByTag\b`), + httpmock.GraphQLQuery(`{ "data": { "repository": { "release": null }}}`, func(q string, vars map[string]interface{}) { + assert.Equal(t, owner, vars["owner"]) + assert.Equal(t, repoName, vars["name"]) + assert.Equal(t, tagName, vars["tagName"]) + })) + } } diff --git a/pkg/cmd/release/upload/upload.go b/pkg/cmd/release/upload/upload.go index 8f573bd4f..b3af37c4a 100644 --- a/pkg/cmd/release/upload/upload.go +++ b/pkg/cmd/release/upload/upload.go @@ -1,6 +1,7 @@ package upload import ( + "context" "fmt" "net/http" "strings" @@ -80,7 +81,7 @@ func uploadRun(opts *UploadOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/view/view.go b/pkg/cmd/release/view/view.go index b0f130edb..1f6fb6a34 100644 --- a/pkg/cmd/release/view/view.go +++ b/pkg/cmd/release/view/view.go @@ -1,6 +1,7 @@ package view import ( + "context" "fmt" "io" "net/http" @@ -82,15 +83,16 @@ func viewRun(opts *ViewOptions) error { return err } + ctx := context.Background() var release *shared.Release if opts.TagName == "" { - release, err = shared.FetchLatestRelease(httpClient, baseRepo) + release, err = shared.FetchLatestRelease(ctx, httpClient, baseRepo) if err != nil { return err } } else { - release, err = shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err = shared.FetchRelease(ctx, httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/view/view_test.go b/pkg/cmd/release/view/view_test.go index a1d1d6d5d..5d42ba373 100644 --- a/pkg/cmd/release/view/view_test.go +++ b/pkg/cmd/release/view/view_test.go @@ -10,6 +10,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -213,13 +214,8 @@ func Test_viewRun(t *testing.T) { ios.SetStdinTTY(tt.isTTY) ios.SetStderrTTY(tt.isTTY) - path := "repos/OWNER/REPO/releases/tags/v1.2.3" - if tt.opts.TagName == "" { - path = "repos/OWNER/REPO/releases/latest" - } - fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", path), httpmock.StringResponse(fmt.Sprintf(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, fmt.Sprintf(`{ "tag_name": "v1.2.3", "draft": false, "author": { "login": "MonaLisa" }, @@ -231,7 +227,7 @@ func Test_viewRun(t *testing.T) { { "name": "windows.zip", "size": 12 }, { "name": "linux.tgz", "size": 34 } ] - }`, tt.releasedAt.Format(time.RFC3339), tt.releaseBody))) + }`, tt.releasedAt.Format(time.RFC3339), tt.releaseBody)) tt.opts.IO = ios tt.opts.HttpClient = func() (*http.Client, error) {