WW-5413 Avoid writing small in-memory multipart uploads to disk#1805
WW-5413 Avoid writing small in-memory multipart uploads to disk#1805lukaszlenart wants to merge 13 commits into
Conversation
Lazy-materializing UploadedFile plus a new getInputStream() accessor so small (in-memory) uploads no longer eagerly write a temp file, while getContent() keeps returning a File for backward compatibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rceptor validation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…TENT_TYPE_KEY Mark the orphaned constant @deprecated(forRemoval = true) instead of leaving it silently unused. The message key it referenced was only emitted from an unreachable block in acceptFile() that was removed with the in-memory upload optimization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR implements WW-5413’s in-memory multipart upload optimization by avoiding eager temp-file writes for small uploads and introducing a streaming read API plus lazy file materialization that preserves getContent()’s File runtime type.
Changes:
- Added
UploadedFile.getInputStream()(default) andUploadedFile.isMissing()(default), plus aStrutsUploadedFileoverride for file-backed streaming. - Introduced
StrutsInMemoryUploadedFileto keep small uploads in memory and materialize a secure tempFilelazily ongetContent()/getAbsolutePath(). - Updated multipart parsing + interceptor validation + tests to ensure validation no longer forces disk writes for in-memory uploads.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md | Design spec for lazy materialization + streaming accessor approach |
| docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md | Implementation plan and testing checklist for WW-5413 |
| core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java | Adds default getInputStream() and isMissing() APIs |
| core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java | Overrides getInputStream() for file-backed uploads |
| core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java | New in-memory UploadedFile with lazy temp-file materialization |
| core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java | Stops eager disk writes for in-memory items; simplifies cleanup |
| core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java | Avoids materializing uploads during validation (isMissing()) |
| core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java | Adds regression tests ensuring interceptor validation doesn’t materialize |
| core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java | Covers default interface methods (getInputStream(), isMissing()) |
| core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java | Verifies file-backed streaming |
| core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java | Verifies lazy materialization, serialization, and non-materializing paths |
| core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java | Adds integration coverage for “no disk write until demanded” |
| core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java | Updates assertions to align with lazy materialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…default branches Address review follow-ups on PR #1805: - document that processFileField's retained 'throws IOException' is intentional (subclass source compatibility), not an oversight - add a negative test: getContent() on an unwritable save dir throws StrutsException, stays unmaterialized, and leaves no partial file behind - cover the UploadedFile.getInputStream() default File branch and the no-content IOException branch Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- materialize() now writes with StandardOpenOption.CREATE_NEW and fails closed if the target already exists, so a pre-planted file/symlink is never overwritten or followed (Copilot security note) + regression test - defensively copy the content byte array on construction and reject null content, so the instance owns its bytes and cannot observe caller mutation (Copilot / review) - delete() uses Files.deleteIfExists and logs the real cause on failure instead of a silent File.delete() boolean (Sonar MAJOR) - reorder field modifiers to JLS order 'transient volatile' (Sonar) - tests: assertThat(dir).isEmptyDirectory() instead of listFiles().isEmpty() (Sonar); drop unused DiskFileItem import (Sonar) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|



Fixes WW-5413
Background
The original ticket (filed against 6.3.0) was a commons-io
DeferredFileOutputStream/ThresholdingOutputStreamregression 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 thatFileinStrutsUploadedFile. 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()— newdefaultmethod: read upload bytes without forcing a disk write.default, so third-party implementations keep compiling.getContent()still returns ajava.io.Fileeverywhere (neverbyte[]), so all existing consumers are unaffected.StrutsInMemoryUploadedFile— a byte-array-backedUploadedFile.getInputStream()reads from memory (no disk);getContent()/getAbsolutePath()lazily write the bytes to a secureupload_<uuid>.tmpfile exactly once and cache it;isFile()is false until materialized.Serializable(holds ajava.io.Filetarget, not a non-serializablePath) and thread-safe (synchronizedmaterialize,volatilecache).JakartaMultiPartRequest— builds the in-memory type forisInMemory()items; the eagerFileOutputStreamwrite, thetemporaryFileslist, andcleanUpTemporaryFiles()are removed. Cleanup rides the existingAbstractMultiPartRequest.cleanUp()delete()-when-isFile()loop.AbstractFileUploadInterceptor.acceptFile()— previously calledgetContent()as its first (failed-upload) check, which materialized every small upload during validation on the commonfileUpload/actionFileUploadpath. It now uses a new non-materializingUploadedFile.isMissing()(default= getContent() == null, overridden inStrutsInMemoryUploadedFileto answer from memory). A redundant deadgetContent() == nullblock was removed. Net result: theUploadedFilesAwareflow validates and hands files to the action without ever writing a small upload to disk; only a consumer that explicitly needs aFile(the legacyFile-typed action property viaUploadedFileConverter) triggers the write.Backward compatibility
getContent()runtime type staysjava.io.Filefor every implementation.getInputStream()andisMissing()aredefaultmethods — third-partyUploadedFileimpls are unaffected.Behavior note
A materialization write failure now surfaces as an unchecked
StrutsExceptionat the point of consumption rather than as a parse-timeLocalizedMessage(the interface'sgetContent()/getAbsolutePath()cannot declare checked exceptions). This affects only the rare "cannot write to the save directory" case, and only on the legacyFile/getContent()path — thegetInputStream()fast path never writes.Known limitation
StrutsInMemoryUploadedFilebakes 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 latergetContent()materializes to that originating node's path. Consumers needing content to survive cross-node replication should read viagetInputStream(). Documented on the class Javadoc.Deprecations / minor notes
STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEYinAbstractFileUploadInterceptoris now unused (its only site was the removed dead block). Since it ispublic static final, it is marked@Deprecated(since = "7.3.0", forRemoval = true)rather than removed outright, giving downstream users a migration window. Itsstruts.messages.invalid.content.typei18n message is retained in the resource bundles. The vestigialthrows IOExceptiononJakartaMultiPartRequest.processFileFieldwas left in place to avoid breaking a subclass that catches it fromsuper.Tests
New:
UploadedFileTest,StrutsUploadedFileTest,StrutsInMemoryUploadedFileTest(lazy materialization, no-write stream path, serialization round-trip, secure naming,isMissing()), aJakartaMultiPartRequestTestintegration test proving no disk write until content is requested, andActionFileUploadInterceptorTestcases 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.mdanddocs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md.🤖 Generated with Claude Code