Repository: tipsy/javalin (kotlin)

Issue:
Set fileSizeThreshold MultipartConfigElement so large file size uploads will not exhaust memory
**Actual behavior (the bug)**
I was working on an API for performing large fileuploads and discovered that the server would run out of memory on very large files (3gb or higher).  I found out it is due for the MultipartConfigElement does not have a fileSizeThreshold set.


**Expected behavior**
I would have expected that on very large files that the server would start writing data to the System.getProperty("java.io.tmpdir") directory instead of exhausting memory.

**To Reproduce**
Upload a file over 3gb as a multipart upload.

**Additional context**

In my controller I was able to workaround this by defining the multipart configuration myself with a valide for fileSizeThreadhold 

 ctx.req.setAttribute("org.eclipse.jetty.multipartConfig", new MultipartConfigElement(System.getProperty("java.io.tmpdir"), -1,-1,20000000));
 
and then getting the the file directory using getPart.

ctx.req.getPart("dataFile")

instead of using ctx.uploadedFile("dataFile")

Pull request:
Fix for issues mentioned in https://github.com/tipsy/javalin/issues/1045 (uploading large files bloats JVM heap).

**Issue breakdown**
On each access to `uploadedFiles` methods, underlying `MultipartUtil` calls `preUploadFunction`. Under the hood it executes:
```java
req.setAttribute("org.eclipse.jetty.multipartConfig", MultipartConfigElement(System.getProperty("java.io.tmpdir")))
```
which in the long form looks something like this
```java
req.setAttribute("org.eclipse.jetty.multipartConfig", MultipartConfigElement(System.getProperty("java.io.tmpdir"), -1, -1, 0))
```
In your discussions with @Mynameisbt you were incorrect in assumption that `MultipartConfigElement`'s `fileSizeThreshold`, when set to `0`, always caches to disk. 

Unfortunately this is not the case and `0` actually disables caching behavoir:
```java
# Class org.eclipse.jetty.util.MultiPartInputStreamParser write methods

protected void write(int b) throws IOException {
    if (MultiPartInputStreamParser.this._config.getMaxFileSize() > 0L && this._size + 1L > MultiPartInputStreamParser.this._config.getMaxFileSize()) {
        throw new IllegalStateException("Multipart Mime part " + this._name + " exceeds max filesize");
    } else {
        if (MultiPartInputStreamParser.this._config.getFileSizeThreshold() > 0 && this._size + 1L > (long)MultiPartInputStreamParser.this._config.getFileSizeThreshold() && this._file == null) {
            this.createFile();
        }
        this._out.write(b);
        ++this._size;
    }
}
protected void write(byte[] bytes, int offset, int length) throws IOException {
    if (MultiPartInputStreamParser.this._config.getMaxFileSize() > 0L && this._size + (long)length > MultiPartInputStreamParser.this._config.getMaxFileSize()) {
        throw new IllegalStateException("Multipart Mime part " + this._name + " exceeds max filesize");
    } else {
        if (MultiPartInputStreamParser.this._config.getFileSizeThreshold() > 0 && this._size + (long)length > (long)MultiPartInputStreamParser.this._config.getFileSizeThreshold() && this._file == null) {
            this.createFile();
        }
        this._out.write(bytes, offset, length);
        this._size += (long)length;
    }
}

```

First condition `MultiPartInputStreamParser.this._config.getMaxFileSize() > 0L` is always `false` since `maxFileSize` was set to `-1`. 
So we end up in `else` branch where `MultiPartInputStreamParser.this._config.getFileSizeThreshold() > 0` is also `false` as `fileSizeThreashold` was set to `0`. 

This poses a big issue, since it's not possible to properly set up `org.eclipse.jetty.multipartConfig` anywhere and use `uploadFiles` methods. The only workaround is opt for `HttpServletRequest` api directly.

**The fix**
This PR addresses this issue by first checking if `org.eclipse.jetty.multipartConfig` is already set in request:

- If `true` - do nothing as developers already took care of proper limits
- If `false` - retained original behavior (although we can discuss and change to `MultipartConfigElement(System.getProperty("java.io.tmpdir"), -1, -1, 1)` which would enable disk caching as soon as 1 byte is received

**Testing**
Created test `custom multipart properties applied correctly` in `TestMultipartForms` which demonstrates that `org.eclipse.jetty.multipartConfig` set by developer now takes precedence over default behavior.

fixes #1045

Patch:
diff --git a/javalin/src/main/java/io/javalin/http/util/MultipartUtil.kt b/javalin/src/main/java/io/javalin/http/util/MultipartUtil.kt
index ada7a22b8..a9a5edbcf 100644
--- a/javalin/src/main/java/io/javalin/http/util/MultipartUtil.kt
+++ b/javalin/src/main/java/io/javalin/http/util/MultipartUtil.kt
@@ -13,9 +13,14 @@ import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.Part
 
 object MultipartUtil {
+    private const val MULTIPART_CONFIG_ATTRIBUTE = "org.eclipse.jetty.multipartConfig";
 
     var preUploadFunction: (HttpServletRequest) -> Unit = { req ->
-        req.setAttribute("org.eclipse.jetty.multipartConfig", MultipartConfigElement(System.getProperty("java.io.tmpdir")))
+        val existingConfig = req.getAttribute(MULTIPART_CONFIG_ATTRIBUTE)
+        if (existingConfig == null) {
+            req.setAttribute(MULTIPART_CONFIG_ATTRIBUTE,
+                    MultipartConfigElement(System.getProperty("java.io.tmpdir"), -1, -1, 1))
+        }
     }
 
     fun getUploadedFiles(req: HttpServletRequest, partName: String): List<UploadedFile> {


Test patch:
diff --git a/javalin/src/test/java/io/javalin/TestMultipartForms.kt b/javalin/src/test/java/io/javalin/TestMultipartForms.kt
index 7f6d27027..415f9ca6d 100644
--- a/javalin/src/test/java/io/javalin/TestMultipartForms.kt
+++ b/javalin/src/test/java/io/javalin/TestMultipartForms.kt
@@ -19,7 +19,9 @@ import org.apache.commons.io.IOUtils
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 import java.io.File
+import java.lang.IllegalStateException
 import java.nio.charset.StandardCharsets
+import javax.servlet.MultipartConfigElement
 
 class TestMultipartForms {
 
@@ -114,6 +116,27 @@ class TestMultipartForms {
         assertThat(response.body).isEqualTo("image.png, sound.mp3, text.txt")
     }
 
+    @Test
+    fun `custom multipart properties applied correctly`() = TestUtil.test { app, http ->
+        app.before("/test-upload") { ctx ->
+            ctx.attribute("org.eclipse.jetty.multipartConfig", MultipartConfigElement("/tmp", 10, 10, 5))
+        }
+
+        app.post("/test-upload") { ctx ->
+            ctx.result(ctx.uploadedFiles().joinToString(", ") { it.filename })
+        }
+
+        app.exception(IllegalStateException::class.java) { e, ctx ->
+            ctx.result("${e::class.java.canonicalName} ${e.message}")
+        }
+
+        val response = http.post("/test-upload")
+                .field("upload", File("src/test/resources/upload-test/image.png"))
+                .field("text-field", "text")
+                .asString()
+        assertThat(response.body).isEqualTo("java.lang.IllegalStateException Request exceeds maxRequestSize (10)")
+    }
+
     @Test
     fun `getting all files doesn't throw for non multipart request`() = TestUtil.test { app, http ->
         app.post("/test-upload") { ctx -> ctx.result(ctx.uploadedFiles().joinToString("\n")) }