From 132f1147937d5753c676d525dc2e998a26524be7 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 17 Aug 2021 14:21:41 +0530 Subject: [PATCH] Modified updateUserNotifyProps to directly update the field (#18097) * Modified updateUserNotifyProps to directly update the field The method was only being used during import and it unnecessarily made multiple queries to the DB. Changed to a separate query that just updated the props field. https://community-daily.mattermost.com/plugins/focalboard/workspace/zyoahc9uapdn3xdptac6jb69ic?id=285b80a3-257d-41f6-8cf4-ed80ca9d92e5&v=495cdb4d-c13a-4992-8eb9-80cfee2819a4&c=e4f9a891-85d6-4886-8590-1e327f7f8b8f ```release-note NONE ``` * invalidating cache ```release-note NONE ``` --- app/app_iface.go | 1 - app/import_functions.go | 5 ++++- app/opentracing/opentracing_layer.go | 22 ---------------------- app/user.go | 22 ++++++++++++---------- services/users/users.go | 4 ++++ store/opentracinglayer/opentracinglayer.go | 18 ++++++++++++++++++ store/retrylayer/retrylayer.go | 20 ++++++++++++++++++++ store/sqlstore/user_store.go | 12 ++++++++++++ store/store.go | 1 + store/storetest/mocks/UserStore.go | 14 ++++++++++++++ store/storetest/user_store.go | 16 ++++++++++++++++ store/timerlayer/timerlayer.go | 16 ++++++++++++++++ 12 files changed, 117 insertions(+), 34 deletions(-) diff --git a/app/app_iface.go b/app/app_iface.go index 6990ba11de..364d989d77 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -1082,7 +1082,6 @@ type AppIface interface { UpdateUserActive(c *request.Context, userID string, active bool) *model.AppError UpdateUserAsUser(user *model.User, asAdmin bool) (*model.User, *model.AppError) UpdateUserAuth(userID string, userAuth *model.UserAuth) (*model.UserAuth, *model.AppError) - UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UploadData(c *request.Context, us *model.UploadSession, rd io.Reader) (*model.FileInfo, *model.AppError) diff --git a/app/import_functions.go b/app/import_functions.go index 087a848917..dd5350a868 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -527,7 +527,10 @@ func (a *App) importUser(data *UserImportData, dryRun bool) *model.AppError { } } if hasNotifyPropsChanged { - if savedUser, appErr = a.UpdateUserNotifyProps(user.Id, user.NotifyProps, false); appErr != nil { + if appErr = a.updateUserNotifyProps(user.Id, user.NotifyProps); appErr != nil { + return appErr + } + if savedUser, appErr = a.GetUser(user.Id); appErr != nil { return appErr } } diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 783beee676..b18d31c2e4 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -16816,28 +16816,6 @@ func (a *OpenTracingAppLayer) UpdateUserAuth(userID string, userAuth *model.User return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateUserNotifyProps") - - a.ctx = newCtx - a.app.Srv().Store.SetContext(newCtx) - defer func() { - a.app.Srv().Store.SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - resultVar0, resultVar1 := a.app.UpdateUserNotifyProps(userID, props, sendNotifications) - - if resultVar1 != nil { - span.LogFields(spanlog.Error(resultVar1)) - ext.Error.Set(span, true) - } - - return resultVar0, resultVar1 -} - func (a *OpenTracingAppLayer) UpdateUserRoles(userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpdateUserRoles") diff --git a/app/user.go b/app/user.go index e9f3d0dbd7..30c7851d1a 100644 --- a/app/user.go +++ b/app/user.go @@ -1143,20 +1143,22 @@ func (a *App) UpdateUserActive(c *request.Context, userID string, active bool) * return nil } -func (a *App) UpdateUserNotifyProps(userID string, props map[string]string, sendNotifications bool) (*model.User, *model.AppError) { - user, err := a.GetUser(userID) +func (a *App) updateUserNotifyProps(userID string, props map[string]string) *model.AppError { + err := a.srv.userService.UpdateUserNotifyProps(userID, props) if err != nil { - return nil, err + var appErr *model.AppError + switch { + case errors.As(err, &appErr): + return appErr + default: + return model.NewAppError("UpdateUser", "app.user.update.finding.app_error", nil, err.Error(), http.StatusInternalServerError) + } } - user.NotifyProps = props + a.InvalidateCacheForUser(userID) + a.onUserProfileChange(userID) - ruser, err := a.UpdateUser(user, sendNotifications) - if err != nil { - return nil, err - } - - return ruser, nil + return nil } func (a *App) UpdateMfa(activate bool, userID, token string) *model.AppError { diff --git a/services/users/users.go b/services/users/users.go index 8bc49bb8fd..b374d24f96 100644 --- a/services/users/users.go +++ b/services/users/users.go @@ -193,6 +193,10 @@ func (us *UserService) UpdateUser(user *model.User, allowRoleUpdate bool) (*mode return us.store.Update(user, allowRoleUpdate) } +func (us *UserService) UpdateUserNotifyProps(userID string, props map[string]string) error { + return us.store.UpdateNotifyProps(userID, props) +} + func (us *UserService) DeactivateAllGuests() ([]string, error) { users, err := us.store.DeactivateGuests() if err != nil { diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index d915eff7c3..eb09a59ca1 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -10685,6 +10685,24 @@ func (s *OpenTracingLayerUserStore) UpdateMfaSecret(userID string, secret string return err } +func (s *OpenTracingLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.UpdateNotifyProps") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + err := s.UserStore.UpdateNotifyProps(userID, props) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return err +} + func (s *OpenTracingLayerUserStore) UpdatePassword(userID string, newPassword string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "UserStore.UpdatePassword") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index fa1d5e5232..f080798197 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -11610,6 +11610,26 @@ func (s *RetryLayerUserStore) UpdateMfaSecret(userID string, secret string) erro } +func (s *RetryLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error { + + tries := 0 + for { + err := s.UserStore.UpdateNotifyProps(userID, props) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + } + +} + func (s *RetryLayerUserStore) UpdatePassword(userID string, newPassword string) error { tries := 0 diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index e3934754cd..15381ef145 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -226,6 +226,18 @@ func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) (*model. return &model.UserUpdate{New: user, Old: oldUser}, nil } +func (us SqlUserStore) UpdateNotifyProps(userID string, props map[string]string) error { + if _, err := us.GetMaster().Exec(`UPDATE Users + SET NotifyProps = :NotifyProps + WHERE Id = :UserId`, map[string]interface{}{ + "NotifyProps": model.MapToJson(props), + "UserId": userID}); err != nil { + return errors.Wrapf(err, "failed to update User with userId=%s", userID) + } + + return nil +} + func (us SqlUserStore) UpdateLastPictureUpdate(userId string) error { curTime := model.GetMillis() diff --git a/store/store.go b/store/store.go index b6e6d5992e..f430c8281b 100644 --- a/store/store.go +++ b/store/store.go @@ -356,6 +356,7 @@ type PostStore interface { type UserStore interface { Save(user *model.User) (*model.User, error) Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, error) + UpdateNotifyProps(userID string, props map[string]string) error UpdateLastPictureUpdate(userID string) error ResetLastPictureUpdate(userID string) error UpdatePassword(userID, newPassword string) error diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index e37e80bd0f..06e0db7d94 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -1383,6 +1383,20 @@ func (_m *UserStore) UpdateMfaSecret(userID string, secret string) error { return r0 } +// UpdateNotifyProps provides a mock function with given fields: userID, props +func (_m *UserStore) UpdateNotifyProps(userID string, props map[string]string) error { + ret := _m.Called(userID, props) + + var r0 error + if rf, ok := ret.Get(0).(func(string, map[string]string) error); ok { + r0 = rf(userID, props) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // UpdatePassword provides a mock function with given fields: userID, newPassword func (_m *UserStore) UpdatePassword(userID string, newPassword string) error { ret := _m.Called(userID, newPassword) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 26d97294cf..d9d5f41c15 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -215,6 +215,22 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) { err = ss.User().UpdateLastPictureUpdate(u1.Id) require.NoError(t, err, "Update should not have failed") + + // Test UpdateNotifyProps + u1, err = ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + + props := u1.NotifyProps + props["hello"] = "world" + + err = ss.User().UpdateNotifyProps(u1.Id, props) + require.NoError(t, err) + + ss.User().InvalidateProfileCacheForUser(u1.Id) + + uNew, err := ss.User().Get(context.Background(), u1.Id) + require.NoError(t, err) + assert.Equal(t, props, uNew.NotifyProps) } func testUserStoreUpdateUpdateAt(t *testing.T, ss store.Store) { diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 7543b686f8..ba05bc1a11 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -9634,6 +9634,22 @@ func (s *TimerLayerUserStore) UpdateMfaSecret(userID string, secret string) erro return err } +func (s *TimerLayerUserStore) UpdateNotifyProps(userID string, props map[string]string) error { + start := timemodule.Now() + + err := s.UserStore.UpdateNotifyProps(userID, props) + + elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserStore.UpdateNotifyProps", success, elapsed) + } + return err +} + func (s *TimerLayerUserStore) UpdatePassword(userID string, newPassword string) error { start := timemodule.Now()