From 689a10fe176706342704dc75e10d7c1f23d35540 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Thu, 23 Feb 2023 23:11:10 +0100 Subject: [PATCH] [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 --- .../api/client/bookmarks/bookmarks_test.go | 225 ++++++++++++++++-- internal/processing/account/bookmarks.go | 92 ++++--- internal/processing/fromcommon.go | 5 + 3 files changed, 262 insertions(+), 60 deletions(-) diff --git a/internal/api/client/bookmarks/bookmarks_test.go b/internal/api/client/bookmarks/bookmarks_test.go index fac67ffe7..c39ad49f3 100644 --- a/internal/api/client/bookmarks/bookmarks_test.go +++ b/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() { - t := suite.testTokens["local_account_1"] - oauthToken := oauth.DBTokenToToken(t) - - targetStatus := suite.testStatuses["admin_account_status_1"] - - // setup +func (suite *BookmarkTestSuite) getBookmarks( + account *gtsmodel.Account, + token *gtsmodel.Token, + user *gtsmodel.User, + expectedHTTPStatus int, + maxID string, + 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, suite.testUsers["local_account_1"]) - ctx.Set(oauth.SessionAuthorizedAccount, suite.testAccounts["local_account_1"]) - 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 + ctx.Set(oauth.SessionAuthorizedUser, user) + + // create the request URI + 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 - suite.EqualValues(http.StatusOK, recorder.Code) - + // read the response 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 - err = json.Unmarshal(b, &statuses) - suite.NoError(err) + resp := []*apimodel.Status{} + if err := json.Unmarshal(b, &resp); err != nil { + 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(`; rel="next", ; 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(`; rel="next", ; 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(`; rel="next", ; 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(`; rel="next", ; rel="prev"`, linkHeader) } func TestBookmarkTestSuite(t *testing.T) { diff --git a/internal/processing/account/bookmarks.go b/internal/processing/account/bookmarks.go index 7551b1e0c..28688c20d 100644 --- a/internal/processing/account/bookmarks.go +++ b/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) - filtered := make([]*gtsmodel.Status, 0, len(bookmarks)) - nextMaxIDValue := "" - prevMinIDValue := "" - for i, b := range bookmarks { - s, err := p.db.GetStatusByID(ctx, b.StatusID) + var ( + count = len(bookmarks) + items = make([]interface{}, 0, count) + nextMaxIDValue = id.Highest + prevMinIDValue = id.Lowest + ) + + 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) - if err == nil && visible { - if i == count-1 { - nextMaxIDValue = b.ID - } + visible, err := p.filter.StatusVisible(ctx, status, requestingAccount) + if err != nil { + log.Errorf(ctx, "error checking bookmarked status visibility: %s", err) + continue + } - if i == 0 { - prevMinIDValue = b.ID - } + if !visible { + 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 count == 0 { + if len(items) == 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, - Path: "/api/v1/bookmarks", - NextMaxIDValue: nextMaxIDValue, - PrevMinIDValue: prevMinIDValue, - Limit: limit, - ExtraQueryParams: []string{}, + Items: items, + Path: "/api/v1/bookmarks", + NextMaxIDValue: nextMaxIDValue, + PrevMinIDValue: prevMinIDValue, + Limit: limit, }) } diff --git a/internal/processing/fromcommon.go b/internal/processing/fromcommon.go index a09d428e8..3e4c62c6c 100644 --- a/internal/processing/fromcommon.go +++ b/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 {