From 072c859219741782385d0a814d8b9010ce5d327a Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 27 Jul 2022 19:02:24 +0530 Subject: [PATCH] MM-43828: Pass object length for some image operations (#20711) For user profile and plugin upload, we use a bytes.Buffer. In that case, we know the object size and can find it out from the length of the buffer. This helps reduce multi-part uploads. This approach can also be taken in thumbnail and preview images. However, they use an io.Pipe to directly upload the image as it is being encoded. We could make the whole process in separate parts of writing the full image in the buffer and then upload it. But taking a conservative approach for now. Also, while here, removed some unused code. https://mattermost.atlassian.net/browse/MM-43828 ```release-note NONE ``` --- app/app_iface.go | 5 -- app/file.go | 71 ---------------------------- app/file_bench_test.go | 17 ------- app/opentracing/opentracing_layer.go | 44 ----------------- app/plugin_hooks_test.go | 49 +++++++------------ i18n/en.json | 4 -- shared/filestore/s3store.go | 15 +++++- 7 files changed, 30 insertions(+), 175 deletions(-) diff --git a/app/app_iface.go b/app/app_iface.go index 49f298ab40..3049aaa3de 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -381,10 +381,6 @@ type AppIface interface { // upload, returning a rejection error. In this case FileInfo would have // contained the last "good" FileInfo before the execution of that plugin. UploadFileX(c *request.Context, channelID, name string, input io.Reader, opts ...func(*UploadFileTask)) (*model.FileInfo, *model.AppError) - // Uploads some files to the given team and channel as the given user. files and filenames should have - // the same length. clientIds should either not be provided or have the same length as files and filenames. - // The provided files should be closed by the caller so that they are not leaked. - UploadFiles(c *request.Context, teamID string, channelID string, userID string, files []io.ReadCloser, filenames []string, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) // UserIsInAdminRoleGroup returns true at least one of the user's groups are configured to set the members as // admins in the given syncable. UserIsInAdminRoleGroup(userID, syncableID string, syncableType model.GroupSyncableType) (bool, *model.AppError) @@ -1132,7 +1128,6 @@ type AppIface interface { UpdateUserRolesWithUser(user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UploadData(c *request.Context, us *model.UploadSession, rd io.Reader) (*model.FileInfo, *model.AppError) UploadEmojiImage(id string, imageData *multipart.FileHeader) *model.AppError - UploadMultipartFiles(c *request.Context, teamID string, channelID string, userID string, fileHeaders []*multipart.FileHeader, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) UpsertGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) UpsertGroupMembers(groupID string, userIDs []string) ([]*model.GroupMember, *model.AppError) UpsertGroupSyncable(groupSyncable *model.GroupSyncable) (*model.GroupSyncable, *model.AppError) diff --git a/app/file.go b/app/file.go index 12c954de33..3ec52cb698 100644 --- a/app/file.go +++ b/app/file.go @@ -12,7 +12,6 @@ import ( "fmt" "image" "io" - "mime/multipart" "net/http" "net/url" "os" @@ -453,76 +452,6 @@ func GeneratePublicLinkHash(fileID, salt string) string { return base64.RawURLEncoding.EncodeToString(hash.Sum(nil)) } -func (a *App) UploadMultipartFiles(c *request.Context, teamID string, channelID string, userID string, fileHeaders []*multipart.FileHeader, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) { - files := make([]io.ReadCloser, len(fileHeaders)) - filenames := make([]string, len(fileHeaders)) - - for i, fileHeader := range fileHeaders { - file, fileErr := fileHeader.Open() - if fileErr != nil { - return nil, model.NewAppError("UploadFiles", "api.file.upload_file.read_request.app_error", - map[string]any{"Filename": fileHeader.Filename}, fileErr.Error(), http.StatusBadRequest) - } - - // Will be closed after UploadFiles returns - defer file.Close() - - files[i] = file - filenames[i] = fileHeader.Filename - } - - return a.UploadFiles(c, teamID, channelID, userID, files, filenames, clientIds, now) -} - -// Uploads some files to the given team and channel as the given user. files and filenames should have -// the same length. clientIds should either not be provided or have the same length as files and filenames. -// The provided files should be closed by the caller so that they are not leaked. -func (a *App) UploadFiles(c *request.Context, teamID string, channelID string, userID string, files []io.ReadCloser, filenames []string, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) { - if *a.Config().FileSettings.DriverName == "" { - return nil, model.NewAppError("UploadFiles", "api.file.upload_file.storage.app_error", nil, "", http.StatusNotImplemented) - } - - if len(filenames) != len(files) || (len(clientIds) > 0 && len(clientIds) != len(files)) { - return nil, model.NewAppError("UploadFiles", "api.file.upload_file.incorrect_number_of_files.app_error", nil, "", http.StatusBadRequest) - } - - resStruct := &model.FileUploadResponse{ - FileInfos: []*model.FileInfo{}, - ClientIds: []string{}, - } - - previewPathList := []string{} - thumbnailPathList := []string{} - imageDataList := [][]byte{} - - for i, file := range files { - buf := bytes.NewBuffer(nil) - io.Copy(buf, file) - data := buf.Bytes() - - info, data, err := a.DoUploadFileExpectModification(c, now, teamID, channelID, userID, filenames[i], data) - if err != nil { - return nil, err - } - - if info.PreviewPath != "" || info.ThumbnailPath != "" { - previewPathList = append(previewPathList, info.PreviewPath) - thumbnailPathList = append(thumbnailPathList, info.ThumbnailPath) - imageDataList = append(imageDataList, data) - } - - resStruct.FileInfos = append(resStruct.FileInfos, info) - - if len(clientIds) > 0 { - resStruct.ClientIds = append(resStruct.ClientIds, clientIds[i]) - } - } - - a.HandleImages(previewPathList, thumbnailPathList, imageDataList) - - return resStruct, nil -} - // UploadFile uploads a single file in form of a completely constructed byte array for a channel. func (a *App) UploadFile(c *request.Context, data []byte, channelID string, filename string) (*model.FileInfo, *model.AppError) { _, err := a.GetChannel(c, channelID) diff --git a/app/file_bench_test.go b/app/file_bench_test.go index 515f69cc03..608bd1eb31 100644 --- a/app/file_bench_test.go +++ b/app/file_bench_test.go @@ -9,8 +9,6 @@ import ( "image" "image/gif" "image/jpeg" - "io" - "io/ioutil" "math/rand" "testing" "time" @@ -130,21 +128,6 @@ func BenchmarkUploadFile(b *testing.B) { th.App.RemoveFile(info.Path) }, }, - { - title: "image UploadFiles", - f: func(b *testing.B, n int, data []byte, ext string) { - resp, err := th.App.UploadFiles(th.Context, teamID, channelID, userID, - []io.ReadCloser{ioutil.NopCloser(bytes.NewReader(data))}, - []string{fmt.Sprintf("BenchmarkDoUploadFiles-%d%s", n, ext)}, - []string{}, - time.Now()) - if err != nil { - b.Fatal(err) - } - th.App.Srv().Store.FileInfo().PermanentDelete(resp.FileInfos[0].Id) - th.App.RemoveFile(resp.FileInfos[0].Path) - }, - }, { title: "image UploadFileX Content-Length", f: func(b *testing.B, n int, data []byte, ext string) { diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index c8eaca45fe..b71e18b9e5 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -17813,50 +17813,6 @@ func (a *OpenTracingAppLayer) UploadFileX(c *request.Context, channelID string, return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) UploadFiles(c *request.Context, teamID string, channelID string, userID string, files []io.ReadCloser, filenames []string, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UploadFiles") - - a.ctx = newCtx - a.app.Srv().Store.SetContext(newCtx) - defer func() { - a.app.Srv().Store.SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - resultVar0, resultVar1 := a.app.UploadFiles(c, teamID, channelID, userID, files, filenames, clientIds, now) - - if resultVar1 != nil { - span.LogFields(spanlog.Error(resultVar1)) - ext.Error.Set(span, true) - } - - return resultVar0, resultVar1 -} - -func (a *OpenTracingAppLayer) UploadMultipartFiles(c *request.Context, teamID string, channelID string, userID string, fileHeaders []*multipart.FileHeader, clientIds []string, now time.Time) (*model.FileUploadResponse, *model.AppError) { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UploadMultipartFiles") - - a.ctx = newCtx - a.app.Srv().Store.SetContext(newCtx) - defer func() { - a.app.Srv().Store.SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - resultVar0, resultVar1 := a.app.UploadMultipartFiles(c, teamID, channelID, userID, fileHeaders, clientIds, now) - - if resultVar1 != nil { - span.LogFields(spanlog.Error(resultVar1)) - ext.Error.Set(span, true) - } - - return resultVar0, resultVar1 -} - func (a *OpenTracingAppLayer) UpsertGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UpsertGroupMember") diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go index 32945cfed7..e4eaa4d23e 100644 --- a/app/plugin_hooks_test.go +++ b/app/plugin_hooks_test.go @@ -462,15 +462,12 @@ func TestHookFileWillBeUploaded(t *testing.T) { }, th.App, func(*model.Manifest) plugin.API { return &mockAPI }) defer tearDown() - _, err := th.App.UploadFiles(th.Context, - "noteam", + _, err := th.App.UploadFile(th.Context, + []byte("inputfile"), th.BasicChannel.Id, - th.BasicUser.Id, - []io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))}, - []string{"testhook.txt"}, - []string{}, - time.Now(), + "testhook.txt", ) + if assert.NotNil(t, err) { assert.Equal(t, "File rejected by plugin. rejected", err.Message) } @@ -515,15 +512,12 @@ func TestHookFileWillBeUploaded(t *testing.T) { }, th.App, func(*model.Manifest) plugin.API { return &mockAPI }) defer tearDown() - _, err := th.App.UploadFiles(th.Context, - "noteam", + _, err := th.App.UploadFile(th.Context, + []byte("inputfile"), th.BasicChannel.Id, - th.BasicUser.Id, - []io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))}, - []string{"testhook.txt"}, - []string{}, - time.Now(), + "testhook.txt", ) + if assert.NotNil(t, err) { assert.Equal(t, "File rejected by plugin. rejected", err.Message) } @@ -562,20 +556,16 @@ func TestHookFileWillBeUploaded(t *testing.T) { }, th.App, func(*model.Manifest) plugin.API { return &mockAPI }) defer tearDown() - response, err := th.App.UploadFiles(th.Context, - "noteam", + response, err := th.App.UploadFile(th.Context, + []byte("inputfile"), th.BasicChannel.Id, - th.BasicUser.Id, - []io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))}, - []string{"testhook.txt"}, - []string{}, - time.Now(), + "testhook.txt", ) + assert.Nil(t, err) assert.NotNil(t, response) - assert.Equal(t, 1, len(response.FileInfos)) - fileID := response.FileInfos[0].Id + fileID := response.Id fileInfo, err := th.App.GetFileInfo(fileID) assert.Nil(t, err) assert.NotNil(t, fileInfo) @@ -638,19 +628,14 @@ func TestHookFileWillBeUploaded(t *testing.T) { }, th.App, func(*model.Manifest) plugin.API { return &mockAPI }) defer tearDown() - response, err := th.App.UploadFiles(th.Context, - "noteam", + response, err := th.App.UploadFile(th.Context, + []byte("inputfile"), th.BasicChannel.Id, - th.BasicUser.Id, - []io.ReadCloser{ioutil.NopCloser(bytes.NewBufferString("inputfile"))}, - []string{"testhook.txt"}, - []string{}, - time.Now(), + "testhook.txt", ) assert.Nil(t, err) assert.NotNil(t, response) - assert.Equal(t, 1, len(response.FileInfos)) - fileID := response.FileInfos[0].Id + fileID := response.Id fileInfo, err := th.App.GetFileInfo(fileID) assert.Nil(t, err) diff --git a/i18n/en.json b/i18n/en.json index 5ad9f74c1d..146a1c8f2c 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1857,10 +1857,6 @@ "id": "api.file.upload_file.incorrect_number_of_client_ids.app_error", "translation": "Unable to upload file(s). Have {{.NumClientIds}} client_ids for {{.NumFiles}} files." }, - { - "id": "api.file.upload_file.incorrect_number_of_files.app_error", - "translation": "Unable to upload files. Incorrect number of files specified." - }, { "id": "api.file.upload_file.large_image.app_error", "translation": "File above maximum dimensions could not be uploaded: {{.Filename}}" diff --git a/shared/filestore/s3store.go b/shared/filestore/s3store.go index 229c3537a5..06d6bec7ed 100644 --- a/shared/filestore/s3store.go +++ b/shared/filestore/s3store.go @@ -4,6 +4,7 @@ package filestore import ( + "bytes" "context" "crypto/tls" "io" @@ -365,7 +366,13 @@ func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, error) { ctx, cancel := context.WithTimeout(context.Background(), b.timeout) defer cancel() options := s3PutOptions(b.encrypt, contentType) - info, err := b.client.PutObject(ctx, b.bucket, path, fr, -1, options) + + objSize := -1 + if buf, ok := fr.(*bytes.Buffer); ok { + objSize = buf.Len() + } + + info, err := b.client.PutObject(ctx, b.bucket, path, fr, int64(objSize), options) if err != nil { return info.Size, errors.Wrapf(err, "unable write the data in the file %s", path) } @@ -393,7 +400,11 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, error) { partName := fp + ".part" ctx2, cancel2 := context.WithTimeout(context.Background(), b.timeout) defer cancel2() - info, err := b.client.PutObject(ctx2, b.bucket, partName, fr, -1, options) + objSize := -1 + if buf, ok := fr.(*bytes.Buffer); ok { + objSize = buf.Len() + } + info, err := b.client.PutObject(ctx2, b.bucket, partName, fr, int64(objSize), options) if err != nil { return 0, errors.Wrapf(err, "unable append the data in the file %s", path) }