Skip to content

Commit

Permalink
Merge pull request #412 from mojotech/cas/403-proper-http-statuses
Browse files Browse the repository at this point in the history 
Use more appropriate HTTP status codes for error cases.
  • Loading branch information
@xiang90
xiang90 committed Dec 22, 2013
2 parents 557ffbb + d89fa13 commit 70c8c09
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 29 deletions.
19 changes: 14 additions & 5 deletions error/error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,19 @@ func (e Error) toJsonString() string {

func (e Error) Write(w http.ResponseWriter) {
w.Header().Add("X-Etcd-Index", fmt.Sprint(e.Index))
// 3xx is reft internal error
if e.ErrorCode/100 == 3 {
http.Error(w, e.toJsonString(), http.StatusInternalServerError)
} else {
http.Error(w, e.toJsonString(), http.StatusBadRequest)
// 3xx is raft internal error
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)
}
9 changes: 8 additions & 1 deletion server/v2/tests/delete_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v2

import (
"fmt"
"net/http"
"net/url"
"testing"

Expand All @@ -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}}`, "")
Expand All @@ -31,17 +33,19 @@ func TestV2DeleteKey(t *testing.T) {
// Ensures that an empty directory is deleted when dir is set.
//
// $ curl -X PUT localhost:4001/v2/keys/foo?dir=true
// $ curl -X PUT localhost:4001/v2/keys/foo ->fail
// $ curl -X DELETE localhost:4001/v2/keys/foo ->fail
// $ curl -X DELETE localhost:4001/v2/keys/foo?dir=true
//
func TestV2DeleteEmptyDirectory(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{})
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}}`, "")
Expand All @@ -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}}`, "")
Expand All @@ -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}}`, "")
Expand Down
13 changes: 11 additions & 2 deletions server/v2/tests/get_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v2

import (
"fmt"
"net/http"
"net/url"
"testing"
"time"
Expand All @@ -13,16 +14,23 @@ 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
//
func TestV2GetKey(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
fullURL := fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar")
resp, _ := tests.Get(fullURL)
assert.Equal(t, resp.StatusCode, http.StatusNotFound)

resp, _ = tests.PutForm(fullURL, v)
tests.ReadBody(resp)
resp, _ = tests.Get(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"))

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{})
Expand Down Expand Up @@ -51,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{})
Expand Down
9 changes: 7 additions & 2 deletions server/v2/tests/post_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package v2
import (
"fmt"
"testing"
"net/http"

"github.com/coreos/etcd/server"
"github.com/coreos/etcd/tests"
Expand All @@ -18,7 +19,9 @@ import (
func TestV2CreateUnique(t *testing.T) {
tests.RunServer(func(s *server.Server) {
// POST should add index to list.
resp, _ := tests.PostForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), nil)
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", "")

Expand All @@ -28,14 +31,16 @@ func TestV2CreateUnique(t *testing.T) {
assert.Equal(t, node["modifiedIndex"], 2, "")

// Second POST should add next index to list.
resp, _ = tests.PostForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), nil)
resp, _ = tests.PostForm(fullURL, nil)
assert.Equal(t, resp.StatusCode, http.StatusCreated)
body = tests.ReadBodyJSON(resp)

node = body["node"].(map[string]interface{})
assert.Equal(t, node["key"], "/foo/bar/3", "")

// 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{})
Expand Down
72 changes: 54 additions & 18 deletions server/v2/tests/put_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v2

import (
"fmt"
"net/http"
"net/url"
"testing"
"time"
Expand All @@ -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}}`, "")
Expand All @@ -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}}`, "")
Expand All @@ -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, "")
Expand All @@ -70,14 +74,15 @@ 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", "")
assert.Equal(t, body["cause"], "Update", "")
})
}

// Ensures that a key is conditionally set only if it previously did not exist.
// Ensures that a key is conditionally set if it previously did not exist.
//
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX -d prevExist=false
//
Expand All @@ -87,25 +92,29 @@ 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", "")
})
}

// Ensures that a key is not conditionally because it previously existed.
// Ensures that a key is not conditionally set because it previously existed.
//
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX -d prevExist=false
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX -d prevExist=false -> fail
//
func TestV2CreateKeyFail(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
v.Set("prevExist", "false")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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", "")
Expand All @@ -123,12 +132,15 @@ func TestV2UpdateKeySuccess(t *testing.T) {
v := url.Values{}

v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
resp, _ = tests.PutForm(fullURL, v)
assert.Equal(t, resp.StatusCode, http.StatusOK)
body := tests.ReadBodyJSON(resp)
assert.Equal(t, body["action"], "update", "")
})
Expand All @@ -144,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", "")
Expand All @@ -156,19 +170,27 @@ func TestV2UpdateKeyFailOnValue(t *testing.T) {

// Ensures that a key is not conditionally set if it previously did not exist.
//
// $ curl -X PUT localhost:4001/v2/keys/foo -d value=XXX -d prevExist=true
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX -d prevExist=true
// $ curl -X PUT localhost:4001/v2/keys/foo -d value=YYY -d prevExist=true -> fail
// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=YYY -d prevExist=true -> fail
//
func TestV2UpdateKeyFailOnMissingDirectory(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "YYY")
v.Set("prevExist", "true")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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", "")
assert.Equal(t, body["cause"], "/foo", "")
})
}

Expand All @@ -181,11 +203,14 @@ func TestV2SetKeyCASOnIndexSuccess(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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{})
Expand All @@ -203,11 +228,14 @@ func TestV2SetKeyCASOnIndexFail(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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", "")
Expand All @@ -226,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", "")
Expand All @@ -242,11 +271,14 @@ func TestV2SetKeyCASOnValueSuccess(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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{})
Expand All @@ -264,11 +296,14 @@ func TestV2SetKeyCASOnValueFail(t *testing.T) {
tests.RunServer(func(s *server.Server) {
v := url.Values{}
v.Set("value", "XXX")
resp, _ := tests.PutForm(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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(fmt.Sprintf("%s%s", s.URL(), "/v2/keys/foo/bar"), v)
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", "")
Expand All @@ -287,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", "")
Expand Down

0 comments on commit 70c8c09

Please sign in to comment.