tsweb: make JSONHandlerFunc implement ReturnHandler, not http.Handler

This way something is capable of logging errors on the server.

Fixes #766

Signed-off-by: David Crawshaw <crawshaw@tailscale.com>

2 files changed, 31 insertions, 22 deletions

diff --git a/tsweb/jsonhandler.go b/tsweb/jsonhandler.go
index 8602a9ad9..56704da7c 100644
--- a/tsweb/jsonhandler.go
+++ b/tsweb/jsonhandler.go
@@ -6,6 +6,7 @@ package tsweb
 
 import (
 	"encoding/json"
+	"fmt"
 	"net/http"
 )
 
@@ -15,23 +16,23 @@ type response struct {
 	Data   interface{} `json:"data,omitempty"`
 }
 
-// TODO: Header
-
-// JSONHandlerFunc only take *http.Request as argument to avoid any misuse of http.ResponseWriter.
-// The function's results must be (status int, data interface{}, err error).
-// Return a HTTPError to show an error message, otherwise JSONHandler will only show "internal server error".
+// JSONHandlerFunc is an HTTP ReturnHandler that writes JSON responses to the client.
+//
+// Return a HTTPError to show an error message, otherwise JSONHandlerFunc will
+// only report "internal server error" to the user.
 type JSONHandlerFunc func(r *http.Request) (status int, data interface{}, err error)
 
-// ServeHTTP calls the JSONHandlerFunc and automatically marshals http responses.
+// ServeHTTPReturn implements the ReturnHandler interface.
 //
 // Use the following code to unmarshal the request body
+//
 //	body := new(DataType)
 //	if err := json.NewDecoder(r.Body).Decode(body); err != nil {
 //	  return http.StatusBadRequest, nil, err
 //	}
 //
-// Check jsonhandler_text.go for examples
-func (fn JSONHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+// See jsonhandler_text.go for examples.
+func (fn JSONHandlerFunc) ServeHTTPReturn(w http.ResponseWriter, r *http.Request) error {
 	w.Header().Set("Content-Type", "application/json")
 	var resp *response
 	status, data, err := fn(r)
@@ -53,6 +54,10 @@ func (fn JSONHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 				Error:  werr.Msg,
 				Data:   data,
 			}
+			// Unwrap the HTTPError here because we are communicating with
+			// the client in this handler. We don't want the wrapping
+			// ReturnHandler to do it too.
+			err = werr.Err
 		} else {
 			resp = &response{
 				Status: "error",
@@ -61,13 +66,17 @@ func (fn JSONHandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	b, err := json.Marshal(resp)
-	if err != nil {
+	b, jerr := json.Marshal(resp)
+	if jerr != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		w.Write([]byte(`{"status":"error","error":"json marshal error"}`))
-		return
+		if err != nil {
+			return fmt.Errorf("%w, and then we could not respond: %v", err, jerr)
+		}
+		return jerr
 	}
 
 	w.WriteHeader(status)
 	w.Write(b)
+	return err
 }
diff --git a/tsweb/jsonhandler_test.go b/tsweb/jsonhandler_test.go
index be7d9d215..d7032f1c2 100644
--- a/tsweb/jsonhandler_test.go
+++ b/tsweb/jsonhandler_test.go
@@ -61,7 +61,7 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("200 simple", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("GET", "/", nil)
-		h21.ServeHTTP(w, r)
+		h21.ServeHTTPReturn(w, r)
 		checkStatus(w, "success", http.StatusOK)
 	})
 
@@ -72,7 +72,7 @@ func TestNewJSONHandler(t *testing.T) {
 
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("GET", "/", nil)
-		h.ServeHTTP(w, r)
+		h.ServeHTTPReturn(w, r)
 		checkStatus(w, "error", http.StatusForbidden)
 	})
 
@@ -83,7 +83,7 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("200 get data", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("GET", "/", nil)
-		h22.ServeHTTP(w, r)
+		h22.ServeHTTPReturn(w, r)
 		checkStatus(w, "success", http.StatusOK)
 	})
 
@@ -102,21 +102,21 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("200 post data", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{"Name": "tailscale"}`))
-		h31.ServeHTTP(w, r)
+		h31.ServeHTTPReturn(w, r)
 		checkStatus(w, "success", http.StatusOK)
 	})
 
 	t.Run("400 bad json", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{`))
-		h31.ServeHTTP(w, r)
+		h31.ServeHTTPReturn(w, r)
 		checkStatus(w, "error", http.StatusBadRequest)
 	})
 
 	t.Run("400 post data error", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{}`))
-		h31.ServeHTTP(w, r)
+		h31.ServeHTTPReturn(w, r)
 		resp := checkStatus(w, "error", http.StatusBadRequest)
 		if resp.Error != "name is empty" {
 			t.Fatalf("wrong error")
@@ -141,7 +141,7 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("200 post data", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{"Price": 10}`))
-		h32.ServeHTTP(w, r)
+		h32.ServeHTTPReturn(w, r)
 		resp := checkStatus(w, "success", http.StatusOK)
 		t.Log(resp.Data)
 		if resp.Data.Price != 20 {
@@ -152,7 +152,7 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("400 post data error", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{}`))
-		h32.ServeHTTP(w, r)
+		h32.ServeHTTPReturn(w, r)
 		resp := checkStatus(w, "error", http.StatusBadRequest)
 		if resp.Error != "price is empty" {
 			t.Fatalf("wrong error")
@@ -162,7 +162,7 @@ func TestNewJSONHandler(t *testing.T) {
 	t.Run("500 internal server error", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		r := httptest.NewRequest("POST", "/", strings.NewReader(`{"Name": "root"}`))
-		h32.ServeHTTP(w, r)
+		h32.ServeHTTPReturn(w, r)
 		resp := checkStatus(w, "error", http.StatusInternalServerError)
 		if resp.Error != "internal server error" {
 			t.Fatalf("wrong error")
@@ -174,7 +174,7 @@ func TestNewJSONHandler(t *testing.T) {
 		r := httptest.NewRequest("POST", "/", nil)
 		JSONHandlerFunc(func(r *http.Request) (int, interface{}, error) {
 			return http.StatusOK, make(chan int), nil
-		}).ServeHTTP(w, r)
+		}).ServeHTTPReturn(w, r)
 		resp := checkStatus(w, "error", http.StatusInternalServerError)
 		if resp.Error != "json marshal error" {
 			t.Fatalf("wrong error")
@@ -186,7 +186,7 @@ func TestNewJSONHandler(t *testing.T) {
 		r := httptest.NewRequest("POST", "/", nil)
 		JSONHandlerFunc(func(r *http.Request) (status int, data interface{}, err error) {
 			return
-		}).ServeHTTP(w, r)
+		}).ServeHTTPReturn(w, r)
 		checkStatus(w, "error", http.StatusInternalServerError)
 	})
 }