From d0906a0c8561e9112f54f1b061c260e0eca47e14 Mon Sep 17 00:00:00 2001 From: Luke P Date: Mon, 1 Jul 2019 06:44:26 -0500 Subject: [PATCH] [MM-16517] Migrate "Status.Get" to Sync by default (#11367) * Test * Revert "Test" This reverts commit 2bbf335ee93ae7cd1dfeea4c805efcafeec8dae5. * Fixed typo in Makefile, line 564: 'persistent' misspelled * Fixed return of status_store.Get to return model.Status and model.AppError, removed connections to result var in same method, generated new mocks and fixed tests. * Fixed status_store.go under /sqlstore and /storetest in addition to removing personal comments * SQL Indentation fix for consistency * Revert merge change to store.go for SaveOrUpdate to avoid error introduced by merge * Changed StoreChannel back to *model.Apperror for SaveOrUpdate in store.go, fixed resulting error in storetest/status_store.go so it did not have conflicting types. Fixed spelling errors in same file * Test for status, err changed according to recommendation upon second look * Changed status variable on line 77 to blank identifier to stop it from shadowing line 24 declaration * It appears last statement in test was using 'status' variable where status is model.STATUS_ONLINE instead of model.STATUS_OFFLINE like status3, making comparison always be 'online != offline', which is always true, proceeding into if statement, guaranteeing test failure * Build fails consistently when line 75 has 'status' in if statement due to shadowing issue due to existing declaration Jenkins complains about in line 24. If this fails then new variable will be necessary * Renamed parameter to avoid overshadowing issue * Undid code addition mistake in storetest/status_store.go and updated line 29 accordingly to account for multiple values. * Remove status3 as used in line 71 in status_store.go. * Undid change in storetest/status_store.go on line 67 which checked for wrong thing --- Makefile | 2 +- app/status.go | 6 +----- store/sqlstore/status_store.go | 32 ++++++++++++---------------- store/store.go | 2 +- store/storetest/mocks/StatusStore.go | 19 ++++++++++++----- store/storetest/status_store.go | 17 +++++++-------- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/Makefile b/Makefile index 8beb1c7941..84e8affa20 100644 --- a/Makefile +++ b/Makefile @@ -562,7 +562,7 @@ clean: stop-docker ## Clean up everything except persistant server data. rm -f cmd/platform/cprofile*.out rm -f cmd/mattermost/cprofile*.out -nuke: clean clean-docker ## Clean plus removes persistant server data. +nuke: clean clean-docker ## Clean plus removes persistent server data. @echo BOOM rm -rf data diff --git a/app/status.go b/app/status.go index f594ee9c4f..b4fd169c00 100644 --- a/app/status.go +++ b/app/status.go @@ -349,11 +349,7 @@ func (a *App) GetStatus(userId string) (*model.Status, *model.AppError) { return status, nil } - result := <-a.Srv.Store.Status().Get(userId) - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.Status), nil + return a.Srv.Store.Status().Get(userId) } func (a *App) IsUserAway(lastActivityAt int64) bool { diff --git a/store/sqlstore/status_store.go b/store/sqlstore/status_store.go index 422544766c..4f67703570 100644 --- a/store/sqlstore/status_store.go +++ b/store/sqlstore/status_store.go @@ -54,26 +54,22 @@ func (s SqlStatusStore) SaveOrUpdate(status *model.Status) *model.AppError { return nil } -func (s SqlStatusStore) Get(userId string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var status model.Status +func (s SqlStatusStore) Get(userId string) (*model.Status, *model.AppError) { + var status model.Status - if err := s.GetReplica().SelectOne(&status, - `SELECT - * - FROM - Status - WHERE - UserId = :UserId`, map[string]interface{}{"UserId": userId}); err != nil { - if err == sql.ErrNoRows { - result.Err = model.NewAppError("SqlStatusStore.Get", MISSING_STATUS_ERROR, nil, err.Error(), http.StatusNotFound) - } else { - result.Err = model.NewAppError("SqlStatusStore.Get", "store.sql_status.get.app_error", nil, err.Error(), http.StatusInternalServerError) - } - } else { - result.Data = &status + if err := s.GetReplica().SelectOne(&status, + `SELECT + * + FROM + Status + WHERE + UserId = :UserId`, map[string]interface{}{"UserId": userId}); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlStatusStore.Get", MISSING_STATUS_ERROR, nil, err.Error(), http.StatusNotFound) } - }) + return nil, model.NewAppError("SqlStatusStore.Get", "store.sql_status.get.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return &status, nil } func (s SqlStatusStore) GetByIds(userIds []string) ([]*model.Status, *model.AppError) { diff --git a/store/store.go b/store/store.go index 4c00e4f89b..2b49dbd22c 100644 --- a/store/store.go +++ b/store/store.go @@ -468,7 +468,7 @@ type EmojiStore interface { type StatusStore interface { SaveOrUpdate(status *model.Status) *model.AppError - Get(userId string) StoreChannel + Get(userId string) (*model.Status, *model.AppError) GetByIds(userIds []string) ([]*model.Status, *model.AppError) GetOnlineAway() ([]*model.Status, *model.AppError) GetOnline() ([]*model.Status, *model.AppError) diff --git a/store/storetest/mocks/StatusStore.go b/store/storetest/mocks/StatusStore.go index 879c11b138..ba0cab9b36 100644 --- a/store/storetest/mocks/StatusStore.go +++ b/store/storetest/mocks/StatusStore.go @@ -14,19 +14,28 @@ type StatusStore struct { } // Get provides a mock function with given fields: userId -func (_m *StatusStore) Get(userId string) store.StoreChannel { +func (_m *StatusStore) Get(userId string) (*model.Status, *model.AppError) { ret := _m.Called(userId) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { + var r0 *model.Status + if rf, ok := ret.Get(0).(func(string) *model.Status); ok { r0 = rf(userId) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.Status) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(userId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetAllFromTeam provides a mock function with given fields: teamId diff --git a/store/storetest/status_store.go b/store/storetest/status_store.go index 23d20582e8..131c628d41 100644 --- a/store/storetest/status_store.go +++ b/store/storetest/status_store.go @@ -26,7 +26,7 @@ func testStatusStore(t *testing.T, ss store.Store) { status.LastActivityAt = 10 - if err := (<-ss.Status().Get(status.UserId)).Err; err != nil { + if _, err := ss.Status().Get(status.UserId); err != nil { t.Fatal(err) } @@ -68,11 +68,10 @@ func testStatusStore(t *testing.T, ss store.Store) { t.Fatal(err) } - if result := <-ss.Status().Get(status.UserId); result.Err != nil { - t.Fatal(result.Err) + if statusParameter, err := ss.Status().Get(status.UserId); err != nil { + t.Fatal(err) } else { - status := result.Data.(*model.Status) - if status.Status != model.STATUS_OFFLINE { + if statusParameter.Status != model.STATUS_OFFLINE { t.Fatal("should be offline") } } @@ -155,21 +154,21 @@ func testGetAllFromTeam(t *testing.T, ss store.Store) { team2Member2Status := &model.Status{UserId: team2Member2.UserId, Status: model.STATUS_OFFLINE, Manual: true, LastActivityAt: model.GetMillis(), ActiveChannel: ""} require.Nil(t, ss.Status().SaveOrUpdate(team2Member2Status)) - if statueses, err := ss.Status().GetAllFromTeam(team1.Id); err != nil { + if statuses, err := ss.Status().GetAllFromTeam(team1.Id); err != nil { t.Fatal(err) } else { assertStatuses([]*model.Status{ team1Member1Status, team1Member2Status, - }, statueses) + }, statuses) } - if statueses, err := ss.Status().GetAllFromTeam(team2.Id); err != nil { + if statuses, err := ss.Status().GetAllFromTeam(team2.Id); err != nil { t.Fatal(err) } else { assertStatuses([]*model.Status{ team2Member1Status, team2Member2Status, - }, statueses) + }, statuses) } }