From 8d2a76c58ce018fb6cd6760a3607cf1ee720037a Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 5 Apr 2023 20:10:05 +0200 Subject: [PATCH] [bugfix] Add proper constraints on status faves, dedupe (#1674) * [bugfix] Start working on multiple like issue * finish up --- ...05130021_status_fave_unique_constraints.go | 166 ++++++++++++++ internal/federation/federatingdb/create.go | 12 +- internal/federation/federatingdb/owns.go | 66 ++++-- internal/federation/federatingdb/undo.go | 202 +++++++++++++----- internal/federation/federatingdb/util.go | 18 +- internal/gtsmodel/statusfave.go | 20 +- 6 files changed, 387 insertions(+), 97 deletions(-) create mode 100644 internal/db/bundb/migrations/20230405130021_status_fave_unique_constraints.go diff --git a/internal/db/bundb/migrations/20230405130021_status_fave_unique_constraints.go b/internal/db/bundb/migrations/20230405130021_status_fave_unique_constraints.go new file mode 100644 index 000000000..c1d4ef2ac --- /dev/null +++ b/internal/db/bundb/migrations/20230405130021_status_fave_unique_constraints.go @@ -0,0 +1,166 @@ +// 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 . + +package migrations + +import ( + "context" + + "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/uptrace/bun" +) + +func init() { + up := func(ctx context.Context, db *bun.DB) error { + // To update fave constraints, we need to migrate faves into a new table. + // See section 7 here: https://www.sqlite.org/lang_altertable.html + + return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error { + // Remove any duplicate faves that were created before constraints. + // We need to ensure that we don't just delete all faves that are + // duplicates--we should keep the original, non-duplicate fave. + // So, produced query will look something like this: + // + // DELETE FROM "status_faves" + // WHERE id IN ( + // WITH cte AS ( + // SELECT + // "id", + // ROW_NUMBER() OVER( + // PARTITION BY "account_id", "status_id" + // ORDER BY "account_id", "status_id" + // ) AS "row_number" + // FROM status_faves + // ) + // SELECT "id" FROM cte + // WHERE "row_number" > 1 + // ) + // + // The above query only deletes status_faves with ids that are + // in the subquery. The subquery selects the IDs of all duplicate + // status faves past the first one, where 'duplicate' means 'has + // the same account id and status id'. + overQ := tx.NewRaw( + "PARTITION BY ?, ? ORDER BY ?, ?", + bun.Ident("account_id"), + bun.Ident("status_id"), + bun.Ident("account_id"), + bun.Ident("status_id"), + ) + + rowNumberQ := tx.NewRaw( + "SELECT ?, ROW_NUMBER() OVER(?) AS ? FROM status_faves", + bun.Ident("id"), + overQ, + bun.Ident("row_number"), + ) + + inQ := tx.NewRaw( + "WITH cte AS (?) SELECT ? FROM cte WHERE ? > 1", + rowNumberQ, + bun.Ident("id"), + bun.Ident("row_number"), + ) + + if _, err := tx. + NewDelete(). + Table("status_faves"). + Where("id IN (?)", inQ). + Exec(ctx); err != nil { + return err + } + + // Create the new faves table. + if _, err := tx. + NewCreateTable(). + ModelTableExpr("new_status_faves"). + Model(>smodel.StatusFave{}). + Exec(ctx); err != nil { + return err + } + + // Specify columns explicitly to + // avoid any Postgres shenanigans. + columns := []string{ + "id", + "created_at", + "updated_at", + "account_id", + "target_account_id", + "status_id", + "uri", + } + + // Copy remaining faves to the new table. + if _, err := tx. + NewInsert(). + Table("new_status_faves"). + Table("status_faves"). + Column(columns...). + Exec(ctx); err != nil { + return err + } + + // Drop the old table. + if _, err := tx. + NewDropTable(). + Table("status_faves"). + Exec(ctx); err != nil { + return err + } + + // Rename new table to old table. + if _, err := tx. + ExecContext( + ctx, + "ALTER TABLE ? RENAME TO ?", + bun.Ident("new_status_faves"), + bun.Ident("status_faves"), + ); err != nil { + return err + } + + // Add indexes to the new table. + for index, columns := range map[string][]string{ + "status_faves_id_idx": {"id"}, + "status_faves_account_id_idx": {"account_id"}, + "status_faves_status_id_idx": {"status_id"}, + } { + if _, err := tx. + NewCreateIndex(). + Table("status_faves"). + Index(index). + Column(columns...). + Exec(ctx); err != nil { + return err + } + } + + return nil + }) + } + + down := func(ctx context.Context, db *bun.DB) error { + return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error { + return nil + }) + } + + if err := Migrations.Register(up, down); err != nil { + panic(err) + } +} diff --git a/internal/federation/federatingdb/create.go b/internal/federation/federatingdb/create.go index 166ea34a9..b14a0597b 100644 --- a/internal/federation/federatingdb/create.go +++ b/internal/federation/federatingdb/create.go @@ -288,13 +288,19 @@ func (f *federatingDB) activityLike(ctx context.Context, asType vocab.Type, rece fave, err := f.typeConverter.ASLikeToFave(ctx, like) if err != nil { - return fmt.Errorf("activityLike: could not convert Like to fave: %s", err) + return fmt.Errorf("activityLike: could not convert Like to fave: %w", err) } fave.ID = id.NewULID() - if err := f.state.DB.Put(ctx, fave); err != nil { - return fmt.Errorf("activityLike: database error inserting fave: %s", err) + if err := f.state.DB.PutStatusFave(ctx, fave); err != nil { + if errors.Is(err, db.ErrAlreadyExists) { + // The Like already exists in the database, which + // means we've already handled side effects. We can + // just return nil here and be done with it. + return nil + } + return fmt.Errorf("activityLike: database error inserting fave: %w", err) } f.state.Workers.EnqueueFederator(ctx, messages.FromFederator{ diff --git a/internal/federation/federatingdb/owns.go b/internal/federation/federatingdb/owns.go index 62cb7a3ea..cc3bb7d5d 100644 --- a/internal/federation/federatingdb/owns.go +++ b/internal/federation/federatingdb/owns.go @@ -19,12 +19,14 @@ import ( "context" + "errors" "fmt" "net/url" "codeberg.org/gruf/go-kv" "github.com/superseriousbusiness/gotosocial/internal/config" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/uris" @@ -46,6 +48,11 @@ func (f *federatingDB) Owns(ctx context.Context, id *url.URL) (bool, error) { return false, nil } + // todo: refactor the below; make sure we use + // proper db functions for everything, and + // preferably clean up by calling subfuncs + // (like we now do for ownsLike). + // apparently it belongs to this host, so what *is* it? // check if it's a status, eg /users/example_username/statuses/SOME_UUID_OF_A_STATUS if uris.IsStatusesPath(id) { @@ -117,28 +124,7 @@ func (f *federatingDB) Owns(ctx context.Context, id *url.URL) (bool, error) { } if uris.IsLikePath(id) { - username, likeID, err := uris.ParseLikedPath(id) - if err != nil { - return false, fmt.Errorf("error parsing like path for url %s: %s", id.String(), err) - } - if _, err := f.state.DB.GetAccountByUsernameDomain(ctx, username, ""); err != nil { - if err == db.ErrNoEntries { - // there are no entries for this username - return false, nil - } - // an actual error happened - return false, fmt.Errorf("database error fetching account with username %s: %s", username, err) - } - if err := f.state.DB.GetByID(ctx, likeID, >smodel.StatusFave{}); err != nil { - if err == db.ErrNoEntries { - // there are no entries - return false, nil - } - // an actual error happened - return false, fmt.Errorf("database error fetching like with id %s: %s", likeID, err) - } - l.Debugf("we own url %s", id.String()) - return true, nil + return f.ownsLike(ctx, id) } if uris.IsBlockPath(id) { @@ -168,3 +154,39 @@ func (f *federatingDB) Owns(ctx context.Context, id *url.URL) (bool, error) { return false, fmt.Errorf("could not match activityID: %s", id.String()) } + +func (f *federatingDB) ownsLike(ctx context.Context, uri *url.URL) (bool, error) { + username, id, err := uris.ParseLikedPath(uri) + if err != nil { + return false, fmt.Errorf("error parsing Like path for url %s: %w", uri.String(), err) + } + + // We're only checking for existence, + // so use barebones context. + bbCtx := gtscontext.SetBarebones(ctx) + + if _, err := f.state.DB.GetAccountByUsernameDomain(bbCtx, username, ""); err != nil { + if errors.Is(err, db.ErrNoEntries) { + // No entries for this acct, + // we don't own this item. + return false, nil + } + + // Actual error. + return false, fmt.Errorf("database error fetching account with username %s: %w", username, err) + } + + if _, err := f.state.DB.GetStatusFaveByID(bbCtx, id); err != nil { + if errors.Is(err, db.ErrNoEntries) { + // No entries for this ID, + // we don't own this item. + return false, nil + } + + // Actual error. + return false, fmt.Errorf("database error fetching status fave with id %s: %w", id, err) + } + + log.Tracef(ctx, "we own Like %s", uri.String()) + return true, nil +} diff --git a/internal/federation/federatingdb/undo.go b/internal/federation/federatingdb/undo.go index 517aa9cc6..c17bd2e90 100644 --- a/internal/federation/federatingdb/undo.go +++ b/internal/federation/federatingdb/undo.go @@ -26,11 +26,13 @@ "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtscontext" + "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" ) func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) error { - l := log.Entry{}.WithContext(ctx) + l := log.WithContext(ctx) if log.Level() >= level.DEBUG { i, err := marshalItem(undo) @@ -55,70 +57,160 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) } for iter := undoObject.Begin(); iter != undoObject.End(); iter = iter.Next() { - if iter.GetType() == nil { + t := iter.GetType() + if t == nil { continue } - switch iter.GetType().GetTypeName() { + + switch t.GetTypeName() { case ap.ActivityFollow: - // UNDO FOLLOW - ASFollow, ok := iter.GetType().(vocab.ActivityStreamsFollow) - if !ok { - return errors.New("UNDO: couldn't parse follow into vocab.ActivityStreamsFollow") + if err := f.undoFollow(ctx, receivingAccount, undo, t); err != nil { + return err } - // make sure the actor owns the follow - if !sameActor(undo.GetActivityStreamsActor(), ASFollow.GetActivityStreamsActor()) { - return errors.New("UNDO: follow actor and activity actor not the same") - } - // convert the follow to something we can understand - gtsFollow, err := f.typeConverter.ASFollowToFollow(ctx, ASFollow) - if err != nil { - return fmt.Errorf("UNDO: error converting asfollow to gtsfollow: %s", err) - } - // make sure the addressee of the original follow is the same as whatever inbox this landed in - if gtsFollow.TargetAccountID != receivingAccount.ID { - return errors.New("UNDO: follow object account and inbox account were not the same") - } - // delete any existing FOLLOW - if err := f.state.DB.DeleteFollowByURI(ctx, gtsFollow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { - return fmt.Errorf("UNDO: db error removing follow: %s", err) - } - // delete any existing FOLLOW REQUEST - if err := f.state.DB.DeleteFollowRequestByURI(ctx, gtsFollow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { - return fmt.Errorf("UNDO: db error removing follow request: %s", err) - } - l.Debug("follow undone") - return nil case ap.ActivityLike: - // UNDO LIKE + if err := f.undoLike(ctx, receivingAccount, undo, t); err != nil { + return err + } case ap.ActivityAnnounce: - // UNDO BOOST/REBLOG/ANNOUNCE + // todo: undo boost / reblog / announce case ap.ActivityBlock: - // UNDO BLOCK - ASBlock, ok := iter.GetType().(vocab.ActivityStreamsBlock) - if !ok { - return errors.New("UNDO: couldn't parse block into vocab.ActivityStreamsBlock") + if err := f.undoBlock(ctx, receivingAccount, undo, t); err != nil { + return err } - // make sure the actor owns the follow - if !sameActor(undo.GetActivityStreamsActor(), ASBlock.GetActivityStreamsActor()) { - return errors.New("UNDO: block actor and activity actor not the same") - } - // convert the block to something we can understand - gtsBlock, err := f.typeConverter.ASBlockToBlock(ctx, ASBlock) - if err != nil { - return fmt.Errorf("UNDO: error converting asblock to gtsblock: %s", err) - } - // make sure the addressee of the original block is the same as whatever inbox this landed in - if gtsBlock.TargetAccountID != receivingAccount.ID { - return errors.New("UNDO: block object account and inbox account were not the same") - } - // delete any existing BLOCK - if err := f.state.DB.DeleteBlockByURI(ctx, gtsBlock.URI); err != nil { - return fmt.Errorf("UNDO: db error removing block: %s", err) - } - l.Debug("block undone") - return nil } } return nil } + +func (f *federatingDB) undoFollow( + ctx context.Context, + receivingAccount *gtsmodel.Account, + undo vocab.ActivityStreamsUndo, + t vocab.Type, +) error { + Follow, ok := t.(vocab.ActivityStreamsFollow) + if !ok { + return errors.New("undoFollow: couldn't parse vocab.Type into vocab.ActivityStreamsFollow") + } + + // Make sure the undo actor owns the target. + if !sameActor(undo.GetActivityStreamsActor(), Follow.GetActivityStreamsActor()) { + // Ignore this Activity. + return nil + } + + follow, err := f.typeConverter.ASFollowToFollow(ctx, Follow) + if err != nil { + return fmt.Errorf("undoFollow: error converting ActivityStreams Follow to follow: %w", err) + } + + // Ensure addressee is follow target. + if follow.TargetAccountID != receivingAccount.ID { + // Ignore this Activity. + return nil + } + + // Delete any existing follow with this URI. + if err := f.state.DB.DeleteFollowByURI(ctx, follow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { + return fmt.Errorf("undoFollow: db error removing follow: %w", err) + } + + // Delete any existing follow request with this URI. + if err := f.state.DB.DeleteFollowRequestByURI(ctx, follow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { + return fmt.Errorf("undoFollow: db error removing follow request: %w", err) + } + + log.Debug(ctx, "Follow undone") + return nil +} + +func (f *federatingDB) undoLike( + ctx context.Context, + receivingAccount *gtsmodel.Account, + undo vocab.ActivityStreamsUndo, + t vocab.Type, +) error { + Like, ok := t.(vocab.ActivityStreamsLike) + if !ok { + return errors.New("undoLike: couldn't parse vocab.Type into vocab.ActivityStreamsLike") + } + + // Make sure the undo actor owns the target. + if !sameActor(undo.GetActivityStreamsActor(), Like.GetActivityStreamsActor()) { + // Ignore this Activity. + return nil + } + + fave, err := f.typeConverter.ASLikeToFave(ctx, Like) + if err != nil { + return fmt.Errorf("undoLike: error converting ActivityStreams Like to fave: %w", err) + } + + // Ensure addressee is fave target. + if fave.TargetAccountID != receivingAccount.ID { + // Ignore this Activity. + return nil + } + + // Ignore URI on Likes, since we often get multiple Likes + // with the same target and account ID, but differing URIs. + // Instead, we'll select using account and target status. + // Regardless of the URI, we can read an Undo Like to mean + // "I don't want to fave this post anymore". + fave, err = f.state.DB.GetStatusFave(gtscontext.SetBarebones(ctx), fave.AccountID, fave.StatusID) + if err != nil { + if errors.Is(err, db.ErrNoEntries) { + // We didn't have a like/fave + // for this combo anyway, ignore. + return nil + } + // Real error. + return fmt.Errorf("undoLike: db error getting fave from %s targeting %s: %w", fave.AccountID, fave.StatusID, err) + } + + // Delete the status fave. + if err := f.state.DB.DeleteStatusFaveByID(ctx, fave.ID); err != nil { + return fmt.Errorf("undoLike: db error deleting fave %s: %w", fave.ID, err) + } + + log.Debug(ctx, "Like undone") + return nil +} + +func (f *federatingDB) undoBlock( + ctx context.Context, + receivingAccount *gtsmodel.Account, + undo vocab.ActivityStreamsUndo, + t vocab.Type, +) error { + Block, ok := t.(vocab.ActivityStreamsBlock) + if !ok { + return errors.New("undoBlock: couldn't parse vocab.Type into vocab.ActivityStreamsBlock") + } + + // Make sure the undo actor owns the target. + if !sameActor(undo.GetActivityStreamsActor(), Block.GetActivityStreamsActor()) { + // Ignore this Activity. + return nil + } + + block, err := f.typeConverter.ASBlockToBlock(ctx, Block) + if err != nil { + return fmt.Errorf("undoBlock: error converting ActivityStreams Block to block: %w", err) + } + + // Ensure addressee is block target. + if block.TargetAccountID != receivingAccount.ID { + // Ignore this Activity. + return nil + } + + // Delete any existing BLOCK + if err := f.state.DB.DeleteBlockByURI(ctx, block.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { + return fmt.Errorf("undoBlock: db error removing block: %w", err) + } + + log.Debug(ctx, "Block undone") + return nil +} diff --git a/internal/federation/federatingdb/util.go b/internal/federation/federatingdb/util.go index 8ad209f5e..5137145f2 100644 --- a/internal/federation/federatingdb/util.go +++ b/internal/federation/federatingdb/util.go @@ -36,23 +36,27 @@ "github.com/superseriousbusiness/gotosocial/internal/uris" ) -func sameActor(activityActor vocab.ActivityStreamsActorProperty, followActor vocab.ActivityStreamsActorProperty) bool { - if activityActor == nil || followActor == nil { +func sameActor(actor1 vocab.ActivityStreamsActorProperty, actor2 vocab.ActivityStreamsActorProperty) bool { + if actor1 == nil || actor2 == nil { return false } - for aIter := activityActor.Begin(); aIter != activityActor.End(); aIter = aIter.Next() { - for fIter := followActor.Begin(); fIter != followActor.End(); fIter = fIter.Next() { - if aIter.GetIRI() == nil { + + for a1Iter := actor1.Begin(); a1Iter != actor1.End(); a1Iter = a1Iter.Next() { + for a2Iter := actor2.Begin(); a2Iter != actor2.End(); a2Iter = a2Iter.Next() { + if a1Iter.GetIRI() == nil { return false } - if fIter.GetIRI() == nil { + + if a2Iter.GetIRI() == nil { return false } - if aIter.GetIRI().String() == fIter.GetIRI().String() { + + if a1Iter.GetIRI().String() == a2Iter.GetIRI().String() { return true } } } + return false } diff --git a/internal/gtsmodel/statusfave.go b/internal/gtsmodel/statusfave.go index dc35a7d63..86096ef5e 100644 --- a/internal/gtsmodel/statusfave.go +++ b/internal/gtsmodel/statusfave.go @@ -21,14 +21,14 @@ // StatusFave refers to a 'fave' or 'like' in the database, from one account, targeting the status of another account type StatusFave struct { - ID string `validate:"required,ulid" bun:"type:CHAR(26),pk,nullzero,notnull,unique"` // id of this item in the database - CreatedAt time.Time `validate:"-" bun:"type:timestamptz,nullzero,notnull,default:current_timestamp"` // when was item created - UpdatedAt time.Time `validate:"-" bun:"type:timestamptz,nullzero,notnull,default:current_timestamp"` // when was item last updated - AccountID string `validate:"required,ulid" bun:"type:CHAR(26),nullzero,notnull"` // id of the account that created ('did') the fave - Account *Account `validate:"-" bun:"rel:belongs-to"` // account that created the fave - TargetAccountID string `validate:"required,ulid" bun:"type:CHAR(26),nullzero,notnull"` // id the account owning the faved status - TargetAccount *Account `validate:"-" bun:"rel:belongs-to"` // account owning the faved status - StatusID string `validate:"required,ulid" bun:"type:CHAR(26),nullzero,notnull"` // database id of the status that has been 'faved' - Status *Status `validate:"-" bun:"rel:belongs-to"` // the faved status - URI string `validate:"required,url" bun:",nullzero,notnull"` // ActivityPub URI of this fave + ID string `validate:"required,ulid" bun:"type:CHAR(26),pk,nullzero,notnull,unique"` // id of this item in the database + CreatedAt time.Time `validate:"-" bun:"type:timestamptz,nullzero,notnull,default:current_timestamp"` // when was item created + UpdatedAt time.Time `validate:"-" bun:"type:timestamptz,nullzero,notnull,default:current_timestamp"` // when was item last updated + AccountID string `validate:"required,ulid" bun:"type:CHAR(26),unique:statusfaveaccountstatus,nullzero,notnull"` // id of the account that created ('did') the fave + Account *Account `validate:"-" bun:"-"` // account that created the fave + TargetAccountID string `validate:"required,ulid" bun:"type:CHAR(26),nullzero,notnull"` // id the account owning the faved status + TargetAccount *Account `validate:"-" bun:"-"` // account owning the faved status + StatusID string `validate:"required,ulid" bun:"type:CHAR(26),unique:statusfaveaccountstatus,nullzero,notnull"` // database id of the status that has been 'faved' + Status *Status `validate:"-" bun:"-"` // the faved status + URI string `validate:"required,url" bun:",nullzero,notnull,unique"` // ActivityPub URI of this fave }