From 11f1accac6c5acac99930dc15806d637d3e02768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Fri, 5 Jul 2019 10:18:45 -0400 Subject: [PATCH] 11460 user.get all profiles in channel (#11515) --- app/channel.go | 8 +- app/notification.go | 7 +- app/user.go | 6 +- store/sqlstore/user_store.go | 123 +++++++++++++---------------- store/store.go | 4 +- store/storetest/mocks/UserStore.go | 38 ++++++--- store/storetest/user_store.go | 69 +++++++++------- 7 files changed, 137 insertions(+), 118 deletions(-) diff --git a/app/channel.go b/app/channel.go index 304eceb3c4..fbc72d22fe 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1893,9 +1893,9 @@ func (a *App) ViewChannel(view *model.ChannelView, userId string, currentSession } func (a *App) PermanentDeleteChannel(channel *model.Channel) *model.AppError { - channelUsers := <-a.Srv.Store.User().GetAllProfilesInChannel(channel.Id, false) - if channelUsers.Err != nil { - return channelUsers.Err + profiles, err := a.Srv.Store.User().GetAllProfilesInChannel(channel.Id, false) + if err != nil { + return err } if err := a.Srv.Store.Post().PermanentDeleteByChannel(channel.Id); err != nil { @@ -1920,7 +1920,7 @@ func (a *App) PermanentDeleteChannel(channel *model.Channel) *model.AppError { if a.IsESIndexingEnabled() { a.Srv.Go(func() { - for _, user := range channelUsers.Data.(map[string]*model.User) { + for _, user := range profiles { if err := a.indexUser(user); err != nil { mlog.Error("Encountered error indexing user", mlog.String("user_id", user.Id), mlog.Err(err)) } diff --git a/app/notification.go b/app/notification.go index 3facbdf968..a234542fb2 100644 --- a/app/notification.go +++ b/app/notification.go @@ -28,7 +28,12 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod return []string{}, nil } - pchan := a.Srv.Store.User().GetAllProfilesInChannel(channel.Id, true) + pchan := make(chan store.StoreResult, 1) + go func() { + props, err := a.Srv.Store.User().GetAllProfilesInChannel(channel.Id, true) + pchan <- store.StoreResult{Data: props, Err: err} + close(pchan) + }() cmnchan := make(chan store.StoreResult, 1) go func() { diff --git a/app/user.go b/app/user.go index ba31ca199b..c80504e65b 100644 --- a/app/user.go +++ b/app/user.go @@ -550,11 +550,7 @@ func (a *App) GetUsersInChannelPageByStatus(channelId string, page int, perPage } func (a *App) GetUsersNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) { - result := <-a.Srv.Store.User().GetProfilesNotInChannel(teamId, channelId, groupConstrained, offset, limit, viewRestrictions) - if result.Err != nil { - return nil, result.Err - } - return result.Data.([]*model.User), nil + return a.Srv.Store.User().GetProfilesNotInChannel(teamId, channelId, groupConstrained, offset, limit, viewRestrictions) } func (a *App) GetUsersNotInChannelMap(teamId string, channelId string, groupConstrained bool, offset int, limit int, asAdmin bool, viewRestrictions *model.ViewUsersRestrictions) (map[string]*model.User, *model.AppError) { diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 908d85e0c2..2b36d133a2 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -606,92 +606,83 @@ func (us SqlUserStore) GetProfilesInChannelByStatus(channelId string, offset int }) } -func (us SqlUserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if allowFromCache { - if cacheItem, ok := profilesInChannelCache.Get(channelId); ok { - if us.metrics != nil { - us.metrics.IncrementMemCacheHitCounter("Profiles in Channel") - } - result.Data = cacheItem.(map[string]*model.User) - return - } else { - if us.metrics != nil { - us.metrics.IncrementMemCacheMissCounter("Profiles in Channel") - } +func (us SqlUserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) (map[string]*model.User, *model.AppError) { + if allowFromCache { + if cacheItem, ok := profilesInChannelCache.Get(channelId); ok { + if us.metrics != nil { + us.metrics.IncrementMemCacheHitCounter("Profiles in Channel") } + return cacheItem.(map[string]*model.User), nil } else { if us.metrics != nil { us.metrics.IncrementMemCacheMissCounter("Profiles in Channel") } } - - query := us.usersQuery. - Join("ChannelMembers cm ON ( cm.UserId = u.Id )"). - Where("cm.ChannelId = ?", channelId). - Where("u.DeleteAt = 0"). - OrderBy("u.Username ASC") - - queryString, args, err := query.ToSql() - if err != nil { - result.Err = model.NewAppError("SqlUserStore.GetAllProfilesInChannel", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) - return + } else { + if us.metrics != nil { + us.metrics.IncrementMemCacheMissCounter("Profiles in Channel") } + } - var users []*model.User - if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { - result.Err = model.NewAppError("SqlUserStore.GetAllProfilesInChannel", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + query := us.usersQuery. + Join("ChannelMembers cm ON ( cm.UserId = u.Id )"). + Where("cm.ChannelId = ?", channelId). + Where("u.DeleteAt = 0"). + OrderBy("u.Username ASC") - userMap := make(map[string]*model.User) + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetAllProfilesInChannel", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) + } - for _, u := range users { - u.Sanitize(map[string]bool{}) - userMap[u.Id] = u - } + var users []*model.User + if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { + return nil, model.NewAppError("SqlUserStore.GetAllProfilesInChannel", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) + } - result.Data = userMap + userMap := make(map[string]*model.User) - if allowFromCache { - profilesInChannelCache.AddWithExpiresInSecs(channelId, userMap, PROFILES_IN_CHANNEL_CACHE_SEC) - } - }) + for _, u := range users { + u.Sanitize(map[string]bool{}) + userMap[u.Id] = u + } + + if allowFromCache { + profilesInChannelCache.AddWithExpiresInSecs(channelId, userMap, PROFILES_IN_CHANNEL_CACHE_SEC) + } + + return userMap, nil } -func (us SqlUserStore) GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - query := us.usersQuery. - Join("TeamMembers tm ON ( tm.UserId = u.Id AND tm.DeleteAt = 0 AND tm.TeamId = ? )", teamId). - LeftJoin("ChannelMembers cm ON ( cm.UserId = u.Id AND cm.ChannelId = ? )", channelId). - Where("cm.UserId IS NULL"). - OrderBy("u.Username ASC"). - Offset(uint64(offset)).Limit(uint64(limit)) +func (us SqlUserStore) GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) { + query := us.usersQuery. + Join("TeamMembers tm ON ( tm.UserId = u.Id AND tm.DeleteAt = 0 AND tm.TeamId = ? )", teamId). + LeftJoin("ChannelMembers cm ON ( cm.UserId = u.Id AND cm.ChannelId = ? )", channelId). + Where("cm.UserId IS NULL"). + OrderBy("u.Username ASC"). + Offset(uint64(offset)).Limit(uint64(limit)) - query = applyViewRestrictionsFilter(query, viewRestrictions, true) + query = applyViewRestrictionsFilter(query, viewRestrictions, true) - if groupConstrained { - query = applyChannelGroupConstrainedFilter(query, channelId) - } + if groupConstrained { + query = applyChannelGroupConstrainedFilter(query, channelId) + } - queryString, args, err := query.ToSql() - if err != nil { - result.Err = model.NewAppError("SqlUserStore.GetProfilesNotInChannel", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetProfilesNotInChannel", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) + } - var users []*model.User - if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { - result.Err = model.NewAppError("SqlUserStore.GetProfilesNotInChannel", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + var users []*model.User + if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { + return nil, model.NewAppError("SqlUserStore.GetProfilesNotInChannel", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) + } - for _, u := range users { - u.Sanitize(map[string]bool{}) - } + for _, u := range users { + u.Sanitize(map[string]bool{}) + } - result.Data = users - }) + return users, nil } func (us SqlUserStore) GetProfilesWithoutTeam(offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 6e5d46a38a..b6b8c1ac69 100644 --- a/store/store.go +++ b/store/store.go @@ -265,8 +265,8 @@ type UserStore interface { InvalidateProfilesInChannelCache(channelId string) GetProfilesInChannel(channelId string, offset int, limit int) StoreChannel GetProfilesInChannelByStatus(channelId string, offset int, limit int) StoreChannel - GetAllProfilesInChannel(channelId string, allowFromCache bool) StoreChannel - GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) StoreChannel + GetAllProfilesInChannel(channelId string, allowFromCache bool) (map[string]*model.User, *model.AppError) + GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) GetProfilesWithoutTeam(offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) StoreChannel GetProfilesByUsernames(usernames []string, viewRestrictions *model.ViewUsersRestrictions) StoreChannel GetAllProfiles(options *model.UserGetOptions) ([]*model.User, *model.AppError) diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index 4bded86d5a..14e50acd00 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -227,19 +227,28 @@ func (_m *UserStore) GetAllProfiles(options *model.UserGetOptions) ([]*model.Use } // GetAllProfilesInChannel provides a mock function with given fields: channelId, allowFromCache -func (_m *UserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) store.StoreChannel { +func (_m *UserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) (map[string]*model.User, *model.AppError) { ret := _m.Called(channelId, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, bool) store.StoreChannel); ok { + var r0 map[string]*model.User + if rf, ok := ret.Get(0).(func(string, bool) map[string]*model.User); ok { r0 = rf(channelId, allowFromCache) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(map[string]*model.User) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + r1 = rf(channelId, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetAllUsingAuthService provides a mock function with given fields: authService @@ -590,19 +599,28 @@ func (_m *UserStore) GetProfilesInChannelByStatus(channelId string, offset int, } // GetProfilesNotInChannel provides a mock function with given fields: teamId, channelId, groupConstrained, offset, limit, viewRestrictions -func (_m *UserStore) GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel { +func (_m *UserStore) GetProfilesNotInChannel(teamId string, channelId string, groupConstrained bool, offset int, limit int, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) { ret := _m.Called(teamId, channelId, groupConstrained, offset, limit, viewRestrictions) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string, bool, int, int, *model.ViewUsersRestrictions) store.StoreChannel); ok { + var r0 []*model.User + if rf, ok := ret.Get(0).(func(string, string, bool, int, int, *model.ViewUsersRestrictions) []*model.User); ok { r0 = rf(teamId, channelId, groupConstrained, offset, limit, viewRestrictions) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).([]*model.User) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, string, bool, int, int, *model.ViewUsersRestrictions) *model.AppError); ok { + r1 = rf(teamId, channelId, groupConstrained, offset, limit, viewRestrictions) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetProfilesNotInTeam provides a mock function with given fields: teamId, groupConstrained, offset, limit, viewRestrictions diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 907e66be93..5eebf7b06b 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -955,37 +955,41 @@ func testUserStoreGetAllProfilesInChannel(t *testing.T, ss store.Store) { })) t.Run("all profiles in channel 1, no caching", func(t *testing.T) { - result := <-ss.User().GetAllProfilesInChannel(c1.Id, false) - require.Nil(t, result.Err) + var profiles map[string]*model.User + profiles, err = ss.User().GetAllProfilesInChannel(c1.Id, false) + require.Nil(t, err) assert.Equal(t, map[string]*model.User{ u1.Id: sanitized(u1), u2.Id: sanitized(u2), u3.Id: sanitized(u3), - }, result.Data.(map[string]*model.User)) + }, profiles) }) t.Run("all profiles in channel 2, no caching", func(t *testing.T) { - result := <-ss.User().GetAllProfilesInChannel(c2.Id, false) - require.Nil(t, result.Err) + var profiles map[string]*model.User + profiles, err = ss.User().GetAllProfilesInChannel(c2.Id, false) + require.Nil(t, err) assert.Equal(t, map[string]*model.User{ u1.Id: sanitized(u1), - }, result.Data.(map[string]*model.User)) + }, profiles) }) t.Run("all profiles in channel 2, caching", func(t *testing.T) { - result := <-ss.User().GetAllProfilesInChannel(c2.Id, true) - require.Nil(t, result.Err) + var profiles map[string]*model.User + profiles, err = ss.User().GetAllProfilesInChannel(c2.Id, true) + require.Nil(t, err) assert.Equal(t, map[string]*model.User{ u1.Id: sanitized(u1), - }, result.Data.(map[string]*model.User)) + }, profiles) }) t.Run("all profiles in channel 2, caching [repeated]", func(t *testing.T) { - result := <-ss.User().GetAllProfilesInChannel(c2.Id, true) - require.Nil(t, result.Err) + var profiles map[string]*model.User + profiles, err = ss.User().GetAllProfilesInChannel(c2.Id, true) + require.Nil(t, err) assert.Equal(t, map[string]*model.User{ u1.Id: sanitized(u1), - }, result.Data.(map[string]*model.User)) + }, profiles) }) ss.User().InvalidateProfilesInChannelCacheByUser(u1.Id) @@ -1043,23 +1047,25 @@ func testUserStoreGetProfilesNotInChannel(t *testing.T, ss store.Store) { require.Nil(t, err) t.Run("get team 1, channel 1, offset 0, limit 100", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c1.Id, false, 0, 100, nil) - require.Nil(t, result.Err) + var profiles []*model.User + profiles, err = ss.User().GetProfilesNotInChannel(teamId, c1.Id, false, 0, 100, nil) + require.Nil(t, err) assert.Equal(t, []*model.User{ sanitized(u1), sanitized(u2), sanitized(u3), - }, result.Data.([]*model.User)) + }, profiles) }) t.Run("get team 1, channel 2, offset 0, limit 100", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c2.Id, false, 0, 100, nil) - require.Nil(t, result.Err) + var profiles []*model.User + profiles, err = ss.User().GetProfilesNotInChannel(teamId, c2.Id, false, 0, 100, nil) + require.Nil(t, err) assert.Equal(t, []*model.User{ sanitized(u1), sanitized(u2), sanitized(u3), - }, result.Data.([]*model.User)) + }, profiles) }) store.Must(ss.Channel().SaveMember(&model.ChannelMember{ @@ -1087,24 +1093,27 @@ func testUserStoreGetProfilesNotInChannel(t *testing.T, ss store.Store) { })) t.Run("get team 1, channel 1, offset 0, limit 100, after update", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c1.Id, false, 0, 100, nil) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{}, result.Data.([]*model.User)) + var profiles []*model.User + profiles, err = ss.User().GetProfilesNotInChannel(teamId, c1.Id, false, 0, 100, nil) + require.Nil(t, err) + assert.Equal(t, []*model.User{}, profiles) }) t.Run("get team 1, channel 2, offset 0, limit 100, after update", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c2.Id, false, 0, 100, nil) - require.Nil(t, result.Err) + var profiles []*model.User + profiles, err = ss.User().GetProfilesNotInChannel(teamId, c2.Id, false, 0, 100, nil) + require.Nil(t, err) assert.Equal(t, []*model.User{ sanitized(u2), sanitized(u3), - }, result.Data.([]*model.User)) + }, profiles) }) t.Run("get team 1, channel 2, offset 0, limit 0, setting group constrained when it's not", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c2.Id, true, 0, 100, nil) - require.Nil(t, result.Err) - assert.Empty(t, result.Data.([]*model.User)) + var profiles []*model.User + profiles, err = ss.User().GetProfilesNotInChannel(teamId, c2.Id, true, 0, 100, nil) + require.Nil(t, err) + assert.Empty(t, profiles) }) // create a group @@ -1131,11 +1140,11 @@ func testUserStoreGetProfilesNotInChannel(t *testing.T, ss store.Store) { require.Nil(t, err) t.Run("get team 1, channel 2, offset 0, limit 0, setting group constrained", func(t *testing.T) { - result := <-ss.User().GetProfilesNotInChannel(teamId, c2.Id, true, 0, 100, nil) - require.Nil(t, result.Err) + profiles, err := ss.User().GetProfilesNotInChannel(teamId, c2.Id, true, 0, 100, nil) + require.Nil(t, err) assert.Equal(t, []*model.User{ sanitized(u2), - }, result.Data.([]*model.User)) + }, profiles) }) }