From cca41427a4319d56f0c57a7b0ab2d33700114495 Mon Sep 17 00:00:00 2001 From: Sheshagiri Rao Mallipedhi Date: Mon, 24 Jun 2019 15:33:29 +0530 Subject: [PATCH] Migrate Status.SaveOrUpdate to Sync by default (#11359) * Migrate Status.SaveOrUpdate to Sync by default * address pr comments --- app/auto_users.go | 4 +-- app/ratelimit_test.go | 1 - app/status.go | 20 +++++++-------- store/sqlstore/status_store.go | 23 ++++++++--------- store/store.go | 2 +- store/storetest/mocks/StatusStore.go | 8 +++--- store/storetest/status_store.go | 38 ++++++++-------------------- store/storetest/user_store.go | 12 ++++----- 8 files changed, 44 insertions(+), 64 deletions(-) diff --git a/app/auto_users.go b/app/auto_users.go index 7932097238..a8fa11dc3f 100644 --- a/app/auto_users.go +++ b/app/auto_users.go @@ -80,8 +80,8 @@ func (cfg *AutoUserCreator) createRandomUser() (*model.User, bool) { } status := &model.Status{UserId: ruser.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} - if result := <-cfg.app.Srv.Store.Status().SaveOrUpdate(status); result.Err != nil { - mlog.Error(result.Err.Error()) + if err := cfg.app.Srv.Store.Status().SaveOrUpdate(status); err != nil { + mlog.Error(err.Error()) return nil, false } diff --git a/app/ratelimit_test.go b/app/ratelimit_test.go index 1ae567e1bb..042c12de70 100644 --- a/app/ratelimit_test.go +++ b/app/ratelimit_test.go @@ -94,7 +94,6 @@ func TestGenerateKey_TrustedHeader(t *testing.T) { req.RemoteAddr = "10.10.10.5:80" req.Header.Set("X-Forwarded-For", "10.6.3.1, 10.5.1.2") - rateLimiter, _ := NewRateLimiter(genRateLimitSettings(true, true, ""), []string{"X-Forwarded-For"}) key := rateLimiter.GenerateKey(req) require.Equal(t, "10.6.3.1", key, "Wrong key on test with allowed trusted proxy header") diff --git a/app/status.go b/app/status.go index bda20b22f9..540b5ac075 100644 --- a/app/status.go +++ b/app/status.go @@ -8,7 +8,6 @@ import ( "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/store" "github.com/mattermost/mattermost-server/utils" ) @@ -216,16 +215,15 @@ func (a *App) SetStatusOnline(userId string, manual bool) { // Only update the database if the status has changed, the status has been manually set, // or enough time has passed since the previous action if status.Status != oldStatus || status.Manual != oldManual || status.LastActivityAt-oldTime > model.STATUS_MIN_UPDATE_TIME { - - var schan store.StoreChannel if broadcast { - schan = a.Srv.Store.Status().SaveOrUpdate(status) + if err := a.Srv.Store.Status().SaveOrUpdate(status); err != nil { + mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", userId, err), mlog.String("user_id", userId)) + } } else { - schan = a.Srv.Store.Status().UpdateLastActivityAt(status.UserId, status.LastActivityAt) - } - - if result := <-schan; result.Err != nil { - mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", userId, result.Err), mlog.String("user_id", userId)) + schan := a.Srv.Store.Status().UpdateLastActivityAt(status.UserId, status.LastActivityAt) + if result := <-schan; result.Err != nil { + mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", userId, result.Err), mlog.String("user_id", userId)) + } } } @@ -308,8 +306,8 @@ func (a *App) SetStatusDoNotDisturb(userId string) { func (a *App) SaveAndBroadcastStatus(status *model.Status) { a.AddStatusCache(status) - if result := <-a.Srv.Store.Status().SaveOrUpdate(status); result.Err != nil { - mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", status.UserId, result.Err)) + if err := a.Srv.Store.Status().SaveOrUpdate(status); err != nil { + mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", status.UserId, err)) } a.BroadcastStatus(status) diff --git a/store/sqlstore/status_store.go b/store/sqlstore/status_store.go index 3731a8d4ca..3c25b6492c 100644 --- a/store/sqlstore/status_store.go +++ b/store/sqlstore/status_store.go @@ -39,20 +39,19 @@ func (s SqlStatusStore) CreateIndexesIfNotExists() { s.CreateIndexIfNotExists("idx_status_status", "Status", "Status") } -func (s SqlStatusStore) SaveOrUpdate(status *model.Status) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if err := s.GetReplica().SelectOne(&model.Status{}, "SELECT * FROM Status WHERE UserId = :UserId", map[string]interface{}{"UserId": status.UserId}); err == nil { - if _, err := s.GetMaster().Update(status); err != nil { - result.Err = model.NewAppError("SqlStatusStore.SaveOrUpdate", "store.sql_status.update.app_error", nil, err.Error(), http.StatusInternalServerError) - } - } else { - if err := s.GetMaster().Insert(status); err != nil { - if !(strings.Contains(err.Error(), "for key 'PRIMARY'") && strings.Contains(err.Error(), "Duplicate entry")) { - result.Err = model.NewAppError("SqlStatusStore.SaveOrUpdate", "store.sql_status.save.app_error", nil, err.Error(), http.StatusInternalServerError) - } +func (s SqlStatusStore) SaveOrUpdate(status *model.Status) *model.AppError { + if err := s.GetReplica().SelectOne(&model.Status{}, "SELECT * FROM Status WHERE UserId = :UserId", map[string]interface{}{"UserId": status.UserId}); err == nil { + if _, err := s.GetMaster().Update(status); err != nil { + return model.NewAppError("SqlStatusStore.SaveOrUpdate", "store.sql_status.update.app_error", nil, err.Error(), http.StatusInternalServerError) + } + } else { + if err := s.GetMaster().Insert(status); err != nil { + if !(strings.Contains(err.Error(), "for key 'PRIMARY'") && strings.Contains(err.Error(), "Duplicate entry")) { + return model.NewAppError("SqlStatusStore.SaveOrUpdate", "store.sql_status.save.app_error", nil, err.Error(), http.StatusInternalServerError) } } - }) + } + return nil } func (s SqlStatusStore) Get(userId string) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 951f0d2fb5..9ae18ebe1d 100644 --- a/store/store.go +++ b/store/store.go @@ -466,7 +466,7 @@ type EmojiStore interface { } type StatusStore interface { - SaveOrUpdate(status *model.Status) StoreChannel + SaveOrUpdate(status *model.Status) *model.AppError Get(userId string) StoreChannel GetByIds(userIds []string) StoreChannel GetOnlineAway() StoreChannel diff --git a/store/storetest/mocks/StatusStore.go b/store/storetest/mocks/StatusStore.go index 87cfda6361..5a110d14ca 100644 --- a/store/storetest/mocks/StatusStore.go +++ b/store/storetest/mocks/StatusStore.go @@ -135,15 +135,15 @@ func (_m *StatusStore) ResetAll() store.StoreChannel { } // SaveOrUpdate provides a mock function with given fields: status -func (_m *StatusStore) SaveOrUpdate(status *model.Status) store.StoreChannel { +func (_m *StatusStore) SaveOrUpdate(status *model.Status) *model.AppError { ret := _m.Called(status) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.Status) store.StoreChannel); ok { + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(*model.Status) *model.AppError); ok { r0 = rf(status) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.AppError) } } diff --git a/store/storetest/status_store.go b/store/storetest/status_store.go index 73b8c1a17f..403f376cf0 100644 --- a/store/storetest/status_store.go +++ b/store/storetest/status_store.go @@ -22,30 +22,19 @@ func TestStatusStore(t *testing.T, ss store.Store) { func testStatusStore(t *testing.T, ss store.Store) { status := &model.Status{UserId: model.NewId(), Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""} - - if err := (<-ss.Status().SaveOrUpdate(status)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(status)) status.LastActivityAt = 10 - if err := (<-ss.Status().SaveOrUpdate(status)).Err; err != nil { - t.Fatal(err) - } - if err := (<-ss.Status().Get(status.UserId)).Err; err != nil { t.Fatal(err) } status2 := &model.Status{UserId: model.NewId(), Status: model.STATUS_AWAY, Manual: false, LastActivityAt: 0, ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(status2)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(status2)) status3 := &model.Status{UserId: model.NewId(), Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(status3)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(status3)) if result := <-ss.Status().GetOnlineAway(); result.Err != nil { t.Fatal(result.Err) @@ -97,7 +86,7 @@ func testStatusStore(t *testing.T, ss store.Store) { func testActiveUserCount(t *testing.T, ss store.Store) { status := &model.Status{UserId: model.NewId(), Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} - store.Must(ss.Status().SaveOrUpdate(status)) + require.Nil(t, ss.Status().SaveOrUpdate(status)) if result := <-ss.Status().GetTotalActiveUsersCount(); result.Err != nil { t.Fatal(result.Err) @@ -158,21 +147,16 @@ func testGetAllFromTeam(t *testing.T, ss store.Store) { } team1Member1Status := &model.Status{UserId: team1Member1.UserId, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(team1Member1Status)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(team1Member1Status)) + team1Member2Status := &model.Status{UserId: team1Member2.UserId, Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(team1Member2Status)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(team1Member2Status)) + team2Member1Status := &model.Status{UserId: team2Member1.UserId, Status: model.STATUS_ONLINE, Manual: true, LastActivityAt: model.GetMillis(), ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(team2Member1Status)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(team2Member1Status)) + team2Member2Status := &model.Status{UserId: team2Member2.UserId, Status: model.STATUS_OFFLINE, Manual: true, LastActivityAt: model.GetMillis(), ActiveChannel: ""} - if err := (<-ss.Status().SaveOrUpdate(team2Member2Status)).Err; err != nil { - t.Fatal(err) - } + require.Nil(t, ss.Status().SaveOrUpdate(team2Member2Status)) if result := <-ss.Status().GetAllFromTeam(team1.Id); result.Err != nil { t.Fatal(result.Err) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 4613ed07f5..ce86117c27 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -822,15 +822,15 @@ func testUserStoreGetProfilesInChannelByStatus(t *testing.T, ss store.Store) { NotifyProps: model.GetDefaultChannelNotifyProps(), })) - store.Must(ss.Status().SaveOrUpdate(&model.Status{ + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{ UserId: u1.Id, Status: model.STATUS_DND, })) - store.Must(ss.Status().SaveOrUpdate(&model.Status{ + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{ UserId: u2.Id, Status: model.STATUS_AWAY, })) - store.Must(ss.Status().SaveOrUpdate(&model.Status{ + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{ UserId: u3.Id, Status: model.STATUS_ONLINE, })) @@ -1965,9 +1965,9 @@ func testUserStoreGetRecentlyActiveUsersForTeam(t *testing.T, ss store.Store) { u2.LastActivityAt = millis - 1 u1.LastActivityAt = millis - 1 - store.Must(ss.Status().SaveOrUpdate(&model.Status{UserId: u1.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u1.LastActivityAt, ActiveChannel: ""})) - store.Must(ss.Status().SaveOrUpdate(&model.Status{UserId: u2.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u2.LastActivityAt, ActiveChannel: ""})) - store.Must(ss.Status().SaveOrUpdate(&model.Status{UserId: u3.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u3.LastActivityAt, ActiveChannel: ""})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u1.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u1.LastActivityAt, ActiveChannel: ""})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u2.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u2.LastActivityAt, ActiveChannel: ""})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u3.Id, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: u3.LastActivityAt, ActiveChannel: ""})) t.Run("get team 1, offset 0, limit 100", func(t *testing.T) { result := <-ss.User().GetRecentlyActiveUsersForTeam(teamId, 0, 100, nil)