From ce61681c92fd0c6856201f0278cf29ec04385c10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Tue, 9 Jul 2019 09:08:28 -0400 Subject: [PATCH] Migrate Plugin.SaveOrUpdate to sync by default (#11592) --- app/plugin_key_value_store.go | 6 ++-- app/plugin_test.go | 4 +-- store/sqlstore/plugin_store.go | 51 +++++++++++++--------------- store/store.go | 2 +- store/storetest/mocks/PluginStore.go | 19 ++++++++--- store/storetest/plugin_store.go | 43 +++++++++++++---------- 6 files changed, 68 insertions(+), 57 deletions(-) diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index 3515a6a1a6..91d946d679 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -34,9 +34,9 @@ func (a *App) SetPluginKeyWithExpiry(pluginId string, key string, value []byte, ExpireAt: expireInSeconds, } - if result := <-a.Srv.Store.Plugin().SaveOrUpdate(kv); result.Err != nil { - mlog.Error("Failed to set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err)) - return result.Err + if _, err := a.Srv.Store.Plugin().SaveOrUpdate(kv); err != nil { + mlog.Error("Failed to set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + return err } // Clean up a previous entry using the hashed key, if it exists. diff --git a/app/plugin_test.go b/app/plugin_test.go index 831a778b8c..621cc2744d 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -66,8 +66,8 @@ func TestPluginKeyValueStore(t *testing.T) { ExpireAt: 0, } - result := <-th.App.Srv.Store.Plugin().SaveOrUpdate(kv) - assert.Nil(t, result.Err) + _, err = th.App.Srv.Store.Plugin().SaveOrUpdate(kv) + assert.Nil(t, err) // Test fetch by keyname (this key does not exist but hashed key will be used for lookup) ret, err = th.App.GetPluginKey(pluginId, "key2") diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index b1dc932542..1686668da8 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -36,39 +36,34 @@ func NewSqlPluginStore(sqlStore SqlStore) store.PluginStore { func (ps SqlPluginStore) CreateIndexesIfNotExists() { } -func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if result.Err = kv.IsValid(); result.Err != nil { - return - } +func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { + if err := kv.IsValid(); err != nil { + return nil, err + } - if ps.DriverName() == model.DATABASE_DRIVER_POSTGRES { - // Unfortunately PostgreSQL pre-9.5 does not have an atomic upsert, so we use - // separate update and insert queries to accomplish our upsert - if rowsAffected, err := ps.GetMaster().Update(kv); err != nil { - result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } else if rowsAffected == 0 { - // No rows were affected by the update, so let's try an insert - if err := ps.GetMaster().Insert(kv); err != nil { - // If the error is from unique constraints violation, it's the result of a - // valid race and we can report success. Otherwise we have a real error and - // need to return it - if !IsUniqueConstraintError(err, []string{"PRIMARY", "PluginId", "Key", "PKey"}) { - result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } + if ps.DriverName() == model.DATABASE_DRIVER_POSTGRES { + // Unfortunately PostgreSQL pre-9.5 does not have an atomic upsert, so we use + // separate update and insert queries to accomplish our upsert + if rowsAffected, err := ps.GetMaster().Update(kv); err != nil { + return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } else if rowsAffected == 0 { + // No rows were affected by the update, so let's try an insert + if err := ps.GetMaster().Insert(kv); err != nil { + // If the error is from unique constraints violation, it's the result of a + // valid race and we can report success. Otherwise we have a real error and + // need to return it + if !IsUniqueConstraintError(err, []string{"PRIMARY", "PluginId", "Key", "PKey"}) { + return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) } } - } else if ps.DriverName() == model.DATABASE_DRIVER_MYSQL { - if _, err := ps.GetMaster().Exec("INSERT INTO PluginKeyValueStore (PluginId, PKey, PValue, ExpireAt) VALUES(:PluginId, :Key, :Value, :ExpireAt) ON DUPLICATE KEY UPDATE PValue = :Value, ExpireAt = :ExpireAt", map[string]interface{}{"PluginId": kv.PluginId, "Key": kv.Key, "Value": kv.Value, "ExpireAt": kv.ExpireAt}); err != nil { - result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) - return - } } + } else if ps.DriverName() == model.DATABASE_DRIVER_MYSQL { + if _, err := ps.GetMaster().Exec("INSERT INTO PluginKeyValueStore (PluginId, PKey, PValue, ExpireAt) VALUES(:PluginId, :Key, :Value, :ExpireAt) ON DUPLICATE KEY UPDATE PValue = :Value, ExpireAt = :ExpireAt", map[string]interface{}{"PluginId": kv.PluginId, "Key": kv.Key, "Value": kv.Value, "ExpireAt": kv.ExpireAt}); err != nil { + return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } + } - result.Data = kv - }) + return kv, nil } func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { diff --git a/store/store.go b/store/store.go index f56b5d422d..68d1768f56 100644 --- a/store/store.go +++ b/store/store.go @@ -535,7 +535,7 @@ type UserAccessTokenStore interface { } type PluginStore interface { - SaveOrUpdate(keyVal *model.PluginKeyValue) StoreChannel + SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) Delete(pluginId, key string) StoreChannel diff --git a/store/storetest/mocks/PluginStore.go b/store/storetest/mocks/PluginStore.go index a653ed6860..753df0c9f4 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -135,17 +135,26 @@ func (_m *PluginStore) List(pluginId string, page int, perPage int) ([]string, * } // SaveOrUpdate provides a mock function with given fields: keyVal -func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) store.StoreChannel { +func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { ret := _m.Called(keyVal) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.PluginKeyValue) store.StoreChannel); ok { + var r0 *model.PluginKeyValue + if rf, ok := ret.Get(0).(func(*model.PluginKeyValue) *model.PluginKeyValue); ok { r0 = rf(keyVal) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.PluginKeyValue) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.PluginKeyValue) *model.AppError); ok { + r1 = rf(keyVal) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } diff --git a/store/storetest/plugin_store.go b/store/storetest/plugin_store.go index 263262fd1d..9e9464a32c 100644 --- a/store/storetest/plugin_store.go +++ b/store/storetest/plugin_store.go @@ -6,6 +6,8 @@ package storetest import ( "testing" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" "github.com/stretchr/testify/assert" @@ -27,8 +29,8 @@ func testPluginSaveGet(t *testing.T, ss store.Store) { ExpireAt: 0, } - if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil { + t.Fatal(err) } defer func() { @@ -46,8 +48,8 @@ func testPluginSaveGet(t *testing.T, ss store.Store) { // Try inserting when already exists kv.Value = []byte(model.NewId()) - if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil { + t.Fatal(err) } if received, err := ss.Plugin().Get(kv.PluginId, kv.Key); err != nil { @@ -67,8 +69,8 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) { ExpireAt: model.GetMillis() + 30000, } - if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil { + t.Fatal(err) } defer func() { @@ -91,8 +93,8 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) { ExpireAt: model.GetMillis() - 5000, } - if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil { + t.Fatal(err) } defer func() { @@ -105,11 +107,12 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) { } func testPluginDelete(t *testing.T, ss store.Store) { - kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ + kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ PluginId: model.NewId(), Key: model.NewId(), Value: []byte(model.NewId()), - })).(*model.PluginKeyValue) + }) + require.Nil(t, err) if result := <-ss.Plugin().Delete(kv.PluginId, kv.Key); result.Err != nil { t.Fatal(result.Err) @@ -119,17 +122,19 @@ func testPluginDelete(t *testing.T, ss store.Store) { func testPluginDeleteAll(t *testing.T, ss store.Store) { pluginId := model.NewId() - kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ + kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ PluginId: pluginId, Key: model.NewId(), Value: []byte(model.NewId()), - })).(*model.PluginKeyValue) + }) + require.Nil(t, err) - kv2 := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ + kv2, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ PluginId: pluginId, Key: model.NewId(), Value: []byte(model.NewId()), - })).(*model.PluginKeyValue) + }) + require.Nil(t, err) if result := <-ss.Plugin().DeleteAllForPlugin(pluginId); result.Err != nil { t.Fatal(result.Err) @@ -147,19 +152,21 @@ func testPluginDeleteAll(t *testing.T, ss store.Store) { func testPluginDeleteExpired(t *testing.T, ss store.Store) { pluginId := model.NewId() - kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ + kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ PluginId: pluginId, Key: model.NewId(), Value: []byte(model.NewId()), ExpireAt: model.GetMillis() - 6000, - })).(*model.PluginKeyValue) + }) + require.Nil(t, err) - kv2 := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ + kv2, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{ PluginId: pluginId, Key: model.NewId(), Value: []byte(model.NewId()), ExpireAt: 0, - })).(*model.PluginKeyValue) + }) + require.Nil(t, err) if result := <-ss.Plugin().DeleteAllExpired(); result.Err != nil { t.Fatal(result.Err)