diff --git a/app/channel.go b/app/channel.go index 051dd35d2b..50ce699546 100644 --- a/app/channel.go +++ b/app/channel.go @@ -96,8 +96,8 @@ func (a *App) JoinDefaultChannels(teamId string, user *model.User, shouldBeAdmin if cmResult := <-a.Srv.Store.Channel().SaveMember(cm); cmResult.Err != nil { err = cmResult.Err } - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if histErr := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); histErr != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", histErr)) } if *a.Config().ServiceSettings.ExperimentalEnableDefaultChannelLeaveJoinMessages { @@ -243,8 +243,8 @@ func (a *App) CreateChannel(channel *model.Channel, addMember bool) (*model.Chan if cmresult := <-a.Srv.Store.Channel().SaveMember(cm); cmresult.Err != nil { return nil, cmresult.Err } - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(channel.CreatorId, sc.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if err := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(channel.CreatorId, sc.Id, model.GetMillis()); err != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) } a.InvalidateCacheForUser(channel.CreatorId) @@ -358,11 +358,11 @@ func (a *App) createDirectChannel(userId string, otherUserId string) (*model.Cha return nil, err } - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId, channel.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId, channel.Id, model.GetMillis()); err != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) } - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(otherUserId, channel.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(otherUserId, channel.Id, model.GetMillis()); err != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) } return channel, nil @@ -470,8 +470,8 @@ func (a *App) createGroupChannel(userIds []string, creatorId string) (*model.Cha if result := <-a.Srv.Store.Channel().SaveMember(cm); result.Err != nil { return nil, result.Err } - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if err := a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); err != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) } } @@ -927,8 +927,8 @@ func (a *App) addUserToChannel(user *model.User, channel *model.Channel, teamMem } a.WaitForChannelMembership(channel.Id, user.Id) - if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); result.Err != nil { - mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", result.Err)) + if err = a.Srv.Store.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()); err != nil { + mlog.Warn(fmt.Sprintf("Failed to update ChannelMemberHistory table %v", err)) } a.InvalidateCacheForUser(user.Id) @@ -1606,8 +1606,8 @@ func (a *App) removeUserFromChannel(userIdToRemove string, removerUserId string, if err := a.Srv.Store.Channel().RemoveMember(channel.Id, userIdToRemove); err != nil { return err } - if cmhResult := <-a.Srv.Store.ChannelMemberHistory().LogLeaveEvent(userIdToRemove, channel.Id, model.GetMillis()); cmhResult.Err != nil { - return cmhResult.Err + if err := a.Srv.Store.ChannelMemberHistory().LogLeaveEvent(userIdToRemove, channel.Id, model.GetMillis()); err != nil { + return err } a.InvalidateCacheForUser(userIdToRemove) diff --git a/app/channel_test.go b/app/channel_test.go index 816673db76..cb91cfe72a 100644 --- a/app/channel_test.go +++ b/app/channel_test.go @@ -13,7 +13,6 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/store" ) func TestPermanentDeleteChannel(t *testing.T) { @@ -165,14 +164,17 @@ func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordTownSquare(t *testi channel, err := th.App.Srv.Store.Channel().GetByName(th.BasicTeam.Id, "town-square", true) require.Nil(t, err) townSquareChannelId := channel.Id - initialNumTownSquareUsers := len(store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, townSquareChannelId)).([]*model.ChannelMemberHistoryResult)) + users, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, townSquareChannelId) + require.Nil(t, err) + initialNumTownSquareUsers := len(users) // create a new user that joins the default channels user := th.CreateUser() th.App.JoinDefaultChannels(th.BasicTeam.Id, user, false, "") // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, townSquareChannelId)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, townSquareChannelId) + require.Nil(t, err) assert.Len(t, histories, initialNumTownSquareUsers+1) found := false @@ -193,14 +195,17 @@ func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordOffTopic(t *testing channel, err := th.App.Srv.Store.Channel().GetByName(th.BasicTeam.Id, "off-topic", true) require.Nil(t, err) offTopicChannelId := channel.Id - initialNumTownSquareUsers := len(store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, offTopicChannelId)).([]*model.ChannelMemberHistoryResult)) + users, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, offTopicChannelId) + require.Nil(t, err) + initialNumTownSquareUsers := len(users) // create a new user that joins the default channels user := th.CreateUser() th.App.JoinDefaultChannels(th.BasicTeam.Id, user, false, "") // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, offTopicChannelId)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, offTopicChannelId) + require.Nil(t, err) assert.Len(t, histories, initialNumTownSquareUsers+1) found := false @@ -251,7 +256,8 @@ func TestCreateChannelPublicCreatesChannelMemberHistoryRecord(t *testing.T) { publicChannel := th.createChannel(th.BasicTeam, model.CHANNEL_OPEN) // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, publicChannel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, publicChannel.Id) + require.Nil(t, err) assert.Len(t, histories, 1) assert.Equal(t, th.BasicUser.Id, histories[0].UserId) assert.Equal(t, publicChannel.Id, histories[0].ChannelId) @@ -265,7 +271,8 @@ func TestCreateChannelPrivateCreatesChannelMemberHistoryRecord(t *testing.T) { privateChannel := th.createChannel(th.BasicTeam, model.CHANNEL_PRIVATE) // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, privateChannel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, privateChannel.Id) + require.Nil(t, err) assert.Len(t, histories, 1) assert.Equal(t, th.BasicUser.Id, histories[0].UserId) assert.Equal(t, privateChannel.Id, histories[0].ChannelId) @@ -302,7 +309,8 @@ func TestCreateGroupChannelCreatesChannelMemberHistoryRecord(t *testing.T) { t.Fatal("Failed to create group channel. Error: " + err.Message) } else { // there should be a ChannelMemberHistory record for each user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id) + require.Nil(t, err) assert.Len(t, histories, 3) channelMemberHistoryUserIds := make([]string, 0) @@ -328,7 +336,8 @@ func TestCreateDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) { t.Fatal("Failed to create direct channel. Error: " + err.Message) } else { // there should be a ChannelMemberHistory record for both users - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id) + require.Nil(t, err) assert.Len(t, histories, 2) historyId0 := histories[0].UserId @@ -356,7 +365,8 @@ func TestGetDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) { t.Fatal("Failed to create direct channel. Error: " + err.Message) } else { // there should be a ChannelMemberHistory record for both users - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id) + require.Nil(t, err) assert.Len(t, histories, 2) historyId0 := histories[0].UserId @@ -392,7 +402,8 @@ func TestAddUserToChannelCreatesChannelMemberHistoryRecord(t *testing.T) { } // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id) + require.Nil(t, err) assert.Len(t, histories, 2) channelMemberHistoryUserIds := make([]string, 0) for _, history := range histories { @@ -408,7 +419,8 @@ func TestAddUserToChannelCreatesChannelMemberHistoryRecord(t *testing.T) { // a user creates a channel publicChannel := th.createChannel(th.BasicTeam, model.CHANNEL_OPEN) - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, publicChannel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, publicChannel.Id) + require.Nil(t, err) assert.Len(t, histories, 1) assert.Equal(t, th.BasicUser.Id, histories[0].UserId) assert.Equal(t, publicChannel.Id, histories[0].ChannelId) @@ -447,7 +459,8 @@ func TestAddChannelMemberNoUserRequestor(t *testing.T) { } // there should be a ChannelMemberHistory record for the user - histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + histories, err := th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id) + require.Nil(t, err) assert.Len(t, histories, 2) channelMemberHistoryUserIds := make([]string, 0) for _, history := range histories { diff --git a/app/syncables_test.go b/app/syncables_test.go index 67bb90af54..3a2e104ccb 100644 --- a/app/syncables_test.go +++ b/app/syncables_test.go @@ -294,9 +294,9 @@ func TestCreateDefaultMemberships(t *testing.T) { timeAfterLeaving := model.GetMillis() // Purging channelmemberhistory doesn't re-add user to channel - result := <-th.App.Srv.Store.ChannelMemberHistory().PermanentDeleteBatch(timeBeforeLeaving, 1000) - if result.Err != nil { - t.Errorf("error permanently deleting channelmemberhistory: %s", result.Err.Error()) + _, err = th.App.Srv.Store.ChannelMemberHistory().PermanentDeleteBatch(timeBeforeLeaving, 1000) + if err != nil { + t.Errorf("error permanently deleting channelmemberhistory: %s", err.Error()) } pErr = th.App.CreateDefaultMemberships(scienceChannelGroupSyncable.UpdateAt) @@ -310,9 +310,9 @@ func TestCreateDefaultMemberships(t *testing.T) { } // Purging channelmemberhistory doesn't re-add user to channel - result = <-th.App.Srv.Jobs.Store.ChannelMemberHistory().PermanentDeleteBatch(timeAfterLeaving, 1000) - if result.Err != nil { - t.Errorf("error permanently deleting channelmemberhistory: %s", result.Err.Error()) + _, err = th.App.Srv.Jobs.Store.ChannelMemberHistory().PermanentDeleteBatch(timeAfterLeaving, 1000) + if err != nil { + t.Errorf("error permanently deleting channelmemberhistory: %s", err.Error()) } pErr = th.App.CreateDefaultMemberships(scienceChannelGroupSyncable.UpdateAt) diff --git a/store/sqlstore/channel_member_history_store.go b/store/sqlstore/channel_member_history_store.go index 9487b954f0..6d96634cbc 100644 --- a/store/sqlstore/channel_member_history_store.go +++ b/store/sqlstore/channel_member_history_store.go @@ -33,62 +33,64 @@ func NewSqlChannelMemberHistoryStore(sqlStore SqlStore) store.ChannelMemberHisto return s } -func (s SqlChannelMemberHistoryStore) LogJoinEvent(userId string, channelId string, joinTime int64) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - channelMemberHistory := &model.ChannelMemberHistory{ - UserId: userId, - ChannelId: channelId, - JoinTime: joinTime, - } +func (s SqlChannelMemberHistoryStore) LogJoinEvent(userId string, channelId string, joinTime int64) *model.AppError { + channelMemberHistory := &model.ChannelMemberHistory{ + UserId: userId, + ChannelId: channelId, + JoinTime: joinTime, + } - if err := s.GetMaster().Insert(channelMemberHistory); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.LogJoinEvent", "store.sql_channel_member_history.log_join_event.app_error", nil, err.Error(), http.StatusInternalServerError) - } - }) + if err := s.GetMaster().Insert(channelMemberHistory); err != nil { + return model.NewAppError("SqlChannelMemberHistoryStore.LogJoinEvent", "store.sql_channel_member_history.log_join_event.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return nil } -func (s SqlChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId string, leaveTime int64) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - query := ` - UPDATE ChannelMemberHistory - SET LeaveTime = :LeaveTime - WHERE UserId = :UserId - AND ChannelId = :ChannelId - AND LeaveTime IS NULL` +func (s SqlChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId string, leaveTime int64) *model.AppError { + query := ` + UPDATE ChannelMemberHistory + SET LeaveTime = :LeaveTime + WHERE UserId = :UserId + AND ChannelId = :ChannelId + AND LeaveTime IS NULL` - params := map[string]interface{}{"UserId": userId, "ChannelId": channelId, "LeaveTime": leaveTime} - if sqlResult, err := s.GetMaster().Exec(query, params); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.LogLeaveEvent", "store.sql_channel_member_history.log_leave_event.update_error", params, err.Error(), http.StatusInternalServerError) - } else if rows, err := sqlResult.RowsAffected(); err == nil && rows != 1 { - // there was no join event to update - this is best effort, so no need to raise an error - mlog.Warn(fmt.Sprintf("Channel join event for user %v and channel %v not found", userId, channelId), mlog.String("user_id", userId)) - } - }) + params := map[string]interface{}{"UserId": userId, "ChannelId": channelId, "LeaveTime": leaveTime} + sqlResult, err := s.GetMaster().Exec(query, params) + if err != nil { + return model.NewAppError("SqlChannelMemberHistoryStore.LogLeaveEvent", "store.sql_channel_member_history.log_leave_event.update_error", params, err.Error(), http.StatusInternalServerError) + } + + if rows, err := sqlResult.RowsAffected(); err == nil && rows != 1 { + // there was no join event to update - this is best effort, so no need to raise an error + mlog.Warn(fmt.Sprintf("Channel join event for user %v and channel %v not found", userId, channelId), mlog.String("user_id", userId)) + } + return nil } -func (s SqlChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if useChannelMemberHistory, err := s.hasDataAtOrBefore(startTime); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err.Error(), http.StatusInternalServerError) - } else if useChannelMemberHistory { - // the export period starts after the ChannelMemberHistory table was first introduced, so we can use the - // data from it for our export - if channelMemberHistories, err := s.getFromChannelMemberHistoryTable(startTime, endTime, channelId); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err.Error(), http.StatusInternalServerError) - } else { - result.Data = channelMemberHistories - } - } else { - // the export period starts before the ChannelMemberHistory table was introduced, so we need to fake the - // data by assuming that anybody who has ever joined the channel in question was present during the export period. - // this may not always be true, but it's better than saying that somebody wasn't there when they were - if channelMemberHistories, err := s.getFromChannelMembersTable(startTime, endTime, channelId); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err.Error(), http.StatusInternalServerError) - } else { - result.Data = channelMemberHistories - } +func (s SqlChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) ([]*model.ChannelMemberHistoryResult, *model.AppError) { + useChannelMemberHistory, err := s.hasDataAtOrBefore(startTime) + if err != nil { + return nil, model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + if useChannelMemberHistory { + // the export period starts after the ChannelMemberHistory table was first introduced, so we can use the + // data from it for our export + channelMemberHistories, err2 := s.getFromChannelMemberHistoryTable(startTime, endTime, channelId) + if err2 != nil { + return nil, model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err2.Error(), http.StatusInternalServerError) } - }) + return channelMemberHistories, nil + } + + // the export period starts before the ChannelMemberHistory table was introduced, so we need to fake the + // data by assuming that anybody who has ever joined the channel in question was present during the export period. + // this may not always be true, but it's better than saying that somebody wasn't there when they were + channelMemberHistories, err := s.getFromChannelMembersTable(startTime, endTime, channelId) + if err != nil { + return nil, model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return channelMemberHistories, nil } func (s SqlChannelMemberHistoryStore) hasDataAtOrBefore(time int64) (bool, error) { @@ -159,35 +161,34 @@ func (s SqlChannelMemberHistoryStore) getFromChannelMembersTable(startTime int64 } } -func (s SqlChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit int64) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var query string - if s.DriverName() == model.DATABASE_DRIVER_POSTGRES { - query = - `DELETE FROM ChannelMemberHistory +func (s SqlChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { + var query string + if s.DriverName() == model.DATABASE_DRIVER_POSTGRES { + query = + `DELETE FROM ChannelMemberHistory WHERE ctid IN ( SELECT ctid FROM ChannelMemberHistory WHERE LeaveTime IS NOT NULL AND LeaveTime <= :EndTime LIMIT :Limit );` - } else { - query = - `DELETE FROM ChannelMemberHistory + } else { + query = + `DELETE FROM ChannelMemberHistory WHERE LeaveTime IS NOT NULL AND LeaveTime <= :EndTime LIMIT :Limit` - } + } - params := map[string]interface{}{"EndTime": endTime, "Limit": limit} - if sqlResult, err := s.GetMaster().Exec(query, params); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.PermanentDeleteBatchForChannel", "store.sql_channel_member_history.permanent_delete_batch.app_error", params, err.Error(), http.StatusInternalServerError) - } else { - if rowsAffected, err1 := sqlResult.RowsAffected(); err1 != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.PermanentDeleteBatchForChannel", "store.sql_channel_member_history.permanent_delete_batch.app_error", params, err.Error(), http.StatusInternalServerError) - } else { - result.Data = rowsAffected - } - } - }) + params := map[string]interface{}{"EndTime": endTime, "Limit": limit} + sqlResult, err := s.GetMaster().Exec(query, params) + if err != nil { + return int64(0), model.NewAppError("SqlChannelMemberHistoryStore.PermanentDeleteBatchForChannel", "store.sql_channel_member_history.permanent_delete_batch.app_error", params, err.Error(), http.StatusInternalServerError) + } + + rowsAffected, err := sqlResult.RowsAffected() + if err != nil { + return int64(0), model.NewAppError("SqlChannelMemberHistoryStore.PermanentDeleteBatchForChannel", "store.sql_channel_member_history.permanent_delete_batch.app_error", params, err.Error(), http.StatusInternalServerError) + } + return rowsAffected, nil } diff --git a/store/store.go b/store/store.go index 819d4cc040..60f7886da2 100644 --- a/store/store.go +++ b/store/store.go @@ -205,10 +205,10 @@ type ChannelStore interface { } type ChannelMemberHistoryStore interface { - LogJoinEvent(userId string, channelId string, joinTime int64) StoreChannel - LogLeaveEvent(userId string, channelId string, leaveTime int64) StoreChannel - GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) StoreChannel - PermanentDeleteBatch(endTime int64, limit int64) StoreChannel + LogJoinEvent(userId string, channelId string, joinTime int64) *model.AppError + LogLeaveEvent(userId string, channelId string, leaveTime int64) *model.AppError + GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) ([]*model.ChannelMemberHistoryResult, *model.AppError) + PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) } type PostStore interface { diff --git a/store/storetest/channel_member_history_store.go b/store/storetest/channel_member_history_store.go index 6ae50cbf56..d49119b499 100644 --- a/store/storetest/channel_member_history_store.go +++ b/store/storetest/channel_member_history_store.go @@ -42,8 +42,8 @@ func testLogJoinEvent(t *testing.T, ss store.Store) { user = *store.Must(ss.User().Save(&user)).(*model.User) // log a join event - result := <-ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) - assert.Nil(t, result.Err) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) + assert.Nil(t, err) } func testLogLeaveEvent(t *testing.T, ss store.Store) { @@ -66,11 +66,11 @@ func testLogLeaveEvent(t *testing.T, ss store.Store) { user = *store.Must(ss.User().Save(&user)).(*model.User) // log a join event, followed by a leave event - result := <-ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) - assert.Nil(t, result.Err) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) + assert.Nil(t, err) - result = <-ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, model.GetMillis()) - assert.Nil(t, result.Err) + err = ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, model.GetMillis()) + assert.Nil(t, err) } func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { @@ -97,20 +97,25 @@ func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { // us from looking in the ChannelMembers table for data that isn't found in the ChannelMemberHistory table leaveTime := model.GetMillis() - 20000 joinTime := leaveTime - 10000 - store.Must(ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime)) - store.Must(ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime)) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime) + require.Nil(t, err) + err = ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime) + require.Nil(t, err) // log a join event leaveTime = model.GetMillis() joinTime = leaveTime - 10000 - store.Must(ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime)) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime) + require.Nil(t, err) // case 1: user joins and leaves the channel before the export period begins - channelMembers := store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-500, joinTime-100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err := ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-500, joinTime-100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 0) // case 2: user joins the channel after the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, joinTime+500, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, joinTime+500, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -120,7 +125,8 @@ func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { assert.Nil(t, channelMembers[0].LeaveTime) // case 3: user joins the channel before the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, joinTime+500, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, joinTime+500, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -130,10 +136,12 @@ func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { assert.Nil(t, channelMembers[0].LeaveTime) // add a leave time for the user - store.Must(ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime)) + err = ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime) + require.Nil(t, err) // case 4: user joins the channel before the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, leaveTime-100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, leaveTime-100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -143,7 +151,8 @@ func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { assert.Equal(t, leaveTime, *channelMembers[0].LeaveTime) // case 5: user joins the channel after the export period begins, and leaves the channel before the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, leaveTime+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, leaveTime+100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -153,7 +162,8 @@ func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { assert.Equal(t, leaveTime, *channelMembers[0].LeaveTime) // case 6: user has joined and left the channel long before the export period begins - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(leaveTime+100, leaveTime+200, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(leaveTime+100, leaveTime+200, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 0) } @@ -179,11 +189,10 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { // clear any existing ChannelMemberHistory data that might interfere with our test var tableDataTruncated = false for !tableDataTruncated { - if result := <-ss.ChannelMemberHistory().PermanentDeleteBatch(model.GetMillis(), 1000); result.Err != nil { - assert.Fail(t, "Failed to truncate ChannelMemberHistory contents", result.Err.Error()) - } else { - tableDataTruncated = result.Data.(int64) == int64(0) - } + var count int64 + count, err = ss.ChannelMemberHistory().PermanentDeleteBatch(model.GetMillis(), 1000) + require.Nil(t, err, "Failed to truncate ChannelMemberHistory contents") + tableDataTruncated = count == int64(0) } // in this test, we're pretending that Message Export was not activated during the export period, so there's no data @@ -200,7 +209,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { // the past, even though the time that they were actually in the channel doesn't necessarily overlap with the export period // case 1: user joins and leaves the channel before the export period begins - channelMembers := store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-500, joinTime-100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err := ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-500, joinTime-100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -210,7 +220,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { assert.Equal(t, joinTime-100, *channelMembers[0].LeaveTime) // case 2: user joins the channel after the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, joinTime+500, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, joinTime+500, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -220,7 +231,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { assert.Equal(t, joinTime+500, *channelMembers[0].LeaveTime) // case 3: user joins the channel before the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, joinTime+500, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, joinTime+500, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -230,7 +242,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { assert.Equal(t, joinTime+500, *channelMembers[0].LeaveTime) // case 4: user joins the channel before the export period begins, but has not yet left the channel when the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, leaveTime-100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+100, leaveTime-100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -240,7 +253,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { assert.Equal(t, leaveTime-100, *channelMembers[0].LeaveTime) // case 5: user joins the channel after the export period begins, and leaves the channel before the export period ends - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, leaveTime+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, leaveTime+100, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -250,7 +264,8 @@ func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { assert.Equal(t, leaveTime+100, *channelMembers[0].LeaveTime) // case 6: user has joined and left the channel long before the export period begins - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(leaveTime+100, leaveTime+200, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(leaveTime+100, leaveTime+200, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -289,22 +304,28 @@ func testPermanentDeleteBatch(t *testing.T, ss store.Store) { // user1 joins and leaves the channel leaveTime := model.GetMillis() joinTime := leaveTime - 10000 - store.Must(ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime)) - store.Must(ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime)) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime) + require.Nil(t, err) + err = ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime) + require.Nil(t, err) // user2 joins the channel but never leaves - store.Must(ss.ChannelMemberHistory().LogJoinEvent(user2.Id, channel.Id, joinTime)) + err = ss.ChannelMemberHistory().LogJoinEvent(user2.Id, channel.Id, joinTime) + require.Nil(t, err) // in between the join time and the leave time, both users were members of the channel - channelMembers := store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+10, leaveTime-10, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err := ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+10, leaveTime-10, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 2) // the permanent delete should delete at least one record - rowsDeleted := store.Must(ss.ChannelMemberHistory().PermanentDeleteBatch(leaveTime, math.MaxInt64)).(int64) + rowsDeleted, err := ss.ChannelMemberHistory().PermanentDeleteBatch(leaveTime, math.MaxInt64) + require.Nil(t, err) assert.NotEqual(t, int64(0), rowsDeleted) // after the delete, there should be one less member in the channel - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+10, leaveTime-10, channel.Id)).([]*model.ChannelMemberHistoryResult) + channelMembers, err = ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime+10, leaveTime-10, channel.Id) + require.Nil(t, err) assert.Len(t, channelMembers, 1) assert.Equal(t, user2.Id, channelMembers[0].UserId) } diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 9d0c080a90..0b15e3b4f6 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -1236,22 +1236,22 @@ func testPendingAutoAddChannelMembers(t *testing.T, ss store.Store) { require.Len(t, channelMembers, 1) // Adding Channel (ChannelMemberHistory) should stop returning result - res = <-ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) - require.Nil(t, res.Err) + err = ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, model.GetMillis()) + require.Nil(t, err) channelMembers, err = ss.Group().ChannelMembersToAdd(0) require.Nil(t, err) require.Len(t, channelMembers, 0) // Leaving Channel (ChannelMemberHistory) should still not return result - res = <-ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, model.GetMillis()) - require.Nil(t, res.Err) + err = ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, model.GetMillis()) + require.Nil(t, err) channelMembers, err = ss.Group().ChannelMembersToAdd(0) require.Nil(t, err) require.Len(t, channelMembers, 0) // Purging ChannelMemberHistory re-returns the result - res = <-ss.ChannelMemberHistory().PermanentDeleteBatch(model.GetMillis()+1, 100) - require.Nil(t, res.Err) + _, err = ss.ChannelMemberHistory().PermanentDeleteBatch(model.GetMillis()+1, 100) + require.Nil(t, err) channelMembers, err = ss.Group().ChannelMembersToAdd(0) require.Nil(t, err) require.Len(t, channelMembers, 1) diff --git a/store/storetest/mocks/ChannelMemberHistoryStore.go b/store/storetest/mocks/ChannelMemberHistoryStore.go index ae8d024d19..9ceac5cbff 100644 --- a/store/storetest/mocks/ChannelMemberHistoryStore.go +++ b/store/storetest/mocks/ChannelMemberHistoryStore.go @@ -5,7 +5,7 @@ package mocks import mock "github.com/stretchr/testify/mock" -import store "github.com/mattermost/mattermost-server/store" +import model "github.com/mattermost/mattermost-server/model" // ChannelMemberHistoryStore is an autogenerated mock type for the ChannelMemberHistoryStore type type ChannelMemberHistoryStore struct { @@ -13,31 +13,40 @@ type ChannelMemberHistoryStore struct { } // GetUsersInChannelDuring provides a mock function with given fields: startTime, endTime, channelId -func (_m *ChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) store.StoreChannel { +func (_m *ChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) ([]*model.ChannelMemberHistoryResult, *model.AppError) { ret := _m.Called(startTime, endTime, channelId) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(int64, int64, string) store.StoreChannel); ok { + var r0 []*model.ChannelMemberHistoryResult + if rf, ok := ret.Get(0).(func(int64, int64, string) []*model.ChannelMemberHistoryResult); ok { r0 = rf(startTime, endTime, channelId) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).([]*model.ChannelMemberHistoryResult) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(int64, int64, string) *model.AppError); ok { + r1 = rf(startTime, endTime, channelId) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // LogJoinEvent provides a mock function with given fields: userId, channelId, joinTime -func (_m *ChannelMemberHistoryStore) LogJoinEvent(userId string, channelId string, joinTime int64) store.StoreChannel { +func (_m *ChannelMemberHistoryStore) LogJoinEvent(userId string, channelId string, joinTime int64) *model.AppError { ret := _m.Called(userId, channelId, joinTime) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string, int64) store.StoreChannel); ok { + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string, string, int64) *model.AppError); ok { r0 = rf(userId, channelId, joinTime) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.AppError) } } @@ -45,15 +54,15 @@ func (_m *ChannelMemberHistoryStore) LogJoinEvent(userId string, channelId strin } // LogLeaveEvent provides a mock function with given fields: userId, channelId, leaveTime -func (_m *ChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId string, leaveTime int64) store.StoreChannel { +func (_m *ChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId string, leaveTime int64) *model.AppError { ret := _m.Called(userId, channelId, leaveTime) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string, int64) store.StoreChannel); ok { + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string, string, int64) *model.AppError); ok { r0 = rf(userId, channelId, leaveTime) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.AppError) } } @@ -61,17 +70,24 @@ func (_m *ChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId stri } // PermanentDeleteBatch provides a mock function with given fields: endTime, limit -func (_m *ChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit int64) store.StoreChannel { +func (_m *ChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { ret := _m.Called(endTime, limit) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(int64, int64) store.StoreChannel); ok { + var r0 int64 + if rf, ok := ret.Get(0).(func(int64, int64) int64); ok { r0 = rf(endTime, limit) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(int64) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(int64, int64) *model.AppError); ok { + r1 = rf(endTime, limit) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) } } - return r0 + return r0, r1 }