From ba98de6ef03b6e9faf942dab6ff0db7708ef0461 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 5 Feb 2014 22:34:41 -0500 Subject: [PATCH 1/3] fix(watch hidden key) Fix hidden keys preventing deeper recursive watches from receiving events If a watcher has given the correct hidden directory, we should allow it to watch the non-hidden events under that hidden directory. This pull request achieves this by checking if the path after the watching prefix has a "/_" which indicates a hidden key. --- store/store_test.go | 13 +++++++++++++ store/watcher_hub.go | 9 +++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index ba398cf88..d39e9993b 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -782,6 +782,19 @@ func TestStoreWatchExpireWithHiddenKey(t *testing.T) { assert.Equal(t, e.Node.Key, "/foofoo", "") } +// Ensure that the store does see hidden key creates if watching deeper than a hidden key in recursive mode. +func TestStoreWatchRecursiveCreateDeeperThanHiddenKey(t *testing.T) { + s := newStore() + w, _ := s.Watch("/_foo/bar", true, false, 0) + s.Create("/_foo/bar/baz", false, "baz", false, Permanent) + + e := nbselect(w.EventChan) + // The NotNil assertion currently fails + assert.NotNil(t, e, "") + assert.Equal(t, e.Action, "create", "") + assert.Equal(t, e.Node.Key, "/_foo/bar/baz", "") +} + // Performs a non-blocking select on an event channel. func nbselect(c <-chan *Event) *Event { select { diff --git a/store/watcher_hub.go b/store/watcher_hub.go index 3ed8d2697..1ca4c9bf5 100644 --- a/store/watcher_hub.go +++ b/store/watcher_hub.go @@ -127,7 +127,7 @@ func (wh *watcherHub) notifyWatchers(e *Event, nodePath string, deleted bool) { w, _ := curr.Value.(*Watcher) originalPath := (e.Node.Key == nodePath) - if (originalPath || !isHidden(e.Node.Key)) && w.notify(e, originalPath, deleted) { + if (originalPath || !isHidden(nodePath, e.Node.Key)) && w.notify(e, originalPath, deleted) { if !w.stream { // do not remove the stream watcher // if we successfully notify a watcher // we need to remove the watcher from the list @@ -158,8 +158,9 @@ func (wh *watcherHub) clone() *watcherHub { } } -// isHidden checks to see if this path is considered hidden i.e. the +// isHidden checks to see if key path is considered hidden to watch path i.e. the // last element is hidden or it's within a hidden directory -func isHidden(nodePath string) bool { - return strings.Contains(nodePath, "/_") +func isHidden(watchPath, keyPath string) bool { + afterPath := path.Clean("/" + keyPath[len(watchPath):]) + return strings.Contains(afterPath, "/_") } From 5851cb5b8db8af6088601d88833981d9ef80c6b8 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 5 Feb 2014 23:31:38 -0500 Subject: [PATCH 2/3] chrod(watcher_hub) add comment to isHidden function --- store/watcher_hub.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/store/watcher_hub.go b/store/watcher_hub.go index 1ca4c9bf5..737fab028 100644 --- a/store/watcher_hub.go +++ b/store/watcher_hub.go @@ -161,6 +161,8 @@ func (wh *watcherHub) clone() *watcherHub { // isHidden checks to see if key path is considered hidden to watch path i.e. the // last element is hidden or it's within a hidden directory func isHidden(watchPath, keyPath string) bool { + // if watch path is just a "/", after path will start without "/" + // add a "/" to deal with the special case when watchPath is "/" afterPath := path.Clean("/" + keyPath[len(watchPath):]) return strings.Contains(afterPath, "/_") } From 1b5f9eb013ca9bb884ae0e726010c5b4998b9c35 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 5 Feb 2014 23:32:12 -0500 Subject: [PATCH 3/3] test (isHidden) add unit test for isHidden function --- store/store_test.go | 1 - store/watcher_hub_test.go | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 store/watcher_hub_test.go diff --git a/store/store_test.go b/store/store_test.go index d39e9993b..aa86f5206 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -789,7 +789,6 @@ func TestStoreWatchRecursiveCreateDeeperThanHiddenKey(t *testing.T) { s.Create("/_foo/bar/baz", false, "baz", false, Permanent) e := nbselect(w.EventChan) - // The NotNil assertion currently fails assert.NotNil(t, e, "") assert.Equal(t, e.Action, "create", "") assert.Equal(t, e.Node.Key, "/_foo/bar/baz", "") diff --git a/store/watcher_hub_test.go b/store/watcher_hub_test.go new file mode 100644 index 000000000..08a30f71b --- /dev/null +++ b/store/watcher_hub_test.go @@ -0,0 +1,43 @@ +package store + +import ( + "testing" +) + +// TestIsHidden tests isHidden functions. +func TestIsHidden(t *testing.T) { + // watch at "/" + // key is "/_foo", hidden to "/" + // expected: hidden = true + watch := "/" + key := "/_foo" + hidden := isHidden(watch, key) + if !hidden { + t.Fatalf("%v should be hidden to %v\n", key, watch) + } + + // watch at "/_foo" + // key is "/_foo", not hidden to "/_foo" + // expected: hidden = false + watch = "/_foo" + hidden = isHidden(watch, key) + if hidden { + t.Fatalf("%v should not be hidden to %v\n", key, watch) + } + + // watch at "/_foo/" + // key is "/_foo/foo", not hidden to "/_foo" + key = "/_foo/foo" + hidden = isHidden(watch, key) + if hidden { + t.Fatalf("%v should not be hidden to %v\n", key, watch) + } + + // watch at "/_foo/" + // key is "/_foo/_foo", hidden to "/_foo" + key = "/_foo/_foo" + hidden = isHidden(watch, key) + if !hidden { + t.Fatalf("%v should be hidden to %v\n", key, watch) + } +}