From b2ff09b35aa00c43d70f59d7e78d97b1f570345a Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Mon, 8 Apr 2024 16:13:31 +0300 Subject: [PATCH 1/7] Add `error` fieldname to multipart response --- http/filestream/errors.go | 17 ++++++++++++++++ http/filestream/filestream.go | 32 ++++++++++++++++++++++++------ http/filestream/filestream_test.go | 29 +++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 http/filestream/errors.go diff --git a/http/filestream/errors.go b/http/filestream/errors.go new file mode 100644 index 0000000..2b2b22c --- /dev/null +++ b/http/filestream/errors.go @@ -0,0 +1,17 @@ +package filestream + +type MultipartError struct { + FileName string `json:"file_name"` + Message string `json:"message"` +} + +func (e MultipartError) Error() string { + return e.Message +} + +func NewMultipartError(fileName, message string) *MultipartError { + return &MultipartError{ + FileName: fileName, + Message: message, + } +} diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index f5f6eb5..ce304aa 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -1,6 +1,8 @@ package filestream import ( + "bytes" + "encoding/json" "errors" "fmt" "io" @@ -11,7 +13,8 @@ import ( ) const ( - FileType = "file" + FileType = "file" + ErrorType = "error" ) // The expected type of function that should be provided to the ReadFilesFromStream func, that returns the writer that should handle each file @@ -60,21 +63,22 @@ type FileInfo struct { } func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) (err error) { + // Close finishes the multipart message and writes the trailing + // boundary end line to the output. + defer ioutils.Close(multipartWriter, &err) for _, file := range filesList { if err = writeFile(multipartWriter, file); err != nil { - return + return writeErrPart(multipartWriter, file, err) } } - // Close finishes the multipart message and writes the trailing - // boundary end line to the output. - return multipartWriter.Close() + return nil } func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { fileReader, err := os.Open(file.Path) if err != nil { - return fmt.Errorf("failed opening %q: %w", file, err) + return fmt.Errorf("failed opening %q: %w", file.Name, err) } defer ioutils.Close(fileReader, &err) fileWriter, err := multipartWriter.CreateFormFile(FileType, file.Name) @@ -84,3 +88,19 @@ func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { _, err = io.Copy(fileWriter, fileReader) return err } + +func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) error { + fileWriter, err := multipartWriter.CreateFormField(ErrorType) + if err != nil { + return fmt.Errorf("failed to CreateFormField: %w", err) + } + + multipartErr := NewMultipartError(file.Name, writeFileErr.Error()) + multipartErrJSON, err := json.Marshal(multipartErr) + if err != nil { + return fmt.Errorf("failed to marshal multipart error: %w", err) + } + + _, err = io.Copy(fileWriter, bytes.NewReader(multipartErrJSON)) + return err +} diff --git a/http/filestream/filestream_test.go b/http/filestream/filestream_test.go index a5203b4..b0e8627 100644 --- a/http/filestream/filestream_test.go +++ b/http/filestream/filestream_test.go @@ -2,6 +2,7 @@ package filestream import ( "bytes" + "encoding/json" "io" "mime/multipart" "os" @@ -46,6 +47,34 @@ func TestWriteFilesToStreamAndReadFilesFromStream(t *testing.T) { assert.Equal(t, file2Content, content) } +func TestWriteFilesToStreamWithError(t *testing.T) { + // Create a temporary directory for the test + sourceDir := t.TempDir() + + nonExistentFileName := "nonexistent.txt" + // Create a FileInfo with a non-existent file + file := &FileInfo{Name: nonExistentFileName, Path: filepath.Join(sourceDir, nonExistentFileName)} + + // Create a buffer and a multipart writer + body := &bytes.Buffer{} + multipartWriter := multipart.NewWriter(body) + + // Call WriteFilesToStream and expect an error + err := WriteFilesToStream(multipartWriter, []*FileInfo{file}) + assert.NoError(t, err) + + multipartReader := multipart.NewReader(body, multipartWriter.Boundary()) + form, err := multipartReader.ReadForm(1024) + assert.NoError(t, err) + + assert.Len(t, form.Value[ErrorType], 1) + var multipartErr MultipartError + assert.NoError(t, json.Unmarshal([]byte(form.Value[ErrorType][0]), &multipartErr)) + + assert.Equal(t, nonExistentFileName, multipartErr.FileName) + assert.NotEmpty(t, multipartErr.Error()) +} + func simpleFileWriter(fileName string) (fileWriter []io.WriteCloser, err error) { writer, err := os.Create(filepath.Join(targetDir, fileName)) if err != nil { From 09d3b881da8e8b43426e3106e8fd40b3dbeaf0e4 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 09:31:09 +0300 Subject: [PATCH 2/7] CR --- http/filestream/filestream.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index ce304aa..40f5402 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -62,17 +62,17 @@ type FileInfo struct { Path string } -func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) (err error) { - // Close finishes the multipart message and writes the trailing - // boundary end line to the output. - defer ioutils.Close(multipartWriter, &err) +func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) error { for _, file := range filesList { - if err = writeFile(multipartWriter, file); err != nil { + if err := writeFile(multipartWriter, file); err != nil { return writeErrPart(multipartWriter, file, err) } } - return nil + // Close finishes the multipart message and writes the trailing + // boundary end line to the output. + // We don't use defer for this because the multipart.Writer's Close() method writes regardless of whether there was an error or if writing hadn't started at all + return multipartWriter.Close() } func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { From 3bf997487afc27c03f7c2f74b04e3d22b3023a19 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 10:22:39 +0300 Subject: [PATCH 3/7] CR --- http/filestream/filestream.go | 39 +++++++++++++++++++----------- http/filestream/filestream_test.go | 7 +++--- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index 40f5402..343a604 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -62,45 +62,56 @@ type FileInfo struct { Path string } -func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) error { +func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) (err error) { + var isContentWritten bool + defer func() { + // The multipartWriter.Close() function automatically writes the closing boundary to the underlying writer, + // regardless of whether any content was written to it. Therefore, if no content was written + // (i.e., no parts were created using the multipartWriter), there is no need to explicitly close the + // multipartWriter. The closing boundary will be correctly handled by calling multipartWriter.Close() + // when it goes out of scope or when explicitly called, ensuring the proper termination of the multipart request. + if isContentWritten { + err = errors.Join(err, multipartWriter.Close()) + } + }() for _, file := range filesList { - if err := writeFile(multipartWriter, file); err != nil { - return writeErrPart(multipartWriter, file, err) + if err = writeFile(multipartWriter, file); err != nil { + isContentWritten, err = writeErrPart(multipartWriter, file, err) + return err } + isContentWritten = true } - // Close finishes the multipart message and writes the trailing - // boundary end line to the output. - // We don't use defer for this because the multipart.Writer's Close() method writes regardless of whether there was an error or if writing hadn't started at all - return multipartWriter.Close() + return nil } func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { fileReader, err := os.Open(file.Path) if err != nil { - return fmt.Errorf("failed opening %q: %w", file.Name, err) + return fmt.Errorf("failed opening file %q: %w", file.Name, err) } defer ioutils.Close(fileReader, &err) fileWriter, err := multipartWriter.CreateFormFile(FileType, file.Name) if err != nil { - return fmt.Errorf("failed to CreateFormFile: %w", err) + return fmt.Errorf("failed to create form file for %q: %w", file.Name, err) } _, err = io.Copy(fileWriter, fileReader) return err } -func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) error { +func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) (bool, error) { + var isPartWritten bool fileWriter, err := multipartWriter.CreateFormField(ErrorType) if err != nil { - return fmt.Errorf("failed to CreateFormField: %w", err) + return isPartWritten, fmt.Errorf("failed to create form field: %w", err) } - + isPartWritten = true multipartErr := NewMultipartError(file.Name, writeFileErr.Error()) multipartErrJSON, err := json.Marshal(multipartErr) if err != nil { - return fmt.Errorf("failed to marshal multipart error: %w", err) + return isPartWritten, fmt.Errorf("failed to marshal multipart error for file %q: %w", file.Name, err) } _, err = io.Copy(fileWriter, bytes.NewReader(multipartErrJSON)) - return err + return isPartWritten, err } diff --git a/http/filestream/filestream_test.go b/http/filestream/filestream_test.go index b0e8627..1947a53 100644 --- a/http/filestream/filestream_test.go +++ b/http/filestream/filestream_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var targetDir string @@ -61,11 +62,11 @@ func TestWriteFilesToStreamWithError(t *testing.T) { // Call WriteFilesToStream and expect an error err := WriteFilesToStream(multipartWriter, []*FileInfo{file}) - assert.NoError(t, err) + require.NoError(t, err) multipartReader := multipart.NewReader(body, multipartWriter.Boundary()) - form, err := multipartReader.ReadForm(1024) - assert.NoError(t, err) + form, err := multipartReader.ReadForm(10 * 1024) + require.NoError(t, err) assert.Len(t, form.Value[ErrorType], 1) var multipartErr MultipartError From 7dac7434c61b83a7ac01b4426cfe7c9d294c1bc0 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 14:46:53 +0300 Subject: [PATCH 4/7] CR --- http/filestream/filestream.go | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index 343a604..8afc881 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -63,23 +63,11 @@ type FileInfo struct { } func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) (err error) { - var isContentWritten bool - defer func() { - // The multipartWriter.Close() function automatically writes the closing boundary to the underlying writer, - // regardless of whether any content was written to it. Therefore, if no content was written - // (i.e., no parts were created using the multipartWriter), there is no need to explicitly close the - // multipartWriter. The closing boundary will be correctly handled by calling multipartWriter.Close() - // when it goes out of scope or when explicitly called, ensuring the proper termination of the multipart request. - if isContentWritten { - err = errors.Join(err, multipartWriter.Close()) - } - }() + defer ioutils.Close(multipartWriter, &err) for _, file := range filesList { if err = writeFile(multipartWriter, file); err != nil { - isContentWritten, err = writeErrPart(multipartWriter, file, err) - return err + return writeErrPart(multipartWriter, file, err) } - isContentWritten = true } return nil @@ -99,19 +87,18 @@ func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { return err } -func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) (bool, error) { - var isPartWritten bool +func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) error { fileWriter, err := multipartWriter.CreateFormField(ErrorType) if err != nil { - return isPartWritten, fmt.Errorf("failed to create form field: %w", err) + return fmt.Errorf("failed to create form field: %w", err) } - isPartWritten = true + multipartErr := NewMultipartError(file.Name, writeFileErr.Error()) multipartErrJSON, err := json.Marshal(multipartErr) if err != nil { - return isPartWritten, fmt.Errorf("failed to marshal multipart error for file %q: %w", file.Name, err) + return fmt.Errorf("failed to marshal multipart error for file %q: %w", file.Name, err) } _, err = io.Copy(fileWriter, bytes.NewReader(multipartErrJSON)) - return isPartWritten, err + return err } From 72c8655934fa6f065a706b4db49542e7a6b369b1 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 14:49:59 +0300 Subject: [PATCH 5/7] CR --- http/filestream/filestream.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index 8afc881..f063998 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -63,6 +63,8 @@ type FileInfo struct { } func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo) (err error) { + // Close finishes the multipart message and writes the trailing + // boundary end line to the output, thereby marking the EOF. defer ioutils.Close(multipartWriter, &err) for _, file := range filesList { if err = writeFile(multipartWriter, file); err != nil { From 3a6db8cf3dd0155834d722f651e66da9b1124609 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 14:52:09 +0300 Subject: [PATCH 6/7] CR --- http/filestream/filestream.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index f063998..070cf97 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -68,7 +68,7 @@ func WriteFilesToStream(multipartWriter *multipart.Writer, filesList []*FileInfo defer ioutils.Close(multipartWriter, &err) for _, file := range filesList { if err = writeFile(multipartWriter, file); err != nil { - return writeErrPart(multipartWriter, file, err) + return writeErr(multipartWriter, file, err) } } @@ -89,7 +89,7 @@ func writeFile(multipartWriter *multipart.Writer, file *FileInfo) (err error) { return err } -func writeErrPart(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) error { +func writeErr(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr error) error { fileWriter, err := multipartWriter.CreateFormField(ErrorType) if err != nil { return fmt.Errorf("failed to create form field: %w", err) From e4de27e3db8cdd9d1347144dd8ed7de6c16ad1d1 Mon Sep 17 00:00:00 2001 From: Omer Zidkoni Date: Tue, 9 Apr 2024 15:41:29 +0300 Subject: [PATCH 7/7] CR --- http/filestream/errors.go | 17 ----------------- http/filestream/filestream.go | 7 ++++++- http/filestream/filestream_test.go | 7 ++----- 3 files changed, 8 insertions(+), 23 deletions(-) delete mode 100644 http/filestream/errors.go diff --git a/http/filestream/errors.go b/http/filestream/errors.go deleted file mode 100644 index 2b2b22c..0000000 --- a/http/filestream/errors.go +++ /dev/null @@ -1,17 +0,0 @@ -package filestream - -type MultipartError struct { - FileName string `json:"file_name"` - Message string `json:"message"` -} - -func (e MultipartError) Error() string { - return e.Message -} - -func NewMultipartError(fileName, message string) *MultipartError { - return &MultipartError{ - FileName: fileName, - Message: message, - } -} diff --git a/http/filestream/filestream.go b/http/filestream/filestream.go index 070cf97..a6b2ca2 100644 --- a/http/filestream/filestream.go +++ b/http/filestream/filestream.go @@ -17,6 +17,11 @@ const ( ErrorType = "error" ) +type MultipartError struct { + FileName string `json:"file_name"` + ErrMessage string `json:"error_message"` +} + // The expected type of function that should be provided to the ReadFilesFromStream func, that returns the writer that should handle each file type FileWriterFunc func(fileName string) (writers []io.WriteCloser, err error) @@ -95,7 +100,7 @@ func writeErr(multipartWriter *multipart.Writer, file *FileInfo, writeFileErr er return fmt.Errorf("failed to create form field: %w", err) } - multipartErr := NewMultipartError(file.Name, writeFileErr.Error()) + multipartErr := MultipartError{FileName: file.Name, ErrMessage: writeFileErr.Error()} multipartErrJSON, err := json.Marshal(multipartErr) if err != nil { return fmt.Errorf("failed to marshal multipart error for file %q: %w", file.Name, err) diff --git a/http/filestream/filestream_test.go b/http/filestream/filestream_test.go index 1947a53..8d9e520 100644 --- a/http/filestream/filestream_test.go +++ b/http/filestream/filestream_test.go @@ -49,12 +49,9 @@ func TestWriteFilesToStreamAndReadFilesFromStream(t *testing.T) { } func TestWriteFilesToStreamWithError(t *testing.T) { - // Create a temporary directory for the test - sourceDir := t.TempDir() - nonExistentFileName := "nonexistent.txt" // Create a FileInfo with a non-existent file - file := &FileInfo{Name: nonExistentFileName, Path: filepath.Join(sourceDir, nonExistentFileName)} + file := &FileInfo{Name: nonExistentFileName, Path: nonExistentFileName} // Create a buffer and a multipart writer body := &bytes.Buffer{} @@ -73,7 +70,7 @@ func TestWriteFilesToStreamWithError(t *testing.T) { assert.NoError(t, json.Unmarshal([]byte(form.Value[ErrorType][0]), &multipartErr)) assert.Equal(t, nonExistentFileName, multipartErr.FileName) - assert.NotEmpty(t, multipartErr.Error()) + assert.NotEmpty(t, multipartErr.ErrMessage) } func simpleFileWriter(fileName string) (fileWriter []io.WriteCloser, err error) {