[MM-15797] Migrate Session.Remove to Sync by default (#11069)

* [MM-15797] Migrate Session.Remove to Sync by default

* Remove unnecessary channel logic and simplify

* Refactor code to be more consistent with code base

* Rename variable to avoid shadow declaration

* Add missing checks in tests

* Regenerate the mocks

* Run gofmt over code

app/oauth.go

@@ -46,7 +46,6 @@ func (a *App) GetOAuthApp(appId string) (*model.OAuthApp, *model.AppError) {
 	if !*a.Config().ServiceSettings.EnableOAuthServiceProvider {
 		return nil, model.NewAppError("GetOAuthApp", "api.oauth.allow_oauth.turn_off.app_error", nil, "", http.StatusNotImplemented)
 	}
-
 	result := <-a.Srv.Store.OAuth().GetApp(appId)
 	if result.Err != nil {
 		return nil, result.Err
@@ -354,7 +353,9 @@ func (a *App) newSession(appName string, user *model.User) (*model.Session, *mod
 
 func (a *App) newSessionUpdateToken(appName string, accessData *model.AccessData, user *model.User) (*model.AccessResponse, *model.AppError) {
 	// Remove the previous session
-	<-a.Srv.Store.Session().Remove(accessData.Token)
+	if err := a.Srv.Store.Session().Remove(accessData.Token); err != nil {
+		mlog.Error(fmt.Sprint(err))
+	}
 
 	session, err := a.newSession(appName, user)
 	if err != nil {
@@ -477,7 +478,12 @@ func (a *App) RegenerateOAuthAppSecret(app *model.OAuthApp) (*model.OAuthApp, *m
 
 func (a *App) RevokeAccessToken(token string) *model.AppError {
 	session, _ := a.GetSession(token)
-	schan := a.Srv.Store.Session().Remove(token)
+
+	schan := make(chan *model.AppError, 1)
+	go func() {
+		schan <- a.Srv.Store.Session().Remove(token)
+		close(schan)
+	}()
 
 	if result := <-a.Srv.Store.OAuth().GetAccessData(token); result.Err != nil {
 		return model.NewAppError("RevokeAccessToken", "api.oauth.revoke_access_token.get.app_error", nil, "", http.StatusBadRequest)
@@ -487,7 +493,7 @@ func (a *App) RevokeAccessToken(token string) *model.AppError {
 		return model.NewAppError("RevokeAccessToken", "api.oauth.revoke_access_token.del_token.app_error", nil, "", http.StatusInternalServerError)
 	}
 
-	if result := <-schan; result.Err != nil {
+	if err := <-schan; err != nil {
 		return model.NewAppError("RevokeAccessToken", "api.oauth.revoke_access_token.del_session.app_error", nil, "", http.StatusInternalServerError)
 	}
 

app/session.go

@@ -102,8 +102,8 @@ func (a *App) RevokeAllSessions(userId string) *model.AppError {
 		if session.IsOAuth {
 			a.RevokeAccessToken(session.Token)
 		} else {
-			if result := <-a.Srv.Store.Session().Remove(session.Id); result.Err != nil {
+			if err := a.Srv.Store.Session().Remove(session.Id); err != nil {
-				return result.Err
+				return err
 			}
 		}
 	}
@@ -195,8 +195,8 @@ func (a *App) RevokeSession(session *model.Session) *model.AppError {
 			return err
 		}
 	} else {
-		if result := <-a.Srv.Store.Session().Remove(session.Id); result.Err != nil {
+		if err := a.Srv.Store.Session().Remove(session.Id); err != nil {
-			return result.Err
+			return err
 		}
 	}
 

store/sqlstore/session_store.go

@@ -144,13 +144,12 @@ func (me SqlSessionStore) GetSessionsWithActiveDeviceIds(userId string) ([]*mode
 	return sessions, nil
 }
 
-func (me SqlSessionStore) Remove(sessionIdOrToken string) store.StoreChannel {
+func (me SqlSessionStore) Remove(sessionIdOrToken string) *model.AppError {
-	return store.Do(func(result *store.StoreResult) {
+	_, err := me.GetMaster().Exec("DELETE FROM Sessions WHERE Id = :Id Or Token = :Token", map[string]interface{}{"Id": sessionIdOrToken, "Token": sessionIdOrToken})
-		_, err := me.GetMaster().Exec("DELETE FROM Sessions WHERE Id = :Id Or Token = :Token", map[string]interface{}{"Id": sessionIdOrToken, "Token": sessionIdOrToken})
+	if err != nil {
-		if err != nil {
+		return model.NewAppError("SqlSessionStore.RemoveSession", "store.sql_session.remove.app_error", nil, "id="+sessionIdOrToken+", err="+err.Error(), http.StatusInternalServerError)
-			result.Err = model.NewAppError("SqlSessionStore.RemoveSession", "store.sql_session.remove.app_error", nil, "id="+sessionIdOrToken+", err="+err.Error(), http.StatusInternalServerError)
+	}
-		}
+	return nil
-	})
 }
 
 func (me SqlSessionStore) RemoveAllSessions() *model.AppError {

store/store.go

@@ -316,7 +316,7 @@ type SessionStore interface {
 	Save(session *model.Session) (*model.Session, *model.AppError)
 	GetSessions(userId string) ([]*model.Session, *model.AppError)
 	GetSessionsWithActiveDeviceIds(userId string) ([]*model.Session, *model.AppError)
-	Remove(sessionIdOrToken string) StoreChannel
+	Remove(sessionIdOrToken string) *model.AppError
 	RemoveAllSessions() *model.AppError
 	PermanentDeleteSessionsByUser(teamId string) *model.AppError
 	UpdateLastActivityAt(sessionId string, time int64) *model.AppError

store/storetest/mocks/SessionStore.go

@@ -6,7 +6,6 @@ package mocks
 
 import mock "github.com/stretchr/testify/mock"
 import model "github.com/mattermost/mattermost-server/model"
-import store "github.com/mattermost/mattermost-server/store"
 
 // SessionStore is an autogenerated mock type for the SessionStore type
 type SessionStore struct {
@@ -133,15 +132,15 @@ func (_m *SessionStore) PermanentDeleteSessionsByUser(teamId string) *model.AppE
 }
 
 // Remove provides a mock function with given fields: sessionIdOrToken
-func (_m *SessionStore) Remove(sessionIdOrToken string) store.StoreChannel {
+func (_m *SessionStore) Remove(sessionIdOrToken string) *model.AppError {
 	ret := _m.Called(sessionIdOrToken)
 
-	var r0 store.StoreChannel
+	var r0 *model.AppError
-	if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok {
+	if rf, ok := ret.Get(0).(func(string) *model.AppError); ok {
 		r0 = rf(sessionIdOrToken)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(store.StoreChannel)
+			r0 = ret.Get(0).(*model.AppError)
 		}
 	}
 

store/storetest/session_store.go

@@ -124,8 +124,8 @@ func testSessionRemove(t *testing.T, ss store.Store) {
 		}
 	}
 
-	store.Must(ss.Session().Remove(s1.Id))
+	removeErr := ss.Session().Remove(s1.Id)
-
+	require.Nil(t, removeErr)
 	if _, err := ss.Session().Get(s1.Id); err == nil {
 		t.Fatal("should have been removed")
 	}
@@ -192,7 +192,8 @@ func testSessionRemoveToken(t *testing.T, ss store.Store) {
 		}
 	}
 
-	store.Must(ss.Session().Remove(s1.Token))
+	removeErr := ss.Session().Remove(s1.Token)
+	require.Nil(t, removeErr)
 
 	if _, err := ss.Session().Get(s1.Id); err == nil {
 		t.Fatal("should have been removed")
@@ -333,6 +334,9 @@ func testSessionCleanup(t *testing.T, ss store.Store) {
 	_, err = ss.Session().Get(s4.Id)
 	assert.NotNil(t, err)
 
-	store.Must(ss.Session().Remove(s1.Id))
+	removeErr := ss.Session().Remove(s1.Id)
-	store.Must(ss.Session().Remove(s2.Id))
+	require.Nil(t, removeErr)
+
+	removeErr = ss.Session().Remove(s2.Id)
+	require.Nil(t, removeErr)
 }