From a6cd2a79612d6d96e0e929ab769ec5e70cd35f5f Mon Sep 17 00:00:00 2001 From: hmhealey Date: Wed, 7 Oct 2015 10:19:02 -0400 Subject: Moved saving multiple user preferences into a database transaction --- api/preference.go | 14 ++--- store/sql_preference_store.go | 112 +++++++++++++++++++++++++++---------- store/sql_preference_store_test.go | 59 +++++++++++++++---- store/sql_store.go | 6 ++ store/store.go | 1 + 5 files changed, 144 insertions(+), 48 deletions(-) diff --git a/api/preference.go b/api/preference.go index 84cfc130c..810917a7c 100644 --- a/api/preference.go +++ b/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().Update(&preference); result.Err != nil { - c.Err = result.Err - return - } - } + if result := <-Srv.Store.Preference().SaveOrUpdate(preferences...); result.Err != nil { + c.Err = result.Err + return } w.Write([]byte("true")) diff --git a/store/sql_preference_store.go b/store/sql_preference_store.go index 3e3a91f61..366a63fa6 100644 --- a/store/sql_preference_store.go +++ b/store/sql_preference_store.go @@ -39,53 +39,97 @@ func (s SqlPreferenceStore) Save(preference *model.Preference) StoreChannel { storeChannel := make(StoreChannel) go func() { - result := StoreResult{} + storeChannel <- s.save(s.GetMaster(), preference) + close(storeChannel) + }() - if result.Err = preference.IsValid(); result.Err != nil { - storeChannel <- result - close(storeChannel) - return - } + return storeChannel +} - if err := s.GetMaster().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()) - } +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.Data = preference + 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 + } - storeChannel <- result + 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(preference *model.Preference) 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 { - storeChannel <- result - close(storeChannel) - return - } + db := s.GetReplica() - if count, err := s.GetMaster().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 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) + 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 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 @@ -95,6 +139,16 @@ func (s SqlPreferenceStore) Update(preference *model.Preference) StoreChannel { return storeChannel } +func (s SqlPreferenceStore) saveOrUpdate(queryable Queryable, preference *model.Preference) *model.AppError { + if result := s.save(queryable, preference); result.Err != nil { + if result := s.update(queryable, preference); result.Err != nil { + return result.Err + } + } + + return nil +} + func (s SqlPreferenceStore) GetByName(userId string, category string, name string) StoreChannel { storeChannel := make(StoreChannel) diff --git a/store/sql_preference_store_test.go b/store/sql_preference_store_test.go index 99b183274..0574ee954 100644 --- a/store/sql_preference_store_test.go +++ b/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 { - t.Fatal(err) + 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 err := (<-store.Preference().Update(&p1)).Err; err == nil { - t.Fatal("update should have failed because of changed user id") + 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 err := (<-store.Preference().Update(&p1)).Err; err == nil { - t.Fatal("update should have failed because of changed category") + 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 err := (<-store.Preference().Update(&p1)).Err; err == nil { - t.Fatal("update should have failed because of changed name") + 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 err := (<-store.Preference().Update(&p1)).Err; err == nil { - t.Fatal("update should have failed because of changed alternate id") + 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") } } diff --git a/store/sql_store.go b/store/sql_store.go index 4b055e455..f8c585979 100644 --- a/store/sql_store.go +++ b/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) +} diff --git a/store/store.go b/store/store.go index 8a8faae85..df55bc6d8 100644 --- a/store/store.go +++ b/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 } -- cgit v1.2.3-1-g7c22