[
https://issues.apache.org/jira/browse/WW-5388?focusedWorklogId=902393&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-902393
]
ASF GitHub Bot logged work on WW-5388:
--------------------------------------
Author: ASF GitHub Bot
Created on: 29/Jan/24 17:48
Start Date: 29/Jan/24 17:48
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469980120
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest<File> {
- static final Logger LOG =
LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private static final Logger LOG =
LogManager.getLogger(JakartaStreamMultiPartRequest.class);
/**
- * Map between file fields and file data.
- */
- protected Map<String, List<FileInfo>> fileInfos = new HashMap<>();
-
- /**
- * Map between non-file fields and values.
- */
- protected Map<String, List<String>> parameters = new HashMap<>();
-
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- public void cleanUp() {
- LOG.debug("Performing File Upload temporary storage cleanup.");
- for (List<FileInfo> fileInfoList : fileInfos.values()) {
- for (FileInfo fileInfo : fileInfoList) {
- File file = fileInfo.getFile();
- LOG.debug("Deleting file '{}'.", file.getName());
- if (!file.delete()) {
- LOG.warn("There was a problem attempting to delete file
'{}'.", file.getName());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir)
throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ Path location = Path.of(saveDir);
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), location);
+
+ LOG.debug("Using Jakarta Stream API to process request");
+ servletFileUpload.getItemIterator(request).forEachRemaining(item -> {
+ if (item.isFormField()) {
+ LOG.debug(() -> "Processing a form field: " +
sanitizeNewlines(item.getFieldName()));
+ processFileItemAsFormField(item);
+ } else {
+ LOG.debug(() -> "Processing a file: " +
sanitizeNewlines(item.getFieldName()));
+ processFileItemAsFileField(item, location);
}
- }
+ });
}
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getContentType(java.lang.String)
- */
- public String[] getContentType(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset
charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFile(java.lang.String)
- */
- public UploadedFile[] getFile(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileNames(java.lang.String)
- */
- public String[] getFileNames(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
-
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(getCanonicalName(fileInfo.getOriginalName()));
+ 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);
}
-
- return names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames()
- */
- public Enumeration<String> getFileParameterNames() {
- return Collections.enumeration(fileInfos.keySet());
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFilesystemName(java.lang.String)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput)
throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- return names.toArray(new String[0]);
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameter(java.lang.String)
- */
- public String getParameter(String name) {
- List<String> values = parameters.get(name);
- if (values != null && !values.isEmpty()) {
- return values.get(0);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files ->
files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterValues(java.lang.String)
- */
- public String[] getParameterValues(String name) {
- List<String> values = parameters.get(name);
- if (values != null && !values.isEmpty()) {
- return values.toArray(new String[0]);
- }
- return null;
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#parse(jakarta.servlet.http.HttpServletRequest,
java.lang.String)
- */
- public void parse(HttpServletRequest request, String saveDir) throws
IOException {
- try {
- setLocale(request);
- processUpload(request, saveDir);
- } catch (Exception e) {
- LOG.debug("Error occurred during parsing of multi part request",
e);
- LocalizedMessage errorMessage = buildErrorMessage(e, new
Object[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed
max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new
FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum
number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir)
throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // Interface with Commons FileUpload API
- // Using the Streaming API
- JakartaServletFileUpload<DiskFileItem, DiskFileItemFactory>
servletFileUpload = new JakartaServletFileUpload<>();
- if (maxSize != null) {
- servletFileUpload.setSizeMax(maxSize);
- }
- if (maxFiles != null) {
- servletFileUpload.setFileCountMax(maxFiles);
- }
- if (maxFileSize != null) {
- servletFileUpload.setFileSizeMax(maxFileSize);
- }
- FileItemInputIterator i =
servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // Delegate the file item stream for a file field to the
- // file item stream handler, but delegation is skipped
- // if the requestSizePermitted check failed based on the
- // complete content-size of the request.
- else {
-
- // prevent processing file field item if request size
not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum
size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file,
Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {},
actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(),
maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new
FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max
size %s",
+ file.length(), fileItemInput.getName(),
maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest
request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size
limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new
FileUploadSizeException(exceptionMessage, getRequestSize(request),
allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new
Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of
all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.getInputStream().toString();
- if (!parameters.containsKey(fieldName)) {
- values = new ArrayList<>();
- parameters.put(fieldName, values);
- } else {
- values = parameters.get(fieldName);
- }
- values.add(fieldValue);
- } catch (IOException e) {
- LOG.warn("Failed to handle form field '{}'.", fieldName, e);
+ protected void processFileItemAsFileField(FileItemInput fileItemInput,
Path location) throws IOException {
+ // Skip file uploads that don't have a file name - meaning that no
file was selected.
+ if (fileItemInput.getName() == null ||
fileItemInput.getName().trim().isEmpty()) {
+ LOG.debug(() -> "No file has been uploaded for the field: " +
sanitizeNewlines(fileItemInput.getFieldName()));
+ return;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream,
String location) {
- // Skip file uploads that don't have a file name - meaning that no
file was selected.
- if (itemStream.getName() == null ||
itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}",
itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception
above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = actualSizeOfUploadedFiles();
Review Comment:
> This seems expensive to compute from scratch every time a file field is
encountered, especially when the application might not have even configured a
`maxSizeOfFiles` limit!
Fixed!
Issue Time Tracking
-------------------
Worklog Id: (was: 902393)
Time Spent: 6h 50m (was: 6h 40m)
> Upgrade Commons Fileupload to FileUpload Jakarta Servlet 6
> ----------------------------------------------------------
>
> Key: WW-5388
> URL: https://issues.apache.org/jira/browse/WW-5388
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.0.0
>
> Time Spent: 6h 50m
> Remaining Estimate: 0h
>
> There is a new version of JakartaEE FileUpload
> {code:xml}
> <dependency>
> <groupId>org.apache.commons</groupId>
> <artifactId>commons-fileupload2-jakarta-servlet6</artifactId>
> <version>2.0.0-M2</version>
> </dependency>
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
