server: use HTTP 400 where there is a client error

We often send a 500 error where it should be 400 or 404. It is important to get these right. A client will assume it did nothing wrong when a 500 is received and perhaps automatically retry the request. This is a conservative change: there are many more places that should be changed from 500. A helper function jsonRespondError is added
tikv · Jun 29, 2018 · 53438ca · 53438ca
1 parent 3ddb132
commit 53438ca
Show file tree

Hide file tree

Showing 12 changed files with 173 additions and 56 deletions.
diff --git a/pkg/apiutil/apiutil.go b/pkg/apiutil/apiutil.go
@@ -21,19 +21,50 @@ import (
 	"github.com/juju/errors"
 )
 
-// ReadJSON reads a JSON data from r and then close it.
-func ReadJSON(r io.ReadCloser, data interface{}) error {
-	defer r.Close()
+// DeferClose captures the error returned from closing (if an error occurs).
+// This is designed to be used in a defer statement.
+func DeferClose(c io.Closer, err *error) {
+	if cerr := c.Close(); cerr != nil && *err == nil {
+		*err = errors.Trace(cerr)
+	}
+}
+
+// JSONError lets callers check for just one error type
+type JSONError struct {
+	err error
+}
+
+func (e JSONError) Error() string {
+	return e.err.Error()
+}
 
+// Cause for compatibility with the errors package
+func (e JSONError) Cause() error {
+	return e.err
+}
+
+func tagJSONError(err error) error {
+	switch err.(type) {
+	case *json.SyntaxError, *json.UnmarshalTypeError:
+		return JSONError{err}
+	}
+	return err
+}
+
+// ReadJSON reads a JSON data from r and then closes it.
+// An error due to invalid json will be returned as a JSONError
+func ReadJSON(r io.ReadCloser, data interface{}) error {
+	var err error
+	defer DeferClose(r, &err)
 	b, err := ioutil.ReadAll(r)
 	if err != nil {
 		return errors.Trace(err)
 	}
 
 	err = json.Unmarshal(b, data)
 	if err != nil {
-		return errors.Trace(err)
+		return tagJSONError(err)
 	}
 
-	return nil
+	return err
 }
diff --git a/server/api/admin.go b/server/api/admin.go
@@ -45,7 +45,7 @@ func (h *adminHandler) HandleDropCacheRegion(w http.ResponseWriter, r *http.Requ
 	regionIDStr := vars["id"]
 	regionID, err := strconv.ParseUint(regionIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 	cluster.DropCacheRegion(regionID)

diff --git a/server/api/config.go b/server/api/config.go
@@ -74,9 +74,7 @@ func (h *confHandler) GetSchedule(w http.ResponseWriter, r *http.Request) {
 
 func (h *confHandler) SetSchedule(w http.ResponseWriter, r *http.Request) {
 	config := h.svr.GetScheduleConfig()
-	err := readJSON(r.Body, config)
-	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &config); err != nil {
 		return
 	}
 
@@ -93,9 +91,7 @@ func (h *confHandler) GetReplication(w http.ResponseWriter, r *http.Request) {
 
 func (h *confHandler) SetReplication(w http.ResponseWriter, r *http.Request) {
 	config := h.svr.GetReplicationConfig()
-	err := readJSON(r.Body, config)
-	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &config); err != nil {
 		return
 	}
 
@@ -108,7 +104,7 @@ func (h *confHandler) GetNamespace(w http.ResponseWriter, r *http.Request) {
 	name := vars["name"]
 
 	if !h.svr.IsNamespaceExist(name) {
-		h.rd.JSON(w, http.StatusInternalServerError, fmt.Sprintf("invalid namespace Name %s, not found", name))
+		h.rd.JSON(w, http.StatusNotFound, fmt.Sprintf("invalid namespace Name %s, not found", name))
 		return
 	}
 
@@ -122,14 +118,12 @@ func (h *confHandler) SetNamespace(w http.ResponseWriter, r *http.Request) {
 	name := vars["name"]
 
 	if !h.svr.IsNamespaceExist(name) {
-		h.rd.JSON(w, http.StatusInternalServerError, fmt.Sprintf("invalid namespace Name %s, not found", name))
+		h.rd.JSON(w, http.StatusNotFound, fmt.Sprintf("invalid namespace Name %s, not found", name))
 		return
 	}
 
 	config := h.svr.GetNamespaceConfig(name)
-	err := readJSON(r.Body, config)
-	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &config); err != nil {
 		return
 	}
 
@@ -142,7 +136,7 @@ func (h *confHandler) DeleteNamespace(w http.ResponseWriter, r *http.Request) {
 	name := vars["name"]
 
 	if !h.svr.IsNamespaceExist(name) {
-		h.rd.JSON(w, http.StatusInternalServerError, fmt.Sprintf("invalid namespace Name %s, not found", name))
+		h.rd.JSON(w, http.StatusNotFound, fmt.Sprintf("invalid namespace Name %s, not found", name))
 		return
 	}
 	h.svr.DeleteNamespaceConfig(name)
@@ -156,11 +150,10 @@ func (h *confHandler) GetLabelProperty(w http.ResponseWriter, r *http.Request) {
 
 func (h *confHandler) SetLabelProperty(w http.ResponseWriter, r *http.Request) {
 	input := make(map[string]string)
-	err := readJSON(r.Body, &input)
-	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &input); err != nil {
 		return
 	}
+	var err error
 	switch input["action"] {
 	case "set":
 		err = h.svr.SetLabelProperty(input["type"], input["label-key"], input["label-value"])

diff --git a/server/api/log.go b/server/api/log.go
@@ -46,7 +46,7 @@ func (h *logHandler) Handle(w http.ResponseWriter, r *http.Request) {
 	}
 	err = json.Unmarshal(data, &level)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 

diff --git a/server/api/member.go b/server/api/member.go
@@ -96,7 +96,7 @@ func (h *memberHandler) DeleteByID(w http.ResponseWriter, r *http.Request) {
 	idStr := mux.Vars(r)["id"]
 	id, err := strconv.ParseUint(idStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -117,9 +117,9 @@ func (h *memberHandler) DeleteByID(w http.ResponseWriter, r *http.Request) {
 }
 
 func (h *memberHandler) SetMemberPropertyByName(w http.ResponseWriter, r *http.Request) {
-	members, err := h.listMembers()
-	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	members, membersErr := h.listMembers()
+	if membersErr != nil {
+		h.rd.JSON(w, http.StatusInternalServerError, membersErr.Error())
 		return
 	}
 
@@ -137,8 +137,7 @@ func (h *memberHandler) SetMemberPropertyByName(w http.ResponseWriter, r *http.R
 	}
 
 	var input map[string]interface{}
-	if err = readJSON(r.Body, &input); err != nil {
-		h.rd.JSON(w, http.StatusBadRequest, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &input); err != nil {
 		return
 	}
 	for k, v := range input {
@@ -149,7 +148,7 @@ func (h *memberHandler) SetMemberPropertyByName(w http.ResponseWriter, r *http.R
 				h.rd.JSON(w, http.StatusBadRequest, "bad format leader priority")
 				return
 			}
-			err = h.svr.SetMemberLeaderPriority(memberID, int(priority))
+			err := h.svr.SetMemberLeaderPriority(memberID, int(priority))
 			if err != nil {
 				h.rd.JSON(w, http.StatusInternalServerError, err.Error())
 				return

diff --git a/server/api/operator.go b/server/api/operator.go
@@ -40,7 +40,7 @@ func (h *operatorHandler) Get(w http.ResponseWriter, r *http.Request) {
 
 	regionID, err := strconv.ParseUint(id, 10, 64)
 	if err != nil {
-		h.r.JSON(w, http.StatusInternalServerError, err.Error())
+		h.r.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -90,14 +90,13 @@ func (h *operatorHandler) List(w http.ResponseWriter, r *http.Request) {
 
 func (h *operatorHandler) Post(w http.ResponseWriter, r *http.Request) {
 	var input map[string]interface{}
-	if err := readJSON(r.Body, &input); err != nil {
-		h.r.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.r, w, r.Body, &input); err != nil {
 		return
 	}
 
 	name, ok := input["name"].(string)
 	if !ok {
-		h.r.JSON(w, http.StatusInternalServerError, "missing operator name")
+		h.r.JSON(w, http.StatusBadRequest, "missing operator name")
 		return
 	}
 
@@ -234,7 +233,7 @@ func (h *operatorHandler) Delete(w http.ResponseWriter, r *http.Request) {
 
 	regionID, err := strconv.ParseUint(id, 10, 64)
 	if err != nil {
-		h.r.JSON(w, http.StatusInternalServerError, err.Error())
+		h.r.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 

diff --git a/server/api/region.go b/server/api/region.go
@@ -92,7 +92,7 @@ func (h *regionHandler) GetRegionByID(w http.ResponseWriter, r *http.Request) {
 	regionIDStr := vars["id"]
 	regionID, err := strconv.ParseUint(regionIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -203,12 +203,12 @@ func (h *regionsHandler) GetRegionSiblings(w http.ResponseWriter, r *http.Reques
 	vars := mux.Vars(r)
 	id, err := strconv.Atoi(vars["id"])
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 	region := cluster.GetRegionInfoByID(uint64(id))
 	if region == nil {
-		h.rd.JSON(w, http.StatusInternalServerError, server.ErrRegionNotFound(uint64(id)).Error())
+		h.rd.JSON(w, http.StatusNotFound, server.ErrRegionNotFound(uint64(id)).Error())
 		return
 	}
 

diff --git a/server/api/scheduler.go b/server/api/scheduler.go
@@ -44,8 +44,7 @@ func (h *schedulerHandler) List(w http.ResponseWriter, r *http.Request) {
 
 func (h *schedulerHandler) Post(w http.ResponseWriter, r *http.Request) {
 	var input map[string]interface{}
-	if err := readJSON(r.Body, &input); err != nil {
-		h.r.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.r, w, r.Body, &input); err != nil {
 		return
 	}
 

diff --git a/server/api/store.go b/server/api/store.go
@@ -141,7 +141,7 @@ func (h *storeHandler) Get(w http.ResponseWriter, r *http.Request) {
 	storeIDStr := vars["id"]
 	storeID, err := strconv.ParseUint(storeIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -166,7 +166,7 @@ func (h *storeHandler) Delete(w http.ResponseWriter, r *http.Request) {
 	storeIDStr := vars["id"]
 	storeID, err := strconv.ParseUint(storeIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -196,7 +196,7 @@ func (h *storeHandler) SetState(w http.ResponseWriter, r *http.Request) {
 	storeIDStr := vars["id"]
 	storeID, err := strconv.ParseUint(storeIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
@@ -227,13 +227,12 @@ func (h *storeHandler) SetLabels(w http.ResponseWriter, r *http.Request) {
 	storeIDStr := vars["id"]
 	storeID, err := strconv.ParseUint(storeIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
 	var input map[string]string
-	if err := readJSON(r.Body, &input); err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &input); err != nil {
 		return
 	}
 
@@ -264,13 +263,12 @@ func (h *storeHandler) SetWeight(w http.ResponseWriter, r *http.Request) {
 	storeIDStr := vars["id"]
 	storeID, err := strconv.ParseUint(storeIDStr, 10, 64)
 	if err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		h.rd.JSON(w, http.StatusBadRequest, err.Error())
 		return
 	}
 
 	var input map[string]interface{}
-	if err := readJSON(r.Body, &input); err != nil {
-		h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+	if err := jsonRespondError(h.rd, w, r.Body, &input); err != nil {
 		return
 	}
 

diff --git a/server/api/util.go b/server/api/util.go
@@ -21,9 +21,24 @@ import (
 	"net/http"
 
 	"github.com/juju/errors"
+	"github.com/pingcap/pd/pkg/apiutil"
 	"github.com/pingcap/pd/server"
+	"github.com/unrolled/render"
 )
 
+func jsonRespondError(r *render.Render, w http.ResponseWriter, body io.ReadCloser, data interface{}) (err error) {
+	if err = apiutil.ReadJSON(body, data); err != nil {
+		switch err.(type) {
+		case apiutil.JSONError:
+			r.JSON(w, http.StatusBadRequest, err.Error())
+		default:
+			r.JSON(w, http.StatusInternalServerError, err.Error())
+
+		}
+	}
+	return
+}
+
 func readJSON(r io.ReadCloser, data interface{}) error {
 	defer r.Close()