[bugfix/chore] `Announce` reliability updates (#2405)v0.13.0-rc1

* [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

5 files changed, 161 insertions, 141 deletions

diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go
index ad42a687d..5bcb35d75 100644
--- a/internal/typeutils/astointernal.go
+++ b/internal/typeutils/astointernal.go
@@ -527,29 +527,15 @@ func (c *Converter) ASBlockToBlock(ctx context.Context, blockable ap.Blockable)
 	}, nil
 }
 
-// ASAnnounceToStatus converts an activitystreams 'announce' into a status.
-//
-// The returned bool indicates whether this status is new (true) or not new (false).
-//
-// In other words, if the status is already in the database with the ID set on the announceable, then that will be returned,
-// the returned bool will be false, and no further processing is necessary. If the returned bool is true, indicating
-// that this is a new announce, then further processing will be necessary, because the returned status will be bareboned and
-// require further dereferencing.
-//
-// This is useful when multiple users on an instance might receive the same boost, and we only want to process the boost once.
-//
-// NOTE -- this is different from one status being boosted multiple times! In this case, new boosts should indeed be created.
-//
-// Implementation note: this function creates and returns a boost WRAPPER
-// status which references the boosted status in its BoostOf field. No
-// dereferencing is done on the boosted status by this function. Callers
-// should look at `status.BoostOf` to see the status being boosted, and do
-// dereferencing on it as appropriate.
-//
-// The returned boolean indicates whether or not the boost has already been
-// seen before by this instance. If it was, then status.BoostOf should be a
-// fully filled-out status. If not, then only status.BoostOf.URI will be set.
-func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Announceable) (*gtsmodel.Status, bool, error) {
+// ASAnnounceToStatus converts an activitystreams 'announce' into a boost
+// wrapper status. The returned bool indicates whether this boost is new
+// (true) or not. If new, callers should use `status.BoostOfURI` to see the
+// status being boosted, and do dereferencing on it as appropriate. If not
+// new, then the boost has already been fully processed and can be ignored.
+func (c *Converter) ASAnnounceToStatus(
+	ctx context.Context,
+	announceable ap.Announceable,
+) (*gtsmodel.Status, bool, error) {
 	// Default assume
 	// we already have.
 	isNew := false
@@ -565,21 +551,21 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno
 	uri := uriObj.String()
 
 	// Check if we already have this boost in the database.
-	status, err := c.state.DB.GetStatusByURI(ctx, uri)
+	boost, err := c.state.DB.GetStatusByURI(ctx, uri)
 	if err != nil && !errors.Is(err, db.ErrNoEntries) {
 		err = gtserror.Newf("db error trying to get status with uri %s: %w", uri, err)
 		return nil, isNew, err
 	}
 
-	if status != nil {
-		// We already have this status,
+	if boost != nil {
+		// We already have this boost,
 		// no need to proceed further.
-		return status, isNew, nil
+		return boost, isNew, nil
 	}
 
-	// Create DB status with URI
-	status = new(gtsmodel.Status)
-	status.URI = uri
+	// Create boost with URI
+	boost = new(gtsmodel.Status)
+	boost.URI = uri
 	isNew = true
 
 	// Get the URI of the boosted status.
@@ -590,22 +576,21 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno
 	}
 
 	// Set the URI of the boosted status on
-	// the new status, for later dereferencing.
-	status.BoostOf = new(gtsmodel.Status)
-	status.BoostOf.URI = boostOf[0].String()
+	// the boost, for later dereferencing.
+	boost.BoostOfURI = boostOf[0].String()
 
 	// Extract published time for the boost,
 	// zero-time will fall back to db defaults.
 	if pub := ap.GetPublished(announceable); !pub.IsZero() {
-		status.CreatedAt = pub
-		status.UpdatedAt = pub
+		boost.CreatedAt = pub
+		boost.UpdatedAt = pub
 	} else {
 		log.Warnf(ctx, "unusable published property on %s", uri)
 	}
 
 	// Extract and load the boost actor account,
 	// (this MUST already be in database by now).
-	status.Account, err = c.getASActorAccount(ctx,
+	boost.Account, err = c.getASActorAccount(ctx,
 		uri,
 		announceable,
 	)
@@ -614,13 +599,13 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno
 	}
 
 	// Set the related status<->account fields.
-	status.AccountURI = status.Account.URI
-	status.AccountID = status.Account.ID
+	boost.AccountURI = boost.Account.URI
+	boost.AccountID = boost.Account.ID
 
 	// Calculate intended visibility of the boost.
-	status.Visibility, err = ap.ExtractVisibility(
+	boost.Visibility, err = ap.ExtractVisibility(
 		announceable,
-		status.Account.FollowersURI,
+		boost.Account.FollowersURI,
 	)
 	if err != nil {
 		err := gtserror.Newf("error extracting status visibility for %s: %w", uri, err)
@@ -629,15 +614,15 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno
 
 	// Below IDs will all be included in the
 	// boosted status, so set them empty here.
-	status.AttachmentIDs = make([]string, 0)
-	status.TagIDs = make([]string, 0)
-	status.MentionIDs = make([]string, 0)
-	status.EmojiIDs = make([]string, 0)
-
-	// Remaining fields on the boost status will be taken
-	// from the boosted status; it's not our job to do all
-	// that dereferencing here.
-	return status, isNew, nil
+	boost.AttachmentIDs = make([]string, 0)
+	boost.TagIDs = make([]string, 0)
+	boost.MentionIDs = make([]string, 0)
+	boost.EmojiIDs = make([]string, 0)
+
+	// Remaining fields on the boost will be
+	// taken from the target status; it's not
+	// our job to do all that dereferencing here.
+	return boost, isNew, nil
 }
 
 // ASFlagToReport converts a remote activitystreams 'flag' representation into a gts model report.
diff --git a/internal/typeutils/astointernal_test.go b/internal/typeutils/astointernal_test.go
index 40bf4c855..4be2434e5 100644
--- a/internal/typeutils/astointernal_test.go
+++ b/internal/typeutils/astointernal_test.go
@@ -463,6 +463,46 @@ func (suite *ASToInternalTestSuite) TestParseFlag6() {
 	suite.Equal(report.Comment, "misinformation")
 }
 
+func (suite *ASToInternalTestSuite) TestParseAnnounce() {
+	// Boost a status that belongs to a local account
+	boostingAccount := suite.testAccounts["remote_account_1"]
+	targetStatus := suite.testStatuses["local_account_2_status_1"]
+	receivingAccount := suite.testAccounts["local_account_1"]
+
+	raw := `{
+  "@context": "https://www.w3.org/ns/activitystreams",
+  "actor": "` + boostingAccount.URI + `",
+  "id": "http://fossbros-anonymous.io/db22128d-884e-4358-9935-6a7c3940535d",
+  "object": ["` + targetStatus.URI + `"],
+  "type": "Announce",
+  "to": "` + receivingAccount.URI + `"
+  }`
+
+	t := suite.jsonToType(raw)
+	asAnnounce, ok := t.(ap.Announceable)
+	if !ok {
+		suite.FailNow("type not coercible")
+	}
+
+	boost, isNew, err := suite.typeconverter.ASAnnounceToStatus(context.Background(), asAnnounce)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.True(isNew)
+	suite.NotNil(boost)
+	suite.Equal(boostingAccount.ID, boost.AccountID)
+	suite.NotNil(boost.Account)
+
+	// Of the 'BoostOf' fields, only BoostOfURI will be set.
+	// Others are set in dereferencing.EnrichAnnounceSafely.
+	suite.Equal(targetStatus.URI, boost.BoostOfURI)
+	suite.Empty(boost.BoostOfID)
+	suite.Nil(boost.BoostOf)
+	suite.Empty(boost.BoostOfAccountID)
+	suite.Nil(boost.BoostOfAccount)
+}
+
 func TestASToInternalTestSuite(t *testing.T) {
 	suite.Run(t, new(ASToInternalTestSuite))
 }
diff --git a/internal/typeutils/internal.go b/internal/typeutils/internal.go
index 1824a4421..095e2b121 100644
--- a/internal/typeutils/internal.go
+++ b/internal/typeutils/internal.go
@@ -19,90 +19,86 @@ package typeutils
 
 import (
 	"context"
-	"time"
 
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/uris"
+	"github.com/superseriousbusiness/gotosocial/internal/util"
 )
 
-// FollowRequestToFollow just converts a follow request into a follow, that's it! No bells and whistles.
-func (c *Converter) FollowRequestToFollow(ctx context.Context, f *gtsmodel.FollowRequest) *gtsmodel.Follow {
-	showReblogs := *f.ShowReblogs
-	notify := *f.Notify
+// FollowRequestToFollow just converts a follow request
+// into a follow, that's it! No bells and whistles.
+func (c *Converter) FollowRequestToFollow(
+	ctx context.Context,
+	fr *gtsmodel.FollowRequest,
+) *gtsmodel.Follow {
 	return &gtsmodel.Follow{
-		ID:              f.ID,
-		CreatedAt:       f.CreatedAt,
-		UpdatedAt:       f.UpdatedAt,
-		AccountID:       f.AccountID,
-		TargetAccountID: f.TargetAccountID,
-		ShowReblogs:     &showReblogs,
-		URI:             f.URI,
-		Notify:          &notify,
+		ID:              fr.ID,
+		CreatedAt:       fr.CreatedAt,
+		UpdatedAt:       fr.UpdatedAt,
+		AccountID:       fr.AccountID,
+		TargetAccountID: fr.TargetAccountID,
+		ShowReblogs:     util.Ptr(*fr.ShowReblogs),
+		URI:             fr.URI,
+		Notify:          util.Ptr(*fr.Notify),
 	}
 }
 
-// StatusToBoost wraps the given status into a boosting status.
-func (c *Converter) StatusToBoost(ctx context.Context, s *gtsmodel.Status, boostingAccount *gtsmodel.Account) (*gtsmodel.Status, error) {
-	// the wrapper won't use the same ID as the boosted status so we generate some new UUIDs
-	accountURIs := uris.GenerateURIsForAccount(boostingAccount.Username)
-	boostWrapperStatusID := id.NewULID()
-	boostWrapperStatusURI := accountURIs.StatusesURI + "/" + boostWrapperStatusID
-	boostWrapperStatusURL := accountURIs.StatusesURL + "/" + boostWrapperStatusID
+// StatusToBoost wraps the target status into a
+// boost wrapper status owned by the requester.
+func (c *Converter) StatusToBoost(
+	ctx context.Context,
+	target *gtsmodel.Status,
+	booster *gtsmodel.Account,
+	applicationID string,
+) (*gtsmodel.Status, error) {
+	// The boost won't use the same IDs as the
+	// target so we need to generate new ones.
+	boostID := id.NewULID()
+	accountURIs := uris.GenerateURIsForAccount(booster.Username)
 
-	local := true
-	if boostingAccount.Domain != "" {
-		local = false
-	}
-
-	sensitive := *s.Sensitive
-	federated := *s.Federated
-	boostable := *s.Boostable
-	replyable := *s.Replyable
-	likeable := *s.Likeable
+	boost := &gtsmodel.Status{
+		ID:  boostID,
+		URI: accountURIs.StatusesURI + "/" + boostID,
+		URL: accountURIs.StatusesURL + "/" + boostID,
 
-	boostWrapperStatus := &gtsmodel.Status{
-		ID:  boostWrapperStatusID,
-		URI: boostWrapperStatusURI,
-		URL: boostWrapperStatusURL,
+		// Inherit some fields from the booster account.
+		Local:                    util.Ptr(booster.IsLocal()),
+		AccountID:                booster.ID,
+		Account:                  booster,
+		AccountURI:               booster.URI,
+		CreatedWithApplicationID: applicationID,
 
-		// the boosted status is not created now, but the boost certainly is
-		CreatedAt:  time.Now(),
-		UpdatedAt:  time.Now(),
-		Local:      &local,
-		AccountID:  boostingAccount.ID,
-		AccountURI: boostingAccount.URI,
-
-		// replies can be boosted, but boosts are never replies
+		// Replies can be boosted, but
+		// boosts are never replies.
 		InReplyToID:        "",
 		InReplyToAccountID: "",
 
-		// these will all be wrapped in the boosted status so set them empty here
+		// These will all be wrapped in the
+		// boosted status so set them empty.
 		AttachmentIDs: []string{},
 		TagIDs:        []string{},
 		MentionIDs:    []string{},
 		EmojiIDs:      []string{},
 
-		// the below fields will be taken from the target status
-		Content:             s.Content,
-		ContentWarning:      s.ContentWarning,
-		ActivityStreamsType: s.ActivityStreamsType,
-		Sensitive:           &sensitive,
-		Language:            s.Language,
-		Text:                s.Text,
-		BoostOfID:           s.ID,
-		BoostOfAccountID:    s.AccountID,
-		Visibility:          s.Visibility,
-		Federated:           &federated,
-		Boostable:           &boostable,
-		Replyable:           &replyable,
-		Likeable:            &likeable,
-
-		// attach these here for convenience -- the boosted status/account won't go in the DB
-		// but they're needed in the processor and for the frontend. Since we have them, we can
-		// attach them so we don't need to fetch them again later (save some DB calls)
-		BoostOf: s,
+		// Remaining fields all
+		// taken from boosted status.
+		Content:             target.Content,
+		ContentWarning:      target.ContentWarning,
+		ActivityStreamsType: target.ActivityStreamsType,
+		Sensitive:           util.Ptr(*target.Sensitive),
+		Language:            target.Language,
+		Text:                target.Text,
+		BoostOfID:           target.ID,
+		BoostOf:             target,
+		BoostOfAccountID:    target.AccountID,
+		BoostOfAccount:      target.Account,
+		Visibility:          target.Visibility,
+		Federated:           util.Ptr(*target.Federated),
+		Boostable:           util.Ptr(*target.Boostable),
+		Replyable:           util.Ptr(*target.Replyable),
+		Likeable:            util.Ptr(*target.Likeable),
 	}
 
-	return boostWrapperStatus, nil
+	return boost, nil
 }
diff --git a/internal/typeutils/internaltoas_test.go b/internal/typeutils/internaltoas_test.go
index 0e0607279..cbeaf3c8c 100644
--- a/internal/typeutils/internaltoas_test.go
+++ b/internal/typeutils/internaltoas_test.go
@@ -724,10 +724,11 @@ func (suite *InternalToASTestSuite) TestSelfBoostFollowersOnlyToAS() {
 	testStatus := suite.testStatuses["local_account_1_status_5"]
 	testAccount := suite.testAccounts["local_account_1"]
 
-	boostWrapperStatus, err := suite.typeconverter.StatusToBoost(ctx, testStatus, testAccount)
+	boostWrapperStatus, err := suite.typeconverter.StatusToBoost(ctx, testStatus, testAccount, "")
 	suite.NoError(err)
 	suite.NotNil(boostWrapperStatus)
 
+	// Set some fields to predictable values for the test.
 	boostWrapperStatus.ID = "01G74JJ1KS331G2JXHRMZCE0ER"
 	boostWrapperStatus.URI = "http://localhost:8080/users/the_mighty_zork/statuses/01G74JJ1KS331G2JXHRMZCE0ER"
 	boostWrapperStatus.CreatedAt = testrig.TimeMustParse("2022-06-09T13:12:00Z")
diff --git a/internal/typeutils/internaltofrontend.go b/internal/typeutils/internaltofrontend.go
index cd53d9ae6..fe5c8ec8f 100644
--- a/internal/typeutils/internaltofrontend.go
+++ b/internal/typeutils/internaltofrontend.go
@@ -733,11 +733,18 @@ func (c *Converter) statusToFrontend(
 	s *gtsmodel.Status,
 	requestingAccount *gtsmodel.Account,
 ) (*apimodel.Status, error) {
+	// Try to populate status struct pointer fields.
+	// We can continue in many cases of partial failure,
+	// but there are some fields we actually need.
 	if err := c.state.DB.PopulateStatus(ctx, s); err != nil {
-		// Ensure author account present + correct;
-		// can't really go further without this!
 		if s.Account == nil {
-			return nil, gtserror.Newf("error(s) populating status, cannot continue: %w", err)
+			err = gtserror.Newf("error(s) populating status, cannot continue (status.Account not set): %w", err)
+			return nil, err
+		}
+
+		if s.BoostOfID != "" && s.BoostOf == nil {
+			err = gtserror.Newf("error(s) populating status, cannot continue (status.BoostOfID set, but status.Boost not set): %w", err)
+			return nil, err
 		}
 
 		log.Errorf(ctx, "error(s) populating status, will continue: %v", err)
@@ -794,12 +801,12 @@ func (c *Converter) statusToFrontend(
 	apiStatus := &apimodel.Status{
 		ID:                 s.ID,
 		CreatedAt:          util.FormatISO8601(s.CreatedAt),
-		InReplyToID:        nil,
-		InReplyToAccountID: nil,
+		InReplyToID:        nil, // Set below.
+		InReplyToAccountID: nil, // Set below.
 		Sensitive:          *s.Sensitive,
 		SpoilerText:        s.ContentWarning,
 		Visibility:         c.VisToAPIVis(ctx, s.Visibility),
-		Language:           nil,
+		Language:           nil, // Set below.
 		URI:                s.URI,
 		URL:                s.URL,
 		RepliesCount:       repliesCount,
@@ -811,56 +818,47 @@ func (c *Converter) statusToFrontend(
 		Reblogged:          interacts.Reblogged,
 		Pinned:             interacts.Pinned,
 		Content:            s.Content,
-		Reblog:             nil,
-		Application:        nil,
+		Reblog:             nil, // Set below.
+		Application:        nil, // Set below.
 		Account:            apiAuthorAccount,
 		MediaAttachments:   apiAttachments,
 		Mentions:           apiMentions,
 		Tags:               apiTags,
 		Emojis:             apiEmojis,
 		Card:               nil, // TODO: implement cards
-		Poll:               nil, // TODO: implement polls
 		Text:               s.Text,
 	}
 
 	// Nullable fields.
-
 	if s.InReplyToID != "" {
-		apiStatus.InReplyToID = func() *string { i := s.InReplyToID; return &i }()
+		apiStatus.InReplyToID = util.Ptr(s.InReplyToID)
 	}
 
 	if s.InReplyToAccountID != "" {
-		apiStatus.InReplyToAccountID = func() *string { i := s.InReplyToAccountID; return &i }()
+		apiStatus.InReplyToAccountID = util.Ptr(s.InReplyToAccountID)
 	}
 
 	if s.Language != "" {
-		apiStatus.Language = func() *string { i := s.Language; return &i }()
+		apiStatus.Language = util.Ptr(s.Language)
 	}
 
 	if s.BoostOf != nil {
-		apiBoostOf, err := c.StatusToAPIStatus(ctx, s.BoostOf, requestingAccount)
+		reblog, err := c.StatusToAPIStatus(ctx, s.BoostOf, requestingAccount)
 		if err != nil {
 			return nil, gtserror.Newf("error converting boosted status: %w", err)
 		}
 
-		apiStatus.Reblog = &apimodel.StatusReblogged{Status: apiBoostOf}
+		apiStatus.Reblog = &apimodel.StatusReblogged{reblog}
 	}
 
-	if appID := s.CreatedWithApplicationID; appID != "" {
-		app := s.CreatedWithApplication
-		if app == nil {
-			app, err = c.state.DB.GetApplicationByID(ctx, appID)
-			if err != nil {
-				return nil, gtserror.Newf("error getting application %s: %w", appID, err)
-			}
-		}
-
-		apiApp, err := c.AppToAPIAppPublic(ctx, app)
+	if app := s.CreatedWithApplication; app != nil {
+		apiStatus.Application, err = c.AppToAPIAppPublic(ctx, app)
 		if err != nil {
-			return nil, gtserror.Newf("error converting application %s: %w", appID, err)
+			return nil, gtserror.Newf(
+				"error converting application %s: %w",
+				s.CreatedWithApplicationID, err,
+			)
 		}
-
-		apiStatus.Application = apiApp
 	}
 
 	if s.Poll != nil {