[bugfix] notification types missing from link header (#3571)

* ensure notification types get included in link header query for notifications * fix type query keys
2024-11-25 15:33:21 +00:00 · 2024-11-25 15:33:21 +00:00 · a444adee97
parent c454b1b488
commit a444adee97
6 changed files with 87 additions and 103 deletions
--- a/internal/api/client/notifications/notificationsget.go
+++ b/internal/api/client/notifications/notificationsget.go
@ -18,14 +18,13 @@
 package notifications

 import (
-	"fmt"
 	"net/http"
-	"strconv"

 	"github.com/gin-gonic/gin"
 	apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/oauth"
+	"github.com/superseriousbusiness/gotosocial/internal/paging"
 )

 // NotificationsGETHandler swagger:operation GET /api/v1/notifications notifications
@ -152,18 +151,6 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) {
 		return
 	}

-	limit := 20
-	limitString := c.Query(LimitKey)
-	if limitString != "" {
-		i, err := strconv.ParseInt(limitString, 10, 32)
-		if err != nil {
-			err := fmt.Errorf("error parsing %s: %s", LimitKey, err)
-			apiutil.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGetV1)
-			return
-		}
-		limit = int(i)
-	}
-
 	types, errWithCode := apiutil.ParseNotificationTypes(c.QueryArray(TypesKey))
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
@ -176,13 +163,20 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) {
 		return
 	}

+	page, errWithCode := paging.ParseIDPage(c,
+		1,  // min limit
+		80, // max limit
+		20, // no limit
+	)
+	if errWithCode != nil {
+		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
+		return
+	}
+
 	resp, errWithCode := m.processor.Timeline().NotificationsGet(
 		c.Request.Context(),
 		authed,
-		c.Query(MaxIDKey),
-		c.Query(SinceIDKey),
-		c.Query(MinIDKey),
-		limit,
+		page,
 		types,
 		exclTypes,
 	)
--- a/internal/db/bundb/notification.go
+++ b/internal/db/bundb/notification.go
@ -26,8 +26,8 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"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/paging"
 	"github.com/superseriousbusiness/gotosocial/internal/state"
 	"github.com/superseriousbusiness/gotosocial/internal/util/xslices"
 	"github.com/uptrace/bun"
@ -192,22 +192,19 @@ func (n *notificationDB) PopulateNotification(ctx context.Context, notif *gtsmod
 func (n *notificationDB) GetAccountNotifications(
 	ctx context.Context,
 	accountID string,
-	maxID string,
-	sinceID string,
-	minID string,
-	limit int,
+	page *paging.Page,
 	types []gtsmodel.NotificationType,
 	excludeTypes []gtsmodel.NotificationType,
 ) ([]*gtsmodel.Notification, error) {
-	// Ensure reasonable
-	if limit < 0 {
-		limit = 0
-	}
+	var (
+		// Get paging params.
+		minID = page.GetMin()
+		maxID = page.GetMax()
+		limit = page.GetLimit()
+		order = page.GetOrder()

 		// Make educated guess for slice size
-	var (
 		notifIDs = make([]string, 0, limit)
-		frontToBack = true
 	)

 	q := n.db.
@ -215,23 +212,14 @@ func (n *notificationDB) GetAccountNotifications(
 		TableExpr("? AS ?", bun.Ident("notifications"), bun.Ident("notification")).
 		Column("notification.id")

-	if maxID == "" {
-		maxID = id.Highest
-	}
-
+	if maxID != "" {
 		// Return only notifs LOWER (ie., older) than maxID.
 		q = q.Where("? < ?", bun.Ident("notification.id"), maxID)
-
-	if sinceID != "" {
-		// Return only notifs HIGHER (ie., newer) than sinceID.
-		q = q.Where("? > ?", bun.Ident("notification.id"), sinceID)
 	}

 	if minID != "" {
 		// Return only notifs HIGHER (ie., newer) than minID.
 		q = q.Where("? > ?", bun.Ident("notification.id"), minID)
-
-		frontToBack = false // page up
 	}

 	if len(types) > 0 {
@ -251,12 +239,12 @@ func (n *notificationDB) GetAccountNotifications(
 		q = q.Limit(limit)
 	}

-	if frontToBack {
-		// Page down.
-		q = q.Order("notification.id DESC")
-	} else {
+	if order == paging.OrderAscending {
 		// Page up.
 		q = q.Order("notification.id ASC")
+	} else {
+		// Page down.
+		q = q.Order("notification.id DESC")
 	}

 	if err := q.Scan(ctx, &notifIDs); err != nil {
@ -269,11 +257,8 @@ func (n *notificationDB) GetAccountNotifications(

 	// If we're paging up, we still want notifications
 	// 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(notifIDs)-1; l < r; l, r = l+1, r-1 {
-			notifIDs[l], notifIDs[r] = notifIDs[r], notifIDs[l]
-		}
+	if order == paging.OrderAscending {
+		slices.Reverse(notifIDs)
 	}

 	// Fetch notification models by their IDs.
--- a/internal/db/bundb/notification_test.go
+++ b/internal/db/bundb/notification_test.go
@ -28,6 +28,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/id"
+	"github.com/superseriousbusiness/gotosocial/internal/paging"
 	"github.com/superseriousbusiness/gotosocial/internal/util"
 )

@ -92,10 +93,11 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithSpam() {
 	notifications, err := suite.db.GetAccountNotifications(
 		gtscontext.SetBarebones(context.Background()),
 		testAccount.ID,
-		id.Highest,
-		id.Lowest,
-		"",
-		20,
+		&paging.Page{
+			Min:   paging.EitherMinID("", id.Lowest),
+			Max:   paging.MaxID(id.Highest),
+			Limit: 20,
+		},
 		nil,
 		nil,
 	)
@ -115,10 +117,11 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithoutSpam() {
 	notifications, err := suite.db.GetAccountNotifications(
 		gtscontext.SetBarebones(context.Background()),
 		testAccount.ID,
-		id.Highest,
-		id.Lowest,
-		"",
-		20,
+		&paging.Page{
+			Min:   paging.EitherMinID("", id.Lowest),
+			Max:   paging.MaxID(id.Highest),
+			Limit: 20,
+		},
 		nil,
 		nil,
 	)
@ -140,10 +143,11 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithSpam() {
 	notifications, err := suite.db.GetAccountNotifications(
 		gtscontext.SetBarebones(context.Background()),
 		testAccount.ID,
-		id.Highest,
-		id.Lowest,
-		"",
-		20,
+		&paging.Page{
+			Min:   paging.EitherMinID("", id.Lowest),
+			Max:   paging.MaxID(id.Highest),
+			Limit: 20,
+		},
 		nil,
 		nil,
 	)
@ -161,10 +165,11 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithSpam() {
 	notifications, err = suite.db.GetAccountNotifications(
 		gtscontext.SetBarebones(context.Background()),
 		testAccount.ID,
-		id.Highest,
-		id.Lowest,
-		"",
-		20,
+		&paging.Page{
+			Min:   paging.EitherMinID("", id.Lowest),
+			Max:   paging.MaxID(id.Highest),
+			Limit: 20,
+		},
 		nil,
 		nil,
 	)
@ -183,10 +188,11 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithTwoAccounts() {
 	notifications, err := suite.db.GetAccountNotifications(
 		gtscontext.SetBarebones(context.Background()),
 		testAccount.ID,
-		id.Highest,
-		id.Lowest,
-		"",
-		20,
+		&paging.Page{
+			Min:   paging.EitherMinID("", id.Lowest),
+			Max:   paging.MaxID(id.Highest),
+			Limit: 20,
+		},
 		nil,
 		nil,
 	)
--- a/internal/db/notification.go
+++ b/internal/db/notification.go
@ -21,6 +21,7 @@ import (
 	"context"

 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/paging"
 )

 // Notification contains functions for creating and getting notifications.
@ -29,7 +30,7 @@ type Notification interface {
 	//
 	// Returned notifications will be ordered ID descending (ie., highest/newest to lowest/oldest).
 	// If types is empty, *all* notification types will be included.
-	GetAccountNotifications(ctx context.Context, accountID string, maxID string, sinceID string, minID string, limit int, types []gtsmodel.NotificationType, excludeTypes []gtsmodel.NotificationType) ([]*gtsmodel.Notification, error)
+	GetAccountNotifications(ctx context.Context, accountID string, page *paging.Page, types []gtsmodel.NotificationType, excludeTypes []gtsmodel.NotificationType) ([]*gtsmodel.Notification, error)

 	// GetNotificationByID returns one notification according to its id.
 	GetNotificationByID(ctx context.Context, id string) (*gtsmodel.Notification, error)
--- a/internal/processing/timeline/notification.go
+++ b/internal/processing/timeline/notification.go
@ -21,6 +21,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"

 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
@ -31,26 +32,21 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/oauth"
+	"github.com/superseriousbusiness/gotosocial/internal/paging"
 	"github.com/superseriousbusiness/gotosocial/internal/util"
 )

 func (p *Processor) NotificationsGet(
 	ctx context.Context,
 	authed *oauth.Auth,
-	maxID string,
-	sinceID string,
-	minID string,
-	limit int,
+	page *paging.Page,
 	types []gtsmodel.NotificationType,
 	excludeTypes []gtsmodel.NotificationType,
 ) (*apimodel.PageableResponse, gtserror.WithCode) {
 	notifs, err := p.state.DB.GetAccountNotifications(
 		ctx,
 		authed.Account.ID,
-		maxID,
-		sinceID,
-		minID,
-		limit,
+		page,
 		types,
 		excludeTypes,
 	)
@ -79,21 +75,14 @@ func (p *Processor) NotificationsGet(

 	var (
 		items = make([]interface{}, 0, count)
-		nextMaxIDValue string
-		prevMinIDValue string
+
+		// Get the lowest and highest
+		// ID values, used for paging.
+		lo = notifs[count-1].ID
+		hi = notifs[0].ID
 	)

-	for i, n := range notifs {
-		// Set next + prev values before filtering and API
-		// converting, so caller can still page properly.
-		if i == count-1 {
-			nextMaxIDValue = n.ID
-		}
-
-		if i == 0 {
-			prevMinIDValue = n.ID
-		}
-
+	for _, n := range notifs {
 		visible, err := p.notifVisible(ctx, n, authed.Account)
 		if err != nil {
 			log.Debugf(ctx, "skipping notification %s because of an error checking notification visibility: %v", n.ID, err)
@ -115,13 +104,22 @@ func (p *Processor) NotificationsGet(
 		items = append(items, item)
 	}

-	return util.PackagePageableResponse(util.PageableResponseParams{
+	// Build type query string.
+	query := make(url.Values)
+	for _, typ := range types {
+		query.Add("types[]", typ.String())
+	}
+	for _, typ := range excludeTypes {
+		query.Add("exclude_types[]", typ.String())
+	}
+
+	return paging.PackageResponse(paging.ResponseParams{
 		Items: items,
-		Path:           "api/v1/notifications",
-		NextMaxIDValue: nextMaxIDValue,
-		PrevMinIDValue: prevMinIDValue,
-		Limit:          limit,
-	})
+		Path:  "/api/v1/notifications",
+		Next:  page.Next(lo, hi),
+		Prev:  page.Prev(lo, hi),
+		Query: query,
+	}), nil
 }

 func (p *Processor) NotificationGet(ctx context.Context, account *gtsmodel.Account, targetNotifID string) (*apimodel.Notification, gtserror.WithCode) {
--- a/internal/processing/workers/surfacenotify_test.go
+++ b/internal/processing/workers/surfacenotify_test.go
@ -89,7 +89,7 @@ func (suite *SurfaceNotifyTestSuite) TestSpamNotifs() {
 	notifs, err := testStructs.State.DB.GetAccountNotifications(
 		gtscontext.SetBarebones(ctx),
 		targetAccount.ID,
-		"", "", "", 0, nil, nil,
+		nil, nil, nil,
 	)
 	if err != nil {
 		suite.FailNow(err.Error())