Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 14:47:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* WW-5413 docs(core): design for in-memory multipart upload optimization

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>

* WW-5413 docs(core): implementation plan for in-memory upload optimization

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-5413 fix(core): make StrutsInMemoryUploadedFile serializable and thread-safe

* WW-5413 refactor(core): drop eager temp-file write for in-memory uploads

* WW-5413 test(core): cover deferred-write behavior for in-memory uploads

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation

* WW-5413 chore(core): clean up partial materialization and cover isMissing()

* WW-5413 docs(core): sync design/plan with interceptor fix and deviations

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-5413 chore(core): deprecate now-unused STRUTS_MESSAGES_INVALID_CONTENT_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>

* WW-5413 test(core): cover materialization failure and getInputStream 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>

* WW-5413 fix(core): address SonarCloud and Copilot review findings

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

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
2026-07-26 04:29:05 +02:00
committed by GitHub
parent 8a5323fbdf
commit c3e9db1b21
13 changed files with 1702 additions and 293 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+28 -95
View File
@@ -25,12 +25,9 @@ import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpl
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.dispatcher.LocalizedMessage;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
@@ -41,13 +38,14 @@ import static org.apache.commons.lang3.StringUtils.normalizeSpace;
* Multipart form data request adapter for Jakarta Commons FileUpload package.
*
* <p>This implementation provides secure handling of multipart requests with proper
* resource management and cleanup. It tracks all temporary files created during
* the upload process and ensures they are properly cleaned up to prevent
* resource leaks and security vulnerabilities.</p>
*
* resource management and cleanup. It tracks all {@link DiskFileItem} instances
* created during the upload process and ensures they are properly cleaned up to
* prevent resource leaks and security vulnerabilities. In-memory uploads are kept
* as byte arrays and only materialized to a temporary file lazily, on demand.</p>
*
* <p>Key features:</p>
* <ul>
* <li>Automatic tracking and cleanup of temporary files</li>
* <li>Automatic tracking and cleanup of underlying disk file items</li>
* <li>Proper error handling with user-friendly error messages</li>
* <li>Support for both in-memory and disk-based file uploads</li>
* <li>Extensible cleanup mechanisms for customization</li>
@@ -79,11 +77,6 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
* List to track all DiskFileItem instances for proper cleanup
*/
private final List<DiskFileItem> diskFileItems = new ArrayList<>();
/**
* List to track temporary files created for in-memory uploads
*/
private final List<File> temporaryFiles = new ArrayList<>();
/**
* Processes the multipart upload request using Jakarta Commons FileUpload.
@@ -181,20 +174,21 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
* <ol>
* <li>Validating the file name and field name are not null/empty</li>
* <li>Determining if the file is stored in memory or on disk</li>
* <li>For in-memory files: creating a temporary file and copying content</li>
* <li>For in-memory files: wrapping the content in a {@link StrutsInMemoryUploadedFile}
* that only writes to disk lazily, on demand</li>
* <li>For disk files: using the existing file directly</li>
* <li>Creating an {@link UploadedFile} abstraction</li>
* <li>Adding the file to the uploaded files collection</li>
* </ol>
*
* <p>Temporary files created for in-memory uploads are automatically
* tracked for cleanup. Any errors during temporary file creation are
* logged and added to the error list for user feedback.</p>
*
*
* @param item the disk file item representing the uploaded file
* @see #cleanUpTemporaryFiles()
* @throws IOException never thrown by this implementation (in-memory content is materialized
* lazily elsewhere); the clause is retained on the signature deliberately for
* source compatibility with subclasses that override this method or catch it
* from {@code super.processFileField(...)}
*/
protected void processFileField(DiskFileItem item, String saveDir) {
// NOTE: `throws IOException` is intentionally retained for subclass source compatibility - do not remove.
protected void processFileField(DiskFileItem item, String saveDir) throws IOException {
// Skip file uploads that don't have a file name - meaning that no file was selected.
if (item.getName() == null || item.getName().trim().isEmpty()) {
LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName()));
@@ -215,40 +209,14 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
List<UploadedFile> values = uploadedFiles.computeIfAbsent(fieldName, k -> new ArrayList<>());
if (item.isInMemory()) {
LOG.debug(() -> "Creating temporary file representing in-memory uploaded item: " + normalizeSpace(item.getFieldName()));
try {
File tempFile = createTemporaryFile(item.getName(), Path.of(saveDir));
// Track the temporary file for explicit cleanup
temporaryFiles.add(tempFile);
// Write the in-memory content to the temporary file
try (java.io.FileOutputStream fos = new java.io.FileOutputStream(tempFile)) {
fos.write(item.get());
}
UploadedFile uploadedFile = StrutsUploadedFile.Builder
.create(tempFile)
.withOriginalName(item.getName())
.withContentType(item.getContentType())
.withInputName(item.getFieldName())
.build();
values.add(uploadedFile);
if (LOG.isDebugEnabled()) {
LOG.debug("Created temporary file for in-memory uploaded item: {} at {}",
normalizeSpace(item.getName()), tempFile.getAbsolutePath());
}
} catch (IOException e) {
LOG.warn("Failed to create temporary file for in-memory uploaded item: {}",
normalizeSpace(item.getName()), e);
// Add the error to the errors list for proper user feedback
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{item.getName()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
}
LOG.debug(() -> "Keeping in-memory uploaded item without writing to disk: " + normalizeSpace(item.getFieldName()));
UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder
.create(item.get(), Path.of(saveDir))
.withOriginalName(item.getName())
.withContentType(item.getContentType())
.withInputName(item.getFieldName())
.build();
values.add(uploadedFile);
} else {
UploadedFile uploadedFile = StrutsUploadedFile.Builder
.create(item.getPath().toFile())
@@ -276,9 +244,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
* be overridden by subclasses to customize cleanup behavior. All exceptions
* are caught and logged to prevent cleanup failures from affecting the
* overall cleanup process.</p>
*
*
* @see #cleanUp()
* @see #cleanUpTemporaryFiles()
*/
protected void cleanUpDiskFileItems() {
LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads");
@@ -299,58 +266,26 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
}
}
/**
* Cleans up temporary files created for in-memory uploads.
*
* <p>This method deletes all temporary files that were created when
* processing in-memory uploads. These files are created in
* {@link #processFileField(DiskFileItem, String)} when an uploaded file is
* stored in memory and needs to be written to disk.</p>
*
* <p>The cleanup process:</p>
* <ol>
* <li>Iterates through all tracked temporary files</li>
* <li>Checks if each file still exists</li>
* <li>Attempts to delete existing files</li>
* <li>Logs warnings for files that cannot be deleted</li>
* </ol>
*
* <p>This method can be overridden by subclasses to customize cleanup
* behavior. All exceptions are caught and logged to ensure cleanup
* continues even if individual file deletions fail.</p>
*
* @see #cleanUp()
* @see #cleanUpDiskFileItems()
*/
protected void cleanUpTemporaryFiles() {
LOG.debug("Cleaning up {} temporary files created for in-memory uploads", temporaryFiles.size());
for (File tempFile : temporaryFiles) {
deleteFile(tempFile.toPath());
}
}
/**
* Performs complete cleanup of all resources associated with this request.
*
*
* <p>This method extends the parent cleanup functionality to ensure proper
* cleanup of Jakarta-specific resources:</p>
* <ol>
* <li>Calls parent cleanup to handle base class resources</li>
* <li>Cleans up all tracked {@link DiskFileItem} instances</li>
* <li>Cleans up all temporary files created for in-memory uploads</li>
* <li>Clears internal tracking collections</li>
* </ol>
*
*
* <p>This method is designed to be safe to call multiple times and will
* not throw exceptions even if cleanup operations fail. All errors are
* logged for debugging purposes.</p>
*
*
* <p><strong>Important:</strong> This method should always be called in a
* finally block to ensure resources are properly released, even if
* exceptions occur during request processing.</p>
*
*
* @see #cleanUpDiskFileItems()
* @see #cleanUpTemporaryFiles()
* @see AbstractMultiPartRequest#cleanUp()
*/
@Override
@@ -358,10 +293,8 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
super.cleanUp();
try {
cleanUpDiskFileItems();
cleanUpTemporaryFiles();
} finally {
diskFileItems.clear();
temporaryFiles.clear();
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java
+213
View File
@@ -0,0 +1,213 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsException;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.Objects;
import java.util.UUID;
/**
* In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept
* in memory ({@code DiskFileItem.isInMemory() == true}).
*
* <p>The content is held as a byte array and is written to a temporary file only the first time a
* caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy
* materialization). Callers reading through {@link #getInputStream()} never touch the disk. The
* temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied
* original filename.</p>
*
* <p><strong>Clustered deployments:</strong> the target temporary path is resolved on the node that
* created this instance. If an un-materialized instance is serialized (for example, session
* replication) and deserialized on another node, a later {@link #getContent()} materializes to that
* originating node's path, which may not exist on the new node. Read via {@link #getInputStream()},
* which never touches disk, when content must survive cross-node replication.</p>
*
* @since 7.3.0
*/
public class StrutsInMemoryUploadedFile implements UploadedFile {
private static final long serialVersionUID = 1L;
private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class);
private final byte[] content;
private final File targetFile;
private final String contentType;
private final String originalName;
private final String inputName;
private transient volatile File materializedFile;
private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType,
String originalName, String inputName) {
// Defensive copy: the instance owns its bytes so callers cannot mutate content after construction.
this.content = Objects.requireNonNull(content, "content").clone();
String name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp";
this.targetFile = saveDir.resolve(name).toFile();
this.contentType = contentType;
this.originalName = originalName;
this.inputName = inputName;
}
private synchronized File materialize() {
if (materializedFile == null) {
try {
// CREATE_NEW fails closed if the target already exists, so we never overwrite or
// follow a pre-planted file/symlink in the upload directory.
Files.write(targetFile.toPath(), content, StandardOpenOption.CREATE_NEW);
} catch (FileAlreadyExistsException e) {
// A file already occupies the target path; do not touch it (possible planted file/symlink).
throw new StrutsException("Refusing to overwrite existing file while materializing in-memory uploaded file: " + targetFile.getName(), e);
} catch (IOException e) {
// Remove a partial file this call may have created before rethrowing.
try {
Files.deleteIfExists(targetFile.toPath());
} catch (IOException suppressed) {
e.addSuppressed(suppressed);
}
throw new StrutsException("Could not materialize in-memory uploaded file: " + targetFile.getName(), e);
}
materializedFile = targetFile;
LOG.debug("Materialized in-memory uploaded item to {}", targetFile.getAbsolutePath());
}
return materializedFile;
}
@Override
public InputStream getInputStream() {
return new ByteArrayInputStream(content);
}
@Override
public boolean isMissing() {
return false;
}
@Override
public Long length() {
return (long) content.length;
}
@Override
public String getName() {
return targetFile.getName();
}
@Override
public boolean isFile() {
File f = materializedFile;
return f != null && f.isFile();
}
@Override
public boolean delete() {
File f = materializedFile;
if (f == null) {
return true;
}
try {
return Files.deleteIfExists(f.toPath());
} catch (IOException e) {
LOG.warn("Could not delete materialized in-memory uploaded file: {}", f.getAbsolutePath(), e);
return false;
}
}
@Override
public String getAbsolutePath() {
return materialize().getAbsolutePath();
}
@Override
public File getContent() {
return materialize();
}
@Override
public String getContentType() {
return contentType;
}
@Override
public String getOriginalName() {
return originalName;
}
@Override
public String getInputName() {
return inputName;
}
@Override
public String toString() {
return "StrutsInMemoryUploadedFile{" +
"contentType='" + contentType + '\'' +
", originalName='" + originalName + '\'' +
", inputName='" + inputName + '\'' +
", size=" + content.length +
'}';
}
public static class Builder {
private final byte[] content;
private final Path saveDir;
private String contentType;
private String originalName;
private String inputName;
private Builder(byte[] content, Path saveDir) {
this.content = content;
this.saveDir = saveDir;
}
public static Builder create(byte[] content, Path saveDir) {
return new Builder(content, saveDir);
}
public Builder withContentType(String contentType) {
this.contentType = contentType;
return this;
}
public Builder withOriginalName(String originalName) {
this.originalName = originalName;
return this;
}
public Builder withInputName(String inputName) {
this.inputName = inputName;
return this;
}
public UploadedFile build() {
return new StrutsInMemoryUploadedFile(content, saveDir, contentType, originalName, inputName);
}
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java
+8
View File
@@ -19,6 +19,9 @@
package org.apache.struts2.dispatcher.multipart;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
public class StrutsUploadedFile implements UploadedFile {
@@ -64,6 +67,11 @@ public class StrutsUploadedFile implements UploadedFile {
return file;
}
@Override
public InputStream getInputStream() throws IOException {
return new FileInputStream(file);
}
@Override
public String getContentType() {
return this.contentType;
core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java
+38
View File
@@ -18,6 +18,11 @@
*/
package org.apache.struts2.dispatcher.multipart;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.Serializable;
/**
@@ -74,4 +79,37 @@ public interface UploadedFile extends Serializable {
*/
String getInputName();
/**
* Streams the uploaded content without forcing it to disk. Implementations backed by
* in-memory bytes can return the bytes directly; file-backed implementations stream the
* file. The default reads whatever {@link #getContent()} exposes.
*
* @return an input stream over the uploaded content
* @throws IOException if the content cannot be read
* @since 7.3.0
*/
default InputStream getInputStream() throws IOException {
Object content = getContent();
if (content instanceof File file) {
return new FileInputStream(file);
}
if (content instanceof byte[] bytes) {
return new ByteArrayInputStream(bytes);
}
throw new IOException("No content stream available for " + getName());
}
/**
* Indicates whether this upload has no content available (for example, the upload failed).
* Implementations that hold their content in memory should override this to answer WITHOUT
* materializing the content to disk. The default reports missing when {@link #getContent()}
* is {@code null}.
*
* @return true if there is no content backing this upload
* @since 7.3.0
*/
default boolean isMissing() {
return getContent() == null;
}
}
core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java
+9 -7
View File
@@ -48,6 +48,13 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor
public static final String STRUTS_MESSAGES_ERROR_UPLOADING_KEY = "struts.messages.error.uploading";
public static final String STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY = "struts.messages.error.file.too.large";
public static final String STRUTS_MESSAGES_INVALID_FILE_KEY = "struts.messages.invalid.file";
/**
* @deprecated since 7.3.0, no longer used. The unreachable content-type null-check in
* {@code acceptFile()} that referenced this key was removed as part of the in-memory upload
* optimization (WW-5413); there is no replacement. This constant will be removed in a future
* version.
*/
@Deprecated(since = "7.3.0", forRemoval = true)
public static final String STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY = "struts.messages.invalid.content.type";
public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed";
public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed";
@@ -114,8 +121,8 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor
validation = validationAware;
}
// If it's null the upload failed
if (file == null || file.getContent() == null) {
// If it's missing the upload failed
if (file == null || file.isMissing()) {
String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOADING_KEY, new String[]{inputName});
if (validation != null) {
validation.addFieldError(inputName, errMsg);
@@ -124,11 +131,6 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor
return false;
}
if (file.getContent() == null) {
String errMsg = getTextMessage(action, STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY, new String[]{originalFilename});
errorMessages.add(errMsg);
LOG.warn(errMsg);
}
if (maximumSize != null && maximumSize < file.length()) {
String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{
inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action)
core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java
+10 -10
View File
@@ -207,8 +207,6 @@ abstract class AbstractMultiPartRequestTest {
.asInstanceOf(InstanceOfAssertFactories.LIST)
.containsOnly("file1", "file2");
assertThat(multiPart.getFile("file1")).allSatisfy(file -> {
assertThat(file.isFile())
.isTrue();
assertThat(file.getOriginalName())
.isEqualTo("test1.csv");
assertThat(file.getContentType())
@@ -220,10 +218,10 @@ abstract class AbstractMultiPartRequestTest {
.exists()
.content()
.isEqualTo("1,2,3,4");
});
assertThat(multiPart.getFile("file2")).allSatisfy(file -> {
assertThat(file.isFile())
.isTrue();
});
assertThat(multiPart.getFile("file2")).allSatisfy(file -> {
assertThat(file.getOriginalName())
.isEqualTo("test2.csv");
assertThat(file.getInputName())
@@ -233,6 +231,8 @@ abstract class AbstractMultiPartRequestTest {
.exists()
.content()
.isEqualTo("5,6,7,8");
assertThat(file.isFile())
.isTrue();
});
}
@@ -258,8 +258,6 @@ abstract class AbstractMultiPartRequestTest {
.asInstanceOf(InstanceOfAssertFactories.LIST)
.containsOnly("file1", "file2");
assertThat(multiPart.getFile("file1")).allSatisfy(file -> {
assertThat(file.isFile())
.isTrue();
assertThat(file.getOriginalName())
.isEqualTo("test1.csv");
assertThat(file.getContentType())
@@ -270,10 +268,10 @@ abstract class AbstractMultiPartRequestTest {
.exists()
.content()
.isEqualTo("1,2,3,4");
});
assertThat(multiPart.getFile("file2")).allSatisfy(file -> {
assertThat(file.isFile())
.isTrue();
});
assertThat(multiPart.getFile("file2")).allSatisfy(file -> {
assertThat(file.getOriginalName())
.isEqualTo("test2.csv");
assertThat(file.getContentType())
@@ -285,6 +283,8 @@ abstract class AbstractMultiPartRequestTest {
.exists()
.content()
.isEqualTo("5,6,7,8");
assertThat(file.isFile())
.isTrue();
});
List<UploadedFile> uploadedFiles = new ArrayList<>();
@@ -431,8 +431,6 @@ abstract class AbstractMultiPartRequestTest {
.asInstanceOf(InstanceOfAssertFactories.LIST)
.containsOnly("file1");
assertThat(multiPart.getFile("file1")).allSatisfy(file -> {
assertThat(file.isFile())
.isTrue();
assertThat(file.getOriginalName())
.isEqualTo("test1.csv");
assertThat(file.getContentType())
@@ -442,6 +440,8 @@ abstract class AbstractMultiPartRequestTest {
.exists()
.content()
.isEqualTo("Ł,Ś,Ż,Ó");
assertThat(file.isFile())
.isTrue();
});
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java
+36 -181
View File
@@ -18,18 +18,15 @@
*/
package org.apache.struts2.dispatcher.multipart;
import org.apache.commons.fileupload2.core.DiskFileItem;
import org.apache.struts2.dispatcher.LocalizedMessage;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.Test;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Field;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
import static org.assertj.core.api.Assertions.assertThat;
@@ -41,119 +38,6 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
return new JakartaMultiPartRequest();
}
@Test
public void temporaryFileCleanupForInMemoryUploads() throws IOException, NoSuchFieldException, IllegalAccessException {
// given - small files that will be in-memory
String content = formFile("file1", "test1.csv", "a,b,c,d") +
formFile("file2", "test2.csv", "1,2,3,4") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
// Access private field to verify temporary files are tracked
Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
tempFilesField.setAccessible(true);
@SuppressWarnings("unchecked")
List<File> temporaryFiles = (List<File>) tempFilesField.get(multiPart);
// Store file paths before cleanup for verification
List<String> tempFilePaths = temporaryFiles.stream()
.map(File::getAbsolutePath)
.toList();
// Verify temporary files exist before cleanup
assertThat(temporaryFiles).isNotEmpty();
for (File tempFile : temporaryFiles) {
assertThat(tempFile).exists();
}
// when - cleanup
multiPart.cleanUp();
// then - verify files are deleted and tracking list is cleared
for (String tempFilePath : tempFilePaths) {
assertThat(new File(tempFilePath)).doesNotExist();
}
assertThat(temporaryFiles).isEmpty();
}
@Test
public void cleanupMethodsCanBeOverridden() {
// Create a custom implementation to test extensibility
class CustomJakartaMultiPartRequest extends JakartaMultiPartRequest {
boolean diskFileItemsCleanedUp = false;
boolean temporaryFilesCleanedUp = false;
@Override
protected void cleanUpDiskFileItems() {
diskFileItemsCleanedUp = true;
super.cleanUpDiskFileItems();
}
@Override
protected void cleanUpTemporaryFiles() {
temporaryFilesCleanedUp = true;
super.cleanUpTemporaryFiles();
}
}
CustomJakartaMultiPartRequest customMultiPart = new CustomJakartaMultiPartRequest();
// when
customMultiPart.cleanUp();
// then
assertThat(customMultiPart.diskFileItemsCleanedUp).isTrue();
assertThat(customMultiPart.temporaryFilesCleanedUp).isTrue();
}
@Test
public void temporaryFileCreationFailureAddsError() throws IOException {
// Create a custom implementation that simulates temp file creation failure
class FaultyJakartaMultiPartRequest extends JakartaMultiPartRequest {
@Override
protected void processFileField(DiskFileItem item, String saveDir) {
// Simulate in-memory upload that fails to create temp file
if (item.isInMemory()) {
try {
// Simulate IOException during temp file creation
throw new IOException("Simulated temp file creation failure");
} catch (IOException e) {
// Add the error to the errors list for proper user feedback
LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(),
new Object[]{item.getName()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
}
} else {
super.processFileField(item, saveDir);
}
}
}
FaultyJakartaMultiPartRequest faultyMultiPart = new FaultyJakartaMultiPartRequest();
// given - small file that would normally be in-memory
String content = formFile("file1", "test1.csv", "a,b") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
faultyMultiPart.parse(mockRequest, tempDir);
// then - verify error is properly captured
assertThat(faultyMultiPart.getErrors())
.hasSize(1)
.first()
.extracting(LocalizedMessage::getTextKey)
.isEqualTo("struts.messages.upload.error.IOException");
}
@Test
public void temporaryFileCreationErrorsAreNotDuplicated() throws IOException {
// Test that duplicate errors are not added to the errors list
@@ -222,66 +106,6 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
assertThat(multiPart.parameters).isEmpty();
}
@Test
public void temporaryFilesCreatedInSaveDirectory() throws IOException, NoSuchFieldException, IllegalAccessException {
// Test that temporary files for in-memory uploads are created in the saveDir, not system temp
String content = formFile("file1", "test1.csv", "small,content") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
// Access private field to get temporary files
Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
tempFilesField.setAccessible(true);
@SuppressWarnings("unchecked")
List<File> temporaryFiles = (List<File>) tempFilesField.get(multiPart);
// then - verify temporary files are created in saveDir
assertThat(temporaryFiles).isNotEmpty();
for (File tempFile : temporaryFiles) {
// Verify the temporary file is in the saveDir, not system temp
assertThat(tempFile.getParent()).isEqualTo(tempDir);
assertThat(tempFile.getName()).startsWith("upload_");
assertThat(tempFile.getName()).endsWith(".tmp");
assertThat(tempFile).exists();
}
}
@Test
public void secureTemporaryFileNaming() throws IOException, NoSuchFieldException, IllegalAccessException {
// Test that temporary files use UUID-based naming for security
String content = formFile("file1", "malicious../../../etc/passwd", "content") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
// Access private field to get temporary files
Field tempFilesField = JakartaMultiPartRequest.class.getDeclaredField("temporaryFiles");
tempFilesField.setAccessible(true);
@SuppressWarnings("unchecked")
List<File> temporaryFiles = (List<File>) tempFilesField.get(multiPart);
// then - verify secure naming prevents directory traversal
assertThat(temporaryFiles).isNotEmpty();
for (File tempFile : temporaryFiles) {
// Verify the temporary file uses secure UUID naming
assertThat(tempFile.getName()).startsWith("upload_");
assertThat(tempFile.getName()).endsWith(".tmp");
// Verify it doesn't contain malicious path elements
assertThat(tempFile.getName()).doesNotContain("..");
assertThat(tempFile.getName()).doesNotContain("/");
assertThat(tempFile.getName()).doesNotContain("\\");
// Verify it's in the correct directory
assertThat(tempFile.getParent()).isEqualTo(tempDir);
}
}
@Test
public void processNormalFormFieldHandlesNullFieldName() throws IOException {
// Test null field name handling in processNormalFormField
@@ -424,7 +248,7 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
@Test
public void processFileFieldHandlesEmptyFileName() throws IOException {
String content =
String content =
endline + "--" + boundary + endline +
"Content-Disposition: form-data; name=\"emptyfile\"; filename=\"\"" + endline +
"Content-Type: text/plain" + endline +
@@ -436,12 +260,12 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
endline +
"valid file content" +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
// then - should only process the file with valid filename
assertThat(multiPart.getErrors()).isEmpty();
assertThat(multiPart.uploadedFiles).hasSize(1);
@@ -453,4 +277,35 @@ public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
.isEqualTo("valid file content");
}
@Test
public void inMemoryUploadIsNotWrittenToDiskUntilContentRequested() throws IOException {
// given - a small file that Commons FileUpload keeps in memory
String content = formFile("file1", "test1.csv", "a,b,c,d") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
UploadedFile file = multiPart.getFile("file1")[0];
// then - nothing written to disk right after parse
assertThat(file.isFile()).isFalse();
// and - content is readable via the stream path without materializing
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), StandardCharsets.UTF_8)).isEqualTo("a,b,c,d");
}
assertThat(file.isFile()).isFalse();
// and - getContent() materializes a real file on demand
File materialized = (File) file.getContent();
assertThat(materialized).exists().hasContent("a,b,c,d");
assertThat(file.isFile()).isTrue();
// and - cleanUp removes the materialized file
multiPart.cleanUp();
assertThat(materialized).doesNotExist();
}
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java
+217
View File
@@ -0,0 +1,217 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.nio.file.Path;
import org.apache.struts2.StrutsException;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
public class StrutsInMemoryUploadedFileTest {
@Rule
public TemporaryFolder tempFolder = new TemporaryFolder();
private Path saveDir;
@Before
public void setUp() {
saveDir = tempFolder.getRoot().toPath();
}
private UploadedFile build(byte[] content) {
return StrutsInMemoryUploadedFile.Builder
.create(content, saveDir)
.withOriginalName("orig.txt")
.withContentType("text/plain")
.withInputName("file")
.build();
}
@Test
public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException {
UploadedFile file = build("hello".getBytes(UTF_8));
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello");
}
assertThat(file.isFile()).isFalse();
assertThat(tempFolder.getRoot()).isEmptyDirectory();
}
@Test
public void getContentThrowsStrutsExceptionAndLeavesNoFileWhenWriteFails() {
// save directory does not exist -> Files.write in materialize() fails
Path missingDir = tempFolder.getRoot().toPath().resolve("no-such-dir");
UploadedFile file = StrutsInMemoryUploadedFile.Builder
.create("x".getBytes(UTF_8), missingDir)
.withOriginalName("orig.txt")
.withContentType("text/plain")
.withInputName("file")
.build();
assertThatThrownBy(file::getContent).isInstanceOf(StrutsException.class);
assertThat(file.isFile()).isFalse(); // not marked materialized
assertThat(missingDir.toFile()).doesNotExist(); // no partial file left behind
assertThat(tempFolder.getRoot()).isEmptyDirectory(); // nothing leaked into the save root
}
@Test
public void getContentFailsClosedWhenTargetAlreadyExists() throws IOException {
UploadedFile file = build("x".getBytes(UTF_8));
// Pre-plant a file at the exact target name (simulates a collision or planted file/symlink).
File planted = new File(tempFolder.getRoot(), file.getName());
java.nio.file.Files.writeString(planted.toPath(), "pre-existing");
assertThatThrownBy(file::getContent).isInstanceOf(StrutsException.class);
// The pre-existing file must be neither overwritten nor deleted.
assertThat(planted).exists();
assertThat(java.nio.file.Files.readString(planted.toPath())).isEqualTo("pre-existing");
assertThat(file.isFile()).isFalse();
}
@Test
public void getContentMaterializesFileExactlyOnce() {
UploadedFile file = build("data".getBytes(UTF_8));
File first = (File) file.getContent();
File second = (File) file.getContent();
assertThat(first).exists().hasContent("data");
assertThat(second).isSameAs(first);
assertThat(tempFolder.getRoot().listFiles()).hasSize(1);
}
@Test
public void getAbsolutePathMaterializesFile() {
UploadedFile file = build("data".getBytes(UTF_8));
String path = file.getAbsolutePath();
assertThat(new File(path)).exists().hasContent("data");
assertThat(file.isFile()).isTrue();
}
@Test
public void isFileFalseBeforeMaterializationTrueAfter() {
UploadedFile file = build("x".getBytes(UTF_8));
assertThat(file.isFile()).isFalse();
file.getContent();
assertThat(file.isFile()).isTrue();
}
@Test
public void lengthAndMetadataDoNotMaterialize() {
UploadedFile file = build("abcd".getBytes(UTF_8));
assertThat(file.length()).isEqualTo(4L);
assertThat(file.getContentType()).isEqualTo("text/plain");
assertThat(file.getOriginalName()).isEqualTo("orig.txt");
assertThat(file.getInputName()).isEqualTo("file");
assertThat(file.getName()).startsWith("upload_").endsWith(".tmp");
assertThat(file.isFile()).isFalse();
assertThat(tempFolder.getRoot()).isEmptyDirectory();
}
@Test
public void deleteRemovesMaterializedFile() {
UploadedFile file = build("x".getBytes(UTF_8));
File materialized = (File) file.getContent();
assertThat(materialized).exists();
assertThat(file.delete()).isTrue();
assertThat(materialized).doesNotExist();
}
@Test
public void deleteIsNoOpWhenNotMaterialized() {
UploadedFile file = build("x".getBytes(UTF_8));
assertThat(file.delete()).isTrue();
assertThat(tempFolder.getRoot()).isEmptyDirectory();
}
@Test
public void maliciousOriginalNameDoesNotLeakIntoTempName() {
UploadedFile file = StrutsInMemoryUploadedFile.Builder
.create("x".getBytes(UTF_8), saveDir)
.withOriginalName("../../etc/passwd")
.build();
file.getContent(); // materialize
assertThat(file.getName()).startsWith("upload_").endsWith(".tmp");
assertThat(file.getName()).doesNotContain("..").doesNotContain("/").doesNotContain("\\");
assertThat(new File(saveDir.toFile(), file.getName())).exists();
}
@Test
public void isMissingIsFalseWithoutMaterializing() {
UploadedFile file = build("x".getBytes(UTF_8));
assertThat(file.isMissing()).isFalse();
assertThat(file.isFile()).isFalse(); // did not materialize
assertThat(tempFolder.getRoot()).isEmptyDirectory();
}
@Test
public void isSerializableWhenNotMaterialized() throws IOException, ClassNotFoundException {
UploadedFile file = build("payload".getBytes(UTF_8));
byte[] bytes;
try (ByteArrayOutputStream bos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(bos)) {
oos.writeObject(file);
bytes = bos.toByteArray();
}
UploadedFile restored;
try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(bytes))) {
restored = (UploadedFile) ois.readObject();
}
assertThat(restored.length()).isEqualTo(7L);
assertThat(restored.getContentType()).isEqualTo("text/plain");
assertThat(restored.getName()).startsWith("upload_").endsWith(".tmp");
try (InputStream in = restored.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("payload");
}
}
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java
+49
View File
@@ -0,0 +1,49 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
public class StrutsUploadedFileTest {
@Rule
public TemporaryFolder tempFolder = new TemporaryFolder();
@Test
public void getInputStreamReadsFileContent() throws IOException {
File backing = tempFolder.newFile("upload_test.tmp");
Files.writeString(backing.toPath(), "hello");
UploadedFile file = StrutsUploadedFile.Builder.create(backing).build();
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello");
}
}
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java
+91
View File
@@ -0,0 +1,91 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Test;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
public class UploadedFileTest {
@Test
public void defaultGetInputStreamReadsByteArrayContent() throws IOException {
UploadedFile file = new UploadedFile() {
public Long length() { return 3L; }
public String getName() { return "x"; }
public String getOriginalName() { return "x"; }
public boolean isFile() { return false; }
public boolean delete() { return true; }
public String getAbsolutePath() { return null; }
public Object getContent() { return "abc".getBytes(UTF_8); }
public String getContentType() { return "text/plain"; }
public String getInputName() { return "file"; }
};
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("abc");
}
}
@Test
public void defaultGetInputStreamReadsFileContent() throws IOException {
File f = File.createTempFile("upload_", ".tmp");
try {
Files.writeString(f.toPath(), "hi");
try (InputStream in = uploadedFileReturning(f).getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hi");
}
} finally {
assertThat(f.delete()).isTrue();
}
}
@Test
public void defaultGetInputStreamThrowsWhenNoContent() {
assertThatThrownBy(uploadedFileReturning(null)::getInputStream)
.isInstanceOf(IOException.class);
}
@Test
public void defaultIsMissingReflectsContent() {
assertThat(uploadedFileReturning("abc".getBytes(UTF_8)).isMissing()).isFalse();
assertThat(uploadedFileReturning(null).isMissing()).isTrue();
}
private static UploadedFile uploadedFileReturning(Object content) {
return new UploadedFile() {
public Long length() { return 0L; }
public String getName() { return "x"; }
public String getOriginalName() { return "x"; }
public boolean isFile() { return false; }
public boolean delete() { return true; }
public String getAbsolutePath() { return null; }
public Object getContent() { return content; }
public String getContentType() { return "text/plain"; }
public String getInputName() { return "file"; }
};
}
}
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+40
View File
@@ -27,6 +27,7 @@ import org.apache.struts2.ValidationAwareSupport;
import org.apache.struts2.action.UploadedFilesAware;
import org.apache.struts2.dispatcher.multipart.JakartaMultiPartRequest;
import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper;
import org.apache.struts2.dispatcher.multipart.StrutsInMemoryUploadedFile;
import org.apache.struts2.dispatcher.multipart.StrutsUploadedFile;
import org.apache.struts2.dispatcher.multipart.UploadedFile;
import org.apache.struts2.locale.DefaultLocaleProvider;
@@ -171,6 +172,45 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase {
.contains("inputName");
}
public void testAcceptFileDoesNotMaterializeInMemoryUpload() {
interceptor.setAllowedTypes("text/plain");
ValidationAwareSupport validation = new ValidationAwareSupport();
UploadedFile file = StrutsInMemoryUploadedFile.Builder
.create("hello".getBytes(StandardCharsets.UTF_8), tempDir.toPath())
.withContentType("text/plain")
.withOriginalName("f.txt")
.withInputName("inputName")
.build();
boolean ok = interceptor.acceptFile(validation, file, "f.txt", "text/plain", "inputName");
assertThat(ok).isTrue();
assertThat(validation.hasErrors()).isFalse();
// The optimization: validation must NOT have written the in-memory upload to disk.
assertThat(file.isFile()).isFalse();
}
public void testRejectedInMemoryUploadIsStillNotMaterialized() {
interceptor.setAllowedTypes("text/plain");
ValidationAwareSupport validation = new ValidationAwareSupport();
UploadedFile file = StrutsInMemoryUploadedFile.Builder
.create("hello".getBytes(StandardCharsets.UTF_8), tempDir.toPath())
.withContentType("text/html")
.withOriginalName("f.html")
.withInputName("inputName")
.build();
// wrong content type -> rejected
boolean ok = interceptor.acceptFile(validation, file, "f.html", "text/html", "inputName");
assertThat(ok).isFalse();
assertThat(validation.hasErrors()).isTrue();
// Even on rejection, no disk write happened.
assertThat(file.isFile()).isFalse();
}
public void testAcceptFileWithMaxSize() throws Exception {
interceptor.setMaximumSize(10L);
docs/superpowers/plans/2026-07-22-WW-5413-inmemory-upload-optimization.md
+827
View File
@@ -0,0 +1,827 @@
# WW-5413 In-memory Multipart Upload Optimization — Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Stop eagerly writing small (in-memory) multipart uploads to a temporary file; keep them in memory and materialize a file only when a caller demands one, while `getContent()` still returns a `java.io.File` for full backward compatibility.
**Architecture:** Add a `default InputStream getInputStream()` to the `UploadedFile` interface (the type-safe, no-disk read path). Introduce `StrutsInMemoryUploadedFile`, a byte-array-backed `UploadedFile` that lazily writes a temp file only on `getContent()`/`getAbsolutePath()`. Rewire `JakartaMultiPartRequest.processFileField()` to build it for `item.isInMemory()`, and drop the now-obsolete eager-write / `temporaryFiles` bookkeeping (cleanup rides the existing `AbstractMultiPartRequest.cleanUp()` `delete()` loop).
**Tech Stack:** Java (Struts core module), Apache Commons FileUpload2 2.0.0-M5, JUnit 4 + AssertJ, Maven.
## Global Constraints
- **Commit prefix:** every commit message MUST start with `WW-5413` followed by a conventional type, e.g. `WW-5413 feat(core): ...`.
- **`getContent()` runtime type MUST remain `java.io.File`** for every `UploadedFile` implementation — never return `byte[]` from it. The bytes-without-a-file path is `getInputStream()` only.
- **Core tests are JUnit 4** (`org.junit.Test`, AssertJ). Do NOT use JUnit 5. New standalone test classes must NOT extend `XWorkTestCase` (irrelevant here) — plain JUnit 4 classes run fine under Surefire.
- **Secure temp-file naming:** materialized files MUST use the pattern `upload_<uuid>.tmp` in the provided save directory; the user-supplied original filename MUST NOT influence the on-disk name.
- **Backward compatibility:** `UploadedFile.getInputStream()` MUST be a `default` method so third-party implementations keep compiling.
- **Build/test command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` (single test) or `-Dtest=ClassName` (whole class).
---
## File Structure
- `core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java` — **modify**: add `default InputStream getInputStream()`.
- `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java` — **modify**: override `getInputStream()` to stream the backing file.
- `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java` — **create**: byte-array-backed, lazily-materializing `UploadedFile`.
- `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java` — **modify**: build `StrutsInMemoryUploadedFile` for in-memory items; remove eager write, `temporaryFiles`, `cleanUpTemporaryFiles()`.
- `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java` — **create**: interface default-method test.
- `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java` — **create**: `getInputStream()` override test.
- `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java` — **create**: full lazy-materialization unit tests.
- `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java` — **modify**: remove 5 tests that reference removed internals; add integration tests for the new behavior.
---
## Task 1: Streaming accessor on the File-backed path
Adds `getInputStream()` to the interface (default) and overrides it in the existing `File`-backed implementation.
**Files:**
- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java`
- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java`
- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java` (create)
- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java` (create)
**Interfaces:**
- Produces: `UploadedFile.getInputStream() throws IOException` returning an `InputStream` over the file's bytes. Default impl: `File` content → `FileInputStream`; `byte[]` content → `ByteArrayInputStream`; otherwise `IOException`.
- [ ] **Step 1: Write the failing interface default-method test**
Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java`:
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Test;
import java.io.IOException;
import java.io.InputStream;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
public class UploadedFileTest {
@Test
public void defaultGetInputStreamReadsByteArrayContent() throws IOException {
UploadedFile file = new UploadedFile() {
public Long length() { return 3L; }
public String getName() { return "x"; }
public String getOriginalName() { return "x"; }
public boolean isFile() { return false; }
public boolean delete() { return true; }
public String getAbsolutePath() { return null; }
public Object getContent() { return "abc".getBytes(UTF_8); }
public String getContentType() { return "text/plain"; }
public String getInputName() { return "file"; }
};
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("abc");
}
}
}
```
- [ ] **Step 2: Run test to verify it fails**
Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest`
Expected: COMPILE FAILURE — `getInputStream()` is not defined on `UploadedFile`.
- [ ] **Step 3: Add the default method to the interface**
In `UploadedFile.java`, add these imports below `import java.io.Serializable;`:
```java
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
```
Add this method inside the interface (e.g. after `getInputName()`):
```java
/**
* Streams the uploaded content without forcing it to disk. Implementations backed by
* in-memory bytes can return the bytes directly; file-backed implementations stream the
* file. The default reads whatever {@link #getContent()} exposes.
*
* @return an input stream over the uploaded content
* @throws IOException if the content cannot be read
* @since 7.3.0
*/
default InputStream getInputStream() throws IOException {
Object content = getContent();
if (content instanceof File file) {
return new FileInputStream(file);
}
if (content instanceof byte[] bytes) {
return new ByteArrayInputStream(bytes);
}
throw new IOException("No content stream available for " + getName());
}
```
- [ ] **Step 4: Run test to verify it passes**
Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest`
Expected: PASS.
- [ ] **Step 5: Write the failing StrutsUploadedFile override test**
Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java`:
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
public class StrutsUploadedFileTest {
@Rule
public TemporaryFolder tempFolder = new TemporaryFolder();
@Test
public void getInputStreamReadsFileContent() throws IOException {
File backing = tempFolder.newFile("upload_test.tmp");
Files.writeString(backing.toPath(), "hello");
UploadedFile file = StrutsUploadedFile.Builder.create(backing).build();
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello");
}
}
}
```
- [ ] **Step 6: Run test to verify it passes via the interface default**
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsUploadedFileTest`
Expected: PASS (the default method already handles the `File` branch).
- [ ] **Step 7: Add an explicit override in StrutsUploadedFile**
In `StrutsUploadedFile.java`, replace the import block:
```java
import java.io.File;
```
with:
```java
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
```
Add this method (e.g. after `getContent()`):
```java
@Override
public InputStream getInputStream() throws IOException {
return new FileInputStream(file);
}
```
- [ ] **Step 8: Run both tests to verify they pass**
Run: `mvn test -DskipAssembly -pl core -Dtest=UploadedFileTest,StrutsUploadedFileTest`
Expected: PASS.
- [ ] **Step 9: Commit**
```bash
git add core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java \
core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java \
core/src/test/java/org/apache/struts2/dispatcher/multipart/UploadedFileTest.java \
core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFileTest.java
git commit -m "WW-5413 feat(core): add UploadedFile.getInputStream() streaming accessor"
```
---
## Task 2: `StrutsInMemoryUploadedFile` (lazy materialization)
The byte-array-backed implementation. This is the core of the optimization.
**Files:**
- Create: `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java`
- Test: `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java` (create)
**Interfaces:**
- Consumes: `UploadedFile` (incl. `getInputStream()` from Task 1), `org.apache.struts2.StrutsException`.
- Produces:
- `StrutsInMemoryUploadedFile.Builder.create(byte[] content, java.nio.file.Path saveDir)` → `Builder`
- `Builder.withContentType(String).withOriginalName(String).withInputName(String).build()` → `UploadedFile`
- Behavior: `getInputStream()` → `ByteArrayInputStream` (no disk); `getContent()`/`getAbsolutePath()` write once to `saveDir/upload_<uuid>.tmp` and cache; `isFile()` false until materialized; `getName()` returns the pre-chosen `upload_<uuid>.tmp`; `delete()` removes the materialized file (no-op, returns `true`, if never materialized).
- [ ] **Step 1: Write the failing unit tests**
Create `core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java`:
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Path;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;
public class StrutsInMemoryUploadedFileTest {
@Rule
public TemporaryFolder tempFolder = new TemporaryFolder();
private Path saveDir;
@Before
public void setUp() {
saveDir = tempFolder.getRoot().toPath();
}
private UploadedFile build(byte[] content) {
return StrutsInMemoryUploadedFile.Builder
.create(content, saveDir)
.withOriginalName("orig.txt")
.withContentType("text/plain")
.withInputName("file")
.build();
}
@Test
public void getInputStreamReturnsBytesWithoutWritingFile() throws IOException {
UploadedFile file = build("hello".getBytes(UTF_8));
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), UTF_8)).isEqualTo("hello");
}
assertThat(file.isFile()).isFalse();
assertThat(tempFolder.getRoot().listFiles()).isEmpty();
}
@Test
public void getContentMaterializesFileExactlyOnce() {
UploadedFile file = build("data".getBytes(UTF_8));
File first = (File) file.getContent();
File second = (File) file.getContent();
assertThat(first).exists().hasContent("data");
assertThat(second).isSameAs(first);
assertThat(tempFolder.getRoot().listFiles()).hasSize(1);
}
@Test
public void getAbsolutePathMaterializesFile() {
UploadedFile file = build("data".getBytes(UTF_8));
String path = file.getAbsolutePath();
assertThat(new File(path)).exists().hasContent("data");
assertThat(file.isFile()).isTrue();
}
@Test
public void isFileFalseBeforeMaterializationTrueAfter() {
UploadedFile file = build("x".getBytes(UTF_8));
assertThat(file.isFile()).isFalse();
file.getContent();
assertThat(file.isFile()).isTrue();
}
@Test
public void lengthAndMetadataDoNotMaterialize() {
UploadedFile file = build("abcd".getBytes(UTF_8));
assertThat(file.length()).isEqualTo(4L);
assertThat(file.getContentType()).isEqualTo("text/plain");
assertThat(file.getOriginalName()).isEqualTo("orig.txt");
assertThat(file.getInputName()).isEqualTo("file");
assertThat(file.getName()).startsWith("upload_").endsWith(".tmp");
assertThat(file.isFile()).isFalse();
assertThat(tempFolder.getRoot().listFiles()).isEmpty();
}
@Test
public void deleteRemovesMaterializedFile() {
UploadedFile file = build("x".getBytes(UTF_8));
File materialized = (File) file.getContent();
assertThat(materialized).exists();
assertThat(file.delete()).isTrue();
assertThat(materialized).doesNotExist();
}
@Test
public void deleteIsNoOpWhenNotMaterialized() {
UploadedFile file = build("x".getBytes(UTF_8));
assertThat(file.delete()).isTrue();
assertThat(tempFolder.getRoot().listFiles()).isEmpty();
}
@Test
public void maliciousOriginalNameDoesNotLeakIntoTempName() {
UploadedFile file = StrutsInMemoryUploadedFile.Builder
.create("x".getBytes(UTF_8), saveDir)
.withOriginalName("../../etc/passwd")
.build();
file.getContent(); // materialize
assertThat(file.getName()).startsWith("upload_").endsWith(".tmp");
assertThat(file.getName()).doesNotContain("..").doesNotContain("/").doesNotContain("\\");
assertThat(new File(saveDir.toFile(), file.getName())).exists();
}
}
```
- [ ] **Step 2: Run tests to verify they fail**
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsInMemoryUploadedFileTest`
Expected: COMPILE FAILURE — `StrutsInMemoryUploadedFile` does not exist.
- [ ] **Step 3: Implement `StrutsInMemoryUploadedFile`**
Create `core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java`:
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.struts2.dispatcher.multipart;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsException;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.UUID;
/**
* In-memory backed {@link UploadedFile} for small multipart uploads that Commons FileUpload kept
* in memory ({@code DiskFileItem.isInMemory() == true}).
*
* <p>The content is held as a byte array and is written to a temporary file only the first time a
* caller demands a {@link File} through {@link #getContent()} or {@link #getAbsolutePath()} (lazy
* materialization). Callers reading through {@link #getInputStream()} never touch the disk. The
* temporary file uses the secure {@code upload_<uuid>.tmp} naming and ignores the user-supplied
* original filename.</p>
*
* @since 7.3.0
*/
public class StrutsInMemoryUploadedFile implements UploadedFile {
private static final Logger LOG = LogManager.getLogger(StrutsInMemoryUploadedFile.class);
private final byte[] content;
private final Path saveDir;
private final String name;
private final String contentType;
private final String originalName;
private final String inputName;
private transient File materializedFile;
private StrutsInMemoryUploadedFile(byte[] content, Path saveDir, String contentType,
String originalName, String inputName) {
this.content = content;
this.saveDir = saveDir;
this.contentType = contentType;
this.originalName = originalName;
this.inputName = inputName;
this.name = "upload_" + UUID.randomUUID().toString().replace("-", "_") + ".tmp";
}
private synchronized File materialize() {
if (materializedFile == null) {
File target = saveDir.resolve(name).toFile();
try {
Files.write(target.toPath(), content);
} catch (IOException e) {
throw new StrutsException("Could not materialize in-memory uploaded file: " + name, e);
}
materializedFile = target;
LOG.debug("Materialized in-memory uploaded item to {}", target.getAbsolutePath());
}
return materializedFile;
}
@Override
public InputStream getInputStream() {
return new ByteArrayInputStream(content);
}
@Override
public Long length() {
return (long) content.length;
}
@Override
public String getName() {
return name;
}
@Override
public boolean isFile() {
return materializedFile != null && materializedFile.isFile();
}
@Override
public boolean delete() {
if (materializedFile != null) {
return materializedFile.delete();
}
return true;
}
@Override
public String getAbsolutePath() {
return materialize().getAbsolutePath();
}
@Override
public File getContent() {
return materialize();
}
@Override
public String getContentType() {
return contentType;
}
@Override
public String getOriginalName() {
return originalName;
}
@Override
public String getInputName() {
return inputName;
}
@Override
public String toString() {
return "StrutsInMemoryUploadedFile{" +
"contentType='" + contentType + '\'' +
", originalName='" + originalName + '\'' +
", inputName='" + inputName + '\'' +
", size=" + content.length +
'}';
}
public static class Builder {
private final byte[] content;
private final Path saveDir;
private String contentType;
private String originalName;
private String inputName;
private Builder(byte[] content, Path saveDir) {
this.content = content;
this.saveDir = saveDir;
}
public static Builder create(byte[] content, Path saveDir) {
return new Builder(content, saveDir);
}
public Builder withContentType(String contentType) {
this.contentType = contentType;
return this;
}
public Builder withOriginalName(String originalName) {
this.originalName = originalName;
return this;
}
public Builder withInputName(String inputName) {
this.inputName = inputName;
return this;
}
public UploadedFile build() {
return new StrutsInMemoryUploadedFile(content, saveDir, contentType, originalName, inputName);
}
}
}
```
> Note: `org.apache.struts2.StrutsException` is the same exception class used in `AbstractMultiPartRequest`. Confirm the import resolves; it is an unchecked `RuntimeException`.
- [ ] **Step 4: Run tests to verify they pass**
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsInMemoryUploadedFileTest`
Expected: PASS (all 8 tests).
- [ ] **Step 5: Commit**
```bash
git add core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFile.java \
core/src/test/java/org/apache/struts2/dispatcher/multipart/StrutsInMemoryUploadedFileTest.java
git commit -m "WW-5413 feat(core): add lazily-materializing StrutsInMemoryUploadedFile"
```
---
## Task 3: Rewire `JakartaMultiPartRequest` and clean up obsolete internals
Switch the in-memory branch to `StrutsInMemoryUploadedFile`, remove the eager write and `temporaryFiles` bookkeeping, and update the existing tests that reference the removed internals so the suite compiles and passes.
**Files:**
- Modify: `core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java`
- Modify: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java`
**Interfaces:**
- Consumes: `StrutsInMemoryUploadedFile.Builder` (Task 2), existing `StrutsUploadedFile.Builder`.
- Produces: `JakartaMultiPartRequest.processFileField(DiskFileItem, String)` no longer creates temp files eagerly; the `temporaryFiles` field and `cleanUpTemporaryFiles()` method are removed.
- [ ] **Step 1: Replace the in-memory branch in `processFileField`**
In `JakartaMultiPartRequest.java`, replace the entire `if (item.isInMemory()) { ... } else { ... }` block (the eager `FileOutputStream` write path) with:
```java
if (item.isInMemory()) {
LOG.debug(() -> "Keeping in-memory uploaded item without writing to disk: " + normalizeSpace(item.getFieldName()));
UploadedFile uploadedFile = StrutsInMemoryUploadedFile.Builder
.create(item.get(), Path.of(saveDir))
.withOriginalName(item.getName())
.withContentType(item.getContentType())
.withInputName(item.getFieldName())
.build();
values.add(uploadedFile);
} else {
UploadedFile uploadedFile = StrutsUploadedFile.Builder
.create(item.getPath().toFile())
.withOriginalName(item.getName())
.withContentType(item.getContentType())
.withInputName(item.getFieldName())
.build();
values.add(uploadedFile);
}
```
- [ ] **Step 2: Remove the `temporaryFiles` field**
Delete this field and its Javadoc (the block around lines 83–86):
```java
/**
* List to track temporary files created for in-memory uploads
*/
private final List<File> temporaryFiles = new ArrayList<>();
```
- [ ] **Step 3: Remove `cleanUpTemporaryFiles()` and its call**
Delete the entire `cleanUpTemporaryFiles()` method (its Javadoc + body). In `cleanUp()`, remove the `cleanUpTemporaryFiles();` line and the `temporaryFiles.clear();` line, leaving:
```java
@Override
public void cleanUp() {
super.cleanUp();
try {
cleanUpDiskFileItems();
} finally {
diskFileItems.clear();
}
}
```
- [ ] **Step 4: Fix imports**
Remove `import java.io.File;` (no longer used in this class). Leave `java.nio.file.Path` (used by `Path.of(saveDir)`). If the compiler reports any other now-unused import (e.g. `java.nio.file.Files`), remove it too. Keep `StringUtils` (still used by `processNormalFormField`).
- [ ] **Step 5: Remove the 5 obsolete tests**
In `JakartaMultiPartRequestTest.java`, delete these test methods entirely — they reference the removed `temporaryFiles` field / `cleanUpTemporaryFiles()` method, or assert the removed parse-time eager-write behavior. Their coverage is replaced by Task 2 (secure naming, materialization) and Task 4 (no eager write, cleanup):
- `temporaryFileCleanupForInMemoryUploads`
- `cleanupMethodsCanBeOverridden`
- `temporaryFileCreationFailureAddsError`
- `temporaryFilesCreatedInSaveDirectory`
- `secureTemporaryFileNaming`
Leave all other tests untouched (`temporaryFileCreationErrorsAreNotDuplicated`, `cleanupIsIdempotent`, `endToEndMultipartProcessingWithCleanup`, `inMemoryVsDiskFileHandling`, `processNormalFormFieldHandlesNullFieldName`, `processFileFieldHandlesNullFieldName`, `diskFileItemCleanupCoverage`, `errorDuplicationPrevention`, `processFileFieldHandlesEmptyFileName`). After deleting, remove any imports left unused by the deletions (e.g. `java.lang.reflect.Field` if no longer referenced).
- [ ] **Step 6: Run the whole class to verify it compiles and passes**
Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest`
Expected: PASS. In particular `inMemoryVsDiskFileHandling` still passes — its small-file assertion goes through `getContent()`, which now materializes the file on demand and returns a `File`.
- [ ] **Step 7: Commit**
```bash
git add core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java \
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java
git commit -m "WW-5413 refactor(core): drop eager temp-file write for in-memory uploads"
```
---
## Task 4: Integration tests for the deferred-write behavior
Prove end-to-end that in-memory uploads are not written to disk until content is demanded, are readable via the stream path without a write, and are cleaned up after materialization.
**Files:**
- Modify: `core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java`
**Interfaces:**
- Consumes: the parse pipeline from Task 3, `UploadedFile.getInputStream()` from Task 1.
- [ ] **Step 1: Add the imports the new test needs**
In `JakartaMultiPartRequestTest.java`, ensure these imports are present (add any missing):
```java
import java.io.InputStream;
```
(`java.io.File`, `java.nio.charset.StandardCharsets`, and `static org.assertj.core.api.Assertions.assertThat` are already imported.)
- [ ] **Step 2: Write the failing integration test**
Add this test method to `JakartaMultiPartRequestTest`:
```java
@Test
public void inMemoryUploadIsNotWrittenToDiskUntilContentRequested() throws IOException {
// given - a small file that Commons FileUpload keeps in memory
String content = formFile("file1", "test1.csv", "a,b,c,d") +
endline + "--" + boundary + "--";
mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
// when
multiPart.parse(mockRequest, tempDir);
UploadedFile file = multiPart.getFile("file1")[0];
// then - nothing written to disk right after parse
assertThat(file.isFile()).isFalse();
// and - content is readable via the stream path without materializing
try (InputStream in = file.getInputStream()) {
assertThat(new String(in.readAllBytes(), StandardCharsets.UTF_8)).isEqualTo("a,b,c,d");
}
assertThat(file.isFile()).isFalse();
// and - getContent() materializes a real file on demand
File materialized = (File) file.getContent();
assertThat(materialized).exists().hasContent("a,b,c,d");
assertThat(file.isFile()).isTrue();
// and - cleanUp removes the materialized file
multiPart.cleanUp();
assertThat(materialized).doesNotExist();
}
```
- [ ] **Step 3: Run it to verify it passes**
Run: `mvn test -DskipAssembly -pl core -Dtest=JakartaMultiPartRequestTest#inMemoryUploadIsNotWrittenToDiskUntilContentRequested`
Expected: PASS.
- [ ] **Step 4: Run the full multipart test package as a regression check**
Run: `mvn test -DskipAssembly -pl core -Dtest='org.apache.struts2.dispatcher.multipart.*'`
Expected: PASS (all multipart tests, including `JakartaStreamMultiPartRequestTest` and `AbstractMultiPartRequestApiCheckTest`).
- [ ] **Step 5: Commit**
```bash
git add core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java
git commit -m "WW-5413 test(core): cover deferred-write behavior for in-memory uploads"
```
---
## Final verification
- [ ] **Run the core module's dispatcher tests** to catch any converter/interceptor fallout from the interface change:
Run: `mvn test -DskipAssembly -pl core -Dtest='org.apache.struts2.dispatcher.**,org.apache.struts2.interceptor.**,org.apache.struts2.conversion.**'`
Expected: PASS. Pay attention to `UploadedFileConverter`-related tests (legacy `File`-typed action property path) — a small upload now reaches the converter as a `StrutsInMemoryUploadedFile` whose `getContent()` returns a materialized `File`, so conversion must still succeed.
---
## Notes on the spec's error-handling trade-off (§4)
The spec deliberately accepts that a materialization **write failure** now surfaces as an unchecked `StrutsException` during consumption (via `getContent()`/`getAbsolutePath()`), rather than as a parse-time `LocalizedMessage`. This is why the old `temporaryFileCreationFailureAddsError` test is removed rather than rewritten: the parse-time graceful-degradation path for in-memory items no longer exists. No new test asserts the exception, since it only fires on genuine filesystem failures in the save directory.
---
## Task 5: Non-materializing interceptor validation (added after whole-branch review)
The whole-branch review found `AbstractFileUploadInterceptor.acceptFile()` calls `file.getContent() == null` first, materializing every in-memory upload during validation on the standard `fileUpload`/`actionFileUpload` path — defeating the optimization for the common consumer.
**Files:** `UploadedFile.java`, `StrutsInMemoryUploadedFile.java`, `AbstractFileUploadInterceptor.java`, `ActionFileUploadInterceptorTest.java`.
Steps: add `default boolean isMissing()` (= `getContent() == null`) to `UploadedFile`; override in `StrutsInMemoryUploadedFile` to `return false` (non-materializing); change the `acceptFile()` guard to `file == null || file.isMissing()` and delete the dead `getContent() == null` block; add JUnit3-style (`public void testXxx`) interceptor tests asserting `isFile()==false` after accept and after reject. Preserve `testAcceptFileWithNoContent` (null-content test-double rejected via the interface default). Commit: `WW-5413 perf(core): avoid materializing in-memory uploads during interceptor validation`.
## Task 6: Polish (from whole-branch review)
**Files:** `StrutsInMemoryUploadedFile.java`, `StrutsInMemoryUploadedFileTest.java`, `UploadedFileTest.java`.
Steps: in `materialize()`, `Files.deleteIfExists(targetFile.toPath())` (with `addSuppressed`) before rethrowing `StrutsException`, so a partial write on failure isn't leaked; add a class Javadoc note on the clustered-deployment serialization limitation; add direct unit tests for `isMissing()` (the override is non-materializing; the interface default reflects `getContent()`). Commit: `WW-5413 chore(core): clean up partial materialization and cover isMissing()`.
> Note left out of scope deliberately: the now-unused `STRUTS_MESSAGES_INVALID_CONTENT_TYPE_KEY` constant in `AbstractFileUploadInterceptor` is a `public static final` and was left in place to avoid an API break. The vestigial `throws IOException` on `JakartaMultiPartRequest.processFileField` was also left in place — removing it could break a subclass that catches `IOException` from `super.processFileField(...)`.
docs/superpowers/specs/2026-07-22-WW-5413-inmemory-upload-optimization-design.md
+136
View File
@@ -0,0 +1,136 @@
# WW-5413 — In-memory multipart upload optimization
**Jira:** [WW-5413](https://issues.apache.org/jira/browse/WW-5413) · **Fix version:** 7.3.0 · **Component:** Core
**Date:** 2026-07-22
## Background
WW-5413 was originally filed against 6.3.0: commons-io 2.16.0 broke `DeferredFileOutputStream`/`ThresholdingOutputStream`, cascading through commons-fileupload's `DiskFileItem` so multipart uploads were read as empty. A prior Struts workaround forced the disk-spill threshold to `-1`, spilling *every* field to disk. The ticket proposed dropping the `-1` threshold, handling the `isInMemory()` case properly, and avoiding unnecessary filesystem writes.
**The root-cause bug is already resolved on `main`.** Struts has since migrated to **commons-fileupload2 2.0.0-M5** + **commons-io 2.22.0**. `AbstractMultiPartRequest.createJakartaFileUpload()` sets no threshold, so it uses `DiskFileItemFactory`'s default (~8 KB); small uploads legitimately stay in memory (`isInMemory() == true`).
**What remains** is the performance half of the ticket. `JakartaMultiPartRequest.processFileField()` still takes every in-memory item and eagerly writes it to a temp file via `FileOutputStream`, then wraps that `File` in `StrutsUploadedFile`. The redundant filesystem write the ticket complained about is still present — it just moved from the fileupload layer into Struts' own code, because `UploadedFile`/`StrutsUploadedFile` are `File`-backed with no in-memory representation.
## Goal
Eliminate the redundant temp-file write for small (in-memory) uploads, while keeping 100% backward compatibility for existing `UploadedFile` consumers — including the legacy `File`-typed action property path and third-party `UploadedFile` implementations.
## Constraints & compatibility facts
- `UploadedFile` was designed for this: `isFile()` documents "real file or maybe just in-memory stream", `getContent()` returns `Object`, `getAbsolutePath()` is "if possible". `UploadedFile extends Serializable`.
- `getContent()` de-facto returns a `java.io.File` everywhere today: `UploadedFileConverter` (legacy `File`-typed action properties), `apps/showcase` actions (`FileUploadAction.getContent()`), and user actions in the wild. **The runtime type of `getContent()` must remain `File`** — a size-dependent `byte[]`/`File` return would break these consumers non-deterministically. This is why we do **not** expose bytes through `getContent()`.
- `AbstractMultiPartRequest.cleanUp()` deletes an uploaded file by calling `UploadedFile.delete()` **only when `isFile()` is true**. The lazy design plugs into this existing hook.
- The in-memory `DiskFileItem` buffer stays valid until `cleanUp()` runs (after action processing), so reading `item.get()` at parse time is safe.
## Approach: lazy materialization + a streaming accessor
Keep `getContent()`/`getAbsolutePath()` returning a `File` (materialized on demand), and add a new, correctly-typed door for reading bytes without forcing a disk write.
### 1. `UploadedFile` interface — new `getInputStream()`
Add one method as a **`default`** so existing third-party implementations keep compiling:
```java
default InputStream getInputStream() throws IOException {
Object c = getContent();
if (c instanceof File f) return new FileInputStream(f);
if (c instanceof byte[] b) return new ByteArrayInputStream(b);
throw new IOException("No content stream available for " + getName());
}
```
This is the type-safe "give me the bytes without forcing a file" path. `getContent()` still returns a `File` for every implementation, so no existing consumer changes.
### 2. New `StrutsInMemoryUploadedFile`
A second `UploadedFile` implementation beside `StrutsUploadedFile` — each class stays single-purpose; `Struts*` naming per project convention. It holds:
- `byte[] content` — the small upload's bytes, from `item.get()`
- `Path saveDir` — where a temp file will be written if ever demanded
- a **stable temp-file name chosen at construction** (`upload_<uuid>.tmp`) — only the *write* is deferred, so `getName()` is stable and matches the eventual file
- metadata: `contentType`, `originalName`, `inputName`
- `transient File materializedFile` — populated lazily, cached
The object must stay `Serializable` (a `DiskFileItem` reference would not be). **Implementation note:** rather than a `Path saveDir` + `String name` (a concrete `Path` such as `sun.nio.fs.UnixPath` is *not* guaranteed `Serializable`), the shipped code stores a single pre-computed `java.io.File targetFile` (which *is* `Serializable`) plus a `serialVersionUID`; `materializedFile` is `volatile transient`.
| Method | Behavior | Touches disk? |
|---|---|---|
| `getInputStream()` | `new ByteArrayInputStream(content)` | **No** |
| `length()` | `content.length` | No |
| `getName()` | the pre-chosen `upload_<uuid>.tmp` | No |
| `getContent()` | `materialize()` → returns the `File` | **Yes, once** (cached) |
| `getAbsolutePath()` | `materialize()` → path string | **Yes, once** (cached) |
| `isFile()` | true only *after* materialization | No |
| `getContentType()` / `getOriginalName()` / `getInputName()` | metadata | No |
| `delete()` | deletes the materialized file if it exists; no-op otherwise | — |
`materialize()` is `synchronized`, writes the bytes once to the pre-chosen path in `saveDir`, and caches the resulting `File`. The temp file is created with the project's secure UUID-named pattern (`upload_<uuid>.tmp`); the naming logic is shared with / extracted alongside `AbstractMultiPartRequest.createTemporaryFile` to avoid divergence.
**Net effect:** rejected uploads, size/type checks (`length()`), and `getInputStream()` consumers **never write**; legacy `File`/`getContent()` consumers write exactly once — same as today, but deferred.
### 3a. Non-materializing interceptor validation (added after whole-branch review)
The net-effect claim above is only real if the framework's own consumers don't force materialization during validation. They did: `AbstractFileUploadInterceptor.acceptFile()` — run for every uploaded file on the standard `fileUpload`/`actionFileUpload` path — called `file.getContent() == null` as its first (failed-upload) guard, which materialized every small in-memory upload before any size/type check. `acceptFile()` only validates metadata; it never needs the bytes.
Fix: add `default boolean isMissing()` to `UploadedFile` (default = `getContent() == null`, so third-party impls and the "no content = failed upload" contract are preserved), override it in `StrutsInMemoryUploadedFile` to return `false` (answered from the in-memory byte array, no materialization), change the `acceptFile()` guard to `file == null || file.isMissing()`, and delete the now-dead second `getContent() == null` block. Result: the `UploadedFilesAware` flow validates and hands files to the action without ever writing a small upload to disk; only a consumer that explicitly asks for a `File` (the legacy `File`-typed action property via `UploadedFileConverter`, which genuinely needs one) triggers the write.
### 3. `JakartaMultiPartRequest.processFileField` + cleanup simplification
The `item.isInMemory()` branch stops writing a temp file eagerly:
```java
if (item.isInMemory()) {
values.add(StrutsInMemoryUploadedFile.Builder
.create(item.get(), Path.of(saveDir))
.withOriginalName(item.getName())
.withContentType(item.getContentType())
.withInputName(item.getFieldName())
.build());
} else {
// unchanged File-based path via item.getPath()
}
```
Because in-memory files no longer create temp files eagerly:
- the `temporaryFiles` field, `cleanUpTemporaryFiles()`, and the `FileOutputStream` write block in `JakartaMultiPartRequest` are removed;
- cleanup of any *materialized* file happens through the existing `AbstractMultiPartRequest.cleanUp()` loop, which already calls `delete()` when `isFile()` is true — no new cleanup path is added;
- `createTemporaryFile` remains in `AbstractMultiPartRequest` (`JakartaStreamMultiPartRequest` still uses it).
`StrutsUploadedFile` keeps its current `File`-backed behavior; it may override `getInputStream()` to return `new FileInputStream(file)` directly rather than relying on the interface default.
`JakartaStreamMultiPartRequest` is unchanged: it always streams to disk (no in-memory threshold) and stays `File`-backed via `StrutsUploadedFile`, inheriting `getInputStream()` for free.
### 4. Error-handling behavior change (explicit)
`getContent()`/`getAbsolutePath()` have no `throws` clause, so a materialization **write failure** surfaces as an unchecked `StrutsException` **during consumption**, rather than as a gracefully-collected `LocalizedMessage` at parse time (today's behavior). This affects only the rare "cannot write to `saveDir`" case, and only on the legacy `File`/`getContent()` path — the `getInputStream()` fast path never writes. Accepted as a reasonable trade for the optimization.
## Testing
**`StrutsInMemoryUploadedFile` unit tests**
- `getInputStream()` returns the exact bytes and creates **no** file on disk
- `getContent()` and `getAbsolutePath()` create the temp file **exactly once** and cache it (second call reuses the same `File`)
- `isFile()` is false before materialization, true after
- `length()` and metadata accessors do **not** materialize
- `delete()` removes a materialized file and is a safe no-op when nothing was materialized
**`JakartaMultiPartRequest` tests**
- a small (in-memory) upload leaves **no** temp file on disk after `parse()`, until `getContent()`/`getAbsolutePath()` is called
- content is readable via both `getInputStream()` and `getContent()`
- a large (on-disk) upload keeps the existing `File`-backed path unchanged
- legacy `File`-typed action-property conversion (`UploadedFileConverter`) still works for a small upload
- `cleanUp()` removes any materialized file and leaves nothing behind
Tests follow the existing multipart test base (core tests are JUnit4 / `XWorkTestCase`-style — an `@Test` added to a `TestCase` subclass silently never runs).
## Out of scope
- Migrating showcase actions / `UploadedFileConverter` onto `getInputStream()` — they continue using `getContent()`. (The upload **interceptor** validation path *was* brought in scope and made non-materializing — see §3a — because it defeated the optimization for the common consumer.)
- Changing the disk-spill threshold or `JakartaStreamMultiPartRequest` streaming strategy.
- Any change to `getContent()`'s runtime type (must remain `File`).
## Known limitation
`StrutsInMemoryUploadedFile.targetFile` is an absolute 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, which may not exist there. Consumers that need content to survive cross-node replication should read via `getInputStream()` (never touches disk). Documented on the class Javadoc.