From 36f79e650c3cdf26290c879a4d5169ba1d7aedf4 Mon Sep 17 00:00:00 2001
From: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Mon, 25 Mar 2024 17:20:45 +0100
Subject: [PATCH] [bugfix] Avoid empty public/local timeline queries (#2784)
---
internal/api/client/timelines/public.go | 2 +-
internal/processing/timeline/public.go | 124 +++++++++++++-----
internal/processing/timeline/public_test.go | 96 ++++++++++++++
internal/processing/timeline/timeline_test.go | 69 ++++++++++
4 files changed, 259 insertions(+), 32 deletions(-)
create mode 100644 internal/processing/timeline/public_test.go
create mode 100644 internal/processing/timeline/timeline_test.go
diff --git a/internal/api/client/timelines/public.go b/internal/api/client/timelines/public.go
index c4ffbc6c8..49530216f 100644
--- a/internal/api/client/timelines/public.go
+++ b/internal/api/client/timelines/public.go
@@ -150,7 +150,7 @@ func (m *Module) PublicTimelineGETHandler(c *gin.Context) {
resp, errWithCode := m.processor.Timeline().PublicTimelineGet(
c.Request.Context(),
- authed,
+ authed.Account,
c.Query(apiutil.MaxIDKey),
c.Query(apiutil.SinceIDKey),
c.Query(apiutil.MinIDKey),
diff --git a/internal/processing/timeline/public.go b/internal/processing/timeline/public.go
index 0eb4a33c0..87de04f4a 100644
--- a/internal/processing/timeline/public.go
+++ b/internal/processing/timeline/public.go
@@ -25,50 +25,112 @@ import (
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/log"
- "github.com/superseriousbusiness/gotosocial/internal/oauth"
"github.com/superseriousbusiness/gotosocial/internal/util"
)
-func (p *Processor) PublicTimelineGet(ctx context.Context, authed *oauth.Auth, maxID string, sinceID string, minID string, limit int, local bool) (*apimodel.PageableResponse, gtserror.WithCode) {
- statuses, err := p.state.DB.GetPublicTimeline(ctx, maxID, sinceID, minID, limit, local)
- if err != nil && !errors.Is(err, db.ErrNoEntries) {
- err = gtserror.Newf("db error getting statuses: %w", err)
- return nil, gtserror.NewErrorInternalError(err)
- }
-
- count := len(statuses)
- if count == 0 {
- return util.EmptyPageableResponse(), nil
- }
-
+func (p *Processor) PublicTimelineGet(
+ ctx context.Context,
+ requester *gtsmodel.Account,
+ maxID string,
+ sinceID string,
+ minID string,
+ limit int,
+ local bool,
+) (*apimodel.PageableResponse, gtserror.WithCode) {
+ const maxAttempts = 3
var (
- items = make([]interface{}, 0, count)
-
- // Set next + prev values before filtering and API
- // converting, so caller can still page properly.
- nextMaxIDValue = statuses[count-1].ID
- prevMinIDValue = statuses[0].ID
+ nextMaxIDValue string
+ prevMinIDValue string
+ items = make([]any, 0, limit)
)
- for _, s := range statuses {
- timelineable, err := p.filter.StatusPublicTimelineable(ctx, authed.Account, s)
- if err != nil {
- log.Errorf(ctx, "error checking status visibility: %v", err)
- continue
+ // Try a few times to select appropriate public
+ // statuses from the db, paging up or down to
+ // reattempt if nothing suitable is found.
+outer:
+ for attempts := 1; ; attempts++ {
+ // Select slightly more than the limit to try to avoid situations where
+ // we filter out all the entries, and have to make another db call.
+ // It's cheaper to select more in 1 query than it is to do multiple queries.
+ statuses, err := p.state.DB.GetPublicTimeline(ctx, maxID, sinceID, minID, limit+5, local)
+ if err != nil && !errors.Is(err, db.ErrNoEntries) {
+ err = gtserror.Newf("db error getting statuses: %w", err)
+ return nil, gtserror.NewErrorInternalError(err)
}
- if !timelineable {
- continue
+ count := len(statuses)
+ if count == 0 {
+ // Nothing relevant (left) in the db.
+ return util.EmptyPageableResponse(), nil
}
- apiStatus, err := p.converter.StatusToAPIStatus(ctx, s, authed.Account)
- if err != nil {
- log.Errorf(ctx, "error convert to api status: %v", err)
- continue
+ // Page up from first status in slice
+ // (ie., one with the highest ID).
+ prevMinIDValue = statuses[0].ID
+
+ inner:
+ for _, s := range statuses {
+ // Push back the next page down ID to
+ // this status, regardless of whether
+ // we end up filtering it out or not.
+ nextMaxIDValue = s.ID
+
+ timelineable, err := p.filter.StatusPublicTimelineable(ctx, requester, s)
+ if err != nil {
+ log.Errorf(ctx, "error checking status visibility: %v", err)
+ continue inner
+ }
+
+ if !timelineable {
+ continue inner
+ }
+
+ apiStatus, err := p.converter.StatusToAPIStatus(ctx, s, requester)
+ if err != nil {
+ log.Errorf(ctx, "error converting to api status: %v", err)
+ continue inner
+ }
+
+ // Looks good, add this.
+ items = append(items, apiStatus)
+
+ // We called the db with a little
+ // more than the desired limit.
+ //
+ // Ensure we don't return more
+ // than the caller asked for.
+ if len(items) == limit {
+ break outer
+ }
}
- items = append(items, apiStatus)
+ if len(items) != 0 {
+ // We've got some items left after
+ // filtering, happily break + return.
+ break
+ }
+
+ if attempts >= maxAttempts {
+ // We reached our attempts limit.
+ // Be nice + warn about it.
+ log.Warn(ctx, "reached max attempts to find items in public timeline")
+ break
+ }
+
+ // We filtered out all items before we
+ // found anything we could return, but
+ // we still have attempts left to try
+ // fetching again. Set paging params
+ // and allow loop to continue.
+ if minID != "" {
+ // Paging up.
+ minID = prevMinIDValue
+ } else {
+ // Paging down.
+ maxID = nextMaxIDValue
+ }
}
return util.PackagePageableResponse(util.PageableResponseParams{
diff --git a/internal/processing/timeline/public_test.go b/internal/processing/timeline/public_test.go
new file mode 100644
index 000000000..f14fee1b9
--- /dev/null
+++ b/internal/processing/timeline/public_test.go
@@ -0,0 +1,96 @@
+// 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 timeline_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/stretchr/testify/suite"
+)
+
+type PublicTestSuite struct {
+ TimelineStandardTestSuite
+}
+
+func (suite *PublicTestSuite) TestPublicTimelineGet() {
+ var (
+ ctx = context.Background()
+ requester = suite.testAccounts["local_account_1"]
+ maxID = ""
+ sinceID = ""
+ minID = ""
+ limit = 10
+ local = false
+ )
+
+ resp, errWithCode := suite.timeline.PublicTimelineGet(
+ ctx,
+ requester,
+ maxID,
+ sinceID,
+ minID,
+ limit,
+ local,
+ )
+
+ // We should have some statuses,
+ // and paging headers should be set.
+ suite.NoError(errWithCode)
+ suite.NotEmpty(resp.Items)
+ suite.NotEmpty(resp.LinkHeader)
+ suite.NotEmpty(resp.NextLink)
+ suite.NotEmpty(resp.PrevLink)
+}
+
+func (suite *PublicTestSuite) TestPublicTimelineGetNotEmpty() {
+ var (
+ ctx = context.Background()
+ requester = suite.testAccounts["local_account_1"]
+ // Select 1 *just above* a status we know should
+ // not be in the public timeline -- a public
+ // reply to one of admin's statuses.
+ maxID = "01HE7XJ1CG84TBKH5V9XKBVGF6"
+ sinceID = ""
+ minID = ""
+ limit = 1
+ local = false
+ )
+
+ resp, errWithCode := suite.timeline.PublicTimelineGet(
+ ctx,
+ requester,
+ maxID,
+ sinceID,
+ minID,
+ limit,
+ local,
+ )
+
+ // We should have a status even though
+ // some other statuses were filtered out.
+ suite.NoError(errWithCode)
+ suite.Len(resp.Items, 1)
+ suite.Equal(`; rel="next", ; rel="prev"`, resp.LinkHeader)
+ suite.Equal(`http://localhost:8080/api/v1/timelines/public?limit=1&max_id=01F8MHCP5P2NWYQ416SBA0XSEV&local=false`, resp.NextLink)
+ suite.Equal(`http://localhost:8080/api/v1/timelines/public?limit=1&min_id=01HE7XJ1CG84TBKH5V9XKBVGF5&local=false`, resp.PrevLink)
+}
+
+func TestPublicTestSuite(t *testing.T) {
+ suite.Run(t, new(PublicTestSuite))
+}
diff --git a/internal/processing/timeline/timeline_test.go b/internal/processing/timeline/timeline_test.go
new file mode 100644
index 000000000..79626830e
--- /dev/null
+++ b/internal/processing/timeline/timeline_test.go
@@ -0,0 +1,69 @@
+// 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 timeline_test
+
+import (
+ "github.com/stretchr/testify/suite"
+ "github.com/superseriousbusiness/gotosocial/internal/db"
+ "github.com/superseriousbusiness/gotosocial/internal/filter/visibility"
+ "github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+ "github.com/superseriousbusiness/gotosocial/internal/processing/timeline"
+ "github.com/superseriousbusiness/gotosocial/internal/state"
+ "github.com/superseriousbusiness/gotosocial/internal/typeutils"
+ "github.com/superseriousbusiness/gotosocial/testrig"
+)
+
+type TimelineStandardTestSuite struct {
+ suite.Suite
+ db db.DB
+ state state.State
+
+ // standard suite models
+ testAccounts map[string]*gtsmodel.Account
+
+ // module being tested
+ timeline timeline.Processor
+}
+
+func (suite *TimelineStandardTestSuite) SetupSuite() {
+ suite.testAccounts = testrig.NewTestAccounts()
+}
+
+func (suite *TimelineStandardTestSuite) SetupTest() {
+ suite.state.Caches.Init()
+ testrig.StartNoopWorkers(&suite.state)
+
+ testrig.InitTestConfig()
+ testrig.InitTestLog()
+
+ suite.db = testrig.NewTestDB(&suite.state)
+ suite.state.DB = suite.db
+
+ suite.timeline = timeline.New(
+ &suite.state,
+ typeutils.NewConverter(&suite.state),
+ visibility.NewFilter(&suite.state),
+ )
+
+ testrig.StandardDBSetup(suite.db, suite.testAccounts)
+}
+
+func (suite *TimelineStandardTestSuite) TearDownTest() {
+ testrig.StandardDBTeardown(suite.db)
+ testrig.StopWorkers(&suite.state)
+}