clean up comment HasReplies cache on child comment deletion

Previously, the cache kept the entry and deletion of the parent comment after child deletion was not possible for the rest of cache life (5m) duration. Now it's possible to delete a parent comment after the deletion of the child comment by a user or admin. Resolves #1481
umputun · Oct 3, 2022 · dd1ba9b · dd1ba9b
1 parent cebe929
commit dd1ba9b
Show file tree

Hide file tree

Showing 3 changed files with 160 additions and 5 deletions.
diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go
@@ -373,6 +373,109 @@ func TestRest_UpdateDelete(t *testing.T) {
 		{URL: "https://radio-t.com/blah2", Count: 0}}, j)
 }
 
+func TestRest_DeleteChildThenParent(t *testing.T) {
+	ts, _, teardown := startupT(t)
+	defer teardown()
+
+	p := store.Comment{Text: "test test #1",
+		Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}}
+	idP := addComment(t, p, ts)
+
+	c1 := store.Comment{Text: "test test #1", ParentID: idP,
+		Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}}
+	idC1 := addComment(t, c1, ts)
+
+	c2 := store.Comment{Text: "test test #1", ParentID: idP,
+		Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah1"}}
+	idC2 := addComment(t, c2, ts)
+
+	// check multi count equals two
+	resp, err := post(t, ts.URL+"/api/v1/counts?site=remark42", `["https://radio-t.com/blah1"]`)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	bb, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	assert.NoError(t, resp.Body.Close())
+	j := []store.PostInfo{}
+	err = json.Unmarshal(bb, &j)
+	assert.NoError(t, err)
+	assert.Equal(t, []store.PostInfo{{URL: "https://radio-t.com/blah1", Count: 3}}, j)
+
+	// update a parent comment fails after child is created
+	client := http.Client{}
+	defer client.CloseIdleConnections()
+	req, err := http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1",
+		strings.NewReader(`{"text": "updated text", "summary":"updated by user"}`))
+	require.NoError(t, err)
+	req.Header.Add("X-JWT", devToken)
+	b, err := client.Do(req)
+	require.NoError(t, err)
+	body, err := io.ReadAll(b.Body)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusBadRequest, b.StatusCode, string(body))
+	assert.NoError(t, b.Body.Close())
+
+	// delete first child comment
+	req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idC1+"?site=remark42&url=https://radio-t.com/blah1",
+		strings.NewReader(`{"delete": true, "summary":"removed by user"}`))
+	require.NoError(t, err)
+	req.Header.Add("X-JWT", devToken)
+	b, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(b.Body)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, b.StatusCode, string(body))
+	assert.NoError(t, b.Body.Close())
+
+	// delete a parent comment, fails as one comment child still present
+	defer client.CloseIdleConnections()
+	req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1",
+		strings.NewReader(`{"delete": true, "summary":"removed by user"}`))
+	require.NoError(t, err)
+	req.Header.Add("X-JWT", devToken)
+	b, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(b.Body)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusBadRequest, b.StatusCode, string(body))
+	assert.NoError(t, b.Body.Close())
+
+	// delete second child comment, as an admin to check both deletion methods
+	req, err = http.NewRequest(http.MethodDelete,
+		fmt.Sprintf("%s/api/v1/admin/comment/%s?site=remark42&url=https://radio-t.com/blah1", ts.URL, idC2), http.NoBody)
+	require.NoError(t, err)
+	requireAdminOnly(t, req)
+	resp, err = sendReq(t, req, adminUmputunToken)
+	assert.NoError(t, err)
+	assert.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+	// delete a parent comment, shouldn't fail after children are deleted
+	defer client.CloseIdleConnections()
+	req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+idP+"?site=remark42&url=https://radio-t.com/blah1",
+		strings.NewReader(`{"delete": true, "summary":"removed by user"}`))
+	require.NoError(t, err)
+	req.Header.Add("X-JWT", devToken)
+	b, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(b.Body)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, b.StatusCode, string(body))
+	assert.NoError(t, b.Body.Close())
+
+	// check multi count decremented to zero
+	resp, err = post(t, ts.URL+"/api/v1/counts?site=remark42", `["https://radio-t.com/blah1"]`)
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	bb, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	assert.NoError(t, resp.Body.Close())
+	j = []store.PostInfo{}
+	err = json.Unmarshal(bb, &j)
+	assert.NoError(t, err)
+	assert.Equal(t, []store.PostInfo{{URL: "https://radio-t.com/blah1", Count: 0}}, j)
+}
+
 func TestRest_UpdateNotOwner(t *testing.T) {
 	ts, srv, teardown := startupT(t)
 	defer teardown()
@@ -396,8 +499,6 @@ func TestRest_UpdateNotOwner(t *testing.T) {
 	assert.Equal(t, http.StatusForbidden, b.StatusCode, string(body), "update from non-owner")
 	assert.Equal(t, `{"code":3,"details":"can not edit comments for other users","error":"rejected"}`+"\n", string(body))
 
-	client = http.Client{}
-	defer client.CloseIdleConnections()
 	req, err = http.NewRequest(http.MethodPut, ts.URL+"/api/v1/comment/"+id1+
 		"?site=remark42&url=https://radio-t.com/blah1", strings.NewReader(`ERRR "text":"updated text", "summary":"my"}`))
 	assert.NoError(t, err)

diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go
@@ -496,6 +496,12 @@ func (s *DataStore) EditComment(locator store.Locator, commentID string, req Edi
 		if e := s.AdminStore.OnEvent(comment.Locator.SiteID, admin.EvDelete); e != nil {
 			log.Printf("[WARN] failed to send delete event, %s", e)
 		}
+		// clean up the comment and it's parent from cache, so that
+		// after cleaning up the child, parent won't be stuck non-deletable till cache expires
+		if s.repliesCache.LoadingCache != nil {
+			s.repliesCache.Delete(commentID)
+			s.repliesCache.Delete(comment.ParentID)
+		}
 		comment.Deleted = true
 		delReq := engine.DeleteRequest{Locator: locator, CommentID: commentID, DeleteMode: store.SoftDelete}
 		return comment, s.Engine.Delete(delReq)
@@ -739,6 +745,17 @@ func (s *DataStore) Delete(locator store.Locator, commentID string, mode store.D
 	if e := s.AdminStore.OnEvent(locator.SiteID, admin.EvDelete); e != nil {
 		log.Printf("[WARN] failed to send delete event, %s", e)
 	}
+	// get comment to learn it's parent ID
+	comment, err := s.Engine.Get(engine.GetRequest{Locator: locator, CommentID: commentID})
+	if err != nil {
+		return err
+	}
+	// clean up the comment and it's parent from cache, so that
+	// after cleaning up the child, parent won't be stuck non-deletable till cache expires
+	if s.repliesCache.LoadingCache != nil {
+		s.repliesCache.Delete(commentID)
+		s.repliesCache.Delete(comment.ParentID)
+	}
 	req := engine.DeleteRequest{Locator: locator, CommentID: commentID, DeleteMode: mode}
 	return s.Engine.Delete(req)
 }

diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go
@@ -971,7 +971,7 @@ func TestService_HasReplies(t *testing.T) {
 	// two comments for https://radio-t.com, no reply
 	eng, teardown := prepStoreEngine(t)
 	defer teardown()
-	b := DataStore{Engine: eng, EditDuration: 100 * time.Millisecond,
+	b := DataStore{Engine: eng,
 		AdminStore: admin.NewStaticStore("secret 123", []string{"radio-t"}, []string{"user2"}, "user@email.com")}
 	defer b.Close()
 
@@ -982,11 +982,10 @@ func TestService_HasReplies(t *testing.T) {
 		Locator:   store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"},
 		User:      store.User{ID: "user1", Name: "user name"},
 	}
-
 	assert.False(t, b.HasReplies(comment))
 
 	reply := store.Comment{
-		ID:        "123456",
+		ID:        "c-1",
 		ParentID:  "id-1",
 		Text:      "some text",
 		Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local),
@@ -995,7 +994,45 @@ func TestService_HasReplies(t *testing.T) {
 	}
 	_, err := b.Create(reply)
 	assert.NoError(t, err)
+	_, found := b.repliesCache.Peek(comment.ID)
+	assert.False(t, found, "not yet checked")
 	assert.True(t, b.HasReplies(comment))
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.True(t, found, "checked and has replies")
+
+	// deletion of the parent comment shouldn't work as the comment has replies
+	_, err = b.EditComment(reply.Locator, comment.ID, EditRequest{Orig: comment.ID, Delete: true, Summary: "user deletes the comment"})
+	assert.EqualError(t, err, "parent comment with reply can't be edited, "+comment.ID)
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.True(t, found, "checked and has replies")
+
+	// should not report replies after deletion of the child
+	err = b.Delete(reply.Locator, reply.ID, store.HardDelete)
+	assert.NoError(t, err)
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.False(t, found, "cleaned up from cache by Delete call")
+	assert.False(t, b.HasReplies(comment))
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.False(t, found, "checked and has no replies")
+
+	// recreate reply with the new ID
+	reply.ID = "c-2"
+	_, err = b.Create(reply)
+	assert.NoError(t, err)
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.False(t, found, "not yet checked")
+	assert.True(t, b.HasReplies(comment))
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.True(t, found, "checked and has replies again")
+
+	// should not report replies after deletion of the child using Edit mechanism
+	_, err = b.EditComment(reply.Locator, reply.ID, EditRequest{Orig: reply.ID, Delete: true, Summary: "user deletes the comment"})
+	assert.NoError(t, err)
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.False(t, found, "cleaned up from cache by EditComment call")
+	assert.False(t, b.HasReplies(comment))
+	_, found = b.repliesCache.Peek(reply.ParentID)
+	assert.False(t, found, "checked and has no replies")
 }
 
 func TestService_UserReplies(t *testing.T) {