sagarmiglani commented on code in PR #75:
URL:
https://github.com/apache/sling-org-apache-sling-engine/pull/75#discussion_r3252619489
##########
src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java:
##########
@@ -34,33 +35,38 @@
*/
public class MultipartRequestParameter extends AbstractRequestParameter {
- private final DiskFileItem delegatee;
+ private final Part delegatee;
private String encodedFileName;
private String cachedValue;
- public MultipartRequestParameter(DiskFileItem delegatee) {
- super(delegatee.getFieldName(), null);
+ public MultipartRequestParameter(Part delegatee) {
+ super(delegatee.getName(), null);
this.delegatee = delegatee;
}
void dispose() throws IOException {
this.delegatee.delete();
}
- DiskFileItem getFileItem() {
+ Part getPart() {
return this.delegatee;
}
@Override
void setEncoding(String encoding) {
super.setEncoding(encoding);
cachedValue = null;
+ encodedFileName = null;
}
public byte[] get() {
- return this.delegatee.get();
+ try {
+ return getInputStream().readAllBytes();
Review Comment:
This method re-reads the part on every call and never closes the stream. I
will also suggest caching the content
(https://github.com/apache/commons-fileupload/blob/FILEUPLOAD_1_3_1/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java#L302)
##########
src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java:
##########
@@ -117,15 +123,16 @@ public String getString() {
return this.cachedValue;
}
- return this.delegatee.getString();
+ return new String(this.get());
Review Comment:
This now uses the JVM platform default charset. Same upload returns
different strings on Linux UTF-8 vs Windows-1252.
The old DiskFileItem.getString() used the part's Content-Type charset.
https://github.com/apache/sling-org-apache-sling-engine/pull/61/changes#diff-456ff33f6f11891702c3f5e9226e0ee6b15090fbd46a0a1ec5ea5901d40f7393L120
##########
src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java:
##########
Review Comment:
Shall we add part.delete now?
https://jakarta.ee/specifications/servlet/5.0/apidocs/jakarta/servlet/http/part#delete()
##########
src/main/java/org/apache/sling/engine/impl/parameters/RequestPartsIterator.java:
##########
@@ -20,61 +20,62 @@
import java.io.IOException;
import java.io.InputStream;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
-import java.util.List;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.Part;
-import org.apache.commons.fileupload.FileItemIterator;
-import org.apache.commons.fileupload.FileItemStream;
-import org.apache.commons.fileupload.FileUpload;
-import org.apache.commons.fileupload.FileUploadException;
-import org.apache.commons.fileupload.RequestContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Contains a Lazy iterator of Parts from the request stream loaded as the
request is streamed using the Commons FileUpload API.
+ * Contains a Lazy iterator of Parts from the request stream loaded as the
request is streamed
*/
public class RequestPartsIterator implements Iterator<Part> {
- private static final Logger LOG =
LoggerFactory.getLogger(RequestPartsIterator.class);
+ private final Logger log = LoggerFactory.getLogger(getClass());
- /** The CommonsFile Upload streaming API iterator */
- private final FileItemIterator itemIterator;
+ /** The Parts iterator */
+ private Iterator<Part> itemIterator = null;
+
+ /** supplier to retrieve the parts when needed */
+ private final HttpServletRequest request;
/**
- * Create and initialse the iterator using the request. The request must
be fresh. Headers can have been read but the stream
- * must not have been parsed.
- * @param servletRequest the request
- * @throws IOException when there is a problem reading the request.
- * @throws FileUploadException when there is a problem parsing the request.
+ * Create and initialize the iterator using the request.
+ *
+ * @param request the current request
*/
- public RequestPartsIterator(final RequestContext context) throws
FileUploadException, IOException {
- FileUpload upload = new FileUpload();
- upload.setFileCountMax(50);
- itemIterator = upload.getItemIterator(context);
+ public RequestPartsIterator(final HttpServletRequest request) {
+ this.request = request;
}
@Override
public boolean hasNext() {
- try {
- return itemIterator.hasNext();
- } catch (final FileUploadException | IOException e) {
- LOG.error("hasNext Item failed cause:" + e.getMessage(), e);
+ // Use a lazy data iterator creation the first time it is needed to
ensure
+ // the container doesn't parse the uploaded files until necessary
+ if (itemIterator == null) {
+ Collection<Part> parts;
+ try {
+ parts = request.getParts();
Review Comment:
IIRC, this was my biggest concern during the work on my PR.
Sling has a
[feature](https://sling.apache.org/news/sling-launchpad-9-released.html#streaming-upload-support)
where a client could send a header Sling-uploadmode: stream, that used
commons-fileupload's FileUpload.getItemIterator(...). That returned a true lazy
iterator.
If i'm not wrong, request.getParts() is required to fully parse the entire
multipart body before returning. The lazy-hasNext pattern is cosmetic here, not
real.
A client that explicitly opted into streaming mode to upload a 50 GB file
will now have the container try to buffer it (memory + disk).
##########
src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java:
##########
Review Comment:
this javadoc needs update :)
##########
src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java:
##########
@@ -69,6 +65,9 @@ public class ParameterSupport {
/** Content type signaling parameters in request body */
private static final String WWW_FORM_URL_ENC =
"application/x-www-form-urlencoded";
+ /** Content type signaling parameters in multipart request body */
+ private static final String MULTPART = "multipart/";
Review Comment:
typo š
MULTPART -> MULTIPART
##########
src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java:
##########
@@ -125,19 +128,19 @@ public Map<String, String[]> getStringParameterMap() {
public Object getPart(final String name) {
final RequestParameter p = this.getValue(name);
- if (p instanceof MultipartRequestParameter) {
- return new SlingPart((MultipartRequestParameter) p);
+ if (p instanceof MultipartRequestParameter mrp) {
+ return mrp.getPart();
Review Comment:
IMO, we should keep the light SlingPart wrapper and return that.
Returning mrp.getPart() as-it-is here skips the encoding round-trip that the
deleted SlingPart.getSubmittedFileName() used to delegate
throughMultipartRequestParameter.getFileName().
##########
src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java:
##########
@@ -90,6 +91,10 @@ public class SlingMainServlet extends GenericServlet {
@Reference
private volatile SlingRequestProcessorImpl requestProcessorImpl;
+ /** extra config properties for multipart file upload support */
+ @Reference
+ private transient RequestParameterConfig reqParamConfig;
Review Comment:
nit: do we need transient?
##########
src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java:
##########
@@ -390,55 +346,22 @@ private static final boolean
isWWWFormEncodedContent(HttpServletRequest request)
return false;
}
- private RequestContext getMultiPartContext() {
- return new RequestContext() {
- @Override
- public String getCharacterEncoding() {
- String enc = getServletRequest().getCharacterEncoding();
- return (enc != null) ? enc : Util.ENCODING_DIRECT;
- }
-
- @Override
- public int getContentLength() {
- return getServletRequest().getContentLength();
- }
-
- @Override
- public String getContentType() {
- return getServletRequest().getContentType();
- }
-
- @Override
- public InputStream getInputStream() throws IOException {
- return getServletRequest().getInputStream();
- }
- };
+ /**
+ * Checks if the request is a multipart post
+ *
+ * @param request the request to check
+ * @return true if the request is multipart, false otherwise
+ */
+ private static boolean isMultipartContent(HttpServletRequest request) {
+ final String contentType = request.getContentType();
+ return contentType != null &&
contentType.toLowerCase(Locale.ENGLISH).startsWith(MULTPART);
}
- private void parseMultiPartPost(ParameterMap parameters) {
- // Create a new file upload handler
- ServletFileUpload upload = new ServletFileUpload();
- upload.setSizeMax(ParameterSupport.maxRequestSize);
- upload.setFileSizeMax(ParameterSupport.maxFileSize);
- upload.setFileItemFactory(
- new DiskFileItemFactory(ParameterSupport.fileSizeThreshold,
ParameterSupport.location));
- upload.setFileCountMax(ParameterSupport.maxFileCount);
- final RequestContext rc = this.getMultiPartContext();
-
- // Parse the request
- List<?> /* FileItem */ items = null;
- try {
- items = upload.parseRequest(rc);
- } catch (FileUploadException fue) {
- this.log.error("parseMultiPartPost: Error parsing request", fue);
- }
-
- if (items != null && items.size() > 0) {
- for (Iterator<?> ii = items.iterator(); ii.hasNext(); ) {
- DiskFileItem fileItem = (DiskFileItem) ii.next();
- RequestParameter pp = new MultipartRequestParameter(fileItem);
- parameters.addParameter(pp, false);
- }
+ private void parseMultiPartPost(ParameterMap parameters) throws
IOException, ServletException {
+ final Collection<Part> parts = this.getServletRequest().getParts();
Review Comment:
maxFileCount seems to be dropped, can we use something similar?
https://github.com/apache/sling-org-apache-sling-engine/pull/61/changes#diff-7ada9ce1f0869aa363c184c894ab0b732ccf28ce4b7e16dda47de7c8a7c3982aR418
##########
src/main/java/org/apache/sling/engine/impl/parameters/ParameterSupport.java:
##########
@@ -323,7 +274,12 @@ private ParameterMap getRequestParameterMapInternal() {
addContainerParameters = false;
useFallback = false;
} else {
- this.parseMultiPartPost(parameters);
+ try {
+ this.parseMultiPartPost(parameters);
+ } catch (final ServletException | IOException e) {
+ this.log.error(
+ "getRequestParameterMapInternal: Error
parsing multipart streamed request", e);
Review Comment:
Log message says "streamed request" but this is the non-streamed branch.
Also, Iām wondering if it's okay to swallow these exceptions.
--
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]
