[feature] request: add request:get-uploaded-file-headers()

[joewiz]

[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Summary

Adds request:get-uploaded-file-headers($name), which returns the part headers of each uploaded file in a multipart request — the remaining item from #6578, where a file part's own headers (e.g. its Content-Type) were discarded. Implements the accessor @line-o proposed on #6578.

Closes #6578. Companion to #6581, which fixes the underlying multipart parsing so uploads are available for every HTTP method (PUT/PATCH, not just POST) — the two together fully resolve #6578, and this function is most useful with #6581 in place.

What changed

• RequestWrapper — new getUploadedFileHeaders(String) interface method (HttpRequestWrapper is the only implementor).
• HttpRequestWrapper — reads each file Part's headers into a map, reusing the same file-part discrimination as getUploadedFileName.
• GetUploadedFileHeaders — the new request: function; registered in RequestModule.

Signature

request:get-uploaded-file-headers($upload-param-name as xs:string) as map(xs:string, xs:string)*

Returns one map (header name → value) per uploaded file submitted under $name, in submission order; the empty sequence when the request is not multipart or the name is not a file part.

request:get-uploaded-file-headers("file")
(: → ( map { "Content-Disposition": 'form-data; name="file"; filename="a.xml"', "Content-Type": "application/xml" } ) :)

Design choices to confirm on review

@line-o's suggestion was as map(xs:string, xs:string). A few decisions worth a look, since a couple depart slightly from that literal shape:

1. Cardinality — map(...)* rather than a single map. The sibling accessors request:get-uploaded-file-name / -size already return sequences, because one field name can carry multiple files (<input type="file" multiple>). Returning one map per file, positionally aligned with those functions, keeps the family consistent; a single map couldn't represent a multi-file upload. Easy to change to a single map if preferred, but the sequence seems more correct.
2. Header-name keys are kept as submitted (not lower-cased). HTTP header names are case-insensitive, so if callers would rather look them up case-insensitively we could normalize keys (e.g. to lower case) — happy to, just flagging that it's currently as-sent (whatever the servlet container reports).
3. One value per header (Part.getHeader, the first value). Parts can in principle repeat a header; map(xs:string, xs:string) collapses that to one value. If repeated part headers matter, the value type would need to widen (e.g. map(xs:string, xs:string*)).

Naming: went with request:get-uploaded-file-headers (over the alternative -headers-for) to match the get-uploaded-file-* family. Happy to switch.

Test plan

• GetUploadedFileHeadersTest (new) — multipart POST with a file part and a plain field; asserts one header map for the file (exposing its Content-Type and Content-Disposition) and none for the field.
• org.exist.xquery.functions.request.** — full package green.
• Codacy (PMD) on the changed files — no new findings.

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Thanks @reinhapa — both addressed.

• Text block (ee37014): the embedded XQuery in the test is now a Java text block instead of concatenation. Apologies for overlooking this before pushing.
• Cleaner (c8c4ecc): replaced the deprecated finalize() that deleted the temporary copies of uploaded file parts with a java.lang.ref.Cleaner. The cleaning action is a static nested class holding only the map of temporary files — never the wrapper — so it can't keep the wrapper from being collected. The temporary-file cache is now eagerly created and final, which also let me drop the lazy null-checks in getFileUploadParam.

Verified with GetUploadedFileHeadersTest and GetParameterTest (the latter exercises the file-upload / temp-file path).

[dizzzz]

Nice to learn about Cleaner, did not know this.

Copilot provides some review feedback, you might want to review this:

---

Gaps in Test Coverage

Comparing to the similar GetParameterTest (which has 10+ test methods covering combinatorics), the GetUploadedFileHeaders test is quite sparse. Here are the gaps:

1. Multi-file uploads ❌

The PR body explicitly states cardinality design for multi-file support, but there's no test for:

Multiple files with the same parameter name ()
Each file should return its own header map
Maps should be positionally aligned with request:get-uploaded-file-name() and request:get-uploaded-file-size()
Why it matters: This is the key design choice (#1 in the PR). Without this test, you can't be confident the sequence cardinality works as intended.

2. Header-name case sensitivity ❌

The PR explicitly mentions header names are kept "as-submitted" (not normalized to lowercase). But there's no test for:

What happens when a servlet container reports header names in different cases (Content-Type vs. content-type vs. CONTENT-TYPE)
Whether the map keys are retrievable case-sensitively (they should be, since LinkedHashMap is case-sensitive)
Why it matters: This is design choice (#2). The current test uses .toLowerCase() on the body string to check assertions, masking any case sensitivity issues.

3. Different MIME types ❌

The current test uses MediaType.TEXT_PLAIN. No tests for:

Binary files (application/octet-stream, application/pdf, etc.)
Other common types (image/png, application/json)
Custom MIME types
Why it matters: Verifies the function is content-agnostic and the headers are as reported by the servlet.

4. Non-existent parameter names ❌

No explicit test for:

Calling request:get-uploaded-file-headers("nonexistent") → should return empty sequence ()
Calling with a field name that's not a file (already implicitly tested, but worth explicit)
Why it matters: Clarifies the null/empty behavior.

5. Missing headers (edge case) ❌

No test for:

An uploaded file with no headers (unlikely in practice, but technically possible)
A file with only Content-Disposition, no Content-Type
Why it matters: Verifies the implementation handles sparse headers gracefully.

6. Empty file ❌

No test for:

Zero-byte file upload
Verifies headers are still present for empty files
Why it matters: Confirms headers aren't lost when file content is absent.

7. Special characters in filename ❌

No test for:

Filenames with non-ASCII characters (e.g., café.txt, Chinese characters)
Verifies Content-Disposition header encoding is preserved
Why it matters: Real-world robustness check.

[dizzzz]

after re-request:

I'll analyze this pull request by examining the changes and identifying test coverage gaps with real risk.Now let me fetch the files changed to understand the implementation:Now I need to fetch the review comments to understand what the "CHANGES_REQUESTED" is about:## Test Gap Analysis

Overall Assessment: The PR has one meaningful test gap with real risk.

---

Gap Identified

Missing test for the isFormDataParsed flag check — MEDIUM severity

Risk: The core logic in getUploadedFileHeaders() returns null when !isFormDataParsed:

if (!isFormDataParsed) {
    return null;
}

This check protects against calling the function on non-multipart requests. However, the test only exercises multipart POST requests — the success path where isFormDataParsed=true.

What's untested:

• Calling getUploadedFileHeaders() on a non-multipart request (e.g., a regular GET or POST with application/x-www-form-urlencoded)
• Calling it on a multipart request where the parameter name doesn't exist

Both should return null or an empty sequence, but this is not verified. Since the sibling functions (getUploadedFileName, getFileUploadParam) have the same guards, and the PR modifies the cleanup logic that runs during wrapper initialization, a regression in the isFormDataParsed check could leak temporary files or expose stale data across requests.

Recommended test additions:

1. Test calling request:get-uploaded-file-headers() on a regular POST (non-multipart) → should return empty sequence
2. Test calling it with a parameter name that doesn't exist in a multipart request → should return empty sequence
3. Test with multiple uploaded files under the same parameter name (to verify positional alignment with getUploadedFileName and getFileUploadParam)

---

Other Observations (Not Gaps)

• ✅ The new TemporaryUploadedFilesCleaner and Cleaner API migration are well-designed and avoid the finalize() anti-pattern — appropriate for resource cleanup
• ✅ GetUploadedFileHeadersTest correctly verifies that file parts expose headers and plain form fields do not
• ✅ The implementation correctly uses getFileItem() to filter out non-file parts (matching the pattern of sibling accessors)
• ✅ Full package test suite passes (as noted in PR description)

[dizzzz]

LGTM - some test remarks

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Thanks @dizzzz — and glad the Cleaner was a useful find; it's the modern replacement for finalize() and a good fit wherever a finalize() was only there to release a resource.

Added tests for the gaps with real risk (611226b):

• Multiple files under one parameter name — verifies one header map per file, positionally aligned with request:get-uploaded-file-name (this is the map-cardinality design choice from the PR body).
• Unknown parameter name → empty sequence.
• Non-multipart request → empty sequence — this one exercises the isFormDataParsed guard you and Copilot both flagged, which the original test didn't reach.

The five cases now share one fixture, parameterized by an inspect URL parameter.

I deliberately left out the remaining Copilot suggestions (MIME-type variety, empty file, sparse/missing headers, non-ASCII filenames): the function just passes each part's headers through as the servlet container reports them, so those would mostly be testing Jetty's multipart parsing rather than this code. Happy to add any of them if you see a specific risk I'm missing.

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

Done (5f4a7ce) — TemporaryUploadedFilesCleaner is now a record. It holds a single component (the map of parts to their temporary copies) and implements Runnable, so the canonical constructor and field replace the hand-written ones with no behavior change. Verified with GetUploadedFileHeadersTest and GetParameterTest (the latter exercises the temp-file cleanup path).

[reinhapa]

Convert TemporaryUploadedFilesCleaner into a record

[dizzzz]

@joewiz please can you have a look?

[line-o]

LGTM