diff --git a/pkg/files/files.go b/pkg/files/files.go index 5426c9ebf..62cab552f 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -97,6 +97,18 @@ func Create(f io.ReadSeeker, realname string, realsize uint64, a web.Auth) (file return CreateWithMime(f, realname, realsize, a, mime.String()) } +// CreateWithSession creates a new file using an existing session to avoid nested transactions +func CreateWithSession(s *xorm.Session, f io.ReadSeeker, realname string, realsize uint64, a web.Auth) (file *File, err error) { + mime, err := mimetype.DetectReader(f) + if err != nil { + return nil, fmt.Errorf("failed to detect mime type: %w", err) + } + if _, err := f.Seek(0, io.SeekStart); err != nil { + return nil, fmt.Errorf("failed to seek after mime detection: %w", err) + } + return CreateWithMimeAndSession(s, f, realname, realsize, a, mime.String(), true) +} + // CreateWithMime creates a new file from an FileHeader and sets its mime type func CreateWithMime(f io.ReadSeeker, realname string, realsize uint64, a web.Auth, mime string) (file *File, err error) { s := db.NewSession() diff --git a/pkg/models/export.go b/pkg/models/export.go index 2189057be..5c953e134 100644 --- a/pkg/models/export.go +++ b/pkg/models/export.go @@ -115,7 +115,7 @@ func ExportUserData(s *xorm.Session, u *user.User) (err error) { // Send a notification return notifications.Notify(u, &DataExportReadyNotification{ User: u, - }) + }, s) } func getRawTasksForExport(s *xorm.Session, projectIDs []int64, a web.Auth) (tasks []*Task, err error) { diff --git a/pkg/models/listeners.go b/pkg/models/listeners.go index 41e9ca8d2..7aea8b152 100644 --- a/pkg/models/listeners.go +++ b/pkg/models/listeners.go @@ -162,7 +162,7 @@ func notifyMentionedUsers(sess *xorm.Session, task *Task, text string, n notific continue } - err = notifications.Notify(u, n) + err = notifications.Notify(u, n, sess) if err != nil { return users, err } @@ -226,7 +226,7 @@ func (s *SendTaskCommentNotification) Handle(msg *message.Message) (err error) { Task: event.Task, Comment: event.Comment, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -315,7 +315,7 @@ func (s *SendTaskAssignedNotification) Handle(msg *message.Message) (err error) Assignee: event.Assignee, Target: subscriber.User, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -368,7 +368,7 @@ func (s *SendTaskDeletedNotification) Handle(msg *message.Message) (err error) { Doer: event.Doer, Task: event.Task, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } @@ -826,7 +826,7 @@ func (s *SendProjectCreatedNotification) Handle(msg *message.Message) (err error Doer: event.Doer, Project: event.Project, } - err = notifications.Notify(subscriber.User, n) + err = notifications.Notify(subscriber.User, n, sess) if err != nil { return } diff --git a/pkg/models/project.go b/pkg/models/project.go index f96f39b54..7b5f17b6c 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1191,7 +1191,7 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { return } - err = fullProject.DeleteBackgroundFileIfExists() + err = fullProject.DeleteBackgroundFileIfExists(s) if err != nil { return } @@ -1278,25 +1278,18 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { // DeleteBackgroundFileIfExists deletes the list's background file from the db and the filesystem, // if one exists -func (p *Project) DeleteBackgroundFileIfExists() (err error) { +func (p *Project) DeleteBackgroundFileIfExists(s *xorm.Session) (err error) { if p.BackgroundFileID == 0 { return } - s := db.NewSession() - defer s.Close() - file := files.File{ID: p.BackgroundFileID} err = file.Delete(s) if err != nil && files.IsErrFileDoesNotExist(err) { return nil } - if err != nil { - _ = s.Rollback() - return err - } - return s.Commit() + return err } // SetProjectBackground sets a background file as project background in the db diff --git a/pkg/models/project_test.go b/pkg/models/project_test.go index 0f88bfce2..761954cdc 100644 --- a/pkg/models/project_test.go +++ b/pkg/models/project_test.go @@ -403,6 +403,7 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) s := db.NewSession() + defer s.Close() file := &files.File{ID: 1} project := Project{ ID: 1, @@ -410,13 +411,14 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { } err := SetProjectBackground(s, project.ID, file, "") require.NoError(t, err) - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) t.Run("project with invalid background", func(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) s := db.NewSession() + defer s.Close() file := &files.File{ID: 9999} project := Project{ ID: 1, @@ -424,14 +426,16 @@ func TestProject_DeleteBackgroundFileIfExists(t *testing.T) { } err := SetProjectBackground(s, project.ID, file, "") require.NoError(t, err) - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) t.Run("project without background", func(t *testing.T) { db.LoadAndAssertFixtures(t) files.InitTestFileFixtures(t) + s := db.NewSession() + defer s.Close() project := Project{ID: 1} - err := project.DeleteBackgroundFileIfExists() + err := project.DeleteBackgroundFileIfExists(s) require.NoError(t, err) }) } diff --git a/pkg/models/task_attachment.go b/pkg/models/task_attachment.go index 64e3a4129..c63182492 100644 --- a/pkg/models/task_attachment.go +++ b/pkg/models/task_attachment.go @@ -60,8 +60,8 @@ func (*TaskAttachment) TableName() string { // NewAttachment creates a new task attachment func (ta *TaskAttachment) NewAttachment(s *xorm.Session, f io.ReadSeeker, realname string, realsize uint64, a web.Auth) error { - // Store the file - file, err := files.Create(f, realname, realsize, a) + // Store the file using the existing session to avoid nested transactions + file, err := files.CreateWithSession(s, f, realname, realsize, a) if err != nil { if files.IsErrFileIsTooLarge(err) { return ErrTaskAttachmentIsTooLarge{Size: realsize} diff --git a/pkg/models/task_overdue_reminder.go b/pkg/models/task_overdue_reminder.go index 31b669e1f..96399cb30 100644 --- a/pkg/models/task_overdue_reminder.go +++ b/pkg/models/task_overdue_reminder.go @@ -178,7 +178,7 @@ func RegisterOverdueReminderCron() { } } - err = notifications.Notify(ut.user, n) + err = notifications.Notify(ut.user, n, s) if err != nil { log.Errorf("[Undone Overdue Tasks Reminder] Could not notify user %d: %s", ut.user.ID, err) return diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 8e5c37b41..def29d04f 100644 --- a/pkg/models/task_reminder.go +++ b/pkg/models/task_reminder.go @@ -383,7 +383,7 @@ func RegisterReminderCron() { } for _, n := range reminders { - err = notifications.Notify(n.User, n) + err = notifications.Notify(n.User, n, s) if err != nil { log.Errorf("[Task Reminder Cron] Could not notify user %d: %s", n.User.ID, err) return diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go index 5beffc813..65a663394 100644 --- a/pkg/models/user_delete.go +++ b/pkg/models/user_delete.go @@ -176,7 +176,7 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) { return notifications.Notify(u, &user.AccountDeletedNotification{ User: u, - }) + }, s) } func ensureProjectAdminUser(s *xorm.Session, l *Project) (hadUsers bool, err error) { diff --git a/pkg/modules/auth/openid/openid_test.go b/pkg/modules/auth/openid/openid_test.go index e32f84465..fca80d426 100644 --- a/pkg/modules/auth/openid/openid_test.go +++ b/pkg/modules/auth/openid/openid_test.go @@ -218,6 +218,8 @@ func TestGetOrCreateUser(t *testing.T) { teamData := getTeamDataFromToken(cl.VikunjaGroups, nil) err := models.SyncExternalTeamsForUser(s, u, teamData, "https://some.issuer", "OIDC") require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) db.AssertMissing(t, "team_members", map[string]interface{}{ "team_id": 14, diff --git a/pkg/modules/background/handler/background.go b/pkg/modules/background/handler/background.go index 9ecd39c93..2f405c8e5 100644 --- a/pkg/modules/background/handler/background.go +++ b/pkg/modules/background/handler/background.go @@ -282,7 +282,7 @@ func SaveBackgroundFile(s *xorm.Session, auth web.Auth, project *models.Project, return err } - f, err := files.Create(bytes.NewReader(buf.Bytes()), filename, filesize, auth) + f, err := files.CreateWithSession(s, bytes.NewReader(buf.Bytes()), filename, filesize, auth) if err != nil { return err } @@ -411,7 +411,7 @@ func RemoveProjectBackground(c *echo.Context) error { return err } - err = project.DeleteBackgroundFileIfExists() + err = project.DeleteBackgroundFileIfExists(s) if err != nil { _ = s.Rollback() return err diff --git a/pkg/notifications/notification.go b/pkg/notifications/notification.go index 83677364d..7f0e1faef 100644 --- a/pkg/notifications/notification.go +++ b/pkg/notifications/notification.go @@ -21,6 +21,8 @@ import ( "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" + + "xorm.io/xorm" ) // Notification is a notification which can be sent via mail or db. @@ -50,20 +52,21 @@ type Notifiable interface { // RouteForDB should return the id of the notifiable entity to save it in the database. RouteForDB() int64 // ShouldNotify provides a last-minute way to cancel a notification. It will be called immediately before - // sending a notification. - ShouldNotify() (should bool, err error) + // sending a notification. An optional session can be passed to reuse an existing transaction. + ShouldNotify(sessions ...*xorm.Session) (should bool, err error) // Lang provides the language which should be used for translations in the mail. Lang() string } -// Notify notifies a notifiable of a notification -func Notify(notifiable Notifiable, notification Notification) (err error) { +// Notify notifies a notifiable of a notification. +// An optional xorm session can be passed to reuse an existing transaction for the DB notification. +func Notify(notifiable Notifiable, notification Notification, sessions ...*xorm.Session) (err error) { if isUnderTest { sentTestNotifications = append(sentTestNotifications, notification) return nil } - should, err := notifiable.ShouldNotify() + should, err := notifiable.ShouldNotify(sessions...) if err != nil || !should { log.Debugf("Not notifying user %d because they are disabled", notifiable.RouteForDB()) return err @@ -74,7 +77,12 @@ func Notify(notifiable Notifiable, notification Notification) (err error) { return } - return notifyDB(notifiable, notification) + var s *xorm.Session + if len(sessions) > 0 && sessions[0] != nil { + s = sessions[0] + } + + return notifyDB(notifiable, notification, s) } func notifyMail(notifiable Notifiable, notification Notification) error { @@ -96,7 +104,7 @@ func notifyMail(notifiable Notifiable, notification Notification) error { return SendMail(mail, notifiable.Lang()) } -func notifyDB(notifiable Notifiable, notification Notification) (err error) { +func notifyDB(notifiable Notifiable, notification Notification, existingSession *xorm.Session) (err error) { dbContent := notification.ToDB() if dbContent == nil { @@ -108,8 +116,6 @@ func notifyDB(notifiable Notifiable, notification Notification) (err error) { return err } - s := db.NewSession() - defer s.Close() dbNotification := &DatabaseNotification{ NotifiableID: notifiable.RouteForDB(), Notification: content, @@ -120,6 +126,14 @@ func notifyDB(notifiable Notifiable, notification Notification) (err error) { dbNotification.SubjectID = subject.SubjectID() } + if existingSession != nil { + _, err = existingSession.Insert(dbNotification) + return err + } + + s := db.NewSession() + defer s.Close() + _, err = s.Insert(dbNotification) if err != nil { _ = s.Rollback() diff --git a/pkg/notifications/notification_test.go b/pkg/notifications/notification_test.go index 397fdbe54..f15eb350b 100644 --- a/pkg/notifications/notification_test.go +++ b/pkg/notifications/notification_test.go @@ -22,6 +22,7 @@ import ( "code.vikunja.io/api/pkg/db" "github.com/stretchr/testify/require" + "xorm.io/xorm" "xorm.io/xorm/schemas" ) @@ -65,7 +66,7 @@ func (t *testNotifiable) RouteForDB() int64 { return 42 } -func (t *testNotifiable) ShouldNotify() (should bool, err error) { +func (t *testNotifiable) ShouldNotify(_ ...*xorm.Session) (should bool, err error) { return t.ShouldSendNotification, nil } @@ -77,9 +78,10 @@ func TestNotify(t *testing.T) { t.Run("normal", func(t *testing.T) { s := db.NewSession() - defer s.Close() _, err := s.Exec("delete from notifications") require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() tn := &testNotification{ Test: "somethingsomething", @@ -112,9 +114,10 @@ func TestNotify(t *testing.T) { t.Run("disabled notifiable", func(t *testing.T) { s := db.NewSession() - defer s.Close() _, err := s.Exec("delete from notifications") require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() tn := &testNotification{ Test: "somethingsomething", diff --git a/pkg/notifications/notify_disabled_test.go b/pkg/notifications/notify_disabled_test.go index 55b950c6a..363046c46 100644 --- a/pkg/notifications/notify_disabled_test.go +++ b/pkg/notifications/notify_disabled_test.go @@ -22,14 +22,16 @@ import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/i18n" + + "xorm.io/xorm" ) type disabledMailNotifiable struct{} -func (d *disabledMailNotifiable) RouteForMail() (string, error) { return "test@example.com", nil } -func (d *disabledMailNotifiable) RouteForDB() int64 { return 1 } -func (d *disabledMailNotifiable) ShouldNotify() (bool, error) { return true, nil } -func (d *disabledMailNotifiable) Lang() string { return "en" } +func (d *disabledMailNotifiable) RouteForMail() (string, error) { return "test@example.com", nil } +func (d *disabledMailNotifiable) RouteForDB() int64 { return 1 } +func (d *disabledMailNotifiable) ShouldNotify(_ ...*xorm.Session) (bool, error) { return true, nil } +func (d *disabledMailNotifiable) Lang() string { return "en" } type disabledMailNotification struct{} diff --git a/pkg/user/delete.go b/pkg/user/delete.go index c08e094a7..e985a5869 100644 --- a/pkg/user/delete.go +++ b/pkg/user/delete.go @@ -70,7 +70,7 @@ func notifyUsersScheduledForDeletion() { err = notifications.Notify(user, &AccountDeletionNotification{ User: user, NotificationNumber: number, - }) + }, s) if err != nil { log.Errorf("Could not notify user %d of their deletion: %s", user.ID, err) continue @@ -100,7 +100,7 @@ func RequestDeletion(s *xorm.Session, user *User) (err error) { return notifications.Notify(user, &AccountDeletionConfirmNotification{ User: user, ConfirmToken: token.Token, - }) + }, s) } // ConfirmDeletion ConformDeletion checks a token and schedules the user for deletion diff --git a/pkg/user/totp.go b/pkg/user/totp.go index 0a2241831..2f9314860 100644 --- a/pkg/user/totp.go +++ b/pkg/user/totp.go @@ -172,7 +172,7 @@ func HandleFailedTOTPAuth(s *xorm.Session, user *User) { attempts := a.(int64) if attempts == 3 { - err = notifications.Notify(user, &InvalidTOTPNotification{User: user}) + err = notifications.Notify(user, &InvalidTOTPNotification{User: user}, s) if err != nil { log.Errorf("Could not send failed TOTP notification to user %d: %s", user.ID, err) return @@ -191,7 +191,7 @@ func HandleFailedTOTPAuth(s *xorm.Session, user *User) { } err = notifications.Notify(user, &PasswordAccountLockedAfterInvalidTOTPNotification{ User: user, - }) + }, s) if err != nil { log.Errorf("Could send password information mail to user %d after 10 failed TOTP attempts: %s", user.ID, err) return diff --git a/pkg/user/update_email.go b/pkg/user/update_email.go index b0ec0bec4..73e104682 100644 --- a/pkg/user/update_email.go +++ b/pkg/user/update_email.go @@ -84,6 +84,6 @@ func UpdateEmail(s *xorm.Session, update *EmailUpdate) (err error) { ConfirmToken: token.Token, } - err = notifications.Notify(update.User, n) + err = notifications.Notify(update.User, n, s) return } diff --git a/pkg/user/user.go b/pkg/user/user.go index a8abdc633..dc28c85e9 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -140,9 +140,14 @@ func (u *User) RouteForDB() int64 { return u.ID } -func (u *User) ShouldNotify() (bool, error) { - s := db.NewSession() - defer s.Close() +func (u *User) ShouldNotify(sessions ...*xorm.Session) (bool, error) { + var s *xorm.Session + if len(sessions) > 0 && sessions[0] != nil { + s = sessions[0] + } else { + s = db.NewSession() + defer s.Close() + } user, err := getUser(s, &User{ID: u.ID}, true) if err != nil { return false, err diff --git a/pkg/user/user_create.go b/pkg/user/user_create.go index 79c790b9e..85f03614b 100644 --- a/pkg/user/user_create.go +++ b/pkg/user/user_create.go @@ -120,7 +120,7 @@ func CreateUser(s *xorm.Session, user *User) (newUser *User, err error) { ConfirmToken: token.Token, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return newUserOut, err } diff --git a/pkg/user/user_password_reset.go b/pkg/user/user_password_reset.go index 9c2e1c684..4450b0e93 100644 --- a/pkg/user/user_password_reset.go +++ b/pkg/user/user_password_reset.go @@ -89,10 +89,11 @@ func ResetPassword(s *xorm.Session, reset *PasswordReset) (userID int64, err err User: user, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return } + // PasswordTokenRequest defines the request format for password reset resqest type PasswordTokenRequest struct { Email string `json:"email" valid:"email,length(0|250)" maxLength:"250"` @@ -130,6 +131,6 @@ func RequestUserPasswordResetToken(s *xorm.Session, user *User) (err error) { Token: token, } - err = notifications.Notify(user, n) + err = notifications.Notify(user, n, s) return } diff --git a/pkg/user/user_test.go b/pkg/user/user_test.go index 2af7b9273..b13c63396 100644 --- a/pkg/user/user_test.go +++ b/pkg/user/user_test.go @@ -650,6 +650,9 @@ func TestConfirmDeletion(t *testing.T) { err := ConfirmDeletion(s, user, token) require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + db.AssertMissing(t, "user_tokens", map[string]interface{}{ "token": token, "kind": TokenAccountDeletion,