Repository: danielgtaylor/restish
Pull request #2 (2020-06-29 21:25:47)

fix: do not cache error responses
This fixes a bug where a temporarily broken API route to get the service description could result in a 24-hour cache of the *error* and needing to clear the cache before being able to use the API again. The easy fix is to just never cache error responses when trying to refresh the API description document.


Patch:
diff --git a/cli/transport.go b/cli/transport.go
index f52bd02..def5fe7 100644
--- a/cli/transport.go
+++ b/cli/transport.go
@@ -21,6 +21,27 @@ func cacheKey(req *http.Request) string {
 	return req.Method + " " + req.URL.String()
 }
 
+// shouldCache returns whether a response should be manually cached.
+func shouldCache(resp *http.Response) bool {
+	// Error responses should not be cached.
+	if resp.StatusCode >= 400 {
+		return false
+	}
+
+	// The older "Expires" header means we should not touch it.
+	if resp.Header.Get("expires") != "" {
+		return false
+	}
+
+	// There is a "Cache-Control" header *AND* it has a cache age set, so we
+	// should not touch it.
+	if strings.Contains(resp.Header.Get("cache-control"), "max-age") {
+		return false
+	}
+
+	return true
+}
+
 // CachedTransport returns an HTTP transport with caching abilities.
 func CachedTransport() *httpcache.Transport {
 	t := httpcache.NewTransport(diskcache.New(path.Join(cacheDir(), "responses")))
@@ -38,7 +59,9 @@ func (m minCachedTransport) RoundTrip(req *http.Request) (*http.Response, error)
 		return nil, err
 	}
 
-	if resp.Header.Get("expires") == "" && !strings.Contains(resp.Header.Get("cache-control"), "max-age") {
+	// Automatically cache for the minimum time if the request is successful and
+	// the response doesn't already have cache headers.
+	if shouldCache(resp) {
 		// Add the minimum max-age.
 		ma := fmt.Sprintf("max-age=%d", int(m.min.Seconds()))
 		if cc := resp.Header.Get("cache-control"); cc != "" {


Test patch:
diff --git a/cli/transport_test.go b/cli/transport_test.go
new file mode 100644
index 0000000..71465e7
--- /dev/null
+++ b/cli/transport_test.go
@@ -0,0 +1,63 @@
+package cli
+
+import (
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"gopkg.in/h2non/gock.v1"
+)
+
+func TestMinCachedTransport(t *testing.T) {
+	defer gock.Off()
+
+	gock.New("http://example.com").Get("/success").Reply(200)
+	gock.New("http://example.com").Get("/cached").Reply(200).SetHeader("cache-control", "max-age=10")
+	gock.New("http://example.com").Get("/expires").Reply(200).SetHeader("expires", "Sun, 1 Jan 2020 12:00:00 GMT")
+	gock.New("http://example.com").Get("/modify").Reply(200).SetHeader("cache-control", "public")
+	gock.New("http://example.com").Get("/error").Reply(400)
+
+	tx := MinCachedTransport(1 * time.Hour)
+
+	// Missing cache, should get added.
+	req, _ := http.NewRequest(http.MethodGet, "http://example.com/success", nil)
+	resp, err := tx.RoundTrip(req)
+
+	assert.NoError(t, err)
+	assert.Equal(t, resp.StatusCode, 200)
+	assert.Equal(t, resp.Header.Get("cache-control"), "max-age=3600")
+
+	// Already-cached requests should not be touched.
+	req, _ = http.NewRequest(http.MethodGet, "http://example.com/cached", nil)
+	resp, err = tx.RoundTrip(req)
+
+	assert.NoError(t, err)
+	assert.Equal(t, resp.StatusCode, 200)
+	assert.Equal(t, resp.Header.Get("cache-control"), "max-age=10")
+
+	// Already-cached requests should not be touched.
+	req, _ = http.NewRequest(http.MethodGet, "http://example.com/expires", nil)
+	resp, err = tx.RoundTrip(req)
+
+	assert.NoError(t, err)
+	assert.Equal(t, resp.StatusCode, 200)
+	assert.NotEmpty(t, resp.Header.Get("expires"))
+	assert.Equal(t, resp.Header.Get("cache-control"), "")
+
+	// Already-set header should be modified instead of replaced.
+	req, _ = http.NewRequest(http.MethodGet, "http://example.com/modify", nil)
+	resp, err = tx.RoundTrip(req)
+
+	assert.NoError(t, err)
+	assert.Equal(t, resp.StatusCode, 200)
+	assert.Equal(t, resp.Header.Get("cache-control"), "public,max-age=3600")
+
+	// Errors should not get cache headers added.
+	req, _ = http.NewRequest(http.MethodGet, "http://example.com/error", nil)
+	resp, err = tx.RoundTrip(req)
+
+	assert.NoError(t, err)
+	assert.Equal(t, resp.StatusCode, 400)
+	assert.Equal(t, resp.Header.Get("cache-control"), "")
+}