From a444adee979375ed5d7af38346029a3d90bc77eb Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:33:21 +0000 Subject: [PATCH] [bugfix] notification types missing from link header (#3571) * ensure notification types get included in link header query for notifications * fix type query keys --- .../client/notifications/notificationsget.go | 30 ++++------ internal/db/bundb/notification.go | 53 +++++++----------- internal/db/bundb/notification_test.go | 46 ++++++++------- internal/db/notification.go | 3 +- internal/processing/timeline/notification.go | 56 +++++++++---------- .../processing/workers/surfacenotify_test.go | 2 +- 6 files changed, 87 insertions(+), 103 deletions(-) diff --git a/internal/api/client/notifications/notificationsget.go b/internal/api/client/notifications/notificationsget.go index cc3e5bdb7..841768c63 100644 --- 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, ) diff --git a/internal/db/bundb/notification.go b/internal/db/bundb/notification.go index a20ab7e3f..d4f8799bd 100644 --- a/internal/db/bundb/notification.go +++ b/internal/db/bundb/notification.go @@ -26,8 +26,8 @@ "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 - } - - // Make educated guess for slice size var ( - notifIDs = make([]string, 0, limit) - frontToBack = true + // Get paging params. + minID = page.GetMin() + maxID = page.GetMax() + limit = page.GetLimit() + order = page.GetOrder() + + // Make educated guess for slice size + notifIDs = make([]string, 0, limit) ) 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 - } - - // 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 maxID != "" { + // Return only notifs LOWER (ie., older) than maxID. + q = q.Where("? < ?", bun.Ident("notification.id"), maxID) } 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, ¬ifIDs); 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. diff --git a/internal/db/bundb/notification_test.go b/internal/db/bundb/notification_test.go index eb2c02066..8e2fb8031 100644 --- a/internal/db/bundb/notification_test.go +++ b/internal/db/bundb/notification_test.go @@ -28,6 +28,7 @@ "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, ) diff --git a/internal/db/notification.go b/internal/db/notification.go index c962023be..c608261dc 100644 --- a/internal/db/notification.go +++ b/internal/db/notification.go @@ -21,6 +21,7 @@ "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) diff --git a/internal/processing/timeline/notification.go b/internal/processing/timeline/notification.go index 92dbf851f..a242c7b74 100644 --- a/internal/processing/timeline/notification.go +++ b/internal/processing/timeline/notification.go @@ -21,6 +21,7 @@ "context" "errors" "fmt" + "net/url" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" "github.com/superseriousbusiness/gotosocial/internal/db" @@ -31,26 +32,21 @@ "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, ) @@ -78,22 +74,15 @@ func (p *Processor) NotificationsGet( compiledMutes := usermute.NewCompiledUserMuteList(mutes) var ( - items = make([]interface{}, 0, count) - nextMaxIDValue string - prevMinIDValue string + items = make([]interface{}, 0, count) + + // 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{ - Items: items, - Path: "api/v1/notifications", - NextMaxIDValue: nextMaxIDValue, - PrevMinIDValue: prevMinIDValue, - Limit: limit, - }) + // 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", + 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) { diff --git a/internal/processing/workers/surfacenotify_test.go b/internal/processing/workers/surfacenotify_test.go index dc445d0ac..52ee89e8b 100644 --- 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())