[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

@@ -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

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(&gtsmodel.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(&gtsmodel.StatusToTag{}).
+			Where("status_id = ?", bun.Ident(id)).
+			Exec(ctx); err != nil {
+			return err
+		}
+
+		// delete the status itself
+		if _, err := tx.
+			NewDelete().
+			Model(&gtsmodel.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)

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))
 }

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)
 

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, &gtsmodel.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,

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,
+					})
 				}
 			}
 

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
 	}

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, &gtsmodel.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
 }

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)

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, &gtsmodel.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,

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, &gtsmodel.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