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 · f4f8e39 · f4f8e39
1 parent 3eccf01
commit f4f8e39
Show file tree

Hide file tree

Showing 3 changed files with 134 additions and 4 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
@@ -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),
@@ -996,6 +995,19 @@ func TestService_HasReplies(t *testing.T) {
 	_, err := b.Create(reply)
 	assert.NoError(t, err)
 	assert.True(t, b.HasReplies(comment))
+
+	// should not report replies after deletion of the child
+	b.Delete(reply.Locator, reply.ID, store.HardDelete)
+	assert.False(t, b.HasReplies(comment))
+
+	// recreate reply
+	_, err := b.Create(reply)
+	assert.NoError(t, err)
+	assert.True(t, b.HasReplies(comment))
+
+	// should not report replies after deletion of the child using Edit mechanism
+	b.EditComment(reply.Locator, reply.ID, store.HardDelete)
+	assert.False(t, b.HasReplies(comment))
 }
 
 func TestService_UserReplies(t *testing.T) {