From 77881fc35748aad73aa05733584820e99d52c620 Mon Sep 17 00:00:00 2001 From: Nathaniel Allred Date: Mon, 25 Jul 2022 16:01:59 -0500 Subject: [PATCH] Mm 43609 (#20657) * return first inaccessible post time instead of has inaccessible posts --- api4/post.go | 6 +-- app/app_iface.go | 2 +- app/opentracing/opentracing_layer.go | 2 +- app/post.go | 16 +++--- app/post_helpers.go | 80 ++++++++++++++++++++-------- app/post_helpers_test.go | 45 +++++++++++++--- app/post_metadata.go | 12 ++--- model/client4.go | 48 ++++++++--------- model/post_list.go | 16 +++--- 9 files changed, 147 insertions(+), 80 deletions(-) diff --git a/api4/post.go b/api4/post.go index eb2dc1a26b..c5a8c5f0b3 100644 --- a/api4/post.go +++ b/api4/post.go @@ -401,7 +401,7 @@ func getPost(c *Context, w http.ResponseWriter, r *http.Request) { // Post is inaccessible due to cloud plan's limit. if err.Id == "app.post.cloud.get.app_error" { - w.Header().Set(model.HeaderHasInaccessiblePosts, "true") + w.Header().Set(model.HeaderFirstInaccessiblePostTime, "1") } return @@ -438,7 +438,7 @@ func getPostsByIds(c *Context, w http.ResponseWriter, r *http.Request) { return } - postsList, hasInaccessiblePosts, err := c.App.GetPostsByIds(postIDs) + postsList, firstInaccessiblePostTime, err := c.App.GetPostsByIds(postIDs) if err != nil { c.Err = err return @@ -471,7 +471,7 @@ func getPostsByIds(c *Context, w http.ResponseWriter, r *http.Request) { posts = append(posts, post) } - w.Header().Set(model.HeaderHasInaccessiblePosts, strconv.FormatBool(hasInaccessiblePosts)) + w.Header().Set(model.HeaderFirstInaccessiblePostTime, strconv.FormatInt(firstInaccessiblePostTime, 10)) if err := json.NewEncoder(w).Encode(posts); err != nil { mlog.Warn("Error while writing response", mlog.Err(err)) diff --git a/app/app_iface.go b/app/app_iface.go index 2ac622e3c0..fcce9b66e3 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -202,7 +202,7 @@ type AppIface interface { // lock instead. GetPluginsEnvironment() *plugin.Environment // GetPostsByIds response bool value indicates, if the post is inaccessible due to cloud plan's limit. - GetPostsByIds(postIDs []string) ([]*model.Post, bool, *model.AppError) + GetPostsByIds(postIDs []string) ([]*model.Post, int64, *model.AppError) // GetPostsUsage returns the total posts count rounded down to the most // significant digit GetPostsUsage() (int64, *model.AppError) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 5761473dcf..28a25fdcd6 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -7881,7 +7881,7 @@ func (a *OpenTracingAppLayer) GetPostsBeforePost(options model.GetPostsOptions) return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) GetPostsByIds(postIDs []string) ([]*model.Post, bool, *model.AppError) { +func (a *OpenTracingAppLayer) GetPostsByIds(postIDs []string) ([]*model.Post, int64, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetPostsByIds") diff --git a/app/post.go b/app/post.go index de087c4995..adfa7302ec 100644 --- a/app/post.go +++ b/app/post.go @@ -870,11 +870,11 @@ func (a *App) GetSinglePost(postID string, includeDeleted bool) (*model.Post, *m } } - isInaccessible, appErr := a.isInaccessiblePost(post) + firstInaccessiblePostTime, appErr := a.isInaccessiblePost(post) if appErr != nil { return nil, appErr } - if isInaccessible { + if firstInaccessiblePostTime != 0 { return nil, model.NewAppError("GetSinglePost", "app.post.cloud.get.app_error", nil, "", http.StatusForbidden) } @@ -1881,24 +1881,24 @@ func (a *App) GetPostIfAuthorized(c request.CTX, postID string, session *model.S } // GetPostsByIds response bool value indicates, if the post is inaccessible due to cloud plan's limit. -func (a *App) GetPostsByIds(postIDs []string) ([]*model.Post, bool, *model.AppError) { +func (a *App) GetPostsByIds(postIDs []string) ([]*model.Post, int64, *model.AppError) { posts, err := a.Srv().Store.Post().GetPostsByIds(postIDs) if err != nil { var nfErr *store.ErrNotFound switch { case errors.As(err, &nfErr): - return nil, false, model.NewAppError("GetPostsByIds", "app.post.get.app_error", nil, nfErr.Error(), http.StatusNotFound) + return nil, 0, model.NewAppError("GetPostsByIds", "app.post.get.app_error", nil, nfErr.Error(), http.StatusNotFound) default: - return nil, false, model.NewAppError("GetPostsByIds", "app.post.get.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, 0, model.NewAppError("GetPostsByIds", "app.post.get.app_error", nil, err.Error(), http.StatusInternalServerError) } } - posts, hasInaccessiblePosts, appErr := a.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) + posts, firstInaccessiblePostTime, appErr := a.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) if appErr != nil { - return nil, false, appErr + return nil, 0, appErr } - return posts, hasInaccessiblePosts, nil + return posts, firstInaccessiblePostTime, nil } func (a *App) GetTopThreadsForTeamSince(c request.CTX, teamID, userID string, opts *model.InsightsOpts) (*model.TopThreadList, *model.AppError) { diff --git a/app/post_helpers.go b/app/post_helpers.go index 88f9487829..4ae91bf60f 100644 --- a/app/post_helpers.go +++ b/app/post_helpers.go @@ -27,6 +27,19 @@ func (b accessibleBounds) noAccessible() bool { return b.start == noAccessibleBounds.start && b.end == noAccessibleBounds.end } +// assumes checking was already performed that at least one post is inaccessible +func (b accessibleBounds) getInaccessibleRange(listLength int) (int, int) { + var start, end int + if b.start == 0 { + start = b.end + 1 + end = listLength - 1 + } else { + start = 0 + end = b.start - 1 + } + return start, end +} + var noAccessibleBounds = accessibleBounds{start: -1, end: -1} var allAccessibleBounds = func(lenPosts int) accessibleBounds { return accessibleBounds{start: 0, end: lenPosts - 1} } @@ -82,11 +95,13 @@ func linearFilterPostList(postList *model.PostList, earliestAccessibleTime int64 n := 0 for i, postId := range order { - if posts[postId].CreateAt >= earliestAccessibleTime { + if createAt := posts[postId].CreateAt; createAt >= earliestAccessibleTime { order[n] = order[i] n++ } else { - postList.HasInaccessiblePosts = true + if createAt > postList.FirstInaccessiblePostTime { + postList.FirstInaccessiblePostTime = createAt + } delete(posts, postId) } } @@ -96,8 +111,10 @@ func linearFilterPostList(postList *model.PostList, earliestAccessibleTime int64 // for example GetPosts in the CollapsedThreads = false path, parents are not added // to Order for postId := range posts { - if posts[postId].CreateAt < earliestAccessibleTime { - postList.HasInaccessiblePosts = true + if createAt := posts[postId].CreateAt; createAt < earliestAccessibleTime { + if createAt > postList.FirstInaccessiblePostTime { + postList.FirstInaccessiblePostTime = createAt + } delete(posts, postId) } } @@ -106,18 +123,20 @@ func linearFilterPostList(postList *model.PostList, earliestAccessibleTime int64 // linearFilterPostsSlice make no assumptions about ordering, go through posts one by one // this is the slower fallback that is still safe if we can not // assume posts are ordered by CreatedAt -func linearFilterPostsSlice(posts []*model.Post, earliestAccessibleTime int64) ([]*model.Post, bool) { - hasInaccessiblePosts := false +func linearFilterPostsSlice(posts []*model.Post, earliestAccessibleTime int64) ([]*model.Post, int64) { + var firstInaccessiblePostTime int64 = 0 n := 0 for i := range posts { - if posts[i].CreateAt >= earliestAccessibleTime { + if createAt := posts[i].CreateAt; createAt >= earliestAccessibleTime { posts[n] = posts[i] n++ } else { - hasInaccessiblePosts = true + if createAt > firstInaccessiblePostTime { + firstInaccessiblePostTime = createAt + } } } - return posts[:n], hasInaccessiblePosts + return posts[:n], firstInaccessiblePostTime } // filterInaccessiblePosts filters out the posts, past the cloud limit @@ -146,13 +165,18 @@ func (a *App) filterInaccessiblePosts(postList *model.PostList, options filterPo } if bounds.noAccessible() { if lenPosts > 0 { - postList.HasInaccessiblePosts = true + firstPostCreatedAt := postList.Posts[postList.Order[0]].CreateAt + lastPostCreatedAt := postList.Posts[postList.Order[len(postList.Order)-1]].CreateAt + postList.FirstInaccessiblePostTime = max(firstPostCreatedAt, lastPostCreatedAt) } postList.Posts = map[string]*model.Post{} postList.Order = []string{} return nil } - postList.HasInaccessiblePosts = true + startInaccessibleIndex, endInaccessibleIndex := bounds.getInaccessibleRange(len(postList.Order)) + startInaccessibleCreatedAt := postList.Posts[postList.Order[startInaccessibleIndex]].CreateAt + endInaccessibleCreatedAt := postList.Posts[postList.Order[endInaccessibleIndex]].CreateAt + postList.FirstInaccessiblePostTime = max(startInaccessibleCreatedAt, endInaccessibleCreatedAt) posts := postList.Posts order := postList.Order @@ -183,9 +207,9 @@ func (a *App) filterInaccessiblePosts(postList *model.PostList, options filterPo } // isInaccessiblePost indicates if the post is past the cloud plan's limit. -func (a *App) isInaccessiblePost(post *model.Post) (bool, *model.AppError) { +func (a *App) isInaccessiblePost(post *model.Post) (int64, *model.AppError) { if post == nil { - return false, nil + return 0, nil } pl := &model.PostList{ @@ -193,22 +217,22 @@ func (a *App) isInaccessiblePost(post *model.Post) (bool, *model.AppError) { Posts: map[string]*model.Post{post.Id: post}, } - return pl.HasInaccessiblePosts, a.filterInaccessiblePosts(pl, filterPostOptions{assumeSortedCreatedAt: true}) + return pl.FirstInaccessiblePostTime, a.filterInaccessiblePosts(pl, filterPostOptions{assumeSortedCreatedAt: true}) } // getFilteredAccessiblePosts returns accessible posts filtered as per the cloud plan's limit and also indicates if there were any inaccessible posts -func (a *App) getFilteredAccessiblePosts(posts []*model.Post, options filterPostOptions) ([]*model.Post, bool, *model.AppError) { +func (a *App) getFilteredAccessiblePosts(posts []*model.Post, options filterPostOptions) ([]*model.Post, int64, *model.AppError) { if len(posts) == 0 { - return posts, false, nil + return posts, 0, nil } filteredPosts := []*model.Post{} lastAccessiblePostTime, appErr := a.GetLastAccessiblePostTime() if appErr != nil { - return filteredPosts, false, model.NewAppError("getFilteredAccessiblePosts", "app.last_accessible_post.app_error", nil, appErr.Error(), http.StatusInternalServerError) + return filteredPosts, 0, model.NewAppError("getFilteredAccessiblePosts", "app.last_accessible_post.app_error", nil, appErr.Error(), http.StatusInternalServerError) } else if lastAccessiblePostTime == 0 { // No need to filter, all posts are accessible - return posts, false, nil + return posts, 0, nil } if options.assumeSortedCreatedAt { @@ -216,16 +240,26 @@ func (a *App) getFilteredAccessiblePosts(posts []*model.Post, options filterPost getCreateAt := func(i int) int64 { return posts[i].CreateAt } bounds := getTimeSortedPostAccessibleBounds(lastAccessiblePostTime, lenPosts, getCreateAt) if bounds.allAccessible(lenPosts) { - return posts, false, nil + return posts, 0, nil } if bounds.noAccessible() { - return filteredPosts, lenPosts > 0, nil + var firstInaccessiblePostTime int64 = 0 + if lenPosts > 0 { + firstPostCreatedAt := posts[0].CreateAt + lastPostCreatedAt := posts[len(posts)-1].CreateAt + firstInaccessiblePostTime = max(firstPostCreatedAt, lastPostCreatedAt) + } + return filteredPosts, firstInaccessiblePostTime, nil } + startInaccessibleIndex, endInaccessibleIndex := bounds.getInaccessibleRange(len(posts)) + firstPostCreatedAt := posts[startInaccessibleIndex].CreateAt + lastPostCreatedAt := posts[endInaccessibleIndex].CreateAt + firstInaccessiblePostTime := max(firstPostCreatedAt, lastPostCreatedAt) filteredPosts = posts[bounds.start : bounds.end+1] - return filteredPosts, true, nil + return filteredPosts, firstInaccessiblePostTime, nil } - filteredPosts, hasInaccessiblePosts := linearFilterPostsSlice(posts, lastAccessiblePostTime) - return filteredPosts, hasInaccessiblePosts, nil + filteredPosts, firstInaccessiblePostTime := linearFilterPostsSlice(posts, lastAccessiblePostTime) + return filteredPosts, firstInaccessiblePostTime, nil } diff --git a/app/post_helpers_test.go b/app/post_helpers_test.go index ebde2f2e1a..34b17e9e34 100644 --- a/app/post_helpers_test.go +++ b/app/post_helpers_test.go @@ -231,6 +231,7 @@ func TestFilterInaccessiblePosts(t *testing.T) { "post_d", "post_e", }, postList.Order) + assert.Equal(t, int64(1), postList.FirstInaccessiblePostTime) }) t.Run("descending order returns correct posts", func(t *testing.T) { @@ -259,6 +260,8 @@ func TestFilterInaccessiblePosts(t *testing.T) { "post_d", "post_c", }, postList.Order) + + assert.Equal(t, int64(1), postList.FirstInaccessiblePostTime) }) t.Run("handles mixed create at ordering correctly if correct options given", func(t *testing.T) { @@ -332,18 +335,20 @@ func TestGetFilteredAccessiblePosts(t *testing.T) { t.Run("ascending order returns correct posts", func(t *testing.T) { posts := []*model.Post{postFromCreateAt(0), postFromCreateAt(1), postFromCreateAt(2), postFromCreateAt(3), postFromCreateAt(4)} - filteredPosts, _, appErr := th.App.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) + filteredPosts, firstInaccessiblePostTime, appErr := th.App.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) assert.Nil(t, appErr) assert.Equal(t, []*model.Post{postFromCreateAt(2), postFromCreateAt(3), postFromCreateAt(4)}, filteredPosts) + assert.Equal(t, int64(1), firstInaccessiblePostTime) }) t.Run("descending order returns correct posts", func(t *testing.T) { posts := []*model.Post{postFromCreateAt(4), postFromCreateAt(3), postFromCreateAt(2), postFromCreateAt(1), postFromCreateAt(0)} - filteredPosts, _, appErr := th.App.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) + filteredPosts, firstInaccessiblePostTime, appErr := th.App.getFilteredAccessiblePosts(posts, filterPostOptions{assumeSortedCreatedAt: true}) assert.Nil(t, appErr) assert.Equal(t, []*model.Post{postFromCreateAt(4), postFromCreateAt(3), postFromCreateAt(2)}, filteredPosts) + assert.Equal(t, int64(1), firstInaccessiblePostTime) }) t.Run("handles mixed create at ordering correctly if correct options given", func(t *testing.T) { @@ -366,12 +371,40 @@ func TestIsInaccessiblePost(t *testing.T) { defer th.TearDown() post := &model.Post{CreateAt: 3} - r, appErr := th.App.isInaccessiblePost(post) + firstInaccessiblePostTime, appErr := th.App.isInaccessiblePost(post) assert.Nil(t, appErr) - assert.Equal(t, false, r) + assert.Equal(t, int64(0), firstInaccessiblePostTime) post = &model.Post{CreateAt: 1} - r, appErr = th.App.isInaccessiblePost(post) + firstInaccessiblePostTime, appErr = th.App.isInaccessiblePost(post) assert.Nil(t, appErr) - assert.Equal(t, true, r) + assert.Equal(t, int64(1), firstInaccessiblePostTime) +} + +func Test_getInaccessibleRange(t *testing.T) { + type test struct { + label string + bounds accessibleBounds + listLength int + expectedStart int + expectedEnd int + } + tests := []test{ + { + label: "inaccessible at end", + bounds: accessibleBounds{start: 0, end: 3}, + listLength: 6, + expectedStart: 4, + expectedEnd: 5, + }, + } + + for _, test := range tests { + t.Run(test.label, func(t *testing.T) { + start, end := test.bounds.getInaccessibleRange(test.listLength) + + assert.Equal(t, test.expectedStart, start) + assert.Equal(t, test.expectedEnd, end) + }) + } } diff --git a/app/post_metadata.go b/app/post_metadata.go index 80809c5ee4..cbc920d506 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -54,12 +54,12 @@ func (s *Server) initPostMetadata() { func (a *App) PreparePostListForClient(c request.CTX, originalList *model.PostList) *model.PostList { list := &model.PostList{ - Posts: make(map[string]*model.Post, len(originalList.Posts)), - Order: originalList.Order, - NextPostId: originalList.NextPostId, - PrevPostId: originalList.PrevPostId, - HasNext: originalList.HasNext, - HasInaccessiblePosts: originalList.HasInaccessiblePosts, + Posts: make(map[string]*model.Post, len(originalList.Posts)), + Order: originalList.Order, + NextPostId: originalList.NextPostId, + PrevPostId: originalList.PrevPostId, + HasNext: originalList.HasNext, + FirstInaccessiblePostTime: originalList.FirstInaccessiblePostTime, } for id, originalPost := range originalList.Posts { diff --git a/model/client4.go b/model/client4.go index ba374830a1..1dc5f0aa8d 100644 --- a/model/client4.go +++ b/model/client4.go @@ -18,30 +18,30 @@ import ( ) const ( - HeaderRequestId = "X-Request-ID" - HeaderVersionId = "X-Version-ID" - HeaderClusterId = "X-Cluster-ID" - HeaderEtagServer = "ETag" - HeaderEtagClient = "If-None-Match" - HeaderForwarded = "X-Forwarded-For" - HeaderRealIP = "X-Real-IP" - HeaderForwardedProto = "X-Forwarded-Proto" - HeaderToken = "token" - HeaderCsrfToken = "X-CSRF-Token" - HeaderBearer = "BEARER" - HeaderAuth = "Authorization" - HeaderCloudToken = "X-Cloud-Token" - HeaderRemoteclusterToken = "X-RemoteCluster-Token" - HeaderRemoteclusterId = "X-RemoteCluster-Id" - HeaderRequestedWith = "X-Requested-With" - HeaderRequestedWithXML = "XMLHttpRequest" - HeaderHasInaccessiblePosts = "Has-Inaccessible-Posts" - HeaderRange = "Range" - STATUS = "status" - StatusOk = "OK" - StatusFail = "FAIL" - StatusUnhealthy = "UNHEALTHY" - StatusRemove = "REMOVE" + HeaderRequestId = "X-Request-ID" + HeaderVersionId = "X-Version-ID" + HeaderClusterId = "X-Cluster-ID" + HeaderEtagServer = "ETag" + HeaderEtagClient = "If-None-Match" + HeaderForwarded = "X-Forwarded-For" + HeaderRealIP = "X-Real-IP" + HeaderForwardedProto = "X-Forwarded-Proto" + HeaderToken = "token" + HeaderCsrfToken = "X-CSRF-Token" + HeaderBearer = "BEARER" + HeaderAuth = "Authorization" + HeaderCloudToken = "X-Cloud-Token" + HeaderRemoteclusterToken = "X-RemoteCluster-Token" + HeaderRemoteclusterId = "X-RemoteCluster-Id" + HeaderRequestedWith = "X-Requested-With" + HeaderRequestedWithXML = "XMLHttpRequest" + HeaderFirstInaccessiblePostTime = "First-Inaccessible-Post-Time" + HeaderRange = "Range" + STATUS = "status" + StatusOk = "OK" + StatusFail = "FAIL" + StatusUnhealthy = "UNHEALTHY" + StatusRemove = "REMOVE" ClientDir = "client" diff --git a/model/post_list.go b/model/post_list.go index 34fc031e3f..093ead6d50 100644 --- a/model/post_list.go +++ b/model/post_list.go @@ -16,8 +16,8 @@ type PostList struct { PrevPostId string `json:"prev_post_id"` // HasNext indicates whether there are more items to be fetched or not. HasNext bool `json:"has_next"` - // HasInaccessiblePosts tells if there are inaccessible posts, past the cloud limit. - HasInaccessiblePosts bool `json:"has_inaccessible_posts"` + // If there are inaccessible posts, FirstInaccessiblePostTime is the time of the latest inaccessible post + FirstInaccessiblePostTime int64 `json:"first_inaccessible_post_time"` } func NewPostList() *PostList { @@ -37,12 +37,12 @@ func (o *PostList) Clone() *PostList { postsCopy[k] = v.Clone() } return &PostList{ - Order: orderCopy, - Posts: postsCopy, - NextPostId: o.NextPostId, - PrevPostId: o.PrevPostId, - HasNext: o.HasNext, - HasInaccessiblePosts: o.HasInaccessiblePosts, + Order: orderCopy, + Posts: postsCopy, + NextPostId: o.NextPostId, + PrevPostId: o.PrevPostId, + HasNext: o.HasNext, + FirstInaccessiblePostTime: o.FirstInaccessiblePostTime, } }