feat(v2/errors): Use more appropriate HTTP status codes for error cases.

This commits adds test coverage for all the error and non-error cases
described below, but only the behavior of the 403, 404 and 412 cases
are changing in this commit.

When setting a key results in a new resource, we asset an HTTP status
code of 201 (aka "Created").

When attempting to get a resource that doesn't exist, we assert an
HTTP status code of 404 (aka "Not Found").

When attempting to delete a directory without dir=true, or a non-empty
directory without recursive=true, but the request is otherwise valid,
we assert an HTTP status code of 403 (aka "Forbidden").

When a precondition (e.g. specified by prevIndex, or prevValue) is not
met, but the request is otherwise syntactically valid, we assert an
HTTP status code of 412 (aka "Precondition Failed").  However,
prevExist is handled slightly differently.  If prevExist=false fails,
then this is treated like a failed precondition, so it should use
PreconditionFailed.  But, if prevExist=true fails, then this is
treated like other requests that require the existence of the
resource, and uses NotFound if the resource doesn't exist.

We continue to assert an HTTP status code of 400 when the request is
syntactically invalid (e.g. when prevIndex=bad_index).

error/error.go

@@ -112,9 +112,18 @@ func (e Error) toJsonString() string {
 func (e Error) Write(w http.ResponseWriter) {
 	w.Header().Add("X-Etcd-Index", fmt.Sprint(e.Index))
 	// 3xx is raft internal error
-	if e.ErrorCode/100 == 3 {
-		http.Error(w, e.toJsonString(), http.StatusInternalServerError)
-	} else {
-		http.Error(w, e.toJsonString(), http.StatusBadRequest)
+	status := http.StatusBadRequest
+	switch e.ErrorCode {
+	case EcodeKeyNotFound:
+		status = http.StatusNotFound
+	case EcodeNotFile, EcodeDirNotEmpty:
+		status = http.StatusForbidden
+	case EcodeTestFailed, EcodeNodeExist:
+		status = http.StatusPreconditionFailed
+	default:
+		if e.ErrorCode/100 == 3 {
+			status = http.StatusInternalServerError
+		}
 	}
+	http.Error(w, e.toJsonString(), status)
 }

server/v2/tests/delete_handler_test.go

@@ -2,6 +2,7 @@ package v2
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"testing"
 
@@ -22,6 +23,7 @@ func TestV2DeleteKey(t *testing.T) {
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
 		tests.ReadBody(resp)
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"delete","node":{"key":"/foo/bar","modifiedIndex":3,"createdIndex":2}}`, "")
@@ -39,9 +41,11 @@ func TestV2DeleteEmptyDirectory(t *testing.T) {
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), url.Values{})
 		tests.ReadBody(resp)
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusForbidden)
 		bodyJson := tests.ReadBodyJSON(resp)
 		assert.Equal(t, bodyJson["errorCode"], 102, "")
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"delete","node":{"key":"/foo","dir":true,"modifiedIndex":3,"createdIndex":2}}`, "")
@@ -59,9 +63,11 @@ func TestV2DeleteNonEmptyDirectory(t *testing.T) {
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar?dir=true"), url.Values{})
 		tests.ReadBody(resp)
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusForbidden)
 		bodyJson := tests.ReadBodyJSON(resp)
 		assert.Equal(t, bodyJson["errorCode"], 108, "")
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true&recursive=true"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"delete","node":{"key":"/foo","dir":true,"modifiedIndex":3,"createdIndex":2}}`, "")
@@ -78,6 +84,7 @@ func TestV2DeleteDirectoryRecursiveImpliesDir(t *testing.T) {
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), url.Values{})
 		tests.ReadBody(resp)
 		resp, err = tests.DeleteForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?recursive=true"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"delete","node":{"key":"/foo","dir":true,"modifiedIndex":3,"createdIndex":2}}`, "")

server/v2/tests/get_handler_test.go

@@ -2,6 +2,7 @@ package v2
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"testing"
 	"time"
@@ -13,6 +14,7 @@ import (
 
 // Ensures that a value can be retrieve for a given key.
 //
+//   $ curl localhost:4001/v2/keys/foo/bar -> fail
 //   $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX
 //   $ curl localhost:4001/v2/keys/foo/bar
 //
@@ -21,9 +23,14 @@ func TestV2GetKey(t *testing.T) {
 		v := url.Values{}
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
-		resp, _ := tests.PutForm(fullURL, v)
+		resp, _ := tests.Get(fullURL)
+		assert.Equal(t, resp.StatusCode, http.StatusNotFound)
+
+		resp, _ = tests.PutForm(fullURL, v)
 		tests.ReadBody(resp)
+
 		resp, _ = tests.Get(fullURL)
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "get", "")
 		node := body["node"].(map[string]interface{})
@@ -52,6 +59,7 @@ func TestV2GetKeyRecursively(t *testing.T) {
 		tests.ReadBody(resp)
 
 		resp, _ = tests.Get(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?recursive=true"))
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "get", "")
 		node := body["node"].(map[string]interface{})

server/v2/tests/post_handler_test.go

@@ -3,6 +3,7 @@ package v2
 import (
 	"fmt"
 	"testing"
+	"net/http"
 
 	"github.com/coreos/etcd/server"
 	"github.com/coreos/etcd/tests"
@@ -20,6 +21,7 @@ func TestV2CreateUnique(t *testing.T) {
 		// POST should add index to list.
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PostForm(fullURL, nil)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "create", "")
 
@@ -30,6 +32,7 @@ func TestV2CreateUnique(t *testing.T) {
 
 		// Second POST should add next index to list.
 		resp, _ = tests.PostForm(fullURL, nil)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body = tests.ReadBodyJSON(resp)
 
 		node = body["node"].(map[string]interface{})
@@ -37,6 +40,7 @@ func TestV2CreateUnique(t *testing.T) {
 
 		// POST to a different key should add index to that list.
 		resp, _ = tests.PostForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/baz"), nil)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body = tests.ReadBodyJSON(resp)
 
 		node = body["node"].(map[string]interface{})

server/v2/tests/put_handler_test.go

@@ -2,6 +2,7 @@ package v2
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"testing"
 	"time"
@@ -20,6 +21,7 @@ func TestV2SetKey(t *testing.T) {
 		v := url.Values{}
 		v.Set("value", "XXX")
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"set","node":{"key":"/foo/bar","value":"XXX","modifiedIndex":2,"createdIndex":2}}`, "")
@@ -33,6 +35,7 @@ func TestV2SetKey(t *testing.T) {
 func TestV2SetDirectory(t *testing.T) {
 	tests.RunServer(func(s *server.Server) {
 		resp, err := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), url.Values{})
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body := tests.ReadBody(resp)
 		assert.Nil(t, err, "")
 		assert.Equal(t, string(body), `{"action":"set","node":{"key":"/foo","dir":true,"modifiedIndex":2,"createdIndex":2}}`, "")
@@ -50,6 +53,7 @@ func TestV2SetKeyWithTTL(t *testing.T) {
 		v.Set("value", "XXX")
 		v.Set("ttl", "20")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body := tests.ReadBodyJSON(resp)
 		node := body["node"].(map[string]interface{})
 		assert.Equal(t, node["ttl"], 20, "")
@@ -70,6 +74,7 @@ func TestV2SetKeyWithBadTTL(t *testing.T) {
 		v.Set("value", "XXX")
 		v.Set("ttl", "bad_ttl")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusBadRequest)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 202, "")
 		assert.Equal(t, body["message"], "The given TTL in POST form is not a number", "")
@@ -87,6 +92,7 @@ func TestV2CreateKeySuccess(t *testing.T) {
 		v.Set("value", "XXX")
 		v.Set("prevExist", "false")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		body := tests.ReadBodyJSON(resp)
 		node := body["node"].(map[string]interface{})
 		assert.Equal(t, node["value"], "XXX", "")
@@ -105,8 +111,10 @@ func TestV2CreateKeyFail(t *testing.T) {
 		v.Set("prevExist", "false")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusPreconditionFailed)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 105, "")
 		assert.Equal(t, body["message"], "Key already exists", "")
@@ -126,11 +134,13 @@ func TestV2UpdateKeySuccess(t *testing.T) {
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 
 		v.Set("value", "YYY")
 		v.Set("prevExist", "true")
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "update", "")
 	})
@@ -146,9 +156,11 @@ func TestV2UpdateKeyFailOnValue(t *testing.T) {
 		v := url.Values{}
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo?dir=true"), v)
 
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		v.Set("value", "YYY")
 		v.Set("prevExist", "true")
 		resp, _ = tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusNotFound)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 100, "")
 		assert.Equal(t, body["message"], "Key not found", "")
@@ -167,12 +179,14 @@ func TestV2UpdateKeyFailOnMissingDirectory(t *testing.T) {
 		v.Set("value", "YYY")
 		v.Set("prevExist", "true")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusNotFound)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 100, "")
 		assert.Equal(t, body["message"], "Key not found", "")
 		assert.Equal(t, body["cause"], "/foo", "")
 
 		resp, _ = tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusNotFound)
 		body = tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 100, "")
 		assert.Equal(t, body["message"], "Key not found", "")
@@ -191,10 +205,12 @@ func TestV2SetKeyCASOnIndexSuccess(t *testing.T) {
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 		v.Set("value", "YYY")
 		v.Set("prevIndex", "2")
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "compareAndSwap", "")
 		node := body["node"].(map[string]interface{})
@@ -214,10 +230,12 @@ func TestV2SetKeyCASOnIndexFail(t *testing.T) {
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 		v.Set("value", "YYY")
 		v.Set("prevIndex", "10")
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusPreconditionFailed)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 101, "")
 		assert.Equal(t, body["message"], "Compare failed", "")
@@ -236,6 +254,7 @@ func TestV2SetKeyCASWithInvalidIndex(t *testing.T) {
 		v.Set("value", "YYY")
 		v.Set("prevIndex", "bad_index")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusBadRequest)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 203, "")
 		assert.Equal(t, body["message"], "The given index in POST form is not a number", "")
@@ -254,10 +273,12 @@ func TestV2SetKeyCASOnValueSuccess(t *testing.T) {
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 		v.Set("value", "YYY")
 		v.Set("prevValue", "XXX")
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusOK)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["action"], "compareAndSwap", "")
 		node := body["node"].(map[string]interface{})
@@ -277,10 +298,12 @@ func TestV2SetKeyCASOnValueFail(t *testing.T) {
 		v.Set("value", "XXX")
 		fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
 		resp, _ := tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusCreated)
 		tests.ReadBody(resp)
 		v.Set("value", "YYY")
 		v.Set("prevValue", "AAA")
 		resp, _ = tests.PutForm(fullURL, v)
+		assert.Equal(t, resp.StatusCode, http.StatusPreconditionFailed)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 101, "")
 		assert.Equal(t, body["message"], "Compare failed", "")
@@ -299,6 +322,7 @@ func TestV2SetKeyCASWithMissingValueFails(t *testing.T) {
 		v.Set("value", "XXX")
 		v.Set("prevValue", "")
 		resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
+		assert.Equal(t, resp.StatusCode, http.StatusBadRequest)
 		body := tests.ReadBodyJSON(resp)
 		assert.Equal(t, body["errorCode"], 201, "")
 		assert.Equal(t, body["message"], "PrevValue is Required in POST form", "")

tests/functional/v1_migration_test.go

@@ -3,6 +3,7 @@ package test
 import (
 	"fmt"
 	"io/ioutil"
+	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -91,7 +92,7 @@ func TestV1ClusterMigration(t *testing.T) {
 	resp, err := tests.Get("http://localhost:4001/v2/keys/message")
 	body := tests.ReadBody(resp)
 	assert.Nil(t, err, "")
-	assert.Equal(t, resp.StatusCode, 400)
+	assert.Equal(t, resp.StatusCode, http.StatusNotFound)
 	assert.Equal(t, string(body), `{"errorCode":100,"message":"Key not found","cause":"/message","index":11}`+"\n")
 
 	// Ensure TTL'd message is removed.