[MM-20353] Change KVSetWithOptions to accept a byte slice (#13213)

* Drop EncodeJSON from PluginKVSetOptions * Add unit tests for KVSetWithOptions with nil values
2019-12-03 10:46:15 +01:00
--- 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)
 }

--- 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
--- 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) {
--- 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"
--- 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,
 	}

--- 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.
 	//
--- 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 {
--- 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)
--- 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)
--- 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
--- 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 {