Migrate User.GetProfileByIds to sync by default (#11510)

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)

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)
 			}
 		}

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)
 	}

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 {

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)

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

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)
 	})
 }