Made structural changes to server-side Preference code based on feedback

4 files changed, 149 insertions, 294 deletions

diff --git a/store/sql_preference_store.go b/store/sql_preference_store.go
index 8d4e99109..04bb5e4ee 100644
--- a/store/sql_preference_store.go
+++ b/store/sql_preference_store.go
@@ -4,6 +4,7 @@
 package store
 
 import (
+	"github.com/go-gorp/gorp"
 	"github.com/mattermost/platform/model"
 	"github.com/mattermost/platform/utils"
 )
@@ -36,103 +37,36 @@ func (s SqlPreferenceStore) CreateIndexesIfNotExists() {
 	s.CreateIndexIfNotExists("idx_preferences_name", "Preferences", "Name")
 }
 
-func (s SqlPreferenceStore) Save(preference *model.Preference) StoreChannel {
-	storeChannel := make(StoreChannel)
-
-	go func() {
-		storeChannel <- s.save(s.GetMaster(), preference)
-		close(storeChannel)
-	}()
-
-	return storeChannel
-}
-
-func (s SqlPreferenceStore) save(queryable Queryable, preference *model.Preference) StoreResult {
-	result := StoreResult{}
-
-	if result.Err = preference.IsValid(); result.Err != nil {
-		return result
-	}
-
-	if err := queryable.Insert(preference); err != nil {
-		if IsUniqueConstraintError(err.Error(), "UserId", "preferences_pkey") {
-			result.Err = model.NewAppError("SqlPreferenceStore.Save", "A preference with that user id, category, name, and alt id already exists",
-				"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
-		} else {
-			result.Err = model.NewAppError("SqlPreferenceStore.Save", "We couldn't save the preference",
-				"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
-		}
-	} else {
-		result.Data = preference
-	}
-
-	return result
-}
-
-func (s SqlPreferenceStore) Update(preference *model.Preference) StoreChannel {
-	storeChannel := make(StoreChannel)
-
-	go func() {
-		storeChannel <- s.update(s.GetMaster(), preference)
-		close(storeChannel)
-	}()
-
-	return storeChannel
-}
-
-func (s SqlPreferenceStore) update(queryable Queryable, preference *model.Preference) StoreResult {
-	result := StoreResult{}
-
-	if result.Err = preference.IsValid(); result.Err != nil {
-		return result
-	}
-
-	if count, err := queryable.Update(preference); err != nil {
-		result.Err = model.NewAppError("SqlPreferenceStore.Update", "We couldn't update the preference",
-			"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
-	} else {
-		result.Data = count
-	}
-
-	return result
-}
-
-func (s SqlPreferenceStore) SaveOrUpdate(preferences ...*model.Preference) StoreChannel {
+func (s SqlPreferenceStore) Save(preferences *model.Preferences) StoreChannel {
 	storeChannel := make(StoreChannel)
 
 	go func() {
 		result := StoreResult{}
 
-		db := s.GetReplica()
-
-		if len(preferences) > 1 {
-			// wrap in a transaction so that if one fails, everything fails
-			transaction, err := db.Begin()
-			if err != nil {
-				result.Err = model.NewAppError("SqlPreferenceStore.SaveOrUpdateMultiple", "Unable to open transaction to update preferences", err.Error())
-			} else {
-				for _, preference := range preferences {
-					if upsertResult := s.saveOrUpdate(transaction, preference); upsertResult.Err != nil {
-						result = upsertResult
-						break
-					}
+		// wrap in a transaction so that if one fails, everything fails
+		transaction, err := s.GetReplica().Begin()
+		if err != nil {
+			result.Err = model.NewAppError("SqlPreferenceStore.Save", "Unable to open transaction to save preferences", err.Error())
+		} else {
+			for _, preference := range *preferences {
+				if upsertResult := s.save(transaction, preference); upsertResult.Err != nil {
+					result = upsertResult
+					break
 				}
+			}
 
-				if result.Err == nil {
-					if err := transaction.Commit(); err != nil {
-						// don't need to rollback here since the transaction is already closed
-						result.Err = model.NewAppError("SqlPreferenceStore.SaveOrUpdateMultiple", "Unable to commit transaction to update preferences", err.Error())
-					} else {
-						result.Data = len(preferences)
-					}
+			if result.Err == nil {
+				if err := transaction.Commit(); err != nil {
+					// don't need to rollback here since the transaction is already closed
+					result.Err = model.NewAppError("SqlPreferenceStore.Save", "Unable to commit transaction to save preferences", err.Error())
 				} else {
-					if err := transaction.Rollback(); err != nil {
-						result.Err = model.NewAppError("SqlPreferenceStore.SaveOrUpdateMultiple", "Unable to rollback transaction to update preferences", err.Error())
-					}
+					result.Data = len(*preferences)
+				}
+			} else {
+				if err := transaction.Rollback(); err != nil {
+					result.Err = model.NewAppError("SqlPreferenceStore.Save", "Unable to rollback transaction to save preferences", err.Error())
 				}
 			}
-		} else {
-			result = s.saveOrUpdate(db, preferences[0])
 		}
 
 		storeChannel <- result
@@ -142,9 +76,13 @@ func (s SqlPreferenceStore) SaveOrUpdate(preferences ...*model.Preference) Store
 	return storeChannel
 }
 
-func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.Preference) StoreResult {
+func (s SqlPreferenceStore) save(transaction *gorp.Transaction, preference *model.Preference) StoreResult {
 	result := StoreResult{}
 
+	if result.Err = preference.IsValid(); result.Err != nil {
+		return result
+	}
+
 	params := map[string]interface{}{
 		"UserId":   preference.UserId,
 		"Category": preference.Category,
@@ -154,7 +92,7 @@ func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.
 	}
 
 	if utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_MYSQL {
-		if sqlResult, err := queryable.Exec(
+		if _, err := transaction.Exec(
 			`INSERT INTO
 				Preferences
 				(UserId, Category, Name, AltId, Value)
@@ -162,13 +100,11 @@ func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.
 				(:UserId, :Category, :Name, :AltId, :Value)
 			ON DUPLICATE KEY UPDATE
 				Value = :Value`, params); err != nil {
-			result.Err = model.NewAppError("SqlPreferenceStore.saveOrUpdate", "We encountered an error while updating preferences", err.Error())
-		} else {
-			result.Data, _ = sqlResult.RowsAffected()
+			result.Err = model.NewAppError("SqlPreferenceStore.save", "We encountered an error while updating preferences", err.Error())
 		}
 	} else if utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES {
-		// postgres has no way to upsert values until version 9.5 and trying inserting and then updating causes Transactions to abort
-		count, err := queryable.SelectInt(
+		// postgres has no way to upsert values until version 9.5 and trying inserting and then updating causes transactions to abort
+		count, err := transaction.SelectInt(
 			`SELECT
 				count(0)
 			FROM
@@ -179,30 +115,57 @@ func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.
 				AND Name = :Name
 				AND AltId = :AltId`, params)
 		if err != nil {
-			result.Err = model.NewAppError("SqlPreferenceStore.saveOrUpdate", "We encountered an error while updating preferences", err.Error())
+			result.Err = model.NewAppError("SqlPreferenceStore.save", "We encountered an error while updating preferences", err.Error())
 			return result
 		}
 
 		if count == 1 {
-			s.update(queryable, preference)
+			s.update(transaction, preference)
 		} else {
-			s.save(queryable, preference)
+			s.insert(transaction, preference)
 		}
 	} else {
-		result.Err = model.NewAppError("SqlPreferenceStore.saveOrUpdate", "We encountered an error while updating preferences",
+		result.Err = model.NewAppError("SqlPreferenceStore.save", "We encountered an error while updating preferences",
 			"Failed to update preference because of missing driver")
 	}
 
 	return result
 }
 
+func (s SqlPreferenceStore) insert(transaction *gorp.Transaction, preference *model.Preference) StoreResult {
+	result := StoreResult{}
+
+	if err := transaction.Insert(preference); err != nil {
+		if IsUniqueConstraintError(err.Error(), "UserId", "preferences_pkey") {
+			result.Err = model.NewAppError("SqlPreferenceStore.insert", "A preference with that user id, category, name, and alt id already exists",
+				"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
+		} else {
+			result.Err = model.NewAppError("SqlPreferenceStore.insert", "We couldn't save the preference",
+				"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
+		}
+	}
+
+	return result
+}
+
+func (s SqlPreferenceStore) update(transaction *gorp.Transaction, preference *model.Preference) StoreResult {
+	result := StoreResult{}
+
+	if _, err := transaction.Update(preference); err != nil {
+		result.Err = model.NewAppError("SqlPreferenceStore.update", "We couldn't update the preference",
+			"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
+	}
+
+	return result
+}
+
 func (s SqlPreferenceStore) GetByName(userId string, category string, name string) StoreChannel {
 	storeChannel := make(StoreChannel)
 
 	go func() {
 		result := StoreResult{}
 
-		var preferences []*model.Preference
+		var preferences model.Preferences
 
 		if _, err := s.GetReplica().Select(&preferences,
 			`SELECT
diff --git a/store/sql_preference_store_test.go b/store/sql_preference_store_test.go
index 0574ee954..6d5ec0ecd 100644
--- a/store/sql_preference_store_test.go
+++ b/store/sql_preference_store_test.go
@@ -8,202 +8,104 @@ import (
 	"testing"
 )
 
-func TestPreferenceStoreSave(t *testing.T) {
-	Setup()
-
-	p1 := model.Preference{
-		UserId:   model.NewId(),
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		AltId:    model.NewId(),
-	}
-
-	if err := (<-store.Preference().Save(&p1)).Err; err != nil {
-		t.Fatal("couldn't save preference", err)
-	}
-
-	if err := (<-store.Preference().Save(&p1)).Err; err == nil {
-		t.Fatal("shouldn't be able to save duplicate preference")
-	}
-
-	p2 := model.Preference{
-		UserId:   model.NewId(),
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		AltId:    p1.AltId,
-	}
-
-	if err := (<-store.Preference().Save(&p2)).Err; err != nil {
-		t.Fatal("couldn't save preference with duplicate category, name, alternate id", err)
-	}
-
-	p3 := model.Preference{
-		UserId:   p1.UserId,
-		Category: model.PREFERENCE_CATEGORY_TEST,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		AltId:    p1.AltId,
-	}
-
-	if err := (<-store.Preference().Save(&p3)).Err; err != nil {
-		t.Fatal("couldn't save preference with duplicate user id, name, alternate id", err)
-	}
-
-	p4 := model.Preference{
-		UserId:   p1.UserId,
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_TEST,
-		AltId:    p1.AltId,
-	}
-
-	if err := (<-store.Preference().Save(&p4)).Err; err != nil {
-		t.Fatal("couldn't save preference with duplicate user id, category, alternate id", err)
-	}
-
-	p5 := model.Preference{
-		UserId:   p1.UserId,
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		AltId:    model.NewId(),
-	}
-
-	if err := (<-store.Preference().Save(&p5)).Err; err != nil {
-		t.Fatal("couldn't save preference with duplicate user id, category, name", err)
-	}
-}
-
-func TestPreferenceStoreUpdate(t *testing.T) {
+func TestPreferenceSave(t *testing.T) {
 	Setup()
 
 	id := model.NewId()
 
-	p1 := model.Preference{
-		UserId:   id,
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-	}
-	Must(store.Preference().Save(&p1))
-
-	p1.Value = "1234garbage"
-	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(result.Err)
-	} else if result.Data.(int64) != 1 {
-		t.Fatal("update should have changed only 1 row")
-	}
-
-	p1.UserId = model.NewId()
-	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(result.Err)
-	} else if result.Data.(int64) != 0 {
-		t.Fatal("update shouldn't have made changes because of changed user id")
-	}
-
-	p1.UserId = id
-	p1.Category = model.PREFERENCE_CATEGORY_TEST
-	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(result.Err)
-	} else if result.Data.(int64) != 0 {
-		t.Fatal("update shouldn't have made changes because of changed category")
-	}
-
-	p1.Category = model.PREFERENCE_CATEGORY_DIRECT_CHANNELS
-	p1.Name = model.PREFERENCE_NAME_TEST
-	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(result.Err)
-	} else if result.Data.(int64) != 0 {
-		t.Fatal("update shouldn't have made changes because of changed name")
-	}
-
-	p1.Name = model.PREFERENCE_NAME_SHOW
-	p1.AltId = model.NewId()
-	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(result.Err)
-	} else if result.Data.(int64) != 0 {
-		t.Fatal("update shouldn't have made changes because of changed alt id")
-	}
-}
-
-func TestPreferenceSaveOrUpdate(t *testing.T) {
-	Setup()
-
-	id := model.NewId()
-
-	p1 := model.Preference{
-		UserId:   id,
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		Value:    "value1",
-	}
-	Must(store.Preference().SaveOrUpdate(&p1))
-
-	if preferences := Must(store.Preference().GetByName(p1.UserId, p1.Category, p1.Name)).([]*model.Preference); len(preferences) != 1 {
-		t.Fatal("got incorrect number of preferences after SaveOrUpdate")
-	} else if preferences[0].Value != "value1" {
-		t.Fatal("should have received value1 after SaveOrUpdate")
-	}
-
-	p1.Value = "value2"
-	Must(store.Preference().SaveOrUpdate(&p1))
-
-	if preferences := Must(store.Preference().GetByName(p1.UserId, p1.Category, p1.Name)).([]*model.Preference); len(preferences) != 1 {
-		t.Fatal("got incorrect number of preferences after second SaveOrUpdate")
-	} else if preferences[0].Value != "value2" {
-		t.Fatal("should have received value2 after SaveOrUpdate")
+	preferences := model.Preferences{
+		{
+			UserId:   id,
+			Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
+			Name:     model.PREFERENCE_NAME_SHOW,
+			Value:    "value1a",
+		},
+		{
+			UserId:   id,
+			Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
+			Name:     model.PREFERENCE_NAME_TEST,
+			Value:    "value1b",
+		},
+	}
+	if count := Must(store.Preference().Save(&preferences)); count != 2 {
+		t.Fatal("got incorrect number of rows saved")
+	}
+
+	for _, preference := range preferences {
+		if data := Must(store.Preference().GetByName(preference.UserId, preference.Category, preference.Name)).(model.Preferences); len(data) != 1 {
+			t.Fatal("got incorrect number of preferences after first Save")
+		} else if *preference != *data[0] {
+			t.Fatal("got incorrect preference after first Save")
+		}
+	}
+
+	preferences[0].Value = "value2a"
+	preferences[1].Value = "value2b"
+	if count := Must(store.Preference().Save(&preferences)); count != 2 {
+		t.Fatal("got incorrect number of rows saved")
+	}
+
+	for _, preference := range preferences {
+		if data := Must(store.Preference().GetByName(preference.UserId, preference.Category, preference.Name)).(model.Preferences); len(data) != 1 {
+			t.Fatal("got incorrect number of preferences after second Save")
+		} else if *preference != *data[0] {
+			t.Fatal("got incorrect preference after second Save")
+		}
 	}
 }
 
 func TestPreferenceGetByName(t *testing.T) {
 	Setup()
 
-	p1 := model.Preference{
-		UserId:   model.NewId(),
-		Category: model.PREFERENCE_CATEGORY_DIRECT_CHANNELS,
-		Name:     model.PREFERENCE_NAME_SHOW,
-		AltId:    model.NewId(),
-	}
-
-	// same user/category/name, different alt id
-	p2 := model.Preference{
-		UserId:   p1.UserId,
-		Category: p1.Category,
-		Name:     p1.Name,
-		AltId:    model.NewId(),
-	}
-
-	// same user/category/alt id, different name
-	p3 := model.Preference{
-		UserId:   p1.UserId,
-		Category: p1.Category,
-		Name:     model.PREFERENCE_NAME_TEST,
-		AltId:    p1.AltId,
-	}
-
-	// same user/name/alt id, different category
-	p4 := model.Preference{
-		UserId:   p1.UserId,
-		Category: model.PREFERENCE_CATEGORY_TEST,
-		Name:     p1.Name,
-		AltId:    p1.AltId,
-	}
-
-	// same name/category/alt id, different user
-	p5 := model.Preference{
-		UserId:   model.NewId(),
-		Category: p1.Category,
-		Name:     p1.Name,
-		AltId:    p1.AltId,
-	}
-
-	Must(store.Preference().Save(&p1))
-	Must(store.Preference().Save(&p2))
-	Must(store.Preference().Save(&p3))
-	Must(store.Preference().Save(&p4))
-	Must(store.Preference().Save(&p5))
-
-	if result := <-store.Preference().GetByName(p1.UserId, p1.Category, p1.Name); result.Err != nil {
+	userId := model.NewId()
+	category := model.PREFERENCE_CATEGORY_DIRECT_CHANNELS
+	name := model.PREFERENCE_NAME_SHOW
+	altId := model.NewId()
+
+	preferences := model.Preferences{
+		{
+			UserId:   userId,
+			Category: category,
+			Name:     name,
+			AltId:    altId,
+		},
+		// same user/category/name, different alt id
+		{
+			UserId:   userId,
+			Category: category,
+			Name:     name,
+			AltId:    model.NewId(),
+		},
+		// same user/category/alt id, different name
+		{
+			UserId:   userId,
+			Category: category,
+			Name:     model.PREFERENCE_NAME_TEST,
+			AltId:    altId,
+		},
+		// same user/name/alt id, different category
+		{
+			UserId:   userId,
+			Category: model.PREFERENCE_CATEGORY_TEST,
+			Name:     name,
+			AltId:    altId,
+		},
+		// same name/category/alt id, different user
+		{
+			UserId:   model.NewId(),
+			Category: category,
+			Name:     name,
+			AltId:    altId,
+		},
+	}
+
+	Must(store.Preference().Save(&preferences))
+
+	if result := <-store.Preference().GetByName(userId, category, name); result.Err != nil {
 		t.Fatal(result.Err)
-	} else if data := result.Data.([]*model.Preference); len(data) != 2 {
+	} else if data := result.Data.(model.Preferences); len(data) != 2 {
 		t.Fatal("got the wrong number of preferences")
-	} else if !((*data[0] == p1 && *data[1] == p2) || (*data[0] == p2 && *data[1] == p1)) {
+	} else if !((*data[0] == *preferences[0] && *data[1] == *preferences[1]) || (*data[0] == *preferences[1] && *data[1] == *preferences[0])) {
 		t.Fatal("got incorrect preferences")
 	}
 }
diff --git a/store/sql_store.go b/store/sql_store.go
index c31515a44..4b055e455 100644
--- a/store/sql_store.go
+++ b/store/sql_store.go
@@ -612,11 +612,3 @@ func decrypt(key []byte, cryptoText string) (string, error) {
 
 	return fmt.Sprintf("%s", ciphertext), nil
 }
-
-// Interface for both gorp.DbMap and gorp.Transaction to allow code for one to be reused with the other
-type Queryable interface {
-	Exec(query string, args ...interface{}) (dbsql.Result, error)
-	Insert(list ...interface{}) error
-	SelectInt(query string, args ...interface{}) (int64, error)
-	Update(list ...interface{}) (int64, error)
-}
diff --git a/store/store.go b/store/store.go
index df55bc6d8..9d9fd6489 100644
--- a/store/store.go
+++ b/store/store.go
@@ -152,8 +152,6 @@ type WebhookStore interface {
 }
 
 type PreferenceStore interface {
-	Save(preference *model.Preference) StoreChannel
-	Update(preference *model.Preference) StoreChannel
-	SaveOrUpdate(preferences ...*model.Preference) StoreChannel
+	Save(preferences *model.Preferences) StoreChannel
 	GetByName(userId string, category string, name string) StoreChannel
 }