From bbfcac84c95195557b792fada150725d0a4c4de0 Mon Sep 17 00:00:00 2001 From: jfrerich Date: Wed, 27 Feb 2019 20:51:06 -0600 Subject: [PATCH] [MM-13721] Fix the etag function GetEtagForProfilesNotInTeam (#10360) * Initial solution for Draft PR * Reformat tabs to spaces for readability * Remove my comments and debugging lines for Core Commiter to see changes more easily * Remove all comments and show only code changes * Match indentation spacing of new query to old query to make diff reading easier for core committer * Remove everything except what want to show core commiter * Restrucure query and resulting etag value. get number of users not in team from SELECT subquery. etag return values is now of the form .. * Remove skipped test for solution to https://mattermost.atlassian.net/browse/MM-13721. Remove comment for failing description of test store u4 with prepended "u4". Similar to u1, u2, u3. This is easier for debugging when looking in the database Added skipped test: check that etag does not change when a user, not in team 1, is added to different team. UpdateAt will change, but users in the set does not * Remove skipped test for solution to https://mattermost.atlassian.net/browse/MM-13721. Remove comment for failing description of test store u4 with prepended "u4". Similar to u1, u2, u3. This is easier for debugging when looking in the database Added skipped test: check that etag does not change when a user, not in team 1, is added to different team. UpdateAt will change, but users in the set does not * remove commented out tests * Restructure and simplify the SQL query for GetEtagForProfilesNotInTeam. - Build the query to get all profiles not in a specified team - select latest UpdateAt Value by getting Max value from UpdateAt field. - select Number of profiles not in Team from count of the returned Ids The previous query required building a complex query with multiple joins and repeated code in a select subquery, and derived table * Format SQL styling indentation, spaces around equal signs, and new lines * Add description for skipped test --- store/sqlstore/user_store.go | 28 +++++++++++++++------------- store/storetest/user_store.go | 27 +++++++++++++++++++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 4486ae8b32..f50a13beea 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1348,23 +1348,25 @@ func (us SqlUserStore) GetProfilesNotInTeam(teamId string, offset int, limit int func (us SqlUserStore) GetEtagForProfilesNotInTeam(teamId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { - updateAt, err := us.GetReplica().SelectInt(` - SELECT - u.UpdateAt - FROM Users u - LEFT JOIN TeamMembers tm - ON tm.UserId = u.Id - AND tm.TeamId = :TeamId - AND tm.DeleteAt = 0 - WHERE tm.UserId IS NULL - ORDER BY u.UpdateAt DESC - LIMIT 1 - `, map[string]interface{}{"TeamId": teamId}) + var querystr string + querystr = ` + SELECT + CONCAT(MAX(UpdateAt), '.', COUNT(Id)) as etag + FROM + Users as u + LEFT JOIN TeamMembers tm + ON tm.UserId = u.Id + AND tm.TeamId = :TeamId + AND tm.DeleteAt = 0 + WHERE + tm.UserId IS NULL + ` + etag, err := us.GetReplica().SelectStr(querystr, map[string]interface{}{"TeamId": teamId}) if err != nil { result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, model.GetMillis()) } else { - result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, updateAt) + result.Data = fmt.Sprintf("%v.%v", model.CurrentVersion, etag) } }) } diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 9910aba292..3138ceffb5 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -2891,10 +2891,7 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u2.Id}, -1)) u2.UpdateAt = store.Must(ss.User().UpdateUpdateAt(u2.Id)).(int64) - // GetEtagForProfilesNotInTeam only works if the most recent user is added to the team, - // otherwise the timestamp simply never changes: see https://mattermost.atlassian.net/browse/MM-13721. t.Run("etag for profiles not in team 1 after update", func(t *testing.T) { - t.Skip() result := <-ss.User().GetEtagForProfilesNotInTeam(teamId) require.Nil(t, result.Err) etag2 = result.Data.(string) @@ -2938,9 +2935,10 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { // Ensure update at timestamp changes time.Sleep(time.Millisecond * 10) - u4 := &model.User{} - u4.Email = MakeEmail() - store.Must(ss.User().Save(u4)) + u4 := store.Must(ss.User().Save(&model.User{ + Email: MakeEmail(), + Username: "u4" + model.NewId(), + })).(*model.User) defer func() { store.Must(ss.User().PermanentDelete(u4.Id)) }() store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u4.Id}, -1)) @@ -2950,6 +2948,23 @@ func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { etag4 := result.Data.(string) require.Equal(t, etag3, etag4, "etag should not have changed") }) + + // Add u3 to team 2 + store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId2, UserId: u3.Id}, -1)) + u3.UpdateAt = store.Must(ss.User().UpdateUpdateAt(u3.Id)).(int64) + + // GetEtagForProfilesNotInTeam produces a new etag every time a member, not + // in the team, gets a new UpdateAt value. In the case that an older member + // in the set joins a different team, their UpdateAt value changes, thus + // creating a new etag (even though the user set doesn't change). A hashing + // solution, which only uses UserIds, would solve this issue. + t.Run("etag for profiles not in team 1 after u3 added to team 2", func(t *testing.T) { + t.Skip() + result := <-ss.User().GetEtagForProfilesNotInTeam(teamId) + require.Nil(t, result.Err) + etag4 := result.Data.(string) + require.Equal(t, etag3, etag4, "etag should not have changed") + }) } func testUserStoreClearAllCustomRoleAssignments(t *testing.T, ss store.Store) {