Skip to content

Commit 3dc1df1

Browse files
lhk0504LinkinStars
authored andcommitted
fix: only decrease the notification red dot cache once per notification
Marking a notification as read is a no-op in the database when it is already read, but ClearIDUnRead decreased the cached red dot count unconditionally. Duplicate sequential requests therefore decreased the counter twice: the first request changed the notification from unread to read, while the second skipped the meaningful database transition but still decreased the cache. Two concurrent requests could do the same, because the update had no "is_read = unread" predicate and the changed row count was discarded. Make the update conditional and atomic (WHERE user_id = ? AND id = ? AND is_read = unread) and only decrease the cache counter when exactly one row was changed. The badge award alert cache cleanup stays independent, since it is already idempotent. Close #1643
1 parent 35ffe7a commit 3dc1df1

5 files changed

Lines changed: 222 additions & 10 deletions

File tree

‎internal/repo/notification/notification_repo.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,20 @@ func (nr *notificationRepo) ClearUnRead(ctx context.Context, userID string, noti
7676
return
7777
}
7878

79-
func (nr *notificationRepo) ClearIDUnRead(ctx context.Context, userID string, id string) (err error) {
79+
// ClearIDUnRead marks the notification as read only when it is still unread. It returns the
80+
// number of changed rows, so the caller can tell whether the notification really changed from
81+
// unread to read and avoid decreasing the red dot cache for duplicate requests.
82+
func (nr *notificationRepo) ClearIDUnRead(ctx context.Context, userID string, id string) (affected int64, err error) {
8083
info := &entity.Notification{}
8184
info.IsRead = schema.NotificationRead
82-
_, err = nr.data.DB.Context(ctx).Where("user_id = ?", userID).And("id = ?", id).Cols("is_read").Update(info)
85+
affected, err = nr.data.DB.Context(ctx).
86+
Where("user_id = ?", userID).
87+
And("id = ?", id).
88+
And("is_read = ?", schema.NotificationNotRead).
89+
Cols("is_read").
90+
Update(info)
8391
if err != nil {
84-
return errors.InternalServer(reason.DatabaseError).WithError(err).WithStack()
92+
err = errors.InternalServer(reason.DatabaseError).WithError(err).WithStack()
8593
}
8694
return
8795
}

‎internal/repo/repo_test/notification_repo_test.go‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ package repo_test
2121

2222
import (
2323
"context"
24+
"sync"
2425
"testing"
2526

2627
"github.com/apache/answer/internal/entity"
@@ -47,13 +48,60 @@ func Test_notificationRepo_ClearIDUnRead(t *testing.T) {
4748
err := notificationRepo.AddNotification(context.TODO(), ent)
4849
require.NoError(t, err)
4950

50-
err = notificationRepo.ClearIDUnRead(context.TODO(), ent.UserID, ent.ID)
51+
affected, err := notificationRepo.ClearIDUnRead(context.TODO(), ent.UserID, ent.ID)
5152
require.NoError(t, err)
53+
assert.EqualValues(t, 1, affected)
5254

5355
got, exists, err := notificationRepo.GetById(context.TODO(), ent.ID)
5456
require.NoError(t, err)
5557
assert.True(t, exists)
5658
assert.Equal(t, schema.NotificationRead, got.IsRead)
59+
60+
// a duplicate request must not report another unread to read transition
61+
affected, err = notificationRepo.ClearIDUnRead(context.TODO(), ent.UserID, ent.ID)
62+
require.NoError(t, err)
63+
assert.EqualValues(t, 0, affected)
64+
65+
// a notification of another user must not be changed
66+
affected, err = notificationRepo.ClearIDUnRead(context.TODO(), "another-user", ent.ID)
67+
require.NoError(t, err)
68+
assert.EqualValues(t, 0, affected)
69+
}
70+
71+
func Test_notificationRepo_ClearIDUnReadConcurrent(t *testing.T) {
72+
notificationRepo := notification.NewNotificationRepo(testDataSource)
73+
ent := buildNotificationEntity()
74+
err := notificationRepo.AddNotification(context.TODO(), ent)
75+
require.NoError(t, err)
76+
77+
const requestCount = 5
78+
var (
79+
waitGroup sync.WaitGroup
80+
mutex sync.Mutex
81+
affected int64
82+
errs []error
83+
)
84+
start := make(chan struct{})
85+
for i := 0; i < requestCount; i++ {
86+
waitGroup.Add(1)
87+
go func() {
88+
defer waitGroup.Done()
89+
<-start
90+
rows, err := notificationRepo.ClearIDUnRead(context.TODO(), ent.UserID, ent.ID)
91+
mutex.Lock()
92+
defer mutex.Unlock()
93+
if err != nil {
94+
errs = append(errs, err)
95+
return
96+
}
97+
affected += rows
98+
}()
99+
}
100+
close(start)
101+
waitGroup.Wait()
102+
103+
require.Empty(t, errs)
104+
assert.EqualValues(t, 1, affected, "only one concurrent request may change the notification")
57105
}
58106

59107
func Test_notificationRepo_ClearUnRead(t *testing.T) {

‎internal/service/notification/notification_service.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -190,18 +190,24 @@ func (ns *NotificationService) ClearIDUnRead(ctx context.Context, userID string,
190190
if !exist || notificationInfo.UserID != userID {
191191
return nil
192192
}
193-
if notificationInfo.IsRead == schema.NotificationNotRead {
194-
err := ns.notificationRepo.ClearIDUnRead(ctx, userID, id)
195-
if err != nil {
196-
return err
197-
}
193+
194+
// The update is conditional and atomic, it only changes the notification while it is still
195+
// unread and reports how many rows were changed.
196+
affected, err := ns.notificationRepo.ClearIDUnRead(ctx, userID, id)
197+
if err != nil {
198+
return err
198199
}
199200

200201
err = ns.notificationCommon.RemoveBadgeAwardAlertCache(ctx, userID, id)
201202
if err != nil {
202203
log.Errorf("remove badge award alert cache failed: %v", err)
203204
}
204205

206+
if affected == 0 {
207+
// The notification was already read, so the red dot cache must not be decreased again.
208+
return nil
209+
}
210+
205211
_ = ns.notificationCommon.DecreaseRedDot(ctx, userID, notificationInfo.Type)
206212
return nil
207213
}
Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package notification
21+
22+
import (
23+
"context"
24+
"fmt"
25+
"testing"
26+
"time"
27+
28+
"github.com/apache/answer/internal/base/constant"
29+
basedata "github.com/apache/answer/internal/base/data"
30+
"github.com/apache/answer/internal/entity"
31+
"github.com/apache/answer/internal/schema"
32+
"github.com/apache/answer/internal/service/noticequeue"
33+
notficationcommon "github.com/apache/answer/internal/service/notification_common"
34+
"github.com/segmentfault/pacman/cache"
35+
"github.com/stretchr/testify/assert"
36+
"github.com/stretchr/testify/require"
37+
)
38+
39+
// redDotTestNotificationRepo is an in-memory notification repo. Its ClearIDUnRead only changes
40+
// the notification while it is still unread and reports the changed rows, the same way the
41+
// database repo does.
42+
type redDotTestNotificationRepo struct {
43+
notifications map[string]*entity.Notification
44+
}
45+
46+
func (f *redDotTestNotificationRepo) AddNotification(_ context.Context, notification *entity.Notification) error {
47+
f.notifications[notification.ID] = notification
48+
return nil
49+
}
50+
51+
func (f *redDotTestNotificationRepo) GetNotificationPage(_ context.Context, _ *schema.NotificationSearch) (
52+
[]*entity.Notification, int64, error) {
53+
return nil, 0, nil
54+
}
55+
56+
func (f *redDotTestNotificationRepo) ClearUnRead(_ context.Context, _ string, _ int) error {
57+
return nil
58+
}
59+
60+
func (f *redDotTestNotificationRepo) ClearIDUnRead(_ context.Context, userID string, id string) (int64, error) {
61+
notification, exist := f.notifications[id]
62+
if !exist || notification.UserID != userID || notification.IsRead != schema.NotificationNotRead {
63+
return 0, nil
64+
}
65+
notification.IsRead = schema.NotificationRead
66+
return 1, nil
67+
}
68+
69+
func (f *redDotTestNotificationRepo) GetByUserIdObjectIdTypeId(_ context.Context, _, _ string, _ int) (
70+
*entity.Notification, bool, error) {
71+
return nil, false, nil
72+
}
73+
74+
func (f *redDotTestNotificationRepo) UpdateNotificationContent(_ context.Context, _ *entity.Notification) error {
75+
return nil
76+
}
77+
78+
func (f *redDotTestNotificationRepo) GetById(_ context.Context, id string) (*entity.Notification, bool, error) {
79+
notification, exist := f.notifications[id]
80+
return notification, exist, nil
81+
}
82+
83+
func (f *redDotTestNotificationRepo) CountNotificationByUser(_ context.Context, _ *entity.Notification) (int64, error) {
84+
return 0, nil
85+
}
86+
87+
func (f *redDotTestNotificationRepo) DeleteNotification(_ context.Context, _ string) error {
88+
return nil
89+
}
90+
91+
func (f *redDotTestNotificationRepo) DeleteUserNotificationConfig(_ context.Context, _ string) error {
92+
return nil
93+
}
94+
95+
// TestNotificationService_ClearIDUnReadDecreaseRedDotOnce checks that reading the same
96+
// notification more than once only decreases the red dot cache once, because only the request
97+
// that really changes the notification from unread to read may decrease it.
98+
func TestNotificationService_ClearIDUnReadDecreaseRedDotOnce(t *testing.T) {
99+
ctx := context.Background()
100+
const (
101+
userID = "red-dot-user"
102+
notificationID = "100"
103+
)
104+
105+
cacheStore, cleanup, err := basedata.NewCache(&basedata.CacheConf{})
106+
require.NoError(t, err)
107+
t.Cleanup(cleanup)
108+
109+
testData := &basedata.Data{Cache: cacheStore}
110+
repo := &redDotTestNotificationRepo{notifications: map[string]*entity.Notification{}}
111+
info := &entity.Notification{
112+
ID: notificationID,
113+
UserID: userID,
114+
ObjectID: "1",
115+
Content: "{}",
116+
Type: schema.NotificationTypeInbox,
117+
IsRead: schema.NotificationNotRead,
118+
Status: schema.NotificationStatusNormal,
119+
}
120+
require.NoError(t, repo.AddNotification(ctx, info))
121+
122+
service := &NotificationService{
123+
data: testData,
124+
notificationRepo: repo,
125+
notificationCommon: notficationcommon.NewNotificationCommon(
126+
testData, repo, nil, nil, nil, nil, noticequeue.NewService(), nil, nil),
127+
}
128+
129+
// the user has 3 unread notifications and reads one of them
130+
redDotKey := fmt.Sprintf(constant.RedDotCacheKey, constant.NotificationTypeInbox, userID)
131+
require.NoError(t, cacheStore.SetInt64(ctx, redDotKey, 3, time.Hour))
132+
133+
require.NoError(t, service.ClearIDUnRead(ctx, userID, notificationID))
134+
assertRedDotCount(t, ctx, cacheStore, redDotKey, 2)
135+
136+
// a duplicate request must not decrease the red dot cache again
137+
require.NoError(t, service.ClearIDUnRead(ctx, userID, notificationID))
138+
assertRedDotCount(t, ctx, cacheStore, redDotKey, 2)
139+
140+
// another user can neither read the notification nor change the red dot cache
141+
require.NoError(t, service.ClearIDUnRead(ctx, "another-user", notificationID))
142+
assertRedDotCount(t, ctx, cacheStore, redDotKey, 2)
143+
}
144+
145+
func assertRedDotCount(t *testing.T, ctx context.Context, c cache.Cache, key string, expected int64) {
146+
count, exist, err := c.GetInt64(ctx, key)
147+
require.NoError(t, err)
148+
require.True(t, exist, "red dot cache key should exist")
149+
assert.Equal(t, expected, count)
150+
}

‎internal/service/notification_common/notification.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type NotificationRepo interface {
5050
AddNotification(ctx context.Context, notification *entity.Notification) (err error)
5151
GetNotificationPage(ctx context.Context, search *schema.NotificationSearch) ([]*entity.Notification, int64, error)
5252
ClearUnRead(ctx context.Context, userID string, notificationType int) (err error)
53-
ClearIDUnRead(ctx context.Context, userID string, id string) (err error)
53+
ClearIDUnRead(ctx context.Context, userID string, id string) (affected int64, err error)
5454
GetByUserIdObjectIdTypeId(ctx context.Context, userID, objectID string, notificationType int) (*entity.Notification, bool, error)
5555
UpdateNotificationContent(ctx context.Context, notification *entity.Notification) (err error)
5656
GetById(ctx context.Context, id string) (*entity.Notification, bool, error)

0 commit comments

Comments
 (0)