diff --git a/api/client_test.go b/api/client_test.go index 0f36e3f69..1701a17a9 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -234,10 +234,9 @@ func TestHTTPHeaders(t *testing.T) { ios, _, _, stderr := iostreams.Test() httpClient, err := NewHTTPClient(HTTPClientOptions{ - AppVersion: "v1.2.3", - Config: tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"}, - Log: ios.ErrOut, - SkipAcceptHeaders: false, + AppVersion: "v1.2.3", + Config: tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"}, + Log: ios.ErrOut, }) assert.NoError(t, err) client := NewClientFromHTTP(httpClient) diff --git a/api/http_client.go b/api/http_client.go index 9f2d59ce3..fcf036008 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -17,14 +17,13 @@ type tokenGetter interface { } type HTTPClientOptions struct { - AppVersion string - CacheTTL time.Duration - Config tokenGetter - EnableCache bool - Log io.Writer - LogColorize bool - LogVerboseHTTP bool - SkipAcceptHeaders bool + AppVersion string + CacheTTL time.Duration + Config tokenGetter + EnableCache bool + Log io.Writer + LogColorize bool + LogVerboseHTTP bool } func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { @@ -50,9 +49,6 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { headers := map[string]string{ userAgent: fmt.Sprintf("GitHub CLI %s", opts.AppVersion), } - if opts.SkipAcceptHeaders { - headers[accept] = "" - } clientOpts.Headers = headers if opts.EnableCache { diff --git a/api/http_client_test.go b/api/http_client_test.go index b1e84e582..f8fe28d40 100644 --- a/api/http_client_test.go +++ b/api/http_client_test.go @@ -20,7 +20,6 @@ func TestNewHTTPClient(t *testing.T) { type args struct { config tokenGetter appVersion string - setAccept bool logVerboseHTTP bool } tests := []struct { @@ -31,11 +30,10 @@ func TestNewHTTPClient(t *testing.T) { wantStderr string }{ { - name: "github.com with Accept header", + name: "github.com", args: args{ config: tinyConfig{"github.com:oauth_token": "MYTOKEN"}, appVersion: "v1.2.3", - setAccept: true, logVerboseHTTP: false, }, host: "github.com", @@ -47,18 +45,16 @@ func TestNewHTTPClient(t *testing.T) { wantStderr: "", }, { - name: "github.com no Accept header", + name: "GHES", args: args{ - config: tinyConfig{"github.com:oauth_token": "MYTOKEN"}, - appVersion: "v1.2.3", - setAccept: false, - logVerboseHTTP: false, + config: tinyConfig{"example.com:oauth_token": "GHETOKEN"}, + appVersion: "v1.2.3", }, - host: "github.com", + host: "example.com", wantHeader: map[string]string{ - "authorization": "token MYTOKEN", + "authorization": "token GHETOKEN", "user-agent": "GitHub CLI v1.2.3", - "accept": "", + "accept": "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview", }, wantStderr: "", }, @@ -67,7 +63,6 @@ func TestNewHTTPClient(t *testing.T) { args: args{ config: tinyConfig{"example.com:oauth_token": "MYTOKEN"}, appVersion: "v1.2.3", - setAccept: true, logVerboseHTTP: false, }, host: "github.com", @@ -78,12 +73,26 @@ func TestNewHTTPClient(t *testing.T) { }, wantStderr: "", }, + { + name: "GHES no authentication token", + args: args{ + config: tinyConfig{"github.com:oauth_token": "MYTOKEN"}, + appVersion: "v1.2.3", + logVerboseHTTP: false, + }, + host: "example.com", + wantHeader: map[string]string{ + "authorization": "", + "user-agent": "GitHub CLI v1.2.3", + "accept": "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview", + }, + wantStderr: "", + }, { name: "github.com in verbose mode", args: args{ config: tinyConfig{"github.com:oauth_token": "MYTOKEN"}, appVersion: "v1.2.3", - setAccept: true, logVerboseHTTP: true, }, host: "github.com", @@ -109,21 +118,6 @@ func TestNewHTTPClient(t *testing.T) { * Request took `), }, - { - name: "GHES Accept header", - args: args{ - config: tinyConfig{"example.com:oauth_token": "GHETOKEN"}, - appVersion: "v1.2.3", - setAccept: true, - }, - host: "example.com", - wantHeader: map[string]string{ - "authorization": "token GHETOKEN", - "user-agent": "GitHub CLI v1.2.3", - "accept": "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview", - }, - wantStderr: "", - }, } var gotReq *http.Request @@ -137,11 +131,10 @@ func TestNewHTTPClient(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ios, _, _, stderr := iostreams.Test() client, err := NewHTTPClient(HTTPClientOptions{ - AppVersion: tt.args.appVersion, - Config: tt.args.config, - Log: ios.ErrOut, - SkipAcceptHeaders: !tt.args.setAccept, - LogVerboseHTTP: tt.args.logVerboseHTTP, + AppVersion: tt.args.appVersion, + Config: tt.args.config, + Log: ios.ErrOut, + LogVerboseHTTP: tt.args.logVerboseHTTP, }) require.NoError(t, err) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 38b6278b4..610f2f830 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -298,14 +298,13 @@ func apiRun(opts *ApiOptions) error { log = opts.IO.Out } opts := api.HTTPClientOptions{ - AppVersion: opts.AppVersion, - CacheTTL: opts.CacheTTL, - Config: cfg.Authentication(), - EnableCache: opts.CacheTTL > 0, - Log: log, - LogColorize: opts.IO.ColorEnabled(), - LogVerboseHTTP: opts.Verbose, - SkipAcceptHeaders: true, + AppVersion: opts.AppVersion, + CacheTTL: opts.CacheTTL, + Config: cfg.Authentication(), + EnableCache: opts.CacheTTL > 0, + Log: log, + LogColorize: opts.IO.ColorEnabled(), + LogVerboseHTTP: opts.Verbose, } return api.NewHTTPClient(opts) } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 178bebd86..3ffc6a0b3 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -1455,3 +1455,58 @@ func Test_parseErrorResponse(t *testing.T) { }) } } + +func Test_apiRun_acceptHeader(t *testing.T) { + tests := []struct { + name string + options ApiOptions + wantAcceptHeader string + }{ + { + name: "sets default accept header", + options: ApiOptions{}, + wantAcceptHeader: "*/*", + }, + { + name: "does not override user accept header", + options: ApiOptions{ + RequestHeaders: []string{"Accept: testing"}, + }, + wantAcceptHeader: "testing", + }, + { + name: "does not override preview names", + options: ApiOptions{ + Previews: []string{"nebula"}, + }, + wantAcceptHeader: "application/vnd.github.nebula-preview+json", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + tt.options.IO = ios + + tt.options.Config = func() (config.Config, error) { + return config.NewBlankConfig(), nil + } + + var gotReq *http.Request + tt.options.HttpClient = func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + gotReq = req + resp := &http.Response{ + StatusCode: 200, + Request: req, + Body: io.NopCloser(bytes.NewBufferString("")), + } + return resp, nil + } + return &http.Client{Transport: tr}, nil + } + + assert.NoError(t, apiRun(&tt.options)) + assert.Equal(t, tt.wantAcceptHeader, gotReq.Header.Get("Accept")) + }) + } +} diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go index 3d6bb240e..b1b503cc2 100644 --- a/pkg/cmd/api/http.go +++ b/pkg/cmd/api/http.go @@ -74,6 +74,9 @@ func httpRequest(client *http.Client, hostname string, method string, p string, if bodyIsJSON && req.Header.Get("Content-Type") == "" { req.Header.Set("Content-Type", "application/json; charset=utf-8") } + if req.Header.Get("Accept") == "" { + req.Header.Set("Accept", "*/*") + } return client.Do(req) } diff --git a/pkg/cmd/api/http_test.go b/pkg/cmd/api/http_test.go index f63566490..2778ea38c 100644 --- a/pkg/cmd/api/http_test.go +++ b/pkg/cmd/api/http_test.go @@ -126,7 +126,25 @@ func Test_httpRequest(t *testing.T) { method: "GET", u: "https://api.github.com/repos/octocat/spoon-knife", body: "", - headers: "", + headers: "Accept: */*\r\n", + }, + }, + { + name: "GET with accept header", + args: args{ + client: &httpClient, + host: "github.com", + method: "GET", + p: "repos/octocat/spoon-knife", + params: nil, + headers: []string{"Accept: testing"}, + }, + wantErr: false, + want: expects{ + method: "GET", + u: "https://api.github.com/repos/octocat/spoon-knife", + body: "", + headers: "Accept: testing\r\n", }, }, { @@ -144,7 +162,7 @@ func Test_httpRequest(t *testing.T) { method: "GET", u: "https://api.github.com/repos/octocat/spoon-knife", body: "", - headers: "", + headers: "Accept: */*\r\n", }, }, { @@ -162,7 +180,7 @@ func Test_httpRequest(t *testing.T) { method: "GET", u: "https://api.github.com/repos/octocat/spoon-knife", body: "", - headers: "", + headers: "Accept: */*\r\n", }, }, { @@ -180,7 +198,7 @@ func Test_httpRequest(t *testing.T) { method: "GET", u: "https://example.org/api/v3/repos/octocat/spoon-knife", body: "", - headers: "", + headers: "Accept: */*\r\n", }, }, { @@ -200,7 +218,7 @@ func Test_httpRequest(t *testing.T) { method: "GET", u: "https://api.github.com/repos/octocat/spoon-knife?a=b", body: "", - headers: "", + headers: "Accept: */*\r\n", }, }, { @@ -220,7 +238,7 @@ func Test_httpRequest(t *testing.T) { method: "POST", u: "https://api.github.com/repos", body: `{"a":"b"}`, - headers: "Content-Type: application/json; charset=utf-8\r\n", + headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n", }, }, { @@ -240,7 +258,7 @@ func Test_httpRequest(t *testing.T) { method: "POST", u: "https://api.github.com/graphql", body: `{"variables":{"a":"b"}}`, - headers: "Content-Type: application/json; charset=utf-8\r\n", + headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n", }, }, { @@ -258,7 +276,7 @@ func Test_httpRequest(t *testing.T) { method: "POST", u: "https://example.org/api/graphql", body: `{}`, - headers: "Content-Type: application/json; charset=utf-8\r\n", + headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n", }, }, {