[MM-62153] Avoid SELECT * in preference_store.go (#30081)

* refractored sql queries

* GetBuilder to Select Builder as preferences is slice (fix failing test)

* test: Add test for GetCategoryAndName preference method

* fix s/GetBuilder/SelectBuilder

* linting

* make mocks

* make store-layers

---------

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

server/channels/store/retrylayer/retrylayer.go

@@ -8802,11 +8802,11 @@ func (s *RetryLayerPreferenceStore) GetCategory(userID string, category string)
 
 }
 
-func (s *RetryLayerPreferenceStore) GetCategoryAndName(category string, nane string) (model.Preferences, error) {
+func (s *RetryLayerPreferenceStore) GetCategoryAndName(category string, name string) (model.Preferences, error) {
 
 	tries := 0
 	for {
-		result, err := s.PreferenceStore.GetCategoryAndName(category, nane)
+		result, err := s.PreferenceStore.GetCategoryAndName(category, name)
 		if err == nil {
 			return result, nil
 		}

server/channels/store/sqlstore/preference_store.go

@@ -14,10 +14,16 @@ import (
 
 type SqlPreferenceStore struct {
 	*SqlStore
+	preferenceSelectQuery sq.SelectBuilder
 }
 
 func newSqlPreferenceStore(sqlStore *SqlStore) store.PreferenceStore {
-	s := &SqlPreferenceStore{sqlStore}
+	s := &SqlPreferenceStore{SqlStore: sqlStore}
+
+	s.preferenceSelectQuery = s.getQueryBuilder().
+		Select("UserId", "Category", "Name", "Value").
+		From("Preferences")
+
 	return s
 }
 
@@ -121,18 +127,12 @@ func (s SqlPreferenceStore) saveTx(transaction *sqlxTxWrapper, preference *model
 
 func (s SqlPreferenceStore) Get(userId string, category string, name string) (*model.Preference, error) {
 	var preference model.Preference
-	query, args, err := s.getQueryBuilder().
+	query := s.preferenceSelectQuery.
-		Select("*").
-		From("Preferences").
 		Where(sq.Eq{"UserId": userId}).
 		Where(sq.Eq{"Category": category}).
-		Where(sq.Eq{"Name": name}).
+		Where(sq.Eq{"Name": name})
-		ToSql()
 
-	if err != nil {
+	if err := s.GetReplica().GetBuilder(&preference, query); err != nil {
-		return nil, errors.Wrap(err, "could not build sql query to get preference")
-	}
-	if err = s.GetReplica().Get(&preference, query, args...); err != nil {
 		return nil, errors.Wrapf(err, "failed to find Preference with userId=%s, category=%s, name=%s", userId, category, name)
 	}
 
@@ -141,50 +141,35 @@ func (s SqlPreferenceStore) Get(userId string, category string, name string) (*m
 
 func (s SqlPreferenceStore) GetCategoryAndName(category string, name string) (model.Preferences, error) {
 	var preferences model.Preferences
-	query, args, err := s.getQueryBuilder().
+	query := s.preferenceSelectQuery.
-		Select("*").
-		From("Preferences").
 		Where(sq.Eq{"Category": category}).
-		Where(sq.Eq{"Name": name}).
+		Where(sq.Eq{"Name": name})
-		ToSql()
+
-	if err != nil {
+	if err := s.GetReplica().SelectBuilder(&preferences, query); err != nil {
-		return nil, errors.Wrap(err, "could not build sql query to get preference")
+		return nil, errors.Wrapf(err, "failed to find Preferences with category=%s, name=%s", category, name)
-	}
-	if err = s.GetReplica().Select(&preferences, query, args...); err != nil {
-		return nil, errors.Wrapf(err, "failed to find Preference with category=%s, name=%s", category, name)
 	}
 	return preferences, nil
 }
 
 func (s SqlPreferenceStore) GetCategory(userId string, category string) (model.Preferences, error) {
 	var preferences model.Preferences
-	query, args, err := s.getQueryBuilder().
+	query := s.preferenceSelectQuery.
-		Select("*").
-		From("Preferences").
 		Where(sq.Eq{"UserId": userId}).
-		Where(sq.Eq{"Category": category}).
+		Where(sq.Eq{"Category": category})
-		ToSql()
+
-	if err != nil {
+	if err := s.GetReplica().SelectBuilder(&preferences, query); err != nil {
-		return nil, errors.Wrap(err, "could not build sql query to get preference")
+		return nil, errors.Wrapf(err, "failed to find Preferences with userId=%s, category=%s", userId, category)
-	}
-	if err = s.GetReplica().Select(&preferences, query, args...); err != nil {
-		return nil, errors.Wrapf(err, "failed to find Preference with userId=%s, category=%s", userId, category)
 	}
 	return preferences, nil
 }
 
 func (s SqlPreferenceStore) GetAll(userId string) (model.Preferences, error) {
 	var preferences model.Preferences
-	query, args, err := s.getQueryBuilder().
+	query := s.preferenceSelectQuery.
-		Select("*").
+		Where(sq.Eq{"UserId": userId})
-		From("Preferences").
+
-		Where(sq.Eq{"UserId": userId}).
+	if err := s.GetReplica().SelectBuilder(&preferences, query); err != nil {
-		ToSql()
+		return nil, errors.Wrapf(err, "failed to find Preferences with userId=%s", userId)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not build sql query to get preference")
-	}
-	if err = s.GetReplica().Select(&preferences, query, args...); err != nil {
-		return nil, errors.Wrapf(err, "failed to find Preference with userId=%s", userId)
 	}
 	return preferences, nil
 }
@@ -208,7 +193,6 @@ func (s SqlPreferenceStore) Delete(userId, category, name string) error {
 		Where(sq.Eq{"UserId": userId}).
 		Where(sq.Eq{"Category": category}).
 		Where(sq.Eq{"Name": name}).ToSql()
-
 	if err != nil {
 		return errors.Wrap(err, "could not build sql query to get delete preference")
 	}
@@ -225,7 +209,6 @@ func (s SqlPreferenceStore) DeleteCategory(userId string, category string) error
 		Delete("Preferences").
 		Where(sq.Eq{"UserId": userId}).
 		Where(sq.Eq{"Category": category}).ToSql()
-
 	if err != nil {
 		return errors.Wrap(err, "could not build sql query to get delete preference by category")
 	}
@@ -242,7 +225,6 @@ func (s SqlPreferenceStore) DeleteCategoryAndName(category string, name string)
 		Delete("Preferences").
 		Where(sq.Eq{"Name": name}).
 		Where(sq.Eq{"Category": category}).ToSql()
-
 	if err != nil {
 		return errors.Wrap(err, "could not build sql query to get delete preference by category and name")
 	}
@@ -299,7 +281,6 @@ func (s SqlPreferenceStore) CleanupFlagsBatch(limit int64) (int64, error) {
 		Where(sq.Eq{"Category": model.PreferenceCategoryFlaggedPost}).
 		Where(sq.Expr("name IN ("+nameInQ+")", nameInArgs...)).
 		ToSql()
-
 	if err != nil {
 		return int64(0), errors.Wrap(err, "could not build sql query to delete preference")
 	}

server/channels/store/store.go

@@ -666,7 +666,7 @@ type CommandWebhookStore interface {
 type PreferenceStore interface {
 	Save(preferences model.Preferences) error
 	GetCategory(userID string, category string) (model.Preferences, error)
-	GetCategoryAndName(category string, nane string) (model.Preferences, error)
+	GetCategoryAndName(category string, name string) (model.Preferences, error)
 	Get(userID string, category string, name string) (*model.Preference, error)
 	GetAll(userID string) (model.Preferences, error)
 	Delete(userID, category, name string) error

server/channels/store/storetest/mocks/PreferenceStore.go

@@ -242,9 +242,9 @@ func (_m *PreferenceStore) GetCategory(userID string, category string) (model.Pr
 	return r0, r1
 }
 
-// GetCategoryAndName provides a mock function with given fields: category, nane
+// GetCategoryAndName provides a mock function with given fields: category, name
-func (_m *PreferenceStore) GetCategoryAndName(category string, nane string) (model.Preferences, error) {
+func (_m *PreferenceStore) GetCategoryAndName(category string, name string) (model.Preferences, error) {
-	ret := _m.Called(category, nane)
+	ret := _m.Called(category, name)
 
 	if len(ret) == 0 {
 		panic("no return value specified for GetCategoryAndName")
@@ -253,10 +253,10 @@ func (_m *PreferenceStore) GetCategoryAndName(category string, nane string) (mod
 	var r0 model.Preferences
 	var r1 error
 	if rf, ok := ret.Get(0).(func(string, string) (model.Preferences, error)); ok {
-		return rf(category, nane)
+		return rf(category, name)
 	}
 	if rf, ok := ret.Get(0).(func(string, string) model.Preferences); ok {
-		r0 = rf(category, nane)
+		r0 = rf(category, name)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(model.Preferences)
@@ -264,7 +264,7 @@ func (_m *PreferenceStore) GetCategoryAndName(category string, nane string) (mod
 	}
 
 	if rf, ok := ret.Get(1).(func(string, string) error); ok {
-		r1 = rf(category, nane)
+		r1 = rf(category, name)
 	} else {
 		r1 = ret.Error(1)
 	}

server/channels/store/storetest/preference_store.go

@@ -18,6 +18,7 @@ func TestPreferenceStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlSt
 	t.Run("PreferenceSave", func(t *testing.T) { testPreferenceSave(t, rctx, ss) })
 	t.Run("PreferenceGet", func(t *testing.T) { testPreferenceGet(t, rctx, ss) })
 	t.Run("PreferenceGetCategory", func(t *testing.T) { testPreferenceGetCategory(t, rctx, ss) })
+	t.Run("PreferenceGetCategoryAndName", func(t *testing.T) { testPreferenceGetCategoryAndName(t, rctx, ss) })
 	t.Run("PreferenceGetAll", func(t *testing.T) { testPreferenceGetAll(t, rctx, ss) })
 	t.Run("PreferenceDeleteByUser", func(t *testing.T) { testPreferenceDeleteByUser(t, rctx, ss) })
 	t.Run("PreferenceDelete", func(t *testing.T) { testPreferenceDelete(t, rctx, ss) })
@@ -27,7 +28,7 @@ func TestPreferenceStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlSt
 	t.Run("PreferenceDeleteInvalidVisibleDmsGms", func(t *testing.T) { testDeleteInvalidVisibleDmsGms(t, rctx, ss, s) })
 }
 
-func testPreferenceSave(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceSave(t *testing.T, _ request.CTX, ss store.Store) {
 	id := model.NewId()
 
 	preferences := model.Preferences{
@@ -63,7 +64,7 @@ func testPreferenceSave(t *testing.T, rctx request.CTX, ss store.Store) {
 	}
 }
 
-func testPreferenceGet(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceGet(t *testing.T, _ request.CTX, ss store.Store) {
 	userId := model.NewId()
 	category := model.PreferenceCategoryDirectChannelShow
 	name := model.NewId()
@@ -103,7 +104,71 @@ func testPreferenceGet(t *testing.T, rctx request.CTX, ss store.Store) {
 	require.Error(t, err, "no error on getting a missing preference")
 }
 
-func testPreferenceGetCategory(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceGetCategoryAndName(t *testing.T, _ request.CTX, ss store.Store) {
+	userId := model.NewId()
+	category := model.PreferenceCategoryGroupChannelShow
+	name := model.NewId()
+
+	preferences := model.Preferences{
+		{
+			UserId:   userId,
+			Category: category,
+			Name:     name,
+			Value:    "user1",
+		},
+		// same category/name, different user
+		{
+			UserId:   model.NewId(),
+			Category: category,
+			Name:     name,
+			Value:    "otherUser",
+		},
+		// same user/name, different category
+		{
+			UserId:   userId,
+			Category: model.NewId(),
+			Name:     name,
+			Value:    "",
+		},
+		// same user/category, different name
+		{
+			UserId:   userId,
+			Category: category,
+			Name:     model.NewId(),
+			Value:    "",
+		},
+	}
+
+	err := ss.Preference().Save(preferences)
+	require.NoError(t, err)
+
+	actualPreferences, err := ss.Preference().GetCategoryAndName(category, name)
+	require.NoError(t, err)
+	assert.Equal(t, 2, len(actualPreferences), "got the wrong number of preferences")
+
+	for _, preference := range preferences {
+		// Preferences with empty values aren't expected.
+		if preference.Value == "" {
+			continue
+		}
+
+		found := false
+		for _, actualPreference := range actualPreferences {
+			if actualPreference == preference {
+				found = true
+			}
+		}
+
+		assert.True(t, found, "didnt find preference with value %s", preference.Value)
+	}
+
+	// make sure getting a missing preference category and name doesn't fail
+	actualPreferences, err = ss.Preference().GetCategoryAndName(model.NewId(), model.NewId())
+	require.NoError(t, err)
+	require.Equal(t, 0, len(actualPreferences), "shouldn't have got any preferences")
+}
+
+func testPreferenceGetCategory(t *testing.T, _ request.CTX, ss store.Store) {
 	userId := model.NewId()
 	category := model.PreferenceCategoryDirectChannelShow
 	name := model.NewId()
@@ -152,7 +217,7 @@ func testPreferenceGetCategory(t *testing.T, rctx request.CTX, ss store.Store) {
 	require.Equal(t, 0, len(preferencesByCategory), "shouldn't have got any preferences")
 }
 
-func testPreferenceGetAll(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceGetAll(t *testing.T, _ request.CTX, ss store.Store) {
 	userId := model.NewId()
 	category := model.PreferenceCategoryDirectChannelShow
 	name := model.NewId()
@@ -195,7 +260,7 @@ func testPreferenceGetAll(t *testing.T, rctx request.CTX, ss store.Store) {
 	}
 }
 
-func testPreferenceDeleteByUser(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceDeleteByUser(t *testing.T, _ request.CTX, ss store.Store) {
 	userId := model.NewId()
 	category := model.PreferenceCategoryDirectChannelShow
 	name := model.NewId()
@@ -233,7 +298,7 @@ func testPreferenceDeleteByUser(t *testing.T, rctx request.CTX, ss store.Store)
 	require.NoError(t, err)
 }
 
-func testPreferenceDelete(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceDelete(t *testing.T, _ request.CTX, ss store.Store) {
 	preference := model.Preference{
 		UserId:   model.NewId(),
 		Category: model.PreferenceCategoryDirectChannelShow,
@@ -255,7 +320,7 @@ func testPreferenceDelete(t *testing.T, rctx request.CTX, ss store.Store) {
 	assert.Empty(t, preferences, "should've returned no preferences")
 }
 
-func testPreferenceDeleteCategory(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceDeleteCategory(t *testing.T, _ request.CTX, ss store.Store) {
 	category := model.NewId()
 	userId := model.NewId()
 
@@ -288,7 +353,7 @@ func testPreferenceDeleteCategory(t *testing.T, rctx request.CTX, ss store.Store
 	assert.Empty(t, preferences, "should've returned no preferences")
 }
 
-func testPreferenceDeleteCategoryAndName(t *testing.T, rctx request.CTX, ss store.Store) {
+func testPreferenceDeleteCategoryAndName(t *testing.T, _ request.CTX, ss store.Store) {
 	category := model.NewId()
 	name := model.NewId()
 	userId := model.NewId()
@@ -404,7 +469,7 @@ func testPreferenceDeleteOrphanedRows(t *testing.T, rctx request.CTX, ss store.S
 	assert.NoError(t, nErr, "newer preference should not have been deleted")
 }
 
-func testDeleteInvalidVisibleDmsGms(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) {
+func testDeleteInvalidVisibleDmsGms(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) {
 	userId1 := model.NewId()
 	userId2 := model.NewId()
 	userId3 := model.NewId()

server/channels/store/timerlayer/timerlayer.go

@@ -6993,10 +6993,10 @@ func (s *TimerLayerPreferenceStore) GetCategory(userID string, category string)
 	return result, err
 }
 
-func (s *TimerLayerPreferenceStore) GetCategoryAndName(category string, nane string) (model.Preferences, error) {
+func (s *TimerLayerPreferenceStore) GetCategoryAndName(category string, name string) (model.Preferences, error) {
 	start := time.Now()
 
-	result, err := s.PreferenceStore.GetCategoryAndName(category, nane)
+	result, err := s.PreferenceStore.GetCategoryAndName(category, name)
 
 	elapsed := float64(time.Since(start)) / float64(time.Second)
 	if s.Root.Metrics != nil {