feat: rework notification table (#9926)

This change is motivated by 5e300a2a87

- Drop the `updated_by` and `commit_id` column, they are unused and have a index for no reason.
- Drop the index on `status` and `created_unix` and make a index on `(user_id, status)`.

## Test
1. Run migration.
2. Confirm the migration succeeds.
3. Check that `notification` table has the correct indexes.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9926
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>
This commit is contained in:
Gusted 2025-11-29 23:03:56 +01:00 committed by Gusted
commit d1cef852ee
12 changed files with 175 additions and 113 deletions

View file

@ -6,7 +6,6 @@ package activities
import (
"context"
"fmt"
"net/url"
"strconv"
"forgejo.org/models/db"
@ -51,24 +50,21 @@ const (
// Notification represents a notification
type Notification struct {
ID int64 `xorm:"pk autoincr"`
UserID int64 `xorm:"INDEX NOT NULL"`
UserID int64 `xorm:"NOT NULL INDEX(s)"`
RepoID int64 `xorm:"INDEX NOT NULL"`
Status NotificationStatus `xorm:"SMALLINT INDEX NOT NULL"`
Status NotificationStatus `xorm:"SMALLINT NOT NULL INDEX(s)"`
Source NotificationSource `xorm:"SMALLINT INDEX NOT NULL"`
IssueID int64 `xorm:"INDEX NOT NULL"`
CommitID string `xorm:"INDEX"`
IssueID int64 `xorm:"INDEX NOT NULL"`
CommentID int64
UpdatedBy int64 `xorm:"INDEX NOT NULL"`
Issue *issues_model.Issue `xorm:"-"`
Repository *repo_model.Repository `xorm:"-"`
Comment *issues_model.Comment `xorm:"-"`
User *user_model.User `xorm:"-"`
CreatedUnix timeutil.TimeStamp `xorm:"created INDEX NOT NULL"`
CreatedUnix timeutil.TimeStamp `xorm:"created NOT NULL"`
UpdatedUnix timeutil.TimeStamp `xorm:"updated INDEX NOT NULL"`
}
@ -88,20 +84,18 @@ func CreateRepoTransferNotification(ctx context.Context, doer, newOwner *user_mo
}
for i := range users {
notify = append(notify, &Notification{
UserID: i,
RepoID: repo.ID,
Status: NotificationStatusUnread,
UpdatedBy: doer.ID,
Source: NotificationSourceRepository,
UserID: i,
RepoID: repo.ID,
Status: NotificationStatusUnread,
Source: NotificationSourceRepository,
})
}
} else {
notify = []*Notification{{
UserID: newOwner.ID,
RepoID: repo.ID,
Status: NotificationStatusUnread,
UpdatedBy: doer.ID,
Source: NotificationSourceRepository,
UserID: newOwner.ID,
RepoID: repo.ID,
Status: NotificationStatusUnread,
Source: NotificationSourceRepository,
}}
}
@ -109,14 +103,13 @@ func CreateRepoTransferNotification(ctx context.Context, doer, newOwner *user_mo
})
}
func createIssueNotification(ctx context.Context, userID int64, issue *issues_model.Issue, commentID, updatedByID int64) error {
func createIssueNotification(ctx context.Context, userID int64, issue *issues_model.Issue, commentID int64) error {
notification := &Notification{
UserID: userID,
RepoID: issue.RepoID,
Status: NotificationStatusUnread,
IssueID: issue.ID,
CommentID: commentID,
UpdatedBy: updatedByID,
}
if issue.IsPull {
@ -128,25 +121,23 @@ func createIssueNotification(ctx context.Context, userID int64, issue *issues_mo
return db.Insert(ctx, notification)
}
func updateIssueNotification(ctx context.Context, userID, issueID, commentID, updatedByID int64) error {
func updateIssueNotification(ctx context.Context, userID, issueID, commentID int64) error {
notification, err := GetIssueNotification(ctx, userID, issueID)
if err != nil {
return err
}
// NOTICE: Only update comment id when the before notification on this issue is read, otherwise you may miss some old comments.
// But we need update update_by so that the notification will be reorder
var cols []string
if notification.Status == NotificationStatusRead {
notification.Status = NotificationStatusUnread
notification.CommentID = commentID
cols = []string{"status", "update_by", "comment_id"}
} else {
notification.UpdatedBy = updatedByID
cols = []string{"update_by"}
// If the notification is not yet read, then only update the update_unix column,
// so that the notification does not point to a newer comment.
if notification.Status != NotificationStatusRead {
notification.UpdatedUnix = timeutil.TimeStampNow()
_, err = db.GetEngine(ctx).ID(notification.ID).Cols("updated_unix").NoAutoTime().Update(notification)
return err
}
_, err = db.GetEngine(ctx).ID(notification.ID).Cols(cols...).Update(notification)
notification.Status = NotificationStatusUnread
notification.CommentID = commentID
_, err = db.GetEngine(ctx).ID(notification.ID).Cols("status", "comment_id").Update(notification)
return err
}
@ -242,8 +233,6 @@ func (n *Notification) HTMLURL(ctx context.Context) string {
return n.Comment.HTMLURL(ctx)
}
return n.Issue.HTMLURL()
case NotificationSourceCommit:
return n.Repository.HTMLURL() + "/commit/" + url.PathEscape(n.CommitID)
case NotificationSourceRepository:
return n.Repository.HTMLURL()
}
@ -258,8 +247,6 @@ func (n *Notification) Link(ctx context.Context) string {
return n.Comment.Link(ctx)
}
return n.Issue.Link()
case NotificationSourceCommit:
return n.Repository.Link() + "/commit/" + url.PathEscape(n.CommitID)
case NotificationSourceRepository:
return n.Repository.Link()
}
@ -370,10 +357,10 @@ func GetNotificationByID(ctx context.Context, notificationID int64) (*Notificati
// UpdateNotificationStatuses updates the statuses of all of a user's notifications that are of the currentStatus type to the desiredStatus
func UpdateNotificationStatuses(ctx context.Context, user *user_model.User, currentStatus, desiredStatus NotificationStatus) error {
n := &Notification{Status: desiredStatus, UpdatedBy: user.ID}
n := &Notification{Status: desiredStatus}
_, err := db.GetEngine(ctx).
Where("user_id = ? AND status = ?", user.ID, currentStatus).
Cols("status", "updated_by", "updated_unix").
Cols("status", "updated_unix").
Update(n)
return err
}

View file

@ -164,12 +164,12 @@ func createOrUpdateIssueNotifications(ctx context.Context, issueID, commentID, n
}
if notificationExists(notifications, issue.ID, userID) {
if err = updateIssueNotification(ctx, userID, issue.ID, commentID, notificationAuthorID); err != nil {
if err = updateIssueNotification(ctx, userID, issue.ID, commentID); err != nil {
return err
}
continue
}
if err = createIssueNotification(ctx, userID, issue, commentID, notificationAuthorID); err != nil {
if err = createIssueNotification(ctx, userID, issue, commentID); err != nil {
return err
}
}

View file

@ -1,17 +1,19 @@
// Copyright 2017 The Gitea Authors. All rights reserved.
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package activities_test
package activities
import (
"context"
"testing"
"time"
activities_model "forgejo.org/models/activities"
"forgejo.org/models/db"
issues_model "forgejo.org/models/issues"
"forgejo.org/models/unittest"
user_model "forgejo.org/models/user"
"forgejo.org/modules/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -21,25 +23,25 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
require.NoError(t, activities_model.CreateOrUpdateIssueNotifications(db.DefaultContext, issue.ID, 0, 2, 0))
require.NoError(t, CreateOrUpdateIssueNotifications(db.DefaultContext, issue.ID, 0, 2, 0))
// User 9 is inactive, thus notifications for user 1 and 4 are created
notf := unittest.AssertExistsAndLoadBean(t, &activities_model.Notification{UserID: 1, IssueID: issue.ID})
assert.Equal(t, activities_model.NotificationStatusUnread, notf.Status)
notf := unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID})
assert.Equal(t, NotificationStatusUnread, notf.Status)
unittest.CheckConsistencyFor(t, &issues_model.Issue{ID: issue.ID})
notf = unittest.AssertExistsAndLoadBean(t, &activities_model.Notification{UserID: 4, IssueID: issue.ID})
assert.Equal(t, activities_model.NotificationStatusUnread, notf.Status)
notf = unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 4, IssueID: issue.ID})
assert.Equal(t, NotificationStatusUnread, notf.Status)
}
func TestNotificationsForUser(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
notfs, err := db.Find[activities_model.Notification](db.DefaultContext, activities_model.FindNotificationOptions{
notfs, err := db.Find[Notification](db.DefaultContext, FindNotificationOptions{
UserID: user.ID,
Status: []activities_model.NotificationStatus{
activities_model.NotificationStatusRead,
activities_model.NotificationStatusUnread,
Status: []NotificationStatus{
NotificationStatusRead,
NotificationStatusUnread,
},
})
require.NoError(t, err)
@ -55,7 +57,7 @@ func TestNotificationsForUser(t *testing.T) {
func TestNotification_GetRepo(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
notf := unittest.AssertExistsAndLoadBean(t, &activities_model.Notification{RepoID: 1})
notf := unittest.AssertExistsAndLoadBean(t, &Notification{RepoID: 1})
repo, err := notf.GetRepo(db.DefaultContext)
require.NoError(t, err)
assert.Equal(t, repo, notf.Repository)
@ -64,7 +66,7 @@ func TestNotification_GetRepo(t *testing.T) {
func TestNotification_GetIssue(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
notf := unittest.AssertExistsAndLoadBean(t, &activities_model.Notification{RepoID: 1})
notf := unittest.AssertExistsAndLoadBean(t, &Notification{RepoID: 1})
issue, err := notf.GetIssue(db.DefaultContext)
require.NoError(t, err)
assert.Equal(t, issue, notf.Issue)
@ -74,19 +76,19 @@ func TestNotification_GetIssue(t *testing.T) {
func TestGetNotificationCount(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
cnt, err := db.Count[activities_model.Notification](db.DefaultContext, activities_model.FindNotificationOptions{
cnt, err := db.Count[Notification](db.DefaultContext, FindNotificationOptions{
UserID: user.ID,
Status: []activities_model.NotificationStatus{
activities_model.NotificationStatusRead,
Status: []NotificationStatus{
NotificationStatusRead,
},
})
require.NoError(t, err)
assert.EqualValues(t, 0, cnt)
cnt, err = db.Count[activities_model.Notification](db.DefaultContext, activities_model.FindNotificationOptions{
cnt, err = db.Count[Notification](db.DefaultContext, FindNotificationOptions{
UserID: user.ID,
Status: []activities_model.NotificationStatus{
activities_model.NotificationStatusUnread,
Status: []NotificationStatus{
NotificationStatusUnread,
},
})
require.NoError(t, err)
@ -97,15 +99,15 @@ func TestSetNotificationStatus(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
notf := unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{UserID: user.ID, Status: activities_model.NotificationStatusRead})
_, err := activities_model.SetNotificationStatus(db.DefaultContext, notf.ID, user, activities_model.NotificationStatusPinned)
&Notification{UserID: user.ID, Status: NotificationStatusRead})
_, err := SetNotificationStatus(db.DefaultContext, notf.ID, user, NotificationStatusPinned)
require.NoError(t, err)
unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{ID: notf.ID, Status: activities_model.NotificationStatusPinned})
&Notification{ID: notf.ID, Status: NotificationStatusPinned})
_, err = activities_model.SetNotificationStatus(db.DefaultContext, 1, user, activities_model.NotificationStatusRead)
_, err = SetNotificationStatus(db.DefaultContext, 1, user, NotificationStatusRead)
require.Error(t, err)
_, err = activities_model.SetNotificationStatus(db.DefaultContext, unittest.NonexistentID, user, activities_model.NotificationStatusRead)
_, err = SetNotificationStatus(db.DefaultContext, unittest.NonexistentID, user, NotificationStatusRead)
require.Error(t, err)
}
@ -113,18 +115,18 @@ func TestUpdateNotificationStatuses(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
notfUnread := unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{UserID: user.ID, Status: activities_model.NotificationStatusUnread})
&Notification{UserID: user.ID, Status: NotificationStatusUnread})
notfRead := unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{UserID: user.ID, Status: activities_model.NotificationStatusRead})
&Notification{UserID: user.ID, Status: NotificationStatusRead})
notfPinned := unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{UserID: user.ID, Status: activities_model.NotificationStatusPinned})
require.NoError(t, activities_model.UpdateNotificationStatuses(db.DefaultContext, user, activities_model.NotificationStatusUnread, activities_model.NotificationStatusRead))
&Notification{UserID: user.ID, Status: NotificationStatusPinned})
require.NoError(t, UpdateNotificationStatuses(db.DefaultContext, user, NotificationStatusUnread, NotificationStatusRead))
unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{ID: notfUnread.ID, Status: activities_model.NotificationStatusRead})
&Notification{ID: notfUnread.ID, Status: NotificationStatusRead})
unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{ID: notfRead.ID, Status: activities_model.NotificationStatusRead})
&Notification{ID: notfRead.ID, Status: NotificationStatusRead})
unittest.AssertExistsAndLoadBean(t,
&activities_model.Notification{ID: notfPinned.ID, Status: activities_model.NotificationStatusPinned})
&Notification{ID: notfPinned.ID, Status: NotificationStatusPinned})
}
func TestSetIssueReadBy(t *testing.T) {
@ -132,10 +134,44 @@ func TestSetIssueReadBy(t *testing.T) {
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
require.NoError(t, db.WithTx(db.DefaultContext, func(ctx context.Context) error {
return activities_model.SetIssueReadBy(ctx, issue.ID, user.ID)
return SetIssueReadBy(ctx, issue.ID, user.ID)
}))
nt, err := activities_model.GetIssueNotification(db.DefaultContext, user.ID, issue.ID)
nt, err := GetIssueNotification(db.DefaultContext, user.ID, issue.ID)
require.NoError(t, err)
assert.Equal(t, activities_model.NotificationStatusRead, nt.Status)
assert.Equal(t, NotificationStatusRead, nt.Status)
}
func TestUpdateIssueNotification(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
now := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
timeutil.MockSet(now)
defer timeutil.MockUnset()
t.Run("Read notification", func(t *testing.T) {
require.NoError(t, updateIssueNotification(t.Context(), 1, 1, 1001))
notification := unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: 1})
assert.Equal(t, NotificationStatusUnread, notification.Status)
assert.EqualValues(t, 0, notification.CommentID)
assert.Equal(t, timeutil.TimeStamp(now.Unix()), notification.UpdatedUnix)
})
t.Run("Unread notification", func(t *testing.T) {
beforeUpdateUnix := unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 2, IssueID: 2}).UpdatedUnix
require.NoError(t, updateIssueNotification(t.Context(), 2, 2, 1001))
notification := unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 2, IssueID: 2})
assert.Equal(t, NotificationStatusUnread, notification.Status)
assert.EqualValues(t, 1001, notification.CommentID)
assert.NotEqual(t, beforeUpdateUnix, notification.UpdatedUnix)
})
t.Run("Pinned notification", func(t *testing.T) {
require.NoError(t, updateIssueNotification(t.Context(), 1, 1, 1001))
notification := unittest.AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: 1})
assert.Equal(t, NotificationStatusUnread, notification.Status)
assert.EqualValues(t, 0, notification.CommentID)
assert.Equal(t, timeutil.TimeStamp(now.Unix()), notification.UpdatedUnix)
})
}

View file

@ -4,7 +4,6 @@
repo_id: 1
status: 1 # unread
source: 1 # issue
updated_by: 2
issue_id: 1
created_unix: 946684800
updated_unix: 946684820
@ -15,7 +14,6 @@
repo_id: 1
status: 2 # read
source: 1 # issue
updated_by: 1
issue_id: 2
created_unix: 946685800
updated_unix: 946685820
@ -26,7 +24,6 @@
repo_id: 1
status: 3 # pinned
source: 1 # issue
updated_by: 1
issue_id: 3
created_unix: 946686800
updated_unix: 946686800
@ -37,7 +34,6 @@
repo_id: 1
status: 1 # unread
source: 1 # issue
updated_by: 1
issue_id: 5
created_unix: 946687800
updated_unix: 946687800
@ -48,7 +44,6 @@
repo_id: 2
status: 1 # unread
source: 1 # issue
updated_by: 5
issue_id: 4
created_unix: 946688800
updated_unix: 946688820

View file

@ -21,7 +21,6 @@ func TestDropIndexIfExists(t *testing.T) {
}
x, deferable := migration_tests.PrepareTestEnv(t, 0, new(Table))
x.ShowSQL(true)
defer deferable()
if x == nil || t.Failed() {
return

View file

@ -0,0 +1,65 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations
import (
activity_model "forgejo.org/models/activities"
"forgejo.org/modules/setting"
"xorm.io/xorm"
)
func init() {
registerMigration(&Migration{
Description: "remove columns and rework indexes for notification table",
Upgrade: reworkNotification,
})
}
func reworkNotification(x *xorm.Engine) error {
type Notification struct {
UserID int64 `xorm:"NOT NULL INDEX(s)"`
Status activity_model.NotificationStatus `xorm:"SMALLINT NOT NULL INDEX(s)"`
}
if err := dropIndexIfExists(x, "notification", "IDX_notification_user_id"); err != nil {
return err
}
if err := dropIndexIfExists(x, "notification", "IDX_notification_created_unix"); err != nil {
return err
}
if err := dropIndexIfExists(x, "notification", "IDX_notification_updated_by"); err != nil {
return err
}
if err := dropIndexIfExists(x, "notification", "IDX_notification_commit_id"); err != nil {
return err
}
if err := dropIndexIfExists(x, "notification", "IDX_notification_status"); err != nil {
return err
}
switch {
case setting.Database.Type.IsSQLite3():
if _, err := x.Exec("ALTER TABLE `notification` DROP COLUMN `updated_by`"); err != nil {
return err
}
if _, err := x.Exec("ALTER TABLE `notification` DROP COLUMN `commit_id`"); err != nil {
return err
}
case setting.Database.Type.IsMySQL(), setting.Database.Type.IsPostgreSQL():
if _, err := x.Exec("ALTER TABLE `notification` DROP COLUMN `updated_by`, DROP COLUMN `commit_id`"); err != nil {
return err
}
}
_, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, new(Notification))
return err
}

View file

@ -67,8 +67,6 @@ func subjectToSource(value []string) (result []activities_model.NotificationSour
result = append(result, activities_model.NotificationSourceIssue)
case "pull":
result = append(result, activities_model.NotificationSourcePullRequest)
case "commit":
result = append(result, activities_model.NotificationSourceCommit)
case "repository":
result = append(result, activities_model.NotificationSourceRepository)
}

View file

@ -80,7 +80,7 @@ func ListRepoNotifications(ctx *context.APIContext) {
// collectionFormat: multi
// items:
// type: string
// enum: [issue,pull,commit,repository]
// enum: [issue,pull,repository]
// - name: since
// in: query
// description: Only show notifications updated after the given time. This is a timestamp in RFC 3339 format

View file

@ -42,7 +42,7 @@ func ListNotifications(ctx *context.APIContext) {
// collectionFormat: multi
// items:
// type: string
// enum: [issue,pull,commit,repository]
// enum: [issue,pull,repository]
// - name: since
// in: query
// description: Only show notifications updated after the given time. This is a timestamp in RFC 3339 format

View file

@ -109,24 +109,17 @@ func getNotifications(ctx *context.Context) {
return
}
sess := db.GetEngine(ctx).Table("notification")
if setting.Database.Type.IsMySQL() {
sess = sess.IndexHint("USE", "", "IDX_notification_user_id")
}
sess.Where("user_id = ?", ctx.Doer.ID).
And("status = ? OR status = ?", status, activities_model.NotificationStatusPinned).
OrderBy("notification.updated_unix DESC")
if perPage > 0 {
if page == 0 {
page = 1
}
sess.Limit(perPage, (page-1)*perPage)
}
nls := make([]*activities_model.Notification, 0, perPage)
if err := sess.Find(&nls); err != nil {
ctx.ServerError("FindNotifications", err)
statuses := []activities_model.NotificationStatus{status, activities_model.NotificationStatusPinned}
nls, err := db.Find[activities_model.Notification](ctx, activities_model.FindNotificationOptions{
ListOptions: db.ListOptions{
PageSize: perPage,
Page: page,
},
UserID: ctx.Doer.ID,
Status: statuses,
})
if err != nil {
ctx.ServerError("db.Find[activities_model.Notification]", err)
return
}
notifications := activities_model.NotificationList(nls)

View file

@ -5,7 +5,6 @@ package convert
import (
"context"
"net/url"
activities_model "forgejo.org/models/activities"
"forgejo.org/models/perm"
@ -67,14 +66,6 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification)
result.Subject.State = "merged"
}
}
case activities_model.NotificationSourceCommit:
url := n.Repository.HTMLURL() + "/commit/" + url.PathEscape(n.CommitID)
result.Subject = &api.NotificationSubject{
Type: api.NotifySubjectCommit,
Title: n.CommitID,
URL: url,
HTMLURL: url,
}
case activities_model.NotificationSourceRepository:
result.Subject = &api.NotificationSubject{
Type: api.NotifySubjectRepository,

View file

@ -2192,7 +2192,6 @@
"enum": [
"issue",
"pull",
"commit",
"repository"
],
"type": "string"
@ -13520,7 +13519,6 @@
"enum": [
"issue",
"pull",
"commit",
"repository"
],
"type": "string"