From 5cecf1afcd1d8e561a5b2d840e5bd1b588a74a27 Mon Sep 17 00:00:00 2001 From: hmhealey Date: Fri, 9 Oct 2015 13:47:11 -0400 Subject: Made structural changes to server-side Preference code based on feedback --- store/sql_preference_store.go | 161 +++++++++------------- store/sql_preference_store_test.go | 270 ++++++++++++------------------------- store/sql_store.go | 8 -- store/store.go | 4 +- 4 files changed, 149 insertions(+), 294 deletions(-) (limited to 'store') 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 } -- cgit v1.2.3-1-g7c22