Migrate Status.SaveOrUpdate to Sync by default (#11359)

* Migrate Status.SaveOrUpdate to Sync by default

* address pr comments

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 {
+	if err := cfg.app.Srv.Store.Status().SaveOrUpdate(status); err != nil {
-		mlog.Error(result.Err.Error())
+		mlog.Error(err.Error())
 		return nil, false
 	}
 

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")

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,18 +215,17 @@ 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 {
-		} else {
+				mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", userId, err), mlog.String("user_id", userId))
-			schan = a.Srv.Store.Status().UpdateLastActivityAt(status.UserId, status.LastActivityAt)
 			}
-
+		} 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))
 			}
 		}
+	}
 
 	if broadcast {
 		a.BroadcastStatus(status)
@@ -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 {
+	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, result.Err))
+		mlog.Error(fmt.Sprintf("Failed to save status for user_id=%v, err=%v", status.UserId, err))
 	}
 
 	a.BroadcastStatus(status)

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 {
+func (s SqlStatusStore) SaveOrUpdate(status *model.Status) *model.AppError {
-	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)
+			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")) {
-					result.Err = model.NewAppError("SqlStatusStore.SaveOrUpdate", "store.sql_status.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+				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 {

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

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
+	var r0 *model.AppError
-	if rf, ok := ret.Get(0).(func(*model.Status) store.StoreChannel); ok {
+	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)
 		}
 	}
 

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: ""}
-
+	require.Nil(t, ss.Status().SaveOrUpdate(status))
-	if err := (<-ss.Status().SaveOrUpdate(status)).Err; err != nil {
-		t.Fatal(err)
-	}
 
 	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 {
+	require.Nil(t, ss.Status().SaveOrUpdate(status2))
-		t.Fatal(err)
-	}
 
 	status3 := &model.Status{UserId: model.NewId(), Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
-	if err := (<-ss.Status().SaveOrUpdate(status3)).Err; err != nil {
+	require.Nil(t, ss.Status().SaveOrUpdate(status3))
-		t.Fatal(err)
-	}
 
 	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 {
+	require.Nil(t, ss.Status().SaveOrUpdate(team1Member1Status))
-		t.Fatal(err)
+
-	}
 	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 {
+	require.Nil(t, ss.Status().SaveOrUpdate(team1Member2Status))
-		t.Fatal(err)
+
-	}
 	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 {
+	require.Nil(t, ss.Status().SaveOrUpdate(team2Member1Status))
-		t.Fatal(err)
+
-	}
 	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 {
+	require.Nil(t, ss.Status().SaveOrUpdate(team2Member2Status))
-		t.Fatal(err)
-	}
 
 	if result := <-ss.Status().GetAllFromTeam(team1.Id); result.Err != nil {
 		t.Fatal(result.Err)

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: ""}))
+	require.Nil(t, 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: ""}))
+	require.Nil(t, 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: 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)