From 9cfd8c5f0b65810e5ddc46e1240614f6005662f6 Mon Sep 17 00:00:00 2001 From: evan-gu Date: Mon, 3 Feb 2014 20:12:36 -0500 Subject: [PATCH] fix(store): make NodeExtern.Value a *string Before this change if the value of a Node was "" it would get dropped from the json encoding and the Node.Value field would be missing. Fix this problem by making NodeExtern.Value a *string so that an empty string will be encoded but a nil value will drop the field. --- pkg/strings/string.go | 7 +++ server/registry.go | 2 +- server/v2/tests/put_handler_test.go | 15 ++++++ store/event.go | 2 +- store/node.go | 5 +- store/node_extern.go | 7 +-- store/response_v1.go | 10 ++-- store/store.go | 14 ++++-- store/store_test.go | 76 ++++++++++++++--------------- 9 files changed, 86 insertions(+), 52 deletions(-) diff --git a/pkg/strings/string.go b/pkg/strings/string.go index 5d898e43a..688b69d31 100644 --- a/pkg/strings/string.go +++ b/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 +} diff --git a/server/registry.go b/server/registry.go index 9ec515d50..46be3ecc6 100644 --- a/server/registry.go +++ b/server/registry.go @@ -168,7 +168,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)) } diff --git a/server/v2/tests/put_handler_test.go b/server/v2/tests/put_handler_test.go index 580eefc3c..7b66415a1 100644 --- a/server/v2/tests/put_handler_test.go +++ b/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}}`) + }) +} diff --git a/store/event.go b/store/event.go index cd7e0815d..dad91ee20 100644 --- a/store/event.go +++ b/store/event.go @@ -64,7 +64,7 @@ func (event *Event) Response(currentIndex uint64) interface{} { } if response.Action == Set { - if response.PrevValue == "" { + if response.PrevValue == nil { response.NewKey = true } } diff --git a/store/node.go b/store/node.go index 0b950b07d..acc54f91b 100644 --- a/store/node.go +++ b/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, } diff --git a/store/node_extern.go b/store/node_extern.go index f1ae45c88..1650238a6 100644 --- a/store/node_extern.go +++ b/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() diff --git a/store/response_v1.go b/store/response_v1.go index 9ff8aa2ac..5b9244bf9 100644 --- a/store/response_v1.go +++ b/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 diff --git a/store/store.go b/store/store.go index 2492d357c..6874768f3 100644 --- a/store/store.go +++ b/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) diff --git a/store/store_test.go b/store/store_test.go index bc5b00bbd..7cf237a55 100644 --- a/store/store_test.go +++ b/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, "")