diff --git a/Makefile b/Makefile index cd7242c1..85dace02 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,7 @@ API_DOWNLOAD_FROM_DENY_PUBLIC_IPS=false API_DOWNLOAD_FROM_ENABLE_ENVIRONMENT_PROXY=false API_DOWNLOAD_FROM_MAX_RETRY=4 API_DOWNLOAD_FROM_MAX_CONCURRENCY=10 -API_DOWNLOAD_FROM_MAX_ENTRIES=1000 +API_DOWNLOAD_FROM_MAX_ENTRIES=0 API_DISABLE_DOWNLOAD_FROM=false API_DISABLE_HEALTH_CHECK_ROUTE_TELEMETRY=true API_DISABLE_ROOT_ROUTE_TELEMETRY=true diff --git a/pkg/modules/api/api.go b/pkg/modules/api/api.go index 663a24e4..0b4506f7 100644 --- a/pkg/modules/api/api.go +++ b/pkg/modules/api/api.go @@ -215,7 +215,7 @@ func (a *Api) Descriptor() gotenberg.ModuleDescriptor { fs.Bool("api-download-from-enable-environment-proxy", false, "Route downloadFrom fetches through the proxy defined by the standard HTTP_PROXY, HTTPS_PROXY, and NO_PROXY variables, including credentials") fs.Int("api-download-from-max-retry", 4, "Set the maximum number of retries for the download from feature") fs.Int("api-download-from-max-concurrency", 10, "Set the maximum number of downloadFrom entries fetched concurrently per request - bounds the outbound fan-out. Set to 0 to disable this feature") - fs.Int("api-download-from-max-entries", 1000, "Set the maximum number of downloadFrom entries allowed per request. Set to 0 to disable this limit, which lets a single request expand into an arbitrarily large array") + fs.Int("api-download-from-max-entries", 0, "Set the maximum number of downloadFrom entries allowed per request. Set to 0 to disable this feature") fs.Bool("api-disable-download-from", false, "Disable the download from feature") fs.Bool("api-disable-health-check-route-telemetry", true, "Disable telemetry for health check route") fs.Bool("api-disable-root-route-telemetry", true, "Disable telemetry for the root route") diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 6792e171..9172e0af 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -51,6 +51,17 @@ type Context struct { outputPaths []string cancelled bool + // fileOrder records the order files were received in, keyed by disk path. + // It breaks ties when two uploads share an original filename, so that + // de-duplicated files keep their upload order instead of being ordered by + // the suffix uniqueFilename added. + fileOrder map[string]int + + // fileBase maps a disk path to the original filename as received, before + // de-duplication. Sorting on it keeps a de-duplicated file next to its + // twin rather than wherever its numbered name would land. + fileBase map[string]string + // outputFilename is the sanitized Gotenberg-Output-Filename header, // snapshotted while the [echo.Context] is still live. Echo returns that // context to a pool as soon as the handler returns, and an asynchronous @@ -515,6 +526,7 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys filename := ctx.uniqueFilename(r.filename) ctx.files[filename] = r.path ctx.diskToOriginal[r.path] = filename + ctx.trackFileOrder(r.path, r.filename) if r.formField != "" { ctx.filesByField[r.formField] = append(ctx.filesByField[r.formField], r.path) } @@ -566,9 +578,11 @@ func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSys return "", fmt.Errorf("copy multipart file to local file: %w", err) } + base := filename filename = ctx.uniqueFilename(filename) ctx.files[filename] = path ctx.diskToOriginal[path] = filename + ctx.trackFileOrder(path, base) return filename, nil } @@ -626,6 +640,8 @@ func (ctx *Context) FormData() *FormData { files: ctx.files, filesByField: ctx.filesByField, diskToOriginal: ctx.diskToOriginal, + fileOrder: ctx.fileOrder, + fileBase: ctx.fileBase, errors: nil, } } @@ -822,6 +838,19 @@ func (ctx *Context) uniqueFilename(filename string) string { } } +// trackFileOrder records where a file arrived in the request and the filename +// it arrived under, so [FormData.paths] can order it the way the caller sent +// it. +func (ctx *Context) trackFileOrder(path, base string) { + if ctx.fileOrder == nil { + ctx.fileOrder = make(map[string]int) + ctx.fileBase = make(map[string]string) + } + + ctx.fileOrder[path] = len(ctx.fileOrder) + ctx.fileBase[path] = base +} + // sanitizeFilename strips path separators (including backslashes, which // [filepath.Base] ignores on Linux) and control characters from a // caller-supplied filename, then NFC-normalizes the result. This prevents a diff --git a/pkg/modules/api/formdata.go b/pkg/modules/api/formdata.go index 8a5d69a7..52869db2 100644 --- a/pkg/modules/api/formdata.go +++ b/pkg/modules/api/formdata.go @@ -41,6 +41,8 @@ type FormData struct { files map[string]string filesByField map[string][]string diskToOriginal map[string]string + fileOrder map[string]int + fileBase map[string]string errors error } @@ -582,24 +584,42 @@ func (form *FormData) paths(extensions []string, target *[]string) *FormData { } // See https://github.com/gotenberg/gotenberg/issues/139. - originals := make(gotenberg.AlphanumericSort, len(entries)) - for i, e := range entries { - originals[i] = e.original - } - sort.Sort(originals) + // + // Sort on the filename as received rather than on the map key. The key + // carries the suffix uniqueFilename adds when two uploads share a name, + // and that suffix would otherwise decide the order: "doc (2).pdf" sorts + // before "doc.pdf". Ordering on the received name keeps the pair adjacent, + // and the arrival index breaks the tie, so duplicates merge in the order + // the caller sent them. A file with a unique name is unaffected, since its + // received name and its key are the same string. + sort.SliceStable(entries, func(i, j int) bool { + nameI := form.receivedName(entries[i].disk, entries[i].original) + nameJ := form.receivedName(entries[j].disk, entries[j].original) + if nameI != nameJ { + return gotenberg.AlphanumericSort{nameI, nameJ}.Less(0, 1) + } + + return form.fileOrder[entries[i].disk] < form.fileOrder[entries[j].disk] + }) - // Build a lookup from original name to disk path. - lookup := make(map[string]string, len(entries)) for _, e := range entries { - lookup[e.original] = e.disk - } - for _, o := range originals { - *target = append(*target, lookup[o]) + *target = append(*target, e.disk) } return form } +// receivedName returns the filename the file at disk arrived under, before +// de-duplication, falling back to fallback. +func (form *FormData) receivedName(disk, fallback string) string { + base, ok := form.fileBase[disk] + if ok { + return base + } + + return fallback +} + // append adds an error to the list of errors. func (form *FormData) append(err error) { form.errors = errors.Join(form.errors, err) diff --git a/pkg/modules/api/formdata_test.go b/pkg/modules/api/formdata_test.go index de6e2a37..bc26a907 100644 --- a/pkg/modules/api/formdata_test.go +++ b/pkg/modules/api/formdata_test.go @@ -1895,3 +1895,81 @@ func TestFormData_Watermarks(t *testing.T) { t.Errorf("expected %+v, got %+v", want, got) } } + +// De-duplicating a repeated filename must not change merge order. Files with +// unique names keep exactly the order they had before de-duplication existed, +// and two files sharing a name merge in the order the caller sent them. +func TestFormData_paths_DuplicateFilenamesKeepUploadOrder(t *testing.T) { + for _, tc := range []struct { + scenario string + files map[string]string + fileBase map[string]string + order map[string]int + want []string + }{ + { + scenario: "unique names sort exactly as before", + files: map[string]string{"b.pdf": "/w/2", "a.pdf": "/w/1", "c.pdf": "/w/3"}, + fileBase: map[string]string{"/w/1": "a.pdf", "/w/2": "b.pdf", "/w/3": "c.pdf"}, + order: map[string]int{"/w/1": 0, "/w/2": 1, "/w/3": 2}, + want: []string{"/w/1", "/w/2", "/w/3"}, + }, + { + scenario: "numeric prefixes still win", + files: map[string]string{"10_x.pdf": "/w/3", "2_x.pdf": "/w/2", "1_x.pdf": "/w/1"}, + fileBase: map[string]string{"/w/1": "1_x.pdf", "/w/2": "2_x.pdf", "/w/3": "10_x.pdf"}, + order: map[string]int{"/w/1": 0, "/w/2": 1, "/w/3": 2}, + want: []string{"/w/1", "/w/2", "/w/3"}, + }, + { + scenario: "duplicates merge in upload order, not suffix order", + files: map[string]string{"doc.pdf": "/w/1", "doc (2).pdf": "/w/2"}, + fileBase: map[string]string{"/w/1": "doc.pdf", "/w/2": "doc.pdf"}, + order: map[string]int{"/w/1": 0, "/w/2": 1}, + want: []string{"/w/1", "/w/2"}, + }, + { + scenario: "duplicates stay adjacent and in position", + files: map[string]string{ + "a.pdf": "/w/1", "doc.pdf": "/w/2", "doc (2).pdf": "/w/3", "z.pdf": "/w/4", + }, + fileBase: map[string]string{ + "/w/1": "a.pdf", "/w/2": "doc.pdf", "/w/3": "doc.pdf", "/w/4": "z.pdf", + }, + order: map[string]int{"/w/1": 0, "/w/2": 1, "/w/3": 2, "/w/4": 3}, + want: []string{"/w/1", "/w/2", "/w/3", "/w/4"}, + }, + { + scenario: "three copies keep their order", + files: map[string]string{"r.pdf": "/w/1", "r (2).pdf": "/w/2", "r (3).pdf": "/w/3"}, + fileBase: map[string]string{"/w/1": "r.pdf", "/w/2": "r.pdf", "/w/3": "r.pdf"}, + order: map[string]int{"/w/1": 0, "/w/2": 1, "/w/3": 2}, + want: []string{"/w/1", "/w/2", "/w/3"}, + }, + } { + t.Run(tc.scenario, func(t *testing.T) { + form := &FormData{ + files: tc.files, + filesByField: map[string][]string{}, + fileBase: tc.fileBase, + fileOrder: tc.order, + } + + // Map iteration is randomised, so run it repeatedly: an unstable + // comparator shows up as a differing result across runs. + for range 50 { + var got []string + form.paths([]string{".pdf"}, &got) + + if len(got) != len(tc.want) { + t.Fatalf("paths() returned %d entries, want %d", len(got), len(tc.want)) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("paths() = %v, want %v", got, tc.want) + } + } + } + }) + } +}