From 620d941b6ee5a54b0f793404c0d21a930179e657 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 5 Jul 2019 17:33:39 -0300 Subject: [PATCH] MM-8602: KV Store Json helper tweaks (#11502) * move kv helpers to helpers_kv*.go * change KVGetJSON return signature Return a boolean and an error, to clearly indicate if no value was found and thus no value unmarshalled into the target interface. Also fix an issue with CompareAndSet to allow an oldValue of nil (i.e. expected to be unset). * add missing license * tweak documentation * document KVSetWithExpiryJSON minimum version --- plugin/api.go | 19 ++-- plugin/helpers.go | 29 ++--- plugin/helpers_bots.go | 40 ------- plugin/helpers_bots_test.go | 176 ------------------------------ plugin/helpers_kv.go | 70 ++++++++++++ plugin/helpers_kv_test.go | 204 +++++++++++++++++++++++++++++++++++ plugin/plugintest/helpers.go | 17 ++- 7 files changed, 311 insertions(+), 244 deletions(-) create mode 100644 plugin/helpers_kv.go create mode 100644 plugin/helpers_kv_test.go diff --git a/plugin/api.go b/plugin/api.go index 5c901dcc01..f9e29c7058 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -456,37 +456,36 @@ type API interface { // KV Store Section - // KVSet will store a key-value pair, unique per plugin. + // KVSet stores a key-value pair, unique per plugin. // Provided helper functions and internal plugin code will use the prefix `mmi_` before keys. Do not use this prefix. KVSet(key string, value []byte) *model.AppError - // KVCompareAndSet will update a key-value pair, - // unique per plugin, to the given new value if the current value == the old value. + // KVCompareAndSet updates a key-value pair, unique per plugin, but only if the current value matches the given oldValue. // Inserts a new key if oldValue == nil. // Returns (false, err) if DB error occurred - // Returns (false, nil) if current value != old value or key already exists when inserting - // Returns (true, nil) if current value == old value or new key is inserted + // Returns (false, nil) if current value != oldValue or key already exists when inserting + // Returns (true, nil) if current value == oldValue or new key is inserted // // Minimum server version: 5.12 KVCompareAndSet(key string, oldValue, newValue []byte) (bool, *model.AppError) - // KVSet will store a key-value pair, unique per plugin with an expiry time + // KVSet stores a key-value pair with an expiry time, unique per plugin. // // Minimum server version: 5.6 KVSetWithExpiry(key string, value []byte, expireInSeconds int64) *model.AppError - // KVGet will retrieve a value based on the key. Returns nil for non-existent keys. + // KVGet retrieves a value based on the key, unique per plugin. Returns nil for non-existent keys. KVGet(key string) ([]byte, *model.AppError) - // KVDelete will remove a key-value pair. Returns nil for non-existent keys. + // KVDelete removes a key-value pair, unique per plugin. Returns nil for non-existent keys. KVDelete(key string) *model.AppError - // KVDeleteAll will remove all key-value pairs for a plugin. + // KVDeleteAll removes all key-value pairs for a plugin. // // Minimum server version: 5.6 KVDeleteAll() *model.AppError - // KVList will list all keys for a plugin. + // KVList lists all keys for a plugin. // // Minimum server version: 5.6 KVList(page, perPage int) ([]string, *model.AppError) diff --git a/plugin/helpers.go b/plugin/helpers.go index a0f77e4972..ed230cd548 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -6,25 +6,28 @@ package plugin import "github.com/mattermost/mattermost-server/model" type Helpers interface { - // EnsureBot either returns an existing bot user or creates a bot user with - // the specifications of the passed bot. - // Returns the id of the bot created or existing. + // EnsureBot either returns an existing bot user matching the given bot, or creates a bot user from the given bot. + // Returns the id of the resulting bot. EnsureBot(bot *model.Bot) (string, error) - // KVGetJSON retrievs a value based on the key. - KVGetJSON(key string, value interface{}) error - - // KVSetJSON stores a key-value pair. + // KVSetJSON stores a key-value pair, unique per plugin, marshalling the given value as a JSON string. KVSetJSON(key string, value interface{}) error - // KVCompareAndSetJSON updates a key-value pair if the current - // value is equal to oldValue. - // Returns (false, err) if DB/marshal/unmarshal error occurred - // Returns (false, nil) if current value != old value - // Returns (true, nil) if current value == old value or new key is inserted + // KVCompareAndSetJSON updates a key-value pair, unique per plugin, but only if the current value matches the given oldValue after marshalling as a JSON string. + // Inserts a new key if oldValue == nil. + // Returns (false, err) if DB error occurred + // Returns (false, nil) if current value != oldValue or key already exists when inserting + // Returns (true, nil) if current value == oldValue or new key is inserted + // + // Minimum server version: 5.12 KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) - // KVSetWithExpiryJSON stores a key-value pair with an expiry time. + // KVGetJSON retrieves a value based on the key, unique per plugin, unmarshalling the previously set JSON string into the given value. Returns true if the key exists. + KVGetJSON(key string, value interface{}) (bool, error) + + // KVSetWithExpiryJSON stores a key-value pair with an expiry time, unique per plugin, marshalling the given value as a JSON string. + // + // Minimum server version: 5.6 KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error } diff --git a/plugin/helpers_bots.go b/plugin/helpers_bots.go index 4dac270dca..664a954d5d 100644 --- a/plugin/helpers_bots.go +++ b/plugin/helpers_bots.go @@ -4,7 +4,6 @@ package plugin import ( - "encoding/json" "time" "github.com/mattermost/mattermost-server/model" @@ -65,42 +64,3 @@ func (p *HelpersImpl) EnsureBot(bot *model.Bot) (retBotId string, retErr error) return createdBot.UserId, nil } - -func (p *HelpersImpl) KVGetJSON(key string, value interface{}) error { - data, err := p.API.KVGet(key) - if err != nil { - return err - } - - return json.Unmarshal(data, value) -} - -func (p *HelpersImpl) KVSetJSON(key string, value interface{}) error { - data, err := json.Marshal(value) - if err != nil { - return err - } - return p.API.KVSet(key, data) -} - -func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) { - oldData, err := json.Marshal(oldValue) - if err != nil { - return false, errors.Wrap(err, "unable to marshal old value") - } - - newData, err := json.Marshal(newValue) - if err != nil { - return false, errors.Wrap(err, "unable to marshal new value") - } - - return p.API.KVCompareAndSet(key, oldData, newData) -} - -func (p *HelpersImpl) KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error { - data, err := json.Marshal(value) - if err != nil { - return err - } - return p.API.KVSetWithExpiry(key, data, expireInSeconds) -} diff --git a/plugin/helpers_bots_test.go b/plugin/helpers_bots_test.go index cbd1b2405b..776c32a10b 100644 --- a/plugin/helpers_bots_test.go +++ b/plugin/helpers_bots_test.go @@ -153,179 +153,3 @@ func TestEnsureBot(t *testing.T) { }) }) } - -func TestKVGetJSON(t *testing.T) { - setupAPI := func() *plugintest.API { - return &plugintest.API{} - } - - t.Run("KVGet error", func(t *testing.T) { - p := &plugin.HelpersImpl{} - - api := setupAPI() - api.On("KVGet", plugin.BOT_USER_KEY).Return(nil, &model.AppError{}) - p.API = api - - var dat map[string]interface{} - - err := p.KVGetJSON(plugin.BOT_USER_KEY, dat) - - api.AssertExpectations(t) - assert.NotNil(t, err) - assert.Nil(t, dat) - }) - - t.Run("Malformed JSON", func(t *testing.T) { - key := "test-key" - - p := &plugin.HelpersImpl{} - - api := setupAPI() - api.On("KVGet", key).Return([]byte(`{{:}"val-a": 10}`), nil) - p.API = api - - var dat map[string]interface{} - - err := p.KVGetJSON(key, &dat) - - api.AssertExpectations(t) - assert.NotNil(t, err) - assert.Nil(t, dat) - }) - - t.Run("Valid parameters passed (happy-path)", func(t *testing.T) { - key := "test-key" - - p := &plugin.HelpersImpl{} - - api := setupAPI() - api.On("KVGet", key).Return([]byte(`{"val-a": 10}`), nil) - p.API = api - - var dat map[string]interface{} - - err := p.KVGetJSON(key, &dat) - - api.AssertExpectations(t) - assert.Nil(t, err) - assert.Equal(t, map[string]interface{}{ - "val-a": float64(10), - }, dat) - }) -} - -func TestKVSetJSON(t *testing.T) { - key := "test-key" - - setupAPI := func() *plugintest.API { - return &plugintest.API{} - } - - t.Run("JSON Marshal error", func(t *testing.T) { - api := setupAPI() - api.AssertNotCalled(t, "KVSet") - - p := &plugin.HelpersImpl{API: api} - - err := p.KVSetJSON(key, func() { return }) - - api.AssertExpectations(t) - assert.NotNil(t, err) - }) - - t.Run("Valid parameters passed (Happy-path)", func(t *testing.T) { - api := setupAPI() - api.On("KVSet", key, []byte(`{"val-a":10}`)).Return(nil) - - p := &plugin.HelpersImpl{API: api} - - err := p.KVSetJSON(key, map[string]interface{}{ - "val-a": float64(10), - }) - - api.AssertExpectations(t) - assert.Nil(t, err) - }) -} - -func TestKVCompareAndSetJSON(t *testing.T) { - key := "test-key" - setupAPI := func() *plugintest.API { - return &plugintest.API{} - } - - t.Run("old value JSON marshal error", func(t *testing.T) { - api := setupAPI() - api.AssertNotCalled(t, "KVCompareAndSet") - p := &plugin.HelpersImpl{API: api} - - ok, err := p.KVCompareAndSetJSON(key, func() { return }, map[string]interface{}{}) - - api.AssertExpectations(t) - assert.Equal(t, false, ok) - assert.NotNil(t, err) - }) - - t.Run("new value JSON marshal error", func(t *testing.T) { - api := setupAPI() - api.AssertNotCalled(t, "KVCompareAndSet") - - p := &plugin.HelpersImpl{API: api} - - ok, err := p.KVCompareAndSetJSON(key, map[string]interface{}{}, func() { return }) - - api.AssertExpectations(t) - assert.Equal(t, false, ok) - assert.NotNil(t, err) - }) - - t.Run("Valid parameters passed (happy-path)", func(t *testing.T) { - api := setupAPI() - api.On("KVCompareAndSet", key, []byte(`{"val-a":10}`), []byte(`{"val-b":20}`)).Return(false, nil) - p := &plugin.HelpersImpl{API: api} - - ok, err := p.KVCompareAndSetJSON(key, map[string]interface{}{ - "val-a": 10, - }, map[string]interface{}{ - "val-b": 20, - }) - - api.AssertExpectations(t) - assert.Equal(t, false, ok) - assert.Nil(t, err) - }) -} - -func TestKVSetWithExpiryJSON(t *testing.T) { - key := "test-key" - - setupAPI := func() *plugintest.API { - return &plugintest.API{} - } - - t.Run("JSON Marshal error", func(t *testing.T) { - api := setupAPI() - api.AssertNotCalled(t, "KVSetWithExpiry") - - p := &plugin.HelpersImpl{API: api} - - err := p.KVSetWithExpiryJSON(key, func() { return }, 100) - - api.AssertExpectations(t) - assert.NotNil(t, err) - }) - - t.Run("valid parameters passed (happy-path)", func(t *testing.T) { - api := setupAPI() - api.On("KVSetWithExpiry", key, []byte(`{"val-a":10}`), int64(100)).Return(nil) - - p := &plugin.HelpersImpl{API: api} - - err := p.KVSetWithExpiryJSON(key, map[string]interface{}{ - "val-a": float64(10), - }, 100) - - api.AssertExpectations(t) - assert.Nil(t, err) - }) -} diff --git a/plugin/helpers_kv.go b/plugin/helpers_kv.go new file mode 100644 index 0000000000..d230698b1c --- /dev/null +++ b/plugin/helpers_kv.go @@ -0,0 +1,70 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "encoding/json" + + "github.com/pkg/errors" +) + +// KVGetJSON is a wrapper around KVGet to simplify reading a JSON object from the key value store. +func (p *HelpersImpl) KVGetJSON(key string, value interface{}) (bool, error) { + data, appErr := p.API.KVGet(key) + if appErr != nil { + return false, appErr + } + if data == nil { + return false, nil + } + + err := json.Unmarshal(data, value) + if err != nil { + return false, err + } + + return true, nil +} + +// KVSetJSON is a wrapper around KVSet to simplify writing a JSON object to the key value store. +func (p *HelpersImpl) KVSetJSON(key string, value interface{}) error { + data, err := json.Marshal(value) + if err != nil { + return err + } + + return p.API.KVSet(key, data) +} + +// KVCompareAndSetJSON is a wrapper around KVCompareAndSet to simplify atomically writing a JSON object to the key value store. +func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) { + var oldData, newData []byte + var err error + + if oldValue != nil { + oldData, err = json.Marshal(oldValue) + if err != nil { + return false, errors.Wrap(err, "unable to marshal old value") + } + } + + if newValue != nil { + newData, err = json.Marshal(newValue) + if err != nil { + return false, errors.Wrap(err, "unable to marshal new value") + } + } + + return p.API.KVCompareAndSet(key, oldData, newData) +} + +// KVSetWithExpiryJSON is a wrapper around KVSetWithExpiry to simplify atomically writing a JSON object with expiry to the key value store. +func (p *HelpersImpl) KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error { + data, err := json.Marshal(value) + if err != nil { + return err + } + + return p.API.KVSetWithExpiry(key, data, expireInSeconds) +} diff --git a/plugin/helpers_kv_test.go b/plugin/helpers_kv_test.go new file mode 100644 index 0000000000..bc313ba05a --- /dev/null +++ b/plugin/helpers_kv_test.go @@ -0,0 +1,204 @@ +package plugin_test + +import ( + "testing" + + "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/plugin" + "github.com/mattermost/mattermost-server/plugin/plugintest" + "github.com/stretchr/testify/assert" +) + +func TestKVGetJSON(t *testing.T) { + t.Run("KVGet error", func(t *testing.T) { + p := &plugin.HelpersImpl{} + + api := &plugintest.API{} + api.On("KVGet", "test-key").Return(nil, &model.AppError{}) + p.API = api + + var dat map[string]interface{} + + ok, err := p.KVGetJSON("test-key", dat) + api.AssertExpectations(t) + assert.False(t, ok) + assert.NotNil(t, err) + assert.Nil(t, dat) + }) + + t.Run("unknown key", func(t *testing.T) { + p := &plugin.HelpersImpl{} + + api := &plugintest.API{} + api.On("KVGet", "test-key").Return(nil, nil) + p.API = api + + var dat map[string]interface{} + + ok, err := p.KVGetJSON("test-key", dat) + api.AssertExpectations(t) + assert.False(t, ok) + assert.Nil(t, err) + assert.Nil(t, dat) + }) + + t.Run("malformed JSON", func(t *testing.T) { + p := &plugin.HelpersImpl{} + + api := &plugintest.API{} + api.On("KVGet", "test-key").Return([]byte(`{{:}"val-a": 10}`), nil) + p.API = api + + var dat map[string]interface{} + + ok, err := p.KVGetJSON("test-key", &dat) + api.AssertExpectations(t) + assert.False(t, ok) + assert.NotNil(t, err) + assert.Nil(t, dat) + }) + + t.Run("wellformed JSON", func(t *testing.T) { + p := &plugin.HelpersImpl{} + + api := &plugintest.API{} + api.On("KVGet", "test-key").Return([]byte(`{"val-a": 10}`), nil) + p.API = api + + var dat map[string]interface{} + + ok, err := p.KVGetJSON("test-key", &dat) + assert.True(t, ok) + api.AssertExpectations(t) + assert.Nil(t, err) + assert.Equal(t, map[string]interface{}{ + "val-a": float64(10), + }, dat) + }) +} + +func TestKVSetJSON(t *testing.T) { + t.Run("JSON marshal error", func(t *testing.T) { + api := &plugintest.API{} + api.AssertNotCalled(t, "KVSet") + + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetJSON("test-key", func() { return }) + api.AssertExpectations(t) + assert.NotNil(t, err) + }) + + t.Run("marshallable struct", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVSet", "test-key", []byte(`{"val-a":10}`)).Return(nil) + + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetJSON("test-key", map[string]interface{}{ + "val-a": float64(10), + }) + + api.AssertExpectations(t) + assert.Nil(t, err) + }) +} + +func TestKVCompareAndSetJSON(t *testing.T) { + t.Run("old value JSON marshal error", func(t *testing.T) { + api := &plugintest.API{} + api.AssertNotCalled(t, "KVCompareAndSet") + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", func() { return }, map[string]interface{}{}) + + api.AssertExpectations(t) + assert.Equal(t, false, ok) + assert.NotNil(t, err) + }) + + t.Run("new value JSON marshal error", func(t *testing.T) { + api := &plugintest.API{} + api.AssertNotCalled(t, "KVCompareAndSet") + + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{}, func() { return }) + + api.AssertExpectations(t) + assert.False(t, ok) + assert.NotNil(t, err) + }) + + t.Run("old value nil", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndSet", "test-key", []byte(nil), []byte(`{"val-b":20}`)).Return(true, nil) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", nil, map[string]interface{}{ + "val-b": 20, + }) + + api.AssertExpectations(t) + assert.True(t, ok) + assert.Nil(t, err) + }) + + t.Run("old value non-nil", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndSet", "test-key", []byte(`{"val-a":10}`), []byte(`{"val-b":20}`)).Return(true, nil) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{ + "val-a": 10, + }, map[string]interface{}{ + "val-b": 20, + }) + + api.AssertExpectations(t) + assert.True(t, ok) + assert.Nil(t, err) + }) + + t.Run("new value nil", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndSet", "test-key", []byte(`{"val-a":10}`), []byte(nil)).Return(true, nil) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{ + "val-a": 10, + }, nil) + + api.AssertExpectations(t) + assert.True(t, ok) + assert.Nil(t, err) + }) +} + +func TestKVSetWithExpiryJSON(t *testing.T) { + t.Run("JSON marshal error", func(t *testing.T) { + api := &plugintest.API{} + api.AssertNotCalled(t, "KVSetWithExpiry") + + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetWithExpiryJSON("test-key", func() { return }, 100) + + api.AssertExpectations(t) + assert.NotNil(t, err) + }) + + t.Run("wellformed JSON", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVSetWithExpiry", "test-key", []byte(`{"val-a":10}`), int64(100)).Return(nil) + + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetWithExpiryJSON("test-key", map[string]interface{}{ + "val-a": float64(10), + }, 100) + + api.AssertExpectations(t) + assert.Nil(t, err) + }) +} diff --git a/plugin/plugintest/helpers.go b/plugin/plugintest/helpers.go index 17fcd75970..bf474c8471 100644 --- a/plugin/plugintest/helpers.go +++ b/plugin/plugintest/helpers.go @@ -55,17 +55,24 @@ func (_m *Helpers) KVCompareAndSetJSON(key string, oldValue interface{}, newValu } // KVGetJSON provides a mock function with given fields: key, value -func (_m *Helpers) KVGetJSON(key string, value interface{}) error { +func (_m *Helpers) KVGetJSON(key string, value interface{}) (bool, error) { ret := _m.Called(key, value) - var r0 error - if rf, ok := ret.Get(0).(func(string, interface{}) error); ok { + var r0 bool + if rf, ok := ret.Get(0).(func(string, interface{}) bool); ok { r0 = rf(key, value) } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(bool) } - return r0 + var r1 error + if rf, ok := ret.Get(1).(func(string, interface{}) error); ok { + r1 = rf(key, value) + } else { + r1 = ret.Error(1) + } + + return r0, r1 } // KVSetJSON provides a mock function with given fields: key, value