Skip to content
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

Jump to bottom

Better interceptor structure for multipart requests #176

Merged
merged 4 commits into from
Oct 5, 2013
Merged

Better interceptor structure for multipart requests #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b31ff82
Better interceptor structure for multipart requests
garcia-jj Oct 5, 2013
3f8618c
Creating directories with Java 7 flavor
garcia-jj Oct 5, 2013
479ff6a
Code indentation, annotation and so on
garcia-jj Oct 5, 2013
2ad56ed
Reverting to old style interceptors
garcia-jj Oct 5, 2013
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
30 changes: 13 additions & 17 deletions ...n/java/br/com/caelum/vraptor/interceptor/multipart/CommonsUploadMultipartInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import br.com.caelum.vraptor.InterceptionException;
import br.com.caelum.vraptor.Intercepts;
import br.com.caelum.vraptor.Validator;
import br.com.caelum.vraptor.controller.ControllerMethod;
import br.com.caelum.vraptor.core.InterceptorStack;
import br.com.caelum.vraptor.http.InvalidParameterException;
import br.com.caelum.vraptor.http.MutableRequest;
import br.com.caelum.vraptor.interceptor.ControllerLookupInterceptor;
import br.com.caelum.vraptor.interceptor.Interceptor;
import br.com.caelum.vraptor.interceptor.ParametersInstantiatorInterceptor;
import br.com.caelum.vraptor.validator.I18nMessage;

import com.google.common.base.Strings;
Expand All @@ -53,20 +53,18 @@

/**
* A multipart interceptor based on Apache Commons Upload. Provided parameters are injected through
* RequestParameters.set and uploaded files are made available through
* {@link HttpServletRequest#setAttribute(String, Object)} and uploaded files are made available through.
*
* @author Guilherme Silveira
* @author Otávio Scherer Garcia
*/
@Intercepts(before = ControllerLookupInterceptor.class, after = {})
@Intercepts(before=ParametersInstantiatorInterceptor.class)
@RequestScoped
public class CommonsUploadMultipartInterceptor
implements MultipartInterceptor {
public class CommonsUploadMultipartInterceptor implements Interceptor {

private static final Logger logger = LoggerFactory.getLogger(CommonsUploadMultipartInterceptor.class);

private HttpServletRequest request;
private MutableRequest parameters;
private MutableRequest request;
private MultipartConfig config;
private Validator validator;
private ServletFileUploadCreator fileUploadCreator;
Expand All @@ -79,10 +77,9 @@ public CommonsUploadMultipartInterceptor() {
}

@Inject
public CommonsUploadMultipartInterceptor(HttpServletRequest request, MutableRequest parameters, MultipartConfig cfg,
Validator validator, ServletFileUploadCreator fileUploadCreator) {
public CommonsUploadMultipartInterceptor(MutableRequest request, MultipartConfig cfg, Validator validator,
ServletFileUploadCreator fileUploadCreator) {
this.request = request;
this.parameters = parameters;
this.validator = validator;
this.config = cfg;
this.fileUploadCreator = fileUploadCreator;
Expand All @@ -95,10 +92,9 @@ public CommonsUploadMultipartInterceptor(HttpServletRequest request, MutableRequ
public boolean accepts(ControllerMethod method) {
return ServletFileUpload.isMultipartContent(request);
}

@Override
public void intercept(InterceptorStack stack, ControllerMethod method, Object instance)
throws InterceptionException {
public void intercept(InterceptorStack stack, ControllerMethod method, Object controllerInstance) {
logger.info("Request contains multipart data. Try to parse with commons-upload.");

FileItemFactory factory = createFactoryForDiskBasedFileItems(config.getDirectory());
Expand Down Expand Up @@ -132,7 +128,7 @@ public void intercept(InterceptorStack stack, ControllerMethod method, Object in

for (String paramName : params.keySet()) {
Collection<String> paramValues = params.get(paramName);
parameters.setParameter(paramName, paramValues.toArray(new String[paramValues.size()]));
request.setParameter(paramName, paramValues.toArray(new String[paramValues.size()]));
}

} catch (final SizeLimitExceededException e) {
Expand All @@ -143,7 +139,7 @@ public void intercept(InterceptorStack stack, ControllerMethod method, Object in
+ "or someone is not sending a RFC1867 compatible multipart request.", e);
}

stack.next(method, instance);
stack.next(method, controllerInstance);
}

private boolean isNotEmpty(FileItem item) {
Expand All @@ -163,7 +159,7 @@ protected void reportSizeLimitExceeded(final SizeLimitExceededException e) {
protected void processFile(FileItem item, String name) {
try {
UploadedFile upload = new DefaultUploadedFile(item.getInputStream(), item.getName(), item.getContentType(), item.getSize());
parameters.setParameter(name, name);
request.setParameter(name, name);
request.setAttribute(name, upload);

logger.debug("Uploaded file: {} with {}", name, upload);
Expand Down
53 changes: 39 additions & 14 deletions ...ore/src/main/java/br/com/caelum/vraptor/interceptor/multipart/DefaultMultipartConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

import javax.enterprise.context.ApplicationScoped;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.Throwables;

/**
* Default implementation for {@link MultipartConfig}.
*
Expand All @@ -38,28 +43,48 @@ public class DefaultMultipartConfig implements MultipartConfig {

private final Logger logger = LoggerFactory.getLogger(DefaultMultipartConfig.class);

@Override
public long getSizeLimit() {
return 2 * 1024 * 1024;
}

@Override
public File getDirectory() {
try {
File tempFile = createTempFile();
tempFile.delete();
return tempFile.getParentFile();
} catch (IOException e) {
logger.warn("Unable to find temp directory, creating a dir inside the application", e);
File tmp = createDirInsideApplication();
tmp.mkdirs();
return tmp;
Path tmp = getTemporaryDirectory();
if (tmp == null) {
tmp = createDirInsideApplication();
}

return tmp.toFile();
}

protected File createDirInsideApplication() {
return new File(".tmp-multipart-upload");
protected Path getTemporaryDirectory() {
try {
Path tmp = Files.createTempFile("vraptor", "upload");
Path parent = tmp.getParent();
logger.debug("Using temporary directory as {}", parent);

Files.delete(tmp);
return parent;
} catch (IOException e) {
logger.warn("Unable to find temp directory", e);
return null;
}
}

protected File createTempFile() throws IOException {
return File.createTempFile("raptor.", ".upload");

protected Path createDirInsideApplication() {
Copy link
Member

Choose a reason for hiding this comment

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

why not temp file? If the deploy is via war, in some servers it won't work. Temp files should always work (except when you are on GAE or other PaaS that doesn't allow files )

Copy link
Member Author

Choose a reason for hiding this comment

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

it's only a typo, see the method getTemporaryDirectory that creates a temp file. I'll change the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lucascs, please see the method getTemporaryDirectory that creates a temp file, then return the parent. createDirInsideApplication it's a fallback method that creates a dir inside application when I can't file temp dir. This method is the same in current version on master. I didn't changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the old interceptor will always be faster... It's a direct a call
against a dynamic call... I do not see many ways to improve..

On Sat, Oct 5, 2013 at 1:04 PM, Otávio Garcia notifications@gitpro.ttaallkk.topwrote:

In
vraptor-core/src/main/java/br/com/caelum/vraptor/interceptor/multipart/DefaultMultipartConfig.java:

}

  • protected File createTempFile() throws IOException {
  •   return File.createTempFile("raptor.", ".upload");
  • protected Path createDirInsideApplication() {

@lucascs https://github.com/lucascs, please see the method
getTemporaryDirectory that creates a temp file, then return the parent.
createDirInsideApplication it's a fallback method that creates a dir
inside application when I can't file temp dir. This method is the same in
current version on master. I didn't changed.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/176/files#r6784155
.

Copy link
Member

Choose a reason for hiding this comment

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

@asouza I didn't understand your comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lucascs, I think that @asouza comment is about that old style interceptors are too faster than new style with annotations.

logger.debug("Creating a dir inside the application");

Path path = Paths.get(".tmp-multipart-upload");

try {
path = Files.createDirectories(path);
logger.debug("Using temporary directory as {}", path);
} catch (IOException e) {
logger.error("Unable to use temp directory inside application", e);
throw Throwables.propagate(e);
}

return path;
}
}
1 change: 1 addition & 0 deletions ...ain/java/br/com/caelum/vraptor/interceptor/multipart/DefaultServletFileUploadCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
public class DefaultServletFileUploadCreator
implements ServletFileUploadCreator {

@Override
public ServletFileUpload create(FileItemFactory fileItemFactory) {
return new ServletFileUpload(fileItemFactory);
}
Expand Down
7 changes: 5 additions & 2 deletions ...r-core/src/main/java/br/com/caelum/vraptor/interceptor/multipart/DefaultUploadedFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ public class DefaultUploadedFile implements UploadedFile {

private final long size;

public DefaultUploadedFile(InputStream content, String completeFileName,
String contentType, long size) {
public DefaultUploadedFile(InputStream content, String completeFileName, String contentType, long size) {
this.content = content;
this.fileName = REGEX_REMOVE_SLASHES.matcher(completeFileName).replaceAll("$1");
this.completeFileName = completeFileName;
Expand All @@ -52,14 +51,17 @@ public String toString() {
+ " contentType=" + this.contentType + "]";
}

@Override
public String getContentType() {
return this.contentType;
}

@Override
public InputStream getFile() {
return content;
}

@Override
public String getFileName() {
return this.fileName;
}
Expand All @@ -68,6 +70,7 @@ public String getCompleteFileName() {
return this.completeFileName;
}

@Override
public long getSize() {
return size;
}
Expand Down
33 changes: 0 additions & 33 deletions ...-core/src/main/java/br/com/caelum/vraptor/interceptor/multipart/MultipartInterceptor.java
View file
Open in desktop

This file was deleted.

20 changes: 13 additions & 7 deletions ...e/src/main/java/br/com/caelum/vraptor/interceptor/multipart/NullMultipartInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,23 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import br.com.caelum.vraptor.InterceptionException;
import br.com.caelum.vraptor.Intercepts;
import br.com.caelum.vraptor.controller.ControllerMethod;
import br.com.caelum.vraptor.core.InterceptorStack;
import br.com.caelum.vraptor.interceptor.Interceptor;
import br.com.caelum.vraptor.interceptor.ParametersInstantiatorInterceptor;

/**
* A null implementation of {@link MultipartInterceptor}.
* A null implementation of {@link MultipartInterceptor}. This interceptor will be activated when
* no commons-fileupload was found in classpath. If application try to upload any files, this
* interceptor will warn a message in console.
*
* @author Otávio Scherer Garcia
* @since 3.1.3
*/
@Intercepts(before=ParametersInstantiatorInterceptor.class)
@RequestScoped
public class NullMultipartInterceptor implements MultipartInterceptor {
public class NullMultipartInterceptor implements Interceptor {

private static final Logger logger = LoggerFactory.getLogger(NullMultipartInterceptor.class);

Expand All @@ -50,17 +55,18 @@ public NullMultipartInterceptor() {
public NullMultipartInterceptor(HttpServletRequest request) {
this.request = request;
}

/**
* Only accepts multipart requests.
*/
@Override
public boolean accepts(ControllerMethod method) {
return request.getMethod().toUpperCase().equals("POST") &&
nullToEmpty(request.getContentType()).startsWith("multipart/form-data");
}

public void intercept(InterceptorStack stack, ControllerMethod method, Object controllerInstance)
throws InterceptionException {
@Override
public void intercept(InterceptorStack stack, ControllerMethod method, Object controllerInstance) {
logger.warn("There is no file upload handlers registered. If you are willing to upload a file, please "
+ "add the commons-fileupload in your classpath");
stack.next(method, controllerInstance);
Expand Down
1 change: 1 addition & 0 deletions ...core/src/main/java/br/com/caelum/vraptor/interceptor/multipart/UploadedFileConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public UploadedFileConverter(HttpServletRequest request) {
this.request = request;
}

@Override
public UploadedFile convert(String value, Class<? extends UploadedFile> type) {
Object upload = request.getAttribute(value);
return upload == null ? null : type.cast(upload);
Expand Down
Loading