etcdhttp: use Form values over query parameters

etcdserver/etcdhttp/http.go

@@ -254,6 +254,9 @@ func genId() int64 {
 	}
 }
 
+// parseRequest converts a received http.Request to a server Request,
+// performing validation of supplied fields as appropriate.
+// If any validation fails, an empty Request and non-nil error is returned.
 func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 	emptyReq := etcdserverpb.Request{}
 
@@ -264,7 +267,6 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 			err.Error(),
 		)
 	}
-	q := r.URL.Query()
 
 	if !strings.HasPrefix(r.URL.Path, keysPrefix) {
 		return emptyReq, etcdErr.NewRequestError(
@@ -275,61 +277,49 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 	p := r.URL.Path[len(keysPrefix):]
 
 	var pIdx, wIdx, ttl uint64
-	if pIdxS := q.Get("prevIndex"); pIdxS != "" {
+	if pIdx, err = getUint64(r.Form, "prevIndex"); err != nil {
-		if pIdx, err = parseUint64(pIdxS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeIndexNaN,
-				fmt.Sprintf("invalid value for prevIndex: %q", pIdxS),
+			fmt.Sprintf("invalid value for prevIndex"),
 		)
 	}
-	}
+	if wIdx, err = getUint64(r.Form, "waitIndex"); err != nil {
-	if wIdxS := q.Get("waitIndex"); wIdxS != "" {
-		if wIdx, err = parseUint64(wIdxS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeIndexNaN,
-				fmt.Sprintf("invalid value for waitIndex: %q", wIdxS),
+			fmt.Sprintf("invalid value for waitIndex"),
 		)
 	}
-	}
+	if ttl, err = getUint64(r.Form, "ttl"); err != nil {
-	if ttlS := q.Get("ttl"); ttlS != "" {
-		if ttl, err = parseUint64(ttlS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeTTLNaN,
-				fmt.Sprintf("invalid value for ttl: %q", ttlS),
+			`invalid value for "ttl"`,
 		)
 	}
-	}
 
 	var rec, sort, wait bool
-	if recS := q.Get("recursive"); recS != "" {
+	if rec, err = getBool(r.Form, "recursive"); err != nil {
-		if rec, err = strconv.ParseBool(recS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeInvalidField,
-				fmt.Sprintf("invalid value for recursive: %q", recS),
+			`invalid value for "recursive"`,
 		)
 	}
-	}
+	if sort, err = getBool(r.Form, "sorted"); err != nil {
-	if sortS := q.Get("sorted"); sortS != "" {
-		if sort, err = strconv.ParseBool(sortS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeInvalidField,
-				fmt.Sprintf("invalid value for sorted: %q", sortS),
+			`invalid value for "sorted"`,
 		)
 	}
-	}
+	if wait, err = getBool(r.Form, "wait"); err != nil {
-	if waitS := q.Get("wait"); waitS != "" {
-		if wait, err = strconv.ParseBool(waitS); err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeInvalidField,
-				fmt.Sprintf("invalid value for wait: %q", waitS),
+			`invalid value for "wait"`,
 		)
 	}
-	}
 
 	// prevExists is nullable, so leave it null if not specified
 	var pe *bool
-	if _, ok := q["prevExists"]; ok {
+	if _, ok := r.Form["prevExists"]; ok {
-		bv, err := strconv.ParseBool(q.Get("prevExists"))
+		bv, err := getBool(r.Form, "prevExists")
 		if err != nil {
 			return emptyReq, etcdErr.NewRequestError(
 				etcdErr.EcodeInvalidField,
@@ -344,7 +334,7 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 		Method:     r.Method,
 		Path:       p,
 		Val:        r.FormValue("value"),
-		PrevValue:  q.Get("prevValue"),
+		PrevValue:  r.FormValue("prevValue"),
 		PrevIndex:  pIdx,
 		PrevExists: pe,
 		Recursive:  rec,
@@ -367,8 +357,26 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 	return rr, nil
 }
 
-func parseUint64(s string) (uint64, error) {
+// getUint64 extracts a uint64 by the given key from a Form. If the key does
-	return strconv.ParseUint(s, 10, 64)
+// not exist in the form, 0 is returned. If the key exists but the value is
+// badly formed, an error is returned. If multiple values are present only the
+// first is considered.
+func getUint64(form url.Values, key string) (i uint64, err error) {
+	if vals, ok := form[key]; ok {
+		i, err = strconv.ParseUint(vals[0], 10, 64)
+	}
+	return
+}
+
+// getBool extracts a bool by the given key from a Form. If the key does not
+// exist in the form, false is returned. If the key exists but the value is
+// badly formed, an error is returned. If multiple values are present only the
+// first is considered.
+func getBool(form url.Values, key string) (b bool, err error) {
+	if vals, ok := form[key]; ok {
+		b, err = strconv.ParseBool(vals[0])
+	}
+	return
 }
 
 // writeError logs and writes the given Error to the ResponseWriter

etcdserver/etcdhttp/http_test.go

@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"path"
 	"reflect"
+	"strings"
 	"sync"
 	"testing"
 
@@ -34,6 +35,18 @@ func mustNewRequest(t *testing.T, p string) *http.Request {
 	}
 }
 
+// mustNewForm takes a set of Values and constructs a PUT *http.Request,
+// with a URL constructed from appending the given path to the standard keysPrefix
+func mustNewForm(t *testing.T, p string, vals url.Values) *http.Request {
+	u := mustNewURL(t, path.Join(keysPrefix, p))
+	req, err := http.NewRequest("PUT", u.String(), strings.NewReader(vals.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	if err != nil {
+		t.Fatalf("error creating new request: %v", err)
+	}
+	return req
+}
+
 func TestBadParseRequest(t *testing.T) {
 	tests := []struct {
 		in    *http.Request
@@ -56,56 +69,90 @@ func TestBadParseRequest(t *testing.T) {
 		},
 		// bad values for prevIndex, waitIndex, ttl
 		{
-			mustNewRequest(t, "?prevIndex=foo"),
+			mustNewForm(t, "foo", url.Values{"prevIndex": []string{"garbage"}}),
 			etcdErr.EcodeIndexNaN,
 		},
 		{
-			mustNewRequest(t, "?prevIndex=1.5"),
+			mustNewForm(t, "foo", url.Values{"prevIndex": []string{"1.5"}}),
 			etcdErr.EcodeIndexNaN,
 		},
 		{
-			mustNewRequest(t, "?prevIndex=-1"),
+			mustNewForm(t, "foo", url.Values{"prevIndex": []string{"-1"}}),
 			etcdErr.EcodeIndexNaN,
 		},
 		{
-			mustNewRequest(t, "?waitIndex=garbage"),
+			mustNewForm(t, "foo", url.Values{"waitIndex": []string{"garbage"}}),
 			etcdErr.EcodeIndexNaN,
 		},
 		{
-			mustNewRequest(t, "?waitIndex=??"),
+			mustNewForm(t, "foo", url.Values{"waitIndex": []string{"??"}}),
 			etcdErr.EcodeIndexNaN,
 		},
 		{
-			mustNewRequest(t, "?ttl=-1"),
+			mustNewForm(t, "foo", url.Values{"ttl": []string{"-1"}}),
 			etcdErr.EcodeTTLNaN,
 		},
-		// bad values for recursive, sorted, wait
+		// bad values for recursive, sorted, wait, prevExists
 		{
-			mustNewRequest(t, "?recursive=hahaha"),
+			mustNewForm(t, "foo", url.Values{"recursive": []string{"hahaha"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?recursive=1234"),
+			mustNewForm(t, "foo", url.Values{"recursive": []string{"1234"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?recursive=?"),
+			mustNewForm(t, "foo", url.Values{"recursive": []string{"?"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?sorted=hahaha"),
+			mustNewForm(t, "foo", url.Values{"sorted": []string{"?"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?sorted=!!"),
+			mustNewForm(t, "foo", url.Values{"sorted": []string{"x"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?wait=notreally"),
+			mustNewForm(t, "foo", url.Values{"wait": []string{"?!"}}),
 			etcdErr.EcodeInvalidField,
 		},
 		{
-			mustNewRequest(t, "?wait=what!"),
+			mustNewForm(t, "foo", url.Values{"wait": []string{"yes"}}),
+			etcdErr.EcodeInvalidField,
+		},
+		{
+			mustNewForm(t, "foo", url.Values{"prevExists": []string{"yes"}}),
+			etcdErr.EcodeInvalidField,
+		},
+		{
+			mustNewForm(t, "foo", url.Values{"prevExists": []string{"#2"}}),
+			etcdErr.EcodeInvalidField,
+		},
+		// query values are considered
+		{
+			mustNewRequest(t, "foo?prevExists=wrong"),
+			etcdErr.EcodeInvalidField,
+		},
+		{
+			mustNewRequest(t, "foo?ttl=wrong"),
+			etcdErr.EcodeTTLNaN,
+		},
+		// but body takes precedence if both are specified
+		{
+			mustNewForm(
+				t,
+				"foo?ttl=12",
+				url.Values{"ttl": []string{"garbage"}},
+			),
+			etcdErr.EcodeTTLNaN,
+		},
+		{
+			mustNewForm(
+				t,
+				"foo?prevExists=false",
+				url.Values{"prevExists": []string{"yes"}},
+			),
 			etcdErr.EcodeInvalidField,
 		},
 	}
@@ -122,6 +169,7 @@ func TestBadParseRequest(t *testing.T) {
 		}
 		if ee.ErrorCode != tt.wcode {
 			t.Errorf("#%d: code=%d, want %v", i, ee.ErrorCode, tt.wcode)
+			t.Logf("cause: %#v", ee.Cause)
 		}
 		if !reflect.DeepEqual(got, etcdserverpb.Request{}) {
 			t.Errorf("#%d: unexpected non-empty Request: %#v", i, got)
@@ -144,67 +192,152 @@ func TestGoodParseRequest(t *testing.T) {
 		},
 		{
 			// value specified
-			mustNewRequest(t, "foo?value=some_value"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"value": []string{"some_value"}},
+			),
 			etcdserverpb.Request{
 				Id:     1234,
+				Method: "PUT",
 				Val:    "some_value",
 				Path:   "/foo",
 			},
 		},
 		{
 			// prevIndex specified
-			mustNewRequest(t, "foo?prevIndex=98765"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"prevIndex": []string{"98765"}},
+			),
 			etcdserverpb.Request{
 				Id:        1234,
+				Method:    "PUT",
 				PrevIndex: 98765,
 				Path:      "/foo",
 			},
 		},
 		{
 			// recursive specified
-			mustNewRequest(t, "foo?recursive=true"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"recursive": []string{"true"}},
+			),
 			etcdserverpb.Request{
 				Id:        1234,
+				Method:    "PUT",
 				Recursive: true,
 				Path:      "/foo",
 			},
 		},
 		{
 			// sorted specified
-			mustNewRequest(t, "foo?sorted=true"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"sorted": []string{"true"}},
+			),
 			etcdserverpb.Request{
 				Id:     1234,
+				Method: "PUT",
 				Sorted: true,
 				Path:   "/foo",
 			},
 		},
 		{
 			// wait specified
-			mustNewRequest(t, "foo?wait=true"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"wait": []string{"true"}},
+			),
 			etcdserverpb.Request{
 				Id:     1234,
+				Method: "PUT",
 				Wait:   true,
 				Path:   "/foo",
 			},
 		},
 		{
 			// prevExists should be non-null if specified
-			mustNewRequest(t, "foo?prevExists=true"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"prevExists": []string{"true"}},
+			),
 			etcdserverpb.Request{
 				Id:         1234,
+				Method:     "PUT",
 				PrevExists: boolp(true),
 				Path:       "/foo",
 			},
 		},
 		{
 			// prevExists should be non-null if specified
-			mustNewRequest(t, "foo?prevExists=false"),
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{"prevExists": []string{"false"}},
+			),
 			etcdserverpb.Request{
 				Id:         1234,
+				Method:     "PUT",
 				PrevExists: boolp(false),
 				Path:       "/foo",
 			},
 		},
+		// mix various fields
+		{
+			mustNewForm(
+				t,
+				"foo",
+				url.Values{
+					"value":      []string{"some value"},
+					"prevExists": []string{"true"},
+					"prevValue":  []string{"previous value"},
+				},
+			),
+			etcdserverpb.Request{
+				Id:         1234,
+				Method:     "PUT",
+				PrevExists: boolp(true),
+				PrevValue:  "previous value",
+				Val:        "some value",
+				Path:       "/foo",
+			},
+		},
+		// query parameters should be used if given
+		{
+			mustNewForm(
+				t,
+				"foo?prevValue=woof",
+				url.Values{},
+			),
+			etcdserverpb.Request{
+				Id:        1234,
+				Method:    "PUT",
+				PrevValue: "woof",
+				Path:      "/foo",
+			},
+		},
+		// but form values should take precedence over query parameters
+		{
+			mustNewForm(
+				t,
+				"foo?prevValue=woof",
+				url.Values{
+					"prevValue": []string{"miaow"},
+				},
+			),
+			etcdserverpb.Request{
+				Id:        1234,
+				Method:    "PUT",
+				PrevValue: "miaow",
+				Path:      "/foo",
+			},
+		},
 	}
 
 	for i, tt := range tests {
@@ -213,7 +346,7 @@ func TestGoodParseRequest(t *testing.T) {
 			t.Errorf("#%d: err = %v, want %v", i, err, nil)
 		}
 		if !reflect.DeepEqual(got, tt.w) {
-			t.Errorf("#%d: bad request: got %#v, want %#v", i, got, tt.w)
+			t.Errorf("#%d: request=%#v, want %#v", i, got, tt.w)
 		}
 	}
 }