[
https://issues.apache.org/jira/browse/WW-5413?focusedWorklogId=1031660&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031660
]
ASF GitHub Bot logged work on WW-5413:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 12:24
Start Date: 22/Jul/26 12:24
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1805:
URL: https://github.com/apache/struts/pull/1805
Fixes [WW-5413](https://issues.apache.org/jira/browse/WW-5413)
## Background
The original ticket (filed against 6.3.0) was a commons-io
`DeferredFileOutputStream`/`ThresholdingOutputStream` regression that made
multipart uploads read as empty. **That root cause is already resolved** by the
migration to commons-fileupload2 2.0.0-M5 + commons-io 2.22.0 — no threshold is
forced to `-1`, so small uploads legitimately stay in memory
(`DiskFileItem.isInMemory()`).
What remained is the ticket's **performance** half:
`JakartaMultiPartRequest.processFileField()` still took every in-memory item
and eagerly wrote it to a temp file, wrapping that `File` in
`StrutsUploadedFile`. The redundant filesystem write just moved from the
fileupload layer into Struts' own code.
## What this changes
Small in-memory uploads are no longer eagerly written to disk. A temp file
is materialized **only when a caller actually asks for a `File`** — and the
standard interceptor validation path no longer forces that.
- **`UploadedFile.getInputStream()`** — new `default` method: read upload
bytes without forcing a disk write. `default`, so third-party implementations
keep compiling. `getContent()` still returns a `java.io.File` everywhere (never
`byte[]`), so all existing consumers are unaffected.
- **`StrutsInMemoryUploadedFile`** — a byte-array-backed `UploadedFile`.
`getInputStream()` reads from memory (no disk);
`getContent()`/`getAbsolutePath()` lazily write the bytes to a secure
`upload_<uuid>.tmp` file exactly once and cache it; `isFile()` is false until
materialized. `Serializable` (holds a `java.io.File` target, not a
non-serializable `Path`) and thread-safe (`synchronized` materialize,
`volatile` cache).
- **`JakartaMultiPartRequest`** — builds the in-memory type for
`isInMemory()` items; the eager `FileOutputStream` write, the `temporaryFiles`
list, and `cleanUpTemporaryFiles()` are removed. Cleanup rides the existing
`AbstractMultiPartRequest.cleanUp()` `delete()`-when-`isFile()` loop.
- **`AbstractFileUploadInterceptor.acceptFile()`** — previously called
`getContent()` as its first (failed-upload) check, which materialized every
small upload during validation on the common `fileUpload`/`actionFileUpload`
path. It now uses a new non-materializing `UploadedFile.isMissing()` (default
`= getContent() == null`, overridden in `StrutsInMemoryUploadedFile` to answer
from memory). A redundant dead `getContent() == null` block was removed. Net
result: the `UploadedFilesAware` flow validates and hands files to the action
**without ever writing a small upload to disk**; only a consumer that
explicitly needs a `File` (the legacy `File`-typed action property via
`UploadedFileConverter`) triggers the write.
## Backward compatibility
- `getContent()` runtime type stays `java.io.File` for every implementation.
- `getInputStream()` and `isMissing()` are `default` methods — third-party
`UploadedFile` impls are unaffected.
- Upload validation is unchanged: size / content-type / extension /
missing-content rejections all still fire (verified), and the secure UUID
temp-file naming (original filename never influences the on-disk name) is
preserved.
## Behavior note
A materialization write failure now surfaces as an unchecked
`StrutsException` at the point of consumption rather than as a parse-time
`LocalizedMessage` (the interface's `getContent()`/`getAbsolutePath()` cannot
declare checked exceptions). This affects only the rare "cannot write to the
save directory" case, and only on the legacy `File`/`getContent()` path — the
`getInputStream()` fast path never writes.
## Known limitation
`StrutsInMemoryUploadedFile` bakes an absolute temp path resolved on the
originating node. If an un-materialized instance is serialized (e.g. session
replication) and deserialized on another node, a later `getContent()`
materializes to that originating node's path. Consumers needing content to
survive cross-node replication should read via `getInputStream()`. Documented
on the class Javadoc.
## Minor cleanup left for a follow-up
`STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY` in
`AbstractFileUploadInterceptor` is now unused (its only site was the removed
dead block) but was left in place because it is `public static final` and
removing it would be an API break. The vestigial `throws IOException` on
`JakartaMultiPartRequest.processFileField` was likewise left in place to avoid
breaking a subclass that catches it from `super`.
## Tests
New: `UploadedFileTest`, `StrutsUploadedFileTest`,
`StrutsInMemoryUploadedFileTest` (lazy materialization, no-write stream path,
serialization round-trip, secure naming, `isMissing()`), a
`JakartaMultiPartRequestTest` integration test proving no disk write until
content is requested, and `ActionFileUploadInterceptorTest` cases proving
validation does not materialize on accept or reject. Existing tests that
asserted the old eager-write behavior were removed or updated. Full multipart +
interceptor + converter suite: **114 tests green**.
## Design docs
`docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md`
and
`docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1031660)
Remaining Estimate: 0h
Time Spent: 10m
> Multipart misbehavior with commons-io 2.16.0 and 2.16.1
> -------------------------------------------------------
>
> Key: WW-5413
> URL: https://issues.apache.org/jira/browse/WW-5413
> Project: Struts 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 6.3.0
> Reporter: Riccardo Proserpio
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> commons-io 2.16.0 has broken the implementation of
> DeferredFileOutputStream changing the behavior of its superclass
> ThresholdingOutputStream: IO-854
>
> The class is used by commons-fileupload DiskFileItem, that is used by Struts
> to handle multipart uploads. The issue causes each multipart part to be read
> as empty.
>
> A fix has been implemented in 2.16.1. However, the fix exposes an issue in
> how the getFile of JakartaMultiPartRequest uses DiskFileItem, that causes it
> to mishandle zero length inputs.
>
> The issue is related to WW-5088 and WW-5146
>
> Moreover, the fix implemented for this issues seems to be dubious and affects
> not only file uploads but every field encoded as multipart/form-data: by
> forcing the diskfileitem threshold to be -1, each and every field was written
> to the filesystem.
>
> The behavior of threadshold -1 was underspecified and inconsistent with the
> commons-io implementation, and has been specified in 2.16.1.
>
> To really fix the issue, I suggest to avoid specifying -1 on the
> DiskFileItemFactory and to properly handle the case when the
> DiskFileItem.isInMemory() returns true in the JakartaMultiPartRequest.getFile
> method: in this case getStoreLocation() is defined to return null and the
> bytes should be read from memory instead.
>
> Avoiding always spilling to disk each and every multipart part should also be
> a performance win, considering that multipart can also be used to transfer
> normal form inputs and not only files.
>
> What do you think?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
