Merge pull request #524 from yifan-gu/remove_omitempty_on_value

remove omitempty on value

pkg/strings/string.go

@@ -14,3 +14,10 @@ func TrimSplit(s, sep string) []string {
 	}
 	return trimmed
 }
+
+// Clone returns a copy of the string, so that we can safely point to the
+// copy without worrying about changes via pointers.
+func Clone(s string) string {
+	stringCopy := s
+	return stringCopy
+}

server/registry.go

@@ -180,7 +180,7 @@ func (r *Registry) load(name string) {
 	}
 
 	// Parse as a query string.
-	m, err := url.ParseQuery(e.Node.Value)
+	m, err := url.ParseQuery(*e.Node.Value)
 	if err != nil {
 		panic(fmt.Sprintf("Failed to parse peers entry: %s", name))
 	}

server/v2/tests/put_handler_test.go

@@ -329,3 +329,18 @@ func TestV2SetKeyCASWithMissingValueFails(t *testing.T) {
 		assert.Equal(t, body["cause"], "CompareAndSwap", "")
 	})
 }
+
+// Ensure that we can set an empty value
+//
+//   $ curl -X PUT localhost:4001/v2/keys/foo/bar -d value=
+//
+func TestV2SetKeyCASWithEmptyValueSuccess(t *testing.T) {
+	tests.RunServer(func(s *server.Server) {
+		v := url.Values{}
+		v.Set("value", "")
+		resp, _ := 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.Equal(t, string(body), `{"action":"set","node":{"key":"/foo/bar","value":"","modifiedIndex":2,"createdIndex":2}}`)
+	})
+}

store/event.go

@@ -67,7 +67,7 @@ func (event *Event) Response(currentIndex uint64) interface{} {
 		}
 
 		if response.Action == Set {
-			if response.PrevValue == "" {
+			if response.PrevValue == nil {
 				response.NewKey = true
 			}
 		}

store/node.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	etcdErr "github.com/coreos/etcd/error"
+	ustrings "github.com/coreos/etcd/pkg/strings"
 )
 
 var Permanent time.Time
@@ -284,9 +285,11 @@ func (n *node) Repr(recurisive, sorted bool) *NodeExtern {
 		return node
 	}
 
+	// since n.Value could be changed later, so we need to copy the value out
+	value := ustrings.Clone(n.Value)
 	node := &NodeExtern{
 		Key:           n.Path,
-		Value:         n.Value,
+		Value:         &value,
 		ModifiedIndex: n.ModifiedIndex,
 		CreatedIndex:  n.CreatedIndex,
 	}

store/node_extern.go

@@ -10,8 +10,8 @@ import (
 // PrevValue is the previous value of the node
 // TTL is time to live in second
 type NodeExtern struct {
-	Key           string      `json:"key, omitempty"`
-	Value         string      `json:"value,omitempty"`
+	Key           string      `json:"key,omitempty"`
+	Value         *string     `json:"value,omitempty"`
 	Dir           bool        `json:"dir,omitempty"`
 	Expiration    *time.Time  `json:"expiration,omitempty"`
 	TTL           int64       `json:"ttl,omitempty"`
@@ -48,7 +48,8 @@ func (eNode *NodeExtern) loadInternalNode(n *node, recursive, sorted bool) {
 		}
 
 	} else { // node is a file
-		eNode.Value, _ = n.Read()
+		value, _ := n.Read()
+		eNode.Value = &value
 	}
 
 	eNode.Expiration, eNode.TTL = n.ExpirationAndTTL()

store/response_v1.go

@@ -6,11 +6,11 @@ import (
 
 // The response from the store to the user who issue a command
 type Response struct {
-	Action    string `json:"action"`
-	Key       string `json:"key"`
-	Dir       bool   `json:"dir,omitempty"`
-	PrevValue string `json:"prevValue,omitempty"`
-	Value     string `json:"value,omitempty"`
+	Action    string  `json:"action"`
+	Key       string  `json:"key"`
+	Dir       bool    `json:"dir,omitempty"`
+	PrevValue *string `json:"prevValue,omitempty"`
+	Value     *string `json:"value,omitempty"`
 
 	// If the key did not exist before the action,
 	// this field should be set to true

store/store.go

@@ -26,6 +26,7 @@ import (
 	"time"
 
 	etcdErr "github.com/coreos/etcd/error"
+	ustrings "github.com/coreos/etcd/pkg/strings"
 )
 
 // The default version to set when the store is first initialized.
@@ -223,7 +224,9 @@ func (s *store) CompareAndSwap(nodePath string, prevValue string, prevIndex uint
 	n.Write(value, s.CurrentIndex)
 	n.UpdateTTL(expireTime)
 
-	eNode.Value = value
+	// copy the value for safety
+	valueCopy := ustrings.Clone(value)
+	eNode.Value = &valueCopy
 	eNode.Expiration, eNode.TTL = n.ExpirationAndTTL()
 
 	s.WatcherHub.notify(e)
@@ -413,7 +416,10 @@ func (s *store) Update(nodePath string, newValue string, expireTime time.Time) (
 	}
 
 	n.Write(newValue, nextIndex)
-	eNode.Value = newValue
+
+	// copy the value for safety
+	newValueCopy := ustrings.Clone(newValue)
+	eNode.Value = &newValueCopy
 
 	// update ttl
 	n.UpdateTTL(expireTime)
@@ -482,7 +488,9 @@ func (s *store) internalCreate(nodePath string, dir bool, value string, unique,
 	}
 
 	if !dir { // create file
-		eNode.Value = value
+		// copy the value for safety
+		valueCopy := ustrings.Clone(value)
+		eNode.Value = &valueCopy
 
 		n = newKV(s, nodePath, value, nextIndex, d, "", expireTime)
 

store/store_test.go

@@ -32,7 +32,7 @@ func TestStoreGetValue(t *testing.T) {
 	assert.Nil(t, err, "")
 	assert.Equal(t, e.Action, "get", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 }
 
 // Ensure that the store can recrusively retrieve a directory listing.
@@ -52,16 +52,16 @@ func TestStoreGetDirectory(t *testing.T) {
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.Equal(t, len(e.Node.Nodes), 2, "")
 	assert.Equal(t, e.Node.Nodes[0].Key, "/foo/bar", "")
-	assert.Equal(t, e.Node.Nodes[0].Value, "X", "")
+	assert.Equal(t, *e.Node.Nodes[0].Value, "X", "")
 	assert.Equal(t, e.Node.Nodes[0].Dir, false, "")
 	assert.Equal(t, e.Node.Nodes[1].Key, "/foo/baz", "")
 	assert.Equal(t, e.Node.Nodes[1].Dir, true, "")
 	assert.Equal(t, len(e.Node.Nodes[1].Nodes), 2, "")
 	assert.Equal(t, e.Node.Nodes[1].Nodes[0].Key, "/foo/baz/bat", "")
-	assert.Equal(t, e.Node.Nodes[1].Nodes[0].Value, "Y", "")
+	assert.Equal(t, *e.Node.Nodes[1].Nodes[0].Value, "Y", "")
 	assert.Equal(t, e.Node.Nodes[1].Nodes[0].Dir, false, "")
 	assert.Equal(t, e.Node.Nodes[1].Nodes[1].Key, "/foo/baz/ttl", "")
-	assert.Equal(t, e.Node.Nodes[1].Nodes[1].Value, "Y", "")
+	assert.Equal(t, *e.Node.Nodes[1].Nodes[1].Value, "Y", "")
 	assert.Equal(t, e.Node.Nodes[1].Nodes[1].Dir, false, "")
 	assert.Equal(t, e.Node.Nodes[1].Nodes[1].TTL, 3, "")
 }
@@ -93,7 +93,7 @@ func TestSet(t *testing.T) {
 	assert.Equal(t, e.Action, "set", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "", "")
+	assert.Equal(t, *e.Node.Value, "", "")
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -105,7 +105,7 @@ func TestSet(t *testing.T) {
 	assert.Equal(t, e.Action, "set", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -113,7 +113,7 @@ func TestSet(t *testing.T) {
 	// check prevNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "", "")
+	assert.Equal(t, *e.PrevNode.Value, "", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 	// Set /foo="baz" (for testing prevNode)
 	e, err = s.Set("/foo", false, "baz", Permanent)
@@ -121,7 +121,7 @@ func TestSet(t *testing.T) {
 	assert.Equal(t, e.Action, "set", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -129,7 +129,7 @@ func TestSet(t *testing.T) {
 	// check prevNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(2), "")
 
 	// Set /dir as a directory
@@ -138,7 +138,7 @@ func TestSet(t *testing.T) {
 	assert.Equal(t, e.Action, "set", "")
 	assert.Equal(t, e.Node.Key, "/dir", "")
 	assert.True(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "", "")
+	assert.Nil(t, e.Node.Value)
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -154,7 +154,7 @@ func TestStoreCreateValue(t *testing.T) {
 	assert.Equal(t, e.Action, "create", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -166,7 +166,7 @@ func TestStoreCreateValue(t *testing.T) {
 	assert.Equal(t, e.Action, "create", "")
 	assert.Equal(t, e.Node.Key, "/empty", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "", "")
+	assert.Equal(t, *e.Node.Value, "", "")
 	assert.Nil(t, e.Node.Nodes, "")
 	assert.Nil(t, e.Node.Expiration, "")
 	assert.Equal(t, e.Node.TTL, 0, "")
@@ -211,17 +211,17 @@ func TestStoreUpdateValue(t *testing.T) {
 	assert.Equal(t, e.Action, "update", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 	assert.Equal(t, e.Node.TTL, 0, "")
 	assert.Equal(t, e.Node.ModifiedIndex, uint64(2), "")
 	// check prevNode
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.TTL, 0, "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 
 	// update /foo=""
 	e, err = s.Update("/foo", "", Permanent)
@@ -229,17 +229,17 @@ func TestStoreUpdateValue(t *testing.T) {
 	assert.Equal(t, e.Action, "update", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
 	assert.False(t, e.Node.Dir, "")
-	assert.Equal(t, e.Node.Value, "", "")
+	assert.Equal(t, *e.Node.Value, "", "")
 	assert.Equal(t, e.Node.TTL, 0, "")
 	assert.Equal(t, e.Node.ModifiedIndex, uint64(3), "")
 	// check prevNode
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "baz", "")
+	assert.Equal(t, *e.PrevNode.Value, "baz", "")
 	assert.Equal(t, e.PrevNode.TTL, 0, "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(2), "")
 
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "", "")
+	assert.Equal(t, *e.Node.Value, "", "")
 }
 
 // Ensure that the store cannot update a directory.
@@ -267,7 +267,7 @@ func TestStoreUpdateValueTTL(t *testing.T) {
 	s.Create("/foo", false, "bar", false, Permanent)
 	_, err := s.Update("/foo", "baz", time.Now().Add(500*time.Millisecond))
 	e, _ := s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 
 	time.Sleep(600 * time.Millisecond)
 	e, err = s.Get("/foo", false, false)
@@ -289,7 +289,7 @@ func TestStoreUpdateDirTTL(t *testing.T) {
 	s.Create("/foo/bar", false, "baz", false, Permanent)
 	_, err := s.Update("/foo", "", time.Now().Add(500*time.Millisecond))
 	e, _ := s.Get("/foo/bar", false, false)
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 
 	time.Sleep(600 * time.Millisecond)
 	e, err = s.Get("/foo/bar", false, false)
@@ -307,7 +307,7 @@ func TestStoreDeleteValue(t *testing.T) {
 	// check pervNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 }
 
 // Ensure that the store can delete a directory if recursive is specified.
@@ -384,7 +384,7 @@ func TestStoreCompareAndDeletePrevValue(t *testing.T) {
 	// check pervNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 	assert.Equal(t, e.PrevNode.CreatedIndex, uint64(1), "")
 }
@@ -398,7 +398,7 @@ func TestStoreCompareAndDeletePrevValueFailsIfNotMatch(t *testing.T) {
 	assert.Equal(t, err.Message, "Compare failed", "")
 	assert.Nil(t, e, "")
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 }
 
 func TestStoreCompareAndDeletePrevIndex(t *testing.T) {
@@ -410,7 +410,7 @@ func TestStoreCompareAndDeletePrevIndex(t *testing.T) {
 	// check pervNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 	assert.Equal(t, e.PrevNode.CreatedIndex, uint64(1), "")
 }
@@ -425,7 +425,7 @@ func TestStoreCompareAndDeletePrevIndexFailsIfNotMatch(t *testing.T) {
 	assert.Equal(t, err.Message, "Compare failed", "")
 	assert.Nil(t, e, "")
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 }
 
 // Ensure that the store cannot delete a directory.
@@ -445,16 +445,16 @@ func TestStoreCompareAndSwapPrevValue(t *testing.T) {
 	e, err := s.CompareAndSwap("/foo", "bar", 0, "baz", Permanent)
 	assert.Nil(t, err, "")
 	assert.Equal(t, e.Action, "compareAndSwap", "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 	// check pervNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 	assert.Equal(t, e.PrevNode.CreatedIndex, uint64(1), "")
 
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 }
 
 // Ensure that the store cannot conditionally update a key if it has the wrong previous value.
@@ -467,7 +467,7 @@ func TestStoreCompareAndSwapPrevValueFailsIfNotMatch(t *testing.T) {
 	assert.Equal(t, err.Message, "Compare failed", "")
 	assert.Nil(t, e, "")
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 }
 
 // Ensure that the store can conditionally update a key if it has a previous index.
@@ -477,16 +477,16 @@ func TestStoreCompareAndSwapPrevIndex(t *testing.T) {
 	e, err := s.CompareAndSwap("/foo", "", 1, "baz", Permanent)
 	assert.Nil(t, err, "")
 	assert.Equal(t, e.Action, "compareAndSwap", "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 	// check pervNode
 	assert.NotNil(t, e.PrevNode, "")
 	assert.Equal(t, e.PrevNode.Key, "/foo", "")
-	assert.Equal(t, e.PrevNode.Value, "bar", "")
+	assert.Equal(t, *e.PrevNode.Value, "bar", "")
 	assert.Equal(t, e.PrevNode.ModifiedIndex, uint64(1), "")
 	assert.Equal(t, e.PrevNode.CreatedIndex, uint64(1), "")
 
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 }
 
 // Ensure that the store cannot conditionally update a key if it has the wrong previous index.
@@ -499,7 +499,7 @@ func TestStoreCompareAndSwapPrevIndexFailsIfNotMatch(t *testing.T) {
 	assert.Equal(t, err.Message, "Compare failed", "")
 	assert.Nil(t, e, "")
 	e, _ = s.Get("/foo", false, false)
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 }
 
 // Ensure that the store can watch for key creation.
@@ -627,7 +627,7 @@ func TestStoreWatchStream(t *testing.T) {
 	e := nbselect(w.EventChan)
 	assert.Equal(t, e.Action, "create", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 	e = nbselect(w.EventChan)
 	assert.Nil(t, e, "")
 	// second modification
@@ -635,7 +635,7 @@ func TestStoreWatchStream(t *testing.T) {
 	e = nbselect(w.EventChan)
 	assert.Equal(t, e.Action, "update", "")
 	assert.Equal(t, e.Node.Key, "/foo", "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 	e = nbselect(w.EventChan)
 	assert.Nil(t, e, "")
 }
@@ -653,11 +653,11 @@ func TestStoreRecover(t *testing.T) {
 
 	e, err := s.Get("/foo/x", false, false)
 	assert.Nil(t, err, "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 
 	e, err = s.Get("/foo/y", false, false)
 	assert.Nil(t, err, "")
-	assert.Equal(t, e.Node.Value, "baz", "")
+	assert.Equal(t, *e.Node.Value, "baz", "")
 }
 
 // Ensure that the store can recover from a previously saved state that includes an expiring key.
@@ -691,7 +691,7 @@ func TestStoreRecoverWithExpiration(t *testing.T) {
 
 	e, err := s.Get("/foo/x", false, false)
 	assert.Nil(t, err, "")
-	assert.Equal(t, e.Node.Value, "bar", "")
+	assert.Equal(t, *e.Node.Value, "bar", "")
 
 	e, err = s.Get("/foo/y", false, false)
 	assert.NotNil(t, err, "")