From 0abb9b4b64e047f0ffa542a0d5bc6c65a51bb0f7 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 14 Jun 2022 08:57:40 -0500 Subject: [PATCH 1/4] Update error message prefix --- gh_test.go | 2 +- internal/api/gql_client_test.go | 2 +- pkg/api/errors.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gh_test.go b/gh_test.go index 9e6f62d..967bd6a 100644 --- a/gh_test.go +++ b/gh_test.go @@ -130,7 +130,7 @@ func TestGQLClientError(t *testing.T) { res := struct{ Organization struct{ Name string } }{} err = client.Do("QUERY", nil, &res) - assert.EqualError(t, err, "GQL: Could not resolve to an Organization with the login of 'cli'. (organization)") + assert.EqualError(t, err, "GraphQL: Could not resolve to an Organization with the login of 'cli'. (organization)") assert.True(t, gock.IsDone(), printPendingMocks(gock.Pending())) } diff --git a/internal/api/gql_client_test.go b/internal/api/gql_client_test.go index 5cd7959..9cb2f58 100644 --- a/internal/api/gql_client_test.go +++ b/internal/api/gql_client_test.go @@ -37,7 +37,7 @@ func TestGQLClientDo(t *testing.T) { JSON(`{"errors":[{"message":"OH NO"},{"message":"this is fine"}]}`) }, wantErr: true, - wantErrMsg: "GQL: OH NO, this is fine", + wantErrMsg: "GraphQL: OH NO, this is fine", }, { name: "http fail request empty response", diff --git a/pkg/api/errors.go b/pkg/api/errors.go index 950ce5d..57fc400 100644 --- a/pkg/api/errors.go +++ b/pkg/api/errors.go @@ -67,7 +67,7 @@ func (gr GQLError) Error() string { } errorMessages = append(errorMessages, msg) } - return fmt.Sprintf("GQL: %s", strings.Join(errorMessages, ", ")) + return fmt.Sprintf("GraphQL: %s", strings.Join(errorMessages, ", ")) } // Match determines if the GQLError is about a specific type on a specific path. From f04a06536cd6bf36af09779ac56cfa5ad10a5dbf Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 14 Jun 2022 09:03:53 -0500 Subject: [PATCH 2/4] Remove old preview headers --- internal/api/http.go | 4 ---- internal/api/http_test.go | 2 -- 2 files changed, 6 deletions(-) diff --git a/internal/api/http.go b/internal/api/http.go index 4c0e277..17d2997 100644 --- a/internal/api/http.go +++ b/internal/api/http.go @@ -168,10 +168,6 @@ func newHeaderRoundTripper(host string, authToken string, headers map[string]str a := "application/vnd.github.merge-info-preview+json" // Preview for visibility when RESTing repos into an org. a += ", application/vnd.github.nebula-preview" - // Preview for Commit.statusCheckRollup for old GHES versions. - a += ", application/vnd.github.antiope-preview" - // Preview for // PullRequest.isDraft for old GHES versions. - a += ", application/vnd.github.shadow-cat-preview" headers[accept] = a } return headerRoundTripper{host: host, headers: headers, rt: rt} diff --git a/internal/api/http_test.go b/internal/api/http_test.go index 4a789d4..efeb65f 100644 --- a/internal/api/http_test.go +++ b/internal/api/http_test.go @@ -133,8 +133,6 @@ func defaultHeaders() http.Header { h := http.Header{} a := "application/vnd.github.merge-info-preview+json" a += ", application/vnd.github.nebula-preview" - a += ", application/vnd.github.antiope-preview" - a += ", application/vnd.github.shadow-cat-preview" h.Set(contentType, jsonContentType) h.Set(userAgent, "go-gh") h.Set(authorization, fmt.Sprintf("token %s", "oauth_token")) From 4397a6728d9a15a93e30aa5e973ed007b15dd6ee Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 14 Jun 2022 09:15:30 -0500 Subject: [PATCH 3/4] Fix up local test failure --- pkg/repository/repository_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/repository/repository_test.go b/pkg/repository/repository_test.go index 3af334f..c977b69 100644 --- a/pkg/repository/repository_test.go +++ b/pkg/repository/repository_test.go @@ -80,6 +80,9 @@ func TestParse(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + oldDir := os.Getenv("GH_CONFIG_DIR") + os.Setenv("GH_CONFIG_DIR", "nonexistant") + defer os.Setenv("GH_CONFIG_DIR", oldDir) if tt.hostOverride != "" { old := os.Getenv("GH_HOST") os.Setenv("GH_HOST", tt.hostOverride) From ffe65c2379b4e3ed438f24a46c4b2b6ad4488aed Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 14 Jun 2022 11:47:47 -0500 Subject: [PATCH 4/4] Add ability to override api cache options with request headers --- internal/api/cache.go | 30 ++++++++++++ internal/api/cache_test.go | 93 ++++++++++++++++++++++++++++++++++++++ internal/api/http.go | 16 +++---- 3 files changed, 130 insertions(+), 9 deletions(-) diff --git a/internal/api/cache.go b/internal/api/cache.go index 2878404..74542a7 100644 --- a/internal/api/cache.go +++ b/internal/api/cache.go @@ -82,10 +82,25 @@ func (c cache) RoundTripper(rt http.RoundTripper) http.RoundTripper { } func (crt cacheRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + reqDir, reqTTL := requestCacheOptions(req) + + if crt.fs.ttl == 0 && reqTTL == 0 { + return crt.rt.RoundTrip(req) + } + if !isCacheableRequest(req) { return crt.rt.RoundTrip(req) } + origDir := crt.fs.dir + if reqDir != "" { + crt.fs.dir = reqDir + } + origTTL := crt.fs.ttl + if reqTTL != 0 { + crt.fs.ttl = reqTTL + } + key, keyErr := cacheKey(req) if keyErr == nil { if res, err := crt.fs.read(key); err == nil { @@ -98,9 +113,24 @@ func (crt cacheRoundTripper) RoundTrip(req *http.Request) (*http.Response, error if err == nil && keyErr == nil && isCacheableResponse(res) { _ = crt.fs.store(key, res) } + + crt.fs.dir = origDir + crt.fs.ttl = origTTL + return res, err } +// Allow an individual request to override cache options. +func requestCacheOptions(req *http.Request) (string, time.Duration) { + var dur time.Duration + dir := req.Header.Get("X-GH-CACHE-DIR") + ttl := req.Header.Get("X-GH-CACHE-TTL") + if ttl != "" { + dur, _ = time.ParseDuration(ttl) + } + return dir, dur +} + func (fs *fileStorage) filePath(key string) string { if len(key) >= 6 { return filepath.Join(fs.dir, key[0:2], key[2:4], key[4:]) diff --git a/internal/api/cache_test.go b/internal/api/cache_test.go index 5a0bad1..1142f3b 100644 --- a/internal/api/cache_test.go +++ b/internal/api/cache_test.go @@ -7,6 +7,7 @@ import ( "net/http" "path/filepath" "testing" + "time" "github.com/cli/go-gh/pkg/api" "github.com/stretchr/testify/assert" @@ -91,3 +92,95 @@ func TestCacheResponse(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "7: GET http://example.com/error", res) } + +func TestCacheResponseRequestCacheOptions(t *testing.T) { + counter := 0 + fakeHTTP := tripper{ + roundTrip: func(req *http.Request) (*http.Response, error) { + counter += 1 + body := fmt.Sprintf("%d: %s %s", counter, req.Method, req.URL.String()) + status := 200 + if req.URL.Path == "/error" { + status = 500 + } + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(bytes.NewBufferString(body)), + }, nil + }, + } + + cacheDir := filepath.Join(t.TempDir(), "gh-cli-cache") + + httpClient := NewHTTPClient( + &api.ClientOptions{ + Transport: fakeHTTP, + EnableCache: false, + CacheDir: cacheDir, + }) + + do := func(method, url string, body io.Reader) (string, error) { + req, err := http.NewRequest(method, url, body) + if err != nil { + return "", err + } + req.Header.Set("X-GH-CACHE-DIR", cacheDir) + req.Header.Set("X-GH-CACHE-TTL", "1h") + res, err := httpClient.Do(req) + if err != nil { + return "", err + } + defer res.Body.Close() + resBody, err := io.ReadAll(res.Body) + if err != nil { + err = fmt.Errorf("ReadAll: %w", err) + } + return string(resBody), err + } + + var res string + var err error + + res, err = do("GET", "http://example.com/path", nil) + assert.NoError(t, err) + assert.Equal(t, "1: GET http://example.com/path", res) + res, err = do("GET", "http://example.com/path", nil) + assert.NoError(t, err) + assert.Equal(t, "1: GET http://example.com/path", res) + + res, err = do("GET", "http://example.com/path2", nil) + assert.NoError(t, err) + assert.Equal(t, "2: GET http://example.com/path2", res) + + res, err = do("POST", "http://example.com/path2", nil) + assert.NoError(t, err) + assert.Equal(t, "3: POST http://example.com/path2", res) + + res, err = do("POST", "http://example.com/graphql", bytes.NewBufferString(`hello`)) + assert.NoError(t, err) + assert.Equal(t, "4: POST http://example.com/graphql", res) + res, err = do("POST", "http://example.com/graphql", bytes.NewBufferString(`hello`)) + assert.NoError(t, err) + assert.Equal(t, "4: POST http://example.com/graphql", res) + + res, err = do("POST", "http://example.com/graphql", bytes.NewBufferString(`hello2`)) + assert.NoError(t, err) + assert.Equal(t, "5: POST http://example.com/graphql", res) + + res, err = do("GET", "http://example.com/error", nil) + assert.NoError(t, err) + assert.Equal(t, "6: GET http://example.com/error", res) + res, err = do("GET", "http://example.com/error", nil) + assert.NoError(t, err) + assert.Equal(t, "7: GET http://example.com/error", res) +} + +func TestRequestCacheOptions(t *testing.T) { + req, err := http.NewRequest("GET", "some/url", nil) + assert.NoError(t, err) + req.Header.Set("X-GH-CACHE-DIR", "some/dir/path") + req.Header.Set("X-GH-CACHE-TTL", "1h") + dir, ttl := requestCacheOptions(req) + assert.Equal(t, dir, "some/dir/path") + assert.Equal(t, ttl, time.Hour) +} diff --git a/internal/api/http.go b/internal/api/http.go index 17d2997..ef27f83 100644 --- a/internal/api/http.go +++ b/internal/api/http.go @@ -83,16 +83,14 @@ func NewHTTPClient(opts *api.ClientOptions) http.Client { transport = opts.Transport } - if opts.EnableCache { - if opts.CacheDir == "" { - opts.CacheDir = filepath.Join(os.TempDir(), "gh-cli-cache") - } - if opts.CacheTTL == 0 { - opts.CacheTTL = time.Hour * 24 - } - c := cache{dir: opts.CacheDir, ttl: opts.CacheTTL} - transport = c.RoundTripper(transport) + if opts.CacheDir == "" { + opts.CacheDir = filepath.Join(os.TempDir(), "gh-cli-cache") + } + if opts.EnableCache && opts.CacheTTL == 0 { + opts.CacheTTL = time.Hour * 24 } + c := cache{dir: opts.CacheDir, ttl: opts.CacheTTL} + transport = c.RoundTripper(transport) if opts.Log != nil { logger := &httpretty.Logger{