From ed93ca601e9cc6809ee9df0d04f5ff094c941139 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Mon, 3 Oct 2022 01:44:24 +0200 Subject: [PATCH] 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 --- backend/app/rest/api/rest_private_test.go | 105 +++++++++++++++++++++- backend/app/store/service/service.go | 17 ++++ backend/app/store/service/service_test.go | 43 ++++++++- 3 files changed, 160 insertions(+), 5 deletions(-) diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 203f455122..d393833832 100644 --- 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 index b601cfa9b2..8427d4cc99 100644 --- 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 index 33e9ffe328..80f57d2cab 100644 --- 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) {