Copilot commented on code in PR #1283:
URL: https://github.com/apache/struts/pull/1283#discussion_r2217901425
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -19,7 +19,6 @@
package org.apache.struts2.dispatcher.multipart;
import jakarta.servlet.http.HttpServletRequest;
-import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.core.FileItemInput;
Review Comment:
The removed import for DiskFileItemFactory appears to be unused after the
refactoring, but verify that createJakartaFileUpload method removal is
intentional and doesn't break inheritance contract.
```suggestion
```
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -82,51 +81,62 @@ protected void processUpload(HttpServletRequest request,
String saveDir) throws
});
}
- protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset
charset, Path location) {
- DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
-
- LOG.debug("Using file save directory: {}", location);
- builder.setPath(location);
-
- LOG.debug("Sets buffer size: {}", bufferSize);
- builder.setBufferSize(bufferSize);
-
- LOG.debug("Using charset: {}", charset);
- builder.setCharset(charset);
-
- DiskFileItemFactory factory = builder.get();
- return new JakartaServletDiskFileUpload(factory);
- }
-
+ /**
+ * Reads the entire contents of an input stream into a string.
+ *
+ * <p>This method uses a buffered approach to efficiently read the stream
+ * content without loading the entire stream into memory at once. It uses
+ * try-with-resources to ensure proper cleanup of resources.</p>
+ *
+ * @param inputStream the input stream to read from
+ * @return the stream contents as a UTF-8 string
+ * @throws IOException if an error occurs reading the stream
+ */
private String readStream(InputStream inputStream) throws IOException {
- ByteArrayOutputStream result = new ByteArrayOutputStream();
- byte[] buffer = new byte[1024];
- for (int length; (length = inputStream.read(buffer)) != -1; ) {
- result.write(buffer, 0, length);
+ // Use try-with-resources to ensure ByteArrayOutputStream is properly
closed
+ try (ByteArrayOutputStream result = new ByteArrayOutputStream()) {
+ byte[] buffer = new byte[1024]; // 1KB buffer for efficient reading
+ // Read the stream in chunks to avoid loading everything into
memory at once
+ for (int length; (length = inputStream.read(buffer)) != -1; ) {
+ result.write(buffer, 0, length);
+ }
+ // Convert to string using UTF-8 encoding
+ return result.toString(StandardCharsets.UTF_8);
}
- return result.toString(StandardCharsets.UTF_8);
}
/**
- * Processes the FileItem as a normal form field.
- *
- * @param fileItemInput a form field item input
+ * Processes a normal form field (non-file) from the multipart request
using streaming API.
+ *
+ * <p>This method handles text form fields by:</p>
+ * <ol>
+ * <li>Validating the field name is not null</li>
+ * <li>Reading the field value from the input stream</li>
+ * <li>Checking if the field value exceeds maximum string length</li>
+ * <li>Adding the value to the parameters collection</li>
+ * </ol>
+ *
+ * <p>Fields with null names are skipped with a warning log message.</p>
+ * <p>The streaming approach is more memory-efficient for large form
data.</p>
+ *
+ * @param fileItemInput a form field item input from the streaming API
+ * @throws IOException if an error occurs reading the input stream
+ * @see #readStream(InputStream)
+ * @see #exceedsMaxStringLength(String, String)
*/
protected void processFileItemAsFormField(FileItemInput fileItemInput)
throws IOException {
String fieldName = fileItemInput.getFieldName();
+ if (fieldName == null) {
+ LOG.warn("Form field has null fieldName, skipping");
+ return;
+ }
+
String fieldValue = readStream(fileItemInput.getInputStream());
-
if (exceedsMaxStringLength(fieldName, fieldValue)) {
return;
}
- List<String> values;
- if (parameters.containsKey(fieldName)) {
- values = parameters.get(fieldName);
- } else {
- values = new ArrayList<>();
- parameters.put(fieldName, values);
- }
+ List<String> values = parameters.computeIfAbsent(fieldName, k -> new
ArrayList<>());
Review Comment:
[nitpick] Using computeIfAbsent with ArrayList::new lambda would be more
efficient than creating a new ArrayList inside the lambda:
`parameters.computeIfAbsent(fieldName, k -> new ArrayList<>())` should be
`parameters.computeIfAbsent(fieldName, k -> new ArrayList<>())`.
```suggestion
List<String> values = parameters.computeIfAbsent(fieldName,
ArrayList::new);
```
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -20,12 +20,14 @@
import jakarta.servlet.http.HttpServletRequest;
import org.apache.commons.fileupload2.core.DiskFileItem;
-import org.apache.commons.fileupload2.core.DiskFileItemFactory;
+import org.apache.commons.fileupload2.core.RequestContext;
Review Comment:
The removed import for DiskFileItemFactory suggests the
createJakartaFileUpload method was moved to the parent class, but this should
be verified for consistency across implementations.
```suggestion
import org.apache.commons.fileupload2.core.RequestContext;
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
```
##########
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java:
##########
@@ -71,4 +79,316 @@ public void maxSizeOfFiles() throws IOException {
.containsExactly("struts.messages.upload.error.FileUploadSizeException");
}
+ @Test
+ public void readStreamProperlyHandlesResources() throws Exception {
+ // Create a test input stream with known data
+ byte[] testData = "test data for stream
reading".getBytes(StandardCharsets.UTF_8);
+ InputStream testStream = new java.io.ByteArrayInputStream(testData);
+
+ JakartaStreamMultiPartRequest streamMultiPart = new
JakartaStreamMultiPartRequest();
+
+ // Use reflection to access private readStream method
+ Method readStreamMethod =
JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream",
InputStream.class);
+ readStreamMethod.setAccessible(true);
+
+ // when
+ String result = (String) readStreamMethod.invoke(streamMultiPart,
testStream);
+
+ // then
+ assertThat(result).isEqualTo("test data for stream reading");
+ }
+
+ @Test
+ public void readStreamHandlesExceptionsProperly() throws Exception {
+ // Create a stream that throws an exception
+ InputStream faultyStream = new InputStream() {
+ @Override
+ public int read() throws IOException {
+ throw new IOException("Simulated stream failure");
+ }
+ };
+
+ JakartaStreamMultiPartRequest streamMultiPart = new
JakartaStreamMultiPartRequest();
+
+ // Use reflection to access private readStream method
+ Method readStreamMethod =
JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream",
InputStream.class);
+ readStreamMethod.setAccessible(true);
+
+ // when/then - should propagate the exception
+ assertThatThrownBy(() -> readStreamMethod.invoke(streamMultiPart,
faultyStream))
+ .isInstanceOf(InvocationTargetException.class)
+ .cause()
+ .isInstanceOf(IOException.class)
+ .hasMessage("Simulated stream failure");
+ }
+
+ @Test
+ public void readStreamHandlesEmptyStream() throws Exception {
+ // Create an empty stream
+ InputStream emptyStream = new java.io.ByteArrayInputStream(new
byte[0]);
+
+ JakartaStreamMultiPartRequest streamMultiPart = new
JakartaStreamMultiPartRequest();
+
+ // Use reflection to access private readStream method
+ Method readStreamMethod =
JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream",
InputStream.class);
+ readStreamMethod.setAccessible(true);
+
+ // when
+ String result = (String) readStreamMethod.invoke(streamMultiPart,
emptyStream);
+
+ // then
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ public void readStreamHandlesLargeData() throws Exception {
+ // Create a large data stream to test buffer handling
+ StringBuilder largeData = new StringBuilder();
+ for (int i = 0; i < 2000; i++) {
+ largeData.append("line").append(i).append("\n");
+ }
+
+ byte[] testData =
largeData.toString().getBytes(StandardCharsets.UTF_8);
+ InputStream largeStream = new java.io.ByteArrayInputStream(testData);
+
+ JakartaStreamMultiPartRequest streamMultiPart = new
JakartaStreamMultiPartRequest();
+
+ // Use reflection to access private readStream method
+ Method readStreamMethod =
JakartaStreamMultiPartRequest.class.getDeclaredMethod("readStream",
InputStream.class);
+ readStreamMethod.setAccessible(true);
+
+ // when
+ String result = (String) readStreamMethod.invoke(streamMultiPart,
largeStream);
+
+ // then
+ assertThat(result).isEqualTo(largeData.toString());
+ assertThat(result.length()).isGreaterThan(1024); // Verify it's larger
than internal buffer
+ }
+
+ @Test
+ public void processFileItemAsFormFieldHandlesNullFieldName() throws
IOException {
+ // Test the null field name path in processFileItemAsFormField
+ String content = formFile("", "test.csv", "data") + // Field name will
be empty/null-like
+ endline + "--" + boundary + "--";
+
+ mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
+
+ // when
+ multiPart.parse(mockRequest, tempDir);
+
+ // then - should complete without error, but no parameters should be
added
+ assertThat(multiPart.getErrors()).isEmpty();
+ }
+
+ @Test
+ public void processFileItemAsFileFieldHandlesNullFieldName() throws
IOException {
+ // This test covers the null field name path in
processFileItemAsFileField
+ JakartaStreamMultiPartRequest streamMultiPart = new
JakartaStreamMultiPartRequest();
+
+ // Create a mock file item with null field name
+ String content = "--" + boundary + endline +
+ "Content-Disposition: form-data;
filename=\"test.csv\"" + endline +
+ "Content-Type: text/csv" + endline +
+ endline +
+ "test data" +
+ endline + "--" + boundary + "--";
+
+ mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
+
+ // when
+ streamMultiPart.parse(mockRequest, tempDir);
+
+ // then - should complete without error but no files should be uploaded
+ assertThat(streamMultiPart.getErrors()).isEmpty();
+ assertThat(streamMultiPart.uploadedFiles).isEmpty();
Review Comment:
Accessing the package-private field 'uploadedFiles' directly in tests
creates tight coupling. Consider using public getter methods or exposing a
test-friendly method instead.
##########
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java:
##########
@@ -18,11 +18,439 @@
*/
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.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;
+
public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
@Override
protected AbstractMultiPartRequest createMultipartRequest() {
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
+ JakartaMultiPartRequest multiPartWithDuplicateErrors = new
JakartaMultiPartRequest();
+
+ // Simulate adding the same error twice
+ IOException testException = new IOException("Test exception");
+ LocalizedMessage errorMessage =
multiPartWithDuplicateErrors.buildErrorMessage(
+ testException.getClass(), testException.getMessage(), new
Object[]{"test.csv"});
+
+ // when - add same error twice
+ multiPartWithDuplicateErrors.errors.add(errorMessage);
Review Comment:
Directly accessing the package-private 'errors' field in tests creates tight
coupling. Consider using public methods or a test builder pattern to manipulate
error states.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
