From 76eee12ac878a3f0e9c21195474865b9aaab7118 Mon Sep 17 00:00:00 2001 From: Tomas <40318863+tomasmik@users.noreply.github.com> Date: Sat, 8 Jun 2019 07:32:23 +0300 Subject: [PATCH] MM-15850/MM-15851 GetPosts Before/After sync by default (#11035) --- app/post.go | 24 +---- store/sqlstore/post_store.go | 159 ++++++++++++++--------------- store/store.go | 4 +- store/storetest/mocks/PostStore.go | 38 +++++-- store/storetest/post_store.go | 40 +++----- 5 files changed, 128 insertions(+), 137 deletions(-) diff --git a/app/post.go b/app/post.go index b8085d0401..0ee99e979a 100644 --- a/app/post.go +++ b/app/post.go @@ -667,34 +667,18 @@ func (a *App) GetPermalinkPost(postId string, userId string) (*model.PostList, * } func (a *App) GetPostsBeforePost(channelId, postId string, page, perPage int) (*model.PostList, *model.AppError) { - result := <-a.Srv.Store.Post().GetPostsBefore(channelId, postId, perPage, page*perPage) - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.PostList), nil + return a.Srv.Store.Post().GetPostsBefore(channelId, postId, perPage, page*perPage) } func (a *App) GetPostsAfterPost(channelId, postId string, page, perPage int) (*model.PostList, *model.AppError) { - result := <-a.Srv.Store.Post().GetPostsAfter(channelId, postId, perPage, page*perPage) - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.PostList), nil + return a.Srv.Store.Post().GetPostsAfter(channelId, postId, perPage, page*perPage) } func (a *App) GetPostsAroundPost(postId, channelId string, offset, limit int, before bool) (*model.PostList, *model.AppError) { - var pchan store.StoreChannel if before { - pchan = a.Srv.Store.Post().GetPostsBefore(channelId, postId, limit, offset) - } else { - pchan = a.Srv.Store.Post().GetPostsAfter(channelId, postId, limit, offset) + return a.Srv.Store.Post().GetPostsBefore(channelId, postId, limit, offset) } - - result := <-pchan - if result.Err != nil { - return nil, result.Err - } - return result.Data.(*model.PostList), nil + return a.Srv.Store.Post().GetPostsAfter(channelId, postId, limit, offset) } func (a *App) DeletePost(postId, deleteByID string) (*model.Post, *model.AppError) { diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index bfda4fe494..6e3c6cfa32 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -581,98 +581,95 @@ func (s *SqlPostStore) GetPostsSince(channelId string, time int64, allowFromCach }) } -func (s *SqlPostStore) GetPostsBefore(channelId string, postId string, limit int, offset int) store.StoreChannel { +func (s *SqlPostStore) GetPostsBefore(channelId string, postId string, limit int, offset int) (*model.PostList, *model.AppError) { return s.getPostsAround(channelId, postId, limit, offset, true) } -func (s *SqlPostStore) GetPostsAfter(channelId string, postId string, limit int, offset int) store.StoreChannel { +func (s *SqlPostStore) GetPostsAfter(channelId string, postId string, limit int, offset int) (*model.PostList, *model.AppError) { return s.getPostsAround(channelId, postId, limit, offset, false) } -func (s *SqlPostStore) getPostsAround(channelId string, postId string, limit int, offset int, before bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var direction string - var sort string - if before { - direction = "<" - sort = "DESC" - } else { - direction = ">" - sort = "ASC" - } +func (s *SqlPostStore) getPostsAround(channelId string, postId string, limit int, offset int, before bool) (*model.PostList, *model.AppError) { + var direction string + var sort string + if before { + direction = "<" + sort = "DESC" + } else { + direction = ">" + sort = "ASC" + } - var posts []*model.Post - var parents []*model.Post - _, err1 := s.GetReplica().Select(&posts, - `SELECT - * - FROM - Posts - WHERE - CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = :PostId) - AND ChannelId = :ChannelId - AND DeleteAt = 0 - ORDER BY CreateAt `+sort+` - LIMIT :Limit - OFFSET :Offset`, - map[string]interface{}{"ChannelId": channelId, "PostId": postId, "Limit": limit, "Offset": offset}) - _, err2 := s.GetReplica().Select(&parents, - `SELECT - q2.* - FROM - Posts q2 - INNER JOIN - (SELECT DISTINCT - q3.Id, q3.RootId - FROM - (SELECT - Id, RootId - FROM - Posts - WHERE - CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = :PostId) - AND ChannelId = :ChannelId - AND DeleteAt = 0 - ORDER BY CreateAt `+sort+` - LIMIT :Limit OFFSET :Offset) q3 -- q3 contains the Id and RootId of every post in posts - ) q1 -- q1 is q3 with the duplicates removed - ON q1.RootId = q2.Id -- This is the root post of a thread that appears in posts - OR q1.Id = q2.RootId -- This is a comment on a post in posts - OR (q2.RootId != '' AND q1.RootId = q2.RootId) -- This is a comment on a thread that appears in posts - WHERE - ChannelId = :ChannelId + var posts []*model.Post + var parents []*model.Post + _, err := s.GetReplica().Select(&posts, + `SELECT + * + FROM + Posts + WHERE + CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = :PostId) + AND ChannelId = :ChannelId AND DeleteAt = 0 - ORDER BY CreateAt DESC`, - map[string]interface{}{"ChannelId": channelId, "PostId": postId, "Limit": limit, "Offset": offset}) + ORDER BY CreateAt `+sort+` + LIMIT :Limit + OFFSET :Offset`, + map[string]interface{}{"ChannelId": channelId, "PostId": postId, "Limit": limit, "Offset": offset}) + if err != nil { + return nil, model.NewAppError("SqlPostStore.GetPostContext", "store.sql_post.get_posts_around.get.app_error", nil, "channelId="+channelId+err.Error(), http.StatusInternalServerError) + } + _, err = s.GetReplica().Select(&parents, + `SELECT + q2.* + FROM + Posts q2 + INNER JOIN + (SELECT DISTINCT + q3.Id, q3.RootId + FROM + (SELECT + Id, RootId + FROM + Posts + WHERE + CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = :PostId) + AND ChannelId = :ChannelId + AND DeleteAt = 0 + ORDER BY CreateAt `+sort+` + LIMIT :Limit OFFSET :Offset) q3 -- q3 contains the Id and RootId of every post in posts + ) q1 -- q1 is q3 with the duplicates removed + ON q1.RootId = q2.Id -- This is the root post of a thread that appears in posts + OR q1.Id = q2.RootId -- This is a comment on a post in posts + OR (q2.RootId != '' AND q1.RootId = q2.RootId) -- This is a comment on a thread that appears in posts + WHERE + ChannelId = :ChannelId + AND DeleteAt = 0 + ORDER BY CreateAt DESC`, + map[string]interface{}{"ChannelId": channelId, "PostId": postId, "Limit": limit, "Offset": offset}) - if err1 != nil { - result.Err = model.NewAppError("SqlPostStore.GetPostContext", "store.sql_post.get_posts_around.get.app_error", nil, "channelId="+channelId+err1.Error(), http.StatusInternalServerError) - } else if err2 != nil { - result.Err = model.NewAppError("SqlPostStore.GetPostContext", "store.sql_post.get_posts_around.get_parent.app_error", nil, "channelId="+channelId+err2.Error(), http.StatusInternalServerError) - } else { - list := model.NewPostList() + if err != nil { + return nil, model.NewAppError("SqlPostStore.GetPostContext", "store.sql_post.get_posts_around.get_parent.app_error", nil, "channelId="+channelId+err.Error(), http.StatusInternalServerError) + } + list := model.NewPostList() - // We need to flip the order if we selected backwards - if before { - for _, p := range posts { - list.AddPost(p) - list.AddOrder(p.Id) - } - } else { - l := len(posts) - for i := range posts { - list.AddPost(posts[l-i-1]) - list.AddOrder(posts[l-i-1].Id) - } - } - - for _, p := range parents { - list.AddPost(p) - } - - result.Data = list + // We need to flip the order if we selected backwards + if before { + for _, p := range posts { + list.AddPost(p) + list.AddOrder(p.Id) } - }) + } else { + l := len(posts) + for i := range posts { + list.AddPost(posts[l-i-1]) + list.AddOrder(posts[l-i-1].Id) + } + } + + for _, p := range parents { + list.AddPost(p) + } + return list, nil } func (s *SqlPostStore) getRootPosts(channelId string, offset int, limit int) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 9575fc5372..afc998f62b 100644 --- a/store/store.go +++ b/store/store.go @@ -221,8 +221,8 @@ type PostStore interface { GetFlaggedPosts(userId string, offset int, limit int) (*model.PostList, *model.AppError) GetFlaggedPostsForTeam(userId, teamId string, offset int, limit int) (*model.PostList, *model.AppError) GetFlaggedPostsForChannel(userId, channelId string, offset int, limit int) (*model.PostList, *model.AppError) - GetPostsBefore(channelId string, postId string, numPosts int, offset int) StoreChannel - GetPostsAfter(channelId string, postId string, numPosts int, offset int) StoreChannel + GetPostsBefore(channelId string, postId string, numPosts int, offset int) (*model.PostList, *model.AppError) + GetPostsAfter(channelId string, postId string, numPosts int, offset int) (*model.PostList, *model.AppError) GetPostsSince(channelId string, time int64, allowFromCache bool) StoreChannel GetEtag(channelId string, allowFromCache bool) string Search(teamId string, userId string, params *model.SearchParams) StoreChannel diff --git a/store/storetest/mocks/PostStore.go b/store/storetest/mocks/PostStore.go index 989f51e01e..76a5e8812b 100644 --- a/store/storetest/mocks/PostStore.go +++ b/store/storetest/mocks/PostStore.go @@ -302,19 +302,28 @@ func (_m *PostStore) GetPosts(channelId string, offset int, limit int, allowFrom } // GetPostsAfter provides a mock function with given fields: channelId, postId, numPosts, offset -func (_m *PostStore) GetPostsAfter(channelId string, postId string, numPosts int, offset int) store.StoreChannel { +func (_m *PostStore) GetPostsAfter(channelId string, postId string, numPosts int, offset int) (*model.PostList, *model.AppError) { ret := _m.Called(channelId, postId, numPosts, offset) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string, int, int) store.StoreChannel); ok { + var r0 *model.PostList + if rf, ok := ret.Get(0).(func(string, string, int, int) *model.PostList); ok { r0 = rf(channelId, postId, numPosts, offset) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.PostList) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, string, int, int) *model.AppError); ok { + r1 = rf(channelId, postId, numPosts, offset) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetPostsBatchForIndexing provides a mock function with given fields: startTime, endTime, limit @@ -334,19 +343,28 @@ func (_m *PostStore) GetPostsBatchForIndexing(startTime int64, endTime int64, li } // GetPostsBefore provides a mock function with given fields: channelId, postId, numPosts, offset -func (_m *PostStore) GetPostsBefore(channelId string, postId string, numPosts int, offset int) store.StoreChannel { +func (_m *PostStore) GetPostsBefore(channelId string, postId string, numPosts int, offset int) (*model.PostList, *model.AppError) { ret := _m.Called(channelId, postId, numPosts, offset) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, string, int, int) store.StoreChannel); ok { + var r0 *model.PostList + if rf, ok := ret.Get(0).(func(string, string, int, int) *model.PostList); ok { r0 = rf(channelId, postId, numPosts, offset) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.PostList) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, string, int, int) *model.AppError); ok { + r1 = rf(channelId, postId, numPosts, offset) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetPostsByIds provides a mock function with given fields: postIds diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 8bc5fe9022..3d03a00ebd 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -731,19 +731,17 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { } t.Run("should not return anything before the first post", func(t *testing.T) { - res := <-ss.Post().GetPostsBefore(channelId, posts[0].Id, 10, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsBefore(channelId, posts[0].Id, 10, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{}, postList.Order) assert.Equal(t, map[string]*model.Post{}, postList.Posts) }) t.Run("should return posts before a post", func(t *testing.T) { - res := <-ss.Post().GetPostsBefore(channelId, posts[5].Id, 10, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsBefore(channelId, posts[5].Id, 10, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{posts[4].Id, posts[3].Id, posts[2].Id, posts[1].Id, posts[0].Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ posts[0].Id: posts[0], @@ -755,10 +753,9 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { }) t.Run("should limit posts before", func(t *testing.T) { - res := <-ss.Post().GetPostsBefore(channelId, posts[5].Id, 2, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsBefore(channelId, posts[5].Id, 2, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{posts[4].Id, posts[3].Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ posts[3].Id: posts[3], @@ -767,19 +764,17 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { }) t.Run("should not return anything after the last post", func(t *testing.T) { - res := <-ss.Post().GetPostsAfter(channelId, posts[len(posts)-1].Id, 10, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsAfter(channelId, posts[len(posts)-1].Id, 10, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{}, postList.Order) assert.Equal(t, map[string]*model.Post{}, postList.Posts) }) t.Run("should return posts after a post", func(t *testing.T) { - res := <-ss.Post().GetPostsAfter(channelId, posts[5].Id, 10, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsAfter(channelId, posts[5].Id, 10, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{posts[9].Id, posts[8].Id, posts[7].Id, posts[6].Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ posts[6].Id: posts[6], @@ -790,10 +785,9 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { }) t.Run("should limit posts after", func(t *testing.T) { - res := <-ss.Post().GetPostsAfter(channelId, posts[5].Id, 2, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsAfter(channelId, posts[5].Id, 2, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{posts[7].Id, posts[6].Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ posts[6].Id: posts[6], @@ -866,10 +860,9 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { post2.UpdateAt = post6.UpdateAt t.Run("should return each post and thread before a post", func(t *testing.T) { - res := <-ss.Post().GetPostsBefore(channelId, post4.Id, 2, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsBefore(channelId, post4.Id, 2, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{post3.Id, post2.Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ post1.Id: post1, @@ -881,10 +874,9 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { }) t.Run("should return each post and the root of each thread after a post", func(t *testing.T) { - res := <-ss.Post().GetPostsAfter(channelId, post4.Id, 2, 0) - assert.Nil(t, res.Err) + postList, err := ss.Post().GetPostsAfter(channelId, post4.Id, 2, 0) + assert.Nil(t, err) - postList := res.Data.(*model.PostList) assert.Equal(t, []string{post6.Id, post5.Id}, postList.Order) assert.Equal(t, map[string]*model.Post{ post2.Id: post2,