[bugfix] self-referencing collection pages for status replies (#2364)

3 files changed, 143 insertions, 60 deletions

diff --git a/internal/api/activitypub/users/repliesget.go b/internal/api/activitypub/users/repliesget.go
index fd9dc090b..3ac4ccbbb 100644
--- a/internal/api/activitypub/users/repliesget.go
+++ b/internal/api/activitypub/users/repliesget.go
@@ -20,14 +20,13 @@ package users
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
 	"net/http"
-	"strconv"
 	"strings"
 
 	"github.com/gin-gonic/gin"
 	apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+	"github.com/superseriousbusiness/gotosocial/internal/paging"
 )
 
 // StatusRepliesGETHandler swagger:operation GET /users/{username}/statuses/{status}/replies s2sRepliesGet
@@ -120,36 +119,43 @@ func (m *Module) StatusRepliesGETHandler(c *gin.Context) {
 		return
 	}
 
-	var page bool
-	if pageString := c.Query(PageKey); pageString != "" {
-		i, err := strconv.ParseBool(pageString)
-		if err != nil {
-			err := fmt.Errorf("error parsing %s: %s", PageKey, err)
-			apiutil.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGetV1)
-			return
-		}
-		page = i
+	// Look for supplied 'only_other_accounts' query key.
+	onlyOtherAccounts, errWithCode := apiutil.ParseOnlyOtherAccounts(
+		c.Query(apiutil.OnlyOtherAccountsKey),
+		true, // default = enabled
+	)
+	if errWithCode != nil {
+		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
+		return
 	}
 
-	onlyOtherAccounts := false
-	onlyOtherAccountsString := c.Query(OnlyOtherAccountsKey)
-	if onlyOtherAccountsString != "" {
-		i, err := strconv.ParseBool(onlyOtherAccountsString)
-		if err != nil {
-			err := fmt.Errorf("error parsing %s: %s", OnlyOtherAccountsKey, err)
-			apiutil.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGetV1)
-			return
-		}
-		onlyOtherAccounts = i
+	// Look for given paging query parameters.
+	page, errWithCode := paging.ParseIDPage(c,
+		1,  // min limit
+		40, // max limit
+		0,  // default = disabled
+	)
+	if errWithCode != nil {
+		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
+		return
 	}
 
-	minID := ""
-	minIDString := c.Query(MinIDKey)
-	if minIDString != "" {
-		minID = minIDString
+	// COMPATIBILITY FIX: 'page=true' enables paging.
+	if page == nil && c.Query("page") == "true" {
+		page = new(paging.Page)
+		page.Max = paging.MaxID("")
+		page.Min = paging.MinID("")
+		page.Limit = 20 // default
 	}
 
-	resp, errWithCode := m.processor.Fedi().StatusRepliesGet(c.Request.Context(), requestedUsername, requestedStatusID, page, onlyOtherAccounts, c.Query("only_other_accounts") != "", minID)
+	// Fetch serialized status replies response for input status.
+	resp, errWithCode := m.processor.Fedi().StatusRepliesGet(
+		c.Request.Context(),
+		requestedUsername,
+		requestedStatusID,
+		page,
+		onlyOtherAccounts,
+	)
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return
@@ -157,7 +163,8 @@ func (m *Module) StatusRepliesGETHandler(c *gin.Context) {
 
 	b, err := json.Marshal(resp)
 	if err != nil {
-		apiutil.ErrorHandler(c, gtserror.NewErrorInternalError(err), m.processor.InstanceGetV1)
+		errWithCode := gtserror.NewErrorInternalError(err)
+		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return
 	}
 
diff --git a/internal/api/activitypub/users/repliesget_test.go b/internal/api/activitypub/users/repliesget_test.go
index 26492d8ce..ac25f3617 100644
--- a/internal/api/activitypub/users/repliesget_test.go
+++ b/internal/api/activitypub/users/repliesget_test.go
@@ -18,10 +18,10 @@
 package users_test
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
-	"fmt"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -31,6 +31,7 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/superseriousbusiness/activity/streams"
 	"github.com/superseriousbusiness/activity/streams/vocab"
+	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/api/activitypub/users"
 	"github.com/superseriousbusiness/gotosocial/testrig"
 )
@@ -49,7 +50,7 @@ func (suite *RepliesGetTestSuite) TestGetReplies() {
 	// setup request
 	recorder := httptest.NewRecorder()
 	ctx, _ := testrig.CreateGinTestContext(recorder, nil)
-	ctx.Request = httptest.NewRequest(http.MethodGet, targetStatus.URI+"/replies", nil) // the endpoint we're hitting
+	ctx.Request = httptest.NewRequest(http.MethodGet, targetStatus.URI+"/replies?only_other_accounts=false", nil) // the endpoint we're hitting
 	ctx.Request.Header.Set("accept", "application/activity+json")
 	ctx.Request.Header.Set("Signature", signedRequest.SignatureHeader)
 	ctx.Request.Header.Set("Date", signedRequest.DateHeader)
@@ -76,13 +77,26 @@ func (suite *RepliesGetTestSuite) TestGetReplies() {
 	// check response
 	suite.EqualValues(http.StatusOK, recorder.Code)
 
+	// Read response body.
 	result := recorder.Result()
 	defer result.Body.Close()
-	b, err := ioutil.ReadAll(result.Body)
+	b, err := io.ReadAll(result.Body)
 	assert.NoError(suite.T(), err)
-	assert.Equal(suite.T(), `{"@context":"https://www.w3.org/ns/activitystreams","first":{"id":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?page=true","next":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?only_other_accounts=false\u0026page=true","partOf":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies","type":"CollectionPage"},"id":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies","type":"Collection"}`, string(b))
 
-	// should be a Collection
+	// Indent JSON
+	// for readability.
+	b = indentJSON(b)
+
+	// Create JSON string of expected output.
+	expect := toJSON(map[string]any{
+		"@context":   "https://www.w3.org/ns/activitystreams",
+		"type":       "OrderedCollection",
+		"id":         targetStatus.URI + "/replies?only_other_accounts=false",
+		"first":      targetStatus.URI + "/replies?limit=20&only_other_accounts=false",
+		"totalItems": 1,
+	})
+	assert.Equal(suite.T(), expect, string(b))
+
 	m := make(map[string]interface{})
 	err = json.Unmarshal(b, &m)
 	assert.NoError(suite.T(), err)
@@ -90,7 +104,7 @@ func (suite *RepliesGetTestSuite) TestGetReplies() {
 	t, err := streams.ToType(context.Background(), m)
 	assert.NoError(suite.T(), err)
 
-	_, ok := t.(vocab.ActivityStreamsCollection)
+	_, ok := t.(vocab.ActivityStreamsOrderedCollection)
 	assert.True(suite.T(), ok)
 }
 
@@ -131,14 +145,29 @@ func (suite *RepliesGetTestSuite) TestGetRepliesNext() {
 	// check response
 	suite.EqualValues(http.StatusOK, recorder.Code)
 
+	// Read response body.
 	result := recorder.Result()
 	defer result.Body.Close()
-	b, err := ioutil.ReadAll(result.Body)
+	b, err := io.ReadAll(result.Body)
 	assert.NoError(suite.T(), err)
 
-	assert.Equal(suite.T(), `{"@context":"https://www.w3.org/ns/activitystreams","id":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?page=true\u0026only_other_accounts=false","items":"http://localhost:8080/users/admin/statuses/01FF25D5Q0DH7CHD57CTRS6WK0","next":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?only_other_accounts=false\u0026page=true\u0026min_id=01FF25D5Q0DH7CHD57CTRS6WK0","partOf":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies","type":"CollectionPage"}`, string(b))
+	// Indent JSON
+	// for readability.
+	b = indentJSON(b)
+
+	// Create JSON string of expected output.
+	expect := toJSON(map[string]any{
+		"@context":     "https://www.w3.org/ns/activitystreams",
+		"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",
+		"prev":         "http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?limit=20&max_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false",
+		"orderedItems": "http://localhost:8080/users/admin/statuses/01FF25D5Q0DH7CHD57CTRS6WK0",
+		"totalItems":   1,
+	})
+	assert.Equal(suite.T(), expect, string(b))
 
-	// should be a Collection
 	m := make(map[string]interface{})
 	err = json.Unmarshal(b, &m)
 	assert.NoError(suite.T(), err)
@@ -146,10 +175,10 @@ func (suite *RepliesGetTestSuite) TestGetRepliesNext() {
 	t, err := streams.ToType(context.Background(), m)
 	assert.NoError(suite.T(), err)
 
-	page, ok := t.(vocab.ActivityStreamsCollectionPage)
+	page, ok := t.(vocab.ActivityStreamsOrderedCollectionPage)
 	assert.True(suite.T(), ok)
 
-	assert.Equal(suite.T(), page.GetActivityStreamsItems().Len(), 1)
+	assert.Equal(suite.T(), page.GetActivityStreamsOrderedItems().Len(), 1)
 }
 
 func (suite *RepliesGetTestSuite) TestGetRepliesLast() {
@@ -162,7 +191,7 @@ func (suite *RepliesGetTestSuite) TestGetRepliesLast() {
 	// setup request
 	recorder := httptest.NewRecorder()
 	ctx, _ := testrig.CreateGinTestContext(recorder, nil)
-	ctx.Request = httptest.NewRequest(http.MethodGet, targetStatus.URI+"/replies?only_other_accounts=false&page=true&min_id=01FF25D5Q0DH7CHD57CTRS6WK0", nil) // the endpoint we're hitting
+	ctx.Request = httptest.NewRequest(http.MethodGet, targetStatus.URI+"/replies?min_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false", nil)
 	ctx.Request.Header.Set("accept", "application/activity+json")
 	ctx.Request.Header.Set("Signature", signedRequest.SignatureHeader)
 	ctx.Request.Header.Set("Date", signedRequest.DateHeader)
@@ -189,15 +218,27 @@ func (suite *RepliesGetTestSuite) TestGetRepliesLast() {
 	// check response
 	suite.EqualValues(http.StatusOK, recorder.Code)
 
+	// Read response body.
 	result := recorder.Result()
 	defer result.Body.Close()
-	b, err := ioutil.ReadAll(result.Body)
+	b, err := io.ReadAll(result.Body)
 	assert.NoError(suite.T(), err)
 
-	fmt.Println(string(b))
-	assert.Equal(suite.T(), `{"@context":"https://www.w3.org/ns/activitystreams","id":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?page=true\u0026only_other_accounts=false\u0026min_id=01FF25D5Q0DH7CHD57CTRS6WK0","items":[],"next":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies?only_other_accounts=false\u0026page=true","partOf":"http://localhost:8080/users/the_mighty_zork/statuses/01F8MHAMCHF6Y650WCRSCP4WMY/replies","type":"CollectionPage"}`, string(b))
+	// Indent JSON
+	// for readability.
+	b = indentJSON(b)
+
+	// Create JSON string of expected output.
+	expect := toJSON(map[string]any{
+		"@context":     "https://www.w3.org/ns/activitystreams",
+		"type":         "OrderedCollectionPage",
+		"id":           targetStatus.URI + "/replies?min_id=01FF25D5Q0DH7CHD57CTRS6WK0&only_other_accounts=false",
+		"partOf":       targetStatus.URI + "/replies?only_other_accounts=false",
+		"orderedItems": []any{}, // empty
+		"totalItems":   1,
+	})
+	assert.Equal(suite.T(), expect, string(b))
 
-	// should be a Collection
 	m := make(map[string]interface{})
 	err = json.Unmarshal(b, &m)
 	assert.NoError(suite.T(), err)
@@ -205,12 +246,39 @@ func (suite *RepliesGetTestSuite) TestGetRepliesLast() {
 	t, err := streams.ToType(context.Background(), m)
 	assert.NoError(suite.T(), err)
 
-	page, ok := t.(vocab.ActivityStreamsCollectionPage)
+	page, ok := t.(vocab.ActivityStreamsOrderedCollectionPage)
 	assert.True(suite.T(), ok)
 
-	assert.Equal(suite.T(), page.GetActivityStreamsItems().Len(), 0)
+	assert.Equal(suite.T(), page.GetActivityStreamsOrderedItems().Len(), 0)
 }
 
 func TestRepliesGetTestSuite(t *testing.T) {
 	suite.Run(t, new(RepliesGetTestSuite))
 }
+
+// toJSON will return indented JSON serialized form of 'a'.
+func toJSON(a any) string {
+	v, ok := a.(vocab.Type)
+	if ok {
+		m, err := ap.Serialize(v)
+		if err != nil {
+			panic(err)
+		}
+		a = m
+	}
+	b, err := json.MarshalIndent(a, "", "  ")
+	if err != nil {
+		panic(err)
+	}
+	return string(b)
+}
+
+// indentJSON will return indented JSON from raw provided JSON.
+func indentJSON(b []byte) []byte {
+	var dst bytes.Buffer
+	err := json.Indent(&dst, b, "", "  ")
+	if err != nil {
+		panic(err)
+	}
+	return dst.Bytes()
+}
diff --git a/internal/api/util/parsequery.go b/internal/api/util/parsequery.go
index 6a9116dcf..da6320b67 100644
--- a/internal/api/util/parsequery.go
+++ b/internal/api/util/parsequery.go
@@ -41,6 +41,10 @@ const (
 	SinceIDKey = "since_id"
 	MinIDKey   = "min_id"
 
+	/* AP endpoint keys */
+
+	OnlyOtherAccountsKey = "only_other_accounts"
+
 	/* Search keys */
 
 	SearchExcludeUnreviewedKey = "exclude_unreviewed"
@@ -66,20 +70,6 @@ const (
 	DomainPermissionImportKey = "import"
 )
 
-// parseError returns gtserror.WithCode set to 400 Bad Request, to indicate
-// to the caller that a key was set to a value that could not be parsed.
-func parseError(key string, value, defaultValue any, err error) gtserror.WithCode {
-	err = fmt.Errorf("error parsing key %s with value %s as %T: %w", key, value, defaultValue, err)
-	return gtserror.NewErrorBadRequest(err, err.Error())
-}
-
-// requiredError returns gtserror.WithCode set to 400 Bad Request, to indicate
-// to the caller a required key value was not provided, or was empty.
-func requiredError(key string) gtserror.WithCode {
-	err := fmt.Errorf("required key %s was not set or had empty value", key)
-	return gtserror.NewErrorBadRequest(err, err.Error())
-}
-
 /*
 	Parse functions for *OPTIONAL* parameters with default values.
 */
@@ -129,6 +119,10 @@ func ParseDomainPermissionImport(value string, defaultValue bool) (bool, gtserro
 	return parseBool(value, defaultValue, DomainPermissionImportKey)
 }
 
+func ParseOnlyOtherAccounts(value string, defaultValue bool) (bool, gtserror.WithCode) {
+	return parseBool(value, defaultValue, OnlyOtherAccountsKey)
+}
+
 /*
 	Parse functions for *REQUIRED* parameters.
 */
@@ -248,3 +242,17 @@ func parseInt(value string, defaultValue int, max int, min int, key string) (int
 
 	return i, nil
 }
+
+// parseError returns gtserror.WithCode set to 400 Bad Request, to indicate
+// to the caller that a key was set to a value that could not be parsed.
+func parseError(key string, value, defaultValue any, err error) gtserror.WithCode {
+	err = fmt.Errorf("error parsing key %s with value %s as %T: %w", key, value, defaultValue, err)
+	return gtserror.NewErrorBadRequest(err, err.Error())
+}
+
+// requiredError returns gtserror.WithCode set to 400 Bad Request, to indicate
+// to the caller a required key value was not provided, or was empty.
+func requiredError(key string) gtserror.WithCode {
+	err := fmt.Errorf("required key %s was not set or had empty value", key)
+	return gtserror.NewErrorBadRequest(err, err.Error())
+}