From 7f53be410567269eaa3ec24333d0fcdd51f57436 Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 17 Jun 2026 21:35:25 +0200 Subject: [PATCH] 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. --- .golangci.yml | 3 + pkg/models/notifications_database.go | 8 +- pkg/models/notifications_refresh.go | 122 +++++++++++++++++++++++ pkg/models/notifications_refresh_test.go | 109 ++++++++++++++++++++ 4 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 pkg/models/notifications_refresh.go create mode 100644 pkg/models/notifications_refresh_test.go diff --git a/.golangci.yml b/.golangci.yml index 19ee2f531..7c75f08de 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -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 diff --git a/pkg/models/notifications_database.go b/pkg/models/notifications_database.go index aaa71103c..75beef580 100644 --- a/pkg/models/notifications_database.go +++ b/pkg/models/notifications_database.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. diff --git a/pkg/models/notifications_refresh.go b/pkg/models/notifications_refresh.go new file mode 100644 index 000000000..81e8a9609 --- /dev/null +++ b/pkg/models/notifications_refresh.go @@ -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 . + +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 + } +} diff --git a/pkg/models/notifications_refresh_test.go b/pkg/models/notifications_refresh_test.go new file mode 100644 index 000000000..b0cef2a43 --- /dev/null +++ b/pkg/models/notifications_refresh_test.go @@ -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 . + +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 +}