[bugfix] Fix paging for empty items (#2236)

* use minID properly for public timeline

* return paged response properly even when 0 items

* use gtserror

* page more consistently (for now)

* test

* aaa

internal/db/bundb/timeline.go

@@ -178,19 +178,22 @@ func (t *timelineDB) GetPublicTimeline(ctx context.Context, maxID string, sinceI
 	}
 
 	// Make educated guess for slice size
-	statusIDs := make([]string, 0, limit)
+	var (
+		statusIDs   = make([]string, 0, limit)
+		frontToBack = true
+	)
 
 	q := t.db.
 		NewSelect().
 		TableExpr("? AS ?", bun.Ident("statuses"), bun.Ident("status")).
-		Column("status.id").
 		// Public only.
 		Where("? = ?", bun.Ident("status.visibility"), gtsmodel.VisibilityPublic).
 		// Ignore boosts.
 		Where("? IS NULL", bun.Ident("status.boost_of_id")).
-		Order("status.id DESC")
+		// Select only IDs from table
+		Column("status.id")
 
-	if maxID == "" {
+	if maxID == "" || maxID >= id.Highest {
 		const future = 24 * time.Hour
 
 		var err error
@@ -206,27 +209,54 @@ func (t *timelineDB) GetPublicTimeline(ctx context.Context, maxID string, sinceI
 	q = q.Where("? < ?", bun.Ident("status.id"), maxID)
 
 	if sinceID != "" {
+		// return only statuses HIGHER (ie., newer) than sinceID
 		q = q.Where("? > ?", bun.Ident("status.id"), sinceID)
 	}
 
 	if minID != "" {
+		// return only statuses HIGHER (ie., newer) than minID
 		q = q.Where("? > ?", bun.Ident("status.id"), minID)
+
+		// page up
+		frontToBack = false
 	}
 
 	if local {
+		// return only statuses posted by local account havers
 		q = q.Where("? = ?", bun.Ident("status.local"), local)
 	}
 
 	if limit > 0 {
+		// limit amount of statuses returned
 		q = q.Limit(limit)
 	}
 
+	if frontToBack {
+		// Page down.
+		q = q.Order("status.id DESC")
+	} else {
+		// Page up.
+		q = q.Order("status.id ASC")
+	}
+
 	if err := q.Scan(ctx, &statusIDs); err != nil {
 		return nil, err
 	}
 
-	statuses := make([]*gtsmodel.Status, 0, len(statusIDs))
+	if len(statusIDs) == 0 {
+		return nil, nil
+	}
 
+	// If we're paging up, we still want statuses
+	// to be sorted by ID desc, so reverse ids slice.
+	// https://zchee.github.io/golang-wiki/SliceTricks/#reversing
+	if !frontToBack {
+		for l, r := 0, len(statusIDs)-1; l < r; l, r = l+1, r-1 {
+			statusIDs[l], statusIDs[r] = statusIDs[r], statusIDs[l]
+		}
+	}
+
+	statuses := make([]*gtsmodel.Status, 0, len(statusIDs))
 	for _, id := range statusIDs {
 		// Fetch status from db for ID
 		status, err := t.state.DB.GetStatusByID(ctx, id)

internal/processing/account/bookmarks.go

@@ -25,7 +25,6 @@ import (
 	"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"
 )
@@ -34,15 +33,23 @@ import (
 // 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) {
 	bookmarks, err := p.state.DB.GetStatusBookmarks(ctx, requestingAccount.ID, limit, maxID, minID)
-	if err != nil {
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
+	count := len(bookmarks)
+	if count == 0 {
+		return util.EmptyPageableResponse(), nil
+	}
+
 	var (
-		count          = len(bookmarks)
 		items = make([]interface{}, 0, count)
-		nextMaxIDValue = id.Highest
-		prevMinIDValue = id.Lowest
+
+		// Set next + prev values before filtering and API
+		// converting, so caller can still page properly.
+		// Page based on bookmark ID, not status ID.
+		nextMaxIDValue = bookmarks[count-1].ID
+		prevMinIDValue = bookmarks[0].ID
 	)
 
 	for _, bookmark := range bookmarks {
@@ -73,23 +80,6 @@ func (p *Processor) BookmarksGet(ctx context.Context, requestingAccount *gtsmode
 			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).
-		}
-	}
-
-	if len(items) == 0 {
-		return util.EmptyPageableResponse(), nil
 	}
 
 	return util.PackagePageableResponse(util.PageableResponseParams{

internal/processing/account/statuses.go

@@ -74,21 +74,8 @@ func (p *Processor) StatusesGet(
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	if len(statuses) == 0 {
-		return util.EmptyPageableResponse(), nil
-	}
-
-	// Filtering + serialization process is the same for
-	// both pinned status queries and 'normal' ones.
-	filtered, err := p.filter.StatusesVisible(ctx, requestingAccount, statuses)
-	if err != nil {
-		return nil, gtserror.NewErrorInternalError(err)
-	}
-
-	count := len(filtered)
+	count := len(statuses)
 	if count == 0 {
-		// After filtering there were
-		// no statuses left to serve.
 		return util.EmptyPageableResponse(), nil
 	}
 
@@ -97,10 +84,17 @@ func (p *Processor) StatusesGet(
 
 		// Set next + prev values before filtering and API
 		// converting, so caller can still page properly.
-		nextMaxIDValue = filtered[count-1].ID
-		prevMinIDValue = filtered[0].ID
+		nextMaxIDValue = statuses[count-1].ID
+		prevMinIDValue = statuses[0].ID
 	)
 
+	// Filtering + serialization process is the same for
+	// both pinned status queries and 'normal' ones.
+	filtered, err := p.filter.StatusesVisible(ctx, requestingAccount, statuses)
+	if err != nil {
+		return nil, gtserror.NewErrorInternalError(err)
+	}
+
 	for _, s := range filtered {
 		// Convert filtered statuses to API statuses.
 		item, err := p.converter.StatusToAPIStatus(ctx, s, requestingAccount)

internal/processing/admin/report.go

@@ -19,6 +19,7 @@ package admin
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 	"time"
@@ -45,17 +46,20 @@ func (p *Processor) ReportsGet(
 	limit int,
 ) (*apimodel.PageableResponse, gtserror.WithCode) {
 	reports, err := p.state.DB.GetReports(ctx, resolved, accountID, targetAccountID, maxID, sinceID, minID, limit)
-	if err != nil {
-		if err == db.ErrNoEntries {
-			return util.EmptyPageableResponse(), nil
-		}
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	count := len(reports)
-	items := make([]interface{}, 0, count)
-	nextMaxIDValue := reports[count-1].ID
-	prevMinIDValue := reports[0].ID
+	if count == 0 {
+		return util.EmptyPageableResponse(), nil
+	}
+
+	var (
+		items          = make([]interface{}, 0, count)
+		nextMaxIDValue = reports[count-1].ID
+		prevMinIDValue = reports[0].ID
+	)
 
 	for _, r := range reports {
 		item, err := p.converter.ReportToAdminAPIReport(ctx, r, account)
@@ -65,7 +69,7 @@ func (p *Processor) ReportsGet(
 		items = append(items, item)
 	}
 
-	extraQueryParams := []string{}
+	extraQueryParams := make([]string, 0, 3)
 	if resolved != nil {
 		extraQueryParams = append(extraQueryParams, "resolved="+strconv.FormatBool(*resolved))
 	}

internal/processing/timeline/public.go

@@ -20,7 +20,6 @@ package timeline
 import (
 	"context"
 	"errors"
-	"fmt"
 
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
@@ -33,7 +32,7 @@ import (
 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 = fmt.Errorf("PublicTimelineGet: db error getting statuses: %w", err)
+		err = gtserror.Newf("db error getting statuses: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 

internal/util/paging.go

@@ -48,11 +48,6 @@ type PageableResponseParams struct {
 // a bunch of pageable items (notifications, statuses, etc), as well
 // as a Link header to inform callers of where to find next/prev items.
 func PackagePageableResponse(params PageableResponseParams) (*apimodel.PageableResponse, gtserror.WithCode) {
-	if len(params.Items) == 0 {
-		// No items to page through.
-		return EmptyPageableResponse(), nil
-	}
-
 	// Set default paging values, if
 	// they weren't set by the caller.
 	if params.NextMaxIDKey == "" {

internal/util/paging_test.go

@@ -118,6 +118,7 @@ func (suite *PagingSuite) TestPagingNoItems() {
 	config.SetHost("example.org")
 
 	params := util.PageableResponseParams{
+		Path:           "/api/v1/accounts/01H11KA68PM4NNYJEG0FJQ90R3/statuses",
 		NextMaxIDValue: "01H11KA1DM2VH3747YDE7FV5HN",
 		PrevMinIDValue: "01H11KBBVRRDYYC5KEPME1NP5R",
 		Limit:          10,
@@ -129,9 +130,9 @@ func (suite *PagingSuite) TestPagingNoItems() {
 	}
 
 	suite.Empty(resp.Items)
-	suite.Empty(resp.LinkHeader)
-	suite.Empty(resp.NextLink)
-	suite.Empty(resp.PrevLink)
+	suite.Equal(`<https://example.org/api/v1/accounts/01H11KA68PM4NNYJEG0FJQ90R3/statuses?limit=10&max_id=01H11KA1DM2VH3747YDE7FV5HN>; rel="next", <https://example.org/api/v1/accounts/01H11KA68PM4NNYJEG0FJQ90R3/statuses?limit=10&min_id=01H11KBBVRRDYYC5KEPME1NP5R>; rel="prev"`, resp.LinkHeader)
+	suite.Equal(`https://example.org/api/v1/accounts/01H11KA68PM4NNYJEG0FJQ90R3/statuses?limit=10&max_id=01H11KA1DM2VH3747YDE7FV5HN`, resp.NextLink)
+	suite.Equal(`https://example.org/api/v1/accounts/01H11KA68PM4NNYJEG0FJQ90R3/statuses?limit=10&min_id=01H11KBBVRRDYYC5KEPME1NP5R`, resp.PrevLink)
 }
 
 func TestPagingSuite(t *testing.T) {