From 9aa4818c71da87002f3e5e825ac26afa2891983f Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 28 Mar 2025 18:58:26 -0300 Subject: [PATCH] MM-62154: Avoid SELECT * in webhook_store.go (#30463) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * MM-62154: Avoid SELECT * in webhook_store.go - Defined column list functions for both IncomingWebhooks and OutgoingWebhooks tables - Converted all uses of SELECT * to use the column list functions - Used the QueryBuilder pattern for SQL queries for better consistency and readability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * MM-62154: Improve webhook store implementation using query builders - Refactored webhook store to follow session_store.go pattern - Added select query builders in the constructor - Used pre-built queries throughout the store implementation for better maintainability and performance - Removed redundant incomingWebhookColumns() and outgoingWebhookColumns() functions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * MM-62154: Fix whitespace style issues in webhook_store.go - Ran gofmt to remove trailing whitespace - Ensures consistent code formatting 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * MM-62154: Remove unnecessary int(0) casting - Simplified DeleteAt=0 condition by removing redundant int casting - Maintains same behavior while making code cleaner 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --------- Co-authored-by: Mattermost Build --- .../channels/store/sqlstore/webhook_store.go | 112 ++++++++++++++---- 1 file changed, 86 insertions(+), 26 deletions(-) diff --git a/server/channels/store/sqlstore/webhook_store.go b/server/channels/store/sqlstore/webhook_store.go index 7a174a0478..d0fc10b122 100644 --- a/server/channels/store/sqlstore/webhook_store.go +++ b/server/channels/store/sqlstore/webhook_store.go @@ -17,16 +17,61 @@ import ( type SqlWebhookStore struct { *SqlStore metrics einterfaces.MetricsInterface + + incomingWebhookSelectQuery sq.SelectBuilder + outgoingWebhookSelectQuery sq.SelectBuilder } func (s SqlWebhookStore) ClearCaches() { } func newSqlWebhookStore(sqlStore *SqlStore, metrics einterfaces.MetricsInterface) store.WebhookStore { - return &SqlWebhookStore{ + s := &SqlWebhookStore{ SqlStore: sqlStore, metrics: metrics, } + + // Initialize query builders for incoming webhooks + s.incomingWebhookSelectQuery = s.getQueryBuilder(). + Select( + "Id", + "CreateAt", + "UpdateAt", + "DeleteAt", + "UserId", + "ChannelId", + "TeamId", + "DisplayName", + "Description", + "Username", + "IconURL", + "ChannelLocked", + ). + From("IncomingWebhooks") + + // Initialize query builders for outgoing webhooks + s.outgoingWebhookSelectQuery = s.getQueryBuilder(). + Select( + "Id", + "Token", + "CreateAt", + "UpdateAt", + "DeleteAt", + "CreatorId", + "ChannelId", + "TeamId", + "TriggerWords", + "TriggerWhen", + "CallbackURLs", + "DisplayName", + "Description", + "ContentType", + "Username", + "IconURL", + ). + From("OutgoingWebhooks") + + return s } func (s SqlWebhookStore) InvalidateWebhookCache(webhookId string) { @@ -68,7 +113,14 @@ func (s SqlWebhookStore) UpdateIncoming(hook *model.IncomingWebhook) (*model.Inc func (s SqlWebhookStore) GetIncoming(id string, allowFromCache bool) (*model.IncomingWebhook, error) { var webhook model.IncomingWebhook - if err := s.GetReplica().Get(&webhook, "SELECT * FROM IncomingWebhooks WHERE Id = ? AND DeleteAt = 0", id); err != nil { + + query := s.incomingWebhookSelectQuery. + Where(sq.And{ + sq.Eq{"Id": id}, + sq.Eq{"DeleteAt": 0}, + }) + + if err := s.GetReplica().GetBuilder(&webhook, query); err != nil { if err == sql.ErrNoRows { return nil, store.NewErrNotFound("IncomingWebhook", id) } @@ -112,10 +164,10 @@ func (s SqlWebhookStore) GetIncomingList(offset, limit int) ([]*model.IncomingWe func (s SqlWebhookStore) GetIncomingListByUser(userId string, offset, limit int) ([]*model.IncomingWebhook, error) { webhooks := []*model.IncomingWebhook{} - query := s.getQueryBuilder(). - Select("*"). - From("IncomingWebhooks"). - Where(sq.Eq{"DeleteAt": int(0)}).Limit(uint64(limit)).Offset(uint64(offset)) + query := s.incomingWebhookSelectQuery. + Where(sq.Eq{"DeleteAt": 0}). + Limit(uint64(limit)). + Offset(uint64(offset)) if userId != "" { query = query.Where(sq.Eq{"UserId": userId}) @@ -131,13 +183,13 @@ func (s SqlWebhookStore) GetIncomingListByUser(userId string, offset, limit int) func (s SqlWebhookStore) GetIncomingByTeamByUser(teamId string, userId string, offset, limit int) ([]*model.IncomingWebhook, error) { webhooks := []*model.IncomingWebhook{} - query := s.getQueryBuilder(). - Select("*"). - From("IncomingWebhooks"). + query := s.incomingWebhookSelectQuery. Where(sq.And{ sq.Eq{"TeamId": teamId}, - sq.Eq{"DeleteAt": int(0)}, - }).Limit(uint64(limit)).Offset(uint64(offset)) + sq.Eq{"DeleteAt": 0}, + }). + Limit(uint64(limit)). + Offset(uint64(offset)) if userId != "" { query = query.Where(sq.Eq{"UserId": userId}) @@ -157,7 +209,13 @@ func (s SqlWebhookStore) GetIncomingByTeam(teamId string, offset, limit int) ([] func (s SqlWebhookStore) GetIncomingByChannel(channelId string) ([]*model.IncomingWebhook, error) { webhooks := []*model.IncomingWebhook{} - if err := s.GetReplica().Select(&webhooks, "SELECT * FROM IncomingWebhooks WHERE ChannelId = ? AND DeleteAt = 0", channelId); err != nil { + query := s.incomingWebhookSelectQuery. + Where(sq.And{ + sq.Eq{"ChannelId": channelId}, + sq.Eq{"DeleteAt": 0}, + }) + + if err := s.GetReplica().SelectBuilder(&webhooks, query); err != nil { return nil, errors.Wrapf(err, "failed to find IncomingWebhooks with channelId=%s", channelId) } @@ -189,7 +247,13 @@ func (s SqlWebhookStore) SaveOutgoing(webhook *model.OutgoingWebhook) (*model.Ou func (s SqlWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) { var webhook model.OutgoingWebhook - if err := s.GetReplica().Get(&webhook, "SELECT * FROM OutgoingWebhooks WHERE Id = ? AND DeleteAt = 0", id); err != nil { + query := s.outgoingWebhookSelectQuery. + Where(sq.And{ + sq.Eq{"Id": id}, + sq.Eq{"DeleteAt": 0}, + }) + + if err := s.GetReplica().GetBuilder(&webhook, query); err != nil { if err == sql.ErrNoRows { return nil, store.NewErrNotFound("OutgoingWebhook", id) } @@ -203,12 +267,12 @@ func (s SqlWebhookStore) GetOutgoing(id string) (*model.OutgoingWebhook, error) func (s SqlWebhookStore) GetOutgoingListByUser(userId string, offset, limit int) ([]*model.OutgoingWebhook, error) { webhooks := []*model.OutgoingWebhook{} - query := s.getQueryBuilder(). - Select("*"). - From("OutgoingWebhooks"). + query := s.outgoingWebhookSelectQuery. Where(sq.And{ - sq.Eq{"DeleteAt": int(0)}, - }).Limit(uint64(limit)).Offset(uint64(offset)) + sq.Eq{"DeleteAt": 0}, + }). + Limit(uint64(limit)). + Offset(uint64(offset)) if userId != "" { query = query.Where(sq.Eq{"CreatorId": userId}) @@ -228,12 +292,10 @@ func (s SqlWebhookStore) GetOutgoingList(offset, limit int) ([]*model.OutgoingWe func (s SqlWebhookStore) GetOutgoingByChannelByUser(channelId string, userId string, offset, limit int) ([]*model.OutgoingWebhook, error) { webhooks := []*model.OutgoingWebhook{} - query := s.getQueryBuilder(). - Select("*"). - From("OutgoingWebhooks"). + query := s.outgoingWebhookSelectQuery. Where(sq.And{ sq.Eq{"ChannelId": channelId}, - sq.Eq{"DeleteAt": int(0)}, + sq.Eq{"DeleteAt": 0}, }) if userId != "" { @@ -257,12 +319,10 @@ func (s SqlWebhookStore) GetOutgoingByChannel(channelId string, offset, limit in func (s SqlWebhookStore) GetOutgoingByTeamByUser(teamId string, userId string, offset, limit int) ([]*model.OutgoingWebhook, error) { webhooks := []*model.OutgoingWebhook{} - query := s.getQueryBuilder(). - Select("*"). - From("OutgoingWebhooks"). + query := s.outgoingWebhookSelectQuery. Where(sq.And{ sq.Eq{"TeamId": teamId}, - sq.Eq{"DeleteAt": int(0)}, + sq.Eq{"DeleteAt": 0}, }) if userId != "" {