From 8f4e03a52b22971954686c0554a0bd5a753c8e75 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 15 Jul 2019 08:54:30 +0200 Subject: [PATCH] [MM-15925] Add includeDeleted flag to GetForPost in FileInfoStore (#11585) --- app/file.go | 2 +- app/notification.go | 2 +- app/notification_email.go | 2 +- app/post.go | 2 +- store/sqlstore/file_info_store.go | 40 +++++++---- store/store.go | 2 +- store/storetest/file_info_store.go | 94 ++++++++++++++++++++++---- store/storetest/mocks/FileInfoStore.go | 14 ++-- 8 files changed, 119 insertions(+), 39 deletions(-) diff --git a/app/file.go b/app/file.go index bcff96a437..9d9d961e22 100644 --- a/app/file.go +++ b/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{} diff --git a/app/notification.go b/app/notification.go index a5c1665eb7..27d211d580 100644 --- a/app/notification.go +++ b/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) }() diff --git a/app/notification_email.go b/app/notification_email.go index db478b7b1c..07fba3449e 100644 --- a/app/notification_email.go +++ b/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)) } diff --git a/app/post.go b/app/post.go index 7c6ae8d001..4bfb0b8606 100644 --- a/app/post.go +++ b/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 { diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index 444181c82f..6e9298b313 100644 --- a/store/sqlstore/file_info_store.go +++ b/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, - `SELECT - * - FROM - FileInfo - WHERE - PostId = :PostId - AND DeleteAt = 0 - ORDER BY - CreateAt`, map[string]interface{}{"PostId": postId}); err != nil { + query := fs.getQueryBuilder(). + Select("*"). + From("FileInfo"). + Where(sq.Eq{"PostId": postId}). + OrderBy("CreateAt") + + if !includeDeleted { + query = query.Where("DeleteAt = 0") + } + + 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 diff --git a/store/store.go b/store/store.go index 2079827c9e..ca8f9900bb 100644 --- a/store/store.go +++ b/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 diff --git a/store/storetest/file_info_store.go b/store/storetest/file_info_store.go index c18a71b290..f7c56174fc 100644 --- a/store/storetest/file_info_store.go +++ b/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) - require.Nil(t, err) - assert.Len(t, postInfos, 2) + testCases := []struct { + Name string + 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) - require.Nil(t, err) - assert.Len(t, postInfos, 2) + 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, 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) } diff --git a/store/storetest/mocks/FileInfoStore.go b/store/storetest/mocks/FileInfoStore.go index 4c66f24a8a..4a87217349 100644 --- a/store/storetest/mocks/FileInfoStore.go +++ b/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 -func (_m *FileInfoStore) GetForPost(postId string, readFromMaster bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError) { - ret := _m.Called(postId, readFromMaster, allowFromCache) +// GetForPost provides a mock function with given fields: postId, readFromMaster, includeDeleted, allowFromCache +func (_m *FileInfoStore) GetForPost(postId string, readFromMaster bool, includeDeleted bool, allowFromCache bool) ([]*model.FileInfo, *model.AppError) { + ret := _m.Called(postId, readFromMaster, includeDeleted, allowFromCache) var r0 []*model.FileInfo - if rf, ok := ret.Get(0).(func(string, bool, bool) []*model.FileInfo); ok { - r0 = rf(postId, readFromMaster, allowFromCache) + if rf, ok := ret.Get(0).(func(string, bool, bool, bool) []*model.FileInfo); ok { + 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 { - r1 = rf(postId, readFromMaster, allowFromCache) + if rf, ok := ret.Get(1).(func(string, bool, bool, bool) *model.AppError); ok { + r1 = rf(postId, readFromMaster, includeDeleted, allowFromCache) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError)