[bugfix/chore] Announce reliability updates (#2405)

* [bugfix/chore] `Announce` updates

* test update

* fix tests

* TestParseAnnounce

* update comments

* don't lock/unlock, change function signature

* naming stuff

* don't check domain block twice

* UnwrapIfBoost

* beep boop

internal/processing/common/status.go

@@ -119,6 +119,25 @@ func (p *Processor) GetVisibleTargetStatus(
 	return target, nil
 }
 
+// UnwrapIfBoost "unwraps" the given status if
+// it's a boost wrapper, by returning the boosted
+// status it targets (pending visibility checks).
+//
+// Just returns the input status if it's not a boost.
+func (p *Processor) UnwrapIfBoost(
+	ctx context.Context,
+	requester *gtsmodel.Account,
+	status *gtsmodel.Status,
+) (*gtsmodel.Status, gtserror.WithCode) {
+	if status.BoostOfID == "" {
+		return status, nil
+	}
+
+	return p.GetVisibleTargetStatus(ctx,
+		requester, status.BoostOfID,
+	)
+}
+
 // GetAPIStatus fetches the appropriate API status model for target.
 func (p *Processor) GetAPIStatus(
 	ctx context.Context,

internal/processing/fedi/status.go

@@ -51,6 +51,11 @@ func (p *Processor) StatusGet(ctx context.Context, requestedUser string, statusI
 		return nil, gtserror.NewErrorNotFound(errors.New(text))
 	}
 
+	if status.BoostOfID != "" {
+		const text = "status is a boost wrapper"
+		return nil, gtserror.NewErrorNotFound(errors.New(text))
+	}
+
 	visible, err := p.filter.StatusVisible(ctx, requester, status)
 	if err != nil {
 		return nil, gtserror.NewErrorInternalError(err)
@@ -106,6 +111,11 @@ func (p *Processor) StatusRepliesGet(
 		return nil, gtserror.NewErrorNotFound(errors.New(text))
 	}
 
+	if status.BoostOfID != "" {
+		const text = "status is a boost wrapper"
+		return nil, gtserror.NewErrorNotFound(errors.New(text))
+	}
+
 	// Parse replies collection ID from status' URI with onlyOtherAccounts param.
 	onlyOtherAccStr := "only_other_accounts=" + strconv.FormatBool(onlyOtherAccounts)
 	collectionID, err := url.Parse(status.URI + "/replies?" + onlyOtherAccStr)

internal/processing/status/boost.go

@@ -30,106 +30,125 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/messages"
 )
 
-// BoostCreate processes the boost/reblog of a given status, returning the newly-created boost if all is well.
-func (p *Processor) BoostCreate(ctx context.Context, requestingAccount *gtsmodel.Account, application *gtsmodel.Application, targetStatusID string) (*apimodel.Status, gtserror.WithCode) {
-	targetStatus, err := p.state.DB.GetStatusByID(ctx, targetStatusID)
+// BoostCreate processes the boost/reblog of target
+// status, returning the newly-created boost.
+func (p *Processor) BoostCreate(
+	ctx context.Context,
+	requester *gtsmodel.Account,
+	application *gtsmodel.Application,
+	targetID string,
+) (*apimodel.Status, gtserror.WithCode) {
+	// Get target status and ensure it's not a boost.
+	target, errWithCode := p.c.GetVisibleTargetStatus(
+		ctx,
+		requester,
+		targetID,
+	)
+	if errWithCode != nil {
+		return nil, errWithCode
+	}
+
+	target, errWithCode = p.c.UnwrapIfBoost(
+		ctx,
+		requester,
+		target,
+	)
+	if errWithCode != nil {
+		return nil, errWithCode
+	}
+
+	// Ensure valid boost target.
+	boostable, err := p.filter.StatusBoostable(ctx,
+		requester,
+		target,
+	)
 	if err != nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("error fetching status %s: %s", targetStatusID, err))
-	}
-	if targetStatus.Account == nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("no status owner for status %s", targetStatusID))
+		err := gtserror.Newf("error seeing if status %s is boostable: %w", target.ID, err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	// if targetStatusID refers to a boost, then we should redirect
-	// the target to being the status that was boosted; if we don't
-	// do this, then we end up in weird situations where people
-	// boost boosts, and it looks absolutely bizarre in the UI
-	if targetStatus.BoostOfID != "" {
-		if targetStatus.BoostOf == nil {
-			b, err := p.state.DB.GetStatusByID(ctx, targetStatus.BoostOfID)
-			if err != nil {
-				return nil, gtserror.NewErrorNotFound(fmt.Errorf("couldn't fetch boosted status %s", targetStatus.BoostOfID))
-			}
-			targetStatus.BoostOf = b
-		}
-		targetStatus = targetStatus.BoostOf
+	if !boostable {
+		err := gtserror.New("status is not boostable")
+		return nil, gtserror.NewErrorNotFound(err)
 	}
 
-	boostable, err := p.filter.StatusBoostable(ctx, requestingAccount, targetStatus)
-	if err != nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("error seeing if status %s is boostable: %s", targetStatus.ID, err))
-	} else if !boostable {
-		return nil, gtserror.NewErrorNotFound(errors.New("status is not boostable"))
-	}
-
-	// it's visible! it's boostable! so let's boost the FUCK out of it
-	boostWrapperStatus, err := p.converter.StatusToBoost(ctx, targetStatus, requestingAccount)
+	// Status is visible and boostable.
+	boost, err := p.converter.StatusToBoost(ctx,
+		target,
+		requester,
+		application.ID,
+	)
 	if err != nil {
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	boostWrapperStatus.CreatedWithApplicationID = application.ID
-	boostWrapperStatus.BoostOfAccount = targetStatus.Account
-
-	// put the boost in the database
-	if err := p.state.DB.PutStatus(ctx, boostWrapperStatus); err != nil {
+	// Store the new boost.
+	if err := p.state.DB.PutStatus(ctx, boost); err != nil {
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	// send it back to the processor for async processing
+	// Process side effects asynchronously.
 	p.state.Workers.EnqueueClientAPI(ctx, messages.FromClientAPI{
 		APObjectType:   ap.ActivityAnnounce,
 		APActivityType: ap.ActivityCreate,
-		GTSModel:       boostWrapperStatus,
-		OriginAccount:  requestingAccount,
-		TargetAccount:  targetStatus.Account,
+		GTSModel:       boost,
+		OriginAccount:  requester,
+		TargetAccount:  target.Account,
 	})
 
-	return p.c.GetAPIStatus(ctx, requestingAccount, boostWrapperStatus)
+	return p.c.GetAPIStatus(ctx, requester, boost)
 }
 
-// BoostRemove processes the unboost/unreblog of a given status, returning the status if all is well.
-func (p *Processor) BoostRemove(ctx context.Context, requestingAccount *gtsmodel.Account, application *gtsmodel.Application, targetStatusID string) (*apimodel.Status, gtserror.WithCode) {
-	targetStatus, err := p.state.DB.GetStatusByID(ctx, targetStatusID)
-	if err != nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("error fetching status %s: %s", targetStatusID, err))
-	}
-	if targetStatus.Account == nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("no status owner for status %s", targetStatusID))
+// BoostRemove processes the unboost/unreblog of
+// target status, returning the target status.
+func (p *Processor) BoostRemove(
+	ctx context.Context,
+	requester *gtsmodel.Account,
+	application *gtsmodel.Application,
+	targetID string,
+) (*apimodel.Status, gtserror.WithCode) {
+	// Get target status and ensure it's not a boost.
+	target, errWithCode := p.c.GetVisibleTargetStatus(
+		ctx,
+		requester,
+		targetID,
+	)
+	if errWithCode != nil {
+		return nil, errWithCode
 	}
 
-	visible, err := p.filter.StatusVisible(ctx, requestingAccount, targetStatus)
-	if err != nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("error seeing if status %s is visible: %s", targetStatus.ID, err))
-	}
-	if !visible {
-		return nil, gtserror.NewErrorNotFound(errors.New("status is not visible"))
+	target, errWithCode = p.c.UnwrapIfBoost(
+		ctx,
+		requester,
+		target,
+	)
+	if errWithCode != nil {
+		return nil, errWithCode
 	}
 
-	// Check whether the requesting account has boosted the given status ID.
-	boost, err := p.state.DB.GetStatusBoost(ctx, targetStatusID, requestingAccount.ID)
-	if err != nil {
-		return nil, gtserror.NewErrorNotFound(fmt.Errorf("error checking status boost %s: %w", targetStatusID, err))
+	// Check whether requester has actually
+	// boosted target, by trying to get the boost.
+	boost, err := p.state.DB.GetStatusBoost(ctx,
+		target.ID,
+		requester.ID,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err = gtserror.Newf("db error getting boost of %s: %w", target.ID, err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	if boost != nil {
-		// pin some stuff onto the boost while we have it out of the db
-		boost.Account = requestingAccount
-		boost.BoostOf = targetStatus
-		boost.BoostOfAccount = targetStatus.Account
-		boost.BoostOf.Account = targetStatus.Account
-
-		// send it back to the processor for async processing
+		// Status was boosted. Process unboost side effects asynchronously.
 		p.state.Workers.EnqueueClientAPI(ctx, messages.FromClientAPI{
 			APObjectType:   ap.ActivityAnnounce,
 			APActivityType: ap.ActivityUndo,
 			GTSModel:       boost,
-			OriginAccount:  requestingAccount,
-			TargetAccount:  targetStatus.Account,
+			OriginAccount:  requester,
+			TargetAccount:  target.Account,
 		})
 	}
 
-	return p.c.GetAPIStatus(ctx, requestingAccount, targetStatus)
+	return p.c.GetAPIStatus(ctx, requester, target)
 }
 
 // StatusBoostedBy returns a slice of accounts that have boosted the given status, filtered according to privacy settings.

internal/processing/status/fave.go

@@ -33,24 +33,46 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/uris"
 )
 
-func (p *Processor) getFaveableStatus(ctx context.Context, requestingAccount *gtsmodel.Account, targetStatusID string) (*gtsmodel.Status, *gtsmodel.StatusFave, gtserror.WithCode) {
-	targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID)
+func (p *Processor) getFaveableStatus(
+	ctx context.Context,
+	requester *gtsmodel.Account,
+	targetID string,
+) (
+	*gtsmodel.Status,
+	*gtsmodel.StatusFave,
+	gtserror.WithCode,
+) {
+	// Get target status and ensure it's not a boost.
+	target, errWithCode := p.c.GetVisibleTargetStatus(
+		ctx,
+		requester,
+		targetID,
+	)
 	if errWithCode != nil {
 		return nil, nil, errWithCode
 	}
 
-	if !*targetStatus.Likeable {
+	target, errWithCode = p.c.UnwrapIfBoost(
+		ctx,
+		requester,
+		target,
+	)
+	if errWithCode != nil {
+		return nil, nil, errWithCode
+	}
+
+	if !*target.Likeable {
 		err := errors.New("status is not faveable")
 		return nil, nil, gtserror.NewErrorForbidden(err, err.Error())
 	}
 
-	fave, err := p.state.DB.GetStatusFave(ctx, requestingAccount.ID, targetStatusID)
+	fave, err := p.state.DB.GetStatusFave(ctx, requester.ID, target.ID)
 	if err != nil && !errors.Is(err, db.ErrNoEntries) {
 		err = fmt.Errorf("getFaveTarget: error checking existing fave: %w", err)
 		return nil, nil, gtserror.NewErrorInternalError(err)
 	}
 
-	return targetStatus, fave, nil
+	return target, fave, nil
 }
 
 // FaveCreate adds a fave for the requestingAccount, targeting the given status (no-op if fave already exists).

internal/processing/workers/fromfediapi.go

@@ -26,7 +26,6 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
-	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/messages"
 	"github.com/superseriousbusiness/gotosocial/internal/processing/account"
@@ -332,45 +331,44 @@ func (p *fediAPI) CreateLike(ctx context.Context, fMsg messages.FromFediAPI) err
 }
 
 func (p *fediAPI) CreateAnnounce(ctx context.Context, fMsg messages.FromFediAPI) error {
-	status, ok := fMsg.GTSModel.(*gtsmodel.Status)
+	boost, ok := fMsg.GTSModel.(*gtsmodel.Status)
 	if !ok {
 		return gtserror.Newf("%T not parseable as *gtsmodel.Status", fMsg.GTSModel)
 	}
 
 	// Dereference status that this boosts, note
-	// that this will handle dereferencing the status
+	// that this will handle storing the boost in
+	// the db, and dereferencing the target status
 	// ancestors / descendants where appropriate.
-	if err := p.federate.DereferenceAnnounce(ctx,
-		status,
+	var err error
+	boost, err = p.federate.EnrichAnnounce(
+		ctx,
+		boost,
 		fMsg.ReceivingAccount.Username,
-	); err != nil {
+	)
+	if err != nil {
+		if gtserror.IsUnretrievable(err) {
+			// Boosted status domain blocked, nothing to do.
+			log.Debugf(ctx, "skipping announce: %v", err)
+			return nil
+		}
+
+		// Actual error.
 		return gtserror.Newf("error dereferencing announce: %w", err)
 	}
 
-	// Generate an ID for the boost wrapper status.
-	statusID, err := id.NewULIDFromTime(status.CreatedAt)
-	if err != nil {
-		return gtserror.Newf("error generating id: %w", err)
-	}
-	status.ID = statusID
-
-	// Store the boost wrapper status.
-	if err := p.state.DB.PutStatus(ctx, status); err != nil {
-		return gtserror.Newf("db error inserting status: %w", err)
-	}
-
 	// Timeline and notify the announce.
-	if err := p.surface.timelineAndNotifyStatus(ctx, status); err != nil {
+	if err := p.surface.timelineAndNotifyStatus(ctx, boost); err != nil {
 		log.Errorf(ctx, "error timelining and notifying status: %v", err)
 	}
 
-	if err := p.surface.notifyAnnounce(ctx, status); err != nil {
+	if err := p.surface.notifyAnnounce(ctx, boost); err != nil {
 		log.Errorf(ctx, "error notifying announce: %v", err)
 	}
 
 	// Interaction counts changed on the original status;
 	// uncache the prepared version from all timelines.
-	p.surface.invalidateStatusFromTimelines(ctx, status.ID)
+	p.surface.invalidateStatusFromTimelines(ctx, boost.BoostOfID)
 
 	return nil
 }

internal/processing/workers/fromfediapi_test.go

@@ -45,9 +45,7 @@ func (suite *FromFediAPITestSuite) TestProcessFederationAnnounce() {
 	boostingAccount := suite.testAccounts["remote_account_1"]
 	announceStatus := &gtsmodel.Status{}
 	announceStatus.URI = "https://example.org/some-announce-uri"
-	announceStatus.BoostOf = &gtsmodel.Status{
-		URI: boostedStatus.URI,
-	}
+	announceStatus.BoostOfURI = boostedStatus.URI
 	announceStatus.CreatedAt = time.Now()
 	announceStatus.UpdatedAt = time.Now()
 	announceStatus.AccountID = boostingAccount.ID