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

@@ -488,10 +488,7 @@ func getPinnedPosts(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	clientPostList, err := c.App.PreparePostListForClient(posts)
+	clientPostList := c.App.PreparePostListForClient(posts)
-	if err != nil {
-		mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err))
-	}
 
 	w.Header().Set(model.HEADER_ETAG_SERVER, clientPostList.Etag())
 	w.Write([]byte(clientPostList.ToJson()))

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)
+	w.Write([]byte(c.App.PreparePostListForClient(list).ToJson()))
-	if err != nil {
-		mlog.Error("Failed to prepare posts for getPostsForChannel response", mlog.Any("err", err))
-	}
-
-	w.Write([]byte(clientPostList.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)
+	w.Write([]byte(c.App.PreparePostListForClient(posts).ToJson()))
-	if err != nil {
-		mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err))
-	}
-
-	w.Write([]byte(clientPostList.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)
+	post = c.App.PreparePostForClient(post)
-	if err != nil {
-		mlog.Error("Failed to prepare post for getPost response", mlog.Any("err", err))
-	}
 
 	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)
+	clientPostList := c.App.PreparePostListForClient(list)
-	if err != nil {
-		mlog.Error("Failed to prepare posts for getFlaggedPostsForUser response", mlog.Any("err", err))
-	}
 
 	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)
+	clientPostList := c.App.PreparePostListForClient(results.PostList)
-	if err != nil {
-		mlog.Error("Failed to prepare posts for searchPosts response", mlog.Any("err", err))
-	}
 
 	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)
+	rpost = c.App.PreparePostForClient(rpost)
-	if err != nil {
-		mlog.Error("Failed to prepare post for updatePost response", mlog.Any("err", err))
-	}
 
 	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)
+	patchedPost = c.App.PreparePostForClient(patchedPost)
-	if err != nil {
-		mlog.Error("Failed to prepare post for patchPost response", mlog.Any("err", err))
-	}
 
 	w.Write([]byte(patchedPost.ToJson()))
 }

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)

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() {

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),
+		Posts: make(map[string]*model.Post, numPosts),
-		Order: originalList.Order,
+		Order: originalList.Order, // Note that this uses the original Order array, so it isn't a deep copy
 	}
 
-	for id, originalPost := range originalList.Posts {
+	posts := make(chan *model.Post, numPosts)
-		post, err := a.PreparePostForClient(originalPost)
+
-		if err != nil {
+	for _, originalPost := range originalList.Posts {
-			return originalList, err
+		go func(originalPost *model.Post) {
+			posts <- a.PreparePostForClient(originalPost)
+		}(originalPost)
 	}
 
-		list.Posts[id] = post
+	for i := 0; i < numPosts; i++ {
+		post := <-posts
+		list.Posts[post.Id] = post
 	}
 
-	return list, nil
+	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,14 +95,26 @@ 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)
+	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)
 	if err != nil {

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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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))
+		post := th.App.PreparePostForClient(th.CreatePost(th.BasicChannel))
-		require.Nil(t, err)
 
-		clientPost, err := th.App.PreparePostForClient(post)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		// 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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+		clientPost := th.App.PreparePostForClient(post)
-		require.Nil(t, err)
 
 		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)
+	clientPost := th.App.PreparePostForClient(post)
-	if err != nil && err.Id != "app.post.metadata.link.app_error" {
-		t.Fatal(err)
-	}
 
 	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)
+	clientPost := th.App.PreparePostForClient(post)
-	require.Nil(t, err)
 
 	image := &opengraph.Image{}
 	if shouldProxy {

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)
+	post.HasReactions = hasReactions
-	if err != nil {
+	post.UpdateAt = model.GetMillis()
-		mlog.Error("Failed to prepare new post for client after reaction", mlog.Any("err", err))
-	}
 
-	clientPost.HasReactions = hasReactions
+	clientPost := a.PreparePostForClient(post)
-	clientPost.UpdateAt = model.GetMillis()
 
 	umessage := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POST_EDITED, "", post.ChannelId, "", nil)
 	umessage.Add("post", clientPost.ToJson())

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"