diff --git a/api/client.go b/api/client.go index 61b5d844b..0447051cc 100644 --- a/api/client.go +++ b/api/client.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "errors" "fmt" @@ -51,8 +52,8 @@ func (err HTTPError) ScopesSuggestion() string { return err.scopesSuggestion } -// GraphQL performs a GraphQL request and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// GraphQL performs a GraphQL request using the query string and parses the response into data receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features @@ -63,8 +64,8 @@ func (c Client) GraphQL(hostname string, query string, variables map[string]inte return handleResponse(gqlClient.Do(query, variables, data)) } -// GraphQL performs a GraphQL mutation and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// Mutate performs a GraphQL mutation 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) Mutate(hostname, name string, mutation interface{}, variables map[string]interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features @@ -75,8 +76,8 @@ func (c Client) Mutate(hostname, name string, mutation interface{}, variables ma return handleResponse(gqlClient.Mutate(name, mutation, variables)) } -// GraphQL performs a GraphQL query and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// Query 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) Query(hostname, name string, query interface{}, variables map[string]interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features @@ -87,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/go.mod b/go.mod index f2f0eac9b..e1f9d849b 100644 --- a/go.mod +++ b/go.mod @@ -36,7 +36,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.5 golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 - golang.org/x/sync v0.0.0-20210220032951-036812b2e83c + golang.org/x/sync v0.1.0 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 golang.org/x/text v0.3.8 google.golang.org/grpc v1.49.0 diff --git a/go.sum b/go.sum index 4816ac2c1..53d80dced 100644 --- a/go.sum +++ b/go.sum @@ -342,8 +342,8 @@ golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= +golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index ee9ad3429..e667f65ef 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -426,6 +426,7 @@ func createRun(opts *CreateOptions) error { } hasAssets := len(opts.Assets) > 0 + draftWhileUploading := false if hasAssets && !opts.Draft { // Check for an existing release @@ -437,6 +438,7 @@ func createRun(opts *CreateOptions) error { } } // Save the release initially as draft and publish it after all assets have finished uploading + draftWhileUploading = true params["draft"] = true } @@ -445,6 +447,16 @@ func createRun(opts *CreateOptions) error { return err } + cleanupDraftRelease := func(err error) error { + if !draftWhileUploading { + return err + } + if cleanupErr := deleteRelease(httpClient, newRelease); cleanupErr != nil { + return fmt.Errorf("%w\ncleaning up draft failed: %v", err, cleanupErr) + } + return err + } + if hasAssets { uploadURL := newRelease.UploadURL if idx := strings.IndexRune(uploadURL, '{'); idx > 0 { @@ -455,13 +467,13 @@ func createRun(opts *CreateOptions) error { err = shared.ConcurrentUpload(httpClient, uploadURL, opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { - return err + return cleanupDraftRelease(err) } - if !opts.Draft { + if draftWhileUploading { rel, err := publishRelease(httpClient, newRelease.APIURL, opts.DiscussionCategory) if err != nil { - return err + return cleanupDraftRelease(err) } newRelease = rel } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index aec10911a..ee5c1caac 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -728,6 +728,102 @@ func Test_createRun(t *testing.T) { wantStderr: ``, wantErr: `a release with the same tag name already exists: v1.2.3`, }, + { + name: "clean up draft after uploading files fails", + isTTY: false, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(404, ``)) + reg.Register(httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusStringResponse(201, `{ + "url": "https://api.github.com/releases/123", + "upload_url": "https://api.github.com/assets/upload", + "html_url": "https://github.com/OWNER/REPO/releases/tag/v1.2.3" + }`)) + reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(422, `{}`)) + reg.Register(httpmock.REST("DELETE", "releases/123"), httpmock.StatusStringResponse(204, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `HTTP 422 (https://api.github.com/assets/upload?label=&name=ball.tgz)`, + }, + { + name: "clean up draft after publishing fails", + isTTY: false, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(404, ``)) + reg.Register(httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusStringResponse(201, `{ + "url": "https://api.github.com/releases/123", + "upload_url": "https://api.github.com/assets/upload", + "html_url": "https://github.com/OWNER/REPO/releases/tag/v1.2.3" + }`)) + reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(201, `{}`)) + reg.Register(httpmock.REST("PATCH", "releases/123"), httpmock.StatusStringResponse(500, `{}`)) + reg.Register(httpmock.REST("DELETE", "releases/123"), httpmock.StatusStringResponse(204, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `HTTP 500 (https://api.github.com/releases/123)`, + }, + { + name: "upload files but release already exists", + isTTY: true, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(200, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `a release with the same tag name already exists: v1.2.3`, + }, { name: "upload files and create discussion", isTTY: true, diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 2a0b65468..f970d46f1 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -225,3 +225,28 @@ func publishRelease(httpClient *http.Client, releaseURL string, discussionCatego err = json.Unmarshal(b, &release) return &release, err } + +func deleteRelease(httpClient *http.Client, release *shared.Release) error { + req, err := http.NewRequest("DELETE", release.APIURL, nil) + if err != nil { + return err + } + + resp, err := httpClient.Do(req) + if err != nil { + return err + } + if resp.Body != nil { + defer resp.Body.Close() + } + + success := resp.StatusCode >= 200 && resp.StatusCode < 300 + if !success { + return api.HandleHTTPError(resp) + } + + if resp.StatusCode != 204 { + _, _ = io.Copy(io.Discard, resp.Body) + } + return nil +} 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/shared/upload.go b/pkg/cmd/release/shared/upload.go index 802f623c4..939deffe7 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -1,6 +1,7 @@ package shared import ( + "context" "encoding/json" "errors" "io" @@ -13,8 +14,15 @@ import ( "time" "github.com/cli/cli/v2/api" + "golang.org/x/sync/errgroup" ) +type httpDoer interface { + Do(*http.Request) (*http.Response, error) +} + +type errNetwork struct{ error } + type AssetForUpload struct { Name string Label string @@ -90,64 +98,61 @@ func fileExt(fn string) string { return path.Ext(fn) } -func ConcurrentUpload(httpClient *http.Client, uploadURL string, numWorkers int, assets []*AssetForUpload) error { +func ConcurrentUpload(httpClient httpDoer, uploadURL string, numWorkers int, assets []*AssetForUpload) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } - jobs := make(chan AssetForUpload, len(assets)) - results := make(chan error, len(assets)) - - if len(assets) < numWorkers { - numWorkers = len(assets) - } - - for w := 1; w <= numWorkers; w++ { - go func() { - for a := range jobs { - results <- uploadWithDelete(httpClient, uploadURL, a) - } - }() - } + ctx := context.Background() + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(numWorkers) for _, a := range assets { - jobs <- *a + asset := *a + g.Go(func() error { + return uploadWithDelete(gctx, httpClient, uploadURL, asset) + }) } - close(jobs) - var uploadError error - for i := 0; i < len(assets); i++ { - if err := <-results; err != nil { - uploadError = err - } - } - return uploadError + return g.Wait() } +var retryInterval = time.Millisecond * 200 + const maxRetries = 3 -func uploadWithDelete(httpClient *http.Client, uploadURL string, a AssetForUpload) error { +func shouldRetry(err error) bool { + var networkError errNetwork + if errors.As(err, &networkError) { + return true + } + var httpError api.HTTPError + return errors.As(err, &httpError) && httpError.StatusCode >= 500 +} + +func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL string, a AssetForUpload) error { if a.ExistingURL != "" { - err := deleteAsset(httpClient, a.ExistingURL) - if err != nil { + if err := deleteAsset(ctx, httpClient, a.ExistingURL); err != nil { return err } } retries := 0 for { - var httpError api.HTTPError - _, err := uploadAsset(httpClient, uploadURL, a) - // retry upload several times upon receiving HTTP 5xx - if err == nil || !errors.As(err, &httpError) || httpError.StatusCode < 500 || retries < maxRetries { + _, err := uploadAsset(ctx, httpClient, uploadURL, a) + if err == nil || retries == maxRetries || !shouldRetry(err) { return err } retries++ - time.Sleep(time.Duration(retries) * time.Second) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Duration(retries) * retryInterval): + } } } -func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload) (*ReleaseAsset, error) { +func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL string, asset AssetForUpload) (*ReleaseAsset, error) { u, err := url.Parse(uploadURL) if err != nil { return nil, err @@ -163,7 +168,7 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload } defer f.Close() - req, err := http.NewRequest("POST", u.String(), f) + req, err := http.NewRequestWithContext(ctx, "POST", u.String(), f) if err != nil { return nil, err } @@ -173,7 +178,7 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload resp, err := httpClient.Do(req) if err != nil { - return nil, err + return nil, errNetwork{err} } defer resp.Body.Close() @@ -182,22 +187,17 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload return nil, api.HandleHTTPError(resp) } - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var newAsset ReleaseAsset - err = json.Unmarshal(b, &newAsset) - if err != nil { + dec := json.NewDecoder(resp.Body) + if err := dec.Decode(&newAsset); err != nil { return nil, err } return &newAsset, nil } -func deleteAsset(httpClient *http.Client, assetURL string) error { - req, err := http.NewRequest("DELETE", assetURL, nil) +func deleteAsset(ctx context.Context, httpClient httpDoer, assetURL string) error { + req, err := http.NewRequestWithContext(ctx, "DELETE", assetURL, nil) if err != nil { return err } diff --git a/pkg/cmd/release/shared/upload_test.go b/pkg/cmd/release/shared/upload_test.go index da1aa8918..f2b623312 100644 --- a/pkg/cmd/release/shared/upload_test.go +++ b/pkg/cmd/release/shared/upload_test.go @@ -1,6 +1,14 @@ package shared -import "testing" +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "testing" + "time" +) func Test_typeForFilename(t *testing.T) { tests := []struct { @@ -67,3 +75,48 @@ func Test_typeForFilename(t *testing.T) { }) } } + +func Test_uploadWithDelete_retry(t *testing.T) { + retryInterval = time.Millisecond + ctx := context.Background() + + tries := 0 + client := funcClient(func(req *http.Request) (*http.Response, error) { + tries++ + if tries == 1 { + return nil, errors.New("made up exception") + } else if tries == 2 { + return &http.Response{ + Request: req, + StatusCode: 500, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + }, nil + } + return &http.Response{ + Request: req, + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + }, nil + }) + err := uploadWithDelete(ctx, client, "http://example.com/upload", AssetForUpload{ + Name: "asset", + Label: "", + Size: 8, + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`somebody`)), nil + }, + MIMEType: "application/octet-stream", + }) + if err != nil { + t.Errorf("uploadWithDelete() error: %v", err) + } + if tries != 3 { + t.Errorf("tries = %d, expected %d", tries, 3) + } +} + +type funcClient func(*http.Request) (*http.Response, error) + +func (f funcClient) Do(req *http.Request) (*http.Response, error) { + return f(req) +} 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) {