From 2b27e12445ba51e1fa1ab2aceac5fcb3de66845d Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 10 May 2018 18:16:33 -0400 Subject: MM-10188: expect io.Reader in FileBackend.WriteFile (#8765) This is a reworked set of changes originally from @josephGuo to begin reducing the duplicated memory required when uploading files. --- api/emoji_test.go | 2 +- app/emoji.go | 7 +++-- app/file.go | 13 +++++---- app/team.go | 2 +- app/user.go | 4 +-- utils/file_backend.go | 3 +- utils/file_backend_local.go | 28 +++++++++++-------- utils/file_backend_s3.go | 14 +++++----- utils/file_backend_test.go | 67 +++++++++++++++++++++++++++++++++------------ utils/mail_test.go | 6 ++-- 10 files changed, 95 insertions(+), 51 deletions(-) diff --git a/api/emoji_test.go b/api/emoji_test.go index 69922a54a..108c416e1 100644 --- a/api/emoji_test.go +++ b/api/emoji_test.go @@ -272,7 +272,7 @@ func TestDeleteEmoji(t *testing.T) { func createTestEmoji(t *testing.T, a *app.App, emoji *model.Emoji, imageData []byte) *model.Emoji { emoji = store.Must(a.Srv.Store.Emoji().Save(emoji)).(*model.Emoji) - if err := a.WriteFile(imageData, "emoji/"+emoji.Id+"/image"); err != nil { + if _, err := a.WriteFile(bytes.NewReader(imageData), "emoji/"+emoji.Id+"/image"); err != nil { store.Must(a.Srv.Store.Emoji().Delete(emoji.Id, time.Now().Unix())) t.Fatalf("failed to write image: %v", err.Error()) } diff --git a/app/emoji.go b/app/emoji.go index 2f957fbcc..b07331e65 100644 --- a/app/emoji.go +++ b/app/emoji.go @@ -98,7 +98,7 @@ func (a *App) UploadEmojiImage(id string, imageData *multipart.FileHeader) *mode if err := gif.EncodeAll(newbuf, resized_gif); err != nil { return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.gif_encode_error", nil, "", http.StatusBadRequest) } - if err := a.WriteFile(newbuf.Bytes(), getEmojiImagePath(id)); err != nil { + if _, err := a.WriteFile(newbuf, getEmojiImagePath(id)); err != nil { return err } } @@ -110,14 +110,15 @@ func (a *App) UploadEmojiImage(id string, imageData *multipart.FileHeader) *mode if err := png.Encode(newbuf, resized_image); err != nil { return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.encode_error", nil, "", http.StatusBadRequest) } - if err := a.WriteFile(newbuf.Bytes(), getEmojiImagePath(id)); err != nil { + if _, err := a.WriteFile(newbuf, getEmojiImagePath(id)); err != nil { return err } } } } - return a.WriteFile(buf.Bytes(), getEmojiImagePath(id)) + _, appErr := a.WriteFile(buf, getEmojiImagePath(id)) + return appErr } func (a *App) DeleteEmoji(emoji *model.Emoji) *model.AppError { diff --git a/app/file.go b/app/file.go index 87e1986a2..cb8d54cb1 100644 --- a/app/file.go +++ b/app/file.go @@ -78,12 +78,13 @@ func (a *App) MoveFile(oldPath, newPath string) *model.AppError { return backend.MoveFile(oldPath, newPath) } -func (a *App) WriteFile(f []byte, path string) *model.AppError { +func (a *App) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { backend, err := a.FileBackend() if err != nil { - return err + return 0, err } - return backend.WriteFile(f, path) + + return backend.WriteFile(fr, path) } func (a *App) RemoveFile(path string) *model.AppError { @@ -414,7 +415,7 @@ func (a *App) DoUploadFile(now time.Time, rawTeamId string, rawChannelId string, info.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb.jpg" } - if err := a.WriteFile(data, info.Path); err != nil { + if _, err := a.WriteFile(bytes.NewReader(data), info.Path); err != nil { return nil, err } @@ -531,7 +532,7 @@ func (a *App) generateThumbnailImage(img image.Image, thumbnailPath string, widt return } - if err := a.WriteFile(buf.Bytes(), thumbnailPath); err != nil { + if _, err := a.WriteFile(buf, thumbnailPath); err != nil { mlog.Error(fmt.Sprintf("Unable to upload thumbnail path=%v err=%v", thumbnailPath, err)) return } @@ -553,7 +554,7 @@ func (a *App) generatePreviewImage(img image.Image, previewPath string, width in return } - if err := a.WriteFile(buf.Bytes(), previewPath); err != nil { + if _, err := a.WriteFile(buf, previewPath); err != nil { mlog.Error(fmt.Sprintf("Unable to upload preview err=%v", err), mlog.String("path", previewPath)) return } diff --git a/app/team.go b/app/team.go index f5235792f..aca99dd1e 100644 --- a/app/team.go +++ b/app/team.go @@ -1001,7 +1001,7 @@ func (a *App) SetTeamIconFromFile(teamId string, file multipart.File) *model.App path := "teams/" + teamId + "/teamIcon.png" - if err := a.WriteFile(buf.Bytes(), path); err != nil { + if _, err := a.WriteFile(buf, path); err != nil { return model.NewAppError("SetTeamIcon", "api.team.set_team_icon.write_file.app_error", nil, "", http.StatusInternalServerError) } diff --git a/app/user.go b/app/user.go index 2b0442e75..2ee410684 100644 --- a/app/user.go +++ b/app/user.go @@ -754,7 +754,7 @@ func (a *App) GetProfileImage(user *model.User) ([]byte, bool, *model.AppError) } if user.LastPictureUpdate == 0 { - if err := a.WriteFile(img, path); err != nil { + if _, err := a.WriteFile(bytes.NewReader(img), path); err != nil { return nil, false, err } } @@ -810,7 +810,7 @@ func (a *App) SetProfileImageFromFile(userId string, file multipart.File) *model path := "users/" + userId + "/profile.png" - if err := a.WriteFile(buf.Bytes(), path); err != nil { + if _, err := a.WriteFile(buf, path); err != nil { return model.NewAppError("SetProfileImage", "api.user.upload_profile_user.upload_profile.app_error", nil, "", http.StatusInternalServerError) } diff --git a/utils/file_backend.go b/utils/file_backend.go index 42af7f604..0da0bc3b8 100644 --- a/utils/file_backend.go +++ b/utils/file_backend.go @@ -4,6 +4,7 @@ package utils import ( + "io" "net/http" "github.com/mattermost/mattermost-server/model" @@ -15,7 +16,7 @@ type FileBackend interface { ReadFile(path string) ([]byte, *model.AppError) CopyFile(oldPath, newPath string) *model.AppError MoveFile(oldPath, newPath string) *model.AppError - WriteFile(f []byte, path string) *model.AppError + WriteFile(fr io.Reader, path string) (int64, *model.AppError) RemoveFile(path string) *model.AppError ListDirectory(path string) (*[]string, *model.AppError) diff --git a/utils/file_backend_local.go b/utils/file_backend_local.go index f85ace55a..37bca7987 100644 --- a/utils/file_backend_local.go +++ b/utils/file_backend_local.go @@ -4,6 +4,8 @@ package utils import ( + "bytes" + "io" "io/ioutil" "net/http" "os" @@ -22,8 +24,8 @@ type LocalFileBackend struct { } func (b *LocalFileBackend) TestConnection() *model.AppError { - f := []byte("testingwrite") - if err := writeFileLocally(f, filepath.Join(b.directory, TEST_FILE_PATH)); err != nil { + f := bytes.NewReader([]byte("testingwrite")) + if _, err := writeFileLocally(f, filepath.Join(b.directory, TEST_FILE_PATH)); err != nil { return model.NewAppError("TestFileConnection", "Don't have permissions to write to local path specified or other error.", nil, err.Error(), http.StatusInternalServerError) } os.Remove(filepath.Join(b.directory, TEST_FILE_PATH)) @@ -58,21 +60,25 @@ func (b *LocalFileBackend) MoveFile(oldPath, newPath string) *model.AppError { return nil } -func (b *LocalFileBackend) WriteFile(f []byte, path string) *model.AppError { - return writeFileLocally(f, filepath.Join(b.directory, path)) +func (b *LocalFileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { + return writeFileLocally(fr, filepath.Join(b.directory, path)) } -func writeFileLocally(f []byte, path string) *model.AppError { +func writeFileLocally(fr io.Reader, path string) (int64, *model.AppError) { if err := os.MkdirAll(filepath.Dir(path), 0774); err != nil { directory, _ := filepath.Abs(filepath.Dir(path)) - return model.NewAppError("WriteFile", "api.file.write_file_locally.create_dir.app_error", nil, "directory="+directory+", err="+err.Error(), http.StatusInternalServerError) + return 0, model.NewAppError("WriteFile", "api.file.write_file_locally.create_dir.app_error", nil, "directory="+directory+", err="+err.Error(), http.StatusInternalServerError) } - - if err := ioutil.WriteFile(path, f, 0644); err != nil { - return model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError) + fw, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + if err != nil { + return 0, model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError) } - - return nil + defer fw.Close() + written, err := io.Copy(fw, fr) + if err != nil { + return written, model.NewAppError("WriteFile", "api.file.write_file_locally.writing.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return written, nil } func (b *LocalFileBackend) RemoveFile(path string) *model.AppError { diff --git a/utils/file_backend_s3.go b/utils/file_backend_s3.go index 87ed3dd16..1772f09ea 100644 --- a/utils/file_backend_s3.go +++ b/utils/file_backend_s3.go @@ -4,7 +4,7 @@ package utils import ( - "bytes" + "io" "io/ioutil" "net/http" "os" @@ -136,10 +136,10 @@ func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { return nil } -func (b *S3FileBackend) WriteFile(f []byte, path string) *model.AppError { +func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { s3Clnt, err := b.s3New() if err != nil { - return model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + return 0, model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } var contentType string @@ -150,12 +150,12 @@ func (b *S3FileBackend) WriteFile(f []byte, path string) *model.AppError { } options := s3PutOptions(b.encrypt, contentType) - - if _, err = s3Clnt.PutObject(b.bucket, path, bytes.NewReader(f), -1, options); err != nil { - return model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + written, err := s3Clnt.PutObject(b.bucket, path, fr, -1, options) + if err != nil { + return written, model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } - return nil + return written, nil } func (b *S3FileBackend) RemoveFile(path string) *model.AppError { diff --git a/utils/file_backend_test.go b/utils/file_backend_test.go index 047e9df62..7f8265d73 100644 --- a/utils/file_backend_test.go +++ b/utils/file_backend_test.go @@ -4,6 +4,7 @@ package utils import ( + "bytes" "fmt" "io/ioutil" "os" @@ -95,7 +96,9 @@ func (s *FileBackendTestSuite) TestReadWriteFile() { b := []byte("test") path := "tests/" + model.NewId() - s.Nil(s.backend.WriteFile(b, path)) + written, err := s.backend.WriteFile(bytes.NewReader(b), path) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path) read, err := s.backend.ReadFile(path) @@ -109,7 +112,9 @@ func (s *FileBackendTestSuite) TestReadWriteFileImage() { b := []byte("testimage") path := "tests/" + model.NewId() + ".png" - s.Nil(s.backend.WriteFile(b, path)) + written, err := s.backend.WriteFile(bytes.NewReader(b), path) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path) read, err := s.backend.ReadFile(path) @@ -124,8 +129,9 @@ func (s *FileBackendTestSuite) TestCopyFile() { path1 := "tests/" + model.NewId() path2 := "tests/" + model.NewId() - err := s.backend.WriteFile(b, path1) + written, err := s.backend.WriteFile(bytes.NewReader(b), path1) s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path1) err = s.backend.CopyFile(path1, path2) @@ -144,8 +150,9 @@ func (s *FileBackendTestSuite) TestCopyFileToDirectoryThatDoesntExist() { path1 := "tests/" + model.NewId() path2 := "tests/newdirectory/" + model.NewId() - err := s.backend.WriteFile(b, path1) + written, err := s.backend.WriteFile(bytes.NewReader(b), path1) s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path1) err = s.backend.CopyFile(path1, path2) @@ -164,13 +171,15 @@ func (s *FileBackendTestSuite) TestMoveFile() { path1 := "tests/" + model.NewId() path2 := "tests/" + model.NewId() - s.Nil(s.backend.WriteFile(b, path1)) + written, err := s.backend.WriteFile(bytes.NewReader(b), path1) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path1) s.Nil(s.backend.MoveFile(path1, path2)) defer s.backend.RemoveFile(path2) - _, err := s.backend.ReadFile(path1) + _, err = s.backend.ReadFile(path1) s.Error(err) _, err = s.backend.ReadFile(path2) @@ -181,15 +190,26 @@ func (s *FileBackendTestSuite) TestRemoveFile() { b := []byte("test") path := "tests/" + model.NewId() - s.Nil(s.backend.WriteFile(b, path)) + written, err := s.backend.WriteFile(bytes.NewReader(b), path) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") s.Nil(s.backend.RemoveFile(path)) - _, err := s.backend.ReadFile(path) + _, err = s.backend.ReadFile(path) s.Error(err) - s.Nil(s.backend.WriteFile(b, "tests2/foo")) - s.Nil(s.backend.WriteFile(b, "tests2/bar")) - s.Nil(s.backend.WriteFile(b, "tests2/asdf")) + written, err = s.backend.WriteFile(bytes.NewReader(b), "tests2/foo") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), "tests2/bar") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), "tests2/asdf") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + s.Nil(s.backend.RemoveDirectory("tests2")) } @@ -198,9 +218,14 @@ func (s *FileBackendTestSuite) TestListDirectory() { path1 := "19700101/" + model.NewId() path2 := "19800101/" + model.NewId() - s.Nil(s.backend.WriteFile(b, path1)) + written, err := s.backend.WriteFile(bytes.NewReader(b), path1) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path1) - s.Nil(s.backend.WriteFile(b, path2)) + + written, err = s.backend.WriteFile(bytes.NewReader(b), path2) + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") defer s.backend.RemoveFile(path2) paths, err := s.backend.ListDirectory("") @@ -222,13 +247,21 @@ func (s *FileBackendTestSuite) TestListDirectory() { func (s *FileBackendTestSuite) TestRemoveDirectory() { b := []byte("test") - s.Nil(s.backend.WriteFile(b, "tests2/foo")) - s.Nil(s.backend.WriteFile(b, "tests2/bar")) - s.Nil(s.backend.WriteFile(b, "tests2/aaa")) + written, err := s.backend.WriteFile(bytes.NewReader(b), "tests2/foo") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), "tests2/bar") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") + + written, err = s.backend.WriteFile(bytes.NewReader(b), "tests2/aaa") + s.Nil(err) + s.EqualValues(len(b), written, "expected given number of bytes to have been written") s.Nil(s.backend.RemoveDirectory("tests2")) - _, err := s.backend.ReadFile("tests2/foo") + _, err = s.backend.ReadFile("tests2/foo") s.Error(err) _, err = s.backend.ReadFile("tests2/bar") s.Error(err) diff --git a/utils/mail_test.go b/utils/mail_test.go index 6bd8e7044..22a50df5f 100644 --- a/utils/mail_test.go +++ b/utils/mail_test.go @@ -150,8 +150,10 @@ func TestSendMailUsingConfig(t *testing.T) { filePath2 := fmt.Sprintf("test2/%s", fileName) fileContents1 := []byte("hello world") fileContents2 := []byte("foo bar") - assert.Nil(t, fileBackend.WriteFile(fileContents1, filePath1)) - assert.Nil(t, fileBackend.WriteFile(fileContents2, filePath2)) + _, err := fileBackend.WriteFile(bytes.NewReader(fileContents1), filePath1) + assert.Nil(t, err) + _, err := fileBackend.WriteFile(bytes.NewReader(fileContents2), filePath2) + assert.Nil(t, err) defer fileBackend.RemoveFile(filePath1) defer fileBackend.RemoveFile(filePath2) -- cgit v1.2.3-1-g7c22