From 00bb39b2bd894711c8e2a27200565f17791f2177 Mon Sep 17 00:00:00 2001 From: Sheshagiri Rao Mallipedhi Date: Wed, 26 Jun 2019 19:50:02 +0530 Subject: [PATCH] Migrate Channel.GetAllChannelMembersForUser to Sync by default (#11234) * Migrate Channel.GetAllChannelMembersForUser to Sync by default * fix gofmt for sqlstore/channel_store.go --- app/authorization.go | 5 +- app/user.go | 16 +++---- app/web_conn.go | 8 ++-- store/sqlstore/channel_store.go | 68 +++++++++++++-------------- store/store.go | 2 +- store/storetest/channel_store.go | 9 ++-- store/storetest/mocks/ChannelStore.go | 19 ++++++-- 7 files changed, 64 insertions(+), 63 deletions(-) diff --git a/app/authorization.go b/app/authorization.go index 7c855b2f8f..995cc9e3bb 100644 --- a/app/authorization.go +++ b/app/authorization.go @@ -40,11 +40,10 @@ func (a *App) SessionHasPermissionToChannel(session model.Session, channelId str return false } - cmc := a.Srv.Store.Channel().GetAllChannelMembersForUser(session.UserId, true, true) + ids, err := a.Srv.Store.Channel().GetAllChannelMembersForUser(session.UserId, true, true) var channelRoles []string - if cmcresult := <-cmc; cmcresult.Err == nil { - ids := cmcresult.Data.(map[string]string) + if err == nil { if roles, ok := ids[channelId]; ok { channelRoles = strings.Fields(roles) if a.RolesGrantPermission(channelRoles, permission.Id) { diff --git a/app/user.go b/app/user.go index 225fa67631..bd3c430658 100644 --- a/app/user.go +++ b/app/user.go @@ -199,13 +199,13 @@ func (a *App) indexUser(user *model.User) *model.AppError { userTeamsIds = append(userTeamsIds, team.Id) } - userChannelMembers := <-a.Srv.Store.Channel().GetAllChannelMembersForUser(user.Id, false, true) - if userChannelMembers.Err != nil { - return userChannelMembers.Err + userChannelMembers, err := a.Srv.Store.Channel().GetAllChannelMembersForUser(user.Id, false, true) + if err != nil { + return err } userChannelsIds := []string{} - for channelId := range userChannelMembers.Data.(map[string]string) { + for channelId := range userChannelMembers { userChannelsIds = append(userChannelsIds, channelId) } @@ -2167,13 +2167,13 @@ func (a *App) GetViewUsersRestrictions(userId string) (*model.ViewUsersRestricti return &model.ViewUsersRestrictions{Teams: teamIdsWithPermission}, nil } - userChannelMembers := <-a.Srv.Store.Channel().GetAllChannelMembersForUser(userId, true, true) - if userChannelMembers.Err != nil { - return nil, userChannelMembers.Err + userChannelMembers, err := a.Srv.Store.Channel().GetAllChannelMembersForUser(userId, true, true) + if err != nil { + return nil, err } channelIds := []string{} - for channelId := range userChannelMembers.Data.(map[string]string) { + for channelId := range userChannelMembers { channelIds = append(channelIds, channelId) } diff --git a/app/web_conn.go b/app/web_conn.go index 59ca5322dd..b2d5d3dc20 100644 --- a/app/web_conn.go +++ b/app/web_conn.go @@ -336,12 +336,12 @@ func (webCon *WebConn) ShouldSendEvent(msg *model.WebSocketEvent) bool { } if webCon.AllChannelMembers == nil { - result := <-webCon.App.Srv.Store.Channel().GetAllChannelMembersForUser(webCon.UserId, true, false) - if result.Err != nil { - mlog.Error("webhub.shouldSendEvent: " + result.Err.Error()) + result, err := webCon.App.Srv.Store.Channel().GetAllChannelMembersForUser(webCon.UserId, true, false) + if err != nil { + mlog.Error("webhub.shouldSendEvent: " + err.Error()) return false } - webCon.AllChannelMembers = result.Data.(map[string]string) + webCon.AllChannelMembers = result webCon.LastAllChannelMembersTime = model.GetMillis() } diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index dd2c488f88..02e26e44b1 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1465,13 +1465,12 @@ func (s SqlChannelStore) IsUserInChannelUseCache(userId string, channelId string s.metrics.IncrementMemCacheMissCounter("All Channel Members for User") } - result := <-s.GetAllChannelMembersForUser(userId, true, false) - if result.Err != nil { - mlog.Error("SqlChannelStore.IsUserInChannelUseCache: " + result.Err.Error()) + ids, err := s.GetAllChannelMembersForUser(userId, true, false) + if err != nil { + mlog.Error("SqlChannelStore.IsUserInChannelUseCache: " + err.Error()) return false } - ids := result.Data.(map[string]string) if _, ok := ids[channelId]; ok { return true } @@ -1512,33 +1511,32 @@ func (s SqlChannelStore) GetMemberForPost(postId string, userId string) (*model. return dbMember.ToModel(), nil } -func (s SqlChannelStore) GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - cache_key := userId - if includeDeleted { - cache_key += "_deleted" - } - if allowFromCache { - if cacheItem, ok := allChannelMembersForUserCache.Get(cache_key); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("All Channel Members for User") - } - result.Data = cacheItem.(map[string]string) - return +func (s SqlChannelStore) GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) (map[string]string, *model.AppError) { + cache_key := userId + if includeDeleted { + cache_key += "_deleted" + } + if allowFromCache { + if cacheItem, ok := allChannelMembersForUserCache.Get(cache_key); ok { + if s.metrics != nil { + s.metrics.IncrementMemCacheHitCounter("All Channel Members for User") } + ids := cacheItem.(map[string]string) + return ids, nil } + } - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("All Channel Members for User") - } + if s.metrics != nil { + s.metrics.IncrementMemCacheMissCounter("All Channel Members for User") + } - var deletedClause string - if !includeDeleted { - deletedClause = "Channels.DeleteAt = 0 AND" - } + var deletedClause string + if !includeDeleted { + deletedClause = "Channels.DeleteAt = 0 AND" + } - var data allChannelMembers - _, err := s.GetReplica().Select(&data, ` + var data allChannelMembers + _, err := s.GetReplica().Select(&data, ` SELECT ChannelMembers.ChannelId, ChannelMembers.Roles, ChannelMembers.SchemeGuest, ChannelMembers.SchemeUser, ChannelMembers.SchemeAdmin, @@ -1562,18 +1560,16 @@ func (s SqlChannelStore) GetAllChannelMembersForUser(userId string, allowFromCac `+deletedClause+` ChannelMembers.UserId = :UserId`, map[string]interface{}{"UserId": userId}) - if err != nil { - result.Err = model.NewAppError("SqlChannelStore.GetAllChannelMembersForUser", "store.sql_channel.get_channels.get.app_error", nil, "userId="+userId+", err="+err.Error(), http.StatusInternalServerError) - return - } + if err != nil { + return nil, model.NewAppError("SqlChannelStore.GetAllChannelMembersForUser", "store.sql_channel.get_channels.get.app_error", nil, "userId="+userId+", err="+err.Error(), http.StatusInternalServerError) + } - ids := data.ToMapStringString() - result.Data = ids + ids := data.ToMapStringString() - if allowFromCache { - allChannelMembersForUserCache.AddWithExpiresInSecs(cache_key, ids, ALL_CHANNEL_MEMBERS_FOR_USER_CACHE_SEC) - } - }) + if allowFromCache { + allChannelMembersForUserCache.AddWithExpiresInSecs(cache_key, ids, ALL_CHANNEL_MEMBERS_FOR_USER_CACHE_SEC) + } + return ids, nil } func (s SqlChannelStore) InvalidateCacheForChannelMembersNotifyProps(channelId string) { diff --git a/store/store.go b/store/store.go index 04f9f7fc24..8b6d866210 100644 --- a/store/store.go +++ b/store/store.go @@ -163,7 +163,7 @@ type ChannelStore interface { GetMembers(channelId string, offset, limit int) (*model.ChannelMembers, *model.AppError) GetMember(channelId string, userId string) (*model.ChannelMember, *model.AppError) GetChannelMembersTimezones(channelId string) ([]model.StringMap, *model.AppError) - GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) StoreChannel + GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) (map[string]string, *model.AppError) InvalidateAllChannelMembersForUser(userId string) IsUserInChannelUseCache(userId string, channelId string) bool GetAllChannelMembersNotifyPropsForChannel(channelId string, allowFromCache bool) (map[string]model.StringMap, *model.AppError) diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 7c0da9347a..547c3c0575 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -1046,20 +1046,17 @@ func testChannelStoreGetChannels(t *testing.T, ss store.Store) { t.Fatal("missing channel") } - acresult := <-ss.Channel().GetAllChannelMembersForUser(m1.UserId, false, false) - ids := acresult.Data.(map[string]string) + ids, _ := ss.Channel().GetAllChannelMembersForUser(m1.UserId, false, false) if _, ok := ids[o1.Id]; !ok { t.Fatal("missing channel") } - acresult2 := <-ss.Channel().GetAllChannelMembersForUser(m1.UserId, true, false) - ids2 := acresult2.Data.(map[string]string) + ids2, _ := ss.Channel().GetAllChannelMembersForUser(m1.UserId, true, false) if _, ok := ids2[o1.Id]; !ok { t.Fatal("missing channel") } - acresult3 := <-ss.Channel().GetAllChannelMembersForUser(m1.UserId, true, false) - ids3 := acresult3.Data.(map[string]string) + ids3, _ := ss.Channel().GetAllChannelMembersForUser(m1.UserId, true, false) if _, ok := ids3[o1.Id]; !ok { t.Fatal("missing channel") } diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 13ce11f6d9..3436d30db6 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -222,19 +222,28 @@ func (_m *ChannelStore) GetAll(teamId string) ([]*model.Channel, *model.AppError } // GetAllChannelMembersForUser provides a mock function with given fields: userId, allowFromCache, includeDeleted -func (_m *ChannelStore) GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) store.StoreChannel { +func (_m *ChannelStore) GetAllChannelMembersForUser(userId string, allowFromCache bool, includeDeleted bool) (map[string]string, *model.AppError) { ret := _m.Called(userId, allowFromCache, includeDeleted) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, bool, bool) store.StoreChannel); ok { + var r0 map[string]string + if rf, ok := ret.Get(0).(func(string, bool, bool) map[string]string); ok { r0 = rf(userId, allowFromCache, includeDeleted) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(map[string]string) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, bool, bool) *model.AppError); ok { + r1 = rf(userId, allowFromCache, includeDeleted) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetAllChannelMembersNotifyPropsForChannel provides a mock function with given fields: channelId, allowFromCache