MM-21328: Fix KVCompareAndSet when new==old (#13612)

`KVCompareAndSet(key, sameValue, sameValue)` can fail spuriously on MySQL if the underlying `UPDATE` requires no actual changes. As per the [MySQL documentation](https://dev.mysql.com/doc/refman/8.0/en/mysql-affected-rows.html), we can't rely on rows affected in this case:

> For UPDATE statements, the affected-rows value by default is the number of rows actually changed. If you specify the CLIENT_FOUND_ROWS flag to mysql_real_connect() when connecting to mysqld, the affected-rows value is the number of rows “found”; that is, matched by the WHERE clause.

It's not tenable to change `CLIENT_FOUND_ROWS` for the all connection, so handle this case in the code instead by running a `SELECT` after the fact. Note that `KVCompareAndSet` has no guarantee of atomicity in this case, but neither would `CompareAndSwap` on which this is method was inspired.

Finally, note that no changes are required for Postgres, which has sane semantics as the default.

Fixes: https://mattermost.atlassian.net/browse/MM-21328

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

i18n/en.json

@@ -6450,6 +6450,14 @@
     "id": "store.sql_oauth.update_app.updating.app_error",
     "translation": "We encountered an error updating the app"
   },
+  {
+    "id": "store.sql_plugin_store.compare_and_set.mysql_select.app_error",
+    "translation": "Failed to query for existing row on MySQL after KVCompareAndSet with unchanged value."
+  },
+  {
+    "id": "store.sql_plugin_store.compare_and_set.too_many_rows.app_error",
+    "translation": "Found more than 1 row on MySQL after KVCompareAndSet with unchanged value."
+  },
   {
     "id": "store.sql_plugin_store.delete.app_error",
     "translation": "Could not delete plugin key value"

store/sqlstore/plugin_store.go

@@ -4,6 +4,7 @@
 package sqlstore
 
 import (
+	"bytes"
 	"database/sql"
 	"fmt"
 	"net/http"
@@ -108,6 +109,33 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte
 			// Failed to update
 			return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else if rowsAffected == 0 {
+			if ps.DriverName() == model.DATABASE_DRIVER_MYSQL && bytes.Equal(oldValue, kv.Value) {
+				// ROW_COUNT on MySQL is zero even if the row existed but no changes to the row were required.
+				// Check if the row exists with the required value to distinguish this case. Strictly speaking,
+				// this isn't a good use of CompareAndSet anyway, since there's no corresponding guarantee of
+				// atomicity. Nevertheless, let's return results consistent with Postgres and with what might
+				// be expected in this case.
+				count, err := ps.GetReplica().SelectInt(
+					"SELECT COUNT(*) FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Value",
+					map[string]interface{}{
+						"PluginId": kv.PluginId,
+						"Key":      kv.Key,
+						"Value":    kv.Value,
+					},
+				)
+				if err != nil {
+					return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.compare_and_set.mysql_select.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", kv.PluginId, kv.Key, err.Error()), http.StatusInternalServerError)
+				}
+
+				if count == 0 {
+					return false, nil
+				} else if count == 1 {
+					return true, nil
+				} else {
+					return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.compare_and_set.too_many_rows.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, count=%d", kv.PluginId, kv.Key, count), http.StatusInternalServerError)
+				}
+			}
+
 			// No rows were affected by the update, where condition was not satisfied,
 			// return false, but no error.
 			return false, nil

store/storetest/plugin_store.go

@@ -14,6 +14,7 @@ import (
 )
 
 func TestPluginStore(t *testing.T, ss store.Store) {
+	t.Run("CompareAndSet", func(t *testing.T) { testPluginCompareAndSet(t, ss) })
 	t.Run("PluginSaveGet", func(t *testing.T) { testPluginSaveGet(t, ss) })
 	t.Run("PluginSaveGetExpiry", func(t *testing.T) { testPluginSaveGetExpiry(t, ss) })
 	t.Run("PluginDelete", func(t *testing.T) { testPluginDelete(t, ss) })
@@ -21,6 +22,74 @@ func TestPluginStore(t *testing.T, ss store.Store) {
 	t.Run("PluginDeleteExpired", func(t *testing.T) { testPluginDeleteExpired(t, ss) })
 }
 
+func testPluginCompareAndSet(t *testing.T, ss store.Store) {
+	kv := &model.PluginKeyValue{
+		PluginId: model.NewId(),
+		Key:      model.NewId(),
+		Value:    []byte(model.NewId()),
+		ExpireAt: 0,
+	}
+	defer func() {
+		_ = ss.Plugin().Delete(kv.PluginId, kv.Key)
+	}()
+
+	t.Run("set non-existent key should succeed given nil old value", func(t *testing.T) {
+		ok, err := ss.Plugin().CompareAndSet(kv, nil)
+		require.Nil(t, err)
+		assert.True(t, ok)
+	})
+
+	t.Run("set existing key with new value should succeed given same old value", func(t *testing.T) {
+		_, err := ss.Plugin().SaveOrUpdate(kv)
+		require.Nil(t, err)
+
+		kvNew := &model.PluginKeyValue{
+			PluginId: kv.PluginId,
+			Key:      kv.Key,
+			Value:    []byte(model.NewId()),
+			ExpireAt: 0,
+		}
+
+		ok, err := ss.Plugin().CompareAndSet(kvNew, kv.Value)
+		require.Nil(t, err)
+		assert.True(t, ok)
+	})
+
+	t.Run("set existing key with new value should fail given different old value", func(t *testing.T) {
+		_, err := ss.Plugin().SaveOrUpdate(kv)
+		require.Nil(t, err)
+
+		kvNew := &model.PluginKeyValue{
+			PluginId: kv.PluginId,
+			Key:      kv.Key,
+			Value:    []byte(model.NewId()),
+			ExpireAt: 0,
+		}
+
+		ok, err := ss.Plugin().CompareAndSet(kvNew, []byte(model.NewId()))
+		require.Nil(t, err)
+		assert.False(t, ok)
+	})
+
+	t.Run("set existing key with same value should succeed given same old value", func(t *testing.T) {
+		_, err := ss.Plugin().SaveOrUpdate(kv)
+		require.Nil(t, err)
+
+		ok, err := ss.Plugin().CompareAndSet(kv, kv.Value)
+		require.Nil(t, err)
+		assert.True(t, ok)
+	})
+
+	t.Run("set existing key with same value should fail given different old value", func(t *testing.T) {
+		_, err := ss.Plugin().SaveOrUpdate(kv)
+		require.Nil(t, err)
+
+		ok, err := ss.Plugin().CompareAndSet(kv, []byte(model.NewId()))
+		require.Nil(t, err)
+		assert.False(t, ok)
+	})
+}
+
 func testPluginSaveGet(t *testing.T, ss store.Store) {
 	kv := &model.PluginKeyValue{
 		PluginId: model.NewId(),