Change delete CAS behavior to require ModifyIndex

command/kv_delete.go

@@ -43,8 +43,7 @@ KV Delete Options:
                           the next query. The default value is false.
 
   -modify-index=<int>     Unsigned integer representing the ModifyIndex of the
-                          key. This is often combined with the -cas flag, but it
+                          key. This is used in combination with the -cas flag.
-                          can be specified for any key. The default value is 0.
 
   -recurse                Recursively delete all keys with the path. The default
                           value is false.
@@ -57,7 +56,6 @@ func (c *KVDeleteCommand) Run(args []string) int {
 	cmdFlags.Usage = func() { c.Ui.Output(c.Help()) }
 	datacenter := cmdFlags.String("datacenter", "", "")
 	token := cmdFlags.String("token", "", "")
-	stale := cmdFlags.Bool("stale", false, "")
 	cas := cmdFlags.Bool("cas", false, "")
 	modifyIndex := cmdFlags.Uint64("modify-index", 0, "")
 	recurse := cmdFlags.Bool("recurse", false, "")
@@ -94,15 +92,22 @@ func (c *KVDeleteCommand) Run(args []string) int {
 		return 1
 	}
 
+	// ModifyIndex is required for CAS
+	if *cas && *modifyIndex == 0 {
+		c.Ui.Error("Must specify -modify-index with -cas!")
+		return 1
+	}
+
+	// Specifying a ModifyIndex for a non-CAS operation is not possible.
+	if *modifyIndex != 0 && !*cas {
+		c.Ui.Error("Cannot specify -modify-index without -cas!")
+	}
+
 	// It is not valid to use a CAS and recurse in the same call
 	if *recurse && *cas {
 		c.Ui.Error("Cannot specify both -cas and -recurse!")
 		return 1
 	}
-	if *recurse && *modifyIndex != 0 {
-		c.Ui.Error("Cannot specify both -modify-index and -recurse!")
-		return 1
-	}
 
 	// Create and test the HTTP client
 	conf := api.DefaultConfig()
@@ -133,23 +138,6 @@ func (c *KVDeleteCommand) Run(args []string) int {
 			ModifyIndex: *modifyIndex,
 		}
 
-		// If the user did not supply a -modify-index, but wants a check-and-set,
-		// grab the current modify index and store that on the key.
-		if pair.ModifyIndex == 0 {
-			currentPair, _, err := client.KV().Get(key, &api.QueryOptions{
-				Datacenter: *datacenter,
-				Token:      *token,
-				AllowStale: *stale,
-			})
-			if err != nil {
-				c.Ui.Error(fmt.Sprintf("Error! Could not get current key: %s", err))
-				return 1
-			}
-			if currentPair != nil {
-				pair.ModifyIndex = currentPair.ModifyIndex
-			}
-		}
-
 		success, _, err := client.KV().DeleteCAS(pair, wo)
 		if err != nil {
 			c.Ui.Error(fmt.Sprintf("Error! Did not delete key %s: %s", key, err))

command/kv_delete_test.go

@@ -1,6 +1,7 @@
 package command
 
 import (
+	"strconv"
 	"strings"
 	"testing"
 
@@ -25,12 +26,16 @@ func TestKVDeleteCommand_Validation(t *testing.T) {
 		output string
 	}{
 		"-cas and -recurse": {
-			[]string{"-cas", "-recurse"},
+			[]string{"-cas", "-modify-index", "2", "-recurse", "foo"},
 			"Cannot specify both",
 		},
-		"-modify-index and -recurse": {
+		"-cas no -modify-index": {
-			[]string{"-modify-index", "2", "-recurse"},
+			[]string{"-cas", "foo"},
-			"Cannot specify both",
+			"Must specify -modify-index",
+		},
+		"-modify-index no -cas": {
+			[]string{"-modify-index", "2", "foo"},
+			"Cannot specify -modify-index without",
 		},
 		"no key": {
 			[]string{},
@@ -141,3 +146,62 @@ func TestKVDeleteCommand_Recurse(t *testing.T) {
 		}
 	}
 }
+
+func TestKVDeleteCommand_CAS(t *testing.T) {
+	srv, client := testAgentWithAPIClient(t)
+	defer srv.Shutdown()
+	waitForLeader(t, srv.httpAddr)
+
+	ui := new(cli.MockUi)
+	c := &KVDeleteCommand{Ui: ui}
+
+	pair := &api.KVPair{
+		Key:   "foo",
+		Value: []byte("bar"),
+	}
+	_, err := client.KV().Put(pair, nil)
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+
+	args := []string{
+		"-http-addr=" + srv.httpAddr,
+		"-cas",
+		"-modify-index", "1",
+		"foo",
+	}
+
+	code := c.Run(args)
+	if code == 0 {
+		t.Fatalf("bad: expected error")
+	}
+
+	data, _, err := client.KV().Get("foo", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Reset buffers
+	ui.OutputWriter.Reset()
+	ui.ErrorWriter.Reset()
+
+	args = []string{
+		"-http-addr=" + srv.httpAddr,
+		"-cas",
+		"-modify-index", strconv.FormatUint(data.ModifyIndex, 10),
+		"foo",
+	}
+
+	code = c.Run(args)
+	if code != 0 {
+		t.Fatalf("bad: %d. %#v", code, ui.ErrorWriter.String())
+	}
+
+	data, _, err = client.KV().Get("foo", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if data != nil {
+		t.Fatalf("bad: %#v", data)
+	}
+}

website/source/docs/commands/kv/delete.html.markdown.erb

@@ -26,8 +26,7 @@ Usage: `consul kv delete [options] KEY_OR_PREFIX`
   will be used on the next query. The default value is false.
 
 * `-modify-index=<int>` - Unsigned integer representing the ModifyIndex of the
-  key. This is often combined with the -cas flag, but it can be specified for
+  key. This is used in combination with the -cas flag.
-  any key. The default value is 0.
 
 * `-recurse` - Recursively delete all keys with the path. The default value is
   false.
@@ -64,15 +63,6 @@ $ consul kv delete -cas -modify-index=456 redis/config/connections
 Success! Deleted key: redis/config/connections
 ```
 
-It is also possible to have Consul fetch the current ModifyIndex before making
-the query, by omitting the `-modify-index` flag. If the data is changed between
-the initial read and the write, the operation will fail.
-
-```
-$ consul kv delete -cas redis/config/connections
-Success! Deleted key: redis/config/connections
-```
-
 To recursively delete all keys that start with a given prefix, specify the
 `-recurse` flag: