Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
21 changes: 21 additions & 0 deletions src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
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

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

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

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

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

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

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

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..


private volatile boolean allowTrace;

private volatile ServiceRegistration<Servlet> servletRegistration;
Expand Down Expand Up @@ -181,6 +186,22 @@ 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("osgi.http.whiteboard.servlet.multipart.maxFileCount", config.request_max_file_count());

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

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.sling.engine.impl.parameters;

import java.io.IOException;

/**
* This exception is thrown if a request contains more files than the specified
* limit.
*/
public class FileCountLimitExceededException extends IOException {

private static final long serialVersionUID = 6904179610227521789L;

/**
* The limit that was exceeded.
*/
private final long limit;

/**
* Creates a new instance.
*
* @param message The detail message
* @param limit The limit that was exceeded
*/
public FileCountLimitExceededException(final String message, final long limit) {
super(message);
this.limit = limit;
}

/**
* Gets the limit that was exceeded.
*
* @return The limit that was exceeded by the request
*/
public long getLimit() {
return limit;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@

import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.io.UnsupportedEncodingException;
import java.nio.charset.StandardCharsets;

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

/**
* The <code>MultipartRequestParameter</code> represents a request parameter
Expand All @@ -34,33 +36,37 @@
*/
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, String encoding) {
super(delegatee.getName(), encoding);
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 (InputStream inStream = getInputStream()) {
return inStream.readAllBytes();
} 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,27 @@ public String getString() {
return this.cachedValue;
}

return this.delegatee.getString();
// try explicit encoding if available
String encoding = getEncoding();
if (encoding == null) {
// fallback to be used when no explicit value is provided
encoding = StandardCharsets.ISO_8859_1.name();
}
try {
return new String(this.get(), encoding);
} catch (final UnsupportedEncodingException e) {
// don't care, fall back to empty for backward compatibility
return "";
}
}

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
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 new SlingPart(mrp);
}

// 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(new SlingPart(mrp));
}
}
return parts;
Expand Down
Loading