From 3fc5287f6bd0b9439ae0c148756256314a4dfa58 Mon Sep 17 00:00:00 2001 From: Ashish Bhate Date: Sat, 15 Feb 2020 01:51:54 +0530 Subject: [PATCH] MM-17912: Allow searching for files through plugin API (#13647) * constants and options for getting files * Method to get files with options * Add i18n strings for en * Add API methods for getting files with options * gofmt -s file * explicitly set create at in tests * use greater than nanosecond time difference for tests * use gte instead of gt for getting files by created time * use created at time as default sort order for getting file infos * use explicit inline strings instead of format strings * join tables only when required * use if as secondary sort, and update tests * update field docs to reflect previous changes * make page and perPage get options as required * add json struct tags to GetFileOptions * bump minimum server versioni * remove sorting by username and channelname * use bool for sort order type * use FileInfo prefix instead of just File * clearer comments * use zero-based page numbering * test filtering by user and channel * remove unnecessary whitespace * use int instead of uint for page and perPage arguments Co-authored-by: mattermod --- app/file.go | 4 + app/plugin_api.go | 4 + app/plugin_api_test.go | 88 ++++++++++++++++ i18n/en.json | 4 + model/file_info.go | 21 ++++ plugin/api.go | 6 ++ plugin/client_rpc_generated.go | 31 ++++++ plugin/plugintest/api.go | 25 +++++ store/sqlstore/file_info_store.go | 70 +++++++++++++ store/store.go | 1 + store/storetest/file_info_store.go | 139 +++++++++++++++++++++++++ store/storetest/mocks/FileInfoStore.go | 25 +++++ 12 files changed, 418 insertions(+) diff --git a/app/file.go b/app/file.go index bb7e440ce7..1bc35a97bc 100644 --- a/app/file.go +++ b/app/file.go @@ -1097,6 +1097,10 @@ func (a *App) GetFileInfo(fileId string) (*model.FileInfo, *model.AppError) { return a.Srv().Store.FileInfo().Get(fileId) } +func (a *App) GetFileInfos(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + return a.Srv.Store.FileInfo().GetWithOptions(page, perPage, opt) +} + func (a *App) GetFile(fileId string) ([]byte, *model.AppError) { info, err := a.GetFileInfo(fileId) if err != nil { diff --git a/app/plugin_api.go b/app/plugin_api.go index 090c2d5b35..91baba1728 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -567,6 +567,10 @@ func (api *PluginAPI) GetFileInfo(fileId string) (*model.FileInfo, *model.AppErr return api.app.GetFileInfo(fileId) } +func (api *PluginAPI) GetFileInfos(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + return api.app.GetFileInfos(page, perPage, opt) +} + func (api *PluginAPI) GetFileLink(fileId string) (string, *model.AppError) { if !*api.app.Config().FileSettings.EnablePublicLink { return "", model.NewAppError("GetFileLink", "plugin_api.get_file_link.disabled.app_error", nil, "", http.StatusNotImplemented) diff --git a/app/plugin_api_test.go b/app/plugin_api_test.go index 922496de2d..0fcfab6779 100644 --- a/app/plugin_api_test.go +++ b/app/plugin_api_test.go @@ -350,6 +350,94 @@ func TestPluginAPIGetFile(t *testing.T) { require.Nil(t, data) } +func TestPluginAPIGetFileInfos(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + api := th.SetupPluginAPI() + + fileInfo1, err := th.App.DoUploadFile( + time.Date(2020, 1, 1, 1, 1, 1, 1, time.UTC), + th.BasicTeam.Id, + th.BasicChannel.Id, + th.BasicUser.Id, + "testFile1", + []byte("testfile1 Content"), + ) + require.Nil(t, err) + defer func() { + th.App.Srv.Store.FileInfo().PermanentDelete(fileInfo1.Id) + th.App.RemoveFile(fileInfo1.Path) + }() + + fileInfo2, err := th.App.DoUploadFile( + time.Date(2020, 1, 2, 1, 1, 1, 1, time.UTC), + th.BasicTeam.Id, + th.BasicChannel.Id, + th.BasicUser2.Id, + "testFile2", + []byte("testfile2 Content"), + ) + require.Nil(t, err) + defer func() { + th.App.Srv.Store.FileInfo().PermanentDelete(fileInfo2.Id) + th.App.RemoveFile(fileInfo2.Path) + }() + + fileInfo3, err := th.App.DoUploadFile( + time.Date(2020, 1, 3, 1, 1, 1, 1, time.UTC), + th.BasicTeam.Id, + th.BasicChannel.Id, + th.BasicUser.Id, + "testFile3", + []byte("testfile3 Content"), + ) + require.Nil(t, err) + defer func() { + th.App.Srv.Store.FileInfo().PermanentDelete(fileInfo3.Id) + th.App.RemoveFile(fileInfo3.Path) + }() + + _, err = api.CreatePost(&model.Post{ + Message: "testFile1", + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + FileIds: model.StringArray{fileInfo1.Id}, + }) + require.Nil(t, err) + + _, err = api.CreatePost(&model.Post{ + Message: "testFile2", + UserId: th.BasicUser2.Id, + ChannelId: th.BasicChannel.Id, + FileIds: model.StringArray{fileInfo2.Id}, + }) + require.Nil(t, err) + + t.Run("get file infos with no options 2nd page of 1 per page", func(t *testing.T) { + fileInfos, err := api.GetFileInfos(1, 1, nil) + require.Nil(t, err) + require.Len(t, fileInfos, 1) + }) + t.Run("get file infos filtered by user", func(t *testing.T) { + fileInfos, err := api.GetFileInfos(0, 5, &model.GetFileInfosOptions{ + UserIds: []string{th.BasicUser.Id}, + }) + require.Nil(t, err) + require.Len(t, fileInfos, 2) + }) + t.Run("get file infos filtered by channel ordered by created at descending", func(t *testing.T) { + fileInfos, err := api.GetFileInfos(0, 5, &model.GetFileInfosOptions{ + ChannelIds: []string{th.BasicChannel.Id}, + SortBy: model.FILEINFO_SORT_BY_CREATED, + SortDescending: true, + }) + require.Nil(t, err) + require.Len(t, fileInfos, 2) + require.Equal(t, fileInfos[0].Id, fileInfo2.Id) + require.Equal(t, fileInfos[1].Id, fileInfo1.Id) + }) +} + func TestPluginAPISavePluginConfig(t *testing.T) { th := Setup(t) defer th.TearDown() diff --git a/i18n/en.json b/i18n/en.json index 79aed2fc57..934ebed196 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6270,6 +6270,10 @@ "id": "store.sql_file_info.get_for_user_id.app_error", "translation": "Unable to get the file info for the user" }, + { + "id": "store.sql_file_info.get_with_options.app_error", + "translation": "Unable to get the file info with options" + }, { "id": "store.sql_file_info.permanent_delete.app_error", "translation": "Unable to permanently delete the file info" diff --git a/model/file_info.go b/model/file_info.go index 4e528ed0ba..20134cf456 100644 --- a/model/file_info.go +++ b/model/file_info.go @@ -15,6 +15,27 @@ import ( "strings" ) +const ( + FILEINFO_SORT_BY_CREATED = "CreateAt" + FILEINFO_SORT_BY_SIZE = "Size" +) + +// GetFileInfosOptions contains options for getting FileInfos +type GetFileInfosOptions struct { + // UserIds optionally limits the FileInfos to those created by the given users. + UserIds []string `json:"user_ids"` + // ChannelIds optionally limits the FileInfos to those created in the given channels. + ChannelIds []string `json:"channel_ids"` + // Since optionally limits FileInfos to those created at or after the given time, specified as Unix time in milliseconds. + Since int64 `json:"since"` + // IncludeDeleted if set includes deleted FileInfos. + IncludeDeleted bool `json:"include_deleted"` + // SortBy sorts the FileInfos by this field. The default is to sort by date created. + SortBy string `json:"sort_by"` + // SortDescending changes the sort direction to descending order when true. + SortDescending bool `json:"sort_descending"` +} + type FileInfo struct { Id string `json:"id"` CreatorId string `json:"user_id"` diff --git a/plugin/api.go b/plugin/api.go index 3df873a20f..7334165c27 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -659,6 +659,12 @@ type API interface { // Minimum server version: 5.3 GetFileInfo(fileId string) (*model.FileInfo, *model.AppError) + // GetFileInfos gets File Infos with options + // + // @tag File + // Minimum server version: 5.22 + GetFileInfos(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) + // GetFile gets content of a file by it's ID // // @tag File diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index 288854eabb..7c2ae442e5 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -3320,6 +3320,37 @@ func (s *apiRPCServer) GetFileInfo(args *Z_GetFileInfoArgs, returns *Z_GetFileIn return nil } +type Z_GetFileInfosArgs struct { + A int + B int + C *model.GetFileInfosOptions +} + +type Z_GetFileInfosReturns struct { + A []*model.FileInfo + B *model.AppError +} + +func (g *apiRPCClient) GetFileInfos(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + _args := &Z_GetFileInfosArgs{page, perPage, opt} + _returns := &Z_GetFileInfosReturns{} + if err := g.client.Call("Plugin.GetFileInfos", _args, _returns); err != nil { + log.Printf("RPC call to GetFileInfos API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) GetFileInfos(args *Z_GetFileInfosArgs, returns *Z_GetFileInfosReturns) error { + if hook, ok := s.impl.(interface { + GetFileInfos(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) + }); ok { + returns.A, returns.B = hook.GetFileInfos(args.A, args.B, args.C) + } else { + return encodableError(fmt.Errorf("API GetFileInfos called but not implemented.")) + } + return nil +} + type Z_GetFileArgs struct { A string } diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index c2c5417d28..51fa5b3416 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -1000,6 +1000,31 @@ func (_m *API) GetFileInfo(fileId string) (*model.FileInfo, *model.AppError) { return r0, r1 } +// GetFileInfos provides a mock function with given fields: page, perPage, opt +func (_m *API) GetFileInfos(page int, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + ret := _m.Called(page, perPage, opt) + + var r0 []*model.FileInfo + if rf, ok := ret.Get(0).(func(int, int, *model.GetFileInfosOptions) []*model.FileInfo); ok { + r0 = rf(page, perPage, opt) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.FileInfo) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(int, int, *model.GetFileInfosOptions) *model.AppError); ok { + r1 = rf(page, perPage, opt) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetFileLink provides a mock function with given fields: fileId func (_m *API) GetFileLink(fileId string) (string, *model.AppError) { ret := _m.Called(fileId) diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index 0189723552..78cb4cefda 100644 --- a/store/sqlstore/file_info_store.go +++ b/store/sqlstore/file_info_store.go @@ -5,6 +5,7 @@ package sqlstore import ( "database/sql" + "fmt" "net/http" sq "github.com/Masterminds/squirrel" @@ -95,6 +96,75 @@ func (fs SqlFileInfoStore) Get(id string) (*model.FileInfo, *model.AppError) { return info, nil } +func (fs SqlFileInfoStore) GetWithOptions(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + if perPage < 0 || page < 0 { + return nil, model.NewAppError("SqlFileInfoStore.GetWithOptions", + "store.sql_file_info.get_with_options.app_error", nil, fmt.Sprintf("page=%d and perPage=%d must be non-negative", page, perPage), http.StatusBadRequest) + } + if perPage == 0 { + return nil, nil + } + + if opt == nil { + opt = &model.GetFileInfosOptions{} + } + + query := fs.getQueryBuilder(). + Select("FileInfo.*"). + From("FileInfo") + + if len(opt.ChannelIds) > 0 { + query = query.Join("Posts ON FileInfo.PostId = Posts.Id"). + Where(sq.Eq{"Posts.ChannelId": opt.ChannelIds}) + } + + if len(opt.UserIds) > 0 { + query = query.Where(sq.Eq{"FileInfo.CreatorId": opt.UserIds}) + } + + if opt.Since > 0 { + query = query.Where(sq.GtOrEq{"FileInfo.CreateAt": opt.Since}) + } + + if !opt.IncludeDeleted { + query = query.Where("FileInfo.DeleteAt = 0") + } + + if opt.SortBy == "" { + opt.SortBy = model.FILEINFO_SORT_BY_CREATED + } + sortDirection := "ASC" + if opt.SortDescending { + sortDirection = "DESC" + } + + switch opt.SortBy { + case model.FILEINFO_SORT_BY_CREATED: + query = query.OrderBy("FileInfo.CreateAt " + sortDirection) + case model.FILEINFO_SORT_BY_SIZE: + query = query.OrderBy("FileInfo.Size " + sortDirection) + default: + return nil, model.NewAppError("SqlFileInfoStore.GetWithOptions", + "store.sql_file_info.get_with_options.app_error", nil, "invalid sort option", http.StatusBadRequest) + } + + query = query.OrderBy("FileInfo.Id ASC") // secondary sort for sort stability + + query = query.Limit(uint64(perPage)).Offset(uint64(perPage * page)) + + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlFileInfoStore.GetWithOptions", + "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + } + var infos []*model.FileInfo + if _, err := fs.GetReplica().Select(&infos, queryString, args...); err != nil { + return nil, model.NewAppError("SqlFileInfoStore.GetWithOptions", + "store.sql_file_info.get_with_options.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return infos, nil +} + func (fs SqlFileInfoStore) GetByPath(path string) (*model.FileInfo, *model.AppError) { info := &model.FileInfo{} diff --git a/store/store.go b/store/store.go index c0fce55baa..dfcaf67dc1 100644 --- a/store/store.go +++ b/store/store.go @@ -494,6 +494,7 @@ type FileInfoStore interface { GetByPath(path string) (*model.FileInfo, *model.AppError) GetForPost(postId string, readFromMaster, includeDeleted, allowFromCache bool) ([]*model.FileInfo, *model.AppError) GetForUser(userId string) ([]*model.FileInfo, *model.AppError) + GetWithOptions(page, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) InvalidateFileInfosForPostCache(postId string) AttachToPost(fileId string, postId string, creatorId string) *model.AppError DeleteForPost(postId string) (string, *model.AppError) diff --git a/store/storetest/file_info_store.go b/store/storetest/file_info_store.go index 33cd81ae51..3b247c5279 100644 --- a/store/storetest/file_info_store.go +++ b/store/storetest/file_info_store.go @@ -7,6 +7,7 @@ import ( "fmt" "sort" "testing" + "time" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" @@ -20,6 +21,7 @@ func TestFileInfoStore(t *testing.T, ss store.Store) { t.Run("FileInfoSaveGetByPath", func(t *testing.T) { testFileInfoSaveGetByPath(t, ss) }) t.Run("FileInfoGetForPost", func(t *testing.T) { testFileInfoGetForPost(t, ss) }) t.Run("FileInfoGetForUser", func(t *testing.T) { testFileInfoGetForUser(t, ss) }) + t.Run("FileInfoGetWithOptions", func(t *testing.T) { testFileInfoGetWithOptions(t, ss) }) t.Run("FileInfoAttachToPost", func(t *testing.T) { testFileInfoAttachToPost(t, ss) }) t.Run("FileInfoDeleteForPost", func(t *testing.T) { testFileInfoDeleteForPost(t, ss) }) t.Run("FileInfoPermanentDelete", func(t *testing.T) { testFileInfoPermanentDelete(t, ss) }) @@ -256,6 +258,143 @@ func testFileInfoGetForUser(t *testing.T, ss store.Store) { assert.Len(t, userPosts, 1) } +func testFileInfoGetWithOptions(t *testing.T, ss store.Store) { + makePost := func(chId string, user string) model.Post { + post := model.Post{} + post.ChannelId = chId + post.UserId = user + _, err := ss.Post().Save(&post) + require.Nil(t, err) + return post + } + + makeFile := func(post model.Post, user string, createAt int64, idPrefix string) model.FileInfo { + id := model.NewId() + id = idPrefix + id[1:] // hacky way to get sortable Ids to confirm secondary Id sort works + fileInfo := model.FileInfo{ + Id: id, + CreatorId: user, + Path: "file.txt", + CreateAt: createAt, + } + if post.Id != "" { + fileInfo.PostId = post.Id + } + _, err := ss.FileInfo().Save(&fileInfo) + require.Nil(t, err) + return fileInfo + } + + userId1 := model.NewId() + userId2 := model.NewId() + + channelId1 := model.NewId() + channelId2 := model.NewId() + channelId3 := model.NewId() + + post1_1 := makePost(channelId1, userId1) // post 1 by user 1 + post1_2 := makePost(channelId3, userId1) // post 2 by user 1 + post2_1 := makePost(channelId2, userId2) + post2_2 := makePost(channelId3, userId2) + + epoch := time.Date(2020, 1, 1, 1, 1, 1, 1, time.UTC) + file1_1 := makeFile(post1_1, userId1, epoch.AddDate(0, 0, 1).Unix(), "a") // file 1 by user 1 + file1_2 := makeFile(post1_2, userId1, epoch.AddDate(0, 0, 2).Unix(), "b") // file 2 by user 1 + file1_3 := makeFile(model.Post{}, userId1, epoch.AddDate(0, 0, 3).Unix(), "c") // file that is not attached to a post + file2_1 := makeFile(post2_1, userId2, epoch.AddDate(0, 0, 4).Unix(), "d") // file 2 by user 1 + file2_2 := makeFile(post2_2, userId2, epoch.AddDate(0, 0, 5).Unix(), "e") + + // delete a file + _, err := ss.FileInfo().DeleteForPost(file2_2.PostId) + require.Nil(t, err) + + testCases := []struct { + Name string + Page, PerPage int + Opt *model.GetFileInfosOptions + ExpectedFileIds []string + }{ + { + Name: "Get files with nil option", + Page: 0, + PerPage: 10, + Opt: nil, + ExpectedFileIds: []string{file1_1.Id, file1_2.Id, file1_3.Id, file2_1.Id}, + }, + { + Name: "Get files including deleted", + Page: 0, + PerPage: 10, + Opt: &model.GetFileInfosOptions{IncludeDeleted: true}, + ExpectedFileIds: []string{file1_1.Id, file1_2.Id, file1_3.Id, file2_1.Id, file2_2.Id}, + }, + { + Name: "Get files including deleted filtered by channel", + Page: 0, + PerPage: 10, + Opt: &model.GetFileInfosOptions{ + IncludeDeleted: true, + ChannelIds: []string{channelId3}, + }, + ExpectedFileIds: []string{file1_2.Id, file2_2.Id}, + }, + { + Name: "Get files including deleted filtered by channel and user", + Page: 0, + PerPage: 10, + Opt: &model.GetFileInfosOptions{ + IncludeDeleted: true, + UserIds: []string{userId1}, + ChannelIds: []string{channelId3}, + }, + ExpectedFileIds: []string{file1_2.Id}, + }, + { + Name: "Get files including deleted sorted by created at", + Page: 0, + PerPage: 10, + Opt: &model.GetFileInfosOptions{ + IncludeDeleted: true, + SortBy: model.FILEINFO_SORT_BY_CREATED, + }, + ExpectedFileIds: []string{file1_1.Id, file1_2.Id, file1_3.Id, file2_1.Id, file2_2.Id}, + }, + { + Name: "Get files filtered by user ordered by created at descending", + Page: 0, + PerPage: 10, + Opt: &model.GetFileInfosOptions{ + UserIds: []string{userId1}, + SortBy: model.FILEINFO_SORT_BY_CREATED, + SortDescending: true, + }, + ExpectedFileIds: []string{file1_3.Id, file1_2.Id, file1_1.Id}, + }, + { + Name: "Get all files including deleted ordered by created descending 2nd page of 3 per page ", + Page: 1, + PerPage: 3, + Opt: &model.GetFileInfosOptions{ + IncludeDeleted: true, + SortBy: model.FILEINFO_SORT_BY_CREATED, + SortDescending: true, + }, + ExpectedFileIds: []string{file1_2.Id, file1_1.Id}, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + fileInfos, err := ss.FileInfo().GetWithOptions(tc.Page, tc.PerPage, tc.Opt) + require.Nil(t, err) + require.Len(t, fileInfos, len(tc.ExpectedFileIds)) + for i := range tc.ExpectedFileIds { + assert.Equal(t, tc.ExpectedFileIds[i], fileInfos[i].Id) + } + }) + } +} + type byFileInfoId []*model.FileInfo func (a byFileInfoId) Len() int { return len(a) } diff --git a/store/storetest/mocks/FileInfoStore.go b/store/storetest/mocks/FileInfoStore.go index 2ee24bde71..e327aa466c 100644 --- a/store/storetest/mocks/FileInfoStore.go +++ b/store/storetest/mocks/FileInfoStore.go @@ -158,6 +158,31 @@ func (_m *FileInfoStore) GetForUser(userId string) ([]*model.FileInfo, *model.Ap return r0, r1 } +// GetWithOptions provides a mock function with given fields: page, perPage, opt +func (_m *FileInfoStore) GetWithOptions(page int, perPage int, opt *model.GetFileInfosOptions) ([]*model.FileInfo, *model.AppError) { + ret := _m.Called(page, perPage, opt) + + var r0 []*model.FileInfo + if rf, ok := ret.Get(0).(func(int, int, *model.GetFileInfosOptions) []*model.FileInfo); ok { + r0 = rf(page, perPage, opt) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.FileInfo) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(int, int, *model.GetFileInfosOptions) *model.AppError); ok { + r1 = rf(page, perPage, opt) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // InvalidateFileInfosForPostCache provides a mock function with given fields: postId func (_m *FileInfoStore) InvalidateFileInfosForPostCache(postId string) { _m.Called(postId)