Migrate Plugin.SaveOrUpdate to sync by default (#11592)

2025-02-25 18:55:24 -06:00 · 2019-07-09 09:08:28 -04:00
parent e209a7acb4
commit ce61681c92
6 changed files with 68 additions and 57 deletions
--- a/store/sqlstore/plugin_store.go
+++ b/store/sqlstore/plugin_store.go
@@ -36,39 +36,34 @@ func NewSqlPluginStore(sqlStore SqlStore) store.PluginStore {
 func (ps SqlPluginStore) CreateIndexesIfNotExists() {
 }

-func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) store.StoreChannel {
-	return store.Do(func(result *store.StoreResult) {
-		if result.Err = kv.IsValid(); result.Err != nil {
-			return
-		}
+func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) {
+	if err := kv.IsValid(); err != nil {
+		return nil, err
+	}

-		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
-			if rowsAffected, err := ps.GetMaster().Update(kv); err != nil {
-				result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
-				return
-			} else if rowsAffected == 0 {
-				// No rows were affected by the update, so let's try an insert
-				if err := ps.GetMaster().Insert(kv); err != nil {
-					// If the error is from unique constraints violation, it's the result of a
-					// valid race and we can report success. Otherwise we have a real error and
-					// need to return it
-					if !IsUniqueConstraintError(err, []string{"PRIMARY", "PluginId", "Key", "PKey"}) {
-						result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
-						return
-					}
+	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
+		if rowsAffected, err := ps.GetMaster().Update(kv); err != nil {
+			return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+		} else if rowsAffected == 0 {
+			// No rows were affected by the update, so let's try an insert
+			if err := ps.GetMaster().Insert(kv); err != nil {
+				// If the error is from unique constraints violation, it's the result of a
+				// valid race and we can report success. Otherwise we have a real error and
+				// need to return it
+				if !IsUniqueConstraintError(err, []string{"PRIMARY", "PluginId", "Key", "PKey"}) {
+					return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
 				}
 			}
-		} else if ps.DriverName() == model.DATABASE_DRIVER_MYSQL {
-			if _, err := ps.GetMaster().Exec("INSERT INTO PluginKeyValueStore (PluginId, PKey, PValue, ExpireAt) VALUES(:PluginId, :Key, :Value, :ExpireAt) ON DUPLICATE KEY UPDATE PValue = :Value, ExpireAt = :ExpireAt", map[string]interface{}{"PluginId": kv.PluginId, "Key": kv.Key, "Value": kv.Value, "ExpireAt": kv.ExpireAt}); err != nil {
-				result.Err = model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
-				return
-			}
 		}
+	} else if ps.DriverName() == model.DATABASE_DRIVER_MYSQL {
+		if _, err := ps.GetMaster().Exec("INSERT INTO PluginKeyValueStore (PluginId, PKey, PValue, ExpireAt) VALUES(:PluginId, :Key, :Value, :ExpireAt) ON DUPLICATE KEY UPDATE PValue = :Value, ExpireAt = :ExpireAt", map[string]interface{}{"PluginId": kv.PluginId, "Key": kv.Key, "Value": kv.Value, "ExpireAt": kv.ExpireAt}); err != nil {
+			return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+	}

-		result.Data = kv
-	})
+	return kv, nil
 }

 func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) {
--- a/store/store.go
+++ b/store/store.go
@@ -535,7 +535,7 @@ type UserAccessTokenStore interface {
 }

 type PluginStore interface {
-	SaveOrUpdate(keyVal *model.PluginKeyValue) StoreChannel
+	SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError)
 	CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError)
 	Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError)
 	Delete(pluginId, key string) StoreChannel
--- a/store/storetest/mocks/PluginStore.go
+++ b/store/storetest/mocks/PluginStore.go
@@ -135,17 +135,26 @@ func (_m *PluginStore) List(pluginId string, page int, perPage int) ([]string, *
 }

 // SaveOrUpdate provides a mock function with given fields: keyVal
-func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) store.StoreChannel {
+func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) {
 	ret := _m.Called(keyVal)

-	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func(*model.PluginKeyValue) store.StoreChannel); ok {
+	var r0 *model.PluginKeyValue
+	if rf, ok := ret.Get(0).(func(*model.PluginKeyValue) *model.PluginKeyValue); ok {
 		r0 = rf(keyVal)
 	} else {
 		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(store.StoreChannel)
+			r0 = ret.Get(0).(*model.PluginKeyValue)
 		}
 	}

-	return r0
+	var r1 *model.AppError
+	if rf, ok := ret.Get(1).(func(*model.PluginKeyValue) *model.AppError); ok {
+		r1 = rf(keyVal)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
 }
--- a/store/storetest/plugin_store.go
+++ b/store/storetest/plugin_store.go
@@ -6,6 +6,8 @@ package storetest
 import (
 	"testing"

+	"github.com/stretchr/testify/require"
+
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store"
 	"github.com/stretchr/testify/assert"
@@ -27,8 +29,8 @@ func testPluginSaveGet(t *testing.T, ss store.Store) {
 		ExpireAt: 0,
 	}

-	if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil {
-		t.Fatal(result.Err)
+	if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil {
+		t.Fatal(err)
 	}

 	defer func() {
@@ -46,8 +48,8 @@ func testPluginSaveGet(t *testing.T, ss store.Store) {

 	// Try inserting when already exists
 	kv.Value = []byte(model.NewId())
-	if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil {
-		t.Fatal(result.Err)
+	if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil {
+		t.Fatal(err)
 	}

 	if received, err := ss.Plugin().Get(kv.PluginId, kv.Key); err != nil {
@@ -67,8 +69,8 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) {
 		ExpireAt: model.GetMillis() + 30000,
 	}

-	if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil {
-		t.Fatal(result.Err)
+	if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil {
+		t.Fatal(err)
 	}

 	defer func() {
@@ -91,8 +93,8 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) {
 		ExpireAt: model.GetMillis() - 5000,
 	}

-	if result := <-ss.Plugin().SaveOrUpdate(kv); result.Err != nil {
-		t.Fatal(result.Err)
+	if _, err := ss.Plugin().SaveOrUpdate(kv); err != nil {
+		t.Fatal(err)
 	}

 	defer func() {
@@ -105,11 +107,12 @@ func testPluginSaveGetExpiry(t *testing.T, ss store.Store) {
 }

 func testPluginDelete(t *testing.T, ss store.Store) {
-	kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
+	kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
 		PluginId: model.NewId(),
 		Key:      model.NewId(),
 		Value:    []byte(model.NewId()),
-	})).(*model.PluginKeyValue)
+	})
+	require.Nil(t, err)

 	if result := <-ss.Plugin().Delete(kv.PluginId, kv.Key); result.Err != nil {
 		t.Fatal(result.Err)
@@ -119,17 +122,19 @@ func testPluginDelete(t *testing.T, ss store.Store) {
 func testPluginDeleteAll(t *testing.T, ss store.Store) {
 	pluginId := model.NewId()

-	kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
+	kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
 		PluginId: pluginId,
 		Key:      model.NewId(),
 		Value:    []byte(model.NewId()),
-	})).(*model.PluginKeyValue)
+	})
+	require.Nil(t, err)

-	kv2 := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
+	kv2, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
 		PluginId: pluginId,
 		Key:      model.NewId(),
 		Value:    []byte(model.NewId()),
-	})).(*model.PluginKeyValue)
+	})
+	require.Nil(t, err)

 	if result := <-ss.Plugin().DeleteAllForPlugin(pluginId); result.Err != nil {
 		t.Fatal(result.Err)
@@ -147,19 +152,21 @@ func testPluginDeleteAll(t *testing.T, ss store.Store) {
 func testPluginDeleteExpired(t *testing.T, ss store.Store) {
 	pluginId := model.NewId()

-	kv := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
+	kv, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
 		PluginId: pluginId,
 		Key:      model.NewId(),
 		Value:    []byte(model.NewId()),
 		ExpireAt: model.GetMillis() - 6000,
-	})).(*model.PluginKeyValue)
+	})
+	require.Nil(t, err)

-	kv2 := store.Must(ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
+	kv2, err := ss.Plugin().SaveOrUpdate(&model.PluginKeyValue{
 		PluginId: pluginId,
 		Key:      model.NewId(),
 		Value:    []byte(model.NewId()),
 		ExpireAt: 0,
-	})).(*model.PluginKeyValue)
+	})
+	require.Nil(t, err)

 	if result := <-ss.Plugin().DeleteAllExpired(); result.Err != nil {
 		t.Fatal(result.Err)