This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch feature/WW-5371-modern-upload
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/feature/WW-5371-modern-upload
by this push:
new 64c13cc74 WW-5370 Simplifies error handling logic
64c13cc74 is described below
commit 64c13cc746082de62d8c1fe95879491c6cb3094d
Author: Lukasz Lenart <[email protected]>
AuthorDate: Fri Dec 15 09:38:43 2023 +0100
WW-5370 Simplifies error handling logic
---
.../multipart/JakartaStreamMultiPartRequest.java | 1 -
.../interceptor/AbstractFileUploadInterceptor.java | 74 ++++++++++++----------
.../apache/struts2/struts-messages_en.properties | 27 +++++++-
.../ActionFileUploadInterceptorTest.java | 12 ++--
.../interceptor/FileUploadInterceptorTest.java | 12 ++--
5 files changed, 83 insertions(+), 43 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
index 763b5d634..428ae112f 100644
---
a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+++
b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
@@ -44,7 +44,6 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
-import java.util.stream.Collectors;
/**
* Multi-part form data request adapter for Jakarta Commons FileUpload package
that
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java
index c8da384d2..1113f4491 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java
@@ -39,6 +39,7 @@ import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Set;
public abstract class AbstractFileUploadInterceptor extends
AbstractInterceptor {
@@ -53,9 +54,9 @@ public abstract class AbstractFileUploadInterceptor extends
AbstractInterceptor
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";
- protected Long maximumSize;
- protected Set<String> allowedTypesSet = Collections.emptySet();
- protected Set<String> allowedExtensionsSet = Collections.emptySet();
+ private Long maximumSize;
+ private Set<String> allowedTypesSet = Collections.emptySet();
+ private Set<String> allowedExtensionsSet = Collections.emptySet();
private ContentTypeMatcher<Object> matcher;
private Container container;
@@ -100,15 +101,15 @@ public abstract class AbstractFileUploadInterceptor
extends AbstractInterceptor
/**
* Override for added functionality. Checks if the proposed file is
acceptable based on contentType and size.
*
- * @param action - uploading action for message retrieval.
- * @param file - proposed upload file.
- * @param filename - name of the file.
- * @param contentType - contentType of the file.
- * @param inputName - inputName of the file.
+ * @param action - uploading action for message retrieval.
+ * @param file - proposed upload file.
+ * @param originalFilename - name of the file.
+ * @param contentType - contentType of the file.
+ * @param inputName - inputName of the file.
* @return true if the proposed file is acceptable by contentType and size.
*/
- protected boolean acceptFile(Object action, UploadedFile file, String
filename, String contentType, String inputName) {
- boolean fileIsAcceptable = false;
+ protected boolean acceptFile(Object action, UploadedFile file, String
originalFilename, String contentType, String inputName) {
+ Set<String> errorMessages = new HashSet<>();
ValidationAware validation = null;
if (action instanceof ValidationAware) {
@@ -122,35 +123,42 @@ public abstract class AbstractFileUploadInterceptor
extends AbstractInterceptor
validation.addFieldError(inputName, errMsg);
}
LOG.warn(errMsg);
- } else if (file.getContent() == null) {
- String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_UPLOADING_KEY, new String[]{filename});
- if (validation != null) {
- validation.addFieldError(inputName, errMsg);
- }
+ return false;
+ }
+
+ if (file.getContent() == null) {
+ String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_UPLOADING_KEY, new String[]{originalFilename});
+ errorMessages.add(errMsg);
LOG.warn(errMsg);
- } else if (maximumSize != null && maximumSize < file.length()) {
- String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{inputName, filename,
file.getName(), "" + file.length(), getMaximumSizeStr(action)});
- if (validation != null) {
- validation.addFieldError(inputName, 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)
+ });
+ errorMessages.add(errMsg);
LOG.warn(errMsg);
- } else if ((!allowedTypesSet.isEmpty()) &&
(!containsItem(allowedTypesSet, contentType))) {
- String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{inputName,
filename, file.getName(), contentType});
- if (validation != null) {
- validation.addFieldError(inputName, errMsg);
- }
+ }
+ if ((!allowedTypesSet.isEmpty()) && (!containsItem(allowedTypesSet,
contentType))) {
+ String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{
+ inputName, originalFilename, file.getName(), contentType
+ });
+ errorMessages.add(errMsg);
LOG.warn(errMsg);
- } else if ((!allowedExtensionsSet.isEmpty()) &&
(!hasAllowedExtension(allowedExtensionsSet, filename))) {
- String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{inputName,
filename, file.getName(), contentType});
- if (validation != null) {
- validation.addFieldError(inputName, errMsg);
- }
+ }
+ if ((!allowedExtensionsSet.isEmpty()) &&
(!hasAllowedExtension(allowedExtensionsSet, originalFilename))) {
+ String errMsg = getTextMessage(action,
STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{
+ inputName, originalFilename, file.getName(), contentType
+ });
+ errorMessages.add(errMsg);
LOG.warn(errMsg);
- } else {
- fileIsAcceptable = true;
+ }
+ if (validation != null) {
+ for (String errorMsg : errorMessages) {
+ validation.addFieldError(inputName, errorMsg);
+ }
}
- return fileIsAcceptable;
+ return errorMessages.isEmpty();
}
private String getMaximumSizeStr(Object action) {
diff --git
a/core/src/main/resources/org/apache/struts2/struts-messages_en.properties
b/core/src/main/resources/org/apache/struts2/struts-messages_en.properties
index 155e13c39..d70fe0119 100644
--- a/core/src/main/resources/org/apache/struts2/struts-messages_en.properties
+++ b/core/src/main/resources/org/apache/struts2/struts-messages_en.properties
@@ -25,15 +25,40 @@ struts.internal.invalid.token=Form token {0} does not match
the session token {1
struts.messages.bypass.request=Bypassing {0}/{1}
struts.messages.current.file=File {0} {1} {2} {3}
+# 0 - input name
struts.messages.invalid.file=Could not find a Filename for {0}. Verify that a
valid file was submitted.
+# 0 - original filename
struts.messages.invalid.content.type=Could not find a Content-Type for {0}.
Verify that a valid file was submitted.
struts.messages.removing.file=Removing file {0} {1}
struts.messages.error.uploading=Error uploading: {0}
-struts.messages.error.file.too.large=The file is too large to be uploaded: {0}
"{1}" "{2}" {3}
+# 0 - input name
+# 1 - original filename
+# 2 - file name after uploading the file
+# 3 - size of the uploaded files
+# 4 - maximum allowed size
+struts.messages.error.file.too.large=The file is too large to be uploaded: {0}
"{1}" "{2}" has size {3} and allowed mx size is {4}
+# 0 - input name
+# 1 - original filename
+# 2 - file name after uploading the file
+# 3 - content type of the file
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0}
"{1}" "{2}" {3}
+# 0 - input name
+# 1 - original filename
+# 2 - file name after uploading the file
+# 3 - content type of the file
struts.messages.error.file.extension.not.allowed=File extension not allowed:
{0} "{1}" "{2}" {3}
# dedicated messages used to handle various problems with file upload - check
{@link JakartaMultiPartRequest#parse(HttpServletRequest, String)}
+# params depend on exception being handled
+# FileUploadBase.SizeLimitExceededException:
+# 0 - permitted size
+# 1 - actual size
+# FileUploadBase.FileSizeLimitExceededException
+# 0 - file name
+# 1 - permitted size
+# 2 - actual size
+# FileCountLimitExceededException
+# 0 - limit
struts.messages.upload.error.SizeLimitExceededException=Request exceeded
allowed size limit! Max size allowed is: {0} but request was: {1}!
struts.messages.upload.error.IOException=Error uploading: {0}!
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
index 415d4ca67..51747b147 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
@@ -44,6 +44,8 @@ import java.util.Collection;
import java.util.List;
import java.util.Locale;
+import static org.assertj.core.api.Assertions.assertThat;
+
/**
* Test case for {@link ActionFileUploadInterceptor}.
*/
@@ -199,7 +201,6 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
}
public void testAcceptFileWithMaxSize() throws Exception {
- interceptor.setAllowedTypes("text/plain");
interceptor.setMaximumSize(10L);
// when file is not of allowed types
@@ -218,9 +219,12 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
assertEquals(1, errors.size());
String msg = errors.get(0);
// the error message should contain at least this test
- assertTrue(msg.startsWith("The file is too large to be uploaded"));
- assertTrue(msg.indexOf("inputName") > 0);
- assertTrue(msg.indexOf("log4j2.xml") > 0);
+ assertThat(msg).contains(
+ "The file is too large to be uploaded",
+ "inputName",
+ "log4j2.xml",
+ "allowed mx size is 10"
+ );
}
public void testNoMultipartRequest() throws Exception {
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
index 18758c422..14bb23c36 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
@@ -47,6 +47,8 @@ import java.util.List;
import java.util.Locale;
import java.util.Map;
+import static org.assertj.core.api.Assertions.assertThat;
+
/**
* Test case for FileUploadInterceptor.
@@ -198,7 +200,6 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
}
public void testAcceptFileWithMaxSize() throws Exception {
- interceptor.setAllowedTypes("text/plain");
interceptor.setMaximumSize(10L);
// when file is not of allowed types
@@ -217,9 +218,12 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
assertEquals(1, errors.size());
String msg = errors.get(0);
// the error message should contain at least this test
- assertTrue(msg.startsWith("The file is too large to be uploaded"));
- assertTrue(msg.indexOf("inputName") > 0);
- assertTrue(msg.indexOf("log4j2.xml") > 0);
+ assertThat(msg).contains(
+ "The file is too large to be uploaded",
+ "inputName",
+ "log4j2.xml",
+ "allowed mx size is 10"
+ );
}
public void testNoMultipartRequest() throws Exception {
