From 011f179831e461c88c544f33c20c21978b7017e8 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 23 Apr 2025 16:06:04 -0300 Subject: [PATCH] avoid SELECT * in preference store (#30835) --- .../store/sqlstore/preference_store.go | 4 +- .../store/storetest/preference_store.go | 96 +++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/server/channels/store/sqlstore/preference_store.go b/server/channels/store/sqlstore/preference_store.go index 49d524014d..496e433e9c 100644 --- a/server/channels/store/sqlstore/preference_store.go +++ b/server/channels/store/sqlstore/preference_store.go @@ -242,7 +242,7 @@ func (s *SqlPreferenceStore) DeleteOrphanedRows(limit int) (deleted int64, err e // We need the extra level of nesting to deal with MySQL's locking const query = ` DELETE FROM Preferences WHERE Name IN ( - SELECT * FROM ( + SELECT Name FROM ( SELECT Preferences.Name FROM Preferences LEFT JOIN Posts ON Preferences.Name = Posts.Id WHERE Posts.Id IS NULL AND Category = ? @@ -264,7 +264,7 @@ func (s SqlPreferenceStore) CleanupFlagsBatch(limit int64) (int64, error) { // it is better to manually check here, or change the function type to uint64 return int64(0), errors.Errorf("Received a negative limit") } - nameInQ, nameInArgs, err := sq.Select("*"). + nameInQ, nameInArgs, err := sq.Select("Name"). FromSelect( sq.Select("Preferences.Name"). From("Preferences"). diff --git a/server/channels/store/storetest/preference_store.go b/server/channels/store/storetest/preference_store.go index 97099c6bfe..a860f995a7 100644 --- a/server/channels/store/storetest/preference_store.go +++ b/server/channels/store/storetest/preference_store.go @@ -25,6 +25,7 @@ func TestPreferenceStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlSt t.Run("PreferenceDeleteCategory", func(t *testing.T) { testPreferenceDeleteCategory(t, rctx, ss) }) t.Run("PreferenceDeleteCategoryAndName", func(t *testing.T) { testPreferenceDeleteCategoryAndName(t, rctx, ss) }) t.Run("PreferenceDeleteOrphanedRows", func(t *testing.T) { testPreferenceDeleteOrphanedRows(t, rctx, ss) }) + t.Run("PreferenceCleanupFlagsBatch", func(t *testing.T) { testPreferenceCleanupFlagsBatch(t, rctx, ss) }) t.Run("PreferenceDeleteInvalidVisibleDmsGms", func(t *testing.T) { testDeleteInvalidVisibleDmsGms(t, rctx, ss, s) }) } @@ -469,6 +470,101 @@ func testPreferenceDeleteOrphanedRows(t *testing.T, rctx request.CTX, ss store.S assert.NoError(t, nErr, "newer preference should not have been deleted") } +func testPreferenceCleanupFlagsBatch(t *testing.T, rctx request.CTX, ss store.Store) { + t.Run("Normal cleanup case", func(t *testing.T) { + const limit = 100 + team, err := ss.Team().Save(&model.Team{ + DisplayName: "DisplayName", + Name: "team" + model.NewId(), + Email: MakeEmail(), + Type: model.TeamOpen, + }) + require.NoError(t, err) + channel, err := ss.Channel().Save(rctx, &model.Channel{ + TeamId: team.Id, + DisplayName: "DisplayName", + Name: "channel" + model.NewId(), + Type: model.ChannelTypeOpen, + }, -1) + require.NoError(t, err) + category := model.PreferenceCategoryFlaggedPost + userId := model.NewId() + + // Create two posts: one to delete, one to keep + olderPost, err := ss.Post().Save(rctx, &model.Post{ + ChannelId: channel.Id, + UserId: userId, + Message: "message", + CreateAt: 1000, + }) + require.NoError(t, err) + newerPost, err := ss.Post().Save(rctx, &model.Post{ + ChannelId: channel.Id, + UserId: userId, + Message: "message", + CreateAt: 3000, + }) + require.NoError(t, err) + + // Create a third post ID that doesn't exist in the Posts table + nonExistentPostId := model.NewId() + + // Create preferences flagging all three posts + preferences := model.Preferences{ + { + UserId: userId, + Category: category, + Name: olderPost.Id, + Value: "true", + }, + { + UserId: userId, + Category: category, + Name: newerPost.Id, + Value: "true", + }, + { + UserId: userId, + Category: category, + Name: nonExistentPostId, + Value: "true", + }, + } + + err = ss.Preference().Save(preferences) + require.NoError(t, err) + + // Instead of just deleting (soft delete), we'll permanently delete the post + // to create a truly orphaned preference + err = ss.Post().PermanentDelete(rctx, olderPost.Id) + require.NoError(t, err) + + // Verify we have 3 preferences before cleanup + prefs, err := ss.Preference().GetCategory(userId, category) + require.NoError(t, err) + require.Equal(t, 3, len(prefs)) + + // Run CleanupFlagsBatch to remove orphaned preferences + rowsDeleted, err := ss.Preference().CleanupFlagsBatch(limit) + require.NoError(t, err) + require.Equal(t, int64(2), rowsDeleted, "should have deleted 2 preferences (one with permanently deleted post and one with non-existent post)") + + // Check that only the preference for the existing post remains + prefs, err = ss.Preference().GetCategory(userId, category) + require.NoError(t, err) + require.Equal(t, 1, len(prefs), "should only have 1 preference left") + require.Equal(t, newerPost.Id, prefs[0].Name, "remaining preference should be for the undeleted post") + }) + + t.Run("Invalid limit case", func(t *testing.T) { + // Test with a negative limit which should return an error + rowsDeleted, err := ss.Preference().CleanupFlagsBatch(-1) + require.Error(t, err, "Expected error for negative limit") + require.Equal(t, int64(0), rowsDeleted, "Should not delete any rows when limit is negative") + require.Contains(t, err.Error(), "negative limit", "Error message should mention negative limit") + }) +} + func testDeleteInvalidVisibleDmsGms(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) { userId1 := model.NewId() userId2 := model.NewId()