diff --git a/i18n/en.json b/i18n/en.json index 7a10d0f68c..5a5a2dc0c3 100644 --- a/i18n/en.json +++ b/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" diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index 6abf1f0a31..26b20414df 100644 --- a/store/sqlstore/plugin_store.go +++ b/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 diff --git a/store/storetest/plugin_store.go b/store/storetest/plugin_store.go index 869c56cae1..3dcb0b5018 100644 --- a/store/storetest/plugin_store.go +++ b/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(),