From 1951e6c84085fae11d4f0699b3b268df273cef02 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 12 Jul 2023 13:20:15 +0200 Subject: [PATCH] [bugfix] Update account `Update` logic (#1984) --- internal/federation/dereferencing/account.go | 106 +++++++++++-------- internal/federation/federatingdb/update.go | 43 ++++---- internal/processing/fromfederator.go | 20 ++-- 3 files changed, 94 insertions(+), 75 deletions(-) diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go index f7e740d4..ec8d274a 100644 --- a/internal/federation/dereferencing/account.go +++ b/internal/federation/dereferencing/account.go @@ -472,34 +472,45 @@ func (d *deref) enrichAccount(ctx context.Context, requestUser string, uri *url. return latestAcc, apubAcc, nil } -func (d *deref) fetchRemoteAccountAvatar(ctx context.Context, tsport transport.Transport, existing, account *gtsmodel.Account) error { - if account.AvatarRemoteURL == "" { - // No fetching to do. +func (d *deref) fetchRemoteAccountAvatar(ctx context.Context, tsport transport.Transport, existing, latestAcc *gtsmodel.Account) error { + if latestAcc.AvatarRemoteURL == "" { + // No avatar set on newest model, leave + // latest avatar attachment ID empty. return nil } - // By default we set the original media attachment ID. - account.AvatarMediaAttachmentID = existing.AvatarMediaAttachmentID + // By default we keep the previous media attachment ID. This will only + // be changed if and when we have the new media loaded into storage. + latestAcc.AvatarMediaAttachmentID = existing.AvatarMediaAttachmentID - if account.AvatarMediaAttachmentID != "" && - existing.AvatarRemoteURL == account.AvatarRemoteURL { - // Look for an existing media attachment by the known ID. + // If we had a media attachment ID already, and the URL + // of the attachment hasn't changed from existing -> latest, + // then we may be able to just keep our existing attachment + // without having to make any remote calls. + if latestAcc.AvatarMediaAttachmentID != "" && + existing.AvatarRemoteURL == latestAcc.AvatarRemoteURL { + + // Ensure we have media attachment with the known ID. media, err := d.state.DB.GetAttachmentByID(ctx, existing.AvatarMediaAttachmentID) if err != nil && !errors.Is(err, db.ErrNoEntries) { return gtserror.Newf("error getting attachment %s: %w", existing.AvatarMediaAttachmentID, err) } - if media != nil && *media.Cached { - // Media already cached, - // use this existing. + // Ensure attachment has correct properties. + if media != nil && media.RemoteURL == latestAcc.AvatarRemoteURL { + // We already have the most up-to-date + // media attachment, keep using it. return nil } } + // If we reach here, we know we need to fetch the most + // up-to-date version of the attachment from remote. + // Parse and validate the newly provided media URL. - avatarURI, err := url.Parse(account.AvatarRemoteURL) + avatarURI, err := url.Parse(latestAcc.AvatarRemoteURL) if err != nil { - return gtserror.Newf("error parsing url %s: %w", account.AvatarRemoteURL, err) + return gtserror.Newf("error parsing url %s: %w", latestAcc.AvatarRemoteURL, err) } // Acquire lock for derefs map. @@ -507,7 +518,7 @@ func (d *deref) fetchRemoteAccountAvatar(ctx context.Context, tsport transport.T defer unlock() // Look for an existing dereference in progress. - processing, ok := d.derefAvatars[account.AvatarRemoteURL] + processing, ok := d.derefAvatars[latestAcc.AvatarRemoteURL] if !ok { var err error @@ -518,21 +529,21 @@ func (d *deref) fetchRemoteAccountAvatar(ctx context.Context, tsport transport.T } // Create new media processing request from the media manager instance. - processing, err = d.mediaManager.PreProcessMedia(ctx, data, account.ID, &media.AdditionalMediaInfo{ + processing, err = d.mediaManager.PreProcessMedia(ctx, data, latestAcc.ID, &media.AdditionalMediaInfo{ Avatar: func() *bool { v := true; return &v }(), - RemoteURL: &account.AvatarRemoteURL, + RemoteURL: &latestAcc.AvatarRemoteURL, }) if err != nil { - return gtserror.Newf("error preprocessing media for attachment %s: %w", account.AvatarRemoteURL, err) + return gtserror.Newf("error preprocessing media for attachment %s: %w", latestAcc.AvatarRemoteURL, err) } // Store media in map to mark as processing. - d.derefAvatars[account.AvatarRemoteURL] = processing + d.derefAvatars[latestAcc.AvatarRemoteURL] = processing defer func() { // On exit safely remove media from map. unlock := d.derefAvatarsMu.Lock() - delete(d.derefAvatars, account.AvatarRemoteURL) + delete(d.derefAvatars, latestAcc.AvatarRemoteURL) unlock() }() } @@ -542,43 +553,54 @@ func (d *deref) fetchRemoteAccountAvatar(ctx context.Context, tsport transport.T // Start media attachment loading (blocking call). if _, err := processing.LoadAttachment(ctx); err != nil { - return gtserror.Newf("error loading attachment %s: %w", account.AvatarRemoteURL, err) + return gtserror.Newf("error loading attachment %s: %w", latestAcc.AvatarRemoteURL, err) } // Set the newly loaded avatar media attachment ID. - account.AvatarMediaAttachmentID = processing.AttachmentID() + latestAcc.AvatarMediaAttachmentID = processing.AttachmentID() return nil } -func (d *deref) fetchRemoteAccountHeader(ctx context.Context, tsport transport.Transport, existing, account *gtsmodel.Account) error { - if account.HeaderRemoteURL == "" { - // No fetching to do. +func (d *deref) fetchRemoteAccountHeader(ctx context.Context, tsport transport.Transport, existing, latestAcc *gtsmodel.Account) error { + if latestAcc.HeaderRemoteURL == "" { + // No header set on newest model, leave + // latest header attachment ID empty. return nil } - // By default we set the original media attachment ID. - account.HeaderMediaAttachmentID = existing.HeaderMediaAttachmentID + // By default we keep the previous media attachment ID. This will only + // be changed if and when we have the new media loaded into storage. + latestAcc.HeaderMediaAttachmentID = existing.HeaderMediaAttachmentID - if account.HeaderMediaAttachmentID != "" && - existing.HeaderRemoteURL == account.HeaderRemoteURL { - // Look for an existing media attachment by the known ID. + // If we had a media attachment ID already, and the URL + // of the attachment hasn't changed from existing -> latest, + // then we may be able to just keep our existing attachment + // without having to make any remote calls. + if latestAcc.HeaderMediaAttachmentID != "" && + existing.HeaderRemoteURL == latestAcc.HeaderRemoteURL { + + // Ensure we have media attachment with the known ID. media, err := d.state.DB.GetAttachmentByID(ctx, existing.HeaderMediaAttachmentID) if err != nil && !errors.Is(err, db.ErrNoEntries) { return gtserror.Newf("error getting attachment %s: %w", existing.HeaderMediaAttachmentID, err) } - if media != nil && *media.Cached { - // Media already cached, - // use this existing. + // Ensure attachment has correct properties. + if media != nil && media.RemoteURL == latestAcc.HeaderRemoteURL { + // We already have the most up-to-date + // media attachment, keep using it. return nil } } + // If we reach here, we know we need to fetch the most + // up-to-date version of the attachment from remote. + // Parse and validate the newly provided media URL. - headerURI, err := url.Parse(account.HeaderRemoteURL) + headerURI, err := url.Parse(latestAcc.HeaderRemoteURL) if err != nil { - return gtserror.Newf("error parsing url %s: %w", account.HeaderRemoteURL, err) + return gtserror.Newf("error parsing url %s: %w", latestAcc.HeaderRemoteURL, err) } // Acquire lock for derefs map. @@ -586,7 +608,7 @@ func (d *deref) fetchRemoteAccountHeader(ctx context.Context, tsport transport.T defer unlock() // Look for an existing dereference in progress. - processing, ok := d.derefHeaders[account.HeaderRemoteURL] + processing, ok := d.derefHeaders[latestAcc.HeaderRemoteURL] if !ok { var err error @@ -597,21 +619,21 @@ func (d *deref) fetchRemoteAccountHeader(ctx context.Context, tsport transport.T } // Create new media processing request from the media manager instance. - processing, err = d.mediaManager.PreProcessMedia(ctx, data, account.ID, &media.AdditionalMediaInfo{ + processing, err = d.mediaManager.PreProcessMedia(ctx, data, latestAcc.ID, &media.AdditionalMediaInfo{ Header: func() *bool { v := true; return &v }(), - RemoteURL: &account.HeaderRemoteURL, + RemoteURL: &latestAcc.HeaderRemoteURL, }) if err != nil { - return gtserror.Newf("error preprocessing media for attachment %s: %w", account.HeaderRemoteURL, err) + return gtserror.Newf("error preprocessing media for attachment %s: %w", latestAcc.HeaderRemoteURL, err) } // Store media in map to mark as processing. - d.derefHeaders[account.HeaderRemoteURL] = processing + d.derefHeaders[latestAcc.HeaderRemoteURL] = processing defer func() { // On exit safely remove media from map. unlock := d.derefHeadersMu.Lock() - delete(d.derefHeaders, account.HeaderRemoteURL) + delete(d.derefHeaders, latestAcc.HeaderRemoteURL) unlock() }() } @@ -621,11 +643,11 @@ func (d *deref) fetchRemoteAccountHeader(ctx context.Context, tsport transport.T // Start media attachment loading (blocking call). if _, err := processing.LoadAttachment(ctx); err != nil { - return gtserror.Newf("error loading attachment %s: %w", account.HeaderRemoteURL, err) + return gtserror.Newf("error loading attachment %s: %w", latestAcc.HeaderRemoteURL, err) } // Set the newly loaded avatar media attachment ID. - account.HeaderMediaAttachmentID = processing.AttachmentID() + latestAcc.HeaderMediaAttachmentID = processing.AttachmentID() return nil } diff --git a/internal/federation/federatingdb/update.go b/internal/federation/federatingdb/update.go index aad38608..5ac4cc28 100644 --- a/internal/federation/federatingdb/update.go +++ b/internal/federation/federatingdb/update.go @@ -19,13 +19,12 @@ package federatingdb import ( "context" - "errors" - "fmt" "codeberg.org/gruf/go-logger/v2/level" "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/messages" @@ -41,7 +40,7 @@ import ( // // The library makes this call only after acquiring a lock first. func (f *federatingDB) Update(ctx context.Context, asType vocab.Type) error { - l := log.Entry{}.WithContext(ctx) + l := log.WithContext(ctx) if log.Level() >= level.DEBUG { i, err := marshalItem(asType) @@ -66,40 +65,38 @@ func (f *federatingDB) Update(ctx context.Context, asType vocab.Type) error { } func (f *federatingDB) updateAccountable(ctx context.Context, receivingAcct *gtsmodel.Account, requestingAcct *gtsmodel.Account, asType vocab.Type) error { + // Ensure delivered asType is a valid Accountable model. accountable, ok := asType.(ap.Accountable) if !ok { - return errors.New("updateAccountable: could not convert vocab.Type to Accountable") + return gtserror.Newf("could not convert vocab.Type %T to Accountable", asType) } - updatedAcct, err := f.typeConverter.ASRepresentationToAccount(ctx, accountable, "") - if err != nil { - return fmt.Errorf("updateAccountable: error converting to account: %w", err) + // Extract AP URI of the updated Accountable model. + idProp := accountable.GetJSONLDId() + if idProp == nil || !idProp.IsIRI() { + return gtserror.New("Accountable id prop was nil or not IRI") } + updatedAcctURI := idProp.GetIRI() - if updatedAcct.Domain == config.GetHost() || updatedAcct.Domain == config.GetAccountDomain() { - // No need to update local accounts; in fact, if we try - // this it will break the shit out of things so do NOT. + // Don't try to update local accounts, it will break things. + if updatedAcctURI.Host == config.GetHost() { return nil } - if requestingAcct.URI != updatedAcct.URI { - return fmt.Errorf("updateAccountable: update for account %s was requested by account %s, this is not valid", updatedAcct.URI, requestingAcct.URI) + // Ensure Accountable and requesting account are one and the same. + if updatedAcctURIStr := updatedAcctURI.String(); requestingAcct.URI != updatedAcctURIStr { + return gtserror.Newf("update for %s was requested by %s, this is not valid", updatedAcctURIStr, requestingAcct.URI) } - // Set some basic fields on the updated account - // based on what we already know about the requester. - updatedAcct.CreatedAt = requestingAcct.CreatedAt - updatedAcct.ID = requestingAcct.ID - updatedAcct.Language = requestingAcct.Language - updatedAcct.AvatarMediaAttachmentID = requestingAcct.AvatarMediaAttachmentID - updatedAcct.HeaderMediaAttachmentID = requestingAcct.HeaderMediaAttachmentID - - // Pass to the processor for further updating of eg., avatar/header, - // emojis, etc. The actual db insert/update will take place there. + // Pass in to the processor the existing version of the requesting + // account that we have, plus the Accountable representation that + // was delivered along with the Update, for further asynchronous + // updating of eg., avatar/header, emojis, etc. The actual db + // inserts/updates will take place there. f.state.Workers.EnqueueFederator(ctx, messages.FromFederator{ APObjectType: ap.ObjectProfile, APActivityType: ap.ActivityUpdate, - GTSModel: updatedAcct, + GTSModel: requestingAcct, APObjectModel: accountable, ReceivingAccount: receivingAcct, }) diff --git a/internal/processing/fromfederator.go b/internal/processing/fromfederator.go index 52b12126..abe292ca 100644 --- a/internal/processing/fromfederator.go +++ b/internal/processing/fromfederator.go @@ -20,7 +20,6 @@ package processing import ( "context" "errors" - "fmt" "net/url" "codeberg.org/gruf/go-kv" @@ -422,27 +421,28 @@ func (p *Processor) processCreateFlagFromFederator(ctx context.Context, federato // processUpdateAccountFromFederator handles Activity Update and Object Profile func (p *Processor) processUpdateAccountFromFederator(ctx context.Context, federatorMsg messages.FromFederator) error { - incomingAccount, ok := federatorMsg.GTSModel.(*gtsmodel.Account) + // Parse the old/existing account model. + account, ok := federatorMsg.GTSModel.(*gtsmodel.Account) if !ok { - return errors.New("*gtsmodel.Account was not parseable on update account message") + return gtserror.New("account was not parseable as *gtsmodel.Account") } - // Because this was an Update, the new AP Object should be set on the message. - incomingAccountable, ok := federatorMsg.APObjectModel.(ap.Accountable) + // Because this was an Update, the new Accountable should be set on the message. + apubAcc, ok := federatorMsg.APObjectModel.(ap.Accountable) if !ok { - return errors.New("Accountable was not parseable on update account message") + return gtserror.New("Accountable was not parseable on update account message") } // Fetch up-to-date bio, avatar, header, etc. _, _, err := p.federator.RefreshAccount( ctx, federatorMsg.ReceivingAccount.Username, - incomingAccount, - incomingAccountable, - true, + account, + apubAcc, + true, // Force refresh. ) if err != nil { - return fmt.Errorf("error enriching updated account from federator: %s", err) + return gtserror.Newf("error refreshing updated account: %w", err) } return nil