diff --git a/app/channel.go b/app/channel.go index d490b970da..304eceb3c4 100644 --- a/app/channel.go +++ b/app/channel.go @@ -434,11 +434,10 @@ func (a *App) createGroupChannel(userIds []string, creatorId string) (*model.Cha return nil, model.NewAppError("CreateGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest) } - result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true) - if result.Err != nil { - return nil, result.Err + users, err := a.Srv.Store.User().GetProfileByIds(userIds, nil, true) + if err != nil { + return nil, err } - users := result.Data.([]*model.User) if len(users) != len(userIds) { return nil, model.NewAppError("CreateGroupChannel", "api.channel.create_group.bad_user.app_error", nil, "user_ids="+model.ArrayToJson(userIds), http.StatusBadRequest) @@ -483,11 +482,10 @@ func (a *App) GetGroupChannel(userIds []string) (*model.Channel, *model.AppError return nil, model.NewAppError("GetGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest) } - result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true) - if result.Err != nil { - return nil, result.Err + users, err := a.Srv.Store.User().GetProfileByIds(userIds, nil, true) + if err != nil { + return nil, err } - users := result.Data.([]*model.User) if len(users) != len(userIds) { return nil, model.NewAppError("GetGroupChannel", "api.channel.create_group.bad_user.app_error", nil, "user_ids="+model.ArrayToJson(userIds), http.StatusBadRequest) diff --git a/app/slack.go b/app/slack.go index e55dd853da..4e25a0c679 100644 --- a/app/slack.go +++ b/app/slack.go @@ -66,8 +66,8 @@ func replaceUserIds(userStore store.UserStore, text string) string { userIds = append(userIds, match[1]) } - if res := <-userStore.GetProfileByIds(userIds, nil, true); res.Err == nil { - for _, user := range res.Data.([]*model.User) { + if users, err := userStore.GetProfileByIds(userIds, nil, true); err == nil { + for _, user := range users { text = strings.Replace(text, "<@"+user.Id+">", "@"+user.Username, -1) } } diff --git a/app/user.go b/app/user.go index 61df01202a..0fe1191251 100644 --- a/app/user.go +++ b/app/user.go @@ -616,12 +616,12 @@ func (a *App) GetChannelGroupUsers(channelID string) ([]*model.User, *model.AppE func (a *App) GetUsersByIds(userIds []string, options *store.UserGetByIdsOpts) ([]*model.User, *model.AppError) { allowFromCache := options.ViewRestrictions == nil - result := <-a.Srv.Store.User().GetProfileByIds(userIds, options, allowFromCache) - if result.Err != nil { - return nil, result.Err + users, err := a.Srv.Store.User().GetProfileByIds(userIds, options, allowFromCache) + if err != nil { + return nil, err } - return a.sanitizeProfiles(result.Data.([]*model.User), options.IsAdmin), nil + return a.sanitizeProfiles(users, options.IsAdmin), nil } func (a *App) GetUsersByGroupChannelIds(channelIds []string, asAdmin bool) (map[string][]*model.User, *model.AppError) { @@ -1711,13 +1711,11 @@ func (a *App) esSearchUsersInTeam(teamId, term string, options *model.UserSearch return nil, err } - result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) - if result.Err != nil { - return nil, result.Err + users, err := a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) + if err != nil { + return nil, err } - users := result.Data.([]*model.User) - for _, user := range users { a.SanitizeProfile(user, options.IsAdmin) } @@ -1797,8 +1795,21 @@ func (a *App) esAutocompleteUsersInChannel(teamId, channelId, term string, optio if err != nil { return nil, err } - uchan := a.Srv.Store.User().GetProfileByIds(uchanIds, nil, false) - nuchan := a.Srv.Store.User().GetProfileByIds(nuchanIds, nil, false) + + uchan := make(chan store.StoreResult, 1) + go func() { + users, err := a.Srv.Store.User().GetProfileByIds(uchanIds, nil, false) + uchan <- store.StoreResult{Data: users, Err: err} + close(uchan) + }() + + nuchan := make(chan store.StoreResult, 1) + go func() { + users, err := a.Srv.Store.User().GetProfileByIds(nuchanIds, nil, false) + nuchan <- store.StoreResult{Data: users, Err: err} + close(nuchan) + }() + autocomplete := &model.UserAutocompleteInChannel{} result := <-uchan @@ -1900,12 +1911,11 @@ func (a *App) esAutocompleteUsersInTeam(teamId, term string, options *model.User return nil, err } - result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) - if result.Err != nil { - return nil, result.Err + users, err := a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) + if err != nil { + return nil, err } - users := result.Data.([]*model.User) for _, user := range users { a.SanitizeProfile(user, options.IsAdmin) } diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 57d103b5b5..99fd843997 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -837,80 +837,75 @@ func (us SqlUserStore) GetNewUsersForTeam(teamId string, offset, limit int, view return users, nil } -func (us SqlUserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if options == nil { - options = &store.UserGetByIdsOpts{} - } +func (us SqlUserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) ([]*model.User, *model.AppError) { + if options == nil { + options = &store.UserGetByIdsOpts{} + } - users := []*model.User{} - remainingUserIds := make([]string, 0) + users := []*model.User{} + remainingUserIds := make([]string, 0) - if allowFromCache { - for _, userId := range userIds { - if cacheItem, ok := profileByIdsCache.Get(userId); ok { - u := &model.User{} - *u = *cacheItem.(*model.User) + if allowFromCache { + for _, userId := range userIds { + if cacheItem, ok := profileByIdsCache.Get(userId); ok { + u := &model.User{} + *u = *cacheItem.(*model.User) - if options.Since == 0 || u.UpdateAt > options.Since { - users = append(users, u) - } - } else { - remainingUserIds = append(remainingUserIds, userId) + if options.Since == 0 || u.UpdateAt > options.Since { + users = append(users, u) } - } - if us.metrics != nil { - us.metrics.AddMemCacheHitCounter("Profile By Ids", float64(len(users))) - us.metrics.AddMemCacheMissCounter("Profile By Ids", float64(len(remainingUserIds))) - } - } else { - remainingUserIds = userIds - if us.metrics != nil { - us.metrics.AddMemCacheMissCounter("Profile By Ids", float64(len(remainingUserIds))) + } else { + remainingUserIds = append(remainingUserIds, userId) } } - - // If everything came from the cache then just return - if len(remainingUserIds) == 0 { - result.Data = users - return + if us.metrics != nil { + us.metrics.AddMemCacheHitCounter("Profile By Ids", float64(len(users))) + us.metrics.AddMemCacheMissCounter("Profile By Ids", float64(len(remainingUserIds))) } - - query := us.usersQuery. - Where(map[string]interface{}{ - "u.Id": remainingUserIds, - }). - OrderBy("u.Username ASC") - - if options.Since > 0 { - query = query.Where(squirrel.Gt(map[string]interface{}{ - "u.UpdateAt": options.Since, - })) + } else { + remainingUserIds = userIds + if us.metrics != nil { + us.metrics.AddMemCacheMissCounter("Profile By Ids", float64(len(remainingUserIds))) } + } - query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true) + // If everything came from the cache then just return + if len(remainingUserIds) == 0 { + return users, nil + } - queryString, args, err := query.ToSql() - if err != nil { - result.Err = model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + query := us.usersQuery. + Where(map[string]interface{}{ + "u.Id": remainingUserIds, + }). + OrderBy("u.Username ASC") - if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { - result.Err = model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + if options.Since > 0 { + query = query.Where(squirrel.Gt(map[string]interface{}{ + "u.UpdateAt": options.Since, + })) + } - for _, u := range users { - u.Sanitize(map[string]bool{}) + query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true) - cpy := &model.User{} - *cpy = *u - profileByIdsCache.AddWithExpiresInSecs(cpy.Id, cpy, PROFILE_BY_IDS_CACHE_SEC) - } + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.app_error", nil, err.Error(), http.StatusInternalServerError) + } - result.Data = users - }) + if _, err := us.GetReplica().Select(&users, queryString, args...); err != nil { + return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + for _, u := range users { + u.Sanitize(map[string]bool{}) + + cpy := &model.User{} + *cpy = *u + profileByIdsCache.AddWithExpiresInSecs(cpy.Id, cpy, PROFILE_BY_IDS_CACHE_SEC) + } + + return users, nil } type UserWithChannel struct { diff --git a/store/store.go b/store/store.go index 3fd8c7d042..4f2bc62c15 100644 --- a/store/store.go +++ b/store/store.go @@ -271,7 +271,7 @@ type UserStore interface { GetProfilesByUsernames(usernames []string, viewRestrictions *model.ViewUsersRestrictions) StoreChannel GetAllProfiles(options *model.UserGetOptions) StoreChannel GetProfiles(options *model.UserGetOptions) StoreChannel - GetProfileByIds(userIds []string, options *UserGetByIdsOpts, allowFromCache bool) StoreChannel + GetProfileByIds(userIds []string, options *UserGetByIdsOpts, allowFromCache bool) ([]*model.User, *model.AppError) GetProfileByGroupChannelIdsForUser(userId string, channelIds []string) (map[string][]*model.User, *model.AppError) InvalidatProfileCacheForUser(userId string) GetByEmail(email string) (*model.User, *model.AppError) diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index fd7f4dccd0..5f5dc71b4f 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -485,19 +485,28 @@ func (_m *UserStore) GetProfileByGroupChannelIdsForUser(userId string, channelId } // GetProfileByIds provides a mock function with given fields: userIds, options, allowFromCache -func (_m *UserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel { +func (_m *UserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) ([]*model.User, *model.AppError) { ret := _m.Called(userIds, options, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func([]string, *store.UserGetByIdsOpts, bool) store.StoreChannel); ok { + var r0 []*model.User + if rf, ok := ret.Get(0).(func([]string, *store.UserGetByIdsOpts, bool) []*model.User); ok { r0 = rf(userIds, options, allowFromCache) } 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, *store.UserGetByIdsOpts, bool) *model.AppError); ok { + r1 = rf(userIds, options, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetProfiles provides a mock function with given fields: options diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index b33d6f678d..ffb4044402 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -1190,43 +1190,43 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) { defer func() { require.Nil(t, ss.User().PermanentDelete(u4.Id)) }() t.Run("get u1 by id, no caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, false) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User)) + users, err := ss.User().GetProfileByIds([]string{u1.Id}, nil, false) + require.Nil(t, err) + assert.Equal(t, []*model.User{sanitized(u1)}, users) }) t.Run("get u1 by id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, true) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User)) + users, err := ss.User().GetProfileByIds([]string{u1.Id}, nil, true) + require.Nil(t, err) + assert.Equal(t, []*model.User{sanitized(u1)}, users) }) t.Run("get u1, u2, u3 by id, no caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, false) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User)) + users, err := ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, false) + require.Nil(t, err) + assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, users) }) t.Run("get u1, u2, u3 by id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, true) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User)) + users, err := ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, true) + require.Nil(t, err) + assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, users) }) t.Run("get unknown id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{"123"}, nil, true) - require.Nil(t, result.Err) - assert.Equal(t, []*model.User{}, result.Data.([]*model.User)) + users, err := ss.User().GetProfileByIds([]string{"123"}, nil, true) + require.Nil(t, err) + assert.Equal(t, []*model.User{}, users) }) t.Run("should only return users with UpdateAt greater than the since time", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id, u4.Id}, &store.UserGetByIdsOpts{ + users, err := ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id, u4.Id}, &store.UserGetByIdsOpts{ Since: u2.CreateAt, }, true) - require.Nil(t, result.Err) + require.Nil(t, err) // u3 comes from the cache, and u4 does not - assert.Equal(t, []*model.User{sanitized(u3), sanitized(u4)}, result.Data.([]*model.User)) + assert.Equal(t, []*model.User{sanitized(u3), sanitized(u4)}, users) }) }