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. --- 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 ++-- 5 files changed, 80 insertions(+), 38 deletions(-) (limited to 'utils') 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