[bugfix] Rework notifs to use min_id for paging up (#1734)

docs/api/swagger.yaml

@@ -4574,6 +4574,18 @@ paths:
                 ````
             operationId: notifications
             parameters:
+                - description: Return only notifications *OLDER* than the given max notification ID. The notification with the specified ID will not be included in the response.
+                  in: query
+                  name: max_id
+                  type: string
+                - description: Return only notifications *newer* than the given since notification ID. The notification with the specified ID will not be included in the response.
+                  in: query
+                  name: since_id
+                  type: string
+                - description: Return only notifications *immediately newer* than the given since notification ID. The notification with the specified ID will not be included in the response.
+                  in: query
+                  name: min_id
+                  type: string
                 - default: 20
                   description: Number of notifications to return.
                   in: query
@@ -4584,16 +4596,6 @@ paths:
                     type: string
                   name: exclude_types
                   type: array
-                - description: Return only notifications *OLDER* than the given max status ID. The status with the specified ID will not be included in the response.
-                  in: query
-                  name: max_id
-                  type: string
-                - description: |-
-                    Return only notifications *NEWER* than the given since status ID.
-                    The status with the specified ID will not be included in the response.
-                  in: query
-                  name: since_id
-                  type: string
             produces:
                 - application/json
             responses:

internal/api/client/notifications/notifications.go

@@ -36,12 +36,10 @@
 
 	// ExcludeTypes is an array specifying notification types to exclude
 	ExcludeTypesKey = "exclude_types[]"
-	// MaxIDKey is the url query for setting a max notification ID to return
 	MaxIDKey        = "max_id"
-	// LimitKey is for specifying maximum number of notifications to return.
 	LimitKey        = "limit"
-	// SinceIDKey is for specifying the minimum notification ID to return.
 	SinceIDKey      = "since_id"
+	MinIDKey        = "min_id"
 )
 
 type Module struct {

internal/api/client/notifications/notificationsget.go

@@ -50,6 +50,29 @@
 //
 //	parameters:
 //	-
+//		name: max_id
+//		type: string
+//		description: >-
+//			Return only notifications *OLDER* than the given max notification ID.
+//			The notification with the specified ID will not be included in the response.
+//		in: query
+//		required: false
+//	-
+//		name: since_id
+//		type: string
+//		description: >-
+//			Return only notifications *newer* than the given since notification ID.
+//			The notification with the specified ID will not be included in the response.
+//		in: query
+//	-
+//		name: min_id
+//		type: string
+//		description: >-
+//			Return only notifications *immediately newer* than the given since notification ID.
+//			The notification with the specified ID will not be included in the response.
+//		in: query
+//		required: false
+//	-
 //		name: limit
 //		type: integer
 //		description: Number of notifications to return.
@@ -64,22 +87,6 @@
 //			description: Array of types of notifications to exclude (follow, favourite, reblog, mention, poll, follow_request)
 //		in: query
 //		required: false
-//	-
-//		name: max_id
-//		type: string
-//		description: >-
-//			Return only notifications *OLDER* than the given max status ID.
-//			The status with the specified ID will not be included in the response.
-//		in: query
-//		required: false
-//	-
-//		name: since_id
-//		type: string
-//		description: |-
-//			Return only notifications *NEWER* than the given since status ID.
-//			The status with the specified ID will not be included in the response.
-//		in: query
-//		required: false
 //
 //	security:
 //	- OAuth2 Bearer:
@@ -131,21 +138,15 @@ func (m *Module) NotificationsGETHandler(c *gin.Context) {
 		limit = int(i)
 	}
 
-	maxID := ""
-	maxIDString := c.Query(MaxIDKey)
-	if maxIDString != "" {
-		maxID = maxIDString
-	}
-
-	sinceID := ""
-	sinceIDString := c.Query(SinceIDKey)
-	if sinceIDString != "" {
-		sinceID = sinceIDString
-	}
-
-	excludeTypes := c.QueryArray(ExcludeTypesKey)
-
-	resp, errWithCode := m.processor.NotificationsGet(c.Request.Context(), authed, excludeTypes, limit, maxID, sinceID)
+	resp, errWithCode := m.processor.NotificationsGet(
+		c.Request.Context(),
+		authed,
+		c.Query(MaxIDKey),
+		c.Query(SinceIDKey),
+		c.Query(MinIDKey),
+		limit,
+		c.QueryArray(ExcludeTypesKey),
+	)
 	if errWithCode != nil {
 		apiutil.ErrorHandler(c, errWithCode, m.processor.InstanceGetV1)
 		return

internal/db/bundb/notification.go

@@ -23,6 +23,7 @@
 
 	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/state"
 	"github.com/uptrace/bun"
@@ -73,53 +74,93 @@ func (n *notificationDB) GetNotification(
 	}, notificationType, targetAccountID, originAccountID, statusID)
 }
 
-func (n *notificationDB) GetAccountNotifications(ctx context.Context, accountID string, excludeTypes []string, limit int, maxID string, sinceID string) ([]*gtsmodel.Notification, db.Error) {
+func (n *notificationDB) GetAccountNotifications(
+	ctx context.Context,
+	accountID string,
+	maxID string,
+	sinceID string,
+	minID string,
+	limit int,
+	excludeTypes []string,
+) ([]*gtsmodel.Notification, db.Error) {
 	// Ensure reasonable
 	if limit < 0 {
 		limit = 0
 	}
 
-	// Make a guess for slice size
-	notifIDs := make([]string, 0, limit)
+	// Make educated guess for slice size
+	var (
+		notifIDs    = make([]string, 0, limit)
+		frontToBack = true
+	)
 
 	q := n.conn.
 		NewSelect().
 		TableExpr("? AS ?", bun.Ident("notifications"), bun.Ident("notification")).
 		Column("notification.id")
 
-	if maxID != "" {
-		q = q.Where("? < ?", bun.Ident("notification.id"), maxID)
+	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 minID != "" {
+		// Return only notifs HIGHER (ie., newer) than minID.
+		q = q.Where("? > ?", bun.Ident("notification.id"), minID)
+
+		frontToBack = false // page up
+	}
+
 	for _, excludeType := range excludeTypes {
+		// Filter out unwanted notif types.
 		q = q.Where("? != ?", bun.Ident("notification.notification_type"), excludeType)
 	}
 
-	q = q.
-		Where("? = ?", bun.Ident("notification.target_account_id"), accountID).
-		Order("notification.id DESC")
+	// Return only notifs for this account.
+	q = q.Where("? = ?", bun.Ident("notification.target_account_id"), accountID)
 
-	if limit != 0 {
+	if limit > 0 {
 		q = q.Limit(limit)
 	}
 
+	if frontToBack {
+		// Page down.
+		q = q.Order("notification.id DESC")
+	} else {
+		// Page up.
+		q = q.Order("notification.id ASC")
+	}
+
 	if err := q.Scan(ctx, &notifIDs); err != nil {
 		return nil, n.conn.ProcessError(err)
 	}
 
-	notifs := make([]*gtsmodel.Notification, 0, limit)
+	if len(notifIDs) == 0 {
+		return nil, nil
+	}
 
-	// now we have the IDs, select the notifs one by one
-	// reason for this is that for each notif, we can instead get it from our cache if it's cached
+	// 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]
+		}
+	}
+
+	notifs := make([]*gtsmodel.Notification, 0, len(notifIDs))
 	for _, id := range notifIDs {
 		// Attempt fetch from DB
 		notif, err := n.GetNotificationByID(ctx, id)
 		if err != nil {
-			log.Errorf(ctx, "error getting notification %q: %v", id, err)
+			log.Errorf(ctx, "error fetching notification %q: %v", id, err)
 			continue
 		}
 

internal/db/bundb/notification_test.go

@@ -89,7 +89,7 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithSpam() {
 	suite.spamNotifs()
 	testAccount := suite.testAccounts["local_account_1"]
 	before := time.Now()
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
 	timeTaken := time.Since(before)
 	fmt.Printf("\n\n\n withSpam: got %d notifications in %s\n\n\n", len(notifications), timeTaken)
@@ -103,7 +103,7 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithSpam() {
 func (suite *NotificationTestSuite) TestGetAccountNotificationsWithoutSpam() {
 	testAccount := suite.testAccounts["local_account_1"]
 	before := time.Now()
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
 	timeTaken := time.Since(before)
 	fmt.Printf("\n\n\n withoutSpam: got %d notifications in %s\n\n\n", len(notifications), timeTaken)
@@ -120,9 +120,9 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithSpam() {
 	err := suite.db.DeleteNotifications(context.Background(), nil, testAccount.ID, "")
 	suite.NoError(err)
 
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
-	suite.NotNil(notifications)
+	suite.Nil(notifications)
 	suite.Empty(notifications)
 }
 
@@ -132,9 +132,9 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithTwoAccounts() {
 	err := suite.db.DeleteNotifications(context.Background(), nil, testAccount.ID, "")
 	suite.NoError(err)
 
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
-	suite.NotNil(notifications)
+	suite.Nil(notifications)
 	suite.Empty(notifications)
 
 	notif := []*gtsmodel.Notification{}

internal/db/notification.go

@@ -28,7 +28,7 @@ type Notification interface {
 	// GetNotifications returns a slice of notifications that pertain to the given accountID.
 	//
 	// Returned notifications will be ordered ID descending (ie., highest/newest to lowest/oldest).
-	GetAccountNotifications(ctx context.Context, accountID string, excludeTypes []string, limit int, maxID string, sinceID string) ([]*gtsmodel.Notification, Error)
+	GetAccountNotifications(ctx context.Context, accountID string, maxID string, sinceID string, minID string, limit int, excludeTypes []string) ([]*gtsmodel.Notification, Error)
 
 	// GetNotification returns one notification according to its id.
 	GetNotificationByID(ctx context.Context, id string) (*gtsmodel.Notification, Error)

internal/processing/notification.go

@@ -31,34 +31,69 @@
 	"github.com/superseriousbusiness/gotosocial/internal/util"
 )
 
-func (p *Processor) NotificationsGet(ctx context.Context, authed *oauth.Auth, excludeTypes []string, limit int, maxID string, sinceID string) (*apimodel.PageableResponse, gtserror.WithCode) {
-	notifs, err := p.state.DB.GetAccountNotifications(ctx, authed.Account.ID, excludeTypes, limit, maxID, sinceID)
+func (p *Processor) NotificationsGet(ctx context.Context, authed *oauth.Auth, maxID string, sinceID string, minID string, limit int, excludeTypes []string) (*apimodel.PageableResponse, gtserror.WithCode) {
+	notifs, err := p.state.DB.GetAccountNotifications(ctx, authed.Account.ID, maxID, sinceID, minID, limit, excludeTypes)
 	if err != nil {
+		if errors.Is(err, db.ErrNoEntries) {
+			// No notifs (left).
+			return util.EmptyPageableResponse(), nil
+		}
+		// An actual error has occurred.
+		err = fmt.Errorf("NotificationsGet: db error getting notifications: %w", err)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	count := len(notifs)
-
 	if count == 0 {
 		return util.EmptyPageableResponse(), nil
 	}
 
-	items := make([]interface{}, 0, count)
-	nextMaxIDValue := ""
-	prevMinIDValue := ""
-	for i, n := range notifs {
-		item, err := p.tc.NotificationToAPINotification(ctx, n)
-		if err != nil {
-			log.Debugf(ctx, "got an error converting a notification to api, will skip it: %s", err)
-			continue
-		}
+	var (
+		items          = make([]interface{}, 0, count)
+		nextMaxIDValue string
+		prevMinIDValue string
+	)
 
+	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 = item.GetID()
+			nextMaxIDValue = n.ID
 		}
 
 		if i == 0 {
-			prevMinIDValue = item.GetID()
+			prevMinIDValue = n.ID
+		}
+
+		// Ensure this notification should be shown to requester.
+		if n.OriginAccount != nil {
+			// Account is set, ensure it's visible to notif target.
+			visible, err := p.filter.AccountVisible(ctx, authed.Account, n.OriginAccount)
+			if err != nil {
+				log.Debugf(ctx, "skipping notification %s because of an error checking notification visibility: %s", n.ID, err)
+				continue
+			}
+			if !visible {
+				continue
+			}
+		}
+
+		if n.Status != nil {
+			// Status is set, ensure it's visible to notif target.
+			visible, err := p.filter.StatusVisible(ctx, authed.Account, n.Status)
+			if err != nil {
+				log.Debugf(ctx, "skipping notification %s because of an error checking notification visibility: %s", n.ID, err)
+				continue
+			}
+			if !visible {
+				continue
+			}
+		}
+
+		item, err := p.tc.NotificationToAPINotification(ctx, n)
+		if err != nil {
+			log.Debugf(ctx, "skipping notification %s because it couldn't be converted to its api representation: %s", n.ID, err)
+			continue
 		}
 
 		items = append(items, item)
@@ -68,7 +103,6 @@ func (p *Processor) NotificationsGet(ctx context.Context, authed *oauth.Auth, ex
 		Items:          items,
 		Path:           "api/v1/notifications",
 		NextMaxIDValue: nextMaxIDValue,
-		PrevMinIDKey:   "since_id",
 		PrevMinIDValue: prevMinIDValue,
 		Limit:          limit,
 	})

internal/processing/notification_test.go

@@ -32,7 +32,7 @@ type NotificationTestSuite struct {
 // get a notification where someone has liked our status
 func (suite *NotificationTestSuite) TestGetNotifications() {
 	receivingAccount := suite.testAccounts["local_account_1"]
-	notifsResponse, err := suite.processor.NotificationsGet(context.Background(), suite.testAutheds["local_account_1"], []string{}, 10, "", "")
+	notifsResponse, err := suite.processor.NotificationsGet(context.Background(), suite.testAutheds["local_account_1"], "", "", "", 10, nil)
 	suite.NoError(err)
 	suite.Len(notifsResponse.Items, 1)
 	notif, ok := notifsResponse.Items[0].(*apimodel.Notification)
@@ -44,7 +44,7 @@ func (suite *NotificationTestSuite) TestGetNotifications() {
 	suite.NotNil(notif.Status)
 	suite.NotNil(notif.Status.Account)
 	suite.Equal(receivingAccount.ID, notif.Status.Account.ID)
-	suite.Equal(`<http://localhost:8080/api/v1/notifications?limit=10&max_id=01F8Q0ANPTWW10DAKTX7BRPBJP>; rel="next", <http://localhost:8080/api/v1/notifications?limit=10&since_id=01F8Q0ANPTWW10DAKTX7BRPBJP>; rel="prev"`, notifsResponse.LinkHeader)
+	suite.Equal(`<http://localhost:8080/api/v1/notifications?limit=10&max_id=01F8Q0ANPTWW10DAKTX7BRPBJP>; rel="next", <http://localhost:8080/api/v1/notifications?limit=10&min_id=01F8Q0ANPTWW10DAKTX7BRPBJP>; rel="prev"`, notifsResponse.LinkHeader)
 }
 
 func TestNotificationTestSuite(t *testing.T) {