From d0d6ce0a7014631c94cdcb3c01da233151799e0f Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 3 Dec 2019 10:46:15 +0100 Subject: [PATCH] [MM-20353] Change KVSetWithOptions to accept a byte slice (#13213) * Drop EncodeJSON from PluginKVSetOptions * Add unit tests for KVSetWithOptions with nil values --- app/plugin_api.go | 2 +- app/plugin_key_value_store.go | 2 +- app/plugin_test.go | 172 ++++++++++++--------------- i18n/en.json | 4 - model/plugin_kvset_options.go | 40 +------ plugin/api.go | 5 +- plugin/client_rpc_generated.go | 8 +- plugin/plugintest/api.go | 14 +-- store/sqlstore/plugin_store.go | 10 +- store/store.go | 2 +- store/storetest/mocks/PluginStore.go | 6 +- 11 files changed, 104 insertions(+), 161 deletions(-) diff --git a/app/plugin_api.go b/app/plugin_api.go index 0e6d8c3374..4850572fc8 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -697,7 +697,7 @@ func (api *PluginAPI) InstallPlugin(file io.Reader, replace bool) (*model.Manife // KV Store Section -func (api *PluginAPI) KVSetWithOptions(key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (api *PluginAPI) KVSetWithOptions(key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { return api.app.SetPluginKeyWithOptions(api.id, key, value, options) } diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index 77cdcbbd00..4bff3f59be 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -38,7 +38,7 @@ func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newV return a.SetPluginKeyWithOptions(pluginId, key, newValue, options) } -func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { if err := options.IsValid(); err != nil { mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) return false, err diff --git a/app/plugin_test.go b/app/plugin_test.go index 1f4a42bf87..60ec5b5948 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -14,6 +14,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" @@ -197,60 +198,24 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) { assert.Nil(t, th.App.DeletePluginKey(pluginId, "key")) }() - t.Run("fails with a non-serializable object as the new value", func(t *testing.T) { - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", func() {}, model.PluginKVSetOptions{ - EncodeJSON: true, - }) - assert.False(t, result) - assert.NotNil(t, err) - - // verify that after the failure it was not set - ret, err := th.App.GetPluginKey(pluginId, "key") - assert.Nil(t, err) - assert.Equal(t, []byte(nil), ret) - }) - - t.Run("fails with a non-serializable object as the old value", func(t *testing.T) { - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ - "val-a": 10, - }, model.PluginKVSetOptions{ - EncodeJSON: true, - Atomic: true, - OldValue: func() {}, - }) - assert.False(t, result) - assert.NotNil(t, err) - - // verify that after the failure it was not set - ret, err := th.App.GetPluginKey(pluginId, "key") - assert.Nil(t, err) - assert.Equal(t, []byte(nil), ret) - }) - - t.Run("storing a value json encoded works", func(t *testing.T) { - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ - "val-a": 10, - }, model.PluginKVSetOptions{ - EncodeJSON: true, - }) + t.Run("storing a value without providing options works", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-1"), model.PluginKVSetOptions{}) assert.True(t, result) assert.Nil(t, err) // and I can get it back! ret, err := th.App.GetPluginKey(pluginId, "key") assert.Nil(t, err) - assert.Equal(t, []byte(`{"val-a":10}`), ret) + assert.Equal(t, []byte(`value-1`), ret) }) t.Run("test that setting it atomic when it doesn't match doesn't change anything", func(t *testing.T) { - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ - "val-a": 30, - }, model.PluginKVSetOptions{ - EncodeJSON: true, - Atomic: true, - OldValue: map[string]interface{}{ - "val-a": 20, - }, + err := th.App.SetPluginKey(pluginId, "key", []byte("value-1")) + require.Nil(t, err) + + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-3"), model.PluginKVSetOptions{ + Atomic: true, + OldValue: []byte("value-2"), }) assert.False(t, result) assert.Nil(t, err) @@ -258,18 +223,16 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) { // test that the value didn't change ret, err := th.App.GetPluginKey(pluginId, "key") assert.Nil(t, err) - assert.Equal(t, []byte(`{"val-a":10}`), ret) + assert.Equal(t, []byte(`value-1`), ret) }) t.Run("test the atomic change with the proper old value", func(t *testing.T) { - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ - "val-a": 30, - }, model.PluginKVSetOptions{ - EncodeJSON: true, - Atomic: true, - OldValue: map[string]interface{}{ - "val-a": 10, - }, + err := th.App.SetPluginKey(pluginId, "key", []byte("value-2")) + require.Nil(t, err) + + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-3"), model.PluginKVSetOptions{ + Atomic: true, + OldValue: []byte("value-2"), }) assert.True(t, result) assert.Nil(t, err) @@ -277,55 +240,76 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) { // test that the value did change ret, err := th.App.GetPluginKey(pluginId, "key") assert.Nil(t, err) - assert.Equal(t, []byte(`{"val-a":30}`), ret) + assert.Equal(t, []byte(`value-3`), ret) }) -} -func TestPluginKeyValueStoreSetWithOptionsByteArray(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + t.Run("when new value is nil and old value matches with the current, it should delete the currently set value", func(t *testing.T) { + // first set a value. + result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-2", []byte("value-1"), model.PluginKVSetOptions{}) + require.Nil(t, err) + require.True(t, result) - pluginId := "testpluginid" + // now it should delete the set value. + result, err = th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-2", nil, model.PluginKVSetOptions{ + Atomic: true, + OldValue: []byte("value-1"), + }) + assert.Nil(t, err) + assert.True(t, result) - defer func() { - assert.Nil(t, th.App.DeletePluginKey(pluginId, "key")) - }() - - // storing a value works - result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`myvalue`), model.PluginKVSetOptions{}) - assert.True(t, result) - assert.Nil(t, err) - - // and I can get it back! - ret, err := th.App.GetPluginKey(pluginId, "key") - assert.Nil(t, err) - assert.Equal(t, []byte(`myvalue`), ret) - - // test that setting it atomic when it doesn't match doesn't change anything - result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{ - Atomic: true, - OldValue: []byte(`differentvalue`), + ret, err := th.App.GetPluginKey(pluginId, "nil-test-key-2") + assert.Nil(t, err) + assert.Nil(t, ret) }) - assert.False(t, result) - assert.Nil(t, err) - // test that the value didn't change - ret, err = th.App.GetPluginKey(pluginId, "key") - assert.Nil(t, err) - assert.Equal(t, []byte(`myvalue`), ret) + t.Run("when new value is nil and there is a value set for the key already, it should delete the currently set value", func(t *testing.T) { + // first set a value. + result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-3", []byte("value-1"), model.PluginKVSetOptions{}) + require.Nil(t, err) + require.True(t, result) - // now do the atomic change with the proper old value - result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{ - Atomic: true, - OldValue: []byte(`myvalue`), + // now it should delete the set value. + result, err = th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-3", nil, model.PluginKVSetOptions{}) + assert.Nil(t, err) + assert.True(t, result) + + ret, err := th.App.GetPluginKey(pluginId, "nil-test-key-3") + assert.Nil(t, err) + assert.Nil(t, ret) }) - assert.True(t, result) - assert.Nil(t, err) - // test that the value did change - ret, err = th.App.GetPluginKey(pluginId, "key") - assert.Nil(t, err) - assert.Equal(t, []byte(`newvalue`), ret) + t.Run("when old value is nil and there is no value set for the key before, it should set the new value", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-4", []byte("value-1"), model.PluginKVSetOptions{ + Atomic: true, + OldValue: nil, + }) + assert.Nil(t, err) + assert.True(t, result) + + ret, err := th.App.GetPluginKey(pluginId, "nil-test-key-4") + assert.Nil(t, err) + assert.Equal(t, []byte("value-1"), ret) + }) + + t.Run("test that value is set and unset with ExpireInSeconds", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-1"), model.PluginKVSetOptions{ + ExpireInSeconds: 1, + }) + assert.True(t, result) + assert.Nil(t, err) + + // test that the value is set + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`value-1`), ret) + + // test that the value is not longer + time.Sleep(1500 * time.Millisecond) + + ret, err = th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Nil(t, ret) + }) } func TestServePluginRequest(t *testing.T) { diff --git a/i18n/en.json b/i18n/en.json index efa7fc1dc6..62694b0197 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -5254,10 +5254,6 @@ "id": "model.plugin_kvset_options.is_valid.old_value.app_error", "translation": "Invalid old value, it shouldn't be set when the operation is not atomic." }, - { - "id": "model.plugin_kvset_options.serialize_value.app_error", - "translation": "Could not deserialize a value. EncodeJSON: {{.EncodeJSON}}." - }, { "id": "model.post.is_valid.channel_id.app_error", "translation": "Invalid channel id" diff --git a/model/plugin_kvset_options.go b/model/plugin_kvset_options.go index b71555e68c..d1374d6484 100644 --- a/model/plugin_kvset_options.go +++ b/model/plugin_kvset_options.go @@ -4,16 +4,14 @@ package model import ( - "encoding/json" "net/http" ) // PluginKVSetOptions contains information on how to store a value in the plugin KV store. type PluginKVSetOptions struct { - EncodeJSON bool // If true, store the JSON encoding of newValue - Atomic bool // Only store the value if the current value matches the oldValue - OldValue interface{} // The value to compare with the current value. Only used when Atomic is true - ExpireInSeconds int64 // Set an expire counter + Atomic bool // Only store the value if the current value matches the oldValue + OldValue []byte // The value to compare with the current value. Only used when Atomic is true + ExpireInSeconds int64 // Set an expire counter } // IsValid returns nil if the chosen options are valid. @@ -31,36 +29,8 @@ func (opt *PluginKVSetOptions) IsValid() *AppError { return nil } -// GetOldValueSerialized returns the serialized old value either as directly -// or encoded as JSON depending on the chosen options. -func (opt *PluginKVSetOptions) GetOldValueSerialized() ([]byte, *AppError) { - return opt.serializeValue(opt.OldValue) -} - -func (opt *PluginKVSetOptions) serializeValue(value interface{}) ([]byte, *AppError) { - if opt.EncodeJSON { - data, err := json.Marshal(value) - if err != nil { - return nil, NewAppError("PluginKVSetOptions.serializeValue", "model.plugin_kvset_options.serialize_value.app_error", map[string]interface{}{"EncodeJSON": opt.EncodeJSON}, "Could not serialize JSON value", http.StatusBadRequest) - } - return data, nil - } - - castResult, ok := value.([]byte) - if !ok { - return nil, NewAppError("PluginKVSetOptions.SerializeValue", "model.plugin_kvset_options.serialize_value.app_error", map[string]interface{}{"EncodeJSON": opt.EncodeJSON}, "Could not cast value to []byte", http.StatusBadRequest) - } - - return castResult, nil -} - // NewPluginKeyValueFromOptions return a PluginKeyValue given a pluginID, a KV pair and options. -func NewPluginKeyValueFromOptions(pluginId, key string, value interface{}, opt PluginKVSetOptions) (*PluginKeyValue, *AppError) { - serializedValue, err := opt.serializeValue(value) - if err != nil { - return nil, err - } - +func NewPluginKeyValueFromOptions(pluginId, key string, value []byte, opt PluginKVSetOptions) (*PluginKeyValue, *AppError) { expireAt := int64(0) if opt.ExpireInSeconds > 0 { expireAt = GetMillis() + (opt.ExpireInSeconds * 1000) @@ -69,7 +39,7 @@ func NewPluginKeyValueFromOptions(pluginId, key string, value interface{}, opt P kv := &PluginKeyValue{ PluginId: pluginId, Key: key, - Value: serializedValue, + Value: value, ExpireAt: expireAt, } diff --git a/plugin/api.go b/plugin/api.go index 734e8a5d3a..30059a3562 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -758,13 +758,12 @@ type API interface { KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) // KVSetWithOptions stores a key-value pair, unique per plugin, according to the given options. - // If options.EncodeJSON is not true, the type of newValue must be of type []byte. // Returns (false, err) if DB error occurred // Returns (false, nil) if the value was not set // Returns (true, nil) if the value was set // - // Minimum server version: 5.18 - KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) + // Minimum server version: 5.20 + KVSetWithOptions(key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) // KVSet stores a key-value pair with an expiry time, unique per plugin. // diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index b87df1055a..98671b4cc1 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -3698,7 +3698,7 @@ func (s *apiRPCServer) KVCompareAndDelete(args *Z_KVCompareAndDeleteArgs, return type Z_KVSetWithOptionsArgs struct { A string - B interface{} + B []byte C model.PluginKVSetOptions } @@ -3707,8 +3707,8 @@ type Z_KVSetWithOptionsReturns struct { B *model.AppError } -func (g *apiRPCClient) KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { - _args := &Z_KVSetWithOptionsArgs{key, newValue, options} +func (g *apiRPCClient) KVSetWithOptions(key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { + _args := &Z_KVSetWithOptionsArgs{key, value, options} _returns := &Z_KVSetWithOptionsReturns{} if err := g.client.Call("Plugin.KVSetWithOptions", _args, _returns); err != nil { log.Printf("RPC call to KVSetWithOptions API failed: %s", err.Error()) @@ -3718,7 +3718,7 @@ func (g *apiRPCClient) KVSetWithOptions(key string, newValue interface{}, option func (s *apiRPCServer) KVSetWithOptions(args *Z_KVSetWithOptionsArgs, returns *Z_KVSetWithOptionsReturns) error { if hook, ok := s.impl.(interface { - KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) + KVSetWithOptions(key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) }); ok { returns.A, returns.B = hook.KVSetWithOptions(args.A, args.B, args.C) } else { diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index b0059ea3cc..e7f942d750 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -2210,20 +2210,20 @@ func (_m *API) KVSetWithExpiry(key string, value []byte, expireInSeconds int64) return r0 } -// KVSetWithOptions provides a mock function with given fields: key, newValue, options -func (_m *API) KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { - ret := _m.Called(key, newValue, options) +// KVSetWithOptions provides a mock function with given fields: key, value, options +func (_m *API) KVSetWithOptions(key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { + ret := _m.Called(key, value, options) var r0 bool - if rf, ok := ret.Get(0).(func(string, interface{}, model.PluginKVSetOptions) bool); ok { - r0 = rf(key, newValue, options) + if rf, ok := ret.Get(0).(func(string, []byte, model.PluginKVSetOptions) bool); ok { + r0 = rf(key, value, options) } else { r0 = ret.Get(0).(bool) } var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, interface{}, model.PluginKVSetOptions) *model.AppError); ok { - r1 = rf(key, newValue, options) + if rf, ok := ret.Get(1).(func(string, []byte, model.PluginKVSetOptions) *model.AppError); ok { + r1 = rf(key, value, options) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError) diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index d5804676bd..6abf1f0a31 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -148,7 +148,7 @@ func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []b return true, nil } -func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value interface{}, opt model.PluginKVSetOptions) (bool, *model.AppError) { +func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value []byte, opt model.PluginKVSetOptions) (bool, *model.AppError) { if err := opt.IsValid(); err != nil { return false, err } @@ -159,13 +159,7 @@ func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value inter } if opt.Atomic { - var serializedOldValue []byte - serializedOldValue, err = opt.GetOldValueSerialized() - if err != nil { - return false, err - } - - return ps.CompareAndSet(kv, serializedOldValue) + return ps.CompareAndSet(kv, opt.OldValue) } savedKv, err := ps.SaveOrUpdate(kv) diff --git a/store/store.go b/store/store.go index 29092d95b8..c8d46f9e77 100644 --- a/store/store.go +++ b/store/store.go @@ -531,7 +531,7 @@ type PluginStore interface { SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) - SetWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) + SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) Delete(pluginId, key string) *model.AppError DeleteAllForPlugin(PluginId string) *model.AppError diff --git a/store/storetest/mocks/PluginStore.go b/store/storetest/mocks/PluginStore.go index 0eb4c77a38..f441c92f3a 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -184,18 +184,18 @@ func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.Plugin } // SetWithOptions provides a mock function with given fields: pluginId, key, value, options -func (_m *PluginStore) SetWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (_m *PluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { ret := _m.Called(pluginId, key, value, options) var r0 bool - if rf, ok := ret.Get(0).(func(string, string, interface{}, model.PluginKVSetOptions) bool); ok { + if rf, ok := ret.Get(0).(func(string, string, []byte, model.PluginKVSetOptions) bool); ok { r0 = rf(pluginId, key, value, options) } else { r0 = ret.Get(0).(bool) } var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, string, interface{}, model.PluginKVSetOptions) *model.AppError); ok { + if rf, ok := ret.Get(1).(func(string, string, []byte, model.PluginKVSetOptions) *model.AppError); ok { r1 = rf(pluginId, key, value, options) } else { if ret.Get(1) != nil {