diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index 2bde3a45ee..3364376777 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -6,10 +6,12 @@ package app import ( "crypto/sha256" "encoding/base64" + "errors" "net/http" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store" ) func getKeyHash(key string) string { @@ -47,7 +49,13 @@ func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value []byte, updated, err := a.Srv().Store.Plugin().SetWithOptions(pluginId, key, value, options) if err != nil { mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return updated, err + var appErr *model.AppError + switch { + case errors.As(err, &appErr): + return false, appErr + default: + return false, model.NewAppError("SetPluginKeyWithOptions", "app.plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + } } // Clean up a previous entry using the hashed key, if it exists. @@ -67,7 +75,13 @@ func (a *App) CompareAndDeletePluginKey(pluginId string, key string, oldValue [] 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 + var appErr *model.AppError + switch { + case errors.As(err, &appErr): + return deleted, appErr + default: + return false, model.NewAppError("CompareAndDeletePluginKey", "app.plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) + } } // Clean up a previous entry using the hashed key, if it exists. @@ -81,17 +95,17 @@ func (a *App) CompareAndDeletePluginKey(pluginId string, key string, oldValue [] 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 - } else if err.StatusCode != http.StatusNotFound { + } else if nfErr := new(store.ErrNotFound); !errors.As(err, &nfErr) { mlog.Error("Failed to query plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return nil, err + return nil, model.NewAppError("GetPluginKey", "app.plugin_store.get.app_error", nil, err.Error(), http.StatusInternalServerError) } // Lookup using the hashed version of the key for keys written prior to v5.6. if kv, err := a.Srv().Store.Plugin().Get(pluginId, getKeyHash(key)); err == nil { return kv.Value, nil - } else if err.StatusCode != http.StatusNotFound { + } else if nfErr := new(store.ErrNotFound); !errors.As(err, &nfErr) { mlog.Error("Failed to query plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return nil, err + return nil, model.NewAppError("GetPluginKey", "app.plugin_store.get.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil, nil @@ -100,13 +114,13 @@ func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError func (a *App) DeletePluginKey(pluginId string, key string) *model.AppError { if err := a.Srv().Store.Plugin().Delete(pluginId, getKeyHash(key)); err != nil { mlog.Error("Failed to delete plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return err + return model.NewAppError("DeletePluginKey", "app.plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) } // Also delete the key without hashing if err := a.Srv().Store.Plugin().Delete(pluginId, key); err != nil { mlog.Error("Failed to delete plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return err + return model.NewAppError("DeletePluginKey", "app.plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil @@ -115,7 +129,7 @@ func (a *App) DeletePluginKey(pluginId string, key string) *model.AppError { func (a *App) DeleteAllKeysForPlugin(pluginId string) *model.AppError { if err := a.Srv().Store.Plugin().DeleteAllForPlugin(pluginId); err != nil { mlog.Error("Failed to delete all plugin key values", mlog.String("plugin_id", pluginId), mlog.Err(err)) - return err + return model.NewAppError("DeleteAllKeysForPlugin", "app.plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil @@ -128,7 +142,7 @@ func (a *App) DeleteAllExpiredPluginKeys() *model.AppError { if err := a.Srv().Store.Plugin().DeleteAllExpired(); err != nil { mlog.Error("Failed to delete all expired plugin key values", mlog.Err(err)) - return err + return model.NewAppError("DeleteAllExpiredPluginKeys", "app.plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil @@ -139,7 +153,7 @@ func (a *App) ListPluginKeys(pluginId string, page, perPage int) ([]string, *mod if err != nil { mlog.Error("Failed to list plugin key values", mlog.Int("page", page), mlog.Int("perPage", perPage), mlog.Err(err)) - return nil, err + return nil, model.NewAppError("ListPluginKeys", "app.plugin_store.list.app_error", nil, err.Error(), http.StatusInternalServerError) } return data, nil diff --git a/app/plugin_test.go b/app/plugin_test.go index 383fcbf7f0..8fc18a5c33 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -75,8 +75,8 @@ func TestPluginKeyValueStore(t *testing.T) { ExpireAt: 0, } - _, err = th.App.Srv().Store.Plugin().SaveOrUpdate(kv) - assert.Nil(t, err) + _, nErr := th.App.Srv().Store.Plugin().SaveOrUpdate(kv) + assert.Nil(t, nErr) // Test fetch by keyname (this key does not exist but hashed key will be used for lookup) ret, err = th.App.GetPluginKey(pluginId, "key2") diff --git a/i18n/en.json b/i18n/en.json index e5a61bd9e8..89df9c0c57 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4270,6 +4270,22 @@ "id": "app.plugin.write_file.saving.app_error", "translation": "An error occurred while saving the file." }, + { + "id": "app.plugin_store.delete.app_error", + "translation": "Could not delete plugin key value." + }, + { + "id": "app.plugin_store.get.app_error", + "translation": "Could not get plugin key value." + }, + { + "id": "app.plugin_store.list.app_error", + "translation": "Unable to list all the plugin keys." + }, + { + "id": "app.plugin_store.save.app_error", + "translation": "Could not save or update plugin key value." + }, { "id": "app.post.delete.app_error", "translation": "Unable to delete the post." @@ -7202,30 +7218,6 @@ "id": "store.sql_job.update.app_error", "translation": "Unable to update the job." }, - { - "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." - }, - { - "id": "store.sql_plugin_store.get.app_error", - "translation": "Could not get plugin key value." - }, - { - "id": "store.sql_plugin_store.list.app_error", - "translation": "Unable to list all the plugin keys." - }, - { - "id": "store.sql_plugin_store.save.app_error", - "translation": "Could not save or update plugin key value." - }, { "id": "store.sql_post.analytics_posts_count.app_error", "translation": "Unable to get post counts." diff --git a/store/layer_generators/main.go b/store/layer_generators/main.go index 944ace4d58..1bb6839510 100644 --- a/store/layer_generators/main.go +++ b/store/layer_generators/main.go @@ -247,7 +247,7 @@ func generateLayer(name, templateFile string) ([]byte, error) { "errorToBoolean": func(results []string) string { for _, typeName := range results { if isError(typeName) { - return fmt.Sprintf("err == nil") + return "err == nil" } } return "true" diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 5c064b2baf..dcbef9ddcb 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -4513,7 +4513,7 @@ func (s *OpenTracingLayerOAuthStore) UpdateApp(app *model.OAuthApp) (*model.OAut return result, err } -func (s *OpenTracingLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *OpenTracingLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.CompareAndDelete") s.Root.Store.SetContext(newCtx) @@ -4531,7 +4531,7 @@ func (s *OpenTracingLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyVa return result, err } -func (s *OpenTracingLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *OpenTracingLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.CompareAndSet") s.Root.Store.SetContext(newCtx) @@ -4549,7 +4549,7 @@ func (s *OpenTracingLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue return result, err } -func (s *OpenTracingLayerPluginStore) Delete(pluginId string, key string) *model.AppError { +func (s *OpenTracingLayerPluginStore) Delete(pluginId string, key string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.Delete") s.Root.Store.SetContext(newCtx) @@ -4567,7 +4567,7 @@ func (s *OpenTracingLayerPluginStore) Delete(pluginId string, key string) *model return err } -func (s *OpenTracingLayerPluginStore) DeleteAllExpired() *model.AppError { +func (s *OpenTracingLayerPluginStore) DeleteAllExpired() error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.DeleteAllExpired") s.Root.Store.SetContext(newCtx) @@ -4585,7 +4585,7 @@ func (s *OpenTracingLayerPluginStore) DeleteAllExpired() *model.AppError { return err } -func (s *OpenTracingLayerPluginStore) DeleteAllForPlugin(PluginId string) *model.AppError { +func (s *OpenTracingLayerPluginStore) DeleteAllForPlugin(PluginId string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.DeleteAllForPlugin") s.Root.Store.SetContext(newCtx) @@ -4603,7 +4603,7 @@ func (s *OpenTracingLayerPluginStore) DeleteAllForPlugin(PluginId string) *model return err } -func (s *OpenTracingLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, *model.AppError) { +func (s *OpenTracingLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.Get") s.Root.Store.SetContext(newCtx) @@ -4621,7 +4621,7 @@ func (s *OpenTracingLayerPluginStore) Get(pluginId string, key string) (*model.P return result, err } -func (s *OpenTracingLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, *model.AppError) { +func (s *OpenTracingLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.List") s.Root.Store.SetContext(newCtx) @@ -4639,7 +4639,7 @@ func (s *OpenTracingLayerPluginStore) List(pluginId string, page int, perPage in return result, err } -func (s *OpenTracingLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { +func (s *OpenTracingLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.SaveOrUpdate") s.Root.Store.SetContext(newCtx) @@ -4657,7 +4657,7 @@ func (s *OpenTracingLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) return result, err } -func (s *OpenTracingLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (s *OpenTracingLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PluginStore.SetWithOptions") s.Root.Store.SetContext(newCtx) diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index d493509db6..231f8cf707 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -3024,57 +3024,183 @@ func (s *RetryLayerOAuthStore) UpdateApp(app *model.OAuthApp) (*model.OAuthApp, } -func (s *RetryLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *RetryLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { - return s.PluginStore.CompareAndDelete(keyVal, oldValue) + tries := 0 + for { + result, err := s.PluginStore.CompareAndDelete(keyVal, oldValue) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } -func (s *RetryLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *RetryLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { - return s.PluginStore.CompareAndSet(keyVal, oldValue) + tries := 0 + for { + result, err := s.PluginStore.CompareAndSet(keyVal, oldValue) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } -func (s *RetryLayerPluginStore) Delete(pluginId string, key string) *model.AppError { +func (s *RetryLayerPluginStore) Delete(pluginId string, key string) error { - return s.PluginStore.Delete(pluginId, key) + tries := 0 + for { + err := s.PluginStore.Delete(pluginId, key) + if err == nil { + return err + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + } } -func (s *RetryLayerPluginStore) DeleteAllExpired() *model.AppError { +func (s *RetryLayerPluginStore) DeleteAllExpired() error { - return s.PluginStore.DeleteAllExpired() + tries := 0 + for { + err := s.PluginStore.DeleteAllExpired() + if err == nil { + return err + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + } } -func (s *RetryLayerPluginStore) DeleteAllForPlugin(PluginId string) *model.AppError { +func (s *RetryLayerPluginStore) DeleteAllForPlugin(PluginId string) error { - return s.PluginStore.DeleteAllForPlugin(PluginId) + tries := 0 + for { + err := s.PluginStore.DeleteAllForPlugin(PluginId) + if err == nil { + return err + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + } } -func (s *RetryLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, *model.AppError) { +func (s *RetryLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, error) { - return s.PluginStore.Get(pluginId, key) + tries := 0 + for { + result, err := s.PluginStore.Get(pluginId, key) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } -func (s *RetryLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, *model.AppError) { +func (s *RetryLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, error) { - return s.PluginStore.List(pluginId, page, perPage) + tries := 0 + for { + result, err := s.PluginStore.List(pluginId, page, perPage) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } -func (s *RetryLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { +func (s *RetryLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, error) { - return s.PluginStore.SaveOrUpdate(keyVal) + tries := 0 + for { + result, err := s.PluginStore.SaveOrUpdate(keyVal) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } -func (s *RetryLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (s *RetryLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, error) { - return s.PluginStore.SetWithOptions(pluginId, key, value, options) + tries := 0 + for { + result, err := s.PluginStore.SetWithOptions(pluginId, key, value, options) + if err == nil { + return result, err + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + } } diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index 1fe26a6de8..b9bef1d7b2 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -7,9 +7,9 @@ import ( "bytes" "database/sql" "fmt" - "net/http" sq "github.com/Masterminds/squirrel" + "github.com/pkg/errors" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" @@ -39,7 +39,7 @@ func newSqlPluginStore(sqlStore SqlStore) store.PluginStore { func (ps SqlPluginStore) createIndexesIfNotExists() { } -func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { +func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKeyValue, error) { if err := kv.IsValid(); err != nil { return nil, err } @@ -58,11 +58,11 @@ func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKe // 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) + return nil, errors.Wrap(err, "failed to update PluginKeyValue") } 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 { - return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusBadRequest) + return nil, errors.Wrap(err, "failed to save PluginKeyValue") } } } else if ps.DriverName() == model.DATABASE_DRIVER_MYSQL { @@ -74,18 +74,18 @@ func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKe queryString, args, err := query.ToSql() if err != nil { - return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "plugin_tosql") } if _, err := ps.GetMaster().Exec(queryString, args...); err != nil { - return nil, model.NewAppError("SqlPluginStore.SaveOrUpdate", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "failed to upsert PluginKeyValue") } } return kv, nil } -func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte) (bool, error) { if err := kv.IsValid(); err != nil { return false, err } @@ -106,11 +106,11 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte queryString, args, err := query.ToSql() if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "plugin_tosql") } if _, err := ps.GetMaster().Exec(queryString, args...); err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.delete.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "failed to delete PluginKeyValue") } // Insert if oldValue is nil @@ -121,7 +121,7 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte if IsUniqueConstraintError(err, []string{"PRIMARY", "PluginId", "Key", "PKey", "pkey"}) { return false, nil } else { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "failed to insert PluginKeyValue") } } } else { @@ -142,17 +142,17 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte queryString, args, err := query.ToSql() if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "plugin_tosql") } updateResult, err := ps.GetMaster().Exec(queryString, args...) if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "failed to update PluginKeyValue") } if rowsAffected, err := updateResult.RowsAffected(); err != nil { // Failed to update - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "unable to get rows affected") } 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. @@ -173,12 +173,12 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte queryString, args, err := query.ToSql() if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndSet", "store.sql.build_query.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", kv.PluginId, kv.Key, err.Error()), http.StatusInternalServerError) + return false, errors.Wrap(err, "plugin_tosql") } count, err := ps.GetReplica().SelectInt(queryString, args...) 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) + return false, errors.Wrapf(err, "failed to count PluginKeyValue with pluginId=%s and key=%s", kv.PluginId, kv.Key) } if count == 0 { @@ -186,7 +186,7 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte } 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) + return false, errors.Wrapf(err, "got too many rows when counting PluginKeyValue with pluginId=%s, key=%s, rows=%d", kv.PluginId, kv.Key, count) } } @@ -199,7 +199,7 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte return true, nil } -func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []byte) (bool, error) { if err := kv.IsValid(); err != nil { return false, err } @@ -221,16 +221,16 @@ func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []b queryString, args, err := query.ToSql() if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndDelete", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "plugin_tosql") } deleteResult, err := ps.GetMaster().Exec(queryString, args...) if err != nil { - return false, model.NewAppError("SqlPluginStore.CompareAndDelete", "store.sql_plugin_store.save.app_error", nil, err.Error(), http.StatusInternalServerError) + return false, errors.Wrap(err, "failed to delete PluginKeyValue") } 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) + return false, errors.Wrap(err, "unable to get rows affected") } else if rowsAffected == 0 { return false, nil } @@ -238,7 +238,7 @@ func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []b return true, nil } -func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value []byte, opt model.PluginKVSetOptions) (bool, *model.AppError) { +func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value []byte, opt model.PluginKVSetOptions) (bool, error) { if err := opt.IsValid(); err != nil { return false, err } @@ -252,27 +252,16 @@ func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value []byt return ps.CompareAndSet(kv, opt.OldValue) } - savedKv, err := ps.SaveOrUpdate(kv) - if err != nil { - return false, err + savedKv, nErr := ps.SaveOrUpdate(kv) + if nErr != nil { + return false, nErr } return savedKv != nil, nil } -func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) { +func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, error) { currentTime := model.GetMillis() - - failure := func(err error, statusCode int) *model.AppError { - return model.NewAppError( - "SqlPluginStore.Get", - "store.sql_plugin_store.get.app_error", - nil, - fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), - statusCode, - ) - } - query := ps.getQueryBuilder().Select("PluginId, PKey, PValue, ExpireAt"). From("PluginKeyValueStore"). Where(sq.Eq{"PluginId": pluginId}). @@ -280,21 +269,22 @@ func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *mode Where(sq.Or{sq.Eq{"ExpireAt": 0}, sq.Gt{"ExpireAt": currentTime}}) queryString, args, err := query.ToSql() if err != nil { - return nil, failure(err, http.StatusInternalServerError) + return nil, errors.Wrap(err, "plugin_tosql") } + row := ps.GetReplica().Db.QueryRow(queryString, args...) var kv model.PluginKeyValue if err := row.Scan(&kv.PluginId, &kv.Key, &kv.Value, &kv.ExpireAt); err != nil { if err == sql.ErrNoRows { - return nil, failure(err, http.StatusNotFound) + return nil, store.NewErrNotFound("PluginKeyValue", fmt.Sprintf("pluginId=%s, key=%s", pluginId, key)) } - return nil, failure(err, http.StatusInternalServerError) + return nil, errors.Wrapf(err, "failed to get PluginKeyValue with pluginId=%s and key=%s", pluginId, key) } return &kv, nil } -func (ps SqlPluginStore) Delete(pluginId, key string) *model.AppError { +func (ps SqlPluginStore) Delete(pluginId, key string) error { query := ps.getQueryBuilder(). Delete("PluginKeyValueStore"). Where(sq.Eq{"PluginId": pluginId}). @@ -302,34 +292,33 @@ func (ps SqlPluginStore) Delete(pluginId, key string) *model.AppError { queryString, args, err := query.ToSql() if err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql.build_query.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusInternalServerError) + return errors.Wrap(err, "plugin_tosql") } if _, err := ps.GetMaster().Exec(queryString, args...); err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql_plugin_store.delete.app_error", nil, fmt.Sprintf("plugin_id=%v, key=%v, err=%v", pluginId, key, err.Error()), http.StatusInternalServerError) + return errors.Wrapf(err, "failed to delete PluginKeyValue with pluginId=%s and key=%s", pluginId, key) } return nil } -func (ps SqlPluginStore) DeleteAllForPlugin(pluginId string) *model.AppError { +func (ps SqlPluginStore) DeleteAllForPlugin(pluginId string) error { query := ps.getQueryBuilder(). Delete("PluginKeyValueStore"). Where(sq.Eq{"PluginId": pluginId}) queryString, args, err := query.ToSql() if err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql.build_query.app_error", nil, fmt.Sprintf("plugin_id=%v, err=%v", pluginId, err.Error()), http.StatusInternalServerError) + return errors.Wrap(err, "plugin_tosql") } if _, err := ps.GetMaster().Exec(queryString, args...); err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql_plugin_store.delete.app_error", nil, fmt.Sprintf("plugin_id=%v, err=%v", pluginId, err.Error()), http.StatusInternalServerError) + return errors.Wrapf(err, "failed to get all PluginKeyValues with pluginId=%s ", pluginId) } return nil } -func (ps SqlPluginStore) DeleteAllExpired() *model.AppError { +func (ps SqlPluginStore) DeleteAllExpired() error { currentTime := model.GetMillis() - query := ps.getQueryBuilder(). Delete("PluginKeyValueStore"). Where(sq.NotEq{"ExpireAt": 0}). @@ -337,16 +326,16 @@ func (ps SqlPluginStore) DeleteAllExpired() *model.AppError { queryString, args, err := query.ToSql() if err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql.build_query.app_error", nil, fmt.Sprintf("current_time=%v, err=%v", currentTime, err.Error()), http.StatusInternalServerError) + return errors.Wrap(err, "plugin_tosql") } if _, err := ps.GetMaster().Exec(queryString, args...); err != nil { - return model.NewAppError("SqlPluginStore.Delete", "store.sql_plugin_store.delete.app_error", nil, fmt.Sprintf("current_time=%v, err=%v", currentTime, err.Error()), http.StatusInternalServerError) + return errors.Wrap(err, "failed to delete all expired PluginKeyValues") } return nil } -func (ps SqlPluginStore) List(pluginId string, offset int, limit int) ([]string, *model.AppError) { +func (ps SqlPluginStore) List(pluginId string, offset int, limit int) ([]string, error) { if limit <= 0 { limit = defaultPluginKeyFetchLimit } @@ -371,12 +360,12 @@ func (ps SqlPluginStore) List(pluginId string, offset int, limit int) ([]string, queryString, args, err := query.ToSql() if err != nil { - return nil, model.NewAppError("SqlPluginStore.List", "store.sql.build_query.app_error", nil, fmt.Sprintf("plugin_id=%v, err=%v", pluginId, err.Error()), http.StatusInternalServerError) + return nil, errors.Wrap(err, "plugin_tosql") } _, err = ps.GetReplica().Select(&keys, queryString, args...) if err != nil { - return nil, model.NewAppError("SqlPluginStore.List", "store.sql_plugin_store.list.app_error", nil, fmt.Sprintf("plugin_id=%v, err=%v", pluginId, err.Error()), http.StatusInternalServerError) + return nil, errors.Wrapf(err, "failed to get PluginKeyValues with pluginId=%s", pluginId) } return keys, nil diff --git a/store/store.go b/store/store.go index 5e254e2070..f95027b14f 100644 --- a/store/store.go +++ b/store/store.go @@ -587,15 +587,15 @@ 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) - SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) - Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) - Delete(pluginId, key string) *model.AppError - DeleteAllForPlugin(PluginId string) *model.AppError - DeleteAllExpired() *model.AppError - List(pluginId string, page, perPage int) ([]string, *model.AppError) + SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, error) + CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) + CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) + SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, error) + Get(pluginId, key string) (*model.PluginKeyValue, error) + Delete(pluginId, key string) error + DeleteAllForPlugin(PluginId string) error + DeleteAllExpired() error + List(pluginId string, page, perPage int) ([]string, error) } type RoleStore interface { diff --git a/store/storetest/mocks/PluginStore.go b/store/storetest/mocks/PluginStore.go index f441c92f3a..14c564d687 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -15,7 +15,7 @@ type PluginStore struct { } // CompareAndDelete provides a mock function with given fields: keyVal, oldValue -func (_m *PluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (_m *PluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { ret := _m.Called(keyVal, oldValue) var r0 bool @@ -25,20 +25,18 @@ func (_m *PluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue [ r0 = ret.Get(0).(bool) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.PluginKeyValue, []byte) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.PluginKeyValue, []byte) error); ok { r1 = rf(keyVal, oldValue) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } 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) { +func (_m *PluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { ret := _m.Called(keyVal, oldValue) var r0 bool @@ -48,68 +46,60 @@ func (_m *PluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []by r0 = ret.Get(0).(bool) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.PluginKeyValue, []byte) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.PluginKeyValue, []byte) error); ok { r1 = rf(keyVal, oldValue) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // Delete provides a mock function with given fields: pluginId, key -func (_m *PluginStore) Delete(pluginId string, key string) *model.AppError { +func (_m *PluginStore) Delete(pluginId string, key string) error { ret := _m.Called(pluginId, key) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string, string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { r0 = rf(pluginId, key) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // DeleteAllExpired provides a mock function with given fields: -func (_m *PluginStore) DeleteAllExpired() *model.AppError { +func (_m *PluginStore) DeleteAllExpired() error { ret := _m.Called() - var r0 *model.AppError - if rf, ok := ret.Get(0).(func() *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { r0 = rf() } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // DeleteAllForPlugin provides a mock function with given fields: PluginId -func (_m *PluginStore) DeleteAllForPlugin(PluginId string) *model.AppError { +func (_m *PluginStore) DeleteAllForPlugin(PluginId string) error { ret := _m.Called(PluginId) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { r0 = rf(PluginId) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // Get provides a mock function with given fields: pluginId, key -func (_m *PluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, *model.AppError) { +func (_m *PluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, error) { ret := _m.Called(pluginId, key) var r0 *model.PluginKeyValue @@ -121,20 +111,18 @@ func (_m *PluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { r1 = rf(pluginId, key) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // List provides a mock function with given fields: pluginId, page, perPage -func (_m *PluginStore) List(pluginId string, page int, perPage int) ([]string, *model.AppError) { +func (_m *PluginStore) List(pluginId string, page int, perPage int) ([]string, error) { ret := _m.Called(pluginId, page, perPage) var r0 []string @@ -146,20 +134,18 @@ func (_m *PluginStore) List(pluginId string, page int, perPage int) ([]string, * } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, int, int) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, int, int) error); ok { r1 = rf(pluginId, page, perPage) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // SaveOrUpdate provides a mock function with given fields: keyVal -func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { +func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, error) { ret := _m.Called(keyVal) var r0 *model.PluginKeyValue @@ -171,20 +157,18 @@ func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.Plugin } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.PluginKeyValue) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.PluginKeyValue) error); ok { r1 = rf(keyVal) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // SetWithOptions provides a mock function with given fields: pluginId, key, value, options -func (_m *PluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (_m *PluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, error) { ret := _m.Called(pluginId, key, value, options) var r0 bool @@ -194,13 +178,11 @@ func (_m *PluginStore) SetWithOptions(pluginId string, key string, value []byte, r0 = ret.Get(0).(bool) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, string, []byte, model.PluginKVSetOptions) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, string, []byte, model.PluginKVSetOptions) error); ok { r1 = rf(pluginId, key, value, options) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 diff --git a/store/storetest/plugin_store.go b/store/storetest/plugin_store.go index 9c019a0c45..eb9e6b5f2d 100644 --- a/store/storetest/plugin_store.go +++ b/store/storetest/plugin_store.go @@ -4,7 +4,6 @@ package storetest import ( - "net/http" "sort" "testing" @@ -64,7 +63,7 @@ func setupKVs(t *testing.T, ss store.Store) (string, func()) { } } -func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer func(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError)) { +func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer func(kv *model.PluginKeyValue) (*model.PluginKeyValue, error)) { t.Run("invalid kv", func(t *testing.T) { _, tearDown := setupKVs(t, ss) defer tearDown() @@ -78,7 +77,9 @@ func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer kv, err := doer(kv) require.NotNil(t, err) - require.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", err.Id) + appErr, ok := err.(*model.AppError) + require.True(t, ok) + require.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", appErr.Id) assert.Nil(t, kv) }) @@ -107,8 +108,8 @@ func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer assert.Equal(t, []byte(value), kv.Value) assert.Equal(t, expireAt, kv.ExpireAt) - actualKV, err := ss.Plugin().Get(pluginId, key) - require.Nil(t, err) + actualKV, nErr := ss.Plugin().Get(pluginId, key) + require.Nil(t, nErr) assert.Equal(t, kv, actualKV) }) @@ -137,9 +138,10 @@ func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer assert.Nil(t, kv.Value) assert.Equal(t, expireAt, kv.ExpireAt) - actualKV, err := ss.Plugin().Get(pluginId, key) - require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + actualKV, nErr := ss.Plugin().Get(pluginId, key) + _, ok := nErr.(*store.ErrNotFound) + require.NotNil(t, nErr) + assert.True(t, ok) assert.Nil(t, actualKV) }) @@ -174,8 +176,8 @@ func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer assert.Equal(t, []byte(newValue), kv.Value) assert.Equal(t, expireAt, kv.ExpireAt) - actualKV, err := ss.Plugin().Get(pluginId, key) - require.Nil(t, err) + actualKV, nErr := ss.Plugin().Get(pluginId, key) + require.Nil(t, nErr) assert.Equal(t, kv, actualKV) }) @@ -209,22 +211,23 @@ func doTestPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier, doer assert.Nil(t, kv.Value) assert.Equal(t, expireAt, kv.ExpireAt) - actualKV, err := ss.Plugin().Get(pluginId, key) - require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + actualKV, nErr := ss.Plugin().Get(pluginId, key) + _, ok := nErr.(*store.ErrNotFound) + require.NotNil(t, nErr) + assert.True(t, ok) assert.Nil(t, actualKV) }) } func testPluginSaveOrUpdate(t *testing.T, ss store.Store, s SqlSupplier) { - doTestPluginSaveOrUpdate(t, ss, s, func(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { + doTestPluginSaveOrUpdate(t, ss, s, func(kv *model.PluginKeyValue) (*model.PluginKeyValue, error) { return ss.Plugin().SaveOrUpdate(kv) }) } // doTestPluginCompareAndSet exercises the CompareAndSet functionality, but abstracts the actual // call to same to allow reuse with SetWithOptions -func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, compareAndSet func(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError)) { +func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, compareAndSet func(kv *model.PluginKeyValue, oldValue []byte) (bool, error)) { t.Run("invalid kv", func(t *testing.T) { _, tearDown := setupKVs(t, ss) defer tearDown() @@ -238,8 +241,10 @@ func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, comp ok, err := compareAndSet(kv, nil) require.NotNil(t, err) - assert.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", err.Id) assert.False(t, ok) + appErr, ok := err.(*model.AppError) + require.True(t, ok) + assert.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", appErr.Id) }) // assertChanged verifies that CompareAndSet successfully changes to the given value. @@ -250,8 +255,8 @@ func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, comp require.Nil(t, err) require.True(t, ok, "should have succeeded to CompareAndSet") - actualKV, err := ss.Plugin().Get(kv.PluginId, kv.Key) - require.Nil(t, err) + actualKV, nErr := ss.Plugin().Get(kv.PluginId, kv.Key) + require.Nil(t, nErr) // When tested with KVSetWithOptions, a strict comparison can fail because that // function accepts a relative time and makes its own call to model.GetMillis(), @@ -275,13 +280,14 @@ func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, comp require.Nil(t, err) require.False(t, ok, "should have failed to CompareAndSet") - actualKV, err := ss.Plugin().Get(kv.PluginId, kv.Key) + actualKV, nErr := ss.Plugin().Get(kv.PluginId, kv.Key) if existingKV == nil { - require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + require.NotNil(t, nErr) + _, ok := nErr.(*store.ErrNotFound) + assert.True(t, ok) assert.Nil(t, actualKV) } else { - require.Nil(t, err) + require.Nil(t, nErr) assert.Equal(t, existingKV, actualKV) } } @@ -294,9 +300,10 @@ func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, comp require.Nil(t, err) require.True(t, ok, "should have succeeded to CompareAndSet") - actualKV, err := ss.Plugin().Get(kv.PluginId, kv.Key) - require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + actualKV, nErr := ss.Plugin().Get(kv.PluginId, kv.Key) + _, ok = nErr.(*store.ErrNotFound) + require.NotNil(t, nErr) + assert.True(t, ok) assert.Nil(t, actualKV) } @@ -518,7 +525,7 @@ func doTestPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier, comp } func testPluginCompareAndSet(t *testing.T, ss store.Store, s SqlSupplier) { - doTestPluginCompareAndSet(t, ss, s, func(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { + doTestPluginCompareAndSet(t, ss, s, func(kv *model.PluginKeyValue, oldValue []byte) (bool, error) { return ss.Plugin().CompareAndSet(kv, oldValue) }) } @@ -537,8 +544,10 @@ func testPluginCompareAndDelete(t *testing.T, ss store.Store, s SqlSupplier) { ok, err := ss.Plugin().CompareAndDelete(kv, nil) require.NotNil(t, err) - assert.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", err.Id) assert.False(t, ok) + appErr, ok := err.(*model.AppError) + require.True(t, ok) + assert.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", appErr.Id) }) t.Run("non-existent key should fail", func(t *testing.T) { @@ -666,8 +675,10 @@ func testPluginSetWithOptions(t *testing.T, ss store.Store, s SqlSupplier) { ok, err := ss.Plugin().SetWithOptions(pluginId, key, []byte(value), options) require.NotNil(t, err) - require.Equal(t, "model.plugin_kvset_options.is_valid.old_value.app_error", err.Id) assert.False(t, ok) + appErr, ok := err.(*model.AppError) + require.True(t, ok) + require.Equal(t, "model.plugin_kvset_options.is_valid.old_value.app_error", appErr.Id) }) t.Run("invalid kv", func(t *testing.T) { @@ -681,12 +692,14 @@ func testPluginSetWithOptions(t *testing.T, ss store.Store, s SqlSupplier) { ok, err := ss.Plugin().SetWithOptions(pluginId, key, []byte(value), options) require.NotNil(t, err) - require.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", err.Id) assert.False(t, ok) + appErr, ok := err.(*model.AppError) + require.True(t, ok) + require.Equal(t, "model.plugin_key_value.is_valid.plugin_id.app_error", appErr.Id) }) t.Run("atomic", func(t *testing.T) { - doTestPluginCompareAndSet(t, ss, s, func(kv *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { + doTestPluginCompareAndSet(t, ss, s, func(kv *model.PluginKeyValue, oldValue []byte) (bool, error) { now := model.GetMillis() options := model.PluginKVSetOptions{ Atomic: true, @@ -702,7 +715,7 @@ func testPluginSetWithOptions(t *testing.T, ss store.Store, s SqlSupplier) { }) t.Run("non-atomic", func(t *testing.T) { - doTestPluginSaveOrUpdate(t, ss, s, func(kv *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { + doTestPluginSaveOrUpdate(t, ss, s, func(kv *model.PluginKeyValue) (*model.PluginKeyValue, error) { now := model.GetMillis() options := model.PluginKVSetOptions{ Atomic: false, @@ -712,11 +725,11 @@ func testPluginSetWithOptions(t *testing.T, ss store.Store, s SqlSupplier) { options.ExpireInSeconds = (kv.ExpireAt - now) / 1000 } - ok, appErr := ss.Plugin().SetWithOptions(kv.PluginId, kv.Key, kv.Value, options) + ok, err := ss.Plugin().SetWithOptions(kv.PluginId, kv.Key, kv.Value, options) if !ok { - return nil, appErr + return nil, err } else { - return kv, appErr + return kv, err } }) }) @@ -727,9 +740,10 @@ func testPluginGet(t *testing.T, ss store.Store) { pluginId := model.NewId() key := model.NewId() - kv, err := ss.Plugin().Get(pluginId, key) - require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + kv, nErr := ss.Plugin().Get(pluginId, key) + _, ok := nErr.(*store.ErrNotFound) + require.NotNil(t, nErr) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -750,8 +764,9 @@ func testPluginGet(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err = ss.Plugin().Get(model.NewId(), key) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -772,8 +787,9 @@ func testPluginGet(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err = ss.Plugin().Get(pluginId, model.NewId()) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -794,8 +810,9 @@ func testPluginGet(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err = ss.Plugin().Get(pluginId, model.NewId()) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -816,8 +833,9 @@ func testPluginGet(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err = ss.Plugin().Get(pluginId, model.NewId()) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -875,8 +893,9 @@ func testPluginDelete(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err := ss.Plugin().Get(pluginId, key) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) @@ -921,8 +940,9 @@ func testPluginDelete(t *testing.T, ss store.Store) { require.Nil(t, err) kv, err = ss.Plugin().Get(pluginId, key) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, kv) }) } @@ -985,12 +1005,14 @@ func testPluginDeleteAllForPlugin(t *testing.T, ss store.Store) { require.Nil(t, err) _, err = ss.Plugin().Get(kv.PluginId, kv.Key) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) _, err = ss.Plugin().Get(kv.PluginId, kv2.Key) + _, ok = err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) }) } @@ -1168,8 +1190,9 @@ func testPluginDeleteAllExpired(t *testing.T, ss store.Store) { assert.Equal(t, kvA1, actualKVA1) actualKVA2, err := ss.Plugin().Get(pluginIdA, expiredKVA2.Key) + _, ok := err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, actualKVA2) actualKVB1, err := ss.Plugin().Get(pluginIdB, kvB1.Key) @@ -1177,8 +1200,9 @@ func testPluginDeleteAllExpired(t *testing.T, ss store.Store) { assert.Equal(t, kvB1, actualKVB1) actualKVB2, err := ss.Plugin().Get(pluginIdB, expiredKVB2.Key) + _, ok = err.(*store.ErrNotFound) require.NotNil(t, err) - assert.Equal(t, err.StatusCode, http.StatusNotFound) + assert.True(t, ok) assert.Nil(t, actualKVB2) }) } diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index c894a8c37e..7c21fb391e 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -4095,7 +4095,7 @@ func (s *TimerLayerOAuthStore) UpdateApp(app *model.OAuthApp) (*model.OAuthApp, return result, err } -func (s *TimerLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *TimerLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { start := timemodule.Now() result, err := s.PluginStore.CompareAndDelete(keyVal, oldValue) @@ -4111,7 +4111,7 @@ func (s *TimerLayerPluginStore) CompareAndDelete(keyVal *model.PluginKeyValue, o return result, err } -func (s *TimerLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) { +func (s *TimerLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, error) { start := timemodule.Now() result, err := s.PluginStore.CompareAndSet(keyVal, oldValue) @@ -4127,7 +4127,7 @@ func (s *TimerLayerPluginStore) CompareAndSet(keyVal *model.PluginKeyValue, oldV return result, err } -func (s *TimerLayerPluginStore) Delete(pluginId string, key string) *model.AppError { +func (s *TimerLayerPluginStore) Delete(pluginId string, key string) error { start := timemodule.Now() err := s.PluginStore.Delete(pluginId, key) @@ -4143,7 +4143,7 @@ func (s *TimerLayerPluginStore) Delete(pluginId string, key string) *model.AppEr return err } -func (s *TimerLayerPluginStore) DeleteAllExpired() *model.AppError { +func (s *TimerLayerPluginStore) DeleteAllExpired() error { start := timemodule.Now() err := s.PluginStore.DeleteAllExpired() @@ -4159,7 +4159,7 @@ func (s *TimerLayerPluginStore) DeleteAllExpired() *model.AppError { return err } -func (s *TimerLayerPluginStore) DeleteAllForPlugin(PluginId string) *model.AppError { +func (s *TimerLayerPluginStore) DeleteAllForPlugin(PluginId string) error { start := timemodule.Now() err := s.PluginStore.DeleteAllForPlugin(PluginId) @@ -4175,7 +4175,7 @@ func (s *TimerLayerPluginStore) DeleteAllForPlugin(PluginId string) *model.AppEr return err } -func (s *TimerLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, *model.AppError) { +func (s *TimerLayerPluginStore) Get(pluginId string, key string) (*model.PluginKeyValue, error) { start := timemodule.Now() result, err := s.PluginStore.Get(pluginId, key) @@ -4191,7 +4191,7 @@ func (s *TimerLayerPluginStore) Get(pluginId string, key string) (*model.PluginK return result, err } -func (s *TimerLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, *model.AppError) { +func (s *TimerLayerPluginStore) List(pluginId string, page int, perPage int) ([]string, error) { start := timemodule.Now() result, err := s.PluginStore.List(pluginId, page, perPage) @@ -4207,7 +4207,7 @@ func (s *TimerLayerPluginStore) List(pluginId string, page int, perPage int) ([] return result, err } -func (s *TimerLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) { +func (s *TimerLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, error) { start := timemodule.Now() result, err := s.PluginStore.SaveOrUpdate(keyVal) @@ -4223,7 +4223,7 @@ func (s *TimerLayerPluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*mod return result, err } -func (s *TimerLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, *model.AppError) { +func (s *TimerLayerPluginStore) SetWithOptions(pluginId string, key string, value []byte, options model.PluginKVSetOptions) (bool, error) { start := timemodule.Now() result, err := s.PluginStore.SetWithOptions(pluginId, key, value, options)