diff --git a/internal/api/activitypub/users/repliesget_test.go b/internal/api/activitypub/users/repliesget_test.go index e821bc897..cd1df0159 100644 --- a/internal/api/activitypub/users/repliesget_test.go +++ b/internal/api/activitypub/users/repliesget_test.go @@ -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, diff --git a/internal/api/client/accounts/follow_test.go b/internal/api/client/accounts/follow_test.go index 47526da1d..bc6a54735 100644 --- a/internal/api/client/accounts/follow_test.go +++ b/internal/api/client/accounts/follow_test.go @@ -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. diff --git a/internal/api/client/followrequests/get_test.go b/internal/api/client/followrequests/get_test.go index 24f3c9646..35e4488c3 100644 --- a/internal/api/client/followrequests/get_test.go +++ b/internal/api/client/followrequests/get_test.go @@ -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. diff --git a/internal/paging/boundary.go b/internal/paging/boundary.go index bf4508aff..8a3187d7f 100644 --- a/internal/paging/boundary.go +++ b/internal/paging/boundary.go @@ -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, } diff --git a/internal/paging/page.go b/internal/paging/page.go index 8e8261396..082012879 100644 --- a/internal/paging/page.go +++ b/internal/paging/page.go @@ -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 {