break up getThreadsForUser, leverage errgroup (#19709)

* MM-42282: handle teamId parameter correctly

As per https://community-daily.mattermost.com/core/pl/ugs7ue6e4j8a7cgegk1bxje8to, `ThreadStore.GetThreadsForUser` accepts a `teamId` parameter, but incorrectly handles an empty value of `""` as looking only for channels with an empty `teamId` (aka DMs and GMs) instead of finding all channels and effectively ignoring the team property.

Fixes: https://mattermost.atlassian.net/browse/MM-42282

* break up getThreadsForUser, leverage errgroup

This change breaks up `GetThreadsForUser` in the `ThreadStore` into its constituent `GetTotalUnreadThreads`, `GetTotalThreads`, `GetTotalUnreadMentions`, and the original `GetThreadsForUser` but now solely returning the thread structures. Instead of a monolithic method at the store level, the application layer now handles calling bulk requests, leveraging `errgroup` for simpler parallelization.

This change brings with it a few benefits:
* Simpler code, including more idiomatic usage of squirrel
* Simpler SQL, joining tables only when configured conditions require same. (No performance benefit here, since an unused LEFT JOIN generally has no overhead.)
* Discrete Grafana metrics for each store method, giving us better insight into the performance characteristics in play.
* **Performance boost**: reduced overhead when clearing push notifications.

This last point is what prompted the re-re-reactoring in this PR. As I broke things up, I realized that `clearPushNotificationSync` only used the `TotalUnreadMentions`, but asked for the count of total threads and total unread threads. By exposing the discrete methods, this code path avoids two aggregate queries. We clear notifications when marking a thread as read, and when marking a channel with unread mentions as viewed, so I expect we'll see at least a modest boost to performance from simply not wasting these cycles anymore.

No performance improvements are expected from this PR for the general case of using `GetThreadsForUser` to populate the threads view.

* never discard errors from building queries

* no MustSql

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Этот коммит содержится в:
Jesse Hallam
2022-03-15 10:29:00 -03:00
коммит произвёл GitHub
родитель 4bf3d6a7f5
Коммит 47c44a9b7d
10 изменённых файлов: 850 добавлений и 307 удалений

Просмотреть файл

@@ -233,15 +233,15 @@ func (_m *ThreadStore) GetThreadUnreadReplyCount(threadMembership *model.ThreadM
}
// GetThreadsForUser provides a mock function with given fields: userId, teamID, opts
func (_m *ThreadStore) GetThreadsForUser(userId string, teamID string, opts model.GetUserThreadsOpts) (*model.Threads, error) {
func (_m *ThreadStore) GetThreadsForUser(userId string, teamID string, opts model.GetUserThreadsOpts) ([]*model.ThreadResponse, error) {
ret := _m.Called(userId, teamID, opts)
var r0 *model.Threads
if rf, ok := ret.Get(0).(func(string, string, model.GetUserThreadsOpts) *model.Threads); ok {
var r0 []*model.ThreadResponse
if rf, ok := ret.Get(0).(func(string, string, model.GetUserThreadsOpts) []*model.ThreadResponse); ok {
r0 = rf(userId, teamID, opts)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).(*model.Threads)
r0 = ret.Get(0).([]*model.ThreadResponse)
}
}
@@ -255,6 +255,69 @@ func (_m *ThreadStore) GetThreadsForUser(userId string, teamID string, opts mode
return r0, r1
}
// GetTotalThreads provides a mock function with given fields: userId, teamID, opts
func (_m *ThreadStore) GetTotalThreads(userId string, teamID string, opts model.GetUserThreadsOpts) (int64, error) {
ret := _m.Called(userId, teamID, opts)
var r0 int64
if rf, ok := ret.Get(0).(func(string, string, model.GetUserThreadsOpts) int64); ok {
r0 = rf(userId, teamID, opts)
} else {
r0 = ret.Get(0).(int64)
}
var r1 error
if rf, ok := ret.Get(1).(func(string, string, model.GetUserThreadsOpts) error); ok {
r1 = rf(userId, teamID, opts)
} else {
r1 = ret.Error(1)
}
return r0, r1
}
// GetTotalUnreadMentions provides a mock function with given fields: userId, teamID, opts
func (_m *ThreadStore) GetTotalUnreadMentions(userId string, teamID string, opts model.GetUserThreadsOpts) (int64, error) {
ret := _m.Called(userId, teamID, opts)
var r0 int64
if rf, ok := ret.Get(0).(func(string, string, model.GetUserThreadsOpts) int64); ok {
r0 = rf(userId, teamID, opts)
} else {
r0 = ret.Get(0).(int64)
}
var r1 error
if rf, ok := ret.Get(1).(func(string, string, model.GetUserThreadsOpts) error); ok {
r1 = rf(userId, teamID, opts)
} else {
r1 = ret.Error(1)
}
return r0, r1
}
// GetTotalUnreadThreads provides a mock function with given fields: userId, teamID, opts
func (_m *ThreadStore) GetTotalUnreadThreads(userId string, teamID string, opts model.GetUserThreadsOpts) (int64, error) {
ret := _m.Called(userId, teamID, opts)
var r0 int64
if rf, ok := ret.Get(0).(func(string, string, model.GetUserThreadsOpts) int64); ok {
r0 = rf(userId, teamID, opts)
} else {
r0 = ret.Get(0).(int64)
}
var r1 error
if rf, ok := ret.Get(1).(func(string, string, model.GetUserThreadsOpts) error); ok {
r1 = rf(userId, teamID, opts)
} else {
r1 = ret.Error(1)
}
return r0, r1
}
// MaintainMembership provides a mock function with given fields: userID, postID, opts
func (_m *ThreadStore) MaintainMembership(userID string, postID string, opts store.ThreadMembershipOpts) (*model.ThreadMembership, error) {
ret := _m.Called(userID, postID, opts)

Просмотреть файл

@@ -5,7 +5,9 @@ package storetest
import (
"context"
"sort"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -23,7 +25,7 @@ func TestThreadStore(t *testing.T, ss store.Store, s SqlStore) {
testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t, ss, s)
})
t.Run("GetTeamsUnreadForUser", func(t *testing.T) { testGetTeamsUnreadForUser(t, ss) })
t.Run("GetThreadsForUser", func(t *testing.T) { testGetThreadsForUser(t, ss) })
t.Run("GetVarious", func(t *testing.T) { testVarious(t, ss) })
t.Run("MarkAllAsReadByChannels", func(t *testing.T) { testMarkAllAsReadByChannels(t, ss) })
}
@@ -679,7 +681,35 @@ func testGetTeamsUnreadForUser(t *testing.T, ss store.Store) {
assert.Equal(t, int64(1), teamsUnread[team2.Id].ThreadMentionCount)
}
func testGetThreadsForUser(t *testing.T, ss store.Store) {
func testVarious(t *testing.T, ss store.Store) {
createThreadMembership := func(userID, postID string, isMention bool) {
t.Helper()
opts := store.ThreadMembershipOpts{
Following: true,
IncrementMentions: isMention,
UpdateFollowing: true,
UpdateViewedTimestamp: false,
UpdateParticipants: false,
}
_, err := ss.Thread().MaintainMembership(userID, postID, opts)
require.NoError(t, err)
}
viewThread := func(userID, postID string) {
t.Helper()
opts := store.ThreadMembershipOpts{
Following: true,
IncrementMentions: false,
UpdateFollowing: true,
UpdateViewedTimestamp: true,
UpdateParticipants: false,
}
_, err := ss.Thread().MaintainMembership(userID, postID, opts)
require.NoError(t, err)
}
user1, err := ss.User().Save(&model.User{
Username: "user1" + model.NewId(),
Email: MakeEmail(),
@@ -750,6 +780,13 @@ func testGetThreadsForUser(t *testing.T, ss store.Store) {
})
require.NoError(t, err)
team1channel1post3, err := ss.Post().Save(&model.Post{
ChannelId: team1channel1.Id,
UserId: user1ID,
Message: model.NewRandomString(10),
})
require.NoError(t, err)
team2channel1post1, err := ss.Post().Save(&model.Post{
ChannelId: team2channel1.Id,
UserId: user1ID,
@@ -757,6 +794,13 @@ func testGetThreadsForUser(t *testing.T, ss store.Store) {
})
require.NoError(t, err)
team2channel1post2deleted, err := ss.Post().Save(&model.Post{
ChannelId: team2channel1.Id,
UserId: user1ID,
Message: model.NewRandomString(10),
})
require.NoError(t, err)
dm1post1, err := ss.Post().Save(&model.Post{
ChannelId: dm1.Id,
UserId: user1ID,
@@ -773,61 +817,206 @@ func testGetThreadsForUser(t *testing.T, ss store.Store) {
threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post1.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post2.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post3.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, team2channel1.Id, team2channel1post1.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, team2channel1.Id, team2channel1post2deleted.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, dm1.Id, dm1post1.Id, user2ID, model.GetMillis())
threadStoreCreateReply(t, ss, gm1.Id, gm1post1.Id, user2ID, model.GetMillis())
createThreadMembership := func(userID, postID string) {
t.Helper()
// Create thread memberships, with simulated unread mentions.
createThreadMembership(user1ID, team1channel1post1.Id, false)
createThreadMembership(user1ID, team1channel1post2.Id, false)
createThreadMembership(user1ID, team1channel1post3.Id, true)
createThreadMembership(user1ID, team2channel1post1.Id, false)
createThreadMembership(user1ID, team2channel1post2deleted.Id, false)
createThreadMembership(user1ID, dm1post1.Id, false)
createThreadMembership(user1ID, gm1post1.Id, true)
opts := store.ThreadMembershipOpts{
Following: true,
IncrementMentions: false,
UpdateFollowing: true,
UpdateViewedTimestamp: false,
UpdateParticipants: false,
// Have user1 view a subset of the threads
viewThread(user1ID, team1channel1post1.Id)
viewThread(user2ID, team1channel1post2.Id)
viewThread(user1ID, team2channel1post1.Id)
viewThread(user1ID, dm1post1.Id)
// Add reply to a viewed thread to confirm it's unread again.
time.Sleep(1 * time.Millisecond)
threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post2.Id, user2ID, model.GetMillis())
err = ss.Post().Delete(team2channel1post2deleted.Id, model.GetMillis(), user1ID)
require.NoError(t, err)
t.Run("GetTotalUnreadThreads", func(t *testing.T) {
testCases := []struct {
Description string
UserID string
TeamID string
Options model.GetUserThreadsOpts
ExpectedThreads []*model.Post
}{
{"all teams, user1", user1ID, "", model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1,
}},
{"team1, user1", user1ID, team1.Id, model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1,
}},
{"team1, user1, deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1, // (no deleted threads in team1)
}},
{"team2, user1", user1ID, team2.Id, model.GetUserThreadsOpts{}, []*model.Post{
gm1post1, // (no unread threads in team2)
}},
{"team2, user1, deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team2channel1post2deleted, gm1post1,
}},
}
_, err := ss.Thread().MaintainMembership(userID, postID, opts)
require.NoError(t, err)
}
createThreadMembership(user1ID, team1channel1post1.Id)
createThreadMembership(user1ID, team1channel1post2.Id)
createThreadMembership(user1ID, team2channel1post1.Id)
createThreadMembership(user1ID, dm1post1.Id)
createThreadMembership(user1ID, gm1post1.Id)
for _, testCase := range testCases {
t.Run(testCase.Description, func(t *testing.T) {
totalUnreadThreads, err := ss.Thread().GetTotalUnreadThreads(testCase.UserID, testCase.TeamID, testCase.Options)
require.NoError(t, err)
t.Run("no team specified, user1", func(t *testing.T) {
threads, err := ss.Thread().GetThreadsForUser(user1ID, "", model.GetUserThreadsOpts{})
require.NoError(t, err)
// 2 threads from team1, 1 threads from team2, 1 dm thread, 1 gm thread
assert.EqualValues(t, 5, threads.Total)
assert.EqualValues(t, 5, threads.TotalUnreadThreads)
assert.EqualValues(t, 0, threads.TotalUnreadMentions)
assert.Len(t, threads.Threads, 5)
assert.EqualValues(t, int64(len(testCase.ExpectedThreads)), totalUnreadThreads)
})
}
})
t.Run("team1 specified, user1", func(t *testing.T) {
threads, err := ss.Thread().GetThreadsForUser(user1ID, team1.Id, model.GetUserThreadsOpts{})
require.NoError(t, err)
t.Run("GetTotalThreads", func(t *testing.T) {
testCases := []struct {
Description string
UserID string
TeamID string
Options model.GetUserThreadsOpts
// 2 threads from team1, 1 dm thread, 1 gm thread
assert.EqualValues(t, 4, threads.Total)
assert.EqualValues(t, 4, threads.TotalUnreadThreads)
assert.EqualValues(t, 0, threads.TotalUnreadMentions)
assert.Len(t, threads.Threads, 4)
ExpectedThreads []*model.Post
}{
{"all teams, user1", user1ID, "", model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, team2channel1post1, dm1post1, gm1post1,
}},
{"team1, user1", user1ID, team1.Id, model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1,
}},
{"team1, user1, unread", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1,
}},
{"team1, user1, deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1, // (no deleted threads in team1)
}},
{"team1, user1, unread + deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1, // (no deleted threads in team1)
}},
{"team2, user1", user1ID, team2.Id, model.GetUserThreadsOpts{}, []*model.Post{
team2channel1post1, dm1post1, gm1post1,
}},
{"team2, user1, unread", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{
gm1post1, // (no unread in team2)
}},
{"team2, user1, deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team2channel1post1, team2channel1post2deleted, dm1post1, gm1post1,
}},
{"team2, user1, unread + deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{
team2channel1post2deleted, gm1post1,
}},
}
for _, testCase := range testCases {
t.Run(testCase.Description, func(t *testing.T) {
totalThreads, err := ss.Thread().GetTotalThreads(testCase.UserID, testCase.TeamID, testCase.Options)
require.NoError(t, err)
assert.EqualValues(t, int64(len(testCase.ExpectedThreads)), totalThreads)
})
}
})
t.Run("team2 specified, user1", func(t *testing.T) {
threads, err := ss.Thread().GetThreadsForUser(user1ID, team2.Id, model.GetUserThreadsOpts{})
require.NoError(t, err)
t.Run("GetTotalUnreadMentions", func(t *testing.T) {
testCases := []struct {
Description string
UserID string
TeamID string
Options model.GetUserThreadsOpts
// 1 thread from team1, 1 dm thread, 1 gm thread
assert.EqualValues(t, 3, threads.Total)
assert.EqualValues(t, 3, threads.TotalUnreadThreads)
assert.EqualValues(t, 0, threads.TotalUnreadMentions)
assert.Len(t, threads.Threads, 3)
ExpectedThreads []*model.Post
}{
{"all teams, user1", user1ID, "", model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post3, gm1post1,
}},
{"team1, user1", user1ID, team1.Id, model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post3, gm1post1,
}},
{"team2, user1", user1ID, team2.Id, model.GetUserThreadsOpts{}, []*model.Post{
gm1post1,
}},
}
for _, testCase := range testCases {
t.Run(testCase.Description, func(t *testing.T) {
totalUnreadMentions, err := ss.Thread().GetTotalUnreadMentions(testCase.UserID, testCase.TeamID, testCase.Options)
require.NoError(t, err)
assert.EqualValues(t, int64(len(testCase.ExpectedThreads)), totalUnreadMentions)
})
}
})
t.Run("GetThreadsForUser", func(t *testing.T) {
testCases := []struct {
Description string
UserID string
TeamID string
Options model.GetUserThreadsOpts
ExpectedThreads []*model.Post
}{
{"all teams, user1", user1ID, "", model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, team2channel1post1, dm1post1, gm1post1,
}},
{"team1, user1", user1ID, team1.Id, model.GetUserThreadsOpts{}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1,
}},
{"team1, user1, unread", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1,
}},
{"team1, user1, deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1, // (no deleted threads in team1)
}},
{"team1, user1, unread + deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{
team1channel1post2, team1channel1post3, gm1post1, // (no deleted threads in team1)
}},
{"team2, user1", user1ID, team2.Id, model.GetUserThreadsOpts{}, []*model.Post{
team2channel1post1, dm1post1, gm1post1,
}},
{"team2, user1, unread", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{
gm1post1, // (no unread in team2)
}},
{"team2, user1, deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{
team2channel1post1, team2channel1post2deleted, dm1post1, gm1post1,
}},
{"team2, user1, unread + deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{
team2channel1post2deleted, gm1post1,
}},
}
for _, testCase := range testCases {
t.Run(testCase.Description, func(t *testing.T) {
threads, err := ss.Thread().GetThreadsForUser(testCase.UserID, testCase.TeamID, testCase.Options)
require.NoError(t, err)
postIDs := make([]string, 0, len(threads))
for _, thread := range threads {
postIDs = append(postIDs, thread.PostId)
}
sort.Strings(postIDs)
expectedPostIDs := make([]string, 0, len(testCase.ExpectedThreads))
for _, post := range testCase.ExpectedThreads {
expectedPostIDs = append(expectedPostIDs, post.Id)
}
sort.Strings(expectedPostIDs)
assert.Equal(t, expectedPostIDs, postIDs)
})
}
})
}