From 65256843f94bd4edf414d0bdbe4ae936b0737a1e Mon Sep 17 00:00:00 2001 From: Chris Gibson Date: Wed, 26 Mar 2025 13:23:08 +0000 Subject: [PATCH] [GH-30056] Refactor SELECT statements in compliance_store.go (#30124) * Change queries to use builder * extract to tableSelectQuery * Apply suggestions from code review * update tests to check errors, lengths * linting * leverage s.toReserveCase --------- Co-authored-by: Jesse Hallam Co-authored-by: Jesse Hallam Co-authored-by: Mattermost Build --- .../store/sqlstore/compliance_store.go | 40 ++++++++++++++++--- .../store/storetest/compliance_store.go | 34 ++++++++++------ 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/server/channels/store/sqlstore/compliance_store.go b/server/channels/store/sqlstore/compliance_store.go index cd82f50c6c..88ae4804dc 100644 --- a/server/channels/store/sqlstore/compliance_store.go +++ b/server/channels/store/sqlstore/compliance_store.go @@ -18,10 +18,33 @@ import ( type SqlComplianceStore struct { *SqlStore + + tableSelectQuery sq.SelectBuilder } func newSqlComplianceStore(sqlStore *SqlStore) store.ComplianceStore { - return &SqlComplianceStore{sqlStore} + s := SqlComplianceStore{ + SqlStore: sqlStore, + } + + s.tableSelectQuery = s. + getQueryBuilder(). + Select( + "Id", + "CreateAt", + "UserId", + "Status", + "Count", + s.toReserveCase("desc"), + "Type", + "StartAt", + "EndAt", + "Keywords", + "Emails", + ). + From("Compliances") + + return &s } func (s SqlComplianceStore) Save(compliance *model.Compliance) (*model.Compliance, error) { @@ -83,17 +106,24 @@ func (s SqlComplianceStore) Update(compliance *model.Compliance) (*model.Complia } func (s SqlComplianceStore) GetAll(offset, limit int) (model.Compliances, error) { - query := "SELECT * FROM Compliances ORDER BY CreateAt DESC LIMIT ? OFFSET ?" - compliances := model.Compliances{} - if err := s.GetReplica().Select(&compliances, query, limit, offset); err != nil { + query := s.tableSelectQuery. + OrderBy("CreateAt DESC"). + Limit(uint64(limit)). + Offset(uint64(offset)) + + var compliances model.Compliances + if err := s.GetReplica().SelectBuilder(&compliances, query); err != nil { return nil, errors.Wrap(err, "failed to find all Compliances") } + return compliances, nil } func (s SqlComplianceStore) Get(id string) (*model.Compliance, error) { + query := s.tableSelectQuery.Where(sq.Eq{"Id": id}) + var compliance model.Compliance - if err := s.GetReplica().Get(&compliance, `SELECT * FROM Compliances WHERE Id = ?`, id); err != nil { + if err := s.GetReplica().GetBuilder(&compliance, query); err != nil { if err == sql.ErrNoRows { return nil, store.NewErrNotFound("Compliances", id) } diff --git a/server/channels/store/storetest/compliance_store.go b/server/channels/store/storetest/compliance_store.go index 9bfbacd40d..fa013b8fb6 100644 --- a/server/channels/store/storetest/compliance_store.go +++ b/server/channels/store/storetest/compliance_store.go @@ -17,19 +17,19 @@ import ( ) func cleanupStoreState(t *testing.T, rctx request.CTX, ss store.Store) { - //remove existing users + // remove existing users allUsers, err := ss.User().GetAll() require.NoError(t, err, "error cleaning all test users", err) for _, u := range allUsers { err = ss.User().PermanentDelete(rctx, u.Id) require.NoError(t, err, "failed cleaning up test user %s", u.Username) - //remove all posts by this user + // remove all posts by this user nErr := ss.Post().PermanentDeleteByUser(rctx, u.Id) require.NoError(t, nErr, "failed cleaning all posts of test user %s", u.Username) } - //remove existing channels + // remove existing channels allChannels, nErr := ss.Channel().GetAllChannels(0, 100000, store.ChannelSearchOpts{IncludeDeleted: true}) require.NoError(t, nErr, "error cleaning all test channels", nErr) for _, channel := range allChannels { @@ -37,7 +37,7 @@ func cleanupStoreState(t *testing.T, rctx request.CTX, ss store.Store) { require.NoError(t, nErr, "failed cleaning up test channel %s", channel.Id) } - //remove existing teams + // remove existing teams allTeams, nErr := ss.Team().GetAll() require.NoError(t, nErr, "error cleaning all test teams", nErr) for _, team := range allTeams { @@ -72,8 +72,10 @@ func testComplianceStore(t *testing.T, rctx request.CTX, ss store.Store) { require.NoError(t, err) time.Sleep(100 * time.Millisecond) - compliances, _ := ss.Compliance().GetAll(0, 1000) + compliances, err := ss.Compliance().GetAll(0, 1000) + require.NoError(t, err) + require.Len(t, compliances, 2) require.Equal(t, model.ComplianceStatusRunning, compliances[0].Status) require.Equal(t, compliance2.Id, compliances[0].Id) @@ -81,20 +83,26 @@ func testComplianceStore(t *testing.T, rctx request.CTX, ss store.Store) { _, err = ss.Compliance().Update(compliance2) require.NoError(t, err) - compliances, _ = ss.Compliance().GetAll(0, 1000) + compliances, err = ss.Compliance().GetAll(0, 1000) + require.NoError(t, err) + require.Len(t, compliances, 2) require.Equal(t, model.ComplianceStatusFailed, compliances[0].Status) require.Equal(t, compliance2.Id, compliances[0].Id) - compliances, _ = ss.Compliance().GetAll(0, 1) + compliances, err = ss.Compliance().GetAll(0, 1) + require.NoError(t, err) require.Len(t, compliances, 1) - compliances, _ = ss.Compliance().GetAll(1, 1) + compliances, err = ss.Compliance().GetAll(1, 1) + require.NoError(t, err) require.Len(t, compliances, 1) - rc2, _ := ss.Compliance().Get(compliance2.Id) + rc2, err := ss.Compliance().Get(compliance2.Id) + require.NoError(t, err) + require.Equal(t, compliance2.Status, rc2.Status) } @@ -837,7 +845,7 @@ func testEditExportMessage(t *testing.T, rctx request.CTX, ss store.Store) { post1, err = ss.Post().Save(rctx, post1) require.NoError(t, err) - //user 1 edits the previous post + // user 1 edits the previous post post1e := post1.Clone() post1e.Message = "edit " + post1.Message @@ -948,7 +956,7 @@ func testEditAfterExportMessage(t *testing.T, rctx request.CTX, ss store.Store) assert.Equal(t, user1.Username, *v.Username) postEditTime := post1.UpdateAt + 1 - //user 1 edits the previous post + // user 1 edits the previous post post1e := post1.Clone() post1e.EditAt = postEditTime post1e.Message = "edit " + post1.Message @@ -1040,7 +1048,7 @@ func testDeleteExportMessage(t *testing.T, rctx request.CTX, ss store.Store) { post1, err = ss.Post().Save(rctx, post1) require.NoError(t, err) - //user 1 deletes the previous post + // user 1 deletes the previous post postDeleteTime := post1.UpdateAt + 1 err = ss.Post().Delete(rctx, post1.Id, postDeleteTime, user1.Id) require.NoError(t, err) @@ -1143,7 +1151,7 @@ func testDeleteAfterExportMessage(t *testing.T, rctx request.CTX, ss store.Store assert.Equal(t, user1.Email, *v.UserEmail) assert.Equal(t, user1.Username, *v.Username) - //user 1 deletes the previous post + // user 1 deletes the previous post postDeleteTime := post1.UpdateAt + 1 err = ss.Post().Delete(rctx, post1.Id, postDeleteTime, user1.Id) require.NoError(t, err)