diff --git a/app/plugin_api.go b/app/plugin_api.go index da9dfef180..a99067fcb4 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -660,6 +660,10 @@ func (api *PluginAPI) KVCompareAndSet(key string, oldValue, newValue []byte) (bo return api.app.CompareAndSetPluginKey(api.id, key, oldValue, newValue) } +func (api *PluginAPI) KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) { + return api.app.CompareAndDeletePluginKey(api.id, key, oldValue) +} + func (api *PluginAPI) KVSetWithExpiry(key string, value []byte, expireInSeconds int64) *model.AppError { return api.app.SetPluginKeyWithExpiry(api.id, key, value, expireInSeconds) } diff --git a/app/plugin_api_test.go b/app/plugin_api_test.go index 69401f64cc..5f3a756016 100644 --- a/app/plugin_api_test.go +++ b/app/plugin_api_test.go @@ -1224,6 +1224,58 @@ func TestPluginAPIKVCompareAndSet(t *testing.T) { } } +func TestPluginAPIKVCompareAndDelete(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + api := th.SetupPluginAPI() + + testCases := []struct { + Description string + ExpectedValue []byte + }{ + { + Description: "Testing non-nil, non-empty value", + ExpectedValue: []byte("value1"), + }, + { + Description: "Testing empty value", + ExpectedValue: []byte(""), + }, + } + + for i, testCase := range testCases { + t.Run(testCase.Description, func(t *testing.T) { + expectedKey := fmt.Sprintf("Key%d", i) + expectedValue1 := testCase.ExpectedValue + expectedValue2 := []byte("value2") + + // Set the value + err := api.KVSet(expectedKey, expectedValue1) + require.Nil(t, err) + + // Attempt delete using an incorrect old value + deleted, err := api.KVCompareAndDelete(expectedKey, expectedValue2) + require.Nil(t, err) + require.False(t, deleted) + + // Make sure the value is still there + value, err := api.KVGet(expectedKey) + require.Nil(t, err) + require.Equal(t, expectedValue1, value) + + // Attempt delete using the proper value + deleted, err = api.KVCompareAndDelete(expectedKey, expectedValue1) + require.Nil(t, err) + require.True(t, deleted) + + // Verify it's deleted + value, err = api.KVGet(expectedKey) + require.Nil(t, err) + require.Nil(t, value) + }) + } +} + func TestPluginCreateBot(t *testing.T) { th := Setup(t) defer th.TearDown() diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index d512d61999..6bff32c3dd 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -68,6 +68,26 @@ func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newV return updated, nil } +func (a *App) CompareAndDeletePluginKey(pluginId string, key string, oldValue []byte) (bool, *model.AppError) { + kv := &model.PluginKeyValue{ + PluginId: pluginId, + Key: key, + } + + deleted, err := a.Srv.Store.Plugin().CompareAndDelete(kv, oldValue) + if err != nil { + mlog.Error("Failed to compare and delete plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + return deleted, 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 deleted, nil +} + func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError) { if kv, err := a.Srv.Store.Plugin().Get(pluginId, key); err == nil { return kv.Value, nil diff --git a/plugin/api.go b/plugin/api.go index 8b652d3684..fed0bbb3df 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -469,6 +469,14 @@ type API interface { // Minimum server version: 5.12 KVCompareAndSet(key string, oldValue, newValue []byte) (bool, *model.AppError) + // KVCompareAndDelete deletes a key-value pair, unique per plugin, but only if the current value matches the given oldValue. + // Returns (false, err) if DB error occurred + // Returns (false, nil) if current value != oldValue or key does not exist when deleting + // Returns (true, nil) if current value == oldValue and the key was deleted + // + // Minimum server version: 5.16 + KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) + // KVSet stores a key-value pair with an expiry time, unique per plugin. // // Minimum server version: 5.6 diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index 3634e72bce..22b577f3c5 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -3521,6 +3521,36 @@ func (s *apiRPCServer) KVCompareAndSet(args *Z_KVCompareAndSetArgs, returns *Z_K return nil } +type Z_KVCompareAndDeleteArgs struct { + A string + B []byte +} + +type Z_KVCompareAndDeleteReturns struct { + A bool + B *model.AppError +} + +func (g *apiRPCClient) KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) { + _args := &Z_KVCompareAndDeleteArgs{key, oldValue} + _returns := &Z_KVCompareAndDeleteReturns{} + if err := g.client.Call("Plugin.KVCompareAndDelete", _args, _returns); err != nil { + log.Printf("RPC call to KVCompareAndDelete API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) KVCompareAndDelete(args *Z_KVCompareAndDeleteArgs, returns *Z_KVCompareAndDeleteReturns) error { + if hook, ok := s.impl.(interface { + KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) + }); ok { + returns.A, returns.B = hook.KVCompareAndDelete(args.A, args.B) + } else { + return encodableError(fmt.Errorf("API KVCompareAndDelete called but not implemented.")) + } + return nil +} + type Z_KVSetWithExpiryArgs struct { A string B []byte diff --git a/plugin/helpers.go b/plugin/helpers.go index ed230cd548..5f96bf4da1 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -22,6 +22,14 @@ type Helpers interface { // Minimum server version: 5.12 KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) + // KVCompareAndDeleteJSON deletes a key-value pair, unique per plugin, but only if the current value matches the given oldValue after marshalling as a JSON string. + // Returns (false, err) if DB error occurred + // Returns (false, nil) if current value != oldValue or the key was already deleted + // Returns (true, nil) if current value == oldValue + // + // Minimum server version: 5.16 + KVCompareAndDeleteJSON(key string, oldValue interface{}) (bool, error) + // 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) diff --git a/plugin/helpers_kv.go b/plugin/helpers_kv.go index d230698b1c..b489ba7591 100644 --- a/plugin/helpers_kv.go +++ b/plugin/helpers_kv.go @@ -59,6 +59,21 @@ func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newV return p.API.KVCompareAndSet(key, oldData, newData) } +// KVCompareAndDeleteJSON is a wrapper around KVCompareAndDelete to simplify atomically deleting a JSON object from the key value store. +func (p *HelpersImpl) KVCompareAndDeleteJSON(key string, oldValue interface{}) (bool, error) { + var oldData []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") + } + } + + return p.API.KVCompareAndDelete(key, oldData) +} + // 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) diff --git a/plugin/helpers_kv_test.go b/plugin/helpers_kv_test.go index bc313ba05a..23a6d37fdf 100644 --- a/plugin/helpers_kv_test.go +++ b/plugin/helpers_kv_test.go @@ -175,6 +175,46 @@ func TestKVCompareAndSetJSON(t *testing.T) { }) } +func TestKVCompareAndDeleteJSON(t *testing.T) { + t.Run("old value JSON marshal error", func(t *testing.T) { + api := &plugintest.API{} + api.AssertNotCalled(t, "KVCompareAndDelete") + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndDeleteJSON("test-key", func() { return }) + + api.AssertExpectations(t) + assert.Equal(t, false, ok) + assert.NotNil(t, err) + }) + + t.Run("old value nil", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndDelete", "test-key", []byte(nil)).Return(true, nil) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndDeleteJSON("test-key", nil) + + 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("KVCompareAndDelete", "test-key", []byte(`{"val-a":10}`)).Return(true, nil) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndDeleteJSON("test-key", map[string]interface{}{ + "val-a": 10, + }) + + 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{} diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index 768e7635f9..f634748a1e 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -1903,6 +1903,29 @@ func (_m *API) HasPermissionToTeam(userId string, teamId string, permission *mod return r0 } +// KVCompareAndDelete provides a mock function with given fields: key, oldValue +func (_m *API) KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) { + ret := _m.Called(key, oldValue) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, []byte) bool); ok { + r0 = rf(key, oldValue) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, []byte) *model.AppError); ok { + r1 = rf(key, oldValue) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // KVCompareAndSet provides a mock function with given fields: key, oldValue, newValue func (_m *API) KVCompareAndSet(key string, oldValue []byte, newValue []byte) (bool, *model.AppError) { ret := _m.Called(key, oldValue, newValue) diff --git a/plugin/plugintest/helpers.go b/plugin/plugintest/helpers.go index bf474c8471..ba3f92b33a 100644 --- a/plugin/plugintest/helpers.go +++ b/plugin/plugintest/helpers.go @@ -33,6 +33,27 @@ func (_m *Helpers) EnsureBot(bot *model.Bot) (string, error) { return r0, r1 } +// KVCompareAndDeleteJSON provides a mock function with given fields: key, oldValue +func (_m *Helpers) KVCompareAndDeleteJSON(key string, oldValue interface{}) (bool, error) { + ret := _m.Called(key, oldValue) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, interface{}) bool); ok { + r0 = rf(key, oldValue) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, interface{}) error); ok { + r1 = rf(key, oldValue) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // KVCompareAndSetJSON provides a mock function with given fields: key, oldValue, newValue func (_m *Helpers) KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) { ret := _m.Called(key, oldValue, newValue) diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index 90e79b794f..4d2f00a2d8 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -71,6 +71,11 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte return false, err } + if kv.Value == nil { + // Setting a key to nil is the same as removing it + return ps.CompareAndDelete(kv, oldValue) + } + if oldValue == nil { // Insert if oldValue is nil if err := ps.GetMaster().Insert(kv); err != nil { @@ -111,6 +116,37 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte return true, nil } +func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { + if err := kv.IsValid(); err != nil { + return false, err + } + + if oldValue == nil { + // nil can't be stored. Return showing that we didn't do anything + return false, nil + } + + deleteResult, err := ps.GetMaster().Exec( + `DELETE FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Old`, + map[string]interface{}{ + "PluginId": kv.PluginId, + "Key": kv.Key, + "Old": oldValue, + }, + ) + if err != nil { + return false, model.NewAppError("SqlPluginStore.CompareAndDelete", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + if rowsAffected, err := deleteResult.RowsAffected(); err != nil { + return false, model.NewAppError("SqlPluginStore.CompareAndDelete", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } else if rowsAffected == 0 { + return false, nil + } + + return true, nil +} + func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) { var kv *model.PluginKeyValue currentTime := model.GetMillis() diff --git a/store/store.go b/store/store.go index c0d1cabdf5..158c98d929 100644 --- a/store/store.go +++ b/store/store.go @@ -520,6 +520,7 @@ type UserAccessTokenStore interface { 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) 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 c044b0b9ec..d618df0ce4 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -12,6 +12,29 @@ type PluginStore struct { mock.Mock } +// CompareAndDelete provides a mock function with given fields: keyVal, oldValue +func (_m *PluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { + ret := _m.Called(keyVal, oldValue) + + var r0 bool + if rf, ok := ret.Get(0).(func(*model.PluginKeyValue, []byte) bool); ok { + r0 = rf(keyVal, oldValue) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.PluginKeyValue, []byte) *model.AppError); ok { + r1 = rf(keyVal, oldValue) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // CompareAndSet provides a mock function with given fields: keyVal, oldValue func (_m *PluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { ret := _m.Called(keyVal, oldValue)