From ebe03c1d45ed63f97276491bbad1a2b4c5bb059e Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 2 Jul 2025 12:35:54 -0300 Subject: [PATCH] Channel Store: No SELECT * (#32167) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Replace SELECT * with explicit column lists in channel store Migrates channel_store.go away from SELECT * patterns to explicit column lists for better performance, maintainability, and schema safety. - Replace GetPinnedPosts raw SQL with query builder using postSliceColumns() - Replace "cc.*" in group channel search with channelSliceColumns() - Replace GetChannelsBatchForIndexing raw SQL with query builder - Replace channel member and team queries with respective column helpers - Use SelectBuilder helper instead of manual ToSql() calls 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * Replace SELECT * with COUNT(*) in user_test.go Replaces unnecessary SELECT * queries with SELECT COUNT(*) in TestPermanentDeleteUser bot count verification. Only needs to check the count of bots, not retrieve full bot records. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --------- Co-authored-by: Claude --- server/channels/app/user_test.go | 12 ++-- .../channels/store/sqlstore/channel_store.go | 58 ++++++++++--------- .../channels/store/storetest/channel_store.go | 27 ++++++++- 3 files changed, 63 insertions(+), 34 deletions(-) diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index cb35808be6..66bf36ac9d 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -1248,12 +1248,12 @@ func TestPermanentDeleteUser(t *testing.T) { }) assert.Nil(t, err) - bots1 := []*model.Bot{} - bots2 := []*model.Bot{} + var botCount1 int + var botCount2 int - err1 := th.SQLStore.GetMaster().Select(&bots1, "SELECT * FROM Bots") + err1 := th.SQLStore.GetMaster().Get(&botCount1, "SELECT COUNT(*) FROM Bots") assert.NoError(t, err1) - assert.Equal(t, 1, len(bots1)) + assert.Equal(t, 1, botCount1) // test that bot is deleted from bots table retUser1, err := th.App.GetUser(bot.UserId) @@ -1262,9 +1262,9 @@ func TestPermanentDeleteUser(t *testing.T) { err = th.App.PermanentDeleteUser(th.Context, retUser1) assert.Nil(t, err) - err1 = th.SQLStore.GetMaster().Select(&bots2, "SELECT * FROM Bots") + err1 = th.SQLStore.GetMaster().Get(&botCount2, "SELECT COUNT(*) FROM Bots") assert.NoError(t, err1) - assert.Equal(t, 0, len(bots2)) + assert.Equal(t, 0, botCount2) scheduledPost1 := &model.ScheduledPost{ Draft: model.Draft{ diff --git a/server/channels/store/sqlstore/channel_store.go b/server/channels/store/sqlstore/channel_store.go index 1fe216ea60..fcd80e1529 100644 --- a/server/channels/store/sqlstore/channel_store.go +++ b/server/channels/store/sqlstore/channel_store.go @@ -199,7 +199,8 @@ func channelMemberToSlice(member *model.ChannelMember) []any { type channelMemberWithSchemeRolesList []channelMemberWithSchemeRoles func getChannelRoles(schemeGuest, schemeUser, schemeAdmin bool, defaultTeamGuestRole, defaultTeamUserRole, defaultTeamAdminRole, defaultChannelGuestRole, defaultChannelUserRole, defaultChannelAdminRole string, - roles []string) rolesInfo { + roles []string, +) rolesInfo { result := rolesInfo{ roles: []string{}, explicitRoles: []string{}, @@ -761,13 +762,11 @@ func (s SqlChannelStore) saveChannelT(transaction *sqlxTxWrapper, channel *model } insertResult, err := transaction.Exec(query, params...) - if err != nil { return nil, errors.Wrapf(err, "save_channel: id=%s", channel.Id) } rowAffected, err := insertResult.RowsAffected() - if err != nil { return nil, errors.Wrapf(err, "save_channel: id=%s", channel.Id) } @@ -874,7 +873,6 @@ func (s SqlChannelStore) GetChannelUnread(channelId, userId string) (*model.Chan AND UserId = ? AND DeleteAt = 0`, channelId, userId) - if err != nil { if err == sql.ErrNoRows { return nil, store.NewErrNotFound("Channel", fmt.Sprintf("channelId=%s,userId=%s", channelId, userId)) @@ -895,8 +893,19 @@ func (s SqlChannelStore) InvalidateChannelByName(teamId, name string) { func (s SqlChannelStore) GetPinnedPosts(channelId string) (*model.PostList, error) { pl := model.NewPostList() + query := s.getQueryBuilder(). + Select(postSliceColumns()...). + Column("(SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = (CASE WHEN p.RootId = '' THEN p.Id ELSE p.RootId END) AND Posts.DeleteAt = 0) as ReplyCount"). + From("Posts p"). + Where(sq.Eq{ + "IsPinned": true, + "ChannelId": channelId, + "DeleteAt": 0, + }). + OrderBy("CreateAt ASC") + posts := []*model.Post{} - if err := s.GetReplica().Select(&posts, "SELECT *, (SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = (CASE WHEN p.RootId = '' THEN p.Id ELSE p.RootId END) AND Posts.DeleteAt = 0) as ReplyCount FROM Posts p WHERE IsPinned = true AND ChannelId = ? AND DeleteAt = 0 ORDER BY CreateAt ASC", channelId); err != nil { + if err := s.GetReplica().SelectBuilder(&posts, query); err != nil { return nil, errors.Wrap(err, "failed to find Posts") } for _, post := range posts { @@ -1459,7 +1468,6 @@ func (s SqlChannelStore) GetChannelCounts(teamId string, userId string) (*model. AND (TeamId = ? OR TeamId = '') AND DeleteAt = 0 ORDER BY DisplayName`, userId, teamId) - if err != nil { return nil, errors.Wrapf(err, "failed to get channels count with teamId=%s and userId=%s", teamId, userId) } @@ -2116,7 +2124,6 @@ func (s SqlChannelStore) GetChannelMembersTimezones(channelId string) ([]model.S Users ON ChannelMembers.UserId = Id WHERE ChannelId = ? `, channelId) - if err != nil { return nil, errors.Wrapf(err, "failed to find user timezones for users in channels with channelId=%s", channelId) } @@ -2514,7 +2521,6 @@ func (s SqlChannelStore) GetPinnedPostCount(channelId string, allowFromCache boo IsPinned = true AND ChannelId = ? AND DeleteAt = 0`, channelId) - if err != nil { return 0, errors.Wrapf(err, "failed to count pinned Posts with channelId=%s", channelId) } @@ -2683,7 +2689,7 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelIds []string, userId string) return times, nil } - var msgCountQuery, msgCountQueryRoot, lastViewedQuery = sq.Case("ChannelId"), sq.Case("ChannelId"), sq.Case("ChannelId") + msgCountQuery, msgCountQueryRoot, lastViewedQuery := sq.Case("ChannelId"), sq.Case("ChannelId"), sq.Case("ChannelId") for _, t := range lastPostAtTimes { times[t.Id] = t.LastPostAt @@ -2904,7 +2910,6 @@ func (s SqlChannelStore) IncrementMentionCount(channelId string, userIDs []strin "ChannelId": channelId, }). ToSql() - if err != nil { return errors.Wrap(err, "IncrementMentionCount_Tosql") } @@ -3888,7 +3893,7 @@ func (s SqlChannelStore) searchGroupChannelsQuery(userId, term string, isPostgre }). GroupBy("c.Id") - return s.getQueryBuilder().Select("cc.*"). + return s.getQueryBuilder().Select(channelSliceColumns(true, "cc")...). FromSelect(cc, "cc"). Join("ChannelMembers cm on cc.Id = cm.ChannelId"). Join("Users u on u.Id = cm.UserId"). @@ -4338,22 +4343,21 @@ func (s SqlChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterId s } func (s SqlChannelStore) GetChannelsBatchForIndexing(startTime int64, startChannelID string, limit int) ([]*model.Channel, error) { - query := - `SELECT - * - FROM - Channels - WHERE - CreateAt > ? - OR - (CreateAt = ? AND Id > ?) - ORDER BY - CreateAt ASC, Id ASC - LIMIT - ?` + query := s.getQueryBuilder(). + Select(channelSliceColumns(false)...). + From("Channels"). + Where(sq.Or{ + sq.Gt{"CreateAt": startTime}, + sq.And{ + sq.Eq{"CreateAt": startTime}, + sq.Gt{"Id": startChannelID}, + }, + }). + OrderBy("CreateAt ASC", "Id ASC"). + Limit(uint64(limit)) channels := []*model.Channel{} - err := s.GetSearchReplicaX().Select(&channels, query, startTime, startTime, startChannelID, limit) + err := s.GetSearchReplicaX().SelectBuilder(&channels, query) if err != nil { return nil, errors.Wrap(err, "failed to find Channels") } @@ -4398,7 +4402,7 @@ func (s SqlChannelStore) UpdateMembersRole(channelID string, adminIDs []string) // A SELECT and a UPDATE query are needed. // Once we only support PostgreSQL, this can be done in a single query using RETURNING. query, args, err := s.getQueryBuilder(). - Select("*"). + Select(channelMemberSliceColumns()...). From("ChannelMembers"). Where(sq.Eq{"ChannelID": channelID}). Where(sq.Or{sq.Eq{"SchemeGuest": false}, sq.Expr("SchemeGuest IS NULL")}). @@ -4507,7 +4511,7 @@ func (s SqlChannelStore) GetTeamForChannel(channelID string) (*model.Team, error return nil, errors.Wrap(err, "get_team_for_channel_nested_tosql") } query, args, err := s.getQueryBuilder(). - Select("*"). + Select(teamSliceColumns()...). From("Teams").Where(sq.Expr("Id = ("+nestedQ+")", nestedArgs...)).ToSql() if err != nil { return nil, errors.Wrap(err, "get_team_for_channel_tosql") diff --git a/server/channels/store/storetest/channel_store.go b/server/channels/store/storetest/channel_store.go index 83d6d959cc..1acfb1aae2 100644 --- a/server/channels/store/storetest/channel_store.go +++ b/server/channels/store/storetest/channel_store.go @@ -7076,7 +7076,7 @@ func testChannelStoreSearchGroupChannels(t *testing.T, rctx request.CTX, ss stor require.NoError(t, nErr) for _, userID := range userIds { - _, err := ss.Channel().SaveMember(rctx, &model.ChannelMember{ + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ ChannelId: gc2.Id, UserId: userID, NotifyProps: model.GetDefaultChannelNotifyProps(), @@ -7092,6 +7092,20 @@ func testChannelStoreSearchGroupChannels(t *testing.T, rctx request.CTX, ss stor _, nErr = ss.Channel().Save(rctx, &gc3, -1) require.NoError(t, nErr) + // Make gc3 policy enforced + _, err = ss.AccessControlPolicy().Save(rctx, &model.AccessControlPolicy{ + ID: gc3.Id, + Version: model.AccessControlPolicyVersionV0_1, + Type: model.AccessControlPolicyTypeChannel, + Rules: []model.AccessControlPolicyRule{ + { + Actions: []string{}, + Expression: "", + }, + }, + }) + require.NoError(t, err) + for _, userID := range userIds { _, err := ss.Channel().SaveMember(rctx, &model.ChannelMember{ ChannelId: gc3.Id, @@ -7108,6 +7122,16 @@ func testChannelStoreSearchGroupChannels(t *testing.T, rctx request.CTX, ss stor } }() + // assertChannelListPopulated verifies that the channel objects in the given channel list + // are fully populated. + assertChannelListPopulated := func(t *testing.T, channelList model.ChannelList) { + for _, actualChannel := range channelList { + expectedChannel, err := ss.Channel().Get(actualChannel.Id, false) + require.NoError(t, err) + assert.Equal(t, expectedChannel, actualChannel, "channel %q in channel list missing metadata", actualChannel.Id) + } + } + testCases := []struct { Name string UserID string @@ -7169,6 +7193,7 @@ func testChannelStoreSearchGroupChannels(t *testing.T, rctx request.CTX, ss stor } require.ElementsMatch(t, tc.ExpectedResult, resultIds) + assertChannelListPopulated(t, result) }) } }