http: support additional content type (uber-go#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 uber-go#902

Co-authored-by: Abhinav Gupta <abg@uber.com>

http_handler.go

@@ -23,6 +23,7 @@ package zap
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 
 	"go.uber.org/zap/zapcore"
@@ -31,51 +32,101 @@ 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: &current})
-
+		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{
 			Error: "Only GET and PUT are supported.",
 		})
 	}
 }
+
+// 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
+
+}

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")
+		})
+	}
 }