From acea482456fbd4460c68410af8ec4a257b623ec0 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Thu, 5 Apr 2018 15:20:44 +0200 Subject: [PATCH] refactoring multipart from data handling from converter + tests for converter package --- .../merge-timeout-gotenberg.yml | 14 ++ app/converter/converter.go | 43 ++---- app/converter/converter_test.go | 132 ++++++++++++++++++ app/handlers.go | 9 +- orbit.yml | 2 +- 5 files changed, 158 insertions(+), 42 deletions(-) create mode 100644 _tests/configurations/merge-timeout-gotenberg.yml create mode 100644 app/converter/converter_test.go diff --git a/_tests/configurations/merge-timeout-gotenberg.yml b/_tests/configurations/merge-timeout-gotenberg.yml new file mode 100644 index 00000000..cc1323fe --- /dev/null +++ b/_tests/configurations/merge-timeout-gotenberg.yml @@ -0,0 +1,14 @@ +port: 3000 +logs: + level: "DEBUG" + format: "text" +commands: + html: + timeout: 30 + template: "xvfb-run -e /dev/stdout wkhtmltopdf {{ .FilePath }} {{ .ResultFilePath }}" + office: + timeout: 30 + template: "unoconv --format pdf --output \"{{ .ResultFilePath }}\" \"{{ .FilePath }}\"" + merge: + timeout: 0 + template: "pdftk {{ range $filePath := .FilesPaths }} {{ $filePath }} {{ end }} cat output {{ .ResultFilePath }}" diff --git a/app/converter/converter.go b/app/converter/converter.go index 0e1b3e6b..061c391d 100644 --- a/app/converter/converter.go +++ b/app/converter/converter.go @@ -3,6 +3,7 @@ package converter import ( "fmt" + "io" "net/http" "os" @@ -19,22 +20,6 @@ type Converter struct { workingDir string } -// NoFileToConvertError is raised when the converter has no file -// to convert. -type NoFileToConvertError struct{} - -func (e *NoFileToConvertError) Error() string { - return "There is no file to convert" -} - -// FilesKeyNotFoundError is raised when "files" key does not exist -// in the form data -type FilesKeyNotFoundError struct{} - -func (e *FilesKeyNotFoundError) Error() string { - return "\"files\" key was not found in the form data" -} - // NewConverter instantiates a converter by parsing a request. func NewConverter(r *http.Request, contentType ghttp.ContentType) (*Converter, error) { c := &Converter{ @@ -47,26 +32,22 @@ func NewConverter(r *http.Request, contentType ghttp.ContentType) (*Converter, e switch contentType { case ghttp.MultipartFormDataContentType: - err := r.ParseMultipartForm(32 << 20) + reader, err := r.MultipartReader() if err != nil { return nil, err } - formData := r.MultipartForm - files, ok := formData.File["files"] - if !ok { - return nil, &FilesKeyNotFoundError{} - } - - for i := range files { - file, err := files[i].Open() - if err != nil { - return nil, err + for { + part, err := reader.NextPart() + if err == io.EOF { + break } - defer file.Close() + if part.FileName() == "" { + continue + } - f, err := gfile.NewFile(c.workingDir, file) + f, err := gfile.NewFile(c.workingDir, part) if err != nil { return nil, err } @@ -83,10 +64,6 @@ func NewConverter(r *http.Request, contentType ghttp.ContentType) (*Converter, e c.files = append(c.files, f) } - if len(c.files) == 0 { - return nil, &NoFileToConvertError{} - } - return c, nil } diff --git a/app/converter/converter_test.go b/app/converter/converter_test.go new file mode 100644 index 00000000..a1f69178 --- /dev/null +++ b/app/converter/converter_test.go @@ -0,0 +1,132 @@ +package converter + +import ( + "bytes" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/gulien/gotenberg/app/config" + "github.com/gulien/gotenberg/app/converter/process" + ghttp "github.com/gulien/gotenberg/app/http" +) + +func makeRequest(filesPaths ...string) *http.Request { + if len(filesPaths) == 0 { + req := httptest.NewRequest(http.MethodPost, "/", new(bytes.Buffer)) + req.Header.Set("Content-Type", string(ghttp.OctetStreamContentType)) + return req + } + + if len(filesPaths) == 1 { + file, _ := os.Open(filesPaths[0]) + req := httptest.NewRequest(http.MethodPost, "/", file) + req.Header.Set("Content-Type", string(ghttp.OctetStreamContentType)) + return req + } + + r, w := io.Pipe() + mpw := multipart.NewWriter(w) + + go func() { + var part io.Writer + defer w.Close() + + for _, filePath := range filesPaths { + file, _ := os.Open(filePath) + defer file.Close() + + fileInfo, _ := file.Stat() + part, _ = mpw.CreateFormFile("files", fileInfo.Name()) + io.Copy(part, file) + } + + mpw.Close() + }() + + req := httptest.NewRequest(http.MethodPost, "/", r) + req.Header.Set("Content-Type", mpw.FormDataContentType()) + return req +} + +func TestNewConverter(t *testing.T) { + // case 1: uses a request with a single file. + path, _ := filepath.Abs("../../_tests/file.docx") + if _, err := NewConverter(makeRequest(path), ghttp.OctetStreamContentType); err != nil { + t.Error("Converter should have been instantiated!") + } + + // case 2: uses a request with wrong file type. + path, _ = filepath.Abs("../../_tests/configurations/gotenberg.yml") + if _, err := NewConverter(makeRequest(path), ghttp.OctetStreamContentType); err == nil { + t.Error("Converter should not have been instantiated!") + } + + // case 3: uses a request with two files. + oPath, _ := filepath.Abs("../../_tests/file.docx") + path, _ = filepath.Abs("../../_tests/file.pdf") + if _, err := NewConverter(makeRequest(oPath, path), ghttp.MultipartFormDataContentType); err != nil { + t.Error("Converter should have been instantiated!") + } + + // case 4: uses a request with one Office file and one wrong file type. + oPath, _ = filepath.Abs("../../_tests/file.docx") + path, _ = filepath.Abs("../../_tests/configurations/gotenberg.yml") + if _, err := NewConverter(makeRequest(oPath, path), ghttp.MultipartFormDataContentType); err == nil { + t.Error("Converter should not have been instantiated!") + } +} + +func TestConvert(t *testing.T) { + path, _ := filepath.Abs("../../_tests/configurations/gotenberg.yml") + appConfig, _ := config.NewAppConfig(path) + process.Load(appConfig.CommandsConfig) + + // case 1: uses a request with a single file. + path, _ = filepath.Abs("../../_tests/file.docx") + c, _ := NewConverter(makeRequest(path), ghttp.OctetStreamContentType) + if _, err := c.Convert(); err != nil { + t.Error("Converter should have been able to convert an Office document to PDF!") + } + + // case 2: uses a request with two files. + oPath, _ := filepath.Abs("../../_tests/file.docx") + path, _ = filepath.Abs("../../_tests/file.pdf") + c, _ = NewConverter(makeRequest(oPath, path), ghttp.MultipartFormDataContentType) + if _, err := c.Convert(); err != nil { + t.Error("Converter should have been able to convert two files to PDF and merge them!") + } + + // case 3: uses a request with a single file and a configuration with an unsuitable timeout for the conversion commands. + path, _ = filepath.Abs("../../_tests/configurations/timeout-gotenberg.yml") + appConfig, _ = config.NewAppConfig(path) + process.Load(appConfig.CommandsConfig) + path, _ = filepath.Abs("../../_tests/file.docx") + c, _ = NewConverter(makeRequest(path), ghttp.OctetStreamContentType) + if _, err := c.Convert(); err == nil { + t.Error("Converter should not have been able to convert an Office document to PDF!") + } + + // case 4: uses a request with two files and a configuration with an unsuitable timeout for the merge command. + path, _ = filepath.Abs("../../_tests/configurations/merge-timeout-gotenberg.yml") + appConfig, _ = config.NewAppConfig(path) + process.Load(appConfig.CommandsConfig) + oPath, _ = filepath.Abs("../../_tests/file.docx") + path, _ = filepath.Abs("../../_tests/file.pdf") + c, _ = NewConverter(makeRequest(oPath, path), ghttp.MultipartFormDataContentType) + if _, err := c.Convert(); err == nil { + t.Error("Converter should not have been able to merge PDF!") + } +} + +func TestClear(t *testing.T) { + path, _ := filepath.Abs("../../_tests/file.docx") + c, _ := NewConverter(makeRequest(path), ghttp.OctetStreamContentType) + if err := c.Clear(); err != nil { + t.Error("Converter should have been able to clear itself!") + } +} diff --git a/app/handlers.go b/app/handlers.go index 17d56eae..ed8ce92d 100644 --- a/app/handlers.go +++ b/app/handlers.go @@ -70,14 +70,7 @@ func convertHandler(next http.Handler) http.Handler { c, err := converter.NewConverter(r, ct) if err != nil { - if noFileToConvertError, ok := err.(*converter.NoFileToConvertError); ok { - http.Error(w, noFileToConvertError.Error(), http.StatusBadRequest) - } else if filesKeyNotFoundError, ok := err.(*converter.FilesKeyNotFoundError); ok { - http.Error(w, filesKeyNotFoundError.Error(), http.StatusBadRequest) - } else { - http.Error(w, err.Error(), http.StatusInternalServerError) - } - + http.Error(w, err.Error(), http.StatusInternalServerError) logger.Error(err) return } diff --git a/orbit.yml b/orbit.yml index bcdb5b22..3325b4cf 100644 --- a/orbit.yml +++ b/orbit.yml @@ -28,4 +28,4 @@ tasks: - use: up short: Starts the {{ .Orbit.Version }} version of the Gotenberg image run: - - docker run --rm -p 3000:3000/tcp --name=gotenberg gotenberg:{{ .Orbit.Version }} \ No newline at end of file + - docker run --rm -p 3000:3000/tcp gotenberg:{{ .Orbit.Version }} \ No newline at end of file