From 074c78d72535dc370a42d5d2e46f298ecf56c1cd Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Thu, 6 Mar 2014 17:32:26 +0700 Subject: [PATCH] fix(store): corrected CAS and CAD fail cause in response specifically when both prevIndex and prevValue are provided --- server/v1/tests/put_handler_test.go | 2 +- server/v2/tests/put_handler_test.go | 82 ++++++++++++++++++++++++++++- store/node.go | 30 +++++++++-- store/store.go | 20 +++++-- 4 files changed, 122 insertions(+), 12 deletions(-) diff --git a/server/v1/tests/put_handler_test.go b/server/v1/tests/put_handler_test.go index 5e55a6928..3e2f0a2ba 100644 --- a/server/v1/tests/put_handler_test.go +++ b/server/v1/tests/put_handler_test.go @@ -151,7 +151,7 @@ func TestV1SetKeyCASOnValueFail(t *testing.T) { body := tests.ReadBodyJSON(resp) assert.Equal(t, body["errorCode"], 101, "") assert.Equal(t, body["message"], "Compare failed", "") - assert.Equal(t, body["cause"], "[AAA != XXX] [0 != 2]", "") + assert.Equal(t, body["cause"], "[AAA != XXX]", "") assert.Equal(t, body["index"], 2, "") }) } diff --git a/server/v2/tests/put_handler_test.go b/server/v2/tests/put_handler_test.go index 7b66415a1..ec7267847 100644 --- a/server/v2/tests/put_handler_test.go +++ b/server/v2/tests/put_handler_test.go @@ -239,7 +239,7 @@ func TestV2SetKeyCASOnIndexFail(t *testing.T) { body := tests.ReadBodyJSON(resp) assert.Equal(t, body["errorCode"], 101, "") assert.Equal(t, body["message"], "Compare failed", "") - assert.Equal(t, body["cause"], "[ != XXX] [10 != 2]", "") + assert.Equal(t, body["cause"], "[10 != 2]", "") assert.Equal(t, body["index"], 2, "") }) } @@ -307,7 +307,7 @@ func TestV2SetKeyCASOnValueFail(t *testing.T) { body := tests.ReadBodyJSON(resp) assert.Equal(t, body["errorCode"], 101, "") assert.Equal(t, body["message"], "Compare failed", "") - assert.Equal(t, body["cause"], "[AAA != XXX] [0 != 2]", "") + assert.Equal(t, body["cause"], "[AAA != XXX]", "") assert.Equal(t, body["index"], 2, "") }) } @@ -330,6 +330,84 @@ func TestV2SetKeyCASWithMissingValueFails(t *testing.T) { }) } +// Ensures that a key is not set if both previous value and index do not match. +// +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=YYY -d prevValue=AAA -d prevIndex=3 +// +func TestV2SetKeyCASOnValueAndIndexFail(t *testing.T) { + tests.RunServer(func(s *server.Server) { + v := url.Values{} + 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") + v.Set("prevIndex", "3") + 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", "") + assert.Equal(t, body["cause"], "[AAA != XXX] [3 != 2]", "") + assert.Equal(t, body["index"], 2, "") + }) +} + +// Ensures that a key is not set if previous value match but index does not. +// +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=YYY -d prevValue=XXX -d prevIndex=3 +// +func TestV2SetKeyCASOnValueMatchAndIndexFail(t *testing.T) { + tests.RunServer(func(s *server.Server) { + v := url.Values{} + 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") + v.Set("prevIndex", "3") + 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", "") + assert.Equal(t, body["cause"], "[3 != 2]", "") + assert.Equal(t, body["index"], 2, "") + }) +} + +// Ensures that a key is not set if previous index matches but value does not. +// +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=XXX +// $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=YYY -d prevValue=AAA -d prevIndex=2 +// +func TestV2SetKeyCASOnIndexMatchAndValueFail(t *testing.T) { + tests.RunServer(func(s *server.Server) { + v := url.Values{} + 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") + v.Set("prevIndex", "2") + 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", "") + assert.Equal(t, body["cause"], "[AAA != XXX]", "") + assert.Equal(t, body["index"], 2, "") + }) +} + // Ensure that we can set an empty value // // $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value= diff --git a/store/node.go b/store/node.go index acc54f91b..95d5d067d 100644 --- a/store/node.go +++ b/store/node.go @@ -9,6 +9,14 @@ import ( ustrings "github.com/coreos/etcd/pkg/strings" ) +// explanations of Compare function result +const ( + CompareMatch = 0 + CompareIndexNotMatch = 1 + CompareValueNotMatch = 2 + CompareNotMatch = 3 +) + var Permanent time.Time // node is the basic element in the store system. @@ -321,11 +329,23 @@ func (n *node) UpdateTTL(expireTime time.Time) { } } -func (n *node) Compare(prevValue string, prevIndex uint64) bool { - compareValue := (prevValue == "" || n.Value == prevValue) - compareIndex := (prevIndex == 0 || n.ModifiedIndex == prevIndex) - - return compareValue && compareIndex +// Compare function compares node index and value with provided ones. +// second result value explains result and equals to one of Compare.. constants +func (n *node) Compare(prevValue string, prevIndex uint64) (ok bool, which int) { + indexMatch := (prevIndex == 0 || n.ModifiedIndex == prevIndex) + valueMatch := (prevValue == "" || n.Value == prevValue) + ok = valueMatch && indexMatch + switch { + case valueMatch && indexMatch: + which = CompareMatch + case indexMatch && !valueMatch: + which = CompareValueNotMatch + case valueMatch && !indexMatch: + which = CompareIndexNotMatch + default: + which = CompareNotMatch + } + return } // Clone function clone the node recursively and return the new node. diff --git a/store/store.go b/store/store.go index 6874768f3..4dbe2038a 100644 --- a/store/store.go +++ b/store/store.go @@ -181,6 +181,18 @@ func (s *store) Set(nodePath string, dir bool, value string, expireTime time.Tim return e, nil } +// returns user-readable cause of failed comparison +func getCompareFailCause(n *node, which int, prevValue string, prevIndex uint64) string { + switch which { + case CompareIndexNotMatch: + return fmt.Sprintf("[%v != %v]", prevIndex, n.ModifiedIndex) + case CompareValueNotMatch: + return fmt.Sprintf("[%v != %v]", prevValue, n.Value) + default: + return fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex) + } +} + func (s *store) CompareAndSwap(nodePath string, prevValue string, prevIndex uint64, value string, expireTime time.Time) (*Event, error) { @@ -207,8 +219,8 @@ func (s *store) CompareAndSwap(nodePath string, prevValue string, prevIndex uint // If both of the prevValue and prevIndex are given, we will test both of them. // Command will be executed, only if both of the tests are successful. - if !n.Compare(prevValue, prevIndex) { - cause := fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex) + if ok, which := n.Compare(prevValue, prevIndex); !ok { + cause := getCompareFailCause(n, which, prevValue, prevIndex) s.Stats.Inc(CompareAndSwapFail) return nil, etcdErr.NewError(etcdErr.EcodeTestFailed, cause, s.CurrentIndex) } @@ -309,8 +321,8 @@ func (s *store) CompareAndDelete(nodePath string, prevValue string, prevIndex ui // If both of the prevValue and prevIndex are given, we will test both of them. // Command will be executed, only if both of the tests are successful. - if !n.Compare(prevValue, prevIndex) { - cause := fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex) + if ok, which := n.Compare(prevValue, prevIndex); !ok { + cause := getCompareFailCause(n, which, prevValue, prevIndex) s.Stats.Inc(CompareAndDeleteFail) return nil, etcdErr.NewError(etcdErr.EcodeTestFailed, cause, s.CurrentIndex) }