Skip to content

Commit

Permalink
[bugfix] further paging mishaps (superseriousbusiness#2884)
Browse files Browse the repository at this point in the history 
* FURTHER paging shenanigans 🥲

* remove cursor logic from ToLinkURL()

* fix up paging tests

---------

Co-authored-by: tobi <tobi.smethurst@protonmail.com>
  • Loading branch information
@NyaaaWhatsUpDoc @tsmethurst @nyarla
2 people authored and nyarla committed Jun 19, 2024
1 parent a18eeb0 commit 318794d
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 138 deletions.
2 changes: 1 addition & 1 deletion internal/api/activitypub/users/repliesget_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
140 changes: 80 additions & 60 deletions internal/api/client/accounts/follow_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"math/rand"
"io"
"net/http"
"net/http/httptest"
"net/url"
Expand All @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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
}
Expand All @@ -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)

Expand All @@ -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]
Expand Down Expand Up @@ -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
Expand All @@ -221,16 +222,16 @@ 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
// - compare each received with the last index of expected accounts
// - 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] }
}
Expand All @@ -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.
Expand All @@ -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) {
Expand All @@ -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
}
Expand All @@ -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)
Expand All @@ -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]
Expand Down Expand Up @@ -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
Expand All @@ -410,16 +423,16 @@ 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
// - compare each received with the last index of expected accounts
// - 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] }
}
Expand All @@ -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.
Expand Down
Loading

0 comments on commit 318794d

Please sign in to comment.