From 4ba6c3581305e192ac31ef8b88bca0d0aa61bfd7 Mon Sep 17 00:00:00 2001 From: Joshua Bezaleel Abednego Date: Wed, 15 Jul 2020 23:52:35 +0700 Subject: [PATCH] [MM-24664] Plugin_store queries squirrel refactor (#14523) * Use squirrel to build query for plugin store * Typo of PluginkeyValueStore to PluginKeyValueStore * wrong parameter of queryBuilder in Get method * Use casting to int for comparison on ExpireAt * Revert query for CompareAndSet and CompareAndDelete temporarily * Delete query of expired value * Update query when oldValue is not nil * Check count query when there is no row affected * Delete query on CompareAndDelete * Put squirrel in separate import group Co-authored-by: Mattermod Co-authored-by: Agniva De Sarker --- store/sqlstore/plugin_store.go | 168 +++++++++++++++++++++++++-------- 1 file changed, 128 insertions(+), 40 deletions(-) diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index aec1793d93..1fe26a6de8 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -66,7 +66,18 @@ func (ps SqlPluginStore) SaveOrUpdate(kv *model.PluginKeyValue) (*model.PluginKe } } } 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 { + query := ps.getQueryBuilder(). + Insert("PluginKeyValueStore"). + Columns("PluginId", "PKey", "PValue", "ExpireAt"). + Values(kv.PluginId, kv.Key, kv.Value, kv.ExpireAt). + SuffixExpr(sq.Expr("ON DUPLICATE KEY UPDATE PValue = ?, ExpireAt = ?", kv.Value, kv.ExpireAt)) + + 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) + } + + 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) } } @@ -86,12 +97,19 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte if oldValue == nil { // Delete any existing, expired value. - if _, err := ps.GetMaster().Exec("DELETE FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND ExpireAt != 0 AND ExpireAt < :CurrentTime", - map[string]interface{}{ - "PluginId": kv.PluginId, - "Key": kv.Key, - "CurrentTime": model.GetMillis(), - }); err != nil { + query := ps.getQueryBuilder(). + Delete("PluginKeyValueStore"). + Where(sq.Eq{"PluginId": kv.PluginId}). + Where(sq.Eq{"PKey": kv.Key}). + Where(sq.NotEq{"ExpireAt": int(0)}). + Where(sq.Lt{"ExpireAt": model.GetMillis()}) + + 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) + } + + 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) } @@ -110,17 +128,24 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte currentTime := model.GetMillis() // Update if oldValue is not nil - updateResult, err := ps.GetMaster().Exec( - `UPDATE PluginKeyValueStore SET PValue = :New, ExpireAt = :ExpireAt WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Old AND (ExpireAt = 0 OR ExpireAt > :CurrentTime)`, - map[string]interface{}{ - "PluginId": kv.PluginId, - "Key": kv.Key, - "Old": oldValue, - "New": kv.Value, - "ExpireAt": kv.ExpireAt, - "CurrentTime": currentTime, - }, - ) + query := ps.getQueryBuilder(). + Update("PluginKeyValueStore"). + Set("PValue", kv.Value). + Set("ExpireAt", kv.ExpireAt). + Where(sq.Eq{"PluginId": kv.PluginId}). + Where(sq.Eq{"PKey": kv.Key}). + Where(sq.Eq{"PValue": oldValue}). + Where(sq.Or{ + sq.Eq{"ExpireAt": int(0)}, + sq.Gt{"ExpireAt": currentTime}, + }) + + 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) + } + + 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) } @@ -135,15 +160,23 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte // 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 AND (ExpireAt = 0 OR ExpireAt > :CurrentTime)", - map[string]interface{}{ - "PluginId": kv.PluginId, - "Key": kv.Key, - "Value": kv.Value, - "CurrentTime": currentTime, - }, - ) + query := ps.getQueryBuilder(). + Select("COUNT(*)"). + From("PluginKeyValueStore"). + Where(sq.Eq{"PluginId": kv.PluginId}). + Where(sq.Eq{"PKey": kv.Key}). + Where(sq.Eq{"PValue": kv.Value}). + Where(sq.Or{ + sq.Eq{"ExpireAt": int(0)}, + sq.Gt{"ExpireAt": currentTime}, + }) + + 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) + } + + 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) } @@ -176,15 +209,22 @@ func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []b return false, nil } - deleteResult, err := ps.GetMaster().Exec( - `DELETE FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Old AND (ExpireAt = 0 OR ExpireAt > :CurrentTime)`, - map[string]interface{}{ - "PluginId": kv.PluginId, - "Key": kv.Key, - "Old": oldValue, - "CurrentTime": model.GetMillis(), - }, - ) + query := ps.getQueryBuilder(). + Delete("PluginKeyValueStore"). + Where(sq.Eq{"PluginId": kv.PluginId}). + Where(sq.Eq{"PKey": kv.Key}). + Where(sq.Eq{"PValue": oldValue}). + Where(sq.Or{ + sq.Eq{"ExpireAt": int(0)}, + sq.Gt{"ExpireAt": model.GetMillis()}, + }) + + 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) + } + + 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) } @@ -255,14 +295,33 @@ func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *mode } func (ps SqlPluginStore) Delete(pluginId, key string) *model.AppError { - if _, err := ps.GetMaster().Exec("DELETE FROM PluginKeyValueStore WHERE PluginId = :PluginId AND PKey = :Key", map[string]interface{}{"PluginId": pluginId, "Key": key}); err != nil { + query := ps.getQueryBuilder(). + Delete("PluginKeyValueStore"). + Where(sq.Eq{"PluginId": pluginId}). + Where(sq.Eq{"Pkey": key}) + + 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) + } + + 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 nil } func (ps SqlPluginStore) DeleteAllForPlugin(pluginId string) *model.AppError { - if _, err := ps.GetMaster().Exec("DELETE FROM PluginKeyValueStore WHERE PluginId = :PluginId", map[string]interface{}{"PluginId": pluginId}); err != nil { + 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) + } + + 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 nil @@ -270,7 +329,18 @@ func (ps SqlPluginStore) DeleteAllForPlugin(pluginId string) *model.AppError { func (ps SqlPluginStore) DeleteAllExpired() *model.AppError { currentTime := model.GetMillis() - if _, err := ps.GetMaster().Exec("DELETE FROM PluginKeyValueStore WHERE ExpireAt != 0 AND ExpireAt < :CurrentTime", map[string]interface{}{"CurrentTime": currentTime}); err != nil { + + query := ps.getQueryBuilder(). + Delete("PluginKeyValueStore"). + Where(sq.NotEq{"ExpireAt": 0}). + Where(sq.Lt{"ExpireAt": currentTime}) + + 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) + } + + 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 nil @@ -286,7 +356,25 @@ func (ps SqlPluginStore) List(pluginId string, offset int, limit int) ([]string, } var keys []string - _, err := ps.GetReplica().Select(&keys, "SELECT PKey FROM PluginKeyValueStore WHERE PluginId = :PluginId AND (ExpireAt = 0 OR ExpireAt > :CurrentTime) order by PKey limit :Limit offset :Offset", map[string]interface{}{"PluginId": pluginId, "Limit": limit, "Offset": offset, "CurrentTime": model.GetMillis()}) + + query := ps.getQueryBuilder(). + Select("Pkey"). + From("PluginKeyValueStore"). + Where(sq.Eq{"PluginId": pluginId}). + Where(sq.Or{ + sq.Eq{"ExpireAt": int(0)}, + sq.Gt{"ExpireAt": model.GetMillis()}, + }). + OrderBy("PKey"). + Limit(uint64(limit)). + Offset(uint64(offset)) + + 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) + } + + _, 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) }