From 81c30e01f81fe5c9912b8e22b0df620d3ec8aa4d Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 18 Nov 2023 22:58:11 +0100 Subject: [PATCH] cleanup images from deleted comments Previously, images were deleted only from comments deleted before EditDuration expiration. After this change, any deletion of the comment deletes images if they are not used elsewhere in comments under the same page. --- .../_example/memory_store/accessor/image.go | 11 +++ .../memory_store/accessor/image_test.go | 29 +++++++- backend/_example/memory_store/server/image.go | 11 ++- .../memory_store/server/image_test.go | 5 ++ backend/_example/memory_store/server/rpc.go | 1 + backend/app/store/image/bolt_store.go | 13 ++++ backend/app/store/image/bolt_store_test.go | 30 ++++++++ backend/app/store/image/fs_store.go | 25 ++++++- backend/app/store/image/fs_store_test.go | 25 +++++++ backend/app/store/image/image.go | 6 ++ backend/app/store/image/image_mock.go | 48 +++++++++++- backend/app/store/image/remote_store.go | 6 ++ backend/app/store/image/remote_store_test.go | 12 +++ backend/app/store/service/service.go | 29 ++++++++ backend/app/store/service/service_test.go | 74 +++++++++++++++++++ 15 files changed, 318 insertions(+), 7 deletions(-) diff --git a/backend/_example/memory_store/accessor/image.go b/backend/_example/memory_store/accessor/image.go index a5757f5e..c3ce0b21 100644 --- a/backend/_example/memory_store/accessor/image.go +++ b/backend/_example/memory_store/accessor/image.go @@ -70,6 +70,17 @@ func (m *MemImage) Load(id string) ([]byte, error) { return img, nil } +// Delete image by ID +func (m *MemImage) Delete(id string) error { + m.mu.Lock() + // delete key from permanent and staging storage + delete(m.images, id) + delete(m.insertTime, id) + delete(m.imagesStaging, id) + m.mu.Unlock() + return nil +} + // Commit moves image from staging to permanent func (m *MemImage) Commit(id string) error { m.mu.RLock() diff --git a/backend/_example/memory_store/accessor/image_test.go b/backend/_example/memory_store/accessor/image_test.go index 42722754..f0df173b 100644 --- a/backend/_example/memory_store/accessor/image_test.go +++ b/backend/_example/memory_store/accessor/image_test.go @@ -18,7 +18,7 @@ import ( ) // gopher png for test, from https://golang.org/src/image/png/example_test.go -const gopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwUVRzHf2" + +const rawGopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwUVRzHf2" + "+OPbo9d7tsWyiyaZti6eWGAhISoIGKECEKCAiJJkYTiUgTMYSIosYYBBIUIxoSPIINEBDi2VhwkQrVsj1ESgu9doHWdrul7ba" + "73WNm3vOPtsseM9MdwvvrzTs+8/t95ze/33sI5BqiabU6m9En8oNjduLnAEDLUsQXFF8tQ5oxK3vmnNmDSMtrncks9Hhtt" + "/qeWZapHb1ha3UqYSWVl2ZmpWgaXMXGohQAvmeop3bjTRtv6SgaK/Pb9/bFzUrYslbFAmHPp+3WhAYdr+7GN/YnpN46Opv55VDs" + @@ -38,7 +38,9 @@ const gopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwU "1y98c3D27eppUjsZ6fql3jcd5rUe7+ZIlLNQny3Rd+E5Tct3WVhTM5RBCEdiEK0b6B+/ca2gYU393nFj/n1AygRQxPIUA043M42u85+z2S" + "nssKrPl8Mx76NL3E6eXc3be7OD+H4WHbJkKI8AU8irbITQjZ+0hQcPEgId/Fn/pl9crKH02+5o2b9T/eMx7pKoskYgAAAABJRU5ErkJggg==" -func gopherPNG() io.Reader { return base64.NewDecoder(base64.StdEncoding, strings.NewReader(gopher)) } +func gopherPNG() io.Reader { + return base64.NewDecoder(base64.StdEncoding, strings.NewReader(rawGopher)) +} func TestMemImage_LoadAfterSave(t *testing.T) { svc := NewMemImageStore() @@ -57,7 +59,8 @@ func TestMemImage_LoadAfterSave(t *testing.T) { assert.NoError(t, err) assert.Equal(t, gopher, img) - svc.ResetCleanupTimer(id) + err = svc.ResetCleanupTimer(id) + assert.NoError(t, err) err = svc.Commit(id) assert.NoError(t, err) @@ -70,6 +73,26 @@ func TestMemImage_LoadAfterSave(t *testing.T) { assert.Equal(t, gopher, img) } +func TestMemImage_LoadAfterDelete(t *testing.T) { + svc := NewMemImageStore() + gopher, err := io.ReadAll(gopherPNG()) + assert.NoError(t, err) + + id := "test_img" + err = svc.Save(id, gopher) + assert.NoError(t, err) + + err = svc.Delete(id) + assert.NoError(t, err) + + img, err := svc.Load(id) + assert.EqualError(t, err, "image test_img not found") + assert.Empty(t, img) + + err = svc.ResetCleanupTimer(id) + assert.EqualError(t, err, "image test_img not found") +} + func TestMemImage_CommitFail(t *testing.T) { svc := NewMemImageStore() err := svc.Commit("test_id") diff --git a/backend/_example/memory_store/server/image.go b/backend/_example/memory_store/server/image.go index f5e0f420..793deada 100644 --- a/backend/_example/memory_store/server/image.go +++ b/backend/_example/memory_store/server/image.go @@ -35,7 +35,6 @@ func (s *RPC) imgResetClnTimerHndl(id uint64, params json.RawMessage) (rr jrpc.R } err := s.img.ResetCleanupTimer(fileID) return jrpc.EncodeResponse(id, nil, err) - } func (s *RPC) imgLoadHndl(id uint64, params json.RawMessage) (rr jrpc.Response) { @@ -47,6 +46,16 @@ func (s *RPC) imgLoadHndl(id uint64, params json.RawMessage) (rr jrpc.Response) return jrpc.EncodeResponse(id, value, err) } +func (s *RPC) imgDeleteHndl(id uint64, params json.RawMessage) (rr jrpc.Response) { + var fileID string + if err := json.Unmarshal(params, &fileID); err != nil { + return jrpc.Response{Error: err.Error()} + } + err := s.img.Delete(fileID) + return jrpc.EncodeResponse(id, nil, err) + +} + func (s *RPC) imgCommitHndl(id uint64, params json.RawMessage) (rr jrpc.Response) { var fileID string if err := json.Unmarshal(params, &fileID); err != nil { diff --git a/backend/_example/memory_store/server/image_test.go b/backend/_example/memory_store/server/image_test.go index 0f70fd0b..79f133ea 100644 --- a/backend/_example/memory_store/server/image_test.go +++ b/backend/_example/memory_store/server/image_test.go @@ -158,4 +158,9 @@ func TestRPC_imgInfoHndl(t *testing.T) { info, err = ri.Info() assert.NoError(t, err) assert.False(t, info.FirstStagingImageTS.IsZero()) + + err = ri.Delete("test_img") + assert.NoError(t, err) + _, err = ri.Load("test_img") + assert.EqualError(t, err, "image test_img not found") } diff --git a/backend/_example/memory_store/server/rpc.go b/backend/_example/memory_store/server/rpc.go index 411c446b..97e6eca4 100644 --- a/backend/_example/memory_store/server/rpc.go +++ b/backend/_example/memory_store/server/rpc.go @@ -60,6 +60,7 @@ func (s *RPC) addHandlers() { "save_with_id": s.imgSaveWithIDHndl, "reset_cleanup_timer": s.imgResetClnTimerHndl, "load": s.imgLoadHndl, + "delete": s.imgDeleteHndl, "commit": s.imgCommitHndl, "cleanup": s.imgCleanupHndl, "info": s.imgInfoHndl, diff --git a/backend/app/store/image/bolt_store.go b/backend/app/store/image/bolt_store.go index 9d9fe9b2..7b0014de 100644 --- a/backend/app/store/image/bolt_store.go +++ b/backend/app/store/image/bolt_store.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/binary" + "errors" "fmt" "time" @@ -118,6 +119,18 @@ func (b *Bolt) Load(id string) ([]byte, error) { return data, nil } +// Delete image from storage +func (b *Bolt) Delete(id string) error { + return b.db.Update(func(tx *bolt.Tx) error { + // deleting a non-existing key doesn't return an error, so joining errors from deleting an image + // from both buckets is safe and will return nil if there are no errors on the real delete + // or image is absent in both buckets + err := tx.Bucket([]byte(imagesBktName)).Delete([]byte(id)) + err = errors.Join(err, tx.Bucket([]byte(imagesStagedBktName)).Delete([]byte(id))) + return err + }) +} + // Cleanup runs scan of staging and removes old data based on ttl func (b *Bolt) Cleanup(_ context.Context, ttl time.Duration) error { return b.db.Update(func(tx *bolt.Tx) error { diff --git a/backend/app/store/image/bolt_store_test.go b/backend/app/store/image/bolt_store_test.go index d9977b19..232db45d 100644 --- a/backend/app/store/image/bolt_store_test.go +++ b/backend/app/store/image/bolt_store_test.go @@ -58,6 +58,36 @@ func TestBoltStore_LoadAfterSave(t *testing.T) { assert.Error(t, err) } +func TestBoltStore_LoadAfterDelete(t *testing.T) { + svc, teardown := prepareBoltImageStorageTest(t) + defer teardown() + + // delete image from permanent storage + id := "test_img" + err := svc.Save(id, gopherPNGBytes()) + assert.NoError(t, err) + + err = svc.Commit(id) + require.NoError(t, err) + + err = svc.Delete(id) + assert.NoError(t, err) + + _, err = svc.Load(id) + assert.Error(t, err) + + // delete staging image + id = "staging_img" + err = svc.Save(id, gopherPNGBytes()) + assert.NoError(t, err) + + err = svc.Delete(id) + assert.NoError(t, err) + + _, err = svc.Load(id) + assert.Error(t, err) +} + func TestBoltStore_Cleanup(t *testing.T) { svc, teardown := prepareBoltImageStorageTest(t) defer teardown() diff --git a/backend/app/store/image/fs_store.go b/backend/app/store/image/fs_store.go index 09ca5e78..cf9a3907 100644 --- a/backend/app/store/image/fs_store.go +++ b/backend/app/store/image/fs_store.go @@ -23,7 +23,8 @@ type FileSystem struct { Staging string Partitions int - crc struct { + moveLock sync.Mutex // needed only for deleting images or moving them from staging to permanent storage + crc struct { *crc64.Table sync.Once mask string @@ -50,6 +51,8 @@ func (f *FileSystem) Save(id string, img []byte) error { // Commit file stored in staging location by moving it to permanent location func (f *FileSystem) Commit(id string) error { + f.moveLock.Lock() + defer f.moveLock.Unlock() log.Printf("[DEBUG] Commit image %s", id) stagingImage, permImage := f.location(f.Staging, id), f.location(f.Location, id) @@ -107,11 +110,31 @@ func (f *FileSystem) Load(id string) ([]byte, error) { return io.ReadAll(fh) } +// Delete image from storage +func (f *FileSystem) Delete(id string) error { + f.moveLock.Lock() + defer f.moveLock.Unlock() + staging := f.location(f.Staging, id) + // file doesn't exist on staging, delete from permanent location + if _, err := os.Stat(staging); os.IsNotExist(err) { + file := f.location(f.Location, id) + e := os.Remove(file) + _ = os.Remove(path.Dir(file)) // try to remove directory + return e + } + // delete file from staging + err := os.Remove(staging) + _ = os.Remove(path.Dir(staging)) // try to remove directory + return err +} + // Cleanup runs scan of staging and removes old files based on ttl func (f *FileSystem) Cleanup(_ context.Context, ttl time.Duration) error { if _, err := os.Stat(f.Staging); os.IsNotExist(err) { return nil } + f.moveLock.Lock() + defer f.moveLock.Unlock() // we can ignore context as on local FS remove is relatively fast operation err := filepath.Walk(f.Staging, func(fpath string, info os.FileInfo, err error) error { diff --git a/backend/app/store/image/fs_store_test.go b/backend/app/store/image/fs_store_test.go index dbc88953..1c5e564a 100644 --- a/backend/app/store/image/fs_store_test.go +++ b/backend/app/store/image/fs_store_test.go @@ -131,6 +131,31 @@ func TestFsStore_LoadAfterCommit(t *testing.T) { assert.Error(t, err) } +func TestFsStore_LoadAfterDelete(t *testing.T) { + svc, teardown := prepareImageTest(t) + defer teardown() + + id := "test_img" + err := svc.Save(id, gopherPNGBytes()) + assert.NoError(t, err) + err = svc.Commit(id) + require.NoError(t, err) + err = svc.Delete(id) + require.NoError(t, err) + + _, err = svc.Load(id) + assert.Error(t, err) + + // create file on staging + err = svc.Save(id, gopherPNGBytes()) + assert.NoError(t, err) + err = svc.Delete(id) + require.NoError(t, err) + + _, err = svc.Load(id) + assert.Error(t, err) +} + func TestFsStore_location(t *testing.T) { tbl := []struct { partitions int diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go index 29420629..647594c5 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -70,6 +70,7 @@ type Store interface { Info() (StoreInfo, error) // get meta information about storage Save(id string, img []byte) error // store image with passed id to staging Load(id string) ([]byte, error) // load image by ID + Delete(id string) error // delete image by ID ResetCleanupTimer(id string) error // resets cleanup timer for the image, called on comment preview Commit(id string) error // move image from staging to permanent @@ -212,6 +213,11 @@ func (s *Service) Load(id string) ([]byte, error) { return s.store.Load(id) } +// Delete wraps storage Delete function. +func (s *Service) Delete(id string) error { + return s.store.Delete(id) +} + // Save wraps storage Save function, validating and resizing the image before calling it. func (s *Service) Save(userID string, r io.Reader) (id string, err error) { id = path.Join(userID, guid()) diff --git a/backend/app/store/image/image_mock.go b/backend/app/store/image/image_mock.go index 18562dff..106eeb77 100644 --- a/backend/app/store/image/image_mock.go +++ b/backend/app/store/image/image_mock.go @@ -4,9 +4,9 @@ package image import ( - context "context" + "context" "sync" - time "time" + "time" ) // Ensure, that StoreMock does implement Store. @@ -25,6 +25,9 @@ var _ Store = &StoreMock{} // CommitFunc: func(id string) error { // panic("mock out the Commit method") // }, +// DeleteFunc: func(id string) error { +// panic("mock out the Delete method") +// }, // InfoFunc: func() (StoreInfo, error) { // panic("mock out the Info method") // }, @@ -50,6 +53,9 @@ type StoreMock struct { // CommitFunc mocks the Commit method. CommitFunc func(id string) error + // DeleteFunc mocks the Delete method. + DeleteFunc func(id string) error + // InfoFunc mocks the Info method. InfoFunc func() (StoreInfo, error) @@ -76,6 +82,11 @@ type StoreMock struct { // ID is the id argument value. ID string } + // Delete holds details about calls to the Delete method. + Delete []struct { + // ID is the id argument value. + ID string + } // Info holds details about calls to the Info method. Info []struct { } @@ -99,6 +110,7 @@ type StoreMock struct { } lockCleanup sync.RWMutex lockCommit sync.RWMutex + lockDelete sync.RWMutex lockInfo sync.RWMutex lockLoad sync.RWMutex lockResetCleanupTimer sync.RWMutex @@ -173,6 +185,38 @@ func (mock *StoreMock) CommitCalls() []struct { return calls } +// Delete calls DeleteFunc. +func (mock *StoreMock) Delete(id string) error { + if mock.DeleteFunc == nil { + panic("StoreMock.DeleteFunc: method is nil but Store.Delete was just called") + } + callInfo := struct { + ID string + }{ + ID: id, + } + mock.lockDelete.Lock() + mock.calls.Delete = append(mock.calls.Delete, callInfo) + mock.lockDelete.Unlock() + return mock.DeleteFunc(id) +} + +// DeleteCalls gets all the calls that were made to Delete. +// Check the length with: +// +// len(mockedStore.DeleteCalls()) +func (mock *StoreMock) DeleteCalls() []struct { + ID string +} { + var calls []struct { + ID string + } + mock.lockDelete.RLock() + calls = mock.calls.Delete + mock.lockDelete.RUnlock() + return calls +} + // Info calls InfoFunc. func (mock *StoreMock) Info() (StoreInfo, error) { if mock.InfoFunc == nil { diff --git a/backend/app/store/image/remote_store.go b/backend/app/store/image/remote_store.go index e7cec50e..bdb096ca 100644 --- a/backend/app/store/image/remote_store.go +++ b/backend/app/store/image/remote_store.go @@ -41,6 +41,12 @@ func (r *RPC) Load(id string) ([]byte, error) { return io.ReadAll(base64.NewDecoder(base64.StdEncoding, strings.NewReader(rawImg))) } +// Delete image from storage +func (r *RPC) Delete(id string) error { + _, err := r.Call("image.delete", id) + return err +} + // Commit file stored in staging location by moving it to permanent location func (r *RPC) Commit(id string) error { _, err := r.Call("image.commit", id) diff --git a/backend/app/store/image/remote_store_test.go b/backend/app/store/image/remote_store_test.go index 8037b884..cd5d3de0 100644 --- a/backend/app/store/image/remote_store_test.go +++ b/backend/app/store/image/remote_store_test.go @@ -41,6 +41,18 @@ func TestRemote_Load(t *testing.T) { assert.Equal(t, gopherPNGBytes(), res) } +func TestRemote_Delete(t *testing.T) { + ts := testServer(t, `{"method":"image.delete","params":"54321","id":1}`, `{}`) + defer ts.Close() + c := RPC{Client: jrpc.Client{API: ts.URL, Client: http.Client{}}} + + var a Store = &c + _ = a + + err := c.Delete("54321") + assert.NoError(t, err) +} + func TestRemote_Commit(t *testing.T) { ts := testServer(t, `{"method":"image.commit","params":"gopher_id","id":1}`, `{"id":1}`) defer ts.Close() diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index b6a8a045..48e722c9 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -5,6 +5,7 @@ package service import ( "fmt" "math" + "slices" "sort" "strings" "sync" @@ -796,6 +797,34 @@ func (s *DataStore) Delete(locator store.Locator, commentID string, mode store.D s.repliesCache.Delete(commentID) s.repliesCache.Delete(comment.ParentID) } + + // delete images from the comment if they are not reused elsewhere in comments to the same page + idsFn := func() []string { // get IDs of all images from the same URL to verify if image from deleted comment was reused + comments, e := s.Engine.Find(engine.FindRequest{Locator: locator}) + if e != nil { + log.Printf("[WARN] can't get comments %s text for deleted comment image check, %v", comment.ID, err) + return nil + } + var imgIDs = []string{} + for _, cc := range comments { + // exclude the comment we are deleting + if cc.ID != commentID { + imgIDs = append(imgIDs, s.ImageService.ExtractPictures(cc.Text)...) + } + } + return imgIDs + } + commentImgIDs := s.ImageService.ExtractPictures(comment.Text) + pageImgIDs := idsFn() + for _, id := range commentImgIDs { + if !slices.Contains(pageImgIDs, id) { + if err := s.ImageService.Delete(id); err != nil { + log.Printf("[WARN] failed to delete image %s on comment %s deletion, %v", id, commentID, err) + } + } + } + log.Printf("[ERROR] commentImgIDs: %v, pageImgIDs: %v", commentImgIDs, pageImgIDs) + 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 6fe75b39..258953b8 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -1406,6 +1406,80 @@ func TestService_Delete(t *testing.T) { assert.NoError(t, err) } +func TestService_deleteImagesOnCommentDelete(t *testing.T) { + lgr.Setup(lgr.Debug, lgr.CallerFile, lgr.CallerFunc) + + mockStore := image.StoreMock{ + DeleteFunc: func(id string) error { return nil }, + CommitFunc: func(id string) error { return nil }, + ResetCleanupTimerFunc: func(id string) error { return nil }, + } + imgSvc := image.NewService(&mockStore, + image.ServiceParams{ + EditDuration: 50 * time.Millisecond, + ImageAPI: "/images/dev/", + ProxyAPI: "/non_existent", + }) + defer imgSvc.Close(context.TODO()) + + // two comments for https://radio-t.com + eng, teardown := prepStoreEngine(t) + defer teardown() + b := DataStore{Engine: eng, EditDuration: 50 * time.Millisecond, + AdminStore: admin.NewStaticKeyStore("secret 123"), ImageService: imgSvc} + + c := store.Comment{ + ID: "id-22", + Text: `some text xx `, + Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), + Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, + User: store.User{ID: "user1", Name: "user name"}, + } + _, err := b.Engine.Create(c) // create directly with engine, doesn't call submitImages + assert.NoError(t, err) + b.submitImages(c) + // reply to the first comment with one new image and one existing one + c = store.Comment{ + ID: "id-23", + ParentID: "id-22", + Text: `some text xx `, + Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, + User: store.User{ID: "user1", Name: "user name"}, + } + _, err = b.Engine.Create(c) // create directly with engine, doesn't call submitImages + assert.NoError(t, err) + b.submitImages(c) + + // verify that images are in staging store + assert.Equal(t, 4, len(mockStore.ResetCleanupTimerCalls())) + assert.Equal(t, "dev/pic1.png", mockStore.ResetCleanupTimerCalls()[0].ID) + assert.Equal(t, "dev/pic2.png", mockStore.ResetCleanupTimerCalls()[1].ID) + assert.Equal(t, "dev/pic2.png", mockStore.ResetCleanupTimerCalls()[2].ID) + assert.Equal(t, "dev/pic3.png", mockStore.ResetCleanupTimerCalls()[3].ID) + time.Sleep(b.EditDuration + 100*time.Millisecond) + // verify that they got into the main store + assert.Equal(t, 4, len(mockStore.CommitCalls())) + assert.Equal(t, "dev/pic1.png", mockStore.CommitCalls()[0].ID) + assert.Equal(t, "dev/pic2.png", mockStore.CommitCalls()[1].ID) + assert.Equal(t, "dev/pic2.png", mockStore.CommitCalls()[2].ID) + assert.Equal(t, "dev/pic3.png", mockStore.CommitCalls()[3].ID) + + // delete the first comment + err = b.Delete(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "id-22", store.SoftDelete) + assert.NoError(t, err) + // verify that images are deleted from the main store + assert.Equal(t, 1, len(mockStore.DeleteCalls())) + assert.Equal(t, "dev/pic1.png", mockStore.DeleteCalls()[0].ID) + + // delete the second comment + err = b.Delete(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "id-23", store.SoftDelete) + assert.NoError(t, err) + // verify that images are deleted from the main store + assert.Equal(t, 3, len(mockStore.DeleteCalls())) + assert.Equal(t, "dev/pic2.png", mockStore.DeleteCalls()[1].ID) + assert.Equal(t, "dev/pic3.png", mockStore.DeleteCalls()[2].ID) +} + // DeleteUser removes all comments from user func TestService_DeleteUser(t *testing.T) { // two comments for https://radio-t.com, no reply