[bugfix] Fix deleted status causing issues when getting bookmark (#1551)

* [bugfix] Delete bookmark when status deleted

* [chore] Give bookmark processing func some love

* fix paging + embetter tests

internal/api/client/bookmarks/bookmarks_test.go

@@ -19,19 +19,21 @@
 package bookmarks_test
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
-	"strings"
+	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
 	"github.com/superseriousbusiness/gotosocial/internal/api/client/bookmarks"
 	"github.com/superseriousbusiness/gotosocial/internal/api/client/statuses"
-	"github.com/superseriousbusiness/gotosocial/internal/api/model"
+	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
 	"github.com/superseriousbusiness/gotosocial/internal/concurrency"
+	"github.com/superseriousbusiness/gotosocial/internal/config"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/email"
 	"github.com/superseriousbusiness/gotosocial/internal/federation"
@@ -65,6 +67,7 @@ type BookmarkTestSuite struct {
 	testAttachments  map[string]*gtsmodel.MediaAttachment
 	testStatuses     map[string]*gtsmodel.Status
 	testFollows      map[string]*gtsmodel.Follow
+	testBookmarks    map[string]*gtsmodel.StatusBookmark
 
 	// module being tested
 	statusModule   *statuses.Module
@@ -80,6 +83,7 @@ func (suite *BookmarkTestSuite) SetupSuite() {
 	suite.testAttachments = testrig.NewTestAttachments()
 	suite.testStatuses = testrig.NewTestStatuses()
 	suite.testFollows = testrig.NewTestFollows()
+	suite.testBookmarks = testrig.NewTestBookmarks()
 }
 
 func (suite *BookmarkTestSuite) SetupTest() {
@@ -110,37 +114,216 @@ func (suite *BookmarkTestSuite) TearDownTest() {
 	testrig.StandardStorageTeardown(suite.storage)
 }
 
-func (suite *BookmarkTestSuite) TestGetBookmark() {
+func (suite *BookmarkTestSuite) getBookmarks(
-	t := suite.testTokens["local_account_1"]
+	account *gtsmodel.Account,
-	oauthToken := oauth.DBTokenToToken(t)
+	token *gtsmodel.Token,
-
+	user *gtsmodel.User,
-	targetStatus := suite.testStatuses["admin_account_status_1"]
+	expectedHTTPStatus int,
-
+	maxID string,
-	// setup
+	minID string,
+	limit int,
+) ([]*apimodel.Status, string, error) {
+	// instantiate recorder + test context
 	recorder := httptest.NewRecorder()
 	ctx, _ := testrig.CreateGinTestContext(recorder, nil)
+	ctx.Set(oauth.SessionAuthorizedAccount, account)
+	ctx.Set(oauth.SessionAuthorizedToken, oauth.DBTokenToToken(token))
 	ctx.Set(oauth.SessionAuthorizedApplication, suite.testApplications["application_1"])
-	ctx.Set(oauth.SessionAuthorizedToken, oauthToken)
+	ctx.Set(oauth.SessionAuthorizedUser, user)
-	ctx.Set(oauth.SessionAuthorizedUser, suite.testUsers["local_account_1"])
+
-	ctx.Set(oauth.SessionAuthorizedAccount, suite.testAccounts["local_account_1"])
+	// create the request URI
-	ctx.Request = httptest.NewRequest(http.MethodPost, fmt.Sprintf("http://localhost:8080%s", strings.Replace(statuses.BookmarkPath, ":id", targetStatus.ID, 1)), nil) // the endpoint we're hitting
+	requestPath := bookmarks.BasePath + "?" + bookmarks.LimitKey + "=" + strconv.Itoa(limit)
+	if maxID != "" {
+		requestPath = requestPath + "&" + bookmarks.MaxIDKey + "=" + maxID
+	}
+	if minID != "" {
+		requestPath = requestPath + "&" + bookmarks.MinIDKey + "=" + minID
+	}
+	baseURI := config.GetProtocol() + "://" + config.GetHost()
+	requestURI := baseURI + "/api/" + requestPath
+
+	// create the request
+	ctx.Request = httptest.NewRequest(http.MethodGet, requestURI, nil)
 	ctx.Request.Header.Set("accept", "application/json")
 
+	// trigger the handler
 	suite.bookmarkModule.BookmarksGETHandler(ctx)
 
-	// check response
+	// read the response
-	suite.EqualValues(http.StatusOK, recorder.Code)
-
 	result := recorder.Result()
 	defer result.Body.Close()
+
+	if resultCode := recorder.Code; expectedHTTPStatus != resultCode {
+		return nil, "", fmt.Errorf("expected %d got %d", expectedHTTPStatus, resultCode)
+	}
+
 	b, err := ioutil.ReadAll(result.Body)
-	suite.NoError(err)
+	if err != nil {
+		return nil, "", err
+	}
 
-	var statuses []model.Status
+	resp := []*apimodel.Status{}
-	err = json.Unmarshal(b, &statuses)
+	if err := json.Unmarshal(b, &resp); err != nil {
-	suite.NoError(err)
+		return nil, "", err
+	}
 
-	suite.Equal(1, len(statuses))
+	return resp, result.Header.Get("Link"), nil
+}
+
+func (suite *BookmarkTestSuite) TestGetBookmarksSingle() {
+	testAccount := suite.testAccounts["local_account_1"]
+	testToken := suite.testTokens["local_account_1"]
+	testUser := suite.testUsers["local_account_1"]
+
+	statuses, linkHeader, err := suite.getBookmarks(testAccount, testToken, testUser, http.StatusOK, "", "", 10)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Len(statuses, 1)
+	suite.Equal(`<http://localhost:8080/api/v1/bookmarks?limit=10&max_id=01F8MHD2QCZSZ6WQS2ATVPEYJ9>; rel="next", <http://localhost:8080/api/v1/bookmarks?limit=10&min_id=01F8MHD2QCZSZ6WQS2ATVPEYJ9>; rel="prev"`, linkHeader)
+}
+
+func (suite *BookmarkTestSuite) TestGetBookmarksMultiple() {
+	testAccount := suite.testAccounts["local_account_1"]
+	testToken := suite.testTokens["local_account_1"]
+	testUser := suite.testUsers["local_account_1"]
+
+	// Add a few extra bookmarks for this account.
+	ctx := context.Background()
+	for _, b := range []*gtsmodel.StatusBookmark{
+		{
+			ID:              "01GSZPDQYE9WZ26T501KMM876V", // oldest
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_2"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGHY3ACEN11D512V6MR0M",
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_3"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGY4ZSHNV0PR3HSBB1DDV", // newest
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_4"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+	} {
+		if err := suite.db.Put(ctx, b); err != nil {
+			suite.FailNow(err.Error())
+		}
+	}
+
+	statuses, linkHeader, err := suite.getBookmarks(testAccount, testToken, testUser, http.StatusOK, "", "", 10)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Len(statuses, 4)
+	suite.Equal(`<http://localhost:8080/api/v1/bookmarks?limit=10&max_id=01F8MHD2QCZSZ6WQS2ATVPEYJ9>; rel="next", <http://localhost:8080/api/v1/bookmarks?limit=10&min_id=01GSZPGY4ZSHNV0PR3HSBB1DDV>; rel="prev"`, linkHeader)
+}
+
+func (suite *BookmarkTestSuite) TestGetBookmarksMultiplePaging() {
+	testAccount := suite.testAccounts["local_account_1"]
+	testToken := suite.testTokens["local_account_1"]
+	testUser := suite.testUsers["local_account_1"]
+
+	// Add a few extra bookmarks for this account.
+	ctx := context.Background()
+	for _, b := range []*gtsmodel.StatusBookmark{
+		{
+			ID:              "01GSZPDQYE9WZ26T501KMM876V", // oldest
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_2"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGHY3ACEN11D512V6MR0M",
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_3"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGY4ZSHNV0PR3HSBB1DDV", // newest
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_4"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+	} {
+		if err := suite.db.Put(ctx, b); err != nil {
+			suite.FailNow(err.Error())
+		}
+	}
+
+	statuses, linkHeader, err := suite.getBookmarks(testAccount, testToken, testUser, http.StatusOK, "01GSZPGY4ZSHNV0PR3HSBB1DDV", "", 10)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Len(statuses, 3)
+	suite.Equal(`<http://localhost:8080/api/v1/bookmarks?limit=10&max_id=01F8MHD2QCZSZ6WQS2ATVPEYJ9>; rel="next", <http://localhost:8080/api/v1/bookmarks?limit=10&min_id=01GSZPGHY3ACEN11D512V6MR0M>; rel="prev"`, linkHeader)
+}
+
+func (suite *BookmarkTestSuite) TestGetBookmarksNone() {
+	testAccount := suite.testAccounts["local_account_1"]
+	testToken := suite.testTokens["local_account_1"]
+	testUser := suite.testUsers["local_account_1"]
+
+	// Remove all bookmarks for this account.
+	if err := suite.db.DeleteWhere(context.Background(), []db.Where{{Key: "account_id", Value: testAccount.ID}}, &[]*gtsmodel.StatusBookmark{}); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	statuses, linkHeader, err := suite.getBookmarks(testAccount, testToken, testUser, http.StatusOK, "", "", 10)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Empty(statuses)
+	suite.Empty(linkHeader)
+}
+
+func (suite *BookmarkTestSuite) TestGetBookmarksNonexistentStatus() {
+	testAccount := suite.testAccounts["local_account_1"]
+	testToken := suite.testTokens["local_account_1"]
+	testUser := suite.testUsers["local_account_1"]
+
+	// Add a few extra bookmarks for this account.
+	ctx := context.Background()
+	for _, b := range []*gtsmodel.StatusBookmark{
+		{
+			ID:              "01GSZPDQYE9WZ26T501KMM876V", // oldest
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_2"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGHY3ACEN11D512V6MR0M",
+			AccountID:       testAccount.ID,
+			StatusID:        suite.testStatuses["admin_account_status_3"].ID,
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+		{
+			ID:              "01GSZPGY4ZSHNV0PR3HSBB1DDV", // newest
+			AccountID:       testAccount.ID,
+			StatusID:        "01GSZQCRX4CXPECWA5M37QNV9F", // <-- THIS ONE DOESN'T EXIST
+			TargetAccountID: suite.testAccounts["admin_account"].ID,
+		},
+	} {
+		if err := suite.db.Put(ctx, b); err != nil {
+			suite.FailNow(err.Error())
+		}
+	}
+
+	statuses, linkHeader, err := suite.getBookmarks(testAccount, testToken, testUser, http.StatusOK, "", "", 10)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Len(statuses, 3)
+	suite.Equal(`<http://localhost:8080/api/v1/bookmarks?limit=10&max_id=01F8MHD2QCZSZ6WQS2ATVPEYJ9>; rel="next", <http://localhost:8080/api/v1/bookmarks?limit=10&min_id=01GSZPGHY3ACEN11D512V6MR0M>; rel="prev"`, linkHeader)
 }
 
 func TestBookmarkTestSuite(t *testing.T) {

internal/processing/account/bookmarks.go

@@ -20,69 +20,83 @@ package account
 
 import (
 	"context"
-	"fmt"
+	"errors"
 
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
+	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/id"
+	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/util"
 )
 
+// BookmarksGet returns a pageable response of statuses that are bookmarked by requestingAccount.
+// Paging for this response is done based on bookmark ID rather than status ID.
 func (p *Processor) BookmarksGet(ctx context.Context, requestingAccount *gtsmodel.Account, limit int, maxID string, minID string) (*apimodel.PageableResponse, gtserror.WithCode) {
-	if requestingAccount == nil {
-		return nil, gtserror.NewErrorForbidden(fmt.Errorf("cannot retrieve bookmarks without a requesting account"))
-	}
-
 	bookmarks, err := p.db.GetBookmarks(ctx, requestingAccount.ID, limit, maxID, minID)
 	if err != nil {
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	count := len(bookmarks)
+	var (
-	filtered := make([]*gtsmodel.Status, 0, len(bookmarks))
+		count          = len(bookmarks)
-	nextMaxIDValue := ""
+		items          = make([]interface{}, 0, count)
-	prevMinIDValue := ""
+		nextMaxIDValue = id.Highest
-	for i, b := range bookmarks {
+		prevMinIDValue = id.Lowest
-		s, err := p.db.GetStatusByID(ctx, b.StatusID)
+	)
+
+	for _, bookmark := range bookmarks {
+		status, err := p.db.GetStatusByID(ctx, bookmark.StatusID)
 		if err != nil {
-			return nil, gtserror.NewErrorInternalError(err)
+			if errors.Is(err, db.ErrNoEntries) {
+				// We just don't have the status for some reason.
+				// Skip this one.
+				continue
+			}
+			return nil, gtserror.NewErrorInternalError(err) // A real error has occurred.
 		}
 
-		visible, err := p.filter.StatusVisible(ctx, s, requestingAccount)
+		visible, err := p.filter.StatusVisible(ctx, status, requestingAccount)
-		if err == nil && visible {
+		if err != nil {
-			if i == count-1 {
+			log.Errorf(ctx, "error checking bookmarked status visibility: %s", err)
-				nextMaxIDValue = b.ID
+			continue
-			}
+		}
 
-			if i == 0 {
+		if !visible {
-				prevMinIDValue = b.ID
+			continue
-			}
+		}
 
-			filtered = append(filtered, s)
+		// Convert the status.
+		item, err := p.tc.StatusToAPIStatus(ctx, status, requestingAccount)
+		if err != nil {
+			log.Errorf(ctx, "error converting bookmarked status to api: %s", err)
+			continue
+		}
+		items = append(items, item)
+
+		// Page based on bookmark ID, not status ID.
+		// Note that we only set these values here
+		// when we're certain that the caller is able
+		// to see the status, *and* we're sure that
+		// we can produce an api model representation.
+		if bookmark.ID < nextMaxIDValue {
+			nextMaxIDValue = bookmark.ID // Lowest ID (for paging down).
+		}
+		if bookmark.ID > prevMinIDValue {
+			prevMinIDValue = bookmark.ID // Highest ID (for paging up).
 		}
 	}
 
-	count = len(filtered)
+	if len(items) == 0 {
-
-	if count == 0 {
 		return util.EmptyPageableResponse(), nil
 	}
 
-	items := []interface{}{}
-	for _, s := range filtered {
-		item, err := p.tc.StatusToAPIStatus(ctx, s, requestingAccount)
-		if err != nil {
-			return nil, gtserror.NewErrorInternalError(fmt.Errorf("error converting status to api: %s", err))
-		}
-		items = append(items, item)
-	}
-
 	return util.PackagePageableResponse(util.PageableResponseParams{
-		Items:            items,
+		Items:          items,
-		Path:             "/api/v1/bookmarks",
+		Path:           "/api/v1/bookmarks",
-		NextMaxIDValue:   nextMaxIDValue,
+		NextMaxIDValue: nextMaxIDValue,
-		PrevMinIDValue:   prevMinIDValue,
+		PrevMinIDValue: prevMinIDValue,
-		Limit:            limit,
+		Limit:          limit,
-		ExtraQueryParams: []string{},
 	})
 }

internal/processing/fromcommon.go

@@ -456,6 +456,11 @@ func (p *Processor) wipeStatus(ctx context.Context, statusToDelete *gtsmodel.Sta
 		return err
 	}
 
+	// delete all bookmarks that point to this status
+	if err := p.db.DeleteWhere(ctx, []db.Where{{Key: "status_id", Value: statusToDelete.ID}}, &[]*gtsmodel.StatusBookmark{}); err != nil {
+		return err
+	}
+
 	// delete all boosts for this status + remove them from timelines
 	if boosts, err := p.db.GetStatusReblogs(ctx, statusToDelete); err == nil {
 		for _, b := range boosts {