Repository: tipsy/javalin
Pull request #1336 (2021-08-09 17:30:25)

[context] Rework JSON mapping

tipsy: > looks LGTM - thanks for picking this up so quickly @tipsy

No problem, it was a nice find !

Patch:
diff --git a/javalin/src/main/java/io/javalin/core/util/LogUtil.kt b/javalin/src/main/java/io/javalin/core/util/LogUtil.kt
index a3df2463..c614f7ac 100644
--- a/javalin/src/main/java/io/javalin/core/util/LogUtil.kt
+++ b/javalin/src/main/java/io/javalin/core/util/LogUtil.kt
@@ -60,6 +60,7 @@ object LogUtil {
         return when {
             gzipped -> "Body is gzipped (${resBody.length} bytes, not logged)"
             brotlied -> "Body is brotlied (${resBody.length} bytes, not logged)"
+            resBody.contains("resultString unavailable") -> "Body is an InputStream which can't be reset, so it can't be logged"
             else -> "Body is ${resBody.length} bytes (starts on next line):\n    $resBody"
         }
     }
diff --git a/javalin/src/main/java/io/javalin/http/Context.kt b/javalin/src/main/java/io/javalin/http/Context.kt
index af945290..36e16518 100644
--- a/javalin/src/main/java/io/javalin/http/Context.kt
+++ b/javalin/src/main/java/io/javalin/http/Context.kt
@@ -15,7 +15,6 @@ import io.javalin.http.util.ContextUtil.throwPayloadTooLargeIfPayloadTooLarge
 import io.javalin.http.util.CookieStore
 import io.javalin.http.util.MultipartUtil
 import io.javalin.http.util.SeekableWriter
-import io.javalin.plugin.json.canReadStream
 import io.javalin.plugin.json.jsonMapper
 import io.javalin.plugin.rendering.JavalinRenderer
 import java.io.InputStream
@@ -110,12 +109,17 @@ open class Context(@JvmField val req: HttpServletRequest, @JvmField val res: Htt
     fun bodyAsBytes(): ByteArray = body
 
     /** Maps a JSON body to a Java/Kotlin class using the registered [io.javalin.plugin.json.JsonMapper] */
-    fun <T> bodyAsClass(clazz: Class<T>): T =
-        jsonMapper().let { if (it.canReadStream()) it.fromJsonStream(req.inputStream, clazz)!! else it.fromJsonString(body(), clazz)!! }
+    fun <T> bodyAsClass(clazz: Class<T>): T = jsonMapper().fromJsonString(body(), clazz)
 
     /** Reified version of [bodyAsClass] (Kotlin only) */
     inline fun <reified T : Any> bodyAsClass(): T = bodyAsClass(T::class.java)
 
+    /** Maps a JSON body to a Java/Kotlin class using the registered [io.javalin.plugin.json.JsonMapper] */
+    fun <T> bodyStreamAsClass(clazz: Class<T>): T = jsonMapper().fromJsonStream(req.inputStream, clazz)
+
+    /** Reified version of [bodyStreamAsClass] (Kotlin only) */
+    inline fun <reified T : Any> bodyStreamAsClass(): T = bodyStreamAsClass(T::class.java)
+
     /** Gets the request body as a [InputStream] */
     fun bodyAsInputStream(): InputStream = req.inputStream
 
@@ -328,8 +332,8 @@ open class Context(@JvmField val req: HttpServletRequest, @JvmField val res: Htt
      */
     fun result(resultBytes: ByteArray) = result(resultBytes.inputStream())
 
-    /** Gets the current context result as a [String] (if set). */
-    fun resultString() = resultStream?.apply { reset() }?.readBytes()?.toString(responseCharset()).also { resultStream?.reset() }
+    /** Gets the current [resultStream] as a [String] (if possible), and reset [resultStream] */
+    fun resultString() = ContextUtil.readAndResetStreamIfPossible(resultStream, responseCharset())
 
     /**
      * Sets context result to the specified [InputStream].
@@ -431,10 +435,13 @@ open class Context(@JvmField val req: HttpServletRequest, @JvmField val res: Htt
      * Serializes object to a JSON-string using the registered [io.javalin.plugin.json.JsonMapper] and sets it as the context result.
      * Also sets content type to application/json.
      */
-    @JvmOverloads
-    fun json(obj: Any, useStreamingMapper: Boolean = false): Context = contentType("application/json").also {
-        jsonMapper().let { if (useStreamingMapper) result(it.toJsonStream(obj)) else result(it.toJsonString(obj)) }
-    }
+    fun json(obj: Any): Context = result(jsonMapper().toJsonString(obj)).contentType("application/json")
+
+    /**
+     * Serializes object to a JSON-stream using the registered [io.javalin.plugin.json.JsonMapper] and sets it as the context result.
+     * Also sets content type to application/json.
+     */
+    fun jsonStream(obj: Any): Context = result(jsonMapper().toJsonStream(obj)).contentType("application/json")
 
     /**
      * Renders a file with specified values and sets it as the context result.
diff --git a/javalin/src/main/java/io/javalin/http/util/ContextUtil.kt b/javalin/src/main/java/io/javalin/http/util/ContextUtil.kt
index aa86daf0..f9bfe74b 100644
--- a/javalin/src/main/java/io/javalin/http/util/ContextUtil.kt
+++ b/javalin/src/main/java/io/javalin/http/util/ContextUtil.kt
@@ -14,8 +14,10 @@ import io.javalin.http.HandlerEntry
 import io.javalin.http.HandlerType
 import io.javalin.http.HttpCode
 import io.javalin.http.HttpResponseException
+import java.io.InputStream
 import java.net.URL
 import java.net.URLDecoder
+import java.nio.charset.Charset
 import java.util.*
 import javax.servlet.http.HttpServletRequest
 import javax.servlet.http.HttpServletResponse
@@ -38,13 +40,13 @@ object ContextUtil {
 
     fun splitKeyValueStringAndGroupByKey(string: String, charset: String): Map<String, List<String>> {
         return if (string.isEmpty()) mapOf() else string.split("&").map { it.split("=", limit = 2) }.groupBy(
-                { URLDecoder.decode(it[0], charset) },
-                { if (it.size > 1) URLDecoder.decode(it[1], charset) else "" }
+            { URLDecoder.decode(it[0], charset) },
+            { if (it.size > 1) URLDecoder.decode(it[1], charset) else "" }
         ).mapValues { it.value.toList() }
     }
 
     fun pathParamOrThrow(pathParams: Map<String, String?>, key: String, url: String) =
-            pathParams[key.replaceFirst("{", "").replaceFirst("}", "")] ?: throw IllegalArgumentException("'$key' is not a valid path-param for '$url'.")
+        pathParams[key.replaceFirst("{", "").replaceFirst("}", "")] ?: throw IllegalArgumentException("'$key' is not a valid path-param for '$url'.")
 
     fun urlDecode(s: String): String = URLDecoder.decode(s.replace("+", "%2B"), "UTF-8").replace("%2B", "+")
 
@@ -66,12 +68,12 @@ object ContextUtil {
     @JvmStatic
     @JvmOverloads
     fun init(
-            request: HttpServletRequest,
-            response: HttpServletResponse,
-            matchedPath: String = "*",
-            pathParamMap: Map<String, String> = mapOf(),
-            handlerType: HandlerType = HandlerType.INVALID,
-            appAttributes: Map<String, Any> = mapOf()
+        request: HttpServletRequest,
+        response: HttpServletResponse,
+        matchedPath: String = "*",
+        pathParamMap: Map<String, String> = mapOf(),
+        handlerType: HandlerType = HandlerType.INVALID,
+        appAttributes: Map<String, Any> = mapOf()
     ) = Context(request, response, appAttributes).apply {
         this.matchedPath = matchedPath
         this.pathParamMap = pathParamMap
@@ -114,4 +116,10 @@ object ContextUtil {
         return req.getAttribute("$sessionCacheKeyPrefix$key") as T?
     }
 
+    fun readAndResetStreamIfPossible(stream: InputStream?, charset: Charset) = try {
+        stream?.apply { reset() }?.readBytes()?.toString(charset).also { stream?.reset() }
+    } catch (e: Exception) {
+        "resultString unavailable (resultStream couldn't be reset)"
+    }
+
 }
diff --git a/javalin/src/main/java/io/javalin/plugin/json/JsonMapper.kt b/javalin/src/main/java/io/javalin/plugin/json/JsonMapper.kt
index 778f776f..979f9fdc 100644
--- a/javalin/src/main/java/io/javalin/plugin/json/JsonMapper.kt
+++ b/javalin/src/main/java/io/javalin/plugin/json/JsonMapper.kt
@@ -13,4 +13,3 @@ import java.io.InputStream
 const val JSON_MAPPER_KEY = "global-json-mapper"
 fun Javalin.jsonMapper(): JsonMapper = this.attribute(JSON_MAPPER_KEY)
 fun Context.jsonMapper(): JsonMapper = this.appAttribute(JSON_MAPPER_KEY)
-fun JsonMapper.canReadStream() = !this::class.java.getMethod("fromJsonStream", InputStream::class.java, Class::class.java).isDefault


Test patch:
diff --git a/javalin/src/test/java/io/javalin/TestJson.kt b/javalin/src/test/java/io/javalin/TestJson.kt
index 827a235d..00e41cda 100644
--- a/javalin/src/test/java/io/javalin/TestJson.kt
+++ b/javalin/src/test/java/io/javalin/TestJson.kt
@@ -37,20 +37,20 @@ class TestJson {
     }
 
     @Test
-    fun `large streamed object doesn't deadlock`() = TestUtil.test { app, http ->
+    fun `default mapper doesn't deadlock when streaming large objects`() = TestUtil.test { app, http ->
         val big = mapOf("big" to "1".repeat(100_000))
-        app.get("/") { it.json(big, useStreamingMapper = true) }
+        app.get("/") { it.jsonStream(big) }
         assertThat(http.getBody("/")).isEqualTo(JavalinJackson().toJsonString(big))
     }
 
     @Test
-    fun `json-mapper throws when mapping unmappable object to json`() = TestUtil.test { app, http ->
-        app.get("/streaming") { it.json(NonSerializableObject(), useStreamingMapper = true) }
+    fun `default mapper throws when mapping unmappable object to json`() = TestUtil.test { app, http ->
+        app.get("/streaming") { it.jsonStream(NonSerializableObject()) }
         http.get("/streaming").let {
             assertThat(it.status).isEqualTo(500)
             assertThat(it.body).isEqualTo("") // error happens when writing the response, can't recover
         }
-        app.get("/string") { it.json(NonSerializableObject(), useStreamingMapper = false) }
+        app.get("/string") { it.json(NonSerializableObject()) }
         http.get("/string").let {
             assertThat(it.status).isEqualTo(500)
             assertThat(it.body).contains("Internal server error") // error happens when serializing, can recover
@@ -58,13 +58,13 @@ class TestJson {
     }
 
     @Test
-    fun `json-mapper maps json to object`() = TestUtil.test { app, http ->
+    fun `default mapper mapper maps json to object`() = TestUtil.test { app, http ->
         app.post("/") { it.result(it.bodyAsClass<SerializableObject>().value1) }
         assertThat(http.post("/").body(serializableObjectString).asString().body).isEqualTo("FirstValue")
     }
 
     @Test
-    fun `mapping invalid json to class throws`() = TestUtil.test { app, http ->
+    fun `default mapper throws when mapping invalid json to class`() = TestUtil.test { app, http ->
         app.get("/") { it.bodyAsClass<NonSerializableObject>() }
         assertThat(http.get("/").status).isEqualTo(500)
     }
@@ -76,17 +76,6 @@ class TestJson {
         assertThat(http.getBody("/")).isEqualTo("""{"NonSerializableObject":[{"message":"DESERIALIZATION_FAILED","args":{},"value":""}]}""")
     }
 
-    @Test
-    fun `custom silly JSON mapper works`() {
-        val sillyMapper = object : JsonMapper {
-            override fun toJsonString(obj: Any): String = "Silly mapper"
-        }
-        TestUtil.test(Javalin.create { it.jsonMapper(sillyMapper) }) { app, http ->
-            app.get("/") { ctx -> ctx.json("Test") }
-            assertThat(http.getBody("/")).isEqualTo("Silly mapper")
-        }
-    }
-
     @Test
     fun `empty mapper throws exception`() = TestUtil.test(Javalin.create { it.jsonMapper(object : JsonMapper {}) }) { app, http ->
         app.get("/") { ctx -> ctx.json("Test") }
@@ -95,17 +84,25 @@ class TestJson {
     }
 
     @Test
-    fun `empty mapper logs proper error message`() = TestUtil.test(Javalin.create { it.jsonMapper(object : JsonMapper {}) }) { app, http ->
-        val log = TestUtil.captureStdOut {
-            JavalinLogger.enabled = true
-            app.get("/") { it.json("Test") }
-            http.getBody("/")
-        }
+    fun `empty mapper logs proper error messages`() = TestUtil.test(Javalin.create { it.jsonMapper(object : JsonMapper {}) }) { app, http ->
+        JavalinLogger.enabled = true
+
+        var log = TestUtil.captureStdOut { app.get("/write-string") { it.json("") }.also { http.getBody("/write-string") } }
         assertThat(log).contains("JsonMapper#toJsonString not implemented")
+
+        log = TestUtil.captureStdOut { app.get("/write-stream") { it.jsonStream("") }.also { http.getBody("/write-stream") } }
+        assertThat(log).contains("JsonMapper#toJsonStream not implemented")
+
+        log = TestUtil.captureStdOut { app.get("/read-string") { it.bodyAsClass<String>() }.also { http.getBody("/read-string") } }
+        assertThat(log).contains("JsonMapper#fromJsonString not implemented")
+
+        log = TestUtil.captureStdOut { app.get("/read-stream") { it.bodyStreamAsClass<String>() }.also { http.getBody("/read-stream") } }
+        assertThat(log).contains("JsonMapper#fromJsonStream not implemented")
     }
 
+
     @Test
-    fun `mapper uses toJsonString if only string available`() {
+    fun `user can configure custom toJsonString`() {
         val sillyMapper = object : JsonMapper {
             override fun toJsonString(obj: Any): String = "toJsonString"
         }
@@ -116,19 +113,18 @@ class TestJson {
     }
 
     @Test
-    fun `mapper uses toJsonStream if string and stream available`() {
+    fun `user can configure custom toJsonStream`() {
         val sillyMapper = object : JsonMapper {
-            override fun toJsonString(obj: Any): String = "toJsonString"
             override fun toJsonStream(obj: Any): InputStream = "toJsonStream".byteInputStream()
         }
         TestUtil.test(Javalin.create { it.jsonMapper(sillyMapper) }) { app, http ->
-            app.get("/") { it.json(SerializableObject(), useStreamingMapper = true) }
+            app.get("/") { it.jsonStream(SerializableObject()) }
             assertThat(http.get("/").body).isEqualTo("toJsonStream")
         }
     }
 
     @Test
-    fun `mapper uses fromJsonString if only string available`() {
+    fun `user can configure custom fromJsonString`() {
         val sillyMapper = object : JsonMapper {
             override fun <T : Any?> fromJsonString(json: String, targetClass: Class<T>): T = "fromJsonString" as T
         }
@@ -139,19 +135,18 @@ class TestJson {
     }
 
     @Test
-    fun `mapper uses fromJsonStream if string and stream available`() {
+    fun `user can configure custom fromJsonStream`() {
         val sillyMapper = object : JsonMapper {
-            override fun <T : Any?> fromJsonString(json: String, targetClass: Class<T>): T = "fromJsonString" as T
             override fun <T : Any?> fromJsonStream(json: InputStream, targetClass: Class<T>): T = "fromJsonStream" as T
         }
         TestUtil.test(Javalin.create { it.jsonMapper(sillyMapper) }) { app, http ->
-            app.get("/") { it.result(it.bodyAsClass<String>()) }
+            app.get("/") { it.result(it.bodyStreamAsClass<String>()) }
             assertThat(http.get("/").body).isEqualTo("fromJsonStream")
         }
     }
 
     @Test
-    fun `custom GSON mapper works`() {
+    fun `user can use GSON`() {
         val gson = GsonBuilder().create()
         val gsonMapper = object : JsonMapper {
             override fun <T> fromJsonString(json: String, targetClass: Class<T>): T = gson.fromJson(json, targetClass)
diff --git a/javalin/src/test/java/io/javalin/TestLogging.kt b/javalin/src/test/java/io/javalin/TestLogging.kt
index 03a6aca8..75a77d90 100644
--- a/javalin/src/test/java/io/javalin/TestLogging.kt
+++ b/javalin/src/test/java/io/javalin/TestLogging.kt
@@ -7,7 +7,9 @@
 package io.javalin
 
 import com.mashape.unirest.http.Unirest
+import io.javalin.core.util.JavalinLogger
 import io.javalin.testing.HttpUtil
+import io.javalin.testing.NonSerializableObject
 import io.javalin.testing.TestLoggingUtil.captureStdOut
 import io.javalin.testing.TestUtil
 import org.assertj.core.api.Assertions.assertThat
@@ -33,6 +35,16 @@ class TestLogging {
         assertThat(log).contains("Hello Async World!")
     }
 
+    @Test
+    fun `dev logging works with inputstreams`() = TestUtil.test(Javalin.create { it.enableDevLogging() }) { app, http ->
+        JavalinLogger.enabled = true
+        val fileStream = TestLogging::class.java.getResourceAsStream("/public/file")
+        app.get("/") { it.result(fileStream) }
+        val log = captureStdOut { http.getBody("/") }
+        assertThat(log).doesNotContain("Stream closed")
+        assertThat(log).contains("Body is an InputStream which can't be reset, so it can't be logged")
+    }
+
     @Test
     fun `custom requestlogger is called`() {
         var loggerCalled = false
@@ -73,14 +85,14 @@ class TestLogging {
     }
 
     @Test
-    fun `debug logging works with binary stream`() {
-        val app = Javalin.create { it.enableDevLogging() }.start(0)
+    fun `debug logging works with binary stream`() = TestUtil.test(Javalin.create { it.enableDevLogging() }) { app, http ->
+        JavalinLogger.enabled = true
         app.get("/") {
             val imagePath = this::class.java.classLoader.getResource("upload-test/image.png")
             val stream = File(imagePath.toURI()).inputStream()
             it.result(stream)
         }
-        val log = captureStdOut { Unirest.get("http://localhost:" + app.port() + "/").asString() }
+        val log = captureStdOut { http.getBody("/") }
         assertThat(log).contains("Body is binary (not logged)")
     }
 }