SuperSeriousBusiness
/
GoToSocial
mirror of https://github.com/superseriousbusiness/gotosocial
1
1
Fork 0
Code Issues Projects Releases Wiki Activity

[bugfix] further paging mishaps (#2884) 

* FURTHER paging shenanigans 🥲

* remove cursor logic from ToLinkURL()

* fix up paging tests

---------

Co-authored-by: tobi <tobi.smethurst@protonmail.com>
This commit is contained in:
kim 2024-04-30 15:22:23 +01:00 committed by GitHub
parent ec7c983e46
commit a8254a40e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 163 additions and 138 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
internal/api/activitypub/users/repliesget_test.go
View File

@ -161,7 +161,7 @@ func (suite *RepliesGetTestSuite) TestGetRepliesNext() {
"type": "OrderedCollectionPage",
"id": targetStatus.URI + "/replies?limit=20&only_other_accounts=false",
"partOf": targetStatus.URI + "/replies?only_other_accounts=false",
"next": "http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?limit=20&min_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false",
"next": "http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?limit=20&max_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false",
"prev": "http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?limit=20&min_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false",
"orderedItems": []string{"http://localhost:8080/users/admin/statuses/01FF25D5Q0DH7CHD57CTRS6WK0"},
"totalItems": 1,

140
internal/api/client/accounts/follow_test.go
View File

@ -21,8 +21,7 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"math/rand"
"io"
"net/http"
"net/http/httptest"
"net/url"
@ -42,9 +41,6 @@ import (
"github.com/tomnomnom/linkheader"
)
// random reader according to current-time source seed.
var randRd = rand.New(rand.NewSource(time.Now().Unix()))
type FollowTestSuite struct {
AccountStandardTestSuite
}
@ -76,33 +72,33 @@ func (suite *FollowTestSuite) TestFollowSelf() {
defer result.Body.Close()
// check the response
b, err := ioutil.ReadAll(result.Body)
b, err := io.ReadAll(result.Body)
_ = b
assert.NoError(suite.T(), err)
}
func (suite *FollowTestSuite) TestGetFollowersPageBackwardLimit2() {
suite.testGetFollowersPage(2, "backward")
func (suite *FollowTestSuite) TestGetFollowersPageNewestToOldestLimit2() {
suite.testGetFollowersPage(2, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowersPageBackwardLimit4() {
suite.testGetFollowersPage(4, "backward")
func (suite *FollowTestSuite) TestGetFollowersPageNewestToOldestLimit4() {
suite.testGetFollowersPage(4, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowersPageBackwardLimit6() {
suite.testGetFollowersPage(6, "backward")
func (suite *FollowTestSuite) TestGetFollowersPageNewestToOldestLimit6() {
suite.testGetFollowersPage(6, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowersPageForwardLimit2() {
suite.testGetFollowersPage(2, "forward")
func (suite *FollowTestSuite) TestGetFollowersPageOldestToNewestLimit2() {
suite.testGetFollowersPage(2, "oldestToNewest")
}
func (suite *FollowTestSuite) TestGetFollowersPageForwardLimit4() {
suite.testGetFollowersPage(4, "forward")
func (suite *FollowTestSuite) TestGetFollowersPageOldestToNewestLimit4() {
suite.testGetFollowersPage(4, "oldestToNewest")
}
func (suite *FollowTestSuite) TestGetFollowersPageForwardLimit6() {
suite.testGetFollowersPage(6, "forward")
func (suite *FollowTestSuite) TestGetFollowersPageOldestToNewestLimit6() {
suite.testGetFollowersPage(6, "oldestToNewest")
}
func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string) {
@ -117,8 +113,11 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
var i int
for _, targetAccount := range suite.testAccounts {
if targetAccount.ID == requestingAccount.ID {
// Have each account in the testrig follow the account
// that is requesting their followers from the API.
for _, account := range suite.testAccounts {
targetAccount := requestingAccount
if account.ID == targetAccount.ID {
// we cannot be our own target...
continue
}
@ -132,9 +131,9 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
ID: id,
CreatedAt: now,
UpdatedAt: now,
URI: fmt.Sprintf("%s/follow/%s", targetAccount.URI, id),
AccountID: targetAccount.ID,
TargetAccountID: requestingAccount.ID,
URI: fmt.Sprintf("%s/follow/%s", account.URI, id),
AccountID: account.ID,
TargetAccountID: targetAccount.ID,
})
suite.NoError(err)
@ -152,15 +151,17 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
var query string
switch direction {
case "backward":
// Set the starting query to page backward from newest.
case "newestToOldest":
// Set the starting query to page from
// newest (ie., first entry in slice).
acc := expectAccounts[0].(*model.Account)
newest, _ := suite.db.GetFollow(ctx, acc.ID, requestingAccount.ID)
expectAccounts = expectAccounts[1:]
query = fmt.Sprintf("limit=%d&max_id=%s", limit, newest.ID)
case "forward":
// Set the starting query to page forward from the oldest.
case "oldestToNewest":
// Set the starting query to page from
// oldest (ie., last entry in slice).
acc := expectAccounts[len(expectAccounts)-1].(*model.Account)
oldest, _ := suite.db.GetFollow(ctx, acc.ID, requestingAccount.ID)
expectAccounts = expectAccounts[:len(expectAccounts)-1]
@ -208,9 +209,9 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
)
switch direction {
case "backward":
// When paging backwards (DESC) we:
// - iter from end of received accounts
case "newestToOldest":
// When paging newest to oldest (ie., first page to last page):
// - iter from start of received accounts
// - iterate backward through received accounts
// - stop when we reach last index of received accounts
// - compare each received with the first index of expected accounts
@ -221,8 +222,8 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
expect = func(i []interface{}) interface{} { return i[0] }
trunc = func(i []interface{}) []interface{} { return i[1:] }
case "forward":
// When paging forwards (ASC) we:
case "oldestToNewest":
// When paging oldest to newest (ie., last page to first page):
// - iter from end of received accounts
// - iterate backward through received accounts
// - stop when we reach first index of received accounts
@ -230,7 +231,7 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
// - after each compare, drop the last index of expected accounts
start = func(i []*model.Account) int { return len(i) - 1 }
iter = func(i int) int { return i - 1 }
check = func(idx int, i []*model.Account) bool { return idx >= 0 }
check = func(idx int, _ []*model.Account) bool { return idx >= 0 }
expect = func(i []interface{}) interface{} { return i[len(i)-1] }
trunc = func(i []interface{}) []interface{} { return i[:len(i)-1] }
}
@ -256,7 +257,14 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
// Parse response link header values.
values := result.Header.Values("Link")
links := linkheader.ParseMultiple(values)
filteredLinks := links.FilterByRel("next")
var filteredLinks linkheader.Links
if direction == "newestToOldest" {
filteredLinks = links.FilterByRel("next")
} else {
filteredLinks = links.FilterByRel("prev")
}
suite.NotEmpty(filteredLinks, "no next link provided with more remaining accounts on page=%d", p)
// A ref link header was set.
@ -271,28 +279,28 @@ func (suite *FollowTestSuite) testGetFollowersPage(limit int, direction string)
}
}
func (suite *FollowTestSuite) TestGetFollowingPageBackwardLimit2() {
suite.testGetFollowingPage(2, "backward")
func (suite *FollowTestSuite) TestGetFollowingPageNewestToOldestLimit2() {
suite.testGetFollowingPage(2, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowingPageBackwardLimit4() {
suite.testGetFollowingPage(4, "backward")
func (suite *FollowTestSuite) TestGetFollowingPageNewestToOldestLimit4() {
suite.testGetFollowingPage(4, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowingPageBackwardLimit6() {
suite.testGetFollowingPage(6, "backward")
func (suite *FollowTestSuite) TestGetFollowingPageNewestToOldestLimit6() {
suite.testGetFollowingPage(6, "newestToOldest")
}
func (suite *FollowTestSuite) TestGetFollowingPageForwardLimit2() {
suite.testGetFollowingPage(2, "forward")
func (suite *FollowTestSuite) TestGetFollowingPageOldestToNewestLimit2() {
suite.testGetFollowingPage(2, "oldestToNewest")
}
func (suite *FollowTestSuite) TestGetFollowingPageForwardLimit4() {
suite.testGetFollowingPage(4, "forward")
func (suite *FollowTestSuite) TestGetFollowingPageOldestToNewestLimit4() {
suite.testGetFollowingPage(4, "oldestToNewest")
}
func (suite *FollowTestSuite) TestGetFollowingPageForwardLimit6() {
suite.testGetFollowingPage(6, "forward")
func (suite *FollowTestSuite) TestGetFollowingPageOldestToNewestLimit6() {
suite.testGetFollowingPage(6, "oldestToNewest")
}
func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string) {
@ -307,8 +315,11 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
var i int
// Have the account that is requesting their following
// list from the API follow each account in the testrig.
for _, targetAccount := range suite.testAccounts {
if targetAccount.ID == requestingAccount.ID {
account := requestingAccount
if targetAccount.ID == account.ID {
// we cannot be our own target...
continue
}
@ -322,8 +333,8 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
ID: id,
CreatedAt: now,
UpdatedAt: now,
URI: fmt.Sprintf("%s/follow/%s", requestingAccount.URI, id),
AccountID: requestingAccount.ID,
URI: fmt.Sprintf("%s/follow/%s", account.URI, id),
AccountID: account.ID,
TargetAccountID: targetAccount.ID,
})
suite.NoError(err)
@ -342,15 +353,17 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
var query string
switch direction {
case "backward":
// Set the starting query to page backward from newest.
case "newestToOldest":
// Set the starting query to page from
// newest (ie., first entry in slice).
acc := expectAccounts[0].(*model.Account)
newest, _ := suite.db.GetFollow(ctx, requestingAccount.ID, acc.ID)
expectAccounts = expectAccounts[1:]
query = fmt.Sprintf("limit=%d&max_id=%s", limit, newest.ID)
case "forward":
// Set the starting query to page forward from the oldest.
case "oldestToNewest":
// Set the starting query to page from
// oldest (ie., last entry in slice).
acc := expectAccounts[len(expectAccounts)-1].(*model.Account)
oldest, _ := suite.db.GetFollow(ctx, requestingAccount.ID, acc.ID)
expectAccounts = expectAccounts[:len(expectAccounts)-1]
@ -397,9 +410,9 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
)
switch direction {
case "backward":
// When paging backwards (DESC) we:
// - iter from end of received accounts
case "newestToOldest":
// When paging newest to oldest (ie., first page to last page):
// - iter from start of received accounts
// - iterate backward through received accounts
// - stop when we reach last index of received accounts
// - compare each received with the first index of expected accounts
@ -410,8 +423,8 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
expect = func(i []interface{}) interface{} { return i[0] }
trunc = func(i []interface{}) []interface{} { return i[1:] }
case "forward":
// When paging forwards (ASC) we:
case "oldestToNewest":
// When paging oldest to newest (ie., last page to first page):
// - iter from end of received accounts
// - iterate backward through received accounts
// - stop when we reach first index of received accounts
@ -419,7 +432,7 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
// - after each compare, drop the last index of expected accounts
start = func(i []*model.Account) int { return len(i) - 1 }
iter = func(i int) int { return i - 1 }
check = func(idx int, i []*model.Account) bool { return idx >= 0 }
check = func(idx int, _ []*model.Account) bool { return idx >= 0 }
expect = func(i []interface{}) interface{} { return i[len(i)-1] }
trunc = func(i []interface{}) []interface{} { return i[:len(i)-1] }
}
@ -445,7 +458,14 @@ func (suite *FollowTestSuite) testGetFollowingPage(limit int, direction string)
// Parse response link header values.
values := result.Header.Values("Link")
links := linkheader.ParseMultiple(values)
filteredLinks := links.FilterByRel("next")
var filteredLinks linkheader.Links
if direction == "newestToOldest" {
filteredLinks = links.FilterByRel("next")
} else {
filteredLinks = links.FilterByRel("prev")
}
suite.NotEmpty(filteredLinks, "no next link provided with more remaining accounts on page=%d", p)
// A ref link header was set.

68
internal/api/client/followrequests/get_test.go
View File

@ -107,28 +107,28 @@ func (suite *GetTestSuite) TestGet() {
]`, dst.String())
}
func (suite *GetTestSuite) TestGetPageBackwardLimit2() {
suite.testGetPage(2, "backward")
func (suite *GetTestSuite) TestGetPageNewestToOldestLimit2() {
suite.testGetPage(2, "newestToOldest")
}
func (suite *GetTestSuite) TestGetPageBackwardLimit4() {
suite.testGetPage(4, "backward")
func (suite *GetTestSuite) TestGetPageNewestToOldestLimit4() {
suite.testGetPage(4, "newestToOldest")
}
func (suite *GetTestSuite) TestGetPageBackwardLimit6() {
suite.testGetPage(6, "backward")
func (suite *GetTestSuite) TestGetPageNewestToOldestLimit6() {
suite.testGetPage(6, "newestToOldest")
}
func (suite *GetTestSuite) TestGetPageForwardLimit2() {
suite.testGetPage(2, "forward")
func (suite *GetTestSuite) TestGetPageOldestToNewestLimit2() {
suite.testGetPage(2, "oldestToNewest")
}
func (suite *GetTestSuite) TestGetPageForwardLimit4() {
suite.testGetPage(4, "forward")
func (suite *GetTestSuite) TestGetPageOldestToNewestLimit4() {
suite.testGetPage(4, "oldestToNewest")
}
func (suite *GetTestSuite) TestGetPageForwardLimit6() {
suite.testGetPage(6, "forward")
func (suite *GetTestSuite) TestGetPageOldestToNewestLimit6() {
suite.testGetPage(6, "oldestToNewest")
}
func (suite *GetTestSuite) testGetPage(limit int, direction string) {
@ -143,8 +143,11 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
var i int
for _, targetAccount := range suite.testAccounts {
if targetAccount.ID == requestingAccount.ID {
// Have each account in the testrig follow req the
// account requesting their followers from the API.
for _, account := range suite.testAccounts {
targetAccount := requestingAccount
if account.ID == requestingAccount.ID {
// we cannot be our own target...
continue
}
@ -158,9 +161,9 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
ID: id,
CreatedAt: now,
UpdatedAt: now,
URI: fmt.Sprintf("%s/follow/%s", targetAccount.URI, id),
AccountID: targetAccount.ID,
TargetAccountID: requestingAccount.ID,
URI: fmt.Sprintf("%s/follow/%s", account.URI, id),
AccountID: account.ID,
TargetAccountID: targetAccount.ID,
})
suite.NoError(err)
@ -178,15 +181,17 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
var query string
switch direction {
case "backward":
// Set the starting query to page backward from newest.
case "newestToOldest":
// Set the starting query to page from
// newest (ie., first entry in slice).
acc := expectAccounts[0].(*model.Account)
newest, _ := suite.db.GetFollowRequest(ctx, acc.ID, requestingAccount.ID)
expectAccounts = expectAccounts[1:]
query = fmt.Sprintf("limit=%d&max_id=%s", limit, newest.ID)
case "forward":
// Set the starting query to page forward from the oldest.
case "oldestToNewest":
// Set the starting query to page from
// oldest (ie., last entry in slice).
acc := expectAccounts[len(expectAccounts)-1].(*model.Account)
oldest, _ := suite.db.GetFollowRequest(ctx, acc.ID, requestingAccount.ID)
expectAccounts = expectAccounts[:len(expectAccounts)-1]
@ -232,9 +237,9 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
)
switch direction {
case "backward":
// When paging backwards (DESC) we:
// - iter from end of received accounts
case "newestToOldest":
// When paging newest to oldest (ie., first page to last page):
// - iter from start of received accounts
// - iterate backward through received accounts
// - stop when we reach last index of received accounts
// - compare each received with the first index of expected accounts
@ -245,8 +250,8 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
expect = func(i []interface{}) interface{} { return i[0] }
trunc = func(i []interface{}) []interface{} { return i[1:] }
case "forward":
// When paging forwards (ASC) we:
case "oldestToNewest":
// When paging oldest to newest (ie., last page to first page):
// - iter from end of received accounts
// - iterate backward through received accounts
// - stop when we reach first index of received accounts
@ -254,7 +259,7 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
// - after each compare, drop the last index of expected accounts
start = func(i []*model.Account) int { return len(i) - 1 }
iter = func(i int) int { return i - 1 }
check = func(idx int, i []*model.Account) bool { return idx >= 0 }
check = func(idx int, _ []*model.Account) bool { return idx >= 0 }
expect = func(i []interface{}) interface{} { return i[len(i)-1] }
trunc = func(i []interface{}) []interface{} { return i[:len(i)-1] }
}
@ -280,7 +285,14 @@ func (suite *GetTestSuite) testGetPage(limit int, direction string) {
// Parse response link header values.
values := result.Header.Values("Link")
links := linkheader.ParseMultiple(values)
filteredLinks := links.FilterByRel("next")
var filteredLinks linkheader.Links
if direction == "newestToOldest" {
filteredLinks = links.FilterByRel("next")
} else {
filteredLinks = links.FilterByRel("prev")
}
suite.NotEmpty(filteredLinks, "no next link provided with more remaining accounts on page=%d", p)
// A ref link header was set.

14
internal/paging/boundary.go
View File

@ -54,6 +54,13 @@ func EitherMinID(minID, sinceID string) Boundary {
the cursor value, and max_id provides a
limiting value to the results.
But to further complicate it...
The "next" and "prev" relative links provided
in the link header are ALWAYS DESCENDING. Which
means we will ALWAYS provide next=?max_id and
prev=?min_id. *shakes fist at mastodon api*
*/
switch {
case minID != "":
@ -67,7 +74,12 @@ func EitherMinID(minID, sinceID string) Boundary {
// SinceID ...
func SinceID(sinceID string) Boundary {
return Boundary{
Name: "since_id",
// even when a since_id query is
// provided, the next / prev rel
// links are DESCENDING with
// next:max_id and prev:min_id.
// so ALWAYS use min_id as name.
Name: "min_id",
Value: sinceID,
Order: OrderDescending,
}

77
internal/paging/page.go
View File

@ -202,8 +202,9 @@ func Page_PageFunc[WithID any](p *Page, in []WithID, get func(WithID) string) []
return in
}
// Next creates a new instance for the next returnable page, using
// given max value. This preserves original limit and max key name.
// Prev creates a new instance for the next returnable page, using
// given max value. This will always assume DESCENDING for Mastodon
// API compatibility, but in case of change it can support both.
func (p *Page) Next(lo, hi string) *Page {
if p == nil || lo == "" || hi == "" {
// no paging.
@ -216,25 +217,22 @@ func (p *Page) Next(lo, hi string) *Page {
// Set original limit.
p2.Limit = p.Limit
if p.order().Ascending() {
// When ascending, next page
// needs to start with min at
// the next highest value.
p2.Min = p.Min.new(hi)
p2.Max = p.Max.new("")
} else {
// When descending, next page
// needs to start with max at
// the next lowest value.
p2.Min = p.Min.new("")
p2.Max = p.Max.new(lo)
}
// NOTE:
// We ALWAYS assume the order
// when creating next / prev
// links is DESCENDING. It will
// always use prev: ?max_name
p2.Min = p.Min.new("")
p2.Max = p.Max.new(lo)
p2.Min.Order = OrderDescending
p2.Max.Order = OrderDescending
return p2
}
// Prev creates a new instance for the prev returnable page, using
// given min value. This preserves original limit and min key name.
// given min value. This will always assume DESCENDING for Mastodon
// API compatibility, but in case of change it can support both.
func (p *Page) Prev(lo, hi string) *Page {
if p == nil || lo == "" || hi == "" {
// no paging.
@ -247,19 +245,15 @@ func (p *Page) Prev(lo, hi string) *Page {
// Set original limit.
p2.Limit = p.Limit
if p.order().Ascending() {
// When ascending, prev page
// needs to start with max at
// the next lowest value.
p2.Min = p.Min.new("")
p2.Max = p.Max.new(lo)
} else {
// When descending, next page
// needs to start with max at
// the next lowest value.
p2.Min = p.Min.new(hi)
p2.Max = p.Max.new("")
}
// NOTE:
// We ALWAYS assume the order
// when creating next / prev
// links is DESCENDING. It will
// always use prev: ?min_name
p2.Min = p.Min.new(hi)
p2.Max = p.Max.new("")
p2.Min.Order = OrderDescending
p2.Max.Order = OrderDescending
return p2
}
@ -289,27 +283,14 @@ func (p *Page) ToLinkURL(proto, host, path string, queryParams url.Values) *url.
queryParams = cloneQuery(queryParams)
}
var cursor string
// Depending on page ordering, the
// page will be cursored by either
// the min or max query parameter.
if p.order().Ascending() {
cursor = p.Min.Name
} else {
cursor = p.Max.Name
if p.Min.Value != "" {
// Set page-minimum cursor value.
queryParams.Set(p.Min.Name, p.Min.Value)
}
if cursor != "" {
if p.Min.Value != "" {
// Set page-minimum cursor value.
queryParams.Set(cursor, p.Min.Value)
}
if p.Max.Value != "" {
// Set page-maximum cursor value.
queryParams.Set(cursor, p.Max.Value)
}
if p.Max.Value != "" {
// Set page-maximum cursor value.
queryParams.Set(p.Max.Name, p.Max.Value)
}
if p.Limit > 0 {