Repository: jooby-project/jooby
Pull request #1309 (2019-04-05 06:02:55)

Refactored request handler to throw a better exception
Had a bit of an issue with getting bad request errors due to missing parameters on a Multipart form upload where the file size exceeded `server.http.MaxRequestSize`
Ideally would like to return a `400 Bad Request` but currently returns a `500 Error`, but the application log shows a more descriptive error, i.e. file size limit  instead of missing parameter.
If you could take a look at this and let me know how it can be improved I'd be happy to do that.
Thanks.


Patch:
diff --git a/jooby/src/main/java/org/jooby/Request.java b/jooby/src/main/java/org/jooby/Request.java
index 7232bb252..7d166f868 100644
--- a/jooby/src/main/java/org/jooby/Request.java
+++ b/jooby/src/main/java/org/jooby/Request.java
@@ -440,7 +440,13 @@ public interface Request extends Registry {
       return req.files(name);
     }
 
-    @Override
+      @Nonnull
+      @Override
+      public List<Upload> files() throws IOException {
+          return req.files();
+      }
+
+      @Override
     public Mutant header(final String name) {
       return req.header(name);
     }
@@ -1085,6 +1091,16 @@ public interface Request extends Registry {
   @Nonnull
   List<Upload> files(final String name) throws IOException;
 
+  /**
+   * Get a list of files {@link Upload} that were uploaded in the request. The request must be a POST with
+   * <code>multipart/form-data</code> content-type.
+   *
+   * @return A list of {@link Upload}.
+   * @throws IOException
+   */
+  @Nonnull
+  List<Upload> files() throws IOException;
+
   /**
    * Get a HTTP header.
    *
diff --git a/jooby/src/main/java/org/jooby/internal/RequestImpl.java b/jooby/src/main/java/org/jooby/internal/RequestImpl.java
index c501b7e15..4a4ed3542 100644
--- a/jooby/src/main/java/org/jooby/internal/RequestImpl.java
+++ b/jooby/src/main/java/org/jooby/internal/RequestImpl.java
@@ -207,19 +207,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.inject.Injector;
 import com.google.inject.Key;
 import com.typesafe.config.Config;
-import static java.util.Objects.requireNonNull;
-import org.jooby.Cookie;
-import org.jooby.Env;
-import org.jooby.Err;
-import org.jooby.MediaType;
-import org.jooby.Mutant;
-import org.jooby.Parser;
-import org.jooby.Request;
-import org.jooby.Response;
-import org.jooby.Route;
-import org.jooby.Session;
-import org.jooby.Status;
-import org.jooby.Upload;
+import org.jooby.*;
 import org.jooby.funzy.Try;
 import org.jooby.internal.parser.ParserExecutor;
 import org.jooby.spi.NativeRequest;
@@ -228,20 +216,15 @@ import org.jooby.spi.NativeUpload;
 import java.io.File;
 import java.io.IOException;
 import java.nio.charset.Charset;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Locale;
+import java.util.*;
 import java.util.Locale.LanguageRange;
-import java.util.Map;
-import java.util.Optional;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import static java.util.Objects.requireNonNull;
+
 public class RequestImpl implements Request {
 
   private final Map<String, Mutant> params = new HashMap<>();
@@ -399,6 +382,13 @@ public class RequestImpl implements Request {
     return uploads;
   }
 
+  public List<Upload> files() throws IOException {
+    return req.files()
+            .stream()
+            .map(upload -> new UploadImpl(injector, upload))
+            .collect(Collectors.toList());
+  }
+
   private Mutant _param(final String name, final Function<String, String> xss) {
     Mutant param = this.params.get(name);
     if (param == null) {
@@ -469,8 +459,7 @@ public class RequestImpl implements Request {
           Integer.toHexString(System.identityHashCode(this)));
       files.add(fbody);
       int bufferSize = conf.getBytes("server.http.RequestBufferSize").intValue();
-      Parser.BodyReference body = new BodyReferenceImpl(length, charset(), fbody, req.in(),
-          bufferSize);
+      Parser.BodyReference body = new BodyReferenceImpl(length, charset(), fbody, req.in(), bufferSize);
       return new MutantImpl(require(ParserExecutor.class), type, body);
     }
     return new MutantImpl(require(ParserExecutor.class), type, new EmptyBodyReference());
@@ -670,5 +659,4 @@ public class RequestImpl implements Request {
   private void destroySession() {
     this.reqSession = Optional.empty();
   }
-
 }
diff --git a/jooby/src/main/java/org/jooby/spi/NativeRequest.java b/jooby/src/main/java/org/jooby/spi/NativeRequest.java
index d1e8d36ed..4fdc7b523 100644
--- a/jooby/src/main/java/org/jooby/spi/NativeRequest.java
+++ b/jooby/src/main/java/org/jooby/spi/NativeRequest.java
@@ -300,6 +300,14 @@ public interface NativeRequest {
    */
   List<NativeUpload> files(String name) throws IOException;
 
+  /**
+   * Get all the files or an empty list.
+   *
+   * @return All the files or an empty list.
+   * @throws IOException If file parsing fails.
+   */
+  List<NativeUpload> files() throws IOException;
+
   /**
    * Input stream that represent the body.
    *
diff --git a/modules/jooby-netty/src/main/java/org/jooby/internal/netty/NettyRequest.java b/modules/jooby-netty/src/main/java/org/jooby/internal/netty/NettyRequest.java
index 341820d0d..939040ada 100644
--- a/modules/jooby-netty/src/main/java/org/jooby/internal/netty/NettyRequest.java
+++ b/modules/jooby-netty/src/main/java/org/jooby/internal/netty/NettyRequest.java
@@ -370,6 +370,12 @@ public class NettyRequest implements NativeRequest {
     return ImmutableList.copyOf(this.files.get(name));
   }
 
+  @Override
+  public List<NativeUpload> files() throws IOException {
+    decodeParams();
+    return ImmutableList.copyOf(this.files.values());
+  }
+
   @Override
   public InputStream in() {
     ByteBuf content = ((HttpContent) req).content();
diff --git a/modules/jooby-servlet/src/main/java/org/jooby/servlet/ServletServletRequest.java b/modules/jooby-servlet/src/main/java/org/jooby/servlet/ServletServletRequest.java
index c4943bee1..cea546e37 100644
--- a/modules/jooby-servlet/src/main/java/org/jooby/servlet/ServletServletRequest.java
+++ b/modules/jooby-servlet/src/main/java/org/jooby/servlet/ServletServletRequest.java
@@ -369,6 +369,20 @@ public class ServletServletRequest implements NativeRequest {
     }
   }
 
+  @Override
+  public List<NativeUpload> files() throws IOException {
+    try {
+      if (multipart) {
+        return req.getParts().stream()
+                .map(part -> new ServletUpload(part, tmpdir))
+                .collect(Collectors.toList());
+      }
+      return Collections.emptyList();
+    } catch (ServletException ex) {
+      throw new IOException("Unable to get files", ex);
+    }
+  }
+
   @Override
   public InputStream in() throws IOException {
     return req.getInputStream();
diff --git a/modules/jooby-undertow/src/main/java/org/jooby/internal/undertow/UndertowRequest.java b/modules/jooby-undertow/src/main/java/org/jooby/internal/undertow/UndertowRequest.java
index 86f426213..0416c868d 100644
--- a/modules/jooby-undertow/src/main/java/org/jooby/internal/undertow/UndertowRequest.java
+++ b/modules/jooby-undertow/src/main/java/org/jooby/internal/undertow/UndertowRequest.java
@@ -208,14 +208,12 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.InetAddress;
 import java.net.URLDecoder;
-import java.util.Collections;
-import java.util.Deque;
-import java.util.List;
-import java.util.Optional;
+import java.util.*;
 import java.util.concurrent.Executor;
 import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
 
-import io.undertow.server.handlers.form.FormDataParser;
+import io.undertow.server.handlers.form.*;
 import org.jooby.Cookie;
 import org.jooby.MediaType;
 import org.jooby.Router;
@@ -233,10 +231,7 @@ import com.typesafe.config.Config;
 
 import io.undertow.server.BlockingHttpExchange;
 import io.undertow.server.HttpServerExchange;
-import io.undertow.server.handlers.form.FormData;
 import io.undertow.server.handlers.form.FormData.FormValue;
-import io.undertow.server.handlers.form.FormEncodedDataDefinition;
-import io.undertow.server.handlers.form.MultiPartParserDefinition;
 import io.undertow.util.AttachmentKey;
 import io.undertow.util.HeaderValues;
 import io.undertow.util.HttpString;
@@ -357,6 +352,21 @@ public class UndertowRequest implements NativeRequest {
     return builder.build();
   }
 
+  @Override
+  public List<NativeUpload> files() throws IOException {
+    FormData formData = parseForm();
+    Iterator<String> keyIterator = formData.iterator();
+    Iterable<String> iterableKeys = () -> keyIterator;
+    List<NativeUpload> retVal = StreamSupport.stream(iterableKeys.spliterator(), true)
+            .map(formData::get)
+            .filter(formValues -> formValues.peekFirst().isFileItem())
+            .flatMap(Collection::stream)
+            .map(UndertowUpload::new)
+            .collect(Collectors.toList());
+
+    return Collections.unmodifiableList(retVal);
+  }
+
   @Override
   public InputStream in() {
     blocking.get();
@@ -410,25 +420,22 @@ public class UndertowRequest implements NativeRequest {
       try {
         String tmpdir = conf.getString("application.tmpdir");
         String charset = conf.getString("application.charset");
-        String value = exchange.getRequestHeaders().getFirst("Content-Type");
-        if (value != null) {
-          MediaType type = MediaType.valueOf(value);
-          if (MediaType.form.name().equals(type.name())) {
-            blocking.get();
-            form = new FormEncodedDataDefinition()
-                .setDefaultEncoding(charset)
-                .create(exchange)
-                .parseBlocking();
-          } else if (MediaType.multipart.name().equals(type.name())) {
-            blocking.get();
-            form = new MultiPartParserDefinition()
+
+        FormEncodedDataDefinition encodedParser = new FormEncodedDataDefinition().setDefaultEncoding(charset);
+        MultiPartParserDefinition multiPartParser = new MultiPartParserDefinition()
                 .setTempFileLocation(new File(tmpdir).toPath())
-                .setDefaultEncoding(charset)
-                .create(exchange)
-                .parseBlocking();
-          }
-        }
-      } catch (IOException x) {
+                .setDefaultEncoding(charset);
+        blocking.get();
+        FormDataParser parser = FormParserFactory
+                .builder(false)
+                .addParser(encodedParser)
+                .addParser(multiPartParser)
+                .build()
+                .createParser(exchange);
+
+        form = parser != null ? parser.parseBlocking() : NO_FORM;
+      } catch (IOException iox) {
+        throw new IllegalArgumentException("Bad Request...", iox);
       }
     }
     return form;


Test patch:
diff --git a/jooby/src/test/java/org/jooby/RequestTest.java b/jooby/src/test/java/org/jooby/RequestTest.java
index f8493197b..81d53d892 100644
--- a/jooby/src/test/java/org/jooby/RequestTest.java
+++ b/jooby/src/test/java/org/jooby/RequestTest.java
@@ -22,6 +22,8 @@ import org.junit.Test;
 import com.google.inject.Key;
 import com.google.inject.TypeLiteral;
 
+import javax.annotation.Nonnull;
+
 public class RequestTest {
   public class RequestMock implements Request {
 
@@ -241,6 +243,11 @@ public class RequestTest {
       throw new UnsupportedOperationException();
     }
 
+    @Nonnull
+    @Override
+    public List<Upload> files() throws IOException {
+      throw new UnsupportedOperationException();
+    }
   }
 
   @Test