Refrain from setting 200 OK on cached responses (#77)

In cases where responses were re-used, due to
validated 304 responses and requests using stale-if-error,
the cached responses had their status replaced by a 200,
which isn't correct.

Fixes #74

Author: gregjones

httpcache.go

@@ -10,7 +10,6 @@ import (
 	"bufio"
 	"bytes"
 	"errors"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -193,16 +192,11 @@ func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error
 			for _, header := range endToEndHeaders {
 				cachedResp.Header[header] = resp.Header[header]
 			}
-			cachedResp.Status = fmt.Sprintf("%d %s", http.StatusOK, http.StatusText(http.StatusOK))
-			cachedResp.StatusCode = http.StatusOK
-
 			resp = cachedResp
 		} else if (err != nil || (cachedResp != nil && resp.StatusCode >= 500)) &&
 			req.Method == "GET" && canStaleOnError(cachedResp.Header, req.Header) {
 			// In case of transport failure and stale-if-error activated, returns cached content
 			// when available
-			cachedResp.Status = fmt.Sprintf("%d %s", http.StatusOK, http.StatusText(http.StatusOK))
-			cachedResp.StatusCode = http.StatusOK
 			return cachedResp, nil
 		} else {
 			if err != nil || resp.StatusCode != http.StatusOK {

httpcache_test.go

@@ -120,6 +120,17 @@ func setup() {
 		w.Write([]byte("Some text content"))
 	}))
 
+	mux.HandleFunc("/cachederror", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		etag := "abc"
+		if r.Header.Get("if-none-match") == etag {
+			w.WriteHeader(http.StatusNotModified)
+			return
+		}
+		w.Header().Set("etag", etag)
+		w.WriteHeader(http.StatusNotFound)
+		w.Write([]byte("Not found"))
+	}))
+
 	updateFieldsCounter := 0
 	mux.HandleFunc("/updatefields", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("X-Counter", strconv.Itoa(updateFieldsCounter))
@@ -873,6 +884,35 @@ func TestUpdateFields(t *testing.T) {
 	}
 }
 
+// This tests the fix for https://github.com/gregjones/httpcache/issues/74.
+// Previously, after validating a cached response, its StatusCode
+// was incorrectly being replaced.
+func TestCachedErrorsKeepStatus(t *testing.T) {
+	resetTest()
+	req, err := http.NewRequest("GET", s.server.URL+"/cachederror", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	{
+		resp, err := s.client.Do(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer resp.Body.Close()
+		io.Copy(ioutil.Discard, resp.Body)
+	}
+	{
+		resp, err := s.client.Do(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer resp.Body.Close()
+		if resp.StatusCode != http.StatusNotFound {
+			t.Fatalf("Status code isn't 404: %d", resp.StatusCode)
+		}
+	}
+}
+
 func TestParseCacheControl(t *testing.T) {
 	resetTest()
 	h := http.Header{}
@@ -1355,6 +1395,57 @@ func TestStaleIfErrorResponseLifetime(t *testing.T) {
 	}
 }
 
+// This tests the fix for https://github.com/gregjones/httpcache/issues/74.
+// Previously, after a stale response was used after encountering an error,
+// its StatusCode was being incorrectly replaced.
+func TestStaleIfErrorKeepsStatus(t *testing.T) {
+	resetTest()
+	now := time.Now()
+	tmock := transportMock{
+		response: &http.Response{
+			Status:     http.StatusText(http.StatusNotFound),
+			StatusCode: http.StatusNotFound,
+			Header: http.Header{
+				"Date":          []string{now.Format(time.RFC1123)},
+				"Cache-Control": []string{"no-cache"},
+			},
+			Body: ioutil.NopCloser(bytes.NewBuffer([]byte("some data"))),
+		},
+		err: nil,
+	}
+	tp := NewMemoryCacheTransport()
+	tp.Transport = &tmock
+
+	// First time, response is cached on success
+	r, _ := http.NewRequest("GET", "http://somewhere.com/", nil)
+	r.Header.Set("Cache-Control", "stale-if-error")
+	resp, err := tp.RoundTrip(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil {
+		t.Fatal("resp is nil")
+	}
+	_, err = ioutil.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// On failure, response is returned from the cache
+	tmock.response = nil
+	tmock.err = errors.New("some error")
+	resp, err = tp.RoundTrip(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil {
+		t.Fatal("resp is nil")
+	}
+	if resp.StatusCode != http.StatusNotFound {
+		t.Fatalf("Status wasn't 404: %d", resp.StatusCode)
+	}
+}
+
 // Test that http.Client.Timeout is respected when cache transport is used.
 // That is so as long as request cancellation is propagated correctly.
 // In the past, that required CancelRequest to be implemented correctly,