diff --git a/pkg/gotenberg/fs.go b/pkg/gotenberg/fs.go index e73e94f6..8c2d0a98 100644 --- a/pkg/gotenberg/fs.go +++ b/pkg/gotenberg/fs.go @@ -51,3 +51,11 @@ func (fs *FileSystem) MkdirAll() (string, error) { return path, nil } + +// PathRename defines the method signature for renaming files. Implement this +// interface if you don't want to rely on [os.Rename], notably for testing +// purpose. +type PathRename interface { + // Rename uses the same signature as [os.Rename]. + Rename(oldpath, newpath string) error +} diff --git a/pkg/gotenberg/mocks.go b/pkg/gotenberg/mocks.go index 4e8fad02..7188e05f 100644 --- a/pkg/gotenberg/mocks.go +++ b/pkg/gotenberg/mocks.go @@ -127,6 +127,15 @@ func (provider *MetricsProviderMock) Metrics() ([]Metric, error) { return provider.MetricsMock() } +// PathRenameMock is a mock for the [PathRename] interface. +type PathRenameMock struct { + RenameMock func(oldpath, newpath string) error +} + +func (rename *PathRenameMock) Rename(oldpath, newpath string) error { + return rename.RenameMock(oldpath, newpath) +} + // Interface guards. var ( _ Module = (*ModuleMock)(nil) diff --git a/pkg/gotenberg/mocks_test.go b/pkg/gotenberg/mocks_test.go index dba04318..9eb52bed 100644 --- a/pkg/gotenberg/mocks_test.go +++ b/pkg/gotenberg/mocks_test.go @@ -188,3 +188,16 @@ func TestMetricsProviderMock(t *testing.T) { t.Errorf("expected no error from MetricsProviderMock.Metrics, but got: %v", err) } } + +func TestPathRenameMock(t *testing.T) { + mock := &PathRenameMock{ + RenameMock: func(oldpath, newpath string) error { + return nil + }, + } + + err := mock.Rename("", "") + if err != nil { + t.Errorf("expected no error from PathRenameMock.Rename, but got: %v", err) + } +} diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index e63c775a..21fa6699 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -34,18 +34,24 @@ var ( // Context is the request context for a "multipart/form-data" requests. type Context struct { - dirPath string - values map[string][]string - files map[string]string - + dirPath string + values map[string][]string + files map[string]string outputPaths []string + cancelled bool - cancelled bool - logger *zap.Logger - echoCtx echo.Context + logger *zap.Logger + echoCtx echo.Context + pathRename gotenberg.PathRename context.Context } +type osPathRename struct{} + +func (o *osPathRename) Rename(oldpath, newpath string) error { + return os.Rename(oldpath, newpath) +} + // newContext returns a [Context] by parsing a "multipart/form-data" request. func newContext(echoCtx echo.Context, logger *zap.Logger, fs *gotenberg.FileSystem, timeout time.Duration) (*Context, context.CancelFunc, error) { processCtx, processCancel := context.WithTimeout(context.Background(), timeout) @@ -55,6 +61,7 @@ func newContext(echoCtx echo.Context, logger *zap.Logger, fs *gotenberg.FileSyst cancelled: false, logger: logger, echoCtx: echoCtx, + pathRename: new(osPathRename), Context: processCtx, } @@ -191,14 +198,19 @@ func (ctx *Context) FormData() *FormData { } // GeneratePath generates a path within the context's working directory. -// It either generates a new UUID-based filename or uses the provided filename. -// It does not create a file. -func (ctx *Context) GeneratePath(filename, extension string) string { - if filename == "" { - // Generate a new UUID-based filename - filename = uuid.New().String() +// It generates a new UUID-based filename. It does not create a file. +func (ctx *Context) GeneratePath(extension string) string { + return fmt.Sprintf("%s/%s%s", ctx.dirPath, uuid.New().String(), extension) +} + +// Rename is just a wrapper around [os.Rename], as we need to mock this +// behavior in our tests. +func (ctx *Context) Rename(oldpath, newpath string) error { + err := ctx.pathRename.Rename(oldpath, newpath) + if err != nil { + return fmt.Errorf("rename path: %w", err) } - return fmt.Sprintf("%s/%s%s", ctx.dirPath, filename, extension) + return nil } // AddOutputPaths adds the given paths. Those paths will be used later to build @@ -250,7 +262,7 @@ func (ctx *Context) BuildOutputFile() (string, error) { ImplicitTopLevelFolder: false, } - archivePath := ctx.GeneratePath("", ".zip") + archivePath := ctx.GeneratePath(".zip") err := z.Archive(ctx.outputPaths, archivePath) if err != nil { @@ -273,3 +285,8 @@ func (ctx *Context) OutputFilename(outputPath string) string { return fmt.Sprintf("%s%s", filename, filepath.Ext(outputPath)) } + +// Interface guard. +var ( + _ gotenberg.PathRename = (*osPathRename)(nil) +) diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index 0b59c5a0..f50deb82 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -3,21 +3,77 @@ package api import ( "bytes" "errors" + "fmt" + "io" "mime/multipart" "net/http" "net/http/httptest" "os" + "path/filepath" "reflect" "strings" "testing" "time" + "github.com/google/uuid" "github.com/labstack/echo/v4" "go.uber.org/zap" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" ) +func TestOsPathRename_Rename(t *testing.T) { + dirPath, err := gotenberg.NewFileSystem().MkdirAll() + if err != nil { + t.Fatalf("create working directory: %v", err) + } + + path := "/tests/test/testdata/api/sample1.txt" + copyPath := filepath.Join(dirPath, fmt.Sprintf("%s.txt", uuid.NewString())) + + in, err := os.Open(path) + if err != nil { + t.Fatalf("open file: %v", err) + } + + defer func() { + err := in.Close() + if err != nil { + t.Fatalf("close file: %v", err) + } + }() + + out, err := os.Create(copyPath) + if err != nil { + t.Fatalf("create new file: %v", err) + } + + defer func() { + err := out.Close() + if err != nil { + t.Fatalf("close new file: %v", err) + } + }() + + _, err = io.Copy(out, in) + if err != nil { + t.Fatalf("copy file to new file: %v", err) + } + + rename := new(osPathRename) + newPath := filepath.Join(dirPath, fmt.Sprintf("%s.txt", uuid.NewString())) + + err = rename.Rename(copyPath, newPath) + if err != nil { + t.Errorf("expected no error but got: %v", err) + } + + err = os.RemoveAll(dirPath) + if err != nil { + t.Fatalf("remove working directory: %v", err) + } +} + func TestNewContext(t *testing.T) { for _, tc := range []struct { scenario string @@ -190,14 +246,44 @@ func TestContext_GeneratePath(t *testing.T) { dirPath: "/foo", } - path := ctx.GeneratePath("", ".pdf") + path := ctx.GeneratePath(".pdf") if !strings.HasPrefix(path, ctx.dirPath) { t.Errorf("expected '%s' to start with '%s'", path, ctx.dirPath) } +} - path = ctx.GeneratePath("foo.txt", ".pdf") - if !strings.Contains(path, "foo.txt.pdf") { - t.Errorf("expected '%s' to start with '%s'", path, ctx.dirPath) +func TestContext_Rename(t *testing.T) { + for _, tc := range []struct { + scenario string + ctx *Context + expectError bool + }{ + { + scenario: "failure", + ctx: &Context{pathRename: &gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return errors.New("cannot rename") + }}}, + expectError: true, + }, + { + scenario: "success", + ctx: &Context{pathRename: &gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return nil + }}}, + expectError: false, + }, + } { + t.Run(tc.scenario, func(t *testing.T) { + err := tc.ctx.Rename("", "") + + if tc.expectError && err == nil { + t.Fatal("expected error but got none", err) + } + + if !tc.expectError && err != nil { + t.Fatalf("expected no error but got: %v", err) + } + }) } } diff --git a/pkg/modules/api/mocks.go b/pkg/modules/api/mocks.go index 63a65d73..667cd14e 100644 --- a/pkg/modules/api/mocks.go +++ b/pkg/modules/api/mocks.go @@ -4,6 +4,8 @@ import ( "github.com/alexliesenfeld/health" "github.com/labstack/echo/v4" "go.uber.org/zap" + + "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" ) // ContextMock is a helper for tests. @@ -76,7 +78,7 @@ func (ctx *ContextMock) SetLogger(logger *zap.Logger) { ctx.logger = logger } -// SetEchoContext sets the echo.Context. +// SetEchoContext sets the [echo.Context]. // // ctx := &api.ContextMock{Context: &api.Context{}} // ctx.setEchoContext(c) @@ -84,6 +86,14 @@ func (ctx *ContextMock) SetEchoContext(c echo.Context) { ctx.Context.echoCtx = c } +// SetPathRename sets the [gotenberg.PathRename]. +// +// ctx := &api.ContextMock{Context: &api.Context{}} +// ctx.setPathRename(rename) +func (ctx *ContextMock) SetPathRename(rename gotenberg.PathRename) { + ctx.Context.pathRename = rename +} + // RouterMock is a mock for the [Router] interface. type RouterMock struct { RoutesMock func() ([]Route, error) diff --git a/pkg/modules/api/mocks_test.go b/pkg/modules/api/mocks_test.go index b0e31771..5910726c 100644 --- a/pkg/modules/api/mocks_test.go +++ b/pkg/modules/api/mocks_test.go @@ -117,6 +117,19 @@ func TestContextMock_SetEchoContext(t *testing.T) { } } +func TestContextMock_SetPathRename(t *testing.T) { + mock := ContextMock{&Context{}} + + expect := new(osPathRename) + mock.SetPathRename(expect) + + actual := mock.pathRename + + if actual != expect { + t.Errorf("expected %v but got %v", expect, actual) + } +} + func TestRouterMock(t *testing.T) { mock := &RouterMock{ RoutesMock: func() ([]Route, error) { diff --git a/pkg/modules/chromium/routes.go b/pkg/modules/chromium/routes.go index f490963c..94667f50 100644 --- a/pkg/modules/chromium/routes.go +++ b/pkg/modules/chromium/routes.go @@ -496,7 +496,7 @@ func markdownToHtml(ctx *api.Context, inputPath string, markdownPaths []string) ) } - inputPath = ctx.GeneratePath("", ".html") + inputPath = ctx.GeneratePath(".html") err = os.WriteFile(inputPath, buffer.Bytes(), 0o600) if err != nil { @@ -507,7 +507,7 @@ func markdownToHtml(ctx *api.Context, inputPath string, markdownPaths []string) } func convertUrl(ctx *api.Context, chromium Api, engine gotenberg.PdfEngine, url string, pdfFormats gotenberg.PdfFormats, options PdfOptions) error { - outputPath := ctx.GeneratePath("", ".pdf") + outputPath := ctx.GeneratePath(".pdf") err := chromium.Pdf(ctx, ctx.Log(), url, outputPath, options) err = handleChromiumError(err, options.Options) @@ -551,7 +551,7 @@ func convertUrl(ctx *api.Context, chromium Api, engine gotenberg.PdfEngine, url zeroValued := gotenberg.PdfFormats{} if pdfFormats != zeroValued { convertInputPath := outputPath - convertOutputPath := ctx.GeneratePath("", ".pdf") + convertOutputPath := ctx.GeneratePath(".pdf") err = engine.Convert(ctx, ctx.Log(), pdfFormats, convertInputPath, convertOutputPath) if err != nil { @@ -572,7 +572,7 @@ func convertUrl(ctx *api.Context, chromium Api, engine gotenberg.PdfEngine, url func screenshotUrl(ctx *api.Context, chromium Api, url string, options ScreenshotOptions) error { ext := fmt.Sprintf(".%s", options.Format) - outputPath := ctx.GeneratePath("", ext) + outputPath := ctx.GeneratePath(ext) err := chromium.Screenshot(ctx, ctx.Log(), url, outputPath, options) err = handleChromiumError(err, options.Options) diff --git a/pkg/modules/libreoffice/routes.go b/pkg/modules/libreoffice/routes.go index 485eb623..f3b656cd 100644 --- a/pkg/modules/libreoffice/routes.go +++ b/pkg/modules/libreoffice/routes.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "net/http" - "path/filepath" "github.com/labstack/echo/v4" @@ -55,8 +54,7 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap // Alright, let's convert each document to PDF. outputPaths := make([]string, len(inputPaths)) for i, inputPath := range inputPaths { - // document.docx -> document.docx.pdf. - outputPaths[i] = ctx.GeneratePath(filepath.Base(inputPath), ".pdf") + outputPaths[i] = ctx.GeneratePath(".pdf") options := libreofficeapi.Options{ Landscape: landscape, PageRanges: nativePageRanges, @@ -91,9 +89,8 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap // So far so good, let's check if we have to merge the PDFs. Quick // win: if there is only one PDF, skip this step. - if len(outputPaths) > 1 && merge { - outputPath := ctx.GeneratePath("", ".pdf") + outputPath := ctx.GeneratePath(".pdf") err = engine.Merge(ctx, ctx.Log(), outputPaths, outputPath) if err != nil { @@ -105,7 +102,7 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap zeroValued := gotenberg.PdfFormats{} if !nativePdfFormats && pdfFormats != zeroValued { convertInputPath := outputPath - convertOutputPath := ctx.GeneratePath("", ".pdf") + convertOutputPath := ctx.GeneratePath(".pdf") err = engine.Convert(ctx, ctx.Log(), pdfFormats, convertInputPath, convertOutputPath) if err != nil { @@ -117,8 +114,7 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap } // Last but not least, add the output path to the context so that - // the Uno is able to send it as a response to the client. - + // the API is able to send it as a response to the client. err = ctx.AddOutputPaths(outputPath) if err != nil { return fmt.Errorf("add output path: %w", err) @@ -135,23 +131,34 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap for i, outputPath := range outputPaths { convertInputPath := outputPath - // document.docx -> document.docx.pdf. - convertOutputPaths[i] = ctx.GeneratePath(filepath.Base(inputPaths[i]), ".pdf") + convertOutputPaths[i] = ctx.GeneratePath(".pdf") err = engine.Convert(ctx, ctx.Log(), pdfFormats, convertInputPath, convertOutputPaths[i]) if err != nil { return fmt.Errorf("convert PDF: %w", err) } - } // Important: the output paths are now the converted files. outputPaths = convertOutputPaths } - // Last but not least, add the output paths to the context so that - // the Uno is able to send them as a response to the client. + if len(outputPaths) > 1 { + // If .zip archive, document.docx -> document.docx.pdf. + for i, inputPath := range inputPaths { + outputPath := fmt.Sprintf("%s.pdf", inputPath) + err = ctx.Rename(outputPaths[i], outputPath) + if err != nil { + return fmt.Errorf("rename output path: %w", err) + } + + outputPaths[i] = outputPath + } + } + + // Last but not least, add the output paths to the context so that + // the API is able to send them as a response to the client. err = ctx.AddOutputPaths(outputPaths...) if err != nil { return fmt.Errorf("add output paths: %w", err) diff --git a/pkg/modules/libreoffice/routes_test.go b/pkg/modules/libreoffice/routes_test.go index 734da2ff..67ab078f 100644 --- a/pkg/modules/libreoffice/routes_test.go +++ b/pkg/modules/libreoffice/routes_test.go @@ -185,7 +185,32 @@ func TestConvertRoute(t *testing.T) { expectError: false, expectHttpError: false, expectOutputPathsCount: 1, - expectOutputPaths: []string{"/document.docx.pdf"}, + }, + { + scenario: "cannot rename many files", + ctx: func() *api.ContextMock { + ctx := &api.ContextMock{Context: new(api.Context)} + ctx.SetFiles(map[string]string{ + "document.docx": "/document.docx", + "document2.docx": "/document2.docx", + "document2.doc": "/document2.doc", + }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return errors.New("cannot rename") + }}) + return ctx + }(), + libreOffice: &libreofficeapi.ApiMock{ + PdfMock: func(ctx context.Context, logger *zap.Logger, inputPath, outputPath string, options libreofficeapi.Options) error { + return nil + }, + ExtensionsMock: func() []string { + return []string{".docx", ".doc"} + }, + }, + expectError: true, + expectHttpError: false, + expectOutputPathsCount: 0, }, { scenario: "success (many files)", @@ -196,6 +221,9 @@ func TestConvertRoute(t *testing.T) { "document2.docx": "/document2.docx", "document2.doc": "/document2.doc", }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return nil + }}) return ctx }(), libreOffice: &libreofficeapi.ApiMock{ @@ -230,6 +258,9 @@ func TestConvertRoute(t *testing.T) { "false", }, }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return nil + }}) return ctx }(), libreOffice: &libreofficeapi.ApiMock{ @@ -266,6 +297,9 @@ func TestConvertRoute(t *testing.T) { "true", }, }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return nil + }}) return ctx }(), libreOffice: &libreofficeapi.ApiMock{ diff --git a/pkg/modules/pdfengines/routes.go b/pkg/modules/pdfengines/routes.go index 59dddca3..041e040a 100644 --- a/pkg/modules/pdfengines/routes.go +++ b/pkg/modules/pdfengines/routes.go @@ -4,8 +4,6 @@ import ( "errors" "fmt" "net/http" - "path/filepath" - "strings" "github.com/labstack/echo/v4" @@ -44,8 +42,7 @@ func mergeRoute(engine gotenberg.PdfEngine) api.Route { } // Alright, let's merge the PDFs. - - outputPath := ctx.GeneratePath("", ".pdf") + outputPath := ctx.GeneratePath(".pdf") err = engine.Merge(ctx, ctx.Log(), inputPaths, outputPath) if err != nil { @@ -58,7 +55,7 @@ func mergeRoute(engine gotenberg.PdfEngine) api.Route { zeroValued := gotenberg.PdfFormats{} if pdfFormats != zeroValued { convertInputPath := outputPath - convertOutputPath := ctx.GeneratePath("", ".pdf") + convertOutputPath := ctx.GeneratePath(".pdf") err = engine.Convert(ctx, ctx.Log(), pdfFormats, convertInputPath, convertOutputPath) if err != nil { @@ -71,7 +68,6 @@ func mergeRoute(engine gotenberg.PdfEngine) api.Route { // Last but not least, add the output path to the context so that // the API is able to send it as a response to the client. - err = ctx.AddOutputPaths(outputPath) if err != nil { return fmt.Errorf("add output path: %w", err) @@ -124,26 +120,29 @@ func convertRoute(engine gotenberg.PdfEngine) api.Route { ) } - // Alright, let's convert the PDFs.s + // Alright, let's convert the PDFs. outputPaths := make([]string, len(inputPaths)) - for i, inputPath := range inputPaths { - if len(outputPaths) > 1 { - // If .zip archive, keep the original filenames. - outputPaths[i] = ctx.GeneratePath(strings.TrimSuffix(filepath.Base(inputPath), filepath.Ext(inputPath)), ".pdf") - } else { - outputPaths[i] = ctx.GeneratePath("", ".pdf") - } + outputPaths[i] = ctx.GeneratePath(".pdf") err = engine.Convert(ctx, ctx.Log(), pdfFormats, inputPath, outputPaths[i]) if err != nil { return fmt.Errorf("convert PDF: %w", err) } + + if len(outputPaths) > 1 { + // If .zip archive, keep the original filename. + err = ctx.Rename(outputPaths[i], inputPath) + if err != nil { + return fmt.Errorf("rename output path: %w", err) + } + + outputPaths[i] = inputPath + } } // Last but not least, add the output paths to the context so that // the API is able to send them as a response to the client. - err = ctx.AddOutputPaths(outputPaths...) if err != nil { return fmt.Errorf("add output paths: %w", err) diff --git a/pkg/modules/pdfengines/routes_test.go b/pkg/modules/pdfengines/routes_test.go index e8eb915c..ae843755 100644 --- a/pkg/modules/pdfengines/routes_test.go +++ b/pkg/modules/pdfengines/routes_test.go @@ -294,6 +294,36 @@ func TestConvertHandler(t *testing.T) { expectHttpError: false, expectOutputPathsCount: 1, }, + { + scenario: "cannot rename many files", + ctx: func() *api.ContextMock { + ctx := &api.ContextMock{Context: new(api.Context)} + ctx.SetFiles(map[string]string{ + "file.pdf": "/file.pdf", + "file2.pdf": "/file2.pdf", + }) + ctx.SetValues(map[string][]string{ + "pdfa": { + gotenberg.PdfA1b, + }, + "pdfua": { + "true", + }, + }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return errors.New("cannot rename") + }}) + return ctx + }(), + engine: &gotenberg.PdfEngineMock{ + ConvertMock: func(ctx context.Context, logger *zap.Logger, formats gotenberg.PdfFormats, inputPath, outputPath string) error { + return nil + }, + }, + expectError: true, + expectHttpError: false, + expectOutputPathsCount: 0, + }, { scenario: "success with PDF/A & PDF/UA form fields (many files)", ctx: func() *api.ContextMock { @@ -310,6 +340,9 @@ func TestConvertHandler(t *testing.T) { "true", }, }) + ctx.SetPathRename(&gotenberg.PathRenameMock{RenameMock: func(oldpath, newpath string) error { + return nil + }}) return ctx }(), engine: &gotenberg.PdfEngineMock{