[bugfix] Add proper constraints on status faves, dedupe (#1674)

* [bugfix] Start working on multiple like issue

* finish up

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 <http://www.gnu.org/licenses/>.
+
+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(&gtsmodel.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)
+	}
+}

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 {
+	if err := f.state.DB.PutStatusFave(ctx, fave); err != nil {
-		return fmt.Errorf("activityLike: database error inserting fave: %s", err)
+		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{

internal/federation/federatingdb/owns.go

@@ -19,12 +19,14 @@ package federatingdb
 
 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)
+		return f.ownsLike(ctx, 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, &gtsmodel.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
 	}
 
 	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
+}

internal/federation/federatingdb/undo.go

@@ -26,11 +26,13 @@ import (
 	"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
+			if err := f.undoFollow(ctx, receivingAccount, undo, t); err != nil {
-			ASFollow, ok := iter.GetType().(vocab.ActivityStreamsFollow)
+				return err
-			if !ok {
-				return errors.New("UNDO: couldn't parse follow into vocab.ActivityStreamsFollow")
 			}
-			// 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
+			if err := f.undoBlock(ctx, receivingAccount, undo, t); err != nil {
-			ASBlock, ok := iter.GetType().(vocab.ActivityStreamsBlock)
+				return err
-			if !ok {
-				return errors.New("UNDO: couldn't parse block into vocab.ActivityStreamsBlock")
 			}
-			// 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
+}

internal/federation/federatingdb/util.go

@@ -36,23 +36,27 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/uris"
 )
 
-func sameActor(activityActor vocab.ActivityStreamsActorProperty, followActor vocab.ActivityStreamsActorProperty) bool {
+func sameActor(actor1 vocab.ActivityStreamsActorProperty, actor2 vocab.ActivityStreamsActorProperty) bool {
-	if activityActor == nil || followActor == nil {
+	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() {
+	for a1Iter := actor1.Begin(); a1Iter != actor1.End(); a1Iter = a1Iter.Next() {
-			if aIter.GetIRI() == nil {
+		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
 }
 

internal/gtsmodel/statusfave.go

@@ -21,14 +21,14 @@ import "time"
 
 // 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
+	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
+	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
+	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
+	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:"rel:belongs-to"`                                              // account that created 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
+	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
+	TargetAccount   *Account  `validate:"-" bun:"-"`                                                                         // 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'
+	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:"rel:belongs-to"`                                              // the faved status
+	Status          *Status   `validate:"-" bun:"-"`                                                                         // the faved status
-	URI             string    `validate:"required,url" bun:",nullzero,notnull"`                                // ActivityPub URI of this fave
+	URI             string    `validate:"required,url" bun:",nullzero,notnull,unique"`                                       // ActivityPub URI of this fave
 }