[MM-15925] Add includeDeleted flag to GetForPost in FileInfoStore (#11585)

app/file.go

@@ -275,7 +275,7 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo {
 	if newPost := result.Posts[post.Id]; len(newPost.Filenames) != len(post.Filenames) {
 		// Another thread has already created FileInfos for this post, so just return those
 		var fileInfos []*model.FileInfo
-		fileInfos, err = a.Srv.Store.FileInfo().GetForPost(post.Id, true, false)
+		fileInfos, err = a.Srv.Store.FileInfo().GetForPost(post.Id, true, false, false)
 		if err != nil {
 			mlog.Error(fmt.Sprintf("Unable to get FileInfos for migrated post, err=%v", err), mlog.String("post_id", post.Id))
 			return []*model.FileInfo{}

app/notification.go

@@ -46,7 +46,7 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod
 	if len(post.FileIds) != 0 {
 		fchan = make(chan store.StoreResult, 1)
 		go func() {
-			fileInfos, err := a.Srv.Store.FileInfo().GetForPost(post.Id, true, true)
+			fileInfos, err := a.Srv.Store.FileInfo().GetForPost(post.Id, true, false, true)
 			fchan <- store.StoreResult{Data: fileInfos, Err: err}
 			close(fchan)
 		}()

app/notification_email.go

@@ -289,7 +289,7 @@ func (a *App) GetMessageForNotification(post *model.Post, translateFunc i18n.Tra
 	}
 
 	// extract the filenames from their paths and determine what type of files are attached
-	infos, err := a.Srv.Store.FileInfo().GetForPost(post.Id, true, true)
+	infos, err := a.Srv.Store.FileInfo().GetForPost(post.Id, true, false, true)
 	if err != nil {
 		mlog.Warn(fmt.Sprintf("Encountered error when getting files for notification message, post_id=%v, err=%v", post.Id, err), mlog.String("post_id", post.Id))
 	}

app/post.go

@@ -1087,7 +1087,7 @@ func (a *App) GetFileInfosForPostWithMigration(postId string) ([]*model.FileInfo
 }
 
 func (a *App) GetFileInfosForPost(postId string, fromMaster bool) ([]*model.FileInfo, *model.AppError) {
-	return a.Srv.Store.FileInfo().GetForPost(postId, fromMaster, true)
+	return a.Srv.Store.FileInfo().GetForPost(postId, fromMaster, false, true)
 }
 
 func (a *App) PostWithProxyAddedToImageURLs(post *model.Post) *model.Post {

store/sqlstore/file_info_store.go

@@ -7,6 +7,8 @@ import (
 	"database/sql"
 	"net/http"
 
+	sq "github.com/Masterminds/squirrel"
+
 	"github.com/mattermost/mattermost-server/einterfaces"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store"
@@ -116,9 +118,13 @@ func (fs SqlFileInfoStore) InvalidateFileInfosForPostCache(postId string) {
 	}
 }
 
-func (fs SqlFileInfoStore) GetForPost(postId string, readFromMaster bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError) {
+func (fs SqlFileInfoStore) GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError) {
+	cacheKey := postId
+	if includeDeleted {
+		cacheKey += "_deleted"
+	}
 	if allowFromCache {
-		if cacheItem, ok := fileInfoCache.Get(postId); ok {
+		if cacheItem, ok := fileInfoCache.Get(cacheKey); ok {
 			if fs.metrics != nil {
 				fs.metrics.IncrementMemCacheHitCounter("File Info Cache")
 			}
@@ -142,21 +148,27 @@ func (fs SqlFileInfoStore) GetForPost(postId string, readFromMaster bool, allowF
 		dbmap = fs.GetMaster()
 	}
 
-	if _, err := dbmap.Select(&infos,
+	query := fs.getQueryBuilder().
-		`SELECT
+		Select("*").
-				*
+		From("FileInfo").
-			FROM
+		Where(sq.Eq{"PostId": postId}).
-				FileInfo
+		OrderBy("CreateAt")
-			WHERE
+
-				PostId = :PostId
+	if !includeDeleted {
-				AND DeleteAt = 0
+		query = query.Where("DeleteAt = 0")
-			ORDER BY
+	}
-				CreateAt`, map[string]interface{}{"PostId": postId}); err != nil {
+
+	queryString, args, err := query.ToSql()
+	if err != nil {
+		return nil, model.NewAppError("SqlFileInfoStore.GetForPost", "store.sql_file_info.get_for_post.app_error", nil, err.Error(), http.StatusInternalServerError)
+	}
+
+	if _, err := dbmap.Select(&infos, queryString, args...); err != nil {
 		return nil, model.NewAppError("SqlFileInfoStore.GetForPost",
 			"store.sql_file_info.get_for_post.app_error", nil, "post_id="+postId+", "+err.Error(), http.StatusInternalServerError)
 	}
 	if len(infos) > 0 {
-		fileInfoCache.AddWithExpiresInSecs(postId, infos, FILE_INFO_CACHE_SEC)
+		fileInfoCache.AddWithExpiresInSecs(cacheKey, infos, FILE_INFO_CACHE_SEC)
 	}
 
 	return infos, nil
@@ -177,7 +189,7 @@ func (fs SqlFileInfoStore) GetForUser(userId string) ([]*model.FileInfo, *model.
 				AND DeleteAt = 0
 			ORDER BY
 				CreateAt`, map[string]interface{}{"CreatorId": userId}); err != nil {
-		return nil, model.NewAppError("SqlFileInfoStore.GetForPost",
+		return nil, model.NewAppError("SqlFileInfoStore.GetForUser",
 			"store.sql_file_info.get_for_user_id.app_error", nil, "creator_id="+userId+", "+err.Error(), http.StatusInternalServerError)
 	}
 	return infos, nil

store/store.go

@@ -486,7 +486,7 @@ type FileInfoStore interface {
 	Save(info *model.FileInfo) (*model.FileInfo, *model.AppError)
 	Get(id string) (*model.FileInfo, *model.AppError)
 	GetByPath(path string) (*model.FileInfo, *model.AppError)
-	GetForPost(postId string, readFromMaster bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError)
+	GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError)
 	GetForUser(userId string) ([]*model.FileInfo, *model.AppError)
 	InvalidateFileInfosForPostCache(postId string)
 	AttachToPost(fileId string, postId string, creatorId string) *model.AppError

store/storetest/file_info_store.go

@@ -128,17 +128,85 @@ func testFileInfoGetForPost(t *testing.T, ss store.Store) {
 		}(newInfo.Id)
 	}
 
-	postInfos, err := ss.FileInfo().GetForPost(postId, true, false)
+	testCases := []struct {
-	require.Nil(t, err)
+		Name           string
-	assert.Len(t, postInfos, 2)
+		PostId         string
+		ReadFromMaster bool
+		IncludeDeleted bool
+		AllowFromCache bool
+		ExpectedPosts  int
+	}{
+		{
+			Name:           "Fetch from master, without deleted and without cache",
+			PostId:         postId,
+			ReadFromMaster: true,
+			IncludeDeleted: false,
+			AllowFromCache: false,
+			ExpectedPosts:  2,
+		},
+		{
+			Name:           "Fetch from master, with deleted and without cache",
+			PostId:         postId,
+			ReadFromMaster: true,
+			IncludeDeleted: true,
+			AllowFromCache: false,
+			ExpectedPosts:  3,
+		},
+		{
+			Name:           "Fetch from master, with deleted and with cache",
+			PostId:         postId,
+			ReadFromMaster: true,
+			IncludeDeleted: true,
+			AllowFromCache: true,
+			ExpectedPosts:  3,
+		},
+		{
+			Name:           "Fetch from replica, without deleted and without cache",
+			PostId:         postId,
+			ReadFromMaster: false,
+			IncludeDeleted: false,
+			AllowFromCache: false,
+			ExpectedPosts:  2,
+		},
+		{
+			Name:           "Fetch from replica, with deleted and without cache",
+			PostId:         postId,
+			ReadFromMaster: false,
+			IncludeDeleted: true,
+			AllowFromCache: false,
+			ExpectedPosts:  3,
+		},
+		{
+			Name:           "Fetch from replica, with deleted and without cache",
+			PostId:         postId,
+			ReadFromMaster: false,
+			IncludeDeleted: true,
+			AllowFromCache: true,
+			ExpectedPosts:  3,
+		},
+		{
+			Name:           "Fetch from replica, without deleted and with cache",
+			PostId:         postId,
+			ReadFromMaster: true,
+			IncludeDeleted: false,
+			AllowFromCache: true,
+			ExpectedPosts:  2,
+		},
+	}
 
-	postInfos, err = ss.FileInfo().GetForPost(postId, false, false)
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			postInfos, err := ss.FileInfo().GetForPost(
+				tc.PostId,
+				tc.ReadFromMaster,
+				tc.IncludeDeleted,
+				tc.AllowFromCache,
+			)
 			require.Nil(t, err)
-	assert.Len(t, postInfos, 2)
+			assert.Len(t, postInfos, tc.ExpectedPosts)
 
-	postInfos, err = ss.FileInfo().GetForPost(postId, true, true)
+		})
-	require.Nil(t, err)
+	}
-	assert.Len(t, postInfos, 2)
 }
 
 func testFileInfoGetForUser(t *testing.T, ss store.Store) {
@@ -212,7 +280,7 @@ func testFileInfoAttachToPost(t *testing.T, ss store.Store) {
 		err = ss.FileInfo().AttachToPost(info2.Id, postId, userId)
 		assert.Nil(t, err)
 
-		data, err := ss.FileInfo().GetForPost(postId, true, false)
+		data, err := ss.FileInfo().GetForPost(postId, true, false, false)
 		assert.Nil(t, err)
 
 		assert.Len(t, data, 2)
@@ -296,7 +364,7 @@ func testFileInfoDeleteForPost(t *testing.T, ss store.Store) {
 	_, err := ss.FileInfo().DeleteForPost(postId)
 	require.Nil(t, err)
 
-	infos, err = ss.FileInfo().GetForPost(postId, true, false)
+	infos, err = ss.FileInfo().GetForPost(postId, true, false, false)
 	require.Nil(t, err)
 	assert.Len(t, infos, 0)
 }
@@ -340,14 +408,14 @@ func testFileInfoPermanentDeleteBatch(t *testing.T, ss store.Store) {
 	})
 	require.Nil(t, err)
 
-	postFiles, err := ss.FileInfo().GetForPost(postId, true, false)
+	postFiles, err := ss.FileInfo().GetForPost(postId, true, false, false)
 	require.Nil(t, err)
 	assert.Len(t, postFiles, 3)
 
 	_, err = ss.FileInfo().PermanentDeleteBatch(1500, 1000)
 	require.Nil(t, err)
 
-	postFiles, err = ss.FileInfo().GetForPost(postId, true, false)
+	postFiles, err = ss.FileInfo().GetForPost(postId, true, false, false)
 	require.Nil(t, err)
 	assert.Len(t, postFiles, 1)
 }

store/storetest/mocks/FileInfoStore.go

@@ -106,13 +106,13 @@ func (_m *FileInfoStore) GetByPath(path string) (*model.FileInfo, *model.AppErro
 	return r0, r1
 }
 
-// GetForPost provides a mock function with given fields: postId, readFromMaster, allowFromCache
+// GetForPost provides a mock function with given fields: postId, readFromMaster, includeDeleted, allowFromCache
-func (_m *FileInfoStore) GetForPost(postId string, readFromMaster bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError) {
+func (_m *FileInfoStore) GetForPost(postId string, readFromMaster bool, includeDeleted bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError) {
-	ret := _m.Called(postId, readFromMaster, allowFromCache)
+	ret := _m.Called(postId, readFromMaster, includeDeleted, allowFromCache)
 
 	var r0 []*model.FileInfo
-	if rf, ok := ret.Get(0).(func(string, bool, bool) []*model.FileInfo); ok {
+	if rf, ok := ret.Get(0).(func(string, bool, bool, bool) []*model.FileInfo); ok {
-		r0 = rf(postId, readFromMaster, allowFromCache)
+		r0 = rf(postId, readFromMaster, includeDeleted, allowFromCache)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).([]*model.FileInfo)
@@ -120,8 +120,8 @@ func (_m *FileInfoStore) GetForPost(postId string, readFromMaster bool, allowFro
 	}
 
 	var r1 *model.AppError
-	if rf, ok := ret.Get(1).(func(string, bool, bool) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string, bool, bool, bool) *model.AppError); ok {
-		r1 = rf(postId, readFromMaster, allowFromCache)
+		r1 = rf(postId, readFromMaster, includeDeleted, allowFromCache)
 	} else {
 		if ret.Get(1) != nil {
 			r1 = ret.Get(1).(*model.AppError)