[MM-62149] Avoid SELECT * in emoji_store.go (#30082)

* refractored select sql queries

* rm unused makeStringArgs

* linting

* leverage builder pattern

---------

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
This commit is contained in:
Arya Khochare 2025-02-10 21:32:50 +05:30 committed by GitHub
parent 9b87970c99
commit f6c4bdf365
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 32 additions and 53 deletions

View File

@ -6,7 +6,6 @@ package sqlstore
import ( import (
"database/sql" "database/sql"
"fmt" "fmt"
"strings"
"github.com/pkg/errors" "github.com/pkg/errors"
@ -14,17 +13,26 @@ import (
"github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/public/shared/request"
"github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/channels/store"
"github.com/mattermost/mattermost/server/v8/einterfaces" "github.com/mattermost/mattermost/server/v8/einterfaces"
sq "github.com/mattermost/squirrel"
) )
type SqlEmojiStore struct { type SqlEmojiStore struct {
*SqlStore *SqlStore
metrics einterfaces.MetricsInterface metrics einterfaces.MetricsInterface
emojiSelectQuery sq.SelectBuilder
} }
func newSqlEmojiStore(sqlStore *SqlStore, metrics einterfaces.MetricsInterface) store.EmojiStore { func newSqlEmojiStore(sqlStore *SqlStore, metrics einterfaces.MetricsInterface) store.EmojiStore {
emojiSelectQuery := sqlStore.getQueryBuilder().
Select("Id", "CreateAt", "UpdateAt", "DeleteAt", "CreatorId", "Name").
From("Emoji").
Where(sq.Eq{"DeleteAt": 0})
return &SqlEmojiStore{ return &SqlEmojiStore{
SqlStore: sqlStore, SqlStore: sqlStore,
metrics: metrics, metrics: metrics,
emojiSelectQuery: emojiSelectQuery,
} }
} }
@ -53,36 +61,27 @@ func (es SqlEmojiStore) GetByName(c request.CTX, name string, allowFromCache boo
} }
func (es SqlEmojiStore) GetMultipleByName(c request.CTX, names []string) ([]*model.Emoji, error) { func (es SqlEmojiStore) GetMultipleByName(c request.CTX, names []string) ([]*model.Emoji, error) {
// Creating (?, ?, ?) len(names) number of times. query := es.emojiSelectQuery.Where(sq.Eq{"Name": names})
keys := strings.Join(strings.Fields(strings.Repeat("? ", len(names))), ",")
args := makeStringArgs(names)
emojis := []*model.Emoji{} var emojis []*model.Emoji
if err := es.DBXFromContext(c.Context()).Select(&emojis, if err := es.DBXFromContext(c.Context()).SelectBuilder(&emojis, query); err != nil {
`SELECT return nil, errors.Wrapf(err, "error getting emojis by names %v", names)
*
FROM
Emoji
WHERE
Name IN (`+keys+`)
AND DeleteAt = 0`, args...); err != nil {
return nil, errors.Wrapf(err, "error getting emoji by names %v", names)
} }
return emojis, nil return emojis, nil
} }
func (es SqlEmojiStore) GetList(offset, limit int, sort string) ([]*model.Emoji, error) { func (es SqlEmojiStore) GetList(offset, limit int, sort string) ([]*model.Emoji, error) {
emojis := []*model.Emoji{} var emojis []*model.Emoji
query := "SELECT * FROM Emoji WHERE DeleteAt = 0"
query := es.emojiSelectQuery
if sort == model.EmojiSortByName { if sort == model.EmojiSortByName {
query += " ORDER BY Name" query = query.OrderBy("Name")
} }
query += " LIMIT ? OFFSET ?" query = query.Limit(uint64(limit)).Offset(uint64(offset))
if err := es.GetReplica().Select(&emojis, query, limit, offset); err != nil { if err := es.GetReplica().SelectBuilder(&emojis, query); err != nil {
return nil, errors.Wrap(err, "could not get list of emojis") return nil, errors.Wrap(err, "could not get list of emojis")
} }
return emojis, nil return emojis, nil
@ -107,7 +106,7 @@ func (es SqlEmojiStore) Delete(emoji *model.Emoji, time int64) error {
} }
func (es SqlEmojiStore) Search(name string, prefixOnly bool, limit int) ([]*model.Emoji, error) { func (es SqlEmojiStore) Search(name string, prefixOnly bool, limit int) ([]*model.Emoji, error) {
emojis := []*model.Emoji{} var emojis []*model.Emoji
name = sanitizeSearchTerm(name, "\\") name = sanitizeSearchTerm(name, "\\")
@ -115,19 +114,14 @@ func (es SqlEmojiStore) Search(name string, prefixOnly bool, limit int) ([]*mode
if !prefixOnly { if !prefixOnly {
term = "%" term = "%"
} }
term += name + "%" term += name + "%"
if err := es.GetReplica().Select(&emojis, query := es.emojiSelectQuery.
`SELECT Where(sq.Like{"Name": term}).
* OrderBy("Name").
FROM Limit(uint64(limit))
Emoji
WHERE if err := es.GetReplica().SelectBuilder(&emojis, query); err != nil {
Name LIKE ?
AND DeleteAt = 0
ORDER BY Name
LIMIT ?`, term, limit); err != nil {
return nil, errors.Wrapf(err, "could not search emojis by name %s", name) return nil, errors.Wrapf(err, "could not search emojis by name %s", name)
} }
return emojis, nil return emojis, nil
@ -137,19 +131,13 @@ func (es SqlEmojiStore) Search(name string, prefixOnly bool, limit int) ([]*mode
func (es SqlEmojiStore) getBy(c request.CTX, what, key string) (*model.Emoji, error) { func (es SqlEmojiStore) getBy(c request.CTX, what, key string) (*model.Emoji, error) {
var emoji model.Emoji var emoji model.Emoji
err := es.DBXFromContext(c.Context()).Get(&emoji, query := es.emojiSelectQuery.Where(sq.Eq{what: key})
`SELECT
* err := es.DBXFromContext(c.Context()).GetBuilder(&emoji, query)
FROM
Emoji
WHERE
`+what+` = ?
AND DeleteAt = 0`, key)
if err != nil { if err != nil {
if err == sql.ErrNoRows { if err == sql.ErrNoRows {
return nil, store.NewErrNotFound("Emoji", fmt.Sprintf("%s=%s", what, key)) return nil, store.NewErrNotFound("Emoji", fmt.Sprintf("%s=%s", what, key))
} }
return nil, errors.Wrapf(err, "could not get emoji by %s with value %s", what, key) return nil, errors.Wrapf(err, "could not get emoji by %s with value %s", what, key)
} }

View File

@ -126,14 +126,6 @@ func constructMySQLJSONArgs(props map[string]string) ([]any, string) {
return args, argString return args, argString
} }
func makeStringArgs(params []string) []any {
args := make([]any, len(params))
for i, name := range params {
args[i] = name
}
return args
}
func constructArrayArgs(ids []string) (string, []any) { func constructArrayArgs(ids []string) (string, []any) {
var placeholder strings.Builder var placeholder strings.Builder
values := make([]any, 0, len(ids)) values := make([]any, 0, len(ids))
@ -160,8 +152,7 @@ func wrapBinaryParamStringMap(ok bool, props model.StringMap) model.StringMap {
// morphWriter is a target to pass to the logger instance of morph. // morphWriter is a target to pass to the logger instance of morph.
// For now, everything is just logged at a debug level. If we need to log // For now, everything is just logged at a debug level. If we need to log
// errors/warnings from the library also, that needs to be seen later. // errors/warnings from the library also, that needs to be seen later.
type morphWriter struct { type morphWriter struct{}
}
func (l *morphWriter) Write(in []byte) (int, error) { func (l *morphWriter) Write(in []byte) (int, error) {
mlog.Debug(string(in)) mlog.Debug(string(in))