fix(notifications): refresh embedded users when reading notifications
Notifications stored before the acting user was resolved with its full profile (#2720) were serialized with only id+username, so they kept rendering the auto-generated username instead of the display name. Reload every embedded user from the database when reading a user's notifications, healing already-stored rows at read time. The refresh is not persisted; a per-page cache fetches each user once.
This commit is contained in:
kolaente
kolaente
parent
7b7c850dd8
commit
7f53be4105
|
|
@ -80,6 +80,9 @@ linters:
|
|||
- linters:
|
||||
- exhaustive
|
||||
path: pkg/models/task_collection_filter\.go
|
||||
- linters:
|
||||
- exhaustive
|
||||
path: pkg/models/notifications_refresh\.go
|
||||
- linters:
|
||||
- gosec
|
||||
path: pkg/utils/random_string\.go
|
||||
|
|
|
|||
|
|
@ -53,7 +53,13 @@ func (d *DatabaseNotifications) ReadAll(s *xorm.Session, a web.Auth, _ string, p
|
|||
}
|
||||
|
||||
limit, start := getLimitFromPageIndex(page, perPage)
|
||||
return notifications.GetNotificationsForUser(s, a.GetID(), limit, start)
|
||||
ns, resultCount, total, err := notifications.GetNotificationsForUser(s, a.GetID(), limit, start)
|
||||
if err != nil {
|
||||
return nil, 0, 0, err
|
||||
}
|
||||
|
||||
refreshNotificationsUsers(s, ns)
|
||||
return ns, resultCount, total, nil
|
||||
}
|
||||
|
||||
// CanUpdate checks if a user can mark a notification as read.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,122 @@
|
|||
// Vikunja is a to-do list application to facilitate your life.
|
||||
// Copyright 2018-present Vikunja and contributors. All rights reserved.
|
||||
//
|
||||
// This program is free software: you can redistribute it and/or modify
|
||||
// it under the terms of the GNU Affero General Public License as published by
|
||||
// the Free Software Foundation, either version 3 of the License, or
|
||||
// (at your option) any later version.
|
||||
//
|
||||
// This program is distributed in the hope that it will be useful,
|
||||
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
// GNU Affero General Public License for more details.
|
||||
//
|
||||
// You should have received a copy of the GNU Affero General Public License
|
||||
// along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
|
||||
package models
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"reflect"
|
||||
|
||||
"code.vikunja.io/api/pkg/log"
|
||||
"code.vikunja.io/api/pkg/notifications"
|
||||
"code.vikunja.io/api/pkg/user"
|
||||
|
||||
"xorm.io/xorm"
|
||||
)
|
||||
|
||||
// maxNotificationUserRefreshDepth bounds the reflection walk so an unexpectedly
|
||||
// deep payload cannot recurse without end.
|
||||
const maxNotificationUserRefreshDepth = 8
|
||||
|
||||
// refreshNotificationsUsers reloads every embedded user of each notification
|
||||
// from the database. Notifications serialized before the acting user was
|
||||
// resolved with its full profile (#2720) stored only id+username, so without
|
||||
// this they keep rendering the auto-generated username instead of the display
|
||||
// name. It runs at read time and is not persisted; one cache is shared across
|
||||
// the batch so a user recurring across notifications is fetched only once.
|
||||
func refreshNotificationsUsers(s *xorm.Session, dbNotifications []*notifications.DatabaseNotification) {
|
||||
cache := make(map[int64]*user.User)
|
||||
for _, dbn := range dbNotifications {
|
||||
refreshNotificationUsers(s, dbn, cache)
|
||||
}
|
||||
}
|
||||
|
||||
func refreshNotificationUsers(s *xorm.Session, dbn *notifications.DatabaseNotification, cache map[int64]*user.User) {
|
||||
typed, ok := notifications.Lookup(dbn.Name)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
raw, err := json.Marshal(dbn.Notification)
|
||||
if err != nil {
|
||||
log.Errorf("Could not marshal notification %d to refresh its users: %v", dbn.ID, err)
|
||||
return
|
||||
}
|
||||
if err := json.Unmarshal(raw, typed); err != nil {
|
||||
log.Errorf("Could not unmarshal notification %d to refresh its users: %v", dbn.ID, err)
|
||||
return
|
||||
}
|
||||
|
||||
refreshUsersInValue(s, reflect.ValueOf(typed), cache, 0)
|
||||
dbn.Notification = typed
|
||||
}
|
||||
|
||||
func refreshUsersInValue(s *xorm.Session, v reflect.Value, cache map[int64]*user.User, depth int) {
|
||||
if depth > maxNotificationUserRefreshDepth || !v.IsValid() {
|
||||
return
|
||||
}
|
||||
|
||||
switch v.Kind() {
|
||||
case reflect.Ptr:
|
||||
if v.IsNil() {
|
||||
return
|
||||
}
|
||||
if u, is := v.Interface().(*user.User); is {
|
||||
refreshUser(s, u, cache)
|
||||
return
|
||||
}
|
||||
refreshUsersInValue(s, v.Elem(), cache, depth+1)
|
||||
case reflect.Struct:
|
||||
for i := 0; i < v.NumField(); i++ {
|
||||
if !v.Type().Field(i).IsExported() {
|
||||
continue
|
||||
}
|
||||
refreshUsersInValue(s, v.Field(i), cache, depth+1)
|
||||
}
|
||||
case reflect.Slice, reflect.Array:
|
||||
for i := 0; i < v.Len(); i++ {
|
||||
refreshUsersInValue(s, v.Index(i), cache, depth+1)
|
||||
}
|
||||
case reflect.Map:
|
||||
for _, key := range v.MapKeys() {
|
||||
refreshUsersInValue(s, v.MapIndex(key), cache, depth+1)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// refreshUser overwrites the user in place with its current database row. A
|
||||
// disabled or locked account is still returned fully populated, so only a
|
||||
// missing user or a real database error leaves the stored value untouched.
|
||||
func refreshUser(s *xorm.Session, u *user.User, cache map[int64]*user.User) {
|
||||
if u == nil || u.ID == 0 {
|
||||
return
|
||||
}
|
||||
|
||||
fresh, cached := cache[u.ID]
|
||||
if !cached {
|
||||
loaded, err := user.GetUserByID(s, u.ID)
|
||||
if err != nil && !user.IsErrUserStatusError(err) {
|
||||
cache[u.ID] = nil
|
||||
return
|
||||
}
|
||||
fresh = loaded
|
||||
cache[u.ID] = fresh
|
||||
}
|
||||
|
||||
if fresh != nil {
|
||||
*u = *fresh
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,109 @@
|
|||
// Vikunja is a to-do list application to facilitate your life.
|
||||
// Copyright 2018-present Vikunja and contributors. All rights reserved.
|
||||
//
|
||||
// This program is free software: you can redistribute it and/or modify
|
||||
// it under the terms of the GNU Affero General Public License as published by
|
||||
// the Free Software Foundation, either version 3 of the License, or
|
||||
// (at your option) any later version.
|
||||
//
|
||||
// This program is distributed in the hope that it will be useful,
|
||||
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
||||
// GNU Affero General Public License for more details.
|
||||
//
|
||||
// You should have received a copy of the GNU Affero General Public License
|
||||
// along with this program. If not, see <https://www.gnu.org/licenses/>.
|
||||
|
||||
package models
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
|
||||
"code.vikunja.io/api/pkg/db"
|
||||
"code.vikunja.io/api/pkg/notifications"
|
||||
"code.vikunja.io/api/pkg/user"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"xorm.io/xorm"
|
||||
)
|
||||
|
||||
// TestDatabaseNotifications_ReadAll_RefreshesUsers guards #2720 for notifications
|
||||
// already in the database: those were serialized with a partial doer (id +
|
||||
// username, no display Name), so reading them must reload the embedded users so
|
||||
// the display name is shown. The fix in the dispatch path only helps new
|
||||
// notifications; old rows are healed here at read time.
|
||||
func TestDatabaseNotifications_ReadAll_RefreshesUsers(t *testing.T) {
|
||||
t.Run("fills in the display name from the database", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// user12 has the display name "Name with spaces" in the fixtures.
|
||||
insertStoredNotification(t, s, 1, &TaskAssignedNotification{
|
||||
Doer: &user.User{ID: 12, Username: "user12"},
|
||||
Assignee: &user.User{ID: 12, Username: "user12"},
|
||||
Task: &Task{ID: 1},
|
||||
})
|
||||
|
||||
got := readAssignedNotification(t, s, 1)
|
||||
require.Equal(t, "Name with spaces", got.Doer.GetName())
|
||||
require.Equal(t, "Name with spaces", got.Assignee.GetName())
|
||||
})
|
||||
|
||||
t.Run("keeps the stored value when the user no longer exists", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
insertStoredNotification(t, s, 1, &TaskAssignedNotification{
|
||||
Doer: &user.User{ID: 999999, Username: "ghost"},
|
||||
Task: &Task{ID: 1},
|
||||
})
|
||||
|
||||
got := readAssignedNotification(t, s, 1)
|
||||
require.Equal(t, "ghost", got.Doer.Username)
|
||||
})
|
||||
|
||||
t.Run("refreshes a disabled user", func(t *testing.T) {
|
||||
db.LoadAndAssertFixtures(t)
|
||||
s := db.NewSession()
|
||||
defer s.Close()
|
||||
|
||||
// user17 is disabled in the fixtures; the reload must still win over the
|
||||
// stale stored value.
|
||||
insertStoredNotification(t, s, 1, &TaskAssignedNotification{
|
||||
Doer: &user.User{ID: 17, Username: "stale"},
|
||||
Task: &Task{ID: 1},
|
||||
})
|
||||
|
||||
got := readAssignedNotification(t, s, 1)
|
||||
require.Equal(t, "user17", got.Doer.Username)
|
||||
})
|
||||
}
|
||||
|
||||
func insertStoredNotification(t *testing.T, s *xorm.Session, notifiableID int64, n notifications.Notification) {
|
||||
t.Helper()
|
||||
content, err := json.Marshal(n)
|
||||
require.NoError(t, err)
|
||||
_, err = s.Insert(¬ifications.DatabaseNotification{
|
||||
NotifiableID: notifiableID,
|
||||
Notification: json.RawMessage(content),
|
||||
Name: n.Name(),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
func readAssignedNotification(t *testing.T, s *xorm.Session, notifiableID int64) *TaskAssignedNotification {
|
||||
t.Helper()
|
||||
result, _, _, err := (&DatabaseNotifications{}).ReadAll(s, &user.User{ID: notifiableID}, "", 1, 50)
|
||||
require.NoError(t, err)
|
||||
|
||||
for _, dbn := range result.([]*notifications.DatabaseNotification) {
|
||||
if n, is := dbn.Notification.(*TaskAssignedNotification); is {
|
||||
return n
|
||||
}
|
||||
}
|
||||
t.Fatal("no task.assigned notification was returned")
|
||||
return nil
|
||||
}
|
||||
Loading…
Reference in New Issue