Skip to content

WW-5413 Avoid writing small in-memory multipart uploads to disk#1805

Open
lukaszlenart wants to merge 13 commits into
mainfrom
WW-5413-inmemory-upload-optimization
Open

WW-5413 Avoid writing small in-memory multipart uploads to disk#1805
lukaszlenart wants to merge 13 commits into
mainfrom
WW-5413-inmemory-upload-optimization

Conversation

@lukaszlenart

@lukaszlenart lukaszlenart commented Jul 22, 2026

Copy link
Copy Markdown
Member

Fixes 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.

Deprecations / minor notes

STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY in AbstractFileUploadInterceptor is now unused (its only site was the removed dead block). Since it is public static final, it is marked @Deprecated(since = "7.3.0", forRemoval = true) rather than removed outright, giving downstream users a migration window. Its struts.messages.invalid.content.type i18n message is retained in the resource bundles. The vestigial throws IOException on JakartaMultiPartRequest.processFileField was 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

lukaszlenart and others added 11 commits July 22, 2026 09:56
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>
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>
@lukaszlenart
lukaszlenart marked this pull request as ready for review July 22, 2026 12:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and UploadedFile.isMissing() (default), plus a StrutsUploadedFile override for file-backed streaming.
  • Introduced StrutsInMemoryUploadedFile to keep small uploads in memory and materialize a secure temp File lazily on getContent()/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.

lukaszlenart and others added 2 commits July 22, 2026 19:13
…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>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants