From b46289cee641f18ec44f4b9b1c9fbbc8b3665976 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 28 Jul 2026 13:30:39 +0200 Subject: [PATCH] Wrap RESTWithNext errors as api.HTTPError Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9950859f-e1b7-4129-9ebb-26018d5434bb --- api/client.go | 7 +- api/client_test.go | 95 ++++++++++++++++++++++++++ pkg/cmd/attestation/api/client_test.go | 41 +++++++++++ pkg/cmd/status/status_test.go | 19 ++++++ 4 files changed, 156 insertions(+), 6 deletions(-) diff --git a/api/client.go b/api/client.go index 8ee525df59d..27a747995c9 100644 --- a/api/client.go +++ b/api/client.go @@ -119,15 +119,10 @@ func (c Client) RESTWithNext(hostname string, method string, p string, body io.R resp, err := restClient.Request(method, p, body) if err != nil { - return "", err + return "", handleResponse(err) } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return "", HandleHTTPError(resp) - } - if resp.StatusCode == http.StatusNoContent { return "", nil } diff --git a/api/client_test.go b/api/client_test.go index f988e090c3a..bf7a93d85b7 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func newTestClient(reg *httpmock.Registry) *Client { @@ -138,6 +139,100 @@ func TestRESTError(t *testing.T) { } } +func TestRESTWithNextError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: http.StatusNotFound, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Not Found"}`)), + Header: http.Header{ + "Content-Type": {"application/json"}, + "X-Accepted-Oauth-Scopes": {"repo"}, + "X-Oauth-Scopes": {"read:user"}, + }, + }, nil + }) + + _, err := client.RESTWithNext("github.com", http.MethodGet, "repos/owner/repo/items", nil, nil) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") + assert.Equal(t, `This API operation needs the "repo" scope. To request it, run: gh auth refresh -h github.com -s repo`, httpErr.ScopesSuggestion()) +} + +func TestRESTAndRESTWithNextErrorTypeParity(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + responder := func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: http.StatusNotFound, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Not Found"}`)), + Header: http.Header{"Content-Type": {"application/json"}}, + }, nil + } + reg.Register(httpmock.MatchAny, responder) + reg.Register(httpmock.MatchAny, responder) + + restErr := client.REST("github.com", http.MethodGet, "repos/owner/repo/items", nil, nil) + _, restWithNextErr := client.RESTWithNext("github.com", http.MethodGet, "repos/owner/repo/items", nil, nil) + + require.Error(t, restErr) + require.Error(t, restWithNextErr) + assert.IsType(t, restErr, restWithNextErr) +} + +func TestRESTWithNext(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(`{"name": "item"}`)), + Header: http.Header{ + "Content-Type": {"application/json"}, + "Link": {`; rel="next", ; rel="last"`}, + }, + }, nil + }) + + response := struct { + Name string `json:"name"` + }{} + next, err := client.RESTWithNext("github.com", http.MethodGet, "repos/owner/repo/items", nil, &response) + + require.NoError(t, err) + assert.Equal(t, "item", response.Name) + assert.Equal(t, "https://api.github.com/repos/owner/repo/items?page=2", next) +} + +func TestRESTWithNextNoContent(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register( + httpmock.REST(http.MethodDelete, "repos/owner/repo/items/1"), + httpmock.StatusStringResponse(http.StatusNoContent, "not JSON"), + ) + + next, err := client.RESTWithNext("github.com", http.MethodDelete, "repos/owner/repo/items/1", nil, nil) + + require.NoError(t, err) + assert.Empty(t, next) +} + func TestHandleHTTPError_GraphQL502(t *testing.T) { req, err := http.NewRequest("GET", "https://api.github.com/user", nil) if err != nil { diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 9f96be3448e..4bb7c493b0a 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -1,11 +1,14 @@ package api import ( + "net/http" "testing" + cliAPI "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/attestation/io" "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/require" ) @@ -379,6 +382,44 @@ func TestGetAttestationsRetries(t *testing.T) { require.Equal(t, bundle.GetMediaType(), "application/vnd.dev.sigstore.bundle.v0.3+json") } +func TestGetAttestationsRetriesRESTWithNextError(t *testing.T) { + originalRetryInterval := getAttestationRetryInterval + getAttestationRetryInterval = 0 + t.Cleanup(func() { + getAttestationRetryInterval = originalRetryInterval + }) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.MatchAny, + httpmock.StatusStringResponse(http.StatusInternalServerError, `{"message":"Internal Server Error"}`), + ) + reg.Register( + httpmock.MatchAny, + httpmock.JSONResponse(map[string]any{ + "attestations": []any{ + map[string]any{"bundle_url": "https://example.com/bundle"}, + }, + }), + ) + + c := &LiveClient{ + githubAPI: cliAPI.NewClientFromHTTP(&http.Client{Transport: reg}), + host: "github.com", + logger: io.NewTestHandler(), + } + attestations, err := c.getAttestations(FetchParams{ + Digest: testDigest, + Limit: 1, + Repo: testRepo, + }) + + require.NoError(t, err) + require.Len(t, attestations, 1) + require.Len(t, reg.Requests, 2) +} + // test total retries func TestGetAttestationsMaxRetries(t *testing.T) { getAttestationRetryInterval = 0 diff --git a/pkg/cmd/status/status_test.go b/pkg/cmd/status/status_test.go index 9be333de73d..685ad5be74b 100644 --- a/pkg/cmd/status/status_test.go +++ b/pkg/cmd/status/status_test.go @@ -111,6 +111,25 @@ func TestStatusRun(t *testing.T) { opts: &StatusOptions{}, wantOut: "Assigned Issues │ Assigned Pull Requests \nNothing here ^_^ │ Nothing here ^_^ \n │ \nReview Requests │ Mentions \nNothing here ^_^ │ Nothing here ^_^ \n │ \nRepository Activity\nNothing here ^_^\n\n", }, + { + name: "notifications 404 is tolerated", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.StringResponse(`{"data": { "assignments": {"nodes": [] }, "reviewRequested": {"nodes": []}}}`)) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`)) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.StringResponse(`[]`)) + }, + opts: &StatusOptions{}, + wantOut: "Assigned Issues │ Assigned Pull Requests \nNothing here ^_^ │ Nothing here ^_^ \n │ \nReview Requests │ Mentions \nNothing here ^_^ │ Nothing here ^_^ \n │ \nRepository Activity\nNothing here ^_^\n\n", + }, { name: "something", httpStubs: func(reg *httpmock.Registry) {