[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/federation/dereferencing/announce.go

@@ -20,66 +20,107 @@ package dereferencing
 import (
 	"context"
 	"errors"
-	"fmt"
 	"net/url"
 
 	"github.com/superseriousbusiness/gotosocial/internal/config"
+	"github.com/superseriousbusiness/gotosocial/internal/db"
+	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/id"
 )
 
-func (d *Dereferencer) DereferenceAnnounce(ctx context.Context, announce *gtsmodel.Status, requestingUsername string) error {
-	if announce.BoostOf == nil {
-		// we can't do anything unfortunately
-		return errors.New("DereferenceAnnounce: no URI to dereference")
+// EnrichAnnounce enriches the given boost wrapper status
+// by either fetching from the DB or dereferencing the target
+// status, populating the boost wrapper's fields based on the
+// target status, and then storing the wrapper in the database.
+// The wrapper is then returned to the caller.
+//
+// The provided boost wrapper status must have BoostOfURI set.
+func (d *Dereferencer) EnrichAnnounce(
+	ctx context.Context,
+	boost *gtsmodel.Status,
+	requestUser string,
+) (*gtsmodel.Status, error) {
+	targetURI := boost.BoostOfURI
+	if targetURI == "" {
+		// We can't do anything.
+		return nil, gtserror.Newf("no URI to dereference")
 	}
 
-	// Parse the boosted status' URI
-	boostedURI, err := url.Parse(announce.BoostOf.URI)
+	// Parse the boost target status URI.
+	targetURIObj, err := url.Parse(targetURI)
 	if err != nil {
-		return fmt.Errorf("DereferenceAnnounce: couldn't parse boosted status URI %s: %s", announce.BoostOf.URI, err)
+		return nil, gtserror.Newf(
+			"couldn't parse boost target status URI %s: %w",
+			targetURI, err,
+		)
 	}
 
-	// Check whether the originating status is from a blocked host
-	if blocked, err := d.state.DB.IsDomainBlocked(ctx, boostedURI.Host); blocked || err != nil {
-		return fmt.Errorf("DereferenceAnnounce: domain %s is blocked", boostedURI.Host)
+	// Fetch/deref status being boosted.
+	var target *gtsmodel.Status
+
+	if targetURIObj.Host == config.GetHost() {
+		// This is a local status, fetch from the database
+		target, err = d.state.DB.GetStatusByURI(ctx, targetURI)
+	} else {
+		// This is a remote status, we need to dereference it.
+		//
+		// d.GetStatusByURI will handle domain block checking for us,
+		// so we don't try to deref an announce target on a blocked host.
+		target, _, err = d.GetStatusByURI(ctx, requestUser, targetURIObj)
 	}
 
-	var boostedStatus *gtsmodel.Status
+	if err != nil {
+		return nil, gtserror.Newf(
+			"error getting boost target status %s: %w",
+			targetURI, err,
+		)
+	}
 
-	if boostedURI.Host == config.GetHost() {
-		// This is a local status, fetch from the database
-		status, err := d.state.DB.GetStatusByURI(ctx, boostedURI.String())
-		if err != nil {
-			return fmt.Errorf("DereferenceAnnounce: error fetching local status %q: %v", announce.BoostOf.URI, err)
-		}
+	// Generate an ID for the boost wrapper status.
+	boost.ID, err = id.NewULIDFromTime(boost.CreatedAt)
+	if err != nil {
+		return nil, gtserror.Newf("error generating id: %w", err)
+	}
 
-		// Set boosted status
-		boostedStatus = status
-	} else {
-		// This is a boost of a remote status, we need to dereference it.
-		status, _, err := d.GetStatusByURI(ctx, requestingUsername, boostedURI)
+	// Populate remaining fields on
+	// the boost wrapper using target.
+	boost.Content = target.Content
+	boost.ContentWarning = target.ContentWarning
+	boost.ActivityStreamsType = target.ActivityStreamsType
+	boost.Sensitive = target.Sensitive
+	boost.Language = target.Language
+	boost.Text = target.Text
+	boost.BoostOfID = target.ID
+	boost.BoostOf = target
+	boost.BoostOfAccountID = target.AccountID
+	boost.BoostOfAccount = target.Account
+	boost.Visibility = target.Visibility
+	boost.Federated = target.Federated
+	boost.Boostable = target.Boostable
+	boost.Replyable = target.Replyable
+	boost.Likeable = target.Likeable
+
+	// Store the boost wrapper status.
+	switch err = d.state.DB.PutStatus(ctx, boost); {
+	case err == nil:
+		// All good baby.
+
+	case errors.Is(err, db.ErrAlreadyExists):
+		// DATA RACE! We likely lost out to another goroutine
+		// in a call to db.Put(Status). Look again in DB by URI.
+		boost, err = d.state.DB.GetStatusByURI(ctx, boost.URI)
 		if err != nil {
-			return fmt.Errorf("DereferenceAnnounce: error dereferencing remote status with id %s: %s", announce.BoostOf.URI, err)
+			err = gtserror.Newf(
+				"error getting boost wrapper status %s from database after race: %w",
+				boost.URI, err,
+			)
 		}
 
-		// Set boosted status
-		boostedStatus = status
+	default:
+		// Proper database error.
+		err = gtserror.Newf("db error inserting status: %w", err)
 	}
 
-	announce.Content = boostedStatus.Content
-	announce.ContentWarning = boostedStatus.ContentWarning
-	announce.ActivityStreamsType = boostedStatus.ActivityStreamsType
-	announce.Sensitive = boostedStatus.Sensitive
-	announce.Language = boostedStatus.Language
-	announce.Text = boostedStatus.Text
-	announce.BoostOfID = boostedStatus.ID
-	announce.BoostOfAccountID = boostedStatus.AccountID
-	announce.Visibility = boostedStatus.Visibility
-	announce.Federated = boostedStatus.Federated
-	announce.Boostable = boostedStatus.Boostable
-	announce.Replyable = boostedStatus.Replyable
-	announce.Likeable = boostedStatus.Likeable
-	announce.BoostOf = boostedStatus
-
-	return nil
+	return boost, err
 }

internal/federation/federatingdb/announce_test.go

@@ -50,8 +50,10 @@ func (suite *AnnounceTestSuite) TestNewAnnounce() {
 	suite.True(ok)
 	suite.Equal(announcingAccount.ID, boost.AccountID)
 
-	// only the URI will be set on the boosted status because it still needs to be dereferenced
-	suite.NotEmpty(boost.BoostOf.URI)
+	// only the URI will be set for the boosted status
+	// because it still needs to be dereferenced
+	suite.Nil(boost.BoostOf)
+	suite.Equal("http://example.org/users/Some_User/statuses/afaba698-5740-4e32-a702-af61aa543bc1", boost.BoostOfURI)
 }
 
 func (suite *AnnounceTestSuite) TestAnnounceTwice() {
@@ -81,8 +83,10 @@ func (suite *AnnounceTestSuite) TestAnnounceTwice() {
 		return nil
 	})
 
-	// only the URI will be set on the boosted status because it still needs to be dereferenced
-	suite.NotEmpty(boost.BoostOf.URI)
+	// only the URI will be set for the boosted status
+	// because it still needs to be dereferenced
+	suite.Nil(boost.BoostOf)
+	suite.Equal("http://example.org/users/Some_User/statuses/afaba698-5740-4e32-a702-af61aa543bc1", boost.BoostOfURI)
 
 	ctx2 := createTestContext(receivingAccount2, announcingAccount)
 	announce2 := suite.testActivities["announce_forwarded_1_turtle"]

internal/gtsmodel/status.go

@@ -50,6 +50,7 @@ type Status struct {
 	InReplyTo                *Status            `bun:"-"`                                                           // status corresponding to inReplyToID
 	InReplyToAccount         *Account           `bun:"rel:belongs-to"`                                              // account corresponding to inReplyToAccountID
 	BoostOfID                string             `bun:"type:CHAR(26),nullzero"`                                      // id of the status this status is a boost of
+	BoostOfURI               string             `bun:"-"`                                                           // URI of the status this status is a boost of; field not inserted in the db, just for dereferencing purposes.
 	BoostOfAccountID         string             `bun:"type:CHAR(26),nullzero"`                                      // id of the account that owns the boosted status
 	BoostOf                  *Status            `bun:"-"`                                                           // status that corresponds to boostOfID
 	BoostOfAccount           *Account           `bun:"rel:belongs-to"`                                              // account that corresponds to boostOfAccountID

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)