From 756d5079803637c5cab717b32d2bdba955e83c5a Mon Sep 17 00:00:00 2001 From: Oncilla Date: Fri, 8 Jan 2021 19:41:19 +0100 Subject: [PATCH 1/6] http: support additional content type 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 --- http_handler.go | 83 ++++++++++++++---- http_handler_test.go | 204 +++++++++++++++++++++++-------------------- 2 files changed, 174 insertions(+), 113 deletions(-) diff --git a/http_handler.go b/http_handler.go index 1b0ecaca9..3a8b71300 100644 --- a/http_handler.go +++ b/http_handler.go @@ -23,7 +23,10 @@ package zap import ( "encoding/json" "fmt" + "io" + "io/ioutil" "net/http" + "net/url" "go.uber.org/zap/zapcore" ) @@ -31,11 +34,36 @@ 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 request body is expected to be URL encoded like: +// +// level=debug +// +// This is the default content type for a curl PUT request. An example curl +// request could look like this: +// +// 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"` @@ -53,25 +81,44 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { enc.Encode(payload{Level: ¤t}) 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." + requestedLvl, err := func(body io.Reader) (*zapcore.Level, error) { + switch r.Header.Get("Content-Type") { + case "application/x-www-form-urlencoded": + pld, err := ioutil.ReadAll(body) + if err != nil { + return nil, err + } + values, err := url.ParseQuery(string(pld)) + if err != nil { + return nil, err + } + lvl := values.Get("level") + if lvl == "" { + return nil, fmt.Errorf("must specify logging level") + } + var l zapcore.Level + if err := l.UnmarshalText([]byte(lvl)); err != nil { + return nil, err + } + return &l, nil + default: + var pld payload + if err := json.NewDecoder(r.Body).Decode(&pld); err != nil { + return nil, fmt.Errorf("malformed request body: %v", err) + } + if pld.Level == nil { + return nil, fmt.Errorf("must specify logging level") + } + return pld.Level, nil } - return "" - }(); errmess != "" { + }(r.Body) + 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: requestedLvl}) default: w.WriteHeader(http.StatusMethodNotAllowed) enc.Encode(errorResponse{ diff --git a/http_handler_test.go b/http_handler_test.go index 474b3c7cd..266d5fb18 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -22,110 +22,124 @@ 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 := map[string]struct { + Method string + ContentType string + Body string + ExpectedCode int + ExpectedLevel zapcore.Level + }{ + "GET": { + Method: http.MethodGet, + ExpectedCode: http.StatusOK, + ExpectedLevel: zap.InfoLevel, + }, + "PUT JSON": { + Method: http.MethodPut, + ExpectedCode: http.StatusOK, + ExpectedLevel: zap.WarnLevel, + Body: `{"level":"warn"}`, + }, + "PUT URL encoded": { + Method: http.MethodPut, + ExpectedCode: http.StatusOK, + ExpectedLevel: zap.WarnLevel, + ContentType: "application/x-www-form-urlencoded", + Body: "level=warn", + }, + "PUT JSON unrecognized": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + Body: `{"level":"unrecognized"}`, + }, + "PUT URL encoded unrecognized": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + ContentType: "application/x-www-form-urlencoded", + Body: "level=unrecognized", + }, + "PUT JSON malformed": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + Body: `{"level":"warn`, + }, + "PUT URL encoded malformed": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + ContentType: "application/x-www-form-urlencoded", + Body: "level", + }, + "PUT JSON unspecified": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + Body: `{}`, + }, + "PUT URL encoded unspecified": { + Method: http.MethodPut, + ExpectedCode: http.StatusBadRequest, + ContentType: "application/x-www-form-urlencoded", + Body: "", + }, + "POST JSON": { + Method: http.MethodPost, + ExpectedCode: http.StatusMethodNotAllowed, + Body: `{"level":"warn"}`, + }, + "POST URL": { + Method: http.MethodPost, + ExpectedCode: http.StatusMethodNotAllowed, + ContentType: "application/x-www-form-urlencoded", + Body: "level=warn", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + lvl := zap.NewAtomicLevel() + lvl.SetLevel(zapcore.InfoLevel) + + ts := httptest.NewServer(lvl) + defer ts.Close() + + req, err := http.NewRequest(test.Method, ts.URL, strings.NewReader(test.Body)) + require.NoError(t, err, "Error constructing %s request.", req.Method) + if test.ContentType != "" { + req.Header.Set("Content-Type", test.ContentType) + } + + res, err := http.DefaultClient.Do(req) + require.NoError(t, err, "Error making %s request.", req.Method) + defer res.Body.Close() + + require.Equal(t, test.ExpectedCode, res.StatusCode, "Unexpected status code.") + if test.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, test.ExpectedLevel, pld.Level, "Unexpected logging level returned") + }) + } } From 794c2dfc7c4afd7c378e4fc2518fb5c555c102e4 Mon Sep 17 00:00:00 2001 From: Oncilla Date: Fri, 8 Jan 2021 20:06:32 +0100 Subject: [PATCH 2/6] cover more error branches --- http_handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_handler_test.go b/http_handler_test.go index 266d5fb18..1abc499fe 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -80,7 +80,7 @@ func TestAtomicLevelServeHTTP(t *testing.T) { Method: http.MethodPut, ExpectedCode: http.StatusBadRequest, ContentType: "application/x-www-form-urlencoded", - Body: "level", + Body: "level=%", }, "PUT JSON unspecified": { Method: http.MethodPut, From e1be881e300720813e12ba8035e108733b19d802 Mon Sep 17 00:00:00 2001 From: Oncilla Date: Mon, 11 Jan 2021 18:09:17 +0100 Subject: [PATCH 3/6] feedback --- http_handler.go | 84 ++++++++++++++----------- http_handler_test.go | 146 +++++++++++++++++++++++-------------------- 2 files changed, 125 insertions(+), 105 deletions(-) diff --git a/http_handler.go b/http_handler.go index 3a8b71300..0666bcf78 100644 --- a/http_handler.go +++ b/http_handler.go @@ -69,56 +69,23 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { 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: - requestedLvl, err := func(body io.Reader) (*zapcore.Level, error) { - switch r.Header.Get("Content-Type") { - case "application/x-www-form-urlencoded": - pld, err := ioutil.ReadAll(body) - if err != nil { - return nil, err - } - values, err := url.ParseQuery(string(pld)) - if err != nil { - return nil, err - } - lvl := values.Get("level") - if lvl == "" { - return nil, fmt.Errorf("must specify logging level") - } - var l zapcore.Level - if err := l.UnmarshalText([]byte(lvl)); err != nil { - return nil, err - } - return &l, nil - default: - var pld payload - if err := json.NewDecoder(r.Body).Decode(&pld); err != nil { - return nil, fmt.Errorf("malformed request body: %v", err) - } - if pld.Level == nil { - return nil, fmt.Errorf("must specify logging level") - } - return pld.Level, nil - } - }(r.Body) + requestedLvl, err := lvl.decodePutRequest(r.Header.Get("Content-Type"), r.Body) if err != nil { w.WriteHeader(http.StatusBadRequest) enc.Encode(errorResponse{Error: err.Error()}) return } - lvl.SetLevel(*requestedLvl) - enc.Encode(payload{Level: requestedLvl}) + lvl.SetLevel(requestedLvl) + enc.Encode(payload{Level: lvl.Level()}) default: w.WriteHeader(http.StatusMethodNotAllowed) enc.Encode(errorResponse{ @@ -126,3 +93,44 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { }) } } + +func (lvl AtomicLevel) decodePutRequest(contentType string, body io.Reader) (zapcore.Level, error) { + if contentType == "application/x-www-form-urlencoded" { + return lvl.decodePutURL(body) + } + return lvl.decodePutJSON(body) +} + +func (lvl AtomicLevel) decodePutURL(body io.Reader) (zapcore.Level, error) { + pld, err := ioutil.ReadAll(body) + if err != nil { + return 0, err + } + values, err := url.ParseQuery(string(pld)) + if err != nil { + return 0, err + } + lvlHeader := values.Get("level") + if lvlHeader == "" { + return 0, fmt.Errorf("must specify logging level") + } + var l zapcore.Level + if err := l.UnmarshalText([]byte(lvlHeader)); err != nil { + return 0, err + } + return l, nil +} + +func (lvl AtomicLevel) 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 1abc499fe..99ffcea1b 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -35,97 +35,109 @@ import ( ) func TestAtomicLevelServeHTTP(t *testing.T) { - tests := map[string]struct { - Method string - ContentType string - Body string - ExpectedCode int - ExpectedLevel zapcore.Level + tests := []struct { + desc string + method string + contentType string + body string + expectedCode int + expectedLevel zapcore.Level }{ - "GET": { - Method: http.MethodGet, - ExpectedCode: http.StatusOK, - ExpectedLevel: zap.InfoLevel, + { + desc: "GET", + method: http.MethodGet, + expectedCode: http.StatusOK, + expectedLevel: zap.InfoLevel, }, - "PUT JSON": { - Method: http.MethodPut, - ExpectedCode: http.StatusOK, - ExpectedLevel: zap.WarnLevel, - Body: `{"level":"warn"}`, + { + desc: "PUT JSON", + method: http.MethodPut, + expectedCode: http.StatusOK, + expectedLevel: zap.WarnLevel, + body: `{"level":"warn"}`, }, - "PUT URL encoded": { - Method: http.MethodPut, - ExpectedCode: http.StatusOK, - ExpectedLevel: zap.WarnLevel, - ContentType: "application/x-www-form-urlencoded", - 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", }, - "PUT JSON unrecognized": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - Body: `{"level":"unrecognized"}`, + { + desc: "PUT JSON unrecognized", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{"level":"unrecognized"}`, }, - "PUT URL encoded unrecognized": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - ContentType: "application/x-www-form-urlencoded", - Body: "level=unrecognized", + { + desc: "PUT URL encoded unrecognized", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "level=unrecognized", }, - "PUT JSON malformed": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - Body: `{"level":"warn`, + { + desc: "PUT JSON malformed", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{"level":"warn`, }, - "PUT URL encoded malformed": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - ContentType: "application/x-www-form-urlencoded", - Body: "level=%", + { + desc: "PUT URL encoded malformed", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "level=%", }, - "PUT JSON unspecified": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - Body: `{}`, + { + desc: "PUT JSON unspecified", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + body: `{}`, }, - "PUT URL encoded unspecified": { - Method: http.MethodPut, - ExpectedCode: http.StatusBadRequest, - ContentType: "application/x-www-form-urlencoded", - Body: "", + { + desc: "PUT URL encoded unspecified", + method: http.MethodPut, + expectedCode: http.StatusBadRequest, + contentType: "application/x-www-form-urlencoded", + body: "", }, - "POST JSON": { - Method: http.MethodPost, - ExpectedCode: http.StatusMethodNotAllowed, - Body: `{"level":"warn"}`, + { + desc: "POST JSON", + method: http.MethodPost, + expectedCode: http.StatusMethodNotAllowed, + body: `{"level":"warn"}`, }, - "POST URL": { - Method: http.MethodPost, - ExpectedCode: http.StatusMethodNotAllowed, - ContentType: "application/x-www-form-urlencoded", - Body: "level=warn", + { + desc: "POST URL", + method: http.MethodPost, + expectedCode: http.StatusMethodNotAllowed, + contentType: "application/x-www-form-urlencoded", + body: "level=warn", }, } - for name, test := range tests { - t.Run(name, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { lvl := zap.NewAtomicLevel() lvl.SetLevel(zapcore.InfoLevel) - ts := httptest.NewServer(lvl) - defer ts.Close() + server := httptest.NewServer(lvl) + defer server.Close() - req, err := http.NewRequest(test.Method, ts.URL, strings.NewReader(test.Body)) + req, err := http.NewRequest(tt.method, server.URL, strings.NewReader(tt.body)) require.NoError(t, err, "Error constructing %s request.", req.Method) - if test.ContentType != "" { - req.Header.Set("Content-Type", test.ContentType) + 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, test.ExpectedCode, res.StatusCode, "Unexpected status code.") - if test.ExpectedCode != http.StatusOK { + 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"` @@ -139,7 +151,7 @@ func TestAtomicLevelServeHTTP(t *testing.T) { Level zapcore.Level `json:"level"` } require.NoError(t, json.NewDecoder(res.Body).Decode(&pld), "Decoding response body") - assert.Equal(t, test.ExpectedLevel, pld.Level, "Unexpected logging level returned") + assert.Equal(t, tt.expectedLevel, pld.Level, "Unexpected logging level returned") }) } } From 6fe80122cf72684e46f7eba78bf90c1c7d36308c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 15 Jan 2021 09:47:05 -0800 Subject: [PATCH 4/6] decode*: Turn into functions These methods do not have need of the atomic level. Turn them into functions to avoid accidentally manipulating or accessing it. --- http_handler.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/http_handler.go b/http_handler.go index 0666bcf78..2d0c612ea 100644 --- a/http_handler.go +++ b/http_handler.go @@ -78,7 +78,7 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { case http.MethodGet: enc.Encode(payload{Level: lvl.Level()}) case http.MethodPut: - requestedLvl, err := lvl.decodePutRequest(r.Header.Get("Content-Type"), r.Body) + requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r.Body) if err != nil { w.WriteHeader(http.StatusBadRequest) enc.Encode(errorResponse{Error: err.Error()}) @@ -94,14 +94,15 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } -func (lvl AtomicLevel) decodePutRequest(contentType string, body io.Reader) (zapcore.Level, error) { +// Decodes incoming PUT requests and returns the requested logging level. +func decodePutRequest(contentType string, body io.Reader) (zapcore.Level, error) { if contentType == "application/x-www-form-urlencoded" { - return lvl.decodePutURL(body) + return decodePutURL(body) } - return lvl.decodePutJSON(body) + return decodePutJSON(body) } -func (lvl AtomicLevel) decodePutURL(body io.Reader) (zapcore.Level, error) { +func decodePutURL(body io.Reader) (zapcore.Level, error) { pld, err := ioutil.ReadAll(body) if err != nil { return 0, err @@ -110,18 +111,18 @@ func (lvl AtomicLevel) decodePutURL(body io.Reader) (zapcore.Level, error) { if err != nil { return 0, err } - lvlHeader := values.Get("level") - if lvlHeader == "" { + lvl := values.Get("level") + if lvl == "" { return 0, fmt.Errorf("must specify logging level") } var l zapcore.Level - if err := l.UnmarshalText([]byte(lvlHeader)); err != nil { + if err := l.UnmarshalText([]byte(lvl)); err != nil { return 0, err } return l, nil } -func (lvl AtomicLevel) decodePutJSON(body io.Reader) (zapcore.Level, error) { +func decodePutJSON(body io.Reader) (zapcore.Level, error) { var pld struct { Level *zapcore.Level `json:"level"` } From 870ced497b9258ec9fb2c5f7ca8da929a586f8b1 Mon Sep 17 00:00:00 2001 From: Oncilla Date: Mon, 25 Jan 2021 17:49:09 +0100 Subject: [PATCH 5/6] use Request.FormValue --- http_handler.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/http_handler.go b/http_handler.go index 2d0c612ea..9bb788a4e 100644 --- a/http_handler.go +++ b/http_handler.go @@ -24,9 +24,7 @@ import ( "encoding/json" "fmt" "io" - "io/ioutil" "net/http" - "net/url" "go.uber.org/zap/zapcore" ) @@ -78,7 +76,7 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { case http.MethodGet: enc.Encode(payload{Level: lvl.Level()}) case http.MethodPut: - requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r.Body) + requestedLvl, err := decodePutRequest(r.Header.Get("Content-Type"), r) if err != nil { w.WriteHeader(http.StatusBadRequest) enc.Encode(errorResponse{Error: err.Error()}) @@ -95,23 +93,15 @@ func (lvl AtomicLevel) ServeHTTP(w http.ResponseWriter, r *http.Request) { } // Decodes incoming PUT requests and returns the requested logging level. -func decodePutRequest(contentType string, body io.Reader) (zapcore.Level, error) { +func decodePutRequest(contentType string, r *http.Request) (zapcore.Level, error) { if contentType == "application/x-www-form-urlencoded" { - return decodePutURL(body) + return decodePutURL(r) } - return decodePutJSON(body) + return decodePutJSON(r.Body) } -func decodePutURL(body io.Reader) (zapcore.Level, error) { - pld, err := ioutil.ReadAll(body) - if err != nil { - return 0, err - } - values, err := url.ParseQuery(string(pld)) - if err != nil { - return 0, err - } - lvl := values.Get("level") +func decodePutURL(r *http.Request) (zapcore.Level, error) { + lvl := r.FormValue("level") if lvl == "" { return 0, fmt.Errorf("must specify logging level") } From 86ae38e72b0b537bb82204a58439fd5ed38ce4da Mon Sep 17 00:00:00 2001 From: Oncilla Date: Tue, 26 Jan 2021 09:17:43 +0100 Subject: [PATCH 6/6] document and test query parameters --- http_handler.go | 11 ++++++++--- http_handler_test.go | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/http_handler.go b/http_handler.go index 9bb788a4e..1297c33b3 100644 --- a/http_handler.go +++ b/http_handler.go @@ -44,13 +44,18 @@ import ( // // Content-Type: application/x-www-form-urlencoded // -// With this content type, the request body is expected to be URL encoded like: +// 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 // -// This is the default content type for a curl PUT request. An example curl -// request could look like this: +// 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 diff --git a/http_handler_test.go b/http_handler_test.go index 99ffcea1b..9fa9c64c1 100644 --- a/http_handler_test.go +++ b/http_handler_test.go @@ -38,6 +38,7 @@ func TestAtomicLevelServeHTTP(t *testing.T) { tests := []struct { desc string method string + query string contentType string body string expectedCode int @@ -64,6 +65,31 @@ func TestAtomicLevelServeHTTP(t *testing.T) { 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, @@ -86,6 +112,13 @@ func TestAtomicLevelServeHTTP(t *testing.T) { { 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=%", @@ -126,7 +159,7 @@ func TestAtomicLevelServeHTTP(t *testing.T) { server := httptest.NewServer(lvl) defer server.Close() - req, err := http.NewRequest(tt.method, server.URL, strings.NewReader(tt.body)) + 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)