MM-16822 - Implement KVSetWithOptions (#11818)

* Add SetWithOptions * Avoid passing two structs to the functions * Rename ExpiryInSeconds -> ExpireInSeconds * Use t.Run for the tests * Fix build * Address feedback * Update log message * Update docs and use KVSetWithOptions in KVCompareAndSetJSON * Improve code style * Use struct instead of pointer to struct * Fix minimum server versions * Update documentation * Address feedback * Revert new implemention of kv helpers * Adress feedback
2019-11-04 09:49:54 -03:00
--- a/app/plugin_api.go
+++ b/app/plugin_api.go
@@ -695,6 +695,10 @@ 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) {
+	return api.app.SetPluginKeyWithOptions(api.id, key, value, options)
+}
+
 func (api *PluginAPI) KVSet(key string, value []byte) *model.AppError {
 	return api.app.SetPluginKey(api.id, key, value)
 }
--- a/app/plugin_key_value_store.go
+++ b/app/plugin_key_value_store.go
@@ -23,40 +23,30 @@ func (a *App) SetPluginKey(pluginId string, key string, value []byte) *model.App
 }

 func (a *App) SetPluginKeyWithExpiry(pluginId string, key string, value []byte, expireInSeconds int64) *model.AppError {
-	if expireInSeconds > 0 {
-		expireInSeconds = model.GetMillis() + (expireInSeconds * 1000)
+	options := model.PluginKVSetOptions{
+		ExpireInSeconds: expireInSeconds,
 	}
-
-	kv := &model.PluginKeyValue{
-		PluginId: pluginId,
-		Key:      key,
-		Value:    value,
-		ExpireAt: expireInSeconds,
-	}
-
-	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.
-	if err := a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key)); err != nil {
-		mlog.Error("Failed to clean up previously hashed plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
-	}
-
-	return nil
+	_, err := a.SetPluginKeyWithOptions(pluginId, key, value, options)
+	return err
 }

 func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newValue []byte) (bool, *model.AppError) {
-	kv := &model.PluginKeyValue{
-		PluginId: pluginId,
-		Key:      key,
-		Value:    newValue,
+	options := model.PluginKVSetOptions{
+		Atomic:   true,
+		OldValue: oldValue,
+	}
+	return a.SetPluginKeyWithOptions(pluginId, key, newValue, options)
+}
+
+func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value interface{}, 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
 	}

-	updated, err := a.Srv.Store.Plugin().CompareAndSet(kv, oldValue)
+	updated, err := a.Srv.Store.Plugin().SetWithOptions(pluginId, key, value, options)
 	if err != nil {
-		mlog.Error("Failed to compare and set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
+		mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err))
 		return updated, err
 	}

--- a/app/plugin_test.go
+++ b/app/plugin_test.go
@@ -16,11 +16,12 @@ import (
 	"testing"

 	"github.com/gorilla/mux"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/plugin"
 	"github.com/mattermost/mattermost-server/utils/fileutils"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )

 func getHashedKey(key string) string {
@@ -186,6 +187,147 @@ func TestPluginKeyValueStoreCompareAndSet(t *testing.T) {
 	assert.Equal(t, []byte("test2"), ret)
 }

+func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	pluginId := "testpluginid"
+
+	defer func() {
+		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,
+		})
+		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)
+	})
+
+	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,
+			},
+		})
+		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(`{"val-a":10}`), 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,
+			},
+		})
+		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(`{"val-a":30}`), ret)
+	})
+}
+
+func TestPluginKeyValueStoreSetWithOptionsByteArray(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	pluginId := "testpluginid"
+
+	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`),
+	})
+	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)
+
+	// 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`),
+	})
+	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)
+}
+
 func TestServePluginRequest(t *testing.T) {
 	th := Setup(t).InitBasic()
 	defer th.TearDown()