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

@@ -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, "")
 	})
 }

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=

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 {
+// Compare function compares node index and value with provided ones.
-	compareValue := (prevValue == "" || n.Value == prevValue)
+// second result value explains result and equals to one of Compare.. constants
-	compareIndex := (prevIndex == 0 || n.ModifiedIndex == prevIndex)
+func (n *node) Compare(prevValue string, prevIndex uint64) (ok bool, which int) {
-
+	indexMatch := (prevIndex == 0 || n.ModifiedIndex == prevIndex)
-	return compareValue && compareIndex
+	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.

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) {
+	if ok, which := n.Compare(prevValue, prevIndex); !ok {
-		cause := fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex)
+		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) {
+	if ok, which := n.Compare(prevValue, prevIndex); !ok {
-		cause := fmt.Sprintf("[%v != %v] [%v != %v]", prevValue, n.Value, prevIndex, n.ModifiedIndex)
+		cause := getCompareFailCause(n, which, prevValue, prevIndex)
 		s.Stats.Inc(CompareAndDeleteFail)
 		return nil, etcdErr.NewError(etcdErr.EcodeTestFailed, cause, s.CurrentIndex)
 	}