etcdserver: restructure to hopefully simplify

Obviate parseBool helper, define emptyReq locally to parseRequest,
have writeEvent return an error which gets logged

etcdserver/etcdhttp/http.go

@@ -33,8 +33,6 @@ const (
 	machinesPrefix = "/v2/machines"
 )
 
-var emptyReq = etcdserverpb.Request{}
-
 type Peers map[int64][]string
 
 func (ps Peers) Pick(id int64) string {
@@ -204,7 +202,10 @@ func (h Handler) serveKeys(ctx context.Context, w http.ResponseWriter, r *http.R
 		return
 	}
 
-	writeEvent(w, ev)
+	if err = writeEvent(w, ev); err != nil {
+		// Should never be reached
+		log.Println("error writing event: %v", err)
+	}
 }
 
 // serveMachines responds address list in the format '0.0.0.0, 1.1.1.1'.
@@ -254,14 +255,16 @@ func genId() int64 {
 }
 
 func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
-	var err error
+	emptyReq := etcdserverpb.Request{}
 
-	if err = r.ParseForm(); err != nil {
+	err := r.ParseForm()
+	if err != nil {
 		return emptyReq, etcdErr.NewRequestError(
 			etcdErr.EcodeInvalidForm,
 			err.Error(),
 		)
 	}
+	q := r.URL.Query()
 
 	if !strings.HasPrefix(r.URL.Path, keysPrefix) {
 		return emptyReq, etcdErr.NewRequestError(
@@ -269,64 +272,89 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 			"incorrect key prefix",
 		)
 	}
-	path := r.URL.Path[len(keysPrefix):]
-
-	q := r.URL.Query()
+	p := r.URL.Path[len(keysPrefix):]
 
 	var pIdx, wIdx, ttl uint64
-	if pIdx, err = parseUint64(q.Get("prevIndex")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeIndexNaN,
-			"invalid value for prevIndex",
-		)
+	if pIdxS := q.Get("prevIndex"); pIdxS != "" {
+		if pIdx, err = parseUint64(pIdxS); err != nil {
+			return emptyReq, etcdErr.NewRequestError(
+				etcdErr.EcodeIndexNaN,
+				fmt.Sprintf("invalid value for prevIndex: %q", pIdxS),
+			)
+		}
 	}
-	if wIdx, err = parseUint64(q.Get("waitIndex")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeIndexNaN,
-			"invalid value for waitIndex",
-		)
+	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),
+			)
+		}
 	}
-	if ttl, err = parseUint64(q.Get("ttl")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeTTLNaN,
-			"invalid value for ttl",
-		)
+	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),
+			)
+		}
 	}
 
 	var rec, sort, wait bool
-	if rec, err = parseBool(q.Get("recursive")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeInvalidField,
-			"invalid value for recursive")
+	if recS := q.Get("recursive"); recS != "" {
+		if rec, err = strconv.ParseBool(recS); err != nil {
+			return emptyReq, etcdErr.NewRequestError(
+				etcdErr.EcodeInvalidField,
+				fmt.Sprintf("invalid value for recursive: %q", recS),
+			)
+		}
 	}
-	if sort, err = parseBool(q.Get("sorted")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeInvalidField,
-			"invalid value for sorted")
+	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),
+			)
+		}
 	}
-	if wait, err = parseBool(q.Get("wait")); err != nil {
-		return emptyReq, etcdErr.NewRequestError(
-			etcdErr.EcodeInvalidField,
-			"invalid value for wait")
+	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),
+			)
+		}
+	}
+
+	// prevExists is nullable, so leave it null if not specified
+	var pe *bool
+	if _, ok := q["prevExists"]; ok {
+		bv, err := strconv.ParseBool(q.Get("prevExists"))
+		if err != nil {
+			return emptyReq, etcdErr.NewRequestError(
+				etcdErr.EcodeInvalidField,
+				"invalid value for prevExists",
+			)
+		}
+		pe = &bv
 	}
 
 	rr := etcdserverpb.Request{
-		Id:        id,
-		Method:    r.Method,
-		Val:       r.FormValue("value"),
-		Path:      path,
-		PrevValue: q.Get("prevValue"),
-		PrevIndex: pIdx,
-		Recursive: rec,
-		Since:     wIdx,
-		Sorted:    sort,
-		Wait:      wait,
+		Id:         id,
+		Method:     r.Method,
+		Path:       p,
+		Val:        r.FormValue("value"),
+		PrevValue:  q.Get("prevValue"),
+		PrevIndex:  pIdx,
+		PrevExists: pe,
+		Recursive:  rec,
+		Since:      wIdx,
+		Sorted:     sort,
+		Wait:       wait,
 	}
 
-	// prevExists is nullable, so leave it null if not specified
-	if _, ok := q["prevExists"]; ok {
-		bv, _ := parseBool(q.Get("prevExists"))
-		rr.PrevExists = &bv
+	if pe != nil {
+		rr.PrevExists = pe
 	}
 
 	if ttl > 0 {
@@ -339,17 +367,7 @@ func parseRequest(r *http.Request, id int64) (etcdserverpb.Request, error) {
 	return rr, nil
 }
 
-func parseBool(s string) (bool, error) {
-	if s == "" {
-		return false, nil
-	}
-	return strconv.ParseBool(s)
-}
-
 func parseUint64(s string) (uint64, error) {
-	if s == "" {
-		return 0, nil
-	}
 	return strconv.ParseUint(s, 10, 64)
 }
 
@@ -370,9 +388,9 @@ func writeError(w http.ResponseWriter, err error) {
 
 // writeEvent serializes the given Event and writes the resulting JSON to the
 // given ResponseWriter
-func writeEvent(w http.ResponseWriter, ev *store.Event) {
+func writeEvent(w http.ResponseWriter, ev *store.Event) error {
 	if ev == nil {
-		return
+		return errors.New("cannot write empty Event!")
 	}
 	w.Header().Set("Content-Type", "application/json")
 	w.Header().Add("X-Etcd-Index", fmt.Sprint(ev.Index()))
@@ -381,9 +399,7 @@ func writeEvent(w http.ResponseWriter, ev *store.Event) {
 		w.WriteHeader(http.StatusCreated)
 	}
 
-	if err := json.NewEncoder(w).Encode(ev); err != nil {
-		panic(err) // should never be reached
-	}
+	return json.NewEncoder(w).Encode(ev)
 }
 
 // waitForEvent waits for a given Watcher to return its associated

etcdserver/etcdhttp/http_test.go

@@ -34,26 +34,6 @@ func mustNewRequest(t *testing.T, p string) *http.Request {
 	}
 }
 
-func TestParseBool(t *testing.T) {
-	got, err := parseBool("")
-	if got != false {
-		t.Fatalf("got %t, want %t", got, false)
-	}
-	if err != nil {
-		t.Fatalf("err = %v, want %v", err, nil)
-	}
-}
-
-func TestParseUint64(t *testing.T) {
-	got, err := parseUint64("")
-	if got != 0 {
-		t.Fatalf("got %d, want %d", got, 0)
-	}
-	if err != nil {
-		t.Fatalf("err = %v, want %v", err, nil)
-	}
-}
-
 func TestBadParseRequest(t *testing.T) {
 	tests := []struct {
 		in    *http.Request