From 23c895031255398f2c9359a17bf1d8e4bb0451e7 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 19 Nov 2018 13:26:40 -0500 Subject: [PATCH] MM-11434 Performance improvements for post metadata (#9849) * Remove unused error return value from PreparePostForClient * Remove unused error return value from PreparePostListForClient * MM-11434 Parallelize PreparePostListForClient * MM-11434 Skip looking reactions and files on post whenever possible * Add note about the use of deprecated fields --- api4/channel.go | 5 +-- api4/post.go | 54 +++++----------------------- app/notification.go | 7 +--- app/post.go | 21 ++--------- app/post_metadata.go | 52 ++++++++++++++++++--------- app/post_metadata_test.go | 75 ++++++++++++++++++++++++--------------- app/reaction.go | 10 ++---- i18n/en.json | 4 --- 8 files changed, 100 insertions(+), 128 deletions(-) diff --git a/api4/channel.go b/api4/channel.go index 335e198ca9..81dfab130e 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -488,10 +488,7 @@ func getPinnedPosts(c *Context, w http.ResponseWriter, r *http.Request) { return } - clientPostList, err := c.App.PreparePostListForClient(posts) - if err != nil { - mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err)) - } + clientPostList := c.App.PreparePostListForClient(posts) w.Header().Set(model.HEADER_ETAG_SERVER, clientPostList.Etag()) w.Write([]byte(clientPostList.ToJson())) diff --git a/api4/post.go b/api4/post.go index 508b076658..c62839bcfc 100644 --- a/api4/post.go +++ b/api4/post.go @@ -9,7 +9,6 @@ import ( "strconv" "time" - "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" ) @@ -68,13 +67,8 @@ func createPost(c *Context, w http.ResponseWriter, r *http.Request) { c.App.SetStatusOnline(c.Session.UserId, false) c.App.UpdateLastActivityAtIfNeeded(c.Session) - clientPost, err := c.App.PreparePostForClient(rp) - if err != nil { - mlog.Error("Failed to prepare post for createPost response", mlog.Any("err", err)) - } - w.WriteHeader(http.StatusCreated) - w.Write([]byte(clientPost.ToJson())) + w.Write([]byte(c.App.PreparePostForClient(rp).ToJson())) } func createEphemeralPost(c *Context, w http.ResponseWriter, r *http.Request) { @@ -101,13 +95,8 @@ func createEphemeralPost(c *Context, w http.ResponseWriter, r *http.Request) { rp := c.App.SendEphemeralPost(ephRequest.UserID, c.App.PostWithProxyRemovedFromImageURLs(ephRequest.Post)) - clientPost, err := c.App.PreparePostForClient(rp) - if err != nil { - mlog.Error("Failed to prepare post for createEphemeralPost response", mlog.Any("err", err)) - } - w.WriteHeader(http.StatusCreated) - w.Write([]byte(clientPost.ToJson())) + w.Write([]byte(c.App.PreparePostForClient(rp).ToJson())) } func getPostsForChannel(c *Context, w http.ResponseWriter, r *http.Request) { @@ -177,12 +166,7 @@ func getPostsForChannel(c *Context, w http.ResponseWriter, r *http.Request) { w.Header().Set(model.HEADER_ETAG_SERVER, etag) } - clientPostList, err := c.App.PreparePostListForClient(list) - if err != nil { - mlog.Error("Failed to prepare posts for getPostsForChannel response", mlog.Any("err", err)) - } - - w.Write([]byte(clientPostList.ToJson())) + w.Write([]byte(c.App.PreparePostListForClient(list).ToJson())) } func getFlaggedPostsForUser(c *Context, w http.ResponseWriter, r *http.Request) { @@ -215,12 +199,7 @@ func getFlaggedPostsForUser(c *Context, w http.ResponseWriter, r *http.Request) return } - clientPostList, err := c.App.PreparePostListForClient(posts) - if err != nil { - mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err)) - } - - w.Write([]byte(clientPostList.ToJson())) + w.Write([]byte(c.App.PreparePostListForClient(posts).ToJson())) } func getPost(c *Context, w http.ResponseWriter, r *http.Request) { @@ -254,10 +233,7 @@ func getPost(c *Context, w http.ResponseWriter, r *http.Request) { } } - post, err = c.App.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare post for getPost response", mlog.Any("err", err)) - } + post = c.App.PreparePostForClient(post) if c.HandleEtag(post.Etag(), "Get Post", w, r) { return @@ -342,10 +318,7 @@ func getPostThread(c *Context, w http.ResponseWriter, r *http.Request) { return } - clientPostList, err := c.App.PreparePostListForClient(list) - if err != nil { - mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err)) - } + clientPostList := c.App.PreparePostListForClient(list) w.Header().Set(model.HEADER_ETAG_SERVER, clientPostList.Etag()) @@ -412,10 +385,7 @@ func searchPosts(c *Context, w http.ResponseWriter, r *http.Request) { return } - clientPostList, err := c.App.PreparePostListForClient(results.PostList) - if err != nil { - mlog.Error("Failed to prepare posts for searchPosts response", mlog.Any("err", err)) - } + clientPostList := c.App.PreparePostListForClient(results.PostList) results = model.MakePostSearchResults(clientPostList, results.Matches) @@ -468,10 +438,7 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { return } - rpost, err = c.App.PreparePostForClient(rpost) - if err != nil { - mlog.Error("Failed to prepare post for updatePost response", mlog.Any("err", err)) - } + rpost = c.App.PreparePostForClient(rpost) w.Write([]byte(rpost.ToJson())) } @@ -513,10 +480,7 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) { return } - patchedPost, err = c.App.PreparePostForClient(patchedPost) - if err != nil { - mlog.Error("Failed to prepare post for patchPost response", mlog.Any("err", err)) - } + patchedPost = c.App.PreparePostForClient(patchedPost) w.Write([]byte(patchedPost.ToJson())) } diff --git a/app/notification.go b/app/notification.go index 4a19cacaad..6746516f1a 100644 --- a/app/notification.go +++ b/app/notification.go @@ -317,13 +317,8 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod } } - clientPost, err := a.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare new post for client", mlog.Any("err", err)) - } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POSTED, "", post.ChannelId, "", nil) - message.Add("post", clientPost.ToJson()) + message.Add("post", a.PreparePostForClient(post).ToJson()) message.Add("channel_type", channel.Type) message.Add("channel_display_name", notification.GetChannelName(model.SHOW_USERNAME, "")) message.Add("channel_name", channel.Name) diff --git a/app/post.go b/app/post.go index 75661ca349..3e87e9e4a1 100644 --- a/app/post.go +++ b/app/post.go @@ -297,13 +297,8 @@ func (a *App) SendEphemeralPost(userId string, post *model.Post) *model.Post { post.Props = model.StringInterface{} } - clientPost, err := a.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare ephemeral post for client", mlog.Any("err", err)) - } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_EPHEMERAL_MESSAGE, "", post.ChannelId, userId, nil) - message.Add("post", clientPost.ToJson()) + message.Add("post", a.PreparePostForClient(post).ToJson()) a.Publish(message) return post @@ -424,13 +419,8 @@ func (a *App) PatchPost(postId string, patch *model.PostPatch) (*model.Post, *mo } func (a *App) sendUpdatedPostEvent(post *model.Post) { - clientPost, err := a.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare updated post for client", mlog.Any("err", err)) - } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POST_EDITED, "", post.ChannelId, "", nil) - message.Add("post", clientPost.ToJson()) + message.Add("post", a.PreparePostForClient(post).ToJson()) a.Publish(message) } @@ -569,13 +559,8 @@ func (a *App) DeletePost(postId, deleteByID string) (*model.Post, *model.AppErro return nil, result.Err } - clientPost, err := a.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare deleted post for client", mlog.Any("err", err)) - } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POST_DELETED, "", post.ChannelId, "", nil) - message.Add("post", clientPost.ToJson()) + message.Add("post", a.PreparePostForClient(post).ToJson()) a.Publish(message) a.Srv.Go(func() { diff --git a/app/post_metadata.go b/app/post_metadata.go index b66b2a437a..8dc404283c 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -31,25 +31,33 @@ func (a *App) InitPostMetadata() { }) } -func (a *App) PreparePostListForClient(originalList *model.PostList) (*model.PostList, *model.AppError) { +func (a *App) PreparePostListForClient(originalList *model.PostList) *model.PostList { + numPosts := len(originalList.Posts) + list := &model.PostList{ - Posts: make(map[string]*model.Post), - Order: originalList.Order, + Posts: make(map[string]*model.Post, numPosts), + Order: originalList.Order, // Note that this uses the original Order array, so it isn't a deep copy } - for id, originalPost := range originalList.Posts { - post, err := a.PreparePostForClient(originalPost) - if err != nil { - return originalList, err - } + posts := make(chan *model.Post, numPosts) - list.Posts[id] = post + for _, originalPost := range originalList.Posts { + go func(originalPost *model.Post) { + posts <- a.PreparePostForClient(originalPost) + }(originalPost) } - return list, nil + for i := 0; i < numPosts; i++ { + post := <-posts + list.Posts[post.Id] = post + } + + close(posts) + + return list } -func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *model.AppError) { +func (a *App) PreparePostForClient(originalPost *model.Post) *model.Post { post := originalPost.Clone() // Proxy image links before constructing metadata so that requests go through the proxy @@ -67,7 +75,7 @@ func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *mode } // Files - if fileInfos, err := a.GetFileInfosForPost(post.Id, false); err != nil { + if fileInfos, err := a.getFileMetadataForPost(post); err != nil { mlog.Warn("Failed to get files for a post", mlog.String("post_id", post.Id), mlog.Any("err", err)) } else { post.Metadata.Files = fileInfos @@ -87,13 +95,25 @@ func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *mode post.Metadata.Images = a.getImagesForPost(post, images) } - return post, nil + return post +} + +func (a *App) getFileMetadataForPost(post *model.Post) ([]*model.FileInfo, *model.AppError) { + if len(post.FileIds) == 0 { // This field is deprecated, but still use it for now to avoid unnecessary database hits + return nil, nil + } + + return a.GetFileInfosForPost(post.Id, false) } func (a *App) getEmojisAndReactionsForPost(post *model.Post) ([]*model.Emoji, []*model.Reaction, *model.AppError) { - reactions, err := a.GetReactionsForPost(post.Id) - if err != nil { - return nil, nil, err + var reactions []*model.Reaction + if post.HasReactions { // This field is deprecated, but still use it for now to avoid unnecessary database hits + var err *model.AppError + reactions, err = a.GetReactionsForPost(post.Id) + if err != nil { + return nil, nil, err + } } emojis, err := a.getCustomEmojisForPost(post, reactions) diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index c71e5b64b3..5b42d6d938 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -18,6 +18,37 @@ import ( "github.com/stretchr/testify/require" ) +func TestPreparePostListForClient(t *testing.T) { + // Most of this logic is covered by TestPreparePostForClient, so this just tests handling of multiple posts + + th := Setup().InitBasic() + defer th.TearDown() + + postList := model.NewPostList() + for i := 0; i < 5; i++ { + postList.AddPost(th.CreatePost(th.BasicChannel)) + } + + clientPostList := th.App.PreparePostListForClient(postList) + + t.Run("doesn't mutate provided post list", func(t *testing.T) { + assert.NotEqual(t, clientPostList, postList, "should've returned a new post list") + assert.NotEqual(t, clientPostList.Posts, postList.Posts, "should've returned a new PostList.Posts") + assert.Equal(t, clientPostList.Order, postList.Order, "should've returned the existing PostList.Order") + + for id, originalPost := range postList.Posts { + assert.NotEqual(t, clientPostList.Posts[id], originalPost, "should've returned new post objects") + assert.Equal(t, clientPostList.Posts[id].Id, originalPost.Id, "should've returned the same posts") + } + }) + + t.Run("adds metadata to each post", func(t *testing.T) { + for _, clientPost := range clientPostList.Posts { + assert.NotNil(t, clientPost.Metadata, "should've populated metadata for each post") + } + }) +} + func TestPreparePostForClient(t *testing.T) { setup := func() *TestHelper { th := Setup().InitBasic() @@ -38,8 +69,7 @@ func TestPreparePostForClient(t *testing.T) { post := th.CreatePost(th.BasicChannel) message := post.Message - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("doesn't mutate provided post", func(t *testing.T) { assert.NotEqual(t, clientPost, post, "should've returned a new post") @@ -63,11 +93,9 @@ func TestPreparePostForClient(t *testing.T) { th := setup() defer th.TearDown() - post, err := th.App.PreparePostForClient(th.CreatePost(th.BasicChannel)) - require.Nil(t, err) + post := th.App.PreparePostForClient(th.CreatePost(th.BasicChannel)) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) assert.False(t, clientPost == post, "should've returned a new post") assert.Equal(t, clientPost, post, "shouldn't have changed any metadata") @@ -81,9 +109,9 @@ func TestPreparePostForClient(t *testing.T) { reaction1 := th.AddReactionToPost(post, th.BasicUser, "smile") reaction2 := th.AddReactionToPost(post, th.BasicUser2, "smile") reaction3 := th.AddReactionToPost(post, th.BasicUser2, "ice_cream") + post.HasReactions = true - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) assert.Len(t, clientPost.Metadata.Reactions, 3, "should've populated Reactions") assert.Equal(t, reaction1, clientPost.Metadata.Reactions[0], "first reaction is incorrect") @@ -107,8 +135,7 @@ func TestPreparePostForClient(t *testing.T) { fileInfo.PostId = post.Id - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) assert.Equal(t, []*model.FileInfo{fileInfo}, clientPost.Metadata.Files, "should've populated Files") }) @@ -140,9 +167,9 @@ func TestPreparePostForClient(t *testing.T) { th.AddReactionToPost(post, th.BasicUser, "smile") th.AddReactionToPost(post, th.BasicUser, "angry") th.AddReactionToPost(post, th.BasicUser2, "angry") + post.HasReactions = true - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("populates emojis", func(t *testing.T) { assert.ElementsMatch(t, []*model.Emoji{}, clientPost.Metadata.Emojis, "should've populated empty Emojis") @@ -185,9 +212,9 @@ func TestPreparePostForClient(t *testing.T) { th.AddReactionToPost(post, th.BasicUser, emoji2.Name) th.AddReactionToPost(post, th.BasicUser2, emoji2.Name) th.AddReactionToPost(post, th.BasicUser2, "angry") + post.HasReactions = true - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("pupulates emojis", func(t *testing.T) { assert.ElementsMatch(t, []*model.Emoji{emoji1, emoji2, emoji3, emoji4}, clientPost.Metadata.Emojis, "should've populated post.Emojis") @@ -210,8 +237,7 @@ func TestPreparePostForClient(t *testing.T) { }, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("populates image dimensions", func(t *testing.T) { imageDimensions := clientPost.Metadata.Images @@ -253,8 +279,7 @@ func TestPreparePostForClient(t *testing.T) { }, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) // Reminder that only the first link gets an embed and dimensions @@ -288,8 +313,7 @@ func TestPreparePostForClient(t *testing.T) { }, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("populates embeds", func(t *testing.T) { assert.ElementsMatch(t, []*model.PostEmbed{ @@ -339,8 +363,7 @@ func TestPreparePostForClient(t *testing.T) { }, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) t.Run("populates embeds", func(t *testing.T) { assert.ElementsMatch(t, []*model.PostEmbed{ @@ -405,10 +428,7 @@ func testProxyLinkedImage(t *testing.T, th *TestHelper, shouldProxy bool) { post, err = th.App.CreatePost(post, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - if err != nil && err.Id != "app.post.metadata.link.app_error" { - t.Fatal(err) - } + clientPost := th.App.PreparePostForClient(post) if shouldProxy { assert.Equal(t, post.Message, fmt.Sprintf(postTemplate, imageURL), "should not have mutated original post") @@ -426,8 +446,7 @@ func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) { }, th.BasicChannel, false) require.Nil(t, err) - clientPost, err := th.App.PreparePostForClient(post) - require.Nil(t, err) + clientPost := th.App.PreparePostForClient(post) image := &opengraph.Image{} if shouldProxy { diff --git a/app/reaction.go b/app/reaction.go index 33980326d5..19dc02788c 100644 --- a/app/reaction.go +++ b/app/reaction.go @@ -6,7 +6,6 @@ package app import ( "net/http" - "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" ) @@ -112,13 +111,10 @@ func (a *App) sendReactionEvent(event string, reaction *model.Reaction, post *mo message.Add("reaction", reaction.ToJson()) a.Publish(message) - clientPost, err := a.PreparePostForClient(post) - if err != nil { - mlog.Error("Failed to prepare new post for client after reaction", mlog.Any("err", err)) - } + post.HasReactions = hasReactions + post.UpdateAt = model.GetMillis() - clientPost.HasReactions = hasReactions - clientPost.UpdateAt = model.GetMillis() + clientPost := a.PreparePostForClient(post) umessage := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POST_EDITED, "", post.ChannelId, "", nil) umessage.Add("post", clientPost.ToJson()) diff --git a/i18n/en.json b/i18n/en.json index 5d7498fa96..3f8dca0f01 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3142,10 +3142,6 @@ "id": "app.plugin.upload_disabled.app_error", "translation": "Plugins and/or plugin uploads have been disabled." }, - { - "id": "app.post.metadata.link.app_error", - "translation": "Failed to get metadata for a link in a post." - }, { "id": "app.role.check_roles_exist.role_not_found", "translation": "The provided role does not exist"