Moved saving multiple user preferences into a database transaction

api/preference.go

@@ -20,7 +20,7 @@ func InitPreference(r *mux.Router) {
 }
 
 func setPreferences(c *Context, w http.ResponseWriter, r *http.Request) {
-	var preferences []model.Preference
+	var preferences []*model.Preference
 
 	decoder := json.NewDecoder(r.Body)
 	if err := decoder.Decode(&preferences); err != nil {
@@ -29,21 +29,17 @@ func setPreferences(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// just attempt to save/update them one by one and abort if one fails
-	// in the future, this could probably be done in a transaction, but that's unnecessary now
 	for _, preference := range preferences {
 		if c.Session.UserId != preference.UserId {
 			c.Err = model.NewAppError("setPreferences", "Unable to set preferences for other user", "session.user_id="+c.Session.UserId+", preference.user_id="+preference.UserId)
 			c.Err.StatusCode = http.StatusUnauthorized
 			return
 		}
+	}
 
-		if result := <-Srv.Store.Preference().Save(&preference); result.Err != nil {
+	if result := <-Srv.Store.Preference().SaveOrUpdate(preferences...); result.Err != nil {
-			if result = <-Srv.Store.Preference().Update(&preference); result.Err != nil {
+		c.Err = result.Err
-				c.Err = result.Err
+		return
-				return
-			}
-		}
 	}
 
 	w.Write([]byte("true"))

store/sql_preference_store.go

@@ -38,25 +38,98 @@ func (s SqlPreferenceStore) CreateIndexesIfNotExists() {
 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 {
+	storeChannel := make(StoreChannel)
+
 	go func() {
 		result := StoreResult{}
 
-		if result.Err = preference.IsValid(); result.Err != nil {
+		db := s.GetReplica()
-			storeChannel <- result
-			close(storeChannel)
-			return
-		}
 
-		if err := s.GetMaster().Insert(preference); err != nil {
+		if len(preferences) > 1 {
-			if IsUniqueConstraintError(err.Error(), "UserId", "preferences_pkey") {
+			// wrap in a transaction so that if one fails, everything fails
-				result.Err = model.NewAppError("SqlPreferenceStore.Save", "A preference with that user id, category, name, and alt id already exists",
+			transaction, err := db.Begin()
-					"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
+			if err != nil {
+				result.Err = model.NewAppError("SqlPreferenceStore.SaveOrUpdateMultiple", "Unable to open transaction to update preferences", err.Error())
 			} else {
-				result.Err = model.NewAppError("SqlPreferenceStore.Save", "We couldn't save the preference",
+				for _, preference := range preferences {
-					"user_id="+preference.UserId+", category="+preference.Category+", name="+preference.Name+", alt_id="+preference.AltId+", "+err.Error())
+					if err := s.saveOrUpdate(transaction, preference); err != nil {
+						result.Err = err
+						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 {
+					if err := transaction.Rollback(); err != nil {
+						result.Err = model.NewAppError("SqlPreferenceStore.SaveOrUpdateMultiple", "Unable to rollback transaction to update preferences", err.Error())
+					}
+				}
 			}
 		} else {
-			result.Data = preference
+			result.Err = s.saveOrUpdate(db, preferences[0])
 		}
 
 		storeChannel <- result
@@ -66,33 +139,14 @@ func (s SqlPreferenceStore) Save(preference *model.Preference) StoreChannel {
 	return storeChannel
 }
 
-func (s SqlPreferenceStore) Update(preference *model.Preference) StoreChannel {
+func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.Preference) *model.AppError {
-	storeChannel := make(StoreChannel)
+	if result := s.save(queryable, preference); result.Err != nil {
-
+		if result := s.update(queryable, preference); result.Err != nil {
-	go func() {
+			return result.Err
-		result := StoreResult{}
-
-		if result.Err = preference.IsValid(); result.Err != nil {
-			storeChannel <- result
-			close(storeChannel)
-			return
 		}
+	}
 
-		if count, err := s.GetMaster().Update(preference); err != nil {
+	return 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 if count != 1 {
-			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)
-		} else {
-			result.Data = preference
-		}
-
-		storeChannel <- result
-		close(storeChannel)
-	}()
-
-	return storeChannel
 }
 
 func (s SqlPreferenceStore) GetByName(userId string, category string, name string) StoreChannel {

store/sql_preference_store_test.go

@@ -84,31 +84,70 @@ func TestPreferenceStoreUpdate(t *testing.T) {
 	Must(store.Preference().Save(&p1))
 
 	p1.Value = "1234garbage"
-	if err := (<-store.Preference().Update(&p1)).Err; err != nil {
+	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal(err)
+		t.Fatal(result.Err)
+	} else if result.Data.(int64) != 1 {
+		t.Fatal("update should have changed only 1 row")
 	}
 
 	p1.UserId = model.NewId()
-	if err := (<-store.Preference().Update(&p1)).Err; err == nil {
+	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal("update should have failed because of changed user id")
+		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 err := (<-store.Preference().Update(&p1)).Err; err == nil {
+	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal("update should have failed because of changed category")
+		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 err := (<-store.Preference().Update(&p1)).Err; err == nil {
+	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal("update should have failed because of changed name")
+		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 err := (<-store.Preference().Update(&p1)).Err; err == nil {
+	if result := (<-store.Preference().Update(&p1)); result.Err != nil {
-		t.Fatal("update should have failed because of changed alternate id")
+		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")
 	}
 }
 

store/sql_store.go

@@ -612,3 +612,9 @@ 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 {
+	Insert(list ...interface{}) error
+	Update(list ...interface{}) (int64, error)
+}

store/store.go

@@ -154,5 +154,6 @@ type WebhookStore interface {
 type PreferenceStore interface {
 	Save(preference *model.Preference) StoreChannel
 	Update(preference *model.Preference) StoreChannel
+	SaveOrUpdate(preferences ...*model.Preference) StoreChannel
 	GetByName(userId string, category string, name string) StoreChannel
 }