delete kv on setting nil (#13769)

Setting a `nil` value should actually delete the row instead of preserve it with a `nil` value.

The existing tests passed because they only checked the return value of `nil`. When running the tests on Postgres, this value is returned as an empty `[]byte{}` instead of `nil`, so update the test to reflect this semantically equivalent behaviour.

app/plugin_test.go

@@ -190,16 +190,12 @@ func TestPluginKeyValueStoreCompareAndSet(t *testing.T) {
 }
 
 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("storing a value without providing options works", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-1"), model.PluginKVSetOptions{})
 		assert.True(t, result)
 		assert.Nil(t, err)
@@ -211,6 +207,9 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 	})
 
 	t.Run("test that setting it atomic when it doesn't match doesn't change anything", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		err := th.App.SetPluginKey(pluginId, "key", []byte("value-1"))
 		require.Nil(t, err)
 
@@ -228,6 +227,9 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 	})
 
 	t.Run("test the atomic change with the proper old value", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		err := th.App.SetPluginKey(pluginId, "key", []byte("value-2"))
 		require.Nil(t, err)
 
@@ -245,6 +247,9 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 	})
 
 	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) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		// first set a value.
 		result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-2", []byte("value-1"), model.PluginKVSetOptions{})
 		require.Nil(t, err)
@@ -264,6 +269,9 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 	})
 
 	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) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		// first set a value.
 		result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-3", []byte("value-1"), model.PluginKVSetOptions{})
 		require.Nil(t, err)
@@ -274,12 +282,21 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 		assert.Nil(t, err)
 		assert.True(t, result)
 
+		// verify a nil value is returned
 		ret, err := th.App.GetPluginKey(pluginId, "nil-test-key-3")
 		assert.Nil(t, err)
 		assert.Nil(t, ret)
+
+		// verify the row is actually gone
+		list, err := th.App.ListPluginKeys(pluginId, 0, 1)
+		assert.Nil(t, err)
+		assert.Empty(t, list)
 	})
 
 	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) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		result, err := th.App.SetPluginKeyWithOptions(pluginId, "nil-test-key-4", []byte("value-1"), model.PluginKVSetOptions{
 			Atomic:   true,
 			OldValue: nil,
@@ -293,6 +310,9 @@ func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) {
 	})
 
 	t.Run("test that value is set and unset with ExpireInSeconds", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+
 		result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte("value-1"), model.PluginKVSetOptions{
 			ExpireInSeconds: 1,
 		})

store/sqlstore/plugin_store.go

@@ -42,6 +42,16 @@ func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKe
 		return nil, err
 	}
 
+	if kv.Value == nil {
+		// Setting a key to nil is the same as removing it
+		err := ps.Delete(kv.PluginId, kv.Key)
+		if err != nil {
+			return nil, err
+		}
+
+		return kv, nil
+	}
+
 	if ps.DriverName() == model.DATABASE_DRIVER_POSTGRES {
 		// Unfortunately PostgreSQL pre-9.5 does not have an atomic upsert, so we use
 		// separate update and insert queries to accomplish our upsert