From 089f161d1e1ceff2406eacdb42433120166a7cd1 Mon Sep 17 00:00:00 2001 From: Chris Hughes <67111991+chrishughes20@users.noreply.github.com> Date: Fri, 16 Feb 2024 07:29:22 -0800 Subject: [PATCH] feat(libreoffice): preserve filenames for zipped files instead of using randomly generated UUID filenames (#793) * feat(libreoffice): preserve filenames for zipped files instead of using generated UUID filenames * chore: move github.com/google/uuid back to its original line * fix: add libreoffice/routes.go to PR --- pkg/modules/api/context.go | 16 ++++-- pkg/modules/libreoffice/routes.go | 18 ++++++- pkg/modules/libreoffice/routes_test.go | 72 ++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index ebb083e5..5efac5be 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -191,10 +191,18 @@ func (ctx *Context) FormData() *FormData { } } -// GeneratePath generates a path within the context's working directory. It -// does not create a file. -func (ctx *Context) GeneratePath(extension string) string { - return fmt.Sprintf("%s/%s%s", ctx.dirPath, uuid.New(), extension) +// GeneratePath generates a path within the context's working directory. It does not create a file. +// It either generates a new UUID-based filename or uses the provided filename. +func (ctx *Context) GeneratePath(extension string, optionalFilename ...string) string { + var filename string + if len(optionalFilename) > 0 { + // Use the provided filename + filename = optionalFilename[0] + } else { + // Generate a new UUID-based filename + filename = uuid.New().String() + } + return fmt.Sprintf("%s/%s%s", ctx.dirPath, filename, extension) } // AddOutputPaths adds the given paths. Those paths will be used later to build diff --git a/pkg/modules/libreoffice/routes.go b/pkg/modules/libreoffice/routes.go index 177308b4..d05b9080 100644 --- a/pkg/modules/libreoffice/routes.go +++ b/pkg/modules/libreoffice/routes.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" "net/http" + "path/filepath" + "strings" "github.com/labstack/echo/v4" @@ -51,10 +53,24 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap PdfUa: pdfua, } + // We need to check and see if there are any duplicate filenames in inputPaths. + filenameCounts := make(map[string]int) + for _, path := range inputPaths { + filename := strings.TrimSuffix(filepath.Base(path), filepath.Ext(path)) + filenameCounts[filename]++ + } + // Alright, let's convert each document to PDF. outputPaths := make([]string, len(inputPaths)) for i, inputPath := range inputPaths { - outputPaths[i] = ctx.GeneratePath(".pdf") + filename := strings.TrimSuffix(filepath.Base(inputPath), filepath.Ext(inputPath)) + extension := filepath.Ext(inputPath) + // Ex: `document.docx`, `document.doc` -> `document.docx.pdf`, `document.doc.pdf` + if filenameCounts[filename] > 1 { + outputPaths[i] = ctx.GeneratePath(".pdf", filename+extension) + } else { + outputPaths[i] = ctx.GeneratePath(".pdf", filename) + } options := libreofficeapi.Options{ Landscape: landscape, diff --git a/pkg/modules/libreoffice/routes_test.go b/pkg/modules/libreoffice/routes_test.go index 6470cc6a..6dd2971e 100644 --- a/pkg/modules/libreoffice/routes_test.go +++ b/pkg/modules/libreoffice/routes_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "reflect" "testing" "github.com/labstack/echo/v4" @@ -22,6 +23,7 @@ func TestConvertRoute(t *testing.T) { engine gotenberg.PdfEngine expectOptions libreofficeapi.Options expectError bool + expectFileNames []string expectHttpError bool expectHttpStatus int expectOutputPathsCount int @@ -492,6 +494,72 @@ func TestConvertRoute(t *testing.T) { expectHttpError: false, expectOutputPathsCount: 1, }, + { + scenario: "success (not merged, unique filenames with different extensions)", + ctx: func() *api.ContextMock { + ctx := &api.ContextMock{Context: new(api.Context)} + ctx.SetFiles(map[string]string{ + "document.docx": "/document.docx", + "document2.docx": "/document2.docx", + }) + ctx.SetValues(map[string][]string{ + "merge": { + "false", + }, + }) + 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"} + }, + }, + engine: &gotenberg.PdfEngineMock{ + MergeMock: func(ctx context.Context, logger *zap.Logger, inputPaths []string, outputPath string) error { + return nil + }, + }, + expectError: false, + expectHttpError: false, + expectOutputPathsCount: 2, + }, + { + scenario: "success (not merged, duplicate filenames with different extensions)", + 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.SetValues(map[string][]string{ + "merge": { + "false", + }, + }) + 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"} + }, + }, + engine: &gotenberg.PdfEngineMock{ + MergeMock: func(ctx context.Context, logger *zap.Logger, inputPaths []string, outputPath string) error { + return nil + }, + }, + expectError: false, + expectHttpError: false, + expectOutputPathsCount: 3, + expectFileNames: []string{"document.pdf", "document2.docx.pdf", "document2.doc.pdf"}, + }, { scenario: "success with non-native PDF/A & PDF/UA (merge)", ctx: func() *api.ContextMock { @@ -617,6 +685,10 @@ func TestConvertRoute(t *testing.T) { if tc.expectOutputPathsCount != len(tc.ctx.OutputPaths()) { t.Errorf("expected %d output paths but got %d", tc.expectOutputPathsCount, len(tc.ctx.OutputPaths())) } + + if !reflect.DeepEqual(tc.ctx.OutputPaths(), tc.expectFileNames) { + t.Errorf("expected output paths %v, got %v", tc.expectFileNames, tc.ctx.OutputPaths()) + } }) } }