From cab6f92bca8e052e9d80bd511b99c2e45905d41b Mon Sep 17 00:00:00 2001 From: Dominik Roos Date: Tue, 2 Feb 2021 07:22:31 +0100 Subject: [PATCH] http: support additional content type (#903) Support `application/x-www-form-urlencoded` as an additional content type for `AtomicLevel.ServeHTTP`. This is the default content type for `curl -X POST`. With this change, interacting with the HTTP endpoint is a bit more user friendly: ``` curl -X PUT localhost:8080/log/level -d level=debug ``` Additionally, the unit tests for the HTTP handler are transformed to a table driven approach. fixes #902 Co-authored-by: Abhinav Gupta --- http_handler.go | 99 ++++++++++++----- http_handler_test.go | 249 ++++++++++++++++++++++++++----------------- 2 files changed, 229 insertions(+), 119 deletions(-) diff --git a/http_handler.go b/http_handler.go index 1b0ecaca9..1297c33b3 100644 --- a/http_handler.go +++ b/http_handler.go @@ -23,6 +23,7 @@ package zap import ( "encoding/json" "fmt" + "io" "net/http" "go.uber.org/zap/zapcore" @@ -31,47 +32,63 @@ import ( // ServeHTTP is a simple JSON endpoint that can report on or change the current // logging level. // -// GET requests return a JSON description of the current logging level. PUT -// requests change the logging level and expect a payload like: +// GET +// +// The GET request returns a JSON description of the current logging level like: // {"level":"info"} // -// It's perfectly safe to change the logging level while a program is running. +// PUT +// +// The PUT request changes the logging level. It is perfectly safe to change the +// logging level while a program is running. Two content types are supported: +// +// Content-Type: application/x-www-form-urlencoded +// +// With this content type, the level can be provided through the request body or +// a query parameter. The log level is URL encoded like: +// +// level=debug +// +// The request body takes precedence over the query parameter, if both are +// specified. +// +// This content type is the default for a curl PUT request. Following are two +// example curl requests that both set the logging level to debug. +// +// curl -X PUT localhost:8080/log/level?level=debug +// curl -X PUT localhost:8080/log/level -d level=debug +// +// For any other content type, the payload is expected to be JSON encoded and +// look like: +// +// {"level":"info"} +// +// An example curl request could look like this: +// +// curl -X PUT localhost:8080/log/level -H "Content-Type: application/json" -d '{"level":"debug"}' +// func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { type errorResponse struct { Error string `json:"error"` } type payload struct { - Level *zapcore.Level `json:"level"` + Level zapcore.Level `json:"level"` } enc := json.NewEncoder(w) switch r.Method { - case http.MethodGet: - current := lvl.Level() - enc.Encode(payload{Level: ¤t}) - + enc.Encode(payload{Level: lvl.Level()}) case http.MethodPut: - var req payload - - if errmess := func() string { - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - return fmt.Sprintf("Request body must be well-formed JSON: %v", err) - } - if req.Level == nil { - return "Must specify a logging level." - } - return "" - }(); errmess != "" { + requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r) + if err != nil { w.WriteHeader(http.StatusBadRequest) - enc.Encode(errorResponse{Error: errmess}) + enc.Encode(errorResponse{Error: err.Error()}) return } - - lvl.SetLevel(*req.Level) - enc.Encode(req) - + lvl.SetLevel(requestedLvl) + enc.Encode(payload{Level: lvl.Level()}) default: w.WriteHeader(http.StatusMethodNotAllowed) enc.Encode(errorResponse{ @@ -79,3 +96,37 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { }) } } + +// Decodes incoming PUT requests and returns the requested logging level. +func decodePutRequest(contentType string, r *http.Request) (zapcore.Level, error) { + if contentType == "application/x-www-form-urlencoded" { + return decodePutURL(r) + } + return decodePutJSON(r.Body) +} + +func decodePutURL(r *http.Request) (zapcore.Level, error) { + lvl := r.FormValue("level") + if lvl == "" { + return 0, fmt.Errorf("must specify logging level") + } + var l zapcore.Level + if err := l.UnmarshalText([]byte(lvl)); err != nil { + return 0, err + } + return l, nil +} + +func decodePutJSON(body io.Reader) (zapcore.Level, error) { + var pld struct { + Level *zapcore.Level `json:"level"` + } + if err := json.NewDecoder(body).Decode(&pld); err != nil { + return 0, fmt.Errorf("malformed request body: %v", err) + } + if pld.Level == nil { + return 0, fmt.Errorf("must specify logging level") + } + return *pld.Level, nil + +} diff --git a/http_handler_test.go b/http_handler_test.go index 474b3c7cd..9fa9c64c1 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -22,110 +22,169 @@ package zap_test import ( "encoding/json" - "fmt" - "io" - "io/ioutil" "net/http" "net/http/httptest" "strings" "testing" - . "go.uber.org/zap" + "go.uber.org/zap" "go.uber.org/zap/zapcore" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func newHandler() (AtomicLevel, *Logger) { - lvl := NewAtomicLevel() - logger := New(zapcore.NewNopCore()) - return lvl, logger -} - -func assertCodeOK(t testing.TB, code int) { - assert.Equal(t, http.StatusOK, code, "Unexpected response status code.") -} - -func assertCodeBadRequest(t testing.TB, code int) { - assert.Equal(t, http.StatusBadRequest, code, "Unexpected response status code.") -} - -func assertCodeMethodNotAllowed(t testing.TB, code int) { - assert.Equal(t, http.StatusMethodNotAllowed, code, "Unexpected response status code.") -} - -func assertResponse(t testing.TB, expectedLevel zapcore.Level, actualBody string) { - assert.Equal(t, fmt.Sprintf(`{"level":"%s"}`, expectedLevel)+"\n", actualBody, "Unexpected response body.") -} - -func assertJSONError(t testing.TB, body string) { - // Don't need to test exact error message, but one should be present. - var payload map[string]interface{} - require.NoError(t, json.Unmarshal([]byte(body), &payload), "Expected error response to be JSON.") - - msg, ok := payload["error"] - require.True(t, ok, "Error message is an unexpected type.") - assert.NotEqual(t, "", msg, "Expected an error message in response.") -} - -func makeRequest(t testing.TB, method string, handler http.Handler, reader io.Reader) (int, string) { - ts := httptest.NewServer(handler) - defer ts.Close() - - req, err := http.NewRequest(method, ts.URL, reader) - require.NoError(t, err, "Error constructing %s request.", method) - - res, err := http.DefaultClient.Do(req) - require.NoError(t, err, "Error making %s request.", method) - defer res.Body.Close() - - body, err := ioutil.ReadAll(res.Body) - require.NoError(t, err, "Error reading request body.") - - return res.StatusCode, string(body) -} - -func TestHTTPHandlerGetLevel(t *testing.T) { - lvl, _ := newHandler() - code, body := makeRequest(t, "GET", lvl, nil) - assertCodeOK(t, code) - assertResponse(t, lvl.Level(), body) -} - -func TestHTTPHandlerPutLevel(t *testing.T) { - lvl, _ := newHandler() - - code, body := makeRequest(t, "PUT", lvl, strings.NewReader(`{"level":"warn"}`)) - - assertCodeOK(t, code) - assertResponse(t, lvl.Level(), body) -} - -func TestHTTPHandlerPutUnrecognizedLevel(t *testing.T) { - lvl, _ := newHandler() - code, body := makeRequest(t, "PUT", lvl, strings.NewReader(`{"level":"unrecognized-level"}`)) - assertCodeBadRequest(t, code) - assertJSONError(t, body) -} - -func TestHTTPHandlerNotJSON(t *testing.T) { - lvl, _ := newHandler() - code, body := makeRequest(t, "PUT", lvl, strings.NewReader(`{`)) - assertCodeBadRequest(t, code) - assertJSONError(t, body) -} - -func TestHTTPHandlerNoLevelSpecified(t *testing.T) { - lvl, _ := newHandler() - code, body := makeRequest(t, "PUT", lvl, strings.NewReader(`{}`)) - assertCodeBadRequest(t, code) - assertJSONError(t, body) -} - -func TestHTTPHandlerMethodNotAllowed(t *testing.T) { - lvl, _ := newHandler() - code, body := makeRequest(t, "POST", lvl, strings.NewReader(`{`)) - assertCodeMethodNotAllowed(t, code) - assertJSONError(t, body) +func TestAtomicLevelServeHTTP(t *testing.T) { + tests := []struct { + desc string + method string + query string + contentType string + body string + expectedCode int + expectedLevel zapcore.Level + }{ + { + desc: "GET", + method: http.MethodGet, + expectedCode: http.StatusOK, + expectedLevel: zap.InfoLevel, + }, + { + desc: "PUT JSON", + method: http.MethodPut, + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + body: `{"level":"warn"}`, + }, + { + desc: "PUT URL encoded", + method: http.MethodPut, + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + contentType: "application/x-www-form-urlencoded", + body: "level=warn", + }, + { + desc: "PUT query parameters", + method: http.MethodPut, + query: "?level=warn", + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + contentType: "application/x-www-form-urlencoded", + }, + { + desc: "body takes precedence over query", + method: http.MethodPut, + query: "?level=info", + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + contentType: "application/x-www-form-urlencoded", + body: "level=warn", + }, + { + desc: "JSON ignores query", + method: http.MethodPut, + query: "?level=info", + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + body: `{"level":"warn"}`, + }, + { + desc: "PUT JSON unrecognized", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{"level":"unrecognized"}`, + }, + { + desc: "PUT URL encoded unrecognized", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "level=unrecognized", + }, + { + desc: "PUT JSON malformed", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{"level":"warn`, + }, + { + desc: "PUT URL encoded malformed", + method: http.MethodPut, + query: "?level=%", + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + }, + { + desc: "PUT Query parameters malformed", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "level=%", + }, + { + desc: "PUT JSON unspecified", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{}`, + }, + { + desc: "PUT URL encoded unspecified", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "", + }, + { + desc: "POST JSON", + method: http.MethodPost, + expectedCode: http.StatusMethodNotAllowed, + body: `{"level":"warn"}`, + }, + { + desc: "POST URL", + method: http.MethodPost, + expectedCode: http.StatusMethodNotAllowed, + contentType: "application/x-www-form-urlencoded", + body: "level=warn", + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + lvl := zap.NewAtomicLevel() + lvl.SetLevel(zapcore.InfoLevel) + + server := httptest.NewServer(lvl) + defer server.Close() + + req, err := http.NewRequest(tt.method, server.URL+tt.query, strings.NewReader(tt.body)) + require.NoError(t, err, "Error constructing %s request.", req.Method) + if tt.contentType != "" { + req.Header.Set("Content-Type", tt.contentType) + } + + res, err := http.DefaultClient.Do(req) + require.NoError(t, err, "Error making %s request.", req.Method) + defer res.Body.Close() + + require.Equal(t, tt.expectedCode, res.StatusCode, "Unexpected status code.") + if tt.expectedCode != http.StatusOK { + // Don't need to test exact error message, but one should be present. + var pld struct { + Error string `json:"error"` + } + require.NoError(t, json.NewDecoder(res.Body).Decode(&pld), "Decoding response body") + assert.NotEmpty(t, pld.Error, "Expected an error message") + return + } + + var pld struct { + Level zapcore.Level `json:"level"` + } + require.NoError(t, json.NewDecoder(res.Body).Decode(&pld), "Decoding response body") + assert.Equal(t, tt.expectedLevel, pld.Level, "Unexpected logging level returned") + }) + } }