add some more tests

coder · sreya · Aug 29, 2022 · Aug 24, 2022 · Aug 24, 2022 · Aug 25, 2022
commit 5d37630c71ff56cae658f4997af750fb84caad07
diff --git a/coderd/httpapi/status_writer.go b/coderd/httpapi/status_writer.go
@@ -4,6 +4,8 @@ import (
 	"bufio"
 	"net"
 	"net/http"
+
+	"golang.org/x/xerrors"
 )
 
 var _ http.ResponseWriter = (*StatusWriter)(nil)
@@ -17,32 +19,38 @@ type StatusWriter struct {
 	Status       int
 	Hijacked     bool
 	ResponseBody []byte
+
+	wroteHeader bool
 }
 
 func (w *StatusWriter) WriteHeader(status int) {
-	w.Status = status
-	w.ResponseWriter.WriteHeader(status)
+	if !w.wroteHeader {
+		w.Status = status
+		w.wroteHeader = true
+		w.ResponseWriter.WriteHeader(status)
+	}
 }
 
 func (w *StatusWriter) Write(b []byte) (int, error) {
 	const maxBodySize = 4096
 
-	if w.Status == 0 {
+	if !w.wroteHeader {
 		w.Status = http.StatusOK
-		w.Status = http.StatusOK
+		w.Status = http.StatusOK
+		w.wroteHeader = true
-		w.Status = http.StatusOK
+		w.Status = http.StatusOK
+		w.wroteHeader = true
 	}
 
 	if w.Status >= http.StatusBadRequest {
-		// Instantiate the recorded response body to be at most
-		// maxBodySize length.
+		// This is technically wrong as multiple calls to write
+		// will simply overwrite w.ResponseBody but given that
+		// we typically only write to the response body once
+		// and this field is only used for logging I'm leaving
+		// this as-is.
 		w.ResponseBody = make([]byte, minInt(len(b), maxBodySize))
 		copy(w.ResponseBody, b)
 	}
 
 	return w.ResponseWriter.Write(b)
 }
 
-// minInt returns the smaller of a or b. This is helpful because math.Min only
-// works with float64s.
 func minInt(a, b int) int {
 	if a < b {
 		return a
@@ -52,5 +60,10 @@ func minInt(a, b int) int {
 
 func (w *StatusWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
 	w.Hijacked = true
-	return w.ResponseWriter.(http.Hijacker).Hijack()
+	hijacker, ok := w.ResponseWriter.(http.Hijacker)
+	if !ok {
+		return nil, nil, xerrors.Errorf("%T is not a http.Hijacker", w.ResponseWriter)
+	}
+
+	return hijacker.Hijack()
 }
diff --git a/coderd/httpapi/status_writer_test.go b/coderd/httpapi/status_writer_test.go
@@ -0,0 +1,129 @@
+package httpapi_test
+
+import (
+	"bufio"
+	"crypto/rand"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
+
+	"github.com/coder/coder/coderd/httpapi"
+)
+
+func TestStatusWriter(t *testing.T) {
+	t.Parallel()
+
+	t.Run("WriteHeader", func(t *testing.T) {
+		t.Parallel()
+
+		var (
+			rec = httptest.NewRecorder()
+			w   = &httpapi.StatusWriter{ResponseWriter: rec}
+		)
+
+		w.WriteHeader(http.StatusOK)
+		require.Equal(t, http.StatusOK, w.Status)
+		// Validate that the code is written to the underlying Response.
+		require.Equal(t, http.StatusOK, rec.Code)
+	})
+
+	t.Run("WriteHeaderTwice", func(t *testing.T) {
+		t.Parallel()
+
+		var (
+			rec  = httptest.NewRecorder()
+			w    = &httpapi.StatusWriter{ResponseWriter: rec}
+			code = http.StatusNotFound
+		)
+
+		w.WriteHeader(code)
+		w.WriteHeader(http.StatusOK)
+		// Validate that we only record the first status code.
+		require.Equal(t, code, w.Status)
+		// Validate that the code is written to the underlying Response.
+		require.Equal(t, code, rec.Code)
+	})
+
+	t.Run("WriteNoHeader", func(t *testing.T) {
+		t.Parallel()
+		var (
+			rec  = httptest.NewRecorder()
+			w    = &httpapi.StatusWriter{ResponseWriter: rec}
+			body = []byte("hello")
+		)
+
+		_, err := w.Write(body)
+		require.NoError(t, err)
+
+		// Should set the status to OK.
+		require.Equal(t, http.StatusOK, w.Status)
+		// We don't record the body for codes <400.
+		require.Equal(t, []byte(nil), w.ResponseBody)
+		require.Equal(t, body, rec.Body.Bytes())
+	})
+
+	t.Run("WriteAfterHeader", func(t *testing.T) {
+		t.Parallel()
+		var (
+			rec  = httptest.NewRecorder()
+			w    = &httpapi.StatusWriter{ResponseWriter: rec}
+			body = []byte("hello")
+			code = http.StatusInternalServerError
+		)
+
+		w.WriteHeader(code)
+		_, err := w.Write(body)
+		require.NoError(t, err)
+
+		require.Equal(t, code, w.Status)
+		require.Equal(t, body, w.ResponseBody)
+		require.Equal(t, body, rec.Body.Bytes())
+	})
+
+	t.Run("WriteMaxBody", func(t *testing.T) {
+		t.Parallel()
+		var (
+			rec = httptest.NewRecorder()
+			w   = &httpapi.StatusWriter{ResponseWriter: rec}
+			// 8kb body.
+			body = make([]byte, 8<<10)
+			code = http.StatusInternalServerError
+		)
+
+		_, err := rand.Read(body)
+		require.NoError(t, err)
+
+		w.WriteHeader(code)
+		_, err = w.Write(body)
+		require.NoError(t, err)
+
+		require.Equal(t, code, w.Status)
+		require.Equal(t, body, rec.Body.Bytes())
+		require.Equal(t, body[:4096], w.ResponseBody)
+	})
+
+	t.Run("Hijack", func(t *testing.T) {
+		t.Parallel()
+		var (
+			rec = httptest.NewRecorder()
+		)
+
+		w := &httpapi.StatusWriter{ResponseWriter: hijacker{rec}}
+
+		_, _, err := w.Hijack()
+		require.Error(t, err)
+		require.Equal(t, "hijacked", err.Error())
+	})
+}
+
+type hijacker struct {
+	http.ResponseWriter
+}
+
+func (hijacker) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+	return nil, nil, xerrors.New("hijacked")
+}
diff --git a/coderd/httpmw/recover_test.go b/coderd/httpmw/recover_test.go
@@ -13,6 +13,8 @@ import (
 )
 
 func TestRecover(t *testing.T) {
+	t.Parallel()
+
 	handler := func(isPanic, hijack bool) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			if isPanic {