MM-16821 - Add a KVCompareAndDelete to the plugin API (#11804)

* Implement KVCompareAndDelete and KVCompareAndDeleteJSON

* Add tests for KVCompareAndDelete

* Update minimum server version

* Handle nil value on CompareAndSet so that it deletes it

* Fix comments

* Tweaks from PR comments

* Go back to deleted, err

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)
 }

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()

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

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

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

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)
 

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)

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{}

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)

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)

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()

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

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)