From ab30c4daf935d33b7e6088d82eb7c49dc2717918 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 12 Dec 2017 09:42:29 -0500 Subject: PLT-8297: Message Export Should Still Produce Valid Exports if ChannelMemberHistory Data is Missing (#7951) * Added a less accurate ChannelMembers fallback that is used if export period occurs before MessageExport was enabled * Fixed test names * Improved testing * Made hasDataFromBefore() a little less strict * Fixed the test to cleanly truncate the ChannelMemberHistory table without exposing the db via the interface * Renamed a function for better clarity * Binary logic is hard --- store/sqlstore/channel_member_history_store.go | 82 +++++++++++++-- store/storetest/channel_member_history_store.go | 132 +++++++++++++++++++++--- 2 files changed, 191 insertions(+), 23 deletions(-) (limited to 'store') diff --git a/store/sqlstore/channel_member_history_store.go b/store/sqlstore/channel_member_history_store.go index aa5037d32..182f37ce9 100644 --- a/store/sqlstore/channel_member_history_store.go +++ b/store/sqlstore/channel_member_history_store.go @@ -6,6 +6,8 @@ package sqlstore import ( "net/http" + "database/sql" + l4g "github.com/alecthomas/log4go" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" @@ -65,7 +67,47 @@ func (s SqlChannelMemberHistoryStore) LogLeaveEvent(userId string, channelId str func (s SqlChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, endTime int64, channelId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - query := ` + 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) hasDataAtOrBefore(time int64) (bool, error) { + type NullableCountResult struct { + Min sql.NullInt64 + } + var result NullableCountResult + query := "SELECT MIN(JoinTime) AS Min FROM ChannelMemberHistory" + if err := s.GetReplica().SelectOne(&result, query); err != nil { + return false, err + } else if result.Min.Valid { + return result.Min.Int64 <= time, nil + } else { + // if the result was null, there are no rows in the table, so there is no data from before + return false, nil + } +} + +func (s SqlChannelMemberHistoryStore) getFromChannelMemberHistoryTable(startTime int64, endTime int64, channelId string) ([]*model.ChannelMemberHistory, error) { + query := ` SELECT cmh.*, u.Email @@ -76,14 +118,37 @@ func (s SqlChannelMemberHistoryStore) GetUsersInChannelDuring(startTime int64, e AND (cmh.LeaveTime IS NULL OR cmh.LeaveTime >= :StartTime) ORDER BY cmh.JoinTime ASC` - params := map[string]interface{}{"ChannelId": channelId, "StartTime": startTime, "EndTime": endTime} - var histories []*model.ChannelMemberHistory - if _, err := s.GetReplica().Select(&histories, query, params); err != nil { - result.Err = model.NewAppError("SqlChannelMemberHistoryStore.GetUsersInChannelAt", "store.sql_channel_member_history.get_users_in_channel_during.app_error", params, err.Error(), http.StatusInternalServerError) - } else { - result.Data = histories + params := map[string]interface{}{"ChannelId": channelId, "StartTime": startTime, "EndTime": endTime} + var histories []*model.ChannelMemberHistory + if _, err := s.GetReplica().Select(&histories, query, params); err != nil { + return nil, err + } else { + return histories, nil + } +} + +func (s SqlChannelMemberHistoryStore) getFromChannelMembersTable(startTime int64, endTime int64, channelId string) ([]*model.ChannelMemberHistory, error) { + query := ` + SELECT DISTINCT + ch.ChannelId, + ch.UserId, + u.email + FROM ChannelMembers AS ch + INNER JOIN Users AS u ON ch.UserId = u.id + WHERE ch.ChannelId = :ChannelId` + + params := map[string]interface{}{"ChannelId": channelId} + var histories []*model.ChannelMemberHistory + if _, err := s.GetReplica().Select(&histories, query, params); err != nil { + return nil, err + } else { + // we have to fill in the join/leave times, because that data doesn't exist in the channel members table + for _, channelMemberHistory := range histories { + channelMemberHistory.JoinTime = startTime + channelMemberHistory.LeaveTime = model.NewInt64(endTime) } - }) + return histories, nil + } } func (s SqlChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit int64) store.StoreChannel { @@ -112,7 +177,6 @@ func (s SqlChannelMemberHistoryStore) PermanentDeleteBatch(endTime int64, limit } 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) - result.Data = int64(0) } else { result.Data = rowsAffected } diff --git a/store/storetest/channel_member_history_store.go b/store/storetest/channel_member_history_store.go index 0be92c6e0..6fe73478c 100644 --- a/store/storetest/channel_member_history_store.go +++ b/store/storetest/channel_member_history_store.go @@ -14,10 +14,11 @@ import ( ) func TestChannelMemberHistoryStore(t *testing.T, ss store.Store) { - t.Run("Log Join Event", func(t *testing.T) { testLogJoinEvent(t, ss) }) - t.Run("Log Leave Event", func(t *testing.T) { testLogLeaveEvent(t, ss) }) - t.Run("Get Users In Channel At Time", func(t *testing.T) { testGetUsersInChannelAt(t, ss) }) - t.Run("Purge History", func(t *testing.T) { testPermanentDeleteBatch(t, ss) }) + t.Run("TestLogJoinEvent", func(t *testing.T) { testLogJoinEvent(t, ss) }) + t.Run("TestLogLeaveEvent", func(t *testing.T) { testLogLeaveEvent(t, ss) }) + t.Run("TestGetUsersInChannelAtChannelMemberHistory", func(t *testing.T) { testGetUsersInChannelAtChannelMemberHistory(t, ss) }) + t.Run("TestGetUsersInChannelAtChannelMembers", func(t *testing.T) { testGetUsersInChannelAtChannelMembers(t, ss) }) + t.Run("TestPermanentDeleteBatch", func(t *testing.T) { testPermanentDeleteBatch(t, ss) }) } func testLogJoinEvent(t *testing.T, ss store.Store) { @@ -67,7 +68,7 @@ func testLogLeaveEvent(t *testing.T, ss store.Store) { assert.Nil(t, result.Err) } -func testGetUsersInChannelAt(t *testing.T, ss store.Store) { +func testGetUsersInChannelAtChannelMemberHistory(t *testing.T, ss store.Store) { // create a test channel channel := model.Channel{ TeamId: model.NewId(), @@ -84,17 +85,25 @@ func testGetUsersInChannelAt(t *testing.T, ss store.Store) { } user = *store.Must(ss.User().Save(&user)).(*model.User) - // log a join event - leaveTime := model.GetMillis() + // the user was previously in the channel a long time ago, before the export period starts + // the existence of this record makes it look like the MessageExport feature has been active for awhile, and prevents + // 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)) + + // log a join event + leaveTime = model.GetMillis() + joinTime = leaveTime - 10000 + store.Must(ss.ChannelMemberHistory().LogJoinEvent(user.Id, channel.Id, joinTime)) - // case 1: both start and end before join time + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 0) - // case 2: start before join time, no leave time - channelMembers = store.Must(ss.ChannelMemberHistory().GetUsersInChannelDuring(joinTime-100, joinTime+100, channel.Id)).([]*model.ChannelMemberHistory) + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) assert.Equal(t, user.Id, channelMembers[0].UserId) @@ -102,7 +111,7 @@ func testGetUsersInChannelAt(t *testing.T, ss store.Store) { assert.Equal(t, joinTime, channelMembers[0].JoinTime) assert.Nil(t, channelMembers[0].LeaveTime) - // case 3: start after join time, no leave time + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) @@ -114,7 +123,7 @@ func testGetUsersInChannelAt(t *testing.T, ss store.Store) { // add a leave time for the user store.Must(ss.ChannelMemberHistory().LogLeaveEvent(user.Id, channel.Id, leaveTime)) - // case 4: start after join time, end before leave time + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) @@ -123,7 +132,7 @@ func testGetUsersInChannelAt(t *testing.T, ss store.Store) { assert.Equal(t, joinTime, channelMembers[0].JoinTime) assert.Equal(t, leaveTime, *channelMembers[0].LeaveTime) - // case 5: start before join time, end after leave time + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 1) assert.Equal(t, channel.Id, channelMembers[0].ChannelId) @@ -132,11 +141,106 @@ func testGetUsersInChannelAt(t *testing.T, ss store.Store) { assert.Equal(t, joinTime, channelMembers[0].JoinTime) assert.Equal(t, leaveTime, *channelMembers[0].LeaveTime) - // case 6: start and end after leave time + // 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.ChannelMemberHistory) assert.Len(t, channelMembers, 0) } +func testGetUsersInChannelAtChannelMembers(t *testing.T, ss store.Store) { + // create a test channel + channel := model.Channel{ + TeamId: model.NewId(), + DisplayName: "Display " + model.NewId(), + Name: "zz" + model.NewId() + "b", + Type: model.CHANNEL_OPEN, + } + channel = *store.Must(ss.Channel().Save(&channel, -1)).(*model.Channel) + + // and a test user + user := model.User{ + Email: model.NewId() + "@mattermost.com", + Nickname: model.NewId(), + } + user = *store.Must(ss.User().Save(&user)).(*model.User) + + // 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) + } + } + + // in this test, we're pretending that Message Export was not activated during the export period, so there's no data + // available in the ChannelMemberHistory table. Instead, we'll fall back to the ChannelMembers table for a rough approximation + joinTime := int64(1000) + leaveTime := joinTime + 5000 + store.Must(ss.Channel().SaveMember(&model.ChannelMember{ + ChannelId: channel.Id, + UserId: user.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + })) + + // in every single case, the user will be included in the export, because ChannelMembers says they were in the channel at some point in + // 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, joinTime-500, channelMembers[0].JoinTime) + 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, joinTime-100, channelMembers[0].JoinTime) + 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, joinTime+100, channelMembers[0].JoinTime) + 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, joinTime+100, channelMembers[0].JoinTime) + 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, joinTime-100, channelMembers[0].JoinTime) + 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.ChannelMemberHistory) + assert.Len(t, channelMembers, 1) + assert.Equal(t, channel.Id, channelMembers[0].ChannelId) + assert.Equal(t, user.Id, channelMembers[0].UserId) + assert.Equal(t, user.Email, channelMembers[0].UserEmail) + assert.Equal(t, leaveTime+100, channelMembers[0].JoinTime) + assert.Equal(t, leaveTime+200, *channelMembers[0].LeaveTime) +} + func testPermanentDeleteBatch(t *testing.T, ss store.Store) { // create a test channel channel := model.Channel{ -- cgit v1.2.3-1-g7c22