Skip to content

SLING-13202 Remove dependency on commons-fileupload #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
enapps-enorman wants to merge 7 commits into
base: master
Choose a base branch
from
Open

SLING-13202 Remove dependency on commons-fileupload #75

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac00012
SLING-13202 Remove dependency on commons-fileupload
enapps-enorman May 14, 2026
5aca587
SLING-13202 cleanup comment
enapps-enorman May 14, 2026
bedd1f5
SLING-13202 resolve sonar warnings and tests for code coverage
enapps-enorman May 14, 2026
39c4d72
SLING-13202 more tests for code coverage
enapps-enorman May 14, 2026
4f84a18
SLING-13202 even more tests for code coverage
enapps-enorman May 15, 2026
6efb03f
SLING-13202 incorporate changes requested by code review
enapps-enorman May 17, 2026
5020c47
SLING-13202 pass the mulitpart maxFileCount value to the servlet config
enapps-enorman May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,6 @@
<version>2.1.2</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>commons-fileupload</groupId>
<artifactId>commons-fileupload</artifactId>
<version>1.6.0</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.framework</artifactId>
Expand Down
20 changes: 20 additions & 0 deletions src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.apache.sling.engine.impl.helper.ClientAbortException;
import org.apache.sling.engine.impl.helper.RequestListenerManager;
import org.apache.sling.engine.impl.helper.SlingServletContext;
import org.apache.sling.engine.impl.parameters.RequestParameterConfig;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
import org.osgi.framework.ServiceRegistration;
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Contributor

@sagarmiglani sagarmiglani May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we need transient?

Copy link
Copy Markdown
Contributor Author

@enapps-enorman enapps-enorman May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this modifier, sonar reports it as a violation of rule "java:S1948" with this reason:
Fields in a "Serializable" class should either be transient or serializable

Copy link
Copy Markdown
Contributor

@cziegeler cziegeler May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think that this we should ignore such default sonar reports. While this makes sense in general for Serializable objects, it does imho not make sense for Servlets (or any other service). These objects will never be serialized and therefore this just clutters the code

Copy link
Copy Markdown
Contributor Author

@enapps-enorman enapps-enorman May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't bother me either way which is why I usually just change it to satisfy sonar. I have no idea how I would go about changing the sonar rules for specific "serializable" types for this or other sling projects. Please tell me how I would accomplish that.

Copy link
Copy Markdown
Contributor

@cziegeler cziegeler May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, but I wouldnt blindly apply sonar suggestions and just apply the ones which make sense to me.

Copy link
Copy Markdown
Contributor Author

@enapps-enorman enapps-enorman May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cziegeler That's the mindset that leads to projects having hundreds or thousands of unresolved warnings. When it gets to that state the real problems get lost in the noise. So I always make a best effort to have all the code I work on have no warnings.

Copy link
Copy Markdown
Contributor

@cziegeler cziegeler May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen dozens of such changes in general which actually made the code worse just to get rid off warnings in some tool. Lets agree that transient in this case is not needed, but it doesnt hurt either. So lets just move on..

Copy link
Copy Markdown
Member

@oliverlietz oliverlietz May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments in SLING-10506.


private volatile boolean allowTrace;

private volatile ServiceRegistration<Servlet> servletRegistration;
Expand Down Expand Up @@ -181,6 +186,21 @@ private Dictionary<String, Object> getServletContextRegistrationProps(final Stri
if (servletName != null) {
servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_NAME, servletName);
}

// configure support for multipart file uploads
servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_ENABLED, true);
// dig the multipart configuration out of the "Request Parameter Handling" configuration
final String multipartLocation = reqParamConfig.resolveLocation();
if (multipartLocation != null) {
servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_LOCATION, multipartLocation);
}
final org.apache.sling.engine.impl.parameters.RequestParameterConfig.Config config = reqParamConfig.getConfig();
servletConfig.put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_MAXFILESIZE, config.file_max());
servletConfig.put(
HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_MAXREQUESTSIZE, config.request_max());
servletConfig.put(
HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_MULTIPART_FILESIZETHRESHOLD, config.file_threshold());

servletConfig.put(Constants.SERVICE_DESCRIPTION, "Apache Sling Engine Main Servlet");
servletConfig.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation");

Expand Down
27 changes: 17 additions & 10 deletions src/main/java/org/apache/sling/engine/impl/parameters/MultipartRequestParameter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@

import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.io.UnsupportedEncodingException;

import org.apache.commons.fileupload.disk.DiskFileItem;
import jakarta.servlet.http.Part;

/**
* The <code>MultipartRequestParameter</code> represents a request parameter
Expand All @@ -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();
Copy link
Copy Markdown
Contributor

@sagarmiglani sagarmiglani May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@enapps-enorman enapps-enorman May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll change this to a try-with-resources block to make sure the steam is closed.

I think caching the bytes would be redundant and a premature optimization? And what if the file is really big? Reading the stream on demand seems safer and lets the container spool it from memory or disk.

} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

public String getContentType() {
Expand All @@ -72,8 +78,8 @@ public InputStream getInputStream() throws IOException {
}

public String getFileName() {
if (this.encodedFileName == null && this.delegatee.getName() != null) {
String tmpFileName = this.delegatee.getName();
if (this.encodedFileName == null && this.delegatee.getSubmittedFileName() != null) {
String tmpFileName = this.delegatee.getSubmittedFileName();
if (this.getEncoding() != null) {
try {
byte[] rawName = tmpFileName.getBytes(Util.ENCODING_DIRECT);
Expand Down Expand Up @@ -117,15 +123,16 @@ public String getString() {
return this.cachedValue;
}

return this.delegatee.getString();
return new String(this.get());
Copy link
Copy Markdown
Contributor

@sagarmiglani sagarmiglani May 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now uses the JVM platform default charset. Same upload returns different strings on Linux UTF-8 vs Windows.
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

Copy link
Copy Markdown
Contributor Author

@enapps-enorman enapps-enorman May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was confused by the comment above that that said "only apply encoding in the case of a form field". If the DiskFileItem did do some other encoding, then I can do the same there. But I am not sure what the practical use case is for needing the file content as a string.

}

public String getString(String enc) throws UnsupportedEncodingException {
return new String(this.get(), enc);
}

public boolean isFormField() {
return this.delegatee.isFormField();
// If getSubmittedFileName() returns null, it is likely a regular form field
return this.delegatee.getSubmittedFileName() == null;
}

public String toString() {
Expand Down
13 changes: 8 additions & 5 deletions src/main/java/org/apache/sling/engine/impl/parameters/ParameterMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ void renameParameter(String oldName, String newName) {
}

super.put(newName, params);
this.stringParameterMap = null;
}

void addParameter(RequestParameter parameter, boolean prependNew) {
Expand Down Expand Up @@ -93,10 +94,12 @@ void addParameter(RequestParameter parameter, boolean prependNew) {

// list of parameters
this.requestParameters.add(parameter);
this.stringParameterMap = null;
}

void setParameters(String name, RequestParameter[] parameters) {
super.put(name, parameters);
stringParameterMap = null;
}

// ---------- String parameter support
Expand Down Expand Up @@ -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();
Comment thread
enapps-enorman marked this conversation as resolved.
Outdated
}

// no such part
return null;
}

public Collection<?> getParts() {
final ArrayList<Part> parts = new ArrayList<Part>(this.size());
final ArrayList<Part> parts = new ArrayList<>(this.size());
for (RequestParameter[] param : this.values()) {
if (param.length >= 1 && param[0] instanceof MultipartRequestParameter) {
parts.add(new SlingPart((MultipartRequestParameter) param[0]));
if (param.length >= 1 && param[0] instanceof MultipartRequestParameter mrp) {
parts.add(mrp.getPart());
}
}
return parts;
Expand Down
Loading