-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http: support additional content type #903
Changes from 2 commits
756d507
794c2df
e1be881
6fe8012
870ced4
86ae38e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -23,19 +23,47 @@ package zap | |||||||||||||
import ( | ||||||||||||||
"encoding/json" | ||||||||||||||
"fmt" | ||||||||||||||
"io" | ||||||||||||||
"io/ioutil" | ||||||||||||||
"net/http" | ||||||||||||||
"net/url" | ||||||||||||||
|
||||||||||||||
"go.uber.org/zap/zapcore" | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
// 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) { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be an effort to reduce error handling duplication. That's a good call but can we perhaps move this to its own unexported method?
Additionally, since zapcore.Level is just an int8, we don't need to return its address:
It's a pointer in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I tried to follow the previous style, but your suggestion is more maintainable and readable IMO. |
||||||||||||||
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 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optionally, this can be shortened.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer being explicit here. Especially after switching to value type. It is not immediately obvious whether the returned value is pre- or post-UnmarshalText without knowing that part of the spec by hart. (TBH I would need to look that up) |
||||||||||||||
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}) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want a JSON output even for non-JSON input? Maybe this is okay and we can add a plain text output later if we need it and the client has an CC @prashantv There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to return non-JSON in the response, I would definitively go for content type negotiation through the In any case, I would vote for this to be part of a follow-up PR if it is desired. |
||||||||||||||
default: | ||||||||||||||
w.WriteHeader(http.StatusMethodNotAllowed) | ||||||||||||||
enc.Encode(errorResponse{ | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||
}{ | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for making this a table test. If you wouldn't mind, we have a preference for slice-based table tests rather than maps:
(See also https://github.com/uber-go/guide/blob/master/style.md#test-tables) |
||||||||||||||||||
"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 { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For table tests, consider renaming the test cases to for _, tt := 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)) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we name the test server srv or server?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||||||||||
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") | ||||||||||||||||||
}) | ||||||||||||||||||
} | ||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great documentation, thank you!