From 17b49e453852768bde41bdedb84e4c218c24ea06 Mon Sep 17 00:00:00 2001 From: Phillip Ahereza Date: Mon, 8 Jul 2019 14:13:10 +0300 Subject: [PATCH] [MM-16790] Migrate Plugin.Get to Sync by default (#11581) * Migrate Plugin.Get to Sync by default * remove unnecessary else branch * remove unnecesary blank line --- app/plugin_key_value_store.go | 20 ++++++++++---------- store/sqlstore/plugin_store.go | 23 ++++++++++------------- store/store.go | 2 +- store/storetest/mocks/PluginStore.go | 19 ++++++++++++++----- store/storetest/plugin_store.go | 28 ++++++++++++---------------- 5 files changed, 47 insertions(+), 45 deletions(-) diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index 559b462f04..782a44c1d1 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -69,19 +69,19 @@ func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newV } func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError) { - if result := <-a.Srv.Store.Plugin().Get(pluginId, key); result.Err == nil { - return result.Data.(*model.PluginKeyValue).Value, nil - } else if result.Err.StatusCode != http.StatusNotFound { - mlog.Error("Failed to query plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err)) - return nil, result.Err + if kv, err := a.Srv.Store.Plugin().Get(pluginId, key); err == nil { + return kv.Value, nil + } else if err.StatusCode != http.StatusNotFound { + mlog.Error("Failed to query plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + return nil, err } // Lookup using the hashed version of the key for keys written prior to v5.6. - if result := <-a.Srv.Store.Plugin().Get(pluginId, getKeyHash(key)); result.Err == nil { - return result.Data.(*model.PluginKeyValue).Value, nil - } else if result.Err.StatusCode != http.StatusNotFound { - mlog.Error("Failed to query plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err)) - return nil, result.Err + if kv, err := a.Srv.Store.Plugin().Get(pluginId, getKeyHash(key)); err == nil { + return kv.Value, nil + } else if err.StatusCode != http.StatusNotFound { + mlog.Error("Failed to query plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + return nil, err } return nil, nil diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index 818a77c30c..5aa6bab95b 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -116,20 +116,17 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte return true, nil } -func (ps SqlPluginStore) Get(pluginId, key string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var kv *model.PluginKeyValue - currentTime := model.GetMillis() - if err := ps.GetReplica().SelectOne(&kv, "SELECT * FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND (ExpireAt = 0 OR ExpireAt > :CurrentTime)", map[string]interface{}{"PluginId": pluginId, "Key": key, "CurrentTime": currentTime}); err != nil { - if err == sql.ErrNoRows { - result.Err = model.NewAppError("SqlPluginStore.Get", "store.sql_plugin_store.get.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusNotFound) - } else { - result.Err = model.NewAppError("SqlPluginStore.Get", "store.sql_plugin_store.get.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusInternalServerError) - } - } else { - result.Data = kv +func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) { + var kv *model.PluginKeyValue + currentTime := model.GetMillis() + if err := ps.GetReplica().SelectOne(&kv, "SELECT * FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND (ExpireAt = 0 OR ExpireAt > :CurrentTime)", map[string]interface{}{"PluginId": pluginId, "Key": key, "CurrentTime": currentTime}); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlPluginStore.Get", "store.sql_plugin_store.get.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusNotFound) } - }) + return nil, model.NewAppError("SqlPluginStore.Get", "store.sql_plugin_store.get.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusInternalServerError) + } + + return kv, nil } func (ps SqlPluginStore) Delete(pluginId, key string) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 93f1bd6c53..c572e008a1 100644 --- a/store/store.go +++ b/store/store.go @@ -537,7 +537,7 @@ type UserAccessTokenStore interface { type PluginStore interface { SaveOrUpdate(keyVal *model.PluginKeyValue) StoreChannel CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) - Get(pluginId, key string) StoreChannel + Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) Delete(pluginId, key string) StoreChannel DeleteAllForPlugin(PluginId string) StoreChannel DeleteAllExpired() StoreChannel diff --git a/store/storetest/mocks/PluginStore.go b/store/storetest/mocks/PluginStore.go index 71ff9d688a..111375052b 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -85,19 +85,28 @@ func (_m *PluginStore) DeleteAllForPlugin(PluginId string) store.StoreChannel { } // Get provides a mock function with given fields: pluginId, key -func (_m *PluginStore) Get(pluginId string, key string) store.StoreChannel { +func (_m *PluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, *model.AppError) { ret := _m.Called(pluginId, key) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string) store.StoreChannel); ok { + var r0 *model.PluginKeyValue + if rf, ok := ret.Get(0).(func(string, string) *model.PluginKeyValue); ok { r0 = rf(pluginId, key) } 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(string, string) *model.AppError); ok { + r1 = rf(pluginId, key) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // List provides a mock function with given fields: pluginId, page, perPage diff --git a/store/storetest/plugin_store.go b/store/storetest/plugin_store.go index 0e14c75390..263262fd1d 100644 --- a/store/storetest/plugin_store.go +++ b/store/storetest/plugin_store.go @@ -35,10 +35,9 @@ func testPluginSaveGet(t *testing.T, ss store.Store) { <-ss.Plugin().Delete(kv.PluginId, kv.Key) }() - if result := <-ss.Plugin().Get(kv.PluginId, kv.Key); result.Err != nil { - t.Fatal(result.Err) + if received, err := ss.Plugin().Get(kv.PluginId, kv.Key); err != nil { + t.Fatal(err) } else { - received := result.Data.(*model.PluginKeyValue) assert.Equal(t, kv.PluginId, received.PluginId) assert.Equal(t, kv.Key, received.Key) assert.Equal(t, kv.Value, received.Value) @@ -51,10 +50,9 @@ func testPluginSaveGet(t *testing.T, ss store.Store) { t.Fatal(result.Err) } - if result := <-ss.Plugin().Get(kv.PluginId, kv.Key); result.Err != nil { - t.Fatal(result.Err) + if received, err := ss.Plugin().Get(kv.PluginId, kv.Key); err != nil { + t.Fatal(err) } else { - received := result.Data.(*model.PluginKeyValue) assert.Equal(t, kv.PluginId, received.PluginId) assert.Equal(t, kv.Key, received.Key) assert.Equal(t, kv.Value, received.Value) @@ -77,10 +75,9 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) { <-ss.Plugin().Delete(kv.PluginId, kv.Key) }() - if result := <-ss.Plugin().Get(kv.PluginId, kv.Key); result.Err != nil { - t.Fatal(result.Err) + if received, err := ss.Plugin().Get(kv.PluginId, kv.Key); err != nil { + t.Fatal(err) } else { - received := result.Data.(*model.PluginKeyValue) assert.Equal(t, kv.PluginId, received.PluginId) assert.Equal(t, kv.Key, received.Key) assert.Equal(t, kv.Value, received.Value) @@ -102,7 +99,7 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) { <-ss.Plugin().Delete(kv.PluginId, kv.Key) }() - if result := <-ss.Plugin().Get(kv.PluginId, kv.Key); result.Err == nil { + if _, err := ss.Plugin().Get(kv.PluginId, kv.Key); err == nil { t.Fatal("result.Err should not be nil") } } @@ -138,11 +135,11 @@ func testPluginDeleteAll(t *testing.T, ss store.Store) { t.Fatal(result.Err) } - if result := <-ss.Plugin().Get(pluginId, kv.Key); result.Err == nil { + if _, err := ss.Plugin().Get(pluginId, kv.Key); err == nil { t.Fatal("result.Err should not be nil") } - if result := <-ss.Plugin().Get(pluginId, kv2.Key); result.Err == nil { + if _, err := ss.Plugin().Get(pluginId, kv2.Key); err == nil { t.Fatal("result.Err should not be nil") } } @@ -168,14 +165,13 @@ func testPluginDeleteExpired(t *testing.T, ss store.Store) { t.Fatal(result.Err) } - if result := <-ss.Plugin().Get(pluginId, kv.Key); result.Err == nil { + if _, err := ss.Plugin().Get(pluginId, kv.Key); err == nil { t.Fatal("result.Err should not be nil") } - if result := <-ss.Plugin().Get(kv2.PluginId, kv2.Key); result.Err != nil { - t.Fatal(result.Err) + if received, err := ss.Plugin().Get(kv2.PluginId, kv2.Key); err != nil { + t.Fatal(err) } else { - received := result.Data.(*model.PluginKeyValue) assert.Equal(t, kv2.PluginId, received.PluginId) assert.Equal(t, kv2.Key, received.Key) assert.Equal(t, kv2.Value, received.Value)