Ensure ClearCache removes attributes in parent list as well

Makes ClearCache more thorough about clearing data about the entry.
Attributes and metadata frequently come from the parent's List command,
so clear that cache as well.

Fixes puppetlabs-toy-chest#667.

Signed-off-by: Michael Smith <[email protected]>

api/cache.go

@@ -29,7 +29,7 @@ var cacheHandler = handler{fn: func(w http.ResponseWriter, r *http.Request) *err
 		return errResp
 	}
 
-	deleted := plugin.ClearCacheFor(path)
+	deleted := plugin.ClearCacheFor(path, true)
 	activity.Record(r.Context(), "API: Cache DELETE %v %+v", path, deleted)
 
 	jsonEncoder := json.NewEncoder(w)

plugin/cache.go

@@ -88,13 +88,30 @@ func allOpKeysIncludingChildrenRegex(path string) *regexp.Regexp {
 }
 
 // ClearCacheFor removes entries from the cache that match or are children of the provided path.
-// If successful, returns an array of deleted keys.
+// If successful, returns an array of deleted keys. Optionally clear the list operation for the
+// parent to remove any attributes related to the specified entry.
 //
 // TODO: If path == "/", we could optimize this by calling cache.Flush(). Not important
 // right now, but may be worth considering in the future.
-func ClearCacheFor(path string) []string {
+func ClearCacheFor(path string, clearParentList bool) []string {
 	rx := allOpKeysIncludingChildrenRegex(path)
-	return cache.Delete(rx)
+	deleted := cache.Delete(rx)
+
+	if clearParentList {
+		parentID, _ := splitID(path)
+		listOpName := defaultOpCodeToNameMap[ListOp]
+		deleted = append(deleted, cache.Delete(opKeyRegex(listOpName, parentID))...)
+	}
+
+	return deleted
+}
+
+// returns (parentID, cname)
+func splitID(entryID string) (string, string) {
+	segments := strings.Split(entryID, "/")
+	parentID := strings.Join(segments[:len(segments)-1], "/")
+	cname := segments[len(segments)-1]
+	return parentID, cname
 }
 
 type opFunc func() (interface{}, error)

plugin/cache_test.go

@@ -140,9 +140,20 @@ func (suite *CacheTestSuite) TestClearCache() {
 	path := "/a"
 	rx := allOpKeysIncludingChildrenRegex(path)
 
-	suite.cache.On("Delete", rx).Return([]string{"/a"})
-	deleted := ClearCacheFor(path)
-	suite.Equal([]string{"/a"}, deleted)
+	suite.cache.On("Delete", rx).Return([]string{path})
+	deleted := ClearCacheFor(path, false)
+	suite.Equal([]string{path}, deleted)
+}
+
+func (suite *CacheTestSuite) TestClearCache_WithParent() {
+	path := "/a/b"
+	rxEntry := allOpKeysIncludingChildrenRegex(path)
+	rxParent := opKeyRegex(defaultOpCodeToNameMap[ListOp], "/a")
+
+	suite.cache.On("Delete", rxEntry).Return([]string{"List:/a/b", "Read:/a/b"})
+	suite.cache.On("Delete", rxParent).Return([]string{"List:/a"})
+	deleted := ClearCacheFor(path, true)
+	suite.Equal([]string{"List:/a/b", "Read:/a/b", "List:/a"}, deleted)
 }
 
 type cacheTestsMockEntry struct {

plugin/methodWrappers.go

@@ -232,14 +232,7 @@ func Signal(ctx context.Context, s Signalable, signal string) error {
 
 	// The signal was successfully sent. Clear the entry's cache and its parent's
 	// cached list result to ensure that fresh data's loaded when needed
-	ClearCacheFor(s.id())
-	parentID, _ := splitID(s.id())
-	listOpName := defaultOpCodeToNameMap[ListOp]
-	entries, _ := cache.Get(listOpName, parentID)
-	if entries != nil {
-		cache.Delete(opKeyRegex(listOpName, parentID))
-	}
-
+	ClearCacheFor(s.id(), true)
 	return nil
 }
 
@@ -254,35 +247,21 @@ func Delete(ctx context.Context, d Deletable) (deleted bool, err error) {
 	// when needed. This includes:
 	//   * Clearing the entry and its children's cache.
 	//   * Updating the parent's cached list result.
-	ClearCacheFor(d.id())
-	parentID, cname := splitID(d.id())
-	listOpName := defaultOpCodeToNameMap[ListOp]
-	entries, _ := cache.Get(listOpName, parentID)
-	if entries == nil {
-		return
-	}
+	//
+	// If !deleted, the entry will eventually be deleted. However it's likely that
+	// Delete did update the entry (e.g. on VMs, Delete causes a state transition).
+	// Thus we also clear the parent's cached list result to ensure fresh data.
+	ClearCacheFor(d.id(), !deleted)
 	if deleted {
 		// The entry was deleted, so delete the entry from the parent's cached list
 		// result.
-		entries.(*EntryMap).Delete(cname)
-	} else {
-		// The entry will eventually be deleted. However it's likely that Delete
-		// did update the entry (e.g. on VMs, Delete causes a state transition).
-		// Thus, we clear the parent's cached list result to ensure fresh data.
-		//
-		// Note that this is symmetric with Signal. This makes sense because for
-		// some entries, deleting that entry is equivalent to sending a termination
-		// signal (e.g. EC2 instances).
-		cache.Delete(opKeyRegex(listOpName, parentID))
+		parentID, cname := splitID(d.id())
+		listOpName := defaultOpCodeToNameMap[ListOp]
+		entries, _ := cache.Get(listOpName, parentID)
+		if entries != nil {
+			entries.(*EntryMap).Delete(cname)
+		}
 	}
 
 	return
 }
-
-// returns (parentID, cname)
-func splitID(entryID string) (string, string) {
-	segments := strings.Split(entryID, "/")
-	parentID := strings.Join(segments[:len(segments)-1], "/")
-	cname := segments[len(segments)-1]
-	return parentID, cname
-}

plugin/methodWrappers_test.go

@@ -266,7 +266,6 @@ func (suite *MethodWrappersTestSuite) TestSignal_SendsSignalAndUpdatesCache() {
 	e.On("Signal", ctx, "start").Return(nil)
 
 	suite.cache.On("Delete", allOpKeysIncludingChildrenRegex(e.id())).Return([]string{})
-	suite.cache.On("Get", "List", "/foo").Return(newEntryMap(), nil)
 	suite.cache.On("Delete", opKeyRegex("List", "/foo")).Return([]string{})
 
 	// Also test case-insensitivity here
@@ -331,7 +330,6 @@ func (suite *MethodWrappersTestSuite) TestDelete_EntryDeletionInProgress_Updates
 	e.On("Delete", mock.Anything).Return(false, nil)
 
 	suite.cache.On("Delete", allOpKeysIncludingChildrenRegex(e.id())).Return([]string{})
-	suite.cache.On("Get", "List", "/foo").Return(newEntryMap(), nil)
 	suite.cache.On("Delete", opKeyRegex("List", "/foo")).Return([]string{})
 
 	deleted, err := Delete(context.Background(), e)
@@ -353,7 +351,7 @@ func (suite *MethodWrappersTestSuite) TestDelete_EntryDeletionInProgress_NoCache
 	deleted, err := Delete(context.Background(), e)
 	if suite.NoError(err) {
 		suite.False(deleted)
-		suite.cache.AssertNotCalled(suite.T(), "Delete", opKeyRegex("List", "/foo"))
+		suite.cache.AssertCalled(suite.T(), "Delete", opKeyRegex("List", "/foo"))
 	}
 }
 

volume/fs_test.go

@@ -198,7 +198,7 @@ func (suite *fsTestSuite) TestFSListExpiredCache() {
 	suite.Equal(1, entries.Len())
 	suite.Contains(entries.Map(), "path")
 
-	_ = plugin.ClearCacheFor("/instance/fs")
+	_ = plugin.ClearCacheFor("/instance/fs", false)
 	entries, err = plugin.List(suite.ctx, entry)
 	if suite.NoError(err) {
 		suite.Equal(1, entries.Len())