[bugfix] Avoid empty public/local timeline queries (#2784)

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),

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{

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 <http://www.gnu.org/licenses/>.
+
+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(`<http://localhost:8080/api/v1/timelines/public?limit=1&max_id=01F8MHCP5P2NWYQ416SBA0XSEV&local=false>; rel="next", <http://localhost:8080/api/v1/timelines/public?limit=1&min_id=01HE7XJ1CG84TBKH5V9XKBVGF5&local=false>; 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))
+}

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