From dea3ef0597d39616276495bab40366cfafc22925 Mon Sep 17 00:00:00 2001 From: David Crawshaw Date: Thu, 17 Sep 2020 08:56:14 -0400 Subject: [PATCH] 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 --- tsweb/jsonhandler.go | 31 ++++++++++++++++++++----------- tsweb/jsonhandler_test.go | 22 +++++++++++----------- 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) }) }