[chore/bugfix] Refactor `ap/extract.go` functions, return URIs more reliably (#1897)

3 files changed, 143 insertions, 68 deletions

diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go
index aeb6a5917..62b2c138a 100644
--- a/internal/typeutils/astointernal.go
+++ b/internal/typeutils/astointernal.go
@@ -27,6 +27,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"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/log"
 	"github.com/superseriousbusiness/gotosocial/internal/uris"
@@ -61,13 +62,13 @@ func (c *converter) ASRepresentationToAccount(ctx context.Context, accountable a
 
 	// avatar aka icon
 	// if this one isn't extractable in a format we recognise we'll just skip it
-	if avatarURL, err := ap.ExtractIconURL(accountable); err == nil {
+	if avatarURL, err := ap.ExtractIconURI(accountable); err == nil {
 		acct.AvatarRemoteURL = avatarURL.String()
 	}
 
 	// header aka image
 	// if this one isn't extractable in a format we recognise we'll just skip it
-	if headerURL, err := ap.ExtractImageURL(accountable); err == nil {
+	if headerURL, err := ap.ExtractImageURI(accountable); err == nil {
 		acct.HeaderRemoteURL = headerURL.String()
 	}
 
@@ -310,7 +311,7 @@ func (c *converter) ASStatusToStatus(ctx context.Context, statusable ap.Statusab
 
 	// which account posted this status?
 	// if we don't know the account yet we can dereference it later
-	attributedTo, err := ap.ExtractAttributedTo(statusable)
+	attributedTo, err := ap.ExtractAttributedToURI(statusable)
 	if err != nil {
 		return nil, errors.New("ASStatusToStatus: attributedTo was empty")
 	}
@@ -386,7 +387,7 @@ func (c *converter) ASFollowToFollowRequest(ctx context.Context, followable ap.F
 	}
 	uri := idProp.GetIRI().String()
 
-	origin, err := ap.ExtractActor(followable)
+	origin, err := ap.ExtractActorURI(followable)
 	if err != nil {
 		return nil, errors.New("error extracting actor property from follow")
 	}
@@ -395,7 +396,7 @@ func (c *converter) ASFollowToFollowRequest(ctx context.Context, followable ap.F
 		return nil, fmt.Errorf("error extracting account with uri %s from the database: %s", origin.String(), err)
 	}
 
-	target, err := ap.ExtractObject(followable)
+	target, err := ap.ExtractObjectURI(followable)
 	if err != nil {
 		return nil, errors.New("error extracting object property from follow")
 	}
@@ -420,7 +421,7 @@ func (c *converter) ASFollowToFollow(ctx context.Context, followable ap.Followab
 	}
 	uri := idProp.GetIRI().String()
 
-	origin, err := ap.ExtractActor(followable)
+	origin, err := ap.ExtractActorURI(followable)
 	if err != nil {
 		return nil, errors.New("error extracting actor property from follow")
 	}
@@ -429,7 +430,7 @@ func (c *converter) ASFollowToFollow(ctx context.Context, followable ap.Followab
 		return nil, fmt.Errorf("error extracting account with uri %s from the database: %s", origin.String(), err)
 	}
 
-	target, err := ap.ExtractObject(followable)
+	target, err := ap.ExtractObjectURI(followable)
 	if err != nil {
 		return nil, errors.New("error extracting object property from follow")
 	}
@@ -454,7 +455,7 @@ func (c *converter) ASLikeToFave(ctx context.Context, likeable ap.Likeable) (*gt
 	}
 	uri := idProp.GetIRI().String()
 
-	origin, err := ap.ExtractActor(likeable)
+	origin, err := ap.ExtractActorURI(likeable)
 	if err != nil {
 		return nil, errors.New("error extracting actor property from like")
 	}
@@ -463,7 +464,7 @@ func (c *converter) ASLikeToFave(ctx context.Context, likeable ap.Likeable) (*gt
 		return nil, fmt.Errorf("error extracting account with uri %s from the database: %s", origin.String(), err)
 	}
 
-	target, err := ap.ExtractObject(likeable)
+	target, err := ap.ExtractObjectURI(likeable)
 	if err != nil {
 		return nil, errors.New("error extracting object property from like")
 	}
@@ -502,7 +503,7 @@ func (c *converter) ASBlockToBlock(ctx context.Context, blockable ap.Blockable)
 	}
 	uri := idProp.GetIRI().String()
 
-	origin, err := ap.ExtractActor(blockable)
+	origin, err := ap.ExtractActorURI(blockable)
 	if err != nil {
 		return nil, errors.New("ASBlockToBlock: error extracting actor property from block")
 	}
@@ -511,7 +512,7 @@ func (c *converter) ASBlockToBlock(ctx context.Context, blockable ap.Blockable)
 		return nil, fmt.Errorf("error extracting account with uri %s from the database: %s", origin.String(), err)
 	}
 
-	target, err := ap.ExtractObject(blockable)
+	target, err := ap.ExtractObjectURI(blockable)
 	if err != nil {
 		return nil, errors.New("ASBlockToBlock: error extracting object property from block")
 	}
@@ -530,72 +531,113 @@ func (c *converter) ASBlockToBlock(ctx context.Context, blockable ap.Blockable)
 	}, nil
 }
 
+// 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) {
-	status := &gtsmodel.Status{}
-	isNew := true
+	// Ensure item has an ID URI set.
+	_, statusURIStr, err := getURI(announceable)
+	if err != nil {
+		err = gtserror.Newf("error extracting URI: %w", err)
+		return nil, false, err
+	}
 
-	// check if we already have the boost in the database
-	idProp := announceable.GetJSONLDId()
-	if idProp == nil || !idProp.IsIRI() {
-		return nil, isNew, errors.New("no id property set on announce, or was not an iri")
+	var (
+		status *gtsmodel.Status
+		isNew  bool
+	)
+
+	// Check if we already have this boost in the database.
+	status, err = c.db.GetStatusByURI(ctx, statusURIStr)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		// Real database error.
+		err = gtserror.Newf("db error trying to get status with uri %s: %w", statusURIStr, err)
+		return nil, isNew, err
 	}
-	uri := idProp.GetIRI().String()
 
-	if status, err := c.db.GetStatusByURI(ctx, uri); err == nil {
-		// we already have it, great, just return it as-is :)
-		isNew = false
+	if status != nil {
+		// We already have this status,
+		// no need to proceed further.
 		return status, isNew, nil
 	}
-	status.URI = uri
 
-	// get the URI of the announced/boosted status
-	boostedStatusURI, err := ap.ExtractObject(announceable)
+	// If we reach here, we're dealing
+	// with a boost we haven't seen before.
+	isNew = true
+
+	// Start assembling the new status
+	// (we already know the URI).
+	status = new(gtsmodel.Status)
+	status.URI = statusURIStr
+
+	// Get the URI of the boosted status.
+	boostOfURI, err := ap.ExtractObjectURI(announceable)
 	if err != nil {
-		return nil, isNew, fmt.Errorf("ASAnnounceToStatus: error getting object from announce: %s", err)
+		err = gtserror.Newf("error extracting Object: %w", err)
+		return nil, isNew, err
 	}
 
-	// set the URI on the new status for dereferencing later
-	status.BoostOf = &gtsmodel.Status{
-		URI: boostedStatusURI.String(),
+	// Set the URI of the boosted status on
+	// the new status, for later dereferencing.
+	boostOf := &gtsmodel.Status{
+		URI: boostOfURI.String(),
 	}
+	status.BoostOf = boostOf
 
-	// get the published time for the announce
+	// Extract published time for the boost.
 	published, err := ap.ExtractPublished(announceable)
 	if err != nil {
-		return nil, isNew, fmt.Errorf("ASAnnounceToStatus: error extracting published time: %s", err)
+		err = gtserror.Newf("error extracting published: %w", err)
+		return nil, isNew, err
 	}
 	status.CreatedAt = published
 	status.UpdatedAt = published
 
-	// get the actor's IRI (ie., the person who boosted the status)
-	actor, err := ap.ExtractActor(announceable)
+	// Extract URI of the boosting account.
+	accountURI, err := ap.ExtractActorURI(announceable)
 	if err != nil {
-		return nil, isNew, fmt.Errorf("ASAnnounceToStatus: error extracting actor: %s", err)
+		err = gtserror.Newf("error extracting Actor: %w", err)
+		return nil, isNew, err
 	}
+	accountURIStr := accountURI.String()
 
-	// get the boosting account based on the URI
-	// this should have been dereferenced already before we hit this point so we can confidently error out if we don't have it
-	boostingAccount, err := c.db.GetAccountByURI(ctx, actor.String())
+	// Try to get the boosting account based on the URI.
+	// This should have been dereferenced already before
+	// we hit this point so we can confidently error out
+	// if we don't have it.
+	account, err := c.db.GetAccountByURI(ctx, accountURIStr)
 	if err != nil {
-		return nil, isNew, fmt.Errorf("ASAnnounceToStatus: error in db fetching account with uri %s: %s", actor.String(), err)
+		err = gtserror.Newf("db error trying to get account with uri %s: %w", accountURIStr, err)
+		return nil, isNew, err
 	}
-	status.AccountID = boostingAccount.ID
-	status.AccountURI = boostingAccount.URI
-	status.Account = boostingAccount
-
-	// these will all be wrapped in the boosted status so set them empty here
-	status.AttachmentIDs = []string{}
-	status.TagIDs = []string{}
-	status.MentionIDs = []string{}
-	status.EmojiIDs = []string{}
+	status.AccountID = account.ID
+	status.AccountURI = account.URI
+	status.Account = account
 
-	visibility, err := ap.ExtractVisibility(announceable, boostingAccount.FollowersURI)
+	// Calculate intended visibility of the boost.
+	visibility, err := ap.ExtractVisibility(announceable, account.FollowersURI)
 	if err != nil {
-		return nil, isNew, fmt.Errorf("ASAnnounceToStatus: error extracting visibility: %s", err)
+		err = gtserror.Newf("error extracting visibility: %w", err)
+		return nil, isNew, err
 	}
 	status.Visibility = visibility
 
-	// the rest of the fields will be taken from the target status, but it's not our job to do the dereferencing here
+	// 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
 }
 
@@ -609,7 +651,7 @@ func (c *converter) ASFlagToReport(ctx context.Context, flaggable ap.Flaggable)
 
 	// Extract account that created the flag / report.
 	// This will usually be an instance actor.
-	actor, err := ap.ExtractActor(flaggable)
+	actor, err := ap.ExtractActorURI(flaggable)
 	if err != nil {
 		return nil, fmt.Errorf("ASFlagToReport: error extracting actor: %w", err)
 	}
@@ -637,7 +679,7 @@ func (c *converter) ASFlagToReport(ctx context.Context, flaggable ap.Flaggable)
 	// maybe some statuses.
 	//
 	// Throw away anything that's not relevant to us.
-	objects, err := ap.ExtractObjects(flaggable)
+	objects, err := ap.ExtractObjectURIs(flaggable)
 	if err != nil {
 		return nil, fmt.Errorf("ASFlagToReport: error extracting objects: %w", err)
 	}
diff --git a/internal/typeutils/util.go b/internal/typeutils/util.go
index bd6c33ee1..0100200dc 100644
--- a/internal/typeutils/util.go
+++ b/internal/typeutils/util.go
@@ -19,9 +19,11 @@ package typeutils
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 
+	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/regexes"
 )
@@ -82,3 +84,19 @@ func misskeyReportInlineURLs(content string) []*url.URL {
 	}
 	return urls
 }
+
+// getURI is a shortcut/util function for extracting
+// the JSONLDId URI of an Activity or Object.
+func getURI(withID ap.WithJSONLDId) (*url.URL, string, error) {
+	idProp := withID.GetJSONLDId()
+	if idProp == nil {
+		return nil, "", errors.New("id prop was nil")
+	}
+
+	if !idProp.IsIRI() {
+		return nil, "", errors.New("id prop was not an IRI")
+	}
+
+	id := idProp.Get()
+	return id, id.String(), nil
+}
diff --git a/internal/typeutils/wrap.go b/internal/typeutils/wrap.go
index c81bf14f1..71099cbee 100644
--- a/internal/typeutils/wrap.go
+++ b/internal/typeutils/wrap.go
@@ -1,13 +1,30 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
 package typeutils
 
 import (
-	"fmt"
 	"net/url"
 
 	"github.com/superseriousbusiness/activity/pub"
 	"github.com/superseriousbusiness/activity/streams"
 	"github.com/superseriousbusiness/activity/streams/vocab"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
+	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/uris"
@@ -19,7 +36,7 @@ func (c *converter) WrapPersonInUpdate(person vocab.ActivityStreamsPerson, origi
 	// set the actor
 	actorURI, err := url.Parse(originAccount.URI)
 	if err != nil {
-		return nil, fmt.Errorf("WrapPersonInUpdate: error parsing url %s: %s", originAccount.URI, err)
+		return nil, gtserror.Newf("error parsing url %s: %w", originAccount.URI, err)
 	}
 	actorProp := streams.NewActivityStreamsActorProperty()
 	actorProp.AppendIRI(actorURI)
@@ -35,7 +52,7 @@ func (c *converter) WrapPersonInUpdate(person vocab.ActivityStreamsPerson, origi
 	idString := uris.GenerateURIForUpdate(originAccount.Username, newID)
 	idURI, err := url.Parse(idString)
 	if err != nil {
-		return nil, fmt.Errorf("WrapPersonInUpdate: error parsing url %s: %s", idString, err)
+		return nil, gtserror.Newf("error parsing url %s: %w", idString, err)
 	}
 	idProp := streams.NewJSONLDIdProperty()
 	idProp.SetIRI(idURI)
@@ -49,7 +66,7 @@ func (c *converter) WrapPersonInUpdate(person vocab.ActivityStreamsPerson, origi
 	// to should be public
 	toURI, err := url.Parse(pub.PublicActivityPubIRI)
 	if err != nil {
-		return nil, fmt.Errorf("WrapPersonInUpdate: error parsing url %s: %s", pub.PublicActivityPubIRI, err)
+		return nil, gtserror.Newf("error parsing url %s: %w", pub.PublicActivityPubIRI, err)
 	}
 	toProp := streams.NewActivityStreamsToProperty()
 	toProp.AppendIRI(toURI)
@@ -58,7 +75,7 @@ func (c *converter) WrapPersonInUpdate(person vocab.ActivityStreamsPerson, origi
 	// bcc followers
 	followersURI, err := url.Parse(originAccount.FollowersURI)
 	if err != nil {
-		return nil, fmt.Errorf("WrapPersonInUpdate: error parsing url %s: %s", originAccount.FollowersURI, err)
+		return nil, gtserror.Newf("error parsing url %s: %w", originAccount.FollowersURI, err)
 	}
 	bccProp := streams.NewActivityStreamsBccProperty()
 	bccProp.AppendIRI(followersURI)
@@ -81,7 +98,7 @@ func (c *converter) WrapNoteInCreate(note vocab.ActivityStreamsNote, objectIRIOn
 
 	// ID property
 	idProp := streams.NewJSONLDIdProperty()
-	createID := fmt.Sprintf("%s/activity", note.GetJSONLDId().GetIRI().String())
+	createID := note.GetJSONLDId().GetIRI().String() + "/activity"
 	createIDIRI, err := url.Parse(createID)
 	if err != nil {
 		return nil, err
@@ -91,9 +108,9 @@ func (c *converter) WrapNoteInCreate(note vocab.ActivityStreamsNote, objectIRIOn
 
 	// Actor Property
 	actorProp := streams.NewActivityStreamsActorProperty()
-	actorIRI, err := ap.ExtractAttributedTo(note)
+	actorIRI, err := ap.ExtractAttributedToURI(note)
 	if err != nil {
-		return nil, fmt.Errorf("WrapNoteInCreate: couldn't extract AttributedTo: %s", err)
+		return nil, gtserror.Newf("couldn't extract AttributedTo: %w", err)
 	}
 	actorProp.AppendIRI(actorIRI)
 	create.SetActivityStreamsActor(actorProp)
@@ -102,27 +119,25 @@ func (c *converter) WrapNoteInCreate(note vocab.ActivityStreamsNote, objectIRIOn
 	publishedProp := streams.NewActivityStreamsPublishedProperty()
 	published, err := ap.ExtractPublished(note)
 	if err != nil {
-		return nil, fmt.Errorf("WrapNoteInCreate: couldn't extract Published: %s", err)
+		return nil, gtserror.Newf("couldn't extract Published: %w", err)
 	}
 	publishedProp.Set(published)
 	create.SetActivityStreamsPublished(publishedProp)
 
 	// To Property
 	toProp := streams.NewActivityStreamsToProperty()
-	tos, err := ap.ExtractTos(note)
-	if err == nil {
-		for _, to := range tos {
-			toProp.AppendIRI(to)
+	if toURIs := ap.ExtractToURIs(note); len(toURIs) != 0 {
+		for _, toURI := range toURIs {
+			toProp.AppendIRI(toURI)
 		}
 		create.SetActivityStreamsTo(toProp)
 	}
 
 	// Cc Property
 	ccProp := streams.NewActivityStreamsCcProperty()
-	ccs, err := ap.ExtractCCs(note)
-	if err == nil {
-		for _, cc := range ccs {
-			ccProp.AppendIRI(cc)
+	if ccURIs := ap.ExtractCcURIs(note); len(ccURIs) != 0 {
+		for _, ccURI := range ccURIs {
+			ccProp.AppendIRI(ccURI)
 		}
 		create.SetActivityStreamsCc(ccProp)
 	}