From 4cf76a2bfcc2c19bdd34f1bd58d8545d3499481b Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 21 Sep 2022 19:55:52 +0200 Subject: [PATCH] [chore] Tidy up status deletion, remove from cache too (#845) * add func for deleting status from db + cache * move deletes entirely back to processor and also only do a delete if the requesting account owns the item being deleted * tidy up unboost processing * delete status more efficiently * fix wrong account id on remote test attachments * fix federator test --- internal/cache/status.go | 5 ++ internal/db/bundb/status.go | 36 +++++++++++++ internal/db/bundb/status_test.go | 10 ++++ internal/db/status.go | 3 ++ internal/federation/federatingdb/delete.go | 20 ++----- internal/processing/account/delete.go | 61 +++++++--------------- internal/processing/fromclientapi.go | 4 ++ internal/processing/fromcommon.go | 27 +++++----- internal/processing/fromfederator_test.go | 9 ++-- internal/processing/status/delete.go | 6 +-- internal/processing/status/unboost.go | 6 --- testrig/testmodels.go | 20 +++---- 12 files changed, 113 insertions(+), 94 deletions(-) diff --git a/internal/cache/status.go b/internal/cache/status.go index f3cbce779..898b50846 100644 --- a/internal/cache/status.go +++ b/internal/cache/status.go @@ -85,6 +85,11 @@ func (c *StatusCache) Put(status *gtsmodel.Status) { c.cache.Set(status.ID, copyStatus(status)) } +// Invalidate invalidates one status from the cache using the ID of the status as key. +func (c *StatusCache) Invalidate(statusID string) { + c.cache.Invalidate(statusID) +} + // copyStatus performs a surface-level copy of status, only keeping attached IDs intact, not the objects. // due to all the data being copied being 99% primitive types or strings (which are immutable and passed by ptr) // this should be a relatively cheap process diff --git a/internal/db/bundb/status.go b/internal/db/bundb/status.go index e247e8940..59417afe4 100644 --- a/internal/db/bundb/status.go +++ b/internal/db/bundb/status.go @@ -227,6 +227,42 @@ func (s *statusDB) UpdateStatus(ctx context.Context, status *gtsmodel.Status) (* return status, err } +func (s *statusDB) DeleteStatusByID(ctx context.Context, id string) db.Error { + err := s.conn.RunInTx(ctx, func(tx bun.Tx) error { + // delete links between this status and any emojis it uses + if _, err := tx. + NewDelete(). + Model(>smodel.StatusToEmoji{}). + Where("status_id = ?", bun.Ident(id)). + Exec(ctx); err != nil { + return err + } + + // delete links between this status and any tags it uses + if _, err := tx. + NewDelete(). + Model(>smodel.StatusToTag{}). + Where("status_id = ?", bun.Ident(id)). + Exec(ctx); err != nil { + return err + } + + // delete the status itself + if _, err := tx. + NewDelete(). + Model(>smodel.Status{ID: id}). + WherePK(). + Exec(ctx); err != nil { + return err + } + + s.cache.Invalidate(id) + return nil + }) + + return s.conn.ProcessError(err) +} + func (s *statusDB) GetStatusParents(ctx context.Context, status *gtsmodel.Status, onlyDirect bool) ([]*gtsmodel.Status, db.Error) { parents := []*gtsmodel.Status{} s.statusParent(ctx, status, &parents, onlyDirect) diff --git a/internal/db/bundb/status_test.go b/internal/db/bundb/status_test.go index 36e329806..a796ebdad 100644 --- a/internal/db/bundb/status_test.go +++ b/internal/db/bundb/status_test.go @@ -25,6 +25,7 @@ import ( "time" "github.com/stretchr/testify/suite" + "github.com/superseriousbusiness/gotosocial/internal/db" ) type StatusTestSuite struct { @@ -132,6 +133,15 @@ func (suite *StatusTestSuite) TestGetStatusChildren() { } } +func (suite *StatusTestSuite) TestDeleteStatus() { + targetStatus := suite.testStatuses["admin_account_status_1"] + err := suite.db.DeleteStatusByID(context.Background(), targetStatus.ID) + suite.NoError(err) + + _, err = suite.db.GetStatusByID(context.Background(), targetStatus.ID) + suite.ErrorIs(err, db.ErrNoEntries) +} + func TestStatusTestSuite(t *testing.T) { suite.Run(t, new(StatusTestSuite)) } diff --git a/internal/db/status.go b/internal/db/status.go index 307d9ea74..55cec5beb 100644 --- a/internal/db/status.go +++ b/internal/db/status.go @@ -41,6 +41,9 @@ type Status interface { // UpdateStatus updates one status in the database and returns it to the caller. UpdateStatus(ctx context.Context, status *gtsmodel.Status) (*gtsmodel.Status, Error) + // DeleteStatusByID deletes one status from the database. + DeleteStatusByID(ctx context.Context, id string) Error + // CountStatusReplies returns the amount of replies recorded for a status, or an error if something goes wrong CountStatusReplies(ctx context.Context, status *gtsmodel.Status) (int, Error) diff --git a/internal/federation/federatingdb/delete.go b/internal/federation/federatingdb/delete.go index 8c3457fae..08f16fd5a 100644 --- a/internal/federation/federatingdb/delete.go +++ b/internal/federation/federatingdb/delete.go @@ -20,12 +20,10 @@ package federatingdb import ( "context" - "fmt" "net/url" "codeberg.org/gruf/go-kv" "github.com/superseriousbusiness/gotosocial/internal/ap" - "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/messages" ) @@ -38,12 +36,11 @@ import ( // The library makes this call only after acquiring a lock first. func (f *federatingDB) Delete(ctx context.Context, id *url.URL) error { l := log.WithFields(kv.Fields{ - {"id", id}, }...) l.Debug("entering Delete") - receivingAccount, _ := extractFromCtx(ctx) + receivingAccount, requestingAccount := extractFromCtx(ctx) if receivingAccount == nil { // If the receiving account wasn't set on the context, that means this request didn't pass // through the API, but came from inside GtS as the result of another activity on this instance. That being so, @@ -53,13 +50,8 @@ func (f *federatingDB) Delete(ctx context.Context, id *url.URL) error { // in a delete we only get the URI, we can't know if we have a status or a profile or something else, // so we have to try a few different things... - s, err := f.db.GetStatusByURI(ctx, id.String()) - if err == nil { - // it's a status - l.Debugf("uri is for status with id: %s", s.ID) - if err := f.db.DeleteByID(ctx, s.ID, >smodel.Status{}); err != nil { - return fmt.Errorf("DELETE: err deleting status: %s", err) - } + if s, err := f.db.GetStatusByURI(ctx, id.String()); err == nil && requestingAccount.ID == s.AccountID { + l.Debugf("uri is for STATUS with id: %s", s.ID) f.fedWorker.Queue(messages.FromFederator{ APObjectType: ap.ObjectNote, APActivityType: ap.ActivityDelete, @@ -68,10 +60,8 @@ func (f *federatingDB) Delete(ctx context.Context, id *url.URL) error { }) } - a, err := f.db.GetAccountByURI(ctx, id.String()) - if err == nil { - // it's an account - l.Debugf("uri is for an account with id %s, passing delete message to the processor", a.ID) + if a, err := f.db.GetAccountByURI(ctx, id.String()); err == nil && requestingAccount.ID == a.ID { + l.Debugf("uri is for ACCOUNT with id %s", a.ID) f.fedWorker.Queue(messages.FromFederator{ APObjectType: ap.ObjectProfile, APActivityType: ap.ActivityDelete, diff --git a/internal/processing/account/delete.go b/internal/processing/account/delete.go index f71f08954..bf7f60d67 100644 --- a/internal/processing/account/delete.go +++ b/internal/processing/account/delete.go @@ -57,7 +57,6 @@ import ( // 18. Delete account itself func (p *processor) Delete(ctx context.Context, account *gtsmodel.Account, origin string) gtserror.WithCode { fields := kv.Fields{ - {"username", account.Username}, } if account.Domain != "" { @@ -163,7 +162,7 @@ selectStatusesLoop: // pass the status delete through the client api channel for processing s.Account = account - l.Debug("putting status in the client api channel") + l.Debug("putting status delete in the client api channel") p.clientWorker.Queue(messages.FromClientAPI{ APObjectType: ap.ObjectNote, APActivityType: ap.ActivityDelete, @@ -172,50 +171,26 @@ selectStatusesLoop: TargetAccount: account, }) - if err := p.db.DeleteByID(ctx, s.ID, s); err != nil { - if !errors.Is(err, db.ErrNoEntries) { - // actual error has occurred - l.Errorf("Delete: db error deleting status %s for account %s: %s", s.ID, account.Username, err) - continue - } - } - // if there are any boosts of this status, delete them as well - boosts := []*gtsmodel.Status{} - if err := p.db.GetWhere(ctx, []db.Where{{Key: "boost_of_id", Value: s.ID}}, &boosts); err != nil { - if !errors.Is(err, db.ErrNoEntries) { - // an actual error has occurred - l.Errorf("Delete: db error selecting boosts of status %s for account %s: %s", s.ID, account.Username, err) - continue - } - } - - for _, b := range boosts { - if b.Account == nil { - bAccount, err := p.db.GetAccountByID(ctx, b.AccountID) - if err != nil { - l.Errorf("Delete: db error populating boosted status account: %v", err) - continue + if boosts, err := p.db.GetStatusReblogs(ctx, s); err == nil { + for _, b := range boosts { + if b.Account == nil { + bAccount, err := p.db.GetAccountByID(ctx, b.AccountID) + if err != nil { + l.Errorf("Delete: db error populating boosted status account: %v", err) + continue + } + b.Account = bAccount } - b.Account = bAccount - } - - l.Debug("putting boost undo in the client api channel") - p.clientWorker.Queue(messages.FromClientAPI{ - APObjectType: ap.ActivityAnnounce, - APActivityType: ap.ActivityUndo, - GTSModel: s, - OriginAccount: b.Account, - TargetAccount: account, - }) - - if err := p.db.DeleteByID(ctx, b.ID, b); err != nil { - if err != db.ErrNoEntries { - // actual error has occurred - l.Errorf("Delete: db error deleting boost with id %s: %s", b.ID, err) - continue - } + l.Debug("putting boost undo in the client api channel") + p.clientWorker.Queue(messages.FromClientAPI{ + APObjectType: ap.ActivityAnnounce, + APActivityType: ap.ActivityUndo, + GTSModel: s, + OriginAccount: b.Account, + TargetAccount: account, + }) } } diff --git a/internal/processing/fromclientapi.go b/internal/processing/fromclientapi.go index 2c4f20d81..d7c9c5d82 100644 --- a/internal/processing/fromclientapi.go +++ b/internal/processing/fromclientapi.go @@ -288,6 +288,10 @@ func (p *processor) processUndoAnnounceFromClientAPI(ctx context.Context, client return errors.New("undo was not parseable as *gtsmodel.Status") } + if err := p.db.DeleteStatusByID(ctx, boost.ID); err != nil { + return err + } + if err := p.deleteStatusFromTimelines(ctx, boost); err != nil { return err } diff --git a/internal/processing/fromcommon.go b/internal/processing/fromcommon.go index fca23304c..3d8270e32 100644 --- a/internal/processing/fromcommon.go +++ b/internal/processing/fromcommon.go @@ -469,29 +469,27 @@ func (p *processor) wipeStatus(ctx context.Context, statusToDelete *gtsmodel.Sta } } - // delete all mentions for this status + // delete all mention entries generated by this status for _, m := range statusToDelete.MentionIDs { if err := p.db.DeleteByID(ctx, m, >smodel.Mention{}); err != nil { return err } } - // delete all notifications for this status + // delete all notification entries generated by this status if err := p.db.DeleteWhere(ctx, []db.Where{{Key: "status_id", Value: statusToDelete.ID}}, &[]*gtsmodel.Notification{}); err != nil { return err } // delete all boosts for this status + remove them from timelines - boosts, err := p.db.GetStatusReblogs(ctx, statusToDelete) - if err != nil { - return err - } - for _, b := range boosts { - if err := p.deleteStatusFromTimelines(ctx, b); err != nil { - return err - } - if err := p.db.DeleteByID(ctx, b.ID, b); err != nil { - return err + if boosts, err := p.db.GetStatusReblogs(ctx, statusToDelete); err == nil { + for _, b := range boosts { + if err := p.deleteStatusFromTimelines(ctx, b); err != nil { + return err + } + if err := p.db.DeleteStatusByID(ctx, b.ID); err != nil { + return err + } } } @@ -500,5 +498,10 @@ func (p *processor) wipeStatus(ctx context.Context, statusToDelete *gtsmodel.Sta return err } + // delete the status itself + if err := p.db.DeleteStatusByID(ctx, statusToDelete.ID); err != nil { + return err + } + return nil } diff --git a/internal/processing/fromfederator_test.go b/internal/processing/fromfederator_test.go index 8489303e8..385f3b134 100644 --- a/internal/processing/fromfederator_test.go +++ b/internal/processing/fromfederator_test.go @@ -368,9 +368,12 @@ func (suite *FromFederatorTestSuite) TestProcessAccountDelete() { suite.False(zorkFollowsSatan) // no statuses from foss satan should be left in the database - dbStatuses, err := suite.db.GetAccountStatuses(ctx, deletedAccount.ID, 0, false, false, "", "", false, false, false) - suite.ErrorIs(err, db.ErrNoEntries) - suite.Empty(dbStatuses) + if !testrig.WaitFor(func() bool { + s, err := suite.db.GetAccountStatuses(ctx, deletedAccount.ID, 0, false, false, "", "", false, false, false) + return s == nil && err == db.ErrNoEntries + }) { + suite.FailNow("timeout waiting for statuses to be deleted") + } dbAccount, err := suite.db.GetAccountByID(ctx, deletedAccount.ID) suite.NoError(err) diff --git a/internal/processing/status/delete.go b/internal/processing/status/delete.go index 6db0d9890..a5c6c27bf 100644 --- a/internal/processing/status/delete.go +++ b/internal/processing/status/delete.go @@ -48,11 +48,7 @@ func (p *processor) Delete(ctx context.Context, requestingAccount *gtsmodel.Acco return nil, gtserror.NewErrorInternalError(fmt.Errorf("error converting status %s to frontend representation: %s", targetStatus.ID, err)) } - if err := p.db.DeleteByID(ctx, targetStatus.ID, >smodel.Status{}); err != nil { - return nil, gtserror.NewErrorInternalError(fmt.Errorf("error deleting status from the database: %s", err)) - } - - // send it back to the processor for async processing + // send the status back to the processor for async processing p.clientWorker.Queue(messages.FromClientAPI{ APObjectType: ap.ObjectNote, APActivityType: ap.ActivityDelete, diff --git a/internal/processing/status/unboost.go b/internal/processing/status/unboost.go index 75158fd40..113a99b18 100644 --- a/internal/processing/status/unboost.go +++ b/internal/processing/status/unboost.go @@ -78,14 +78,8 @@ func (p *processor) Unboost(ctx context.Context, requestingAccount *gtsmodel.Acc } if toUnboost { - // we had a boost, so take some action to get rid of it - if err := p.db.DeleteWhere(ctx, where, >smodel.Status{}); err != nil { - return nil, gtserror.NewErrorInternalError(fmt.Errorf("error unboosting status: %s", err)) - } - // pin some stuff onto the boost while we have it out of the db gtsBoost.Account = requestingAccount - gtsBoost.BoostOf = targetStatus gtsBoost.BoostOfAccount = targetStatus.Account gtsBoost.BoostOf.Account = targetStatus.Account diff --git a/testrig/testmodels.go b/testrig/testmodels.go index b38494642..80e6e904a 100644 --- a/testrig/testmodels.go +++ b/testrig/testmodels.go @@ -822,7 +822,7 @@ func NewTestAttachments() map[string]*gtsmodel.MediaAttachment { "remote_account_1_status_1_attachment_1": { ID: "01FVW7RXPQ8YJHTEXYPE7Q8ZY0", StatusID: "01FVW7JHQFSFK166WWKR8CBA6M", - URL: "http://localhost:8080/fileserver/01F8MH1H7YV1Z7D2C8K2730QBF/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + URL: "http://localhost:8080/fileserver/01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", RemoteURL: "http://fossbros-anonymous.io/attachments/original/13bbc3f8-2b5e-46ea-9531-40b4974d9912.jpeg", CreatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), @@ -845,23 +845,23 @@ func NewTestAttachments() map[string]*gtsmodel.MediaAttachment { Y: 0, }, }, - AccountID: "01F8MH1H7YV1Z7D2C8K2730QBF", + AccountID: "01F8MH5ZK5VRH73AKHQM6Y9VNX", Description: "tweet from thoughts of dog: i drank. all the water. in my bowl. earlier. but just now. i returned. to the same bowl. and it was. full again.. the bowl. is haunted", ScheduledStatusID: "", Blurhash: "LARysgM_IU_3~pD%M_Rj_39FIAt6", Processing: 2, File: gtsmodel.File{ - Path: "01F8MH1H7YV1Z7D2C8K2730QBF/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + Path: "01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", ContentType: "image/jpeg", FileSize: 19310, UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), }, Thumbnail: gtsmodel.Thumbnail{ - Path: "01F8MH1H7YV1Z7D2C8K2730QBF/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + Path: "01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", ContentType: "image/jpeg", FileSize: 20395, UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), - URL: "http://localhost:8080/fileserver/01F8MH1H7YV1Z7D2C8K2730QBF/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + URL: "http://localhost:8080/fileserver/01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", RemoteURL: "http://fossbros-anonymous.io/attachments/small/a499f55b-2d1e-4acd-98d2-1ac2ba6d79b9.jpeg", }, Avatar: FalseBool(), @@ -871,7 +871,7 @@ func NewTestAttachments() map[string]*gtsmodel.MediaAttachment { "remote_account_1_status_1_attachment_2": { ID: "01FVW7RXPQ8YJHTEXYPE7Q8ZY1", StatusID: "01FVW7JHQFSFK166WWKR8CBA6M", - URL: "http://localhost:8080/fileserver/01F8MH1H7YV1Z7D2C8K2730QBF/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + URL: "http://localhost:8080/fileserver/01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", RemoteURL: "http://fossbros-anonymous.io/attachments/original/13bbc3f8-2b5e-46ea-9531-40b4974d9912.jpeg", CreatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), @@ -894,23 +894,23 @@ func NewTestAttachments() map[string]*gtsmodel.MediaAttachment { Y: 0, }, }, - AccountID: "01F8MH1H7YV1Z7D2C8K2730QBF", + AccountID: "01F8MH5ZK5VRH73AKHQM6Y9VNX", Description: "tweet from thoughts of dog: i drank. all the water. in my bowl. earlier. but just now. i returned. to the same bowl. and it was. full again.. the bowl. is haunted", ScheduledStatusID: "", Blurhash: "LARysgM_IU_3~pD%M_Rj_39FIAt6", Processing: 2, File: gtsmodel.File{ - Path: "01F8MH1H7YV1Z7D2C8K2730QBF/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + Path: "01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/original/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", ContentType: "image/jpeg", FileSize: 19310, UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), }, Thumbnail: gtsmodel.Thumbnail{ - Path: "01F8MH1H7YV1Z7D2C8K2730QBF/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + Path: "01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", ContentType: "image/jpeg", FileSize: 20395, UpdatedAt: TimeMustParse("2021-09-20T12:40:37+02:00"), - URL: "http://localhost:8080/fileserver/01F8MH1H7YV1Z7D2C8K2730QBF/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", + URL: "http://localhost:8080/fileserver/01F8MH5ZK5VRH73AKHQM6Y9VNX/attachment/small/01FVW7RXPQ8YJHTEXYPE7Q8ZY0.jpeg", RemoteURL: "http://fossbros-anonymous.io/attachments/small/a499f55b-2d1e-4acd-98d2-1ac2ba6d79b9.jpeg", }, Avatar: FalseBool(),