Repository: tipsy/javalin
Pull request #1088 (2020-10-15 07:53:46)

Fix ignoreTrailingSlashes
The current ignoreTrailingSlashes implementation was buggy : it changed how path are split and leave empty segments in the list.
This PR : 
- add Tests of ignoreTrailingSlashes=false : with tests from standard javalin TestRouting 
- don't change anymore how path are split to List&lt;Segment&gt;
- adapt the match regex to take account of trailing slashes or not
- pass the tests


NPi2Loup: Failing test seems to be a maven central connection reset.
Don't known how to relaunch, need a dummy commit ?
tipsy: Thanks @NPi2Loup ! Is there any performance impact with this change?

Patch:
diff --git a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/external/KotlinOpenApiDsl.kt b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/external/KotlinOpenApiDsl.kt
index b6d17e76..5b26ba8a 100644
--- a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/external/KotlinOpenApiDsl.kt
+++ b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/external/KotlinOpenApiDsl.kt
@@ -104,9 +104,6 @@ internal data class FindSchemaResponse(
 )
 
 internal fun <T> findSchema(clazz: Class<T>): FindSchemaResponse? {
-    getEnumSchema(clazz)?.let {
-        return FindSchemaResponse(it)
-    }
     return when (clazz) {
         String::class.java -> FindSchemaResponse(StringSchema())
         Boolean::class.java -> FindSchemaResponse(BooleanSchema())
@@ -135,17 +132,6 @@ internal fun <T> findSchema(clazz: Class<T>): FindSchemaResponse? {
     }
 }
 
-internal fun <T> getEnumSchema(clazz: Class<T>): Schema<*>? {
-    val values = clazz.enumConstants ?: return null
-
-    val schema = StringSchema()
-    for (enumVal in values) {
-        schema.addEnumItem(enumVal.toString())
-    }
-    return schema
-}
-
-
 internal fun <T> MediaType.example(clazz: Class<T>, value: T, init: Example.() -> Unit) {
     if (examples == null) {
         examples = mutableMapOf()
diff --git a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JacksonToJsonMapper.kt b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JacksonToJsonMapper.kt
index bdf2f4b0..73302117 100644
--- a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JacksonToJsonMapper.kt
+++ b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JacksonToJsonMapper.kt
@@ -7,6 +7,8 @@ import com.fasterxml.jackson.databind.ser.std.ToStringSerializer
 import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
 import io.javalin.plugin.json.ToJsonMapper
 import io.javalin.plugin.openapi.utils.LazyDefaultValue
+import io.swagger.v3.core.jackson.mixin.SchemaMixin
+import io.swagger.v3.oas.models.media.Schema
 import io.swagger.v3.oas.models.security.SecurityScheme
 
 /**
@@ -28,6 +30,7 @@ class JacksonToJsonMapper(
         fun createObjectMapperWithDefaults(): ObjectMapper {
             return jacksonObjectMapper()
                     .setSerializationInclusion(JsonInclude.Include.NON_NULL)
+                    .addMixIn(Schema::class.java, SchemaMixin::class.java)
                     .registerModule(
                             SimpleModule()
                                     .addSerializer(SecurityScheme.Type::class.java, ToStringSerializer())
diff --git a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JavalinModelResolver.kt b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JavalinModelResolver.kt
index 0cf06ab1..7616431c 100644
--- a/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JavalinModelResolver.kt
+++ b/javalin-openapi/src/main/java/io/javalin/plugin/openapi/jackson/JavalinModelResolver.kt
@@ -7,6 +7,7 @@ import io.swagger.v3.core.converter.AnnotatedType
 import io.swagger.v3.core.converter.ModelConverter
 import io.swagger.v3.core.converter.ModelConverterContext
 import io.swagger.v3.core.jackson.ModelResolver
+import io.swagger.v3.core.util.PrimitiveType
 import io.swagger.v3.oas.models.media.DateTimeSchema
 import io.swagger.v3.oas.models.media.Schema
 import java.time.Instant
@@ -18,8 +19,8 @@ class JavalinModelResolver(mapper: ObjectMapper) : ModelResolver(mapper) {
         }
         val type = extractJavaType(annotatedType)
 
-        if (type.isTypeOrSubTypeOf(Instant::class.java) && !_mapper.isEnabled(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)) {
-            return DateTimeSchema()
+        if (type.isTypeOrSubTypeOf(Instant::class.java) && _mapper.isEnabled(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)) {
+            return PrimitiveType.LONG.createProperty();
         }
 
         return super.resolve(annotatedType, context, next)
diff --git a/javalin/src/main/java/io/javalin/core/PathParser.kt b/javalin/src/main/java/io/javalin/core/PathParser.kt
index 5263b71b..a1e6304e 100644
--- a/javalin/src/main/java/io/javalin/core/PathParser.kt
+++ b/javalin/src/main/java/io/javalin/core/PathParser.kt
@@ -11,7 +11,7 @@ import io.javalin.http.util.ContextUtil
 class PathParser(path: String, ignoreTrailingSlashes: Boolean) {
 
     val segments: List<PathSegment> = path.split("/")
-            .filter { !ignoreTrailingSlashes || it.isNotEmpty() } //if not ignoreTrailingSlashes process every token, else only process not empty token
+            .filter { it.isNotEmpty() }
             .map {
                 when {
                     it.startsWith(":") -> PathSegment.Parameter(it.removePrefix(":"))
@@ -22,7 +22,9 @@ class PathParser(path: String, ignoreTrailingSlashes: Boolean) {
 
     val pathParamNames = segments.filterIsInstance<PathSegment.Parameter>().map { it.name }
 
-    private val matchRegex = "^/${segments.joinToString("/") { it.asRegexString() }}/?$".toRegex()
+    //compute matchRegex suffix : if ignoreTrailingSlashes config is set we keep /?, else we use the true path trailing slash : present or absent
+    private val matchRegexSuffix = if(ignoreTrailingSlashes)"/?" else if(path.endsWith("/")) "/" else ""
+    private val matchRegex = ("^/${segments.joinToString("/") { it.asRegexString() }}"+matchRegexSuffix+"$").toRegex()
 
     private val pathParamRegex = matchRegex.pattern.replace("[^/]+?", "([^/]+?)").toRegex()
 
diff --git a/pom.xml b/pom.xml
index 1fcef978..8c1b18d5 100644
--- a/pom.xml
+++ b/pom.xml
@@ -76,7 +76,7 @@
         <pebble.version>3.1.2</pebble.version>
         <redoc.version>2.0.0-rc.23</redoc.version>
         <slf4j.version>1.7.30</slf4j.version>
-        <swagger.models.version>2.1.2</swagger.models.version>
+        <swagger.core.version>2.1.4</swagger.core.version>
         <swagger.parser.version>2.0.19</swagger.parser.version>
         <swagger.ui.version>3.25.2</swagger.ui.version>
         <thymeleaf.version>3.0.11.RELEASE</thymeleaf.version>
@@ -91,7 +91,7 @@
         <javax.servlet.api.version>3.1.0</javax.servlet.api.version>
         <junit.benchmark.version>0.7.2</junit.benchmark.version>
         <junit.version>4.13.1</junit.version>
-        <kotlin.openapi.dsl.version>0.20.2</kotlin.openapi.dsl.version>
+        <kotlin.openapi.dsl.version>1.0.0</kotlin.openapi.dsl.version>
         <mockk.version>1.10.0</mockk.version>
         <bytebuddy.version>1.10.15</bytebuddy.version>
         <bytebuddyagent.version>1.10.15</bytebuddyagent.version>
@@ -216,10 +216,15 @@
                 <artifactId>micrometer-core</artifactId>
                 <version>${micrometer.version}</version>
             </dependency>
+            <dependency>
+                <groupId>io.swagger.core.v3</groupId>
+                <artifactId>swagger-core</artifactId>
+                <version>${swagger.core.version}</version>
+            </dependency>
             <dependency>
                 <groupId>io.swagger.core.v3</groupId>
                 <artifactId>swagger-models</artifactId>
-                <version>${swagger.models.version}</version>
+                <version>${swagger.core.version}</version>
             </dependency>
             <dependency>
                 <groupId>io.swagger.parser.v3</groupId>


Test patch:
diff --git a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApi.kt b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApi.kt
index ba653790..a91cd61e 100644
--- a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApi.kt
+++ b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApi.kt
@@ -45,10 +45,15 @@ import java.time.Instant
 
 class Address(val street: String, val number: Int)
 
-class User(val name: String, val address: Address? = null)
+class User(val name: String, val address: Address? = null, val userType: UserType? = null)
 
 class Log(val timestamp: Instant, val message: String)
 
+enum class UserType{
+    ONE, TWO
+}
+
+
 fun createComplexExampleBaseConfiguration() = openapiDsl {
     info {
         title = "Example"
@@ -255,6 +260,10 @@ fun buildComplexExample(options: OpenApiOptions): Javalin {
 
     app.get("/ignored", documented(document().ignore()) {})
 
+    app.get("/enums/:my-enum-path-param", documented(document()
+            .pathParam<UserType>("my-enum-path-param")
+            .queryParam<UserType>("my-enum-query-param")) {})
+
     return app
 }
 
diff --git a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApiAnnotations.kt b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApiAnnotations.kt
index 51028471..f45c148c 100644
--- a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApiAnnotations.kt
+++ b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/TestOpenApiAnnotations.kt
@@ -294,6 +294,17 @@ fun getQueryParamListHandler(ctx: Context) {
 
 // endregion composed body
 
+@OpenApi(
+        pathParams = [
+            OpenApiParam(name = "my-enum-path-param", type = UserType::class)
+        ],
+        queryParams = [
+            OpenApiParam(name = "my-enum-query-param", type = UserType::class)
+        ]
+)
+fun getEnumParamsHandler(ctx: Context) {
+}
+
 class TestOpenApiAnnotations {
     @Test
     fun `createOpenApiSchema works with complexExample and annotations`() {
@@ -316,6 +327,7 @@ class TestOpenApiAnnotations {
         app.get("/upload-with-form-data", ::getUploadWithFormDataHandler)
         app.get("/resources/*", ::getResources)
         app.get("/ignore", ::getIgnore)
+        app.get("/enums/:my-enum-path-param", ::getEnumParamsHandler)
 
         val actual = JavalinOpenApi.createSchema(app)
 
diff --git a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/json.kt b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/json.kt
index c7d378ee..2c016aea 100644
--- a/javalin-openapi/src/test/java/io/javalin/plugin/openapi/json.kt
+++ b/javalin-openapi/src/test/java/io/javalin/plugin/openapi/json.kt
@@ -39,6 +39,10 @@ val userOpenApiSchema = """
       },
       "address":{
           "$ref": "#/components/schemas/Address"
+      },
+      "userType" : {
+        "type" : "string",
+        "enum" : [ "ONE", "TWO" ]
       }
    }
 }
@@ -64,6 +68,14 @@ val addressOpenApiSchema = """
 }
 """.formatJson()
 
+@Language("JSON")
+val userTypeOpenApiSchema = """
+{
+    "type" : "string",
+    "enum" : [ "ONE", "TWO" ]
+}
+""".formatJson()
+
 @Language("JSON")
 val complexExampleUsersGetResponsesJson = """
 {
@@ -120,31 +132,8 @@ val queryBeanExample = """
       },
       "components" : {
         "schemas" : {
-          "Address" : {
-            "required" : [ "number", "street" ],
-            "type" : "object",
-            "properties" : {
-              "street" : {
-                "type" : "string"
-              },
-              "number" : {
-                "type" : "integer",
-                "format" : "int32"
-              }
-            }
-          },
-          "User" : {
-            "required" : [ "name" ],
-            "type" : "object",
-            "properties" : {
-              "name" : {
-                "type" : "string"
-              },
-              "address" : {
-                "$ref" : "#/components/schemas/Address"
-              }
-            }
-          }
+          "Address" : $addressOpenApiSchema,
+          "User" : $userOpenApiSchema
         }
       }
     }
@@ -882,12 +871,38 @@ val complexExampleJson = """
           }
         }
       }
+    },
+    "/enums/{my-enum-path-param}" : {
+      "get" : {
+        "summary" : "Get enums with myEnumPathParam",
+        "operationId" : "getEnumsWithMyEnumPathParam",
+        "parameters" : [ {
+          "name" : "my-enum-path-param",
+          "in" : "path",
+          "required" : true,
+          "schema" : {
+            "$ref" : "#/components/schemas/UserType"
+          }
+        }, {
+          "name" : "my-enum-query-param",
+          "in" : "query",
+          "schema" : {
+            "$ref" : "#/components/schemas/UserType"
+          }
+        } ],
+        "responses" : {
+          "200" : {
+            "description" : "Default response"
+          }
+        }
+      }
     }
   },
   "components": {
     "schemas": {
       "Address": $addressOpenApiSchema,
-      "User": $userOpenApiSchema
+      "User": $userOpenApiSchema,
+      "UserType": $userTypeOpenApiSchema
     },
     "securitySchemes" : {
       "http" : {
@@ -1144,31 +1159,8 @@ val overrideJson = """
   },
   "components" : {
     "schemas" : {
-      "Address" : {
-        "required" : [ "number", "street" ],
-        "type" : "object",
-        "properties" : {
-          "street" : {
-            "type" : "string"
-          },
-          "number" : {
-            "type" : "integer",
-            "format" : "int32"
-          }
-        }
-      },
-      "User" : {
-        "required" : [ "name" ],
-        "type" : "object",
-        "properties" : {
-          "name" : {
-            "type" : "string"
-          },
-          "address" : {
-            "$ref" : "#/components/schemas/Address"
-          }
-        }
-      }
+      "Address" : $addressOpenApiSchema,
+      "User" : $userOpenApiSchema
     }
   }
 }
@@ -1339,32 +1331,9 @@ val composedExample = """
   },
   "components" : {
     "schemas" : {
-      "Address" : {
-        "required" : [ "number", "street" ],
-        "type" : "object",
-        "properties" : {
-          "street" : {
-            "type" : "string"
-          },
-          "number" : {
-            "type" : "integer",
-            "format" : "int32"
-          }
-        }
-      },
-      "User" : {
-        "required" : [ "name" ],
-        "type" : "object",
-        "properties" : {
-          "name" : {
-            "type" : "string"
-          },
-          "address" : {
-            "$ref" : "#/components/schemas/Address"
-          }
-        }
-      }
+      "Address" : $addressOpenApiSchema,
+      "User" : $userOpenApiSchema
     }
   }
 }
-""".trimIndent()
+""".formatJson()
diff --git a/javalin/src/test/java/io/javalin/TestTrailingSlashes.kt b/javalin/src/test/java/io/javalin/TestTrailingSlashes.kt
index d1d5c2e4..b67664b9 100644
--- a/javalin/src/test/java/io/javalin/TestTrailingSlashes.kt
+++ b/javalin/src/test/java/io/javalin/TestTrailingSlashes.kt
@@ -10,10 +10,16 @@ package io.javalin
 import io.javalin.apibuilder.ApiBuilder.get
 import io.javalin.apibuilder.ApiBuilder.path
 import io.javalin.testing.TestUtil
+import okhttp3.OkHttpClient
+import okhttp3.Request
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Test
+import java.net.URLEncoder
 
 class TestTrailingSlashes {
+    private val okHttp = OkHttpClient().newBuilder().build()
+    fun OkHttpClient.getBody(path: String) = this.newCall(Request.Builder().url(path).get().build()).execute().body()!!.string()
+    val javalin = Javalin.create { it.ignoreTrailingSlashes = false; }
 
     @Test
     fun `trailing slashes are ignored by default`() = TestUtil.test { app, http ->
@@ -36,7 +42,6 @@ class TestTrailingSlashes {
 
     @Test
     fun `trailing slashes are treat as different url, if configuration is set - ApiBuilder`() {
-        val javalin = Javalin.create { it.ignoreTrailingSlashes = false; }
         TestUtil.test(javalin) { app, http ->
             app.get("/a") { ctx -> ctx.result("a") }
             app.get("/a/") { ctx -> ctx.result("a-slash") }
@@ -45,4 +50,102 @@ class TestTrailingSlashes {
         }
     }
 
+    @Test
+    fun `trailing slashes don't change url params behaviour`() {
+        TestUtil.test(javalin) { app, http ->
+            app.get("/a") { ctx -> ctx.result("a") }
+            app.get("/a/") { ctx -> ctx.result("a-slash") }
+            assertThat(http.getBody("/a")).isEqualTo("a")
+            assertThat(http.getBody("/a/")).isEqualTo("a-slash")
+        }
+    }
+
+    @Test
+    fun `wildcard first works`() = TestUtil.test(javalin) { app, http ->
+        app.get("/*/test") { it.result("!") }
+        app.get("/*/test/") { it.result("!/") }
+        assertThat(http.getBody("/en/test")).isEqualTo("!")
+        assertThat(http.getBody("/en/test/")).isEqualTo("!/")
+    }
+
+    @Test
+    fun `wildcard middle works`() = TestUtil.test(javalin) { app, http ->
+        app.get("/test/*/test") { it.result("!") }
+        app.get("/test/*/test/") { it.result("!/") }
+        assertThat(http.getBody("/test/en/test")).isEqualTo("!")
+        assertThat(http.getBody("/test/en/test/")).isEqualTo("!/")
+    }
+
+    @Test
+    fun `wildcard end works`() = TestUtil.test(javalin) { app, http ->
+        app.get("/test/*") { it.result("!") }
+        app.get("/test/*/") { it.result("!/") }
+        assertThat(http.getBody("/test/en")).isEqualTo("!")
+        assertThat(http.getBody("/test/en/")).isEqualTo("!") //wildcard at end match all urls, so trailingSlashes are captured by *
+    }
+
+    @Test
+    fun `case sensitive urls work`() = TestUtil.test(javalin) { app, http ->
+        app.get("/My-Url") { ctx -> ctx.result("OK") }
+        assertThat(http.getBody("/MY-URL")).isEqualTo("Not found")
+        assertThat(http.getBody("/My-Url")).isEqualTo("OK")
+        app.get("/My-Url/") { ctx -> ctx.result("OK/") }
+        assertThat(http.getBody("/MY-URL/")).isEqualTo("Not found")
+        assertThat(http.getBody("/My-Url/")).isEqualTo("OK/")
+    }
+
+    @Test
+    fun `utf-8 encoded path-params work`() = TestUtil.test(javalin) { app, http ->
+        app.get("/:path-param") { ctx -> ctx.result(ctx.pathParam("path-param")) }
+        app.get("/:path-param/") { ctx -> ctx.result(ctx.pathParam("path-param")+"/") }
+        assertThat(okHttp.getBody(http.origin + "/" + URLEncoder.encode("TE/ST", "UTF-8"))).isEqualTo("TE/ST")
+        assertThat(okHttp.getBody(http.origin + "/" + URLEncoder.encode("TE/ST/", "UTF-8"))).isEqualTo("TE/ST/")
+    }
+
+    @Test
+    fun `path-params work case-sensitive`() = TestUtil.test(javalin) { app, http ->
+        app.get("/:userId") { ctx -> ctx.result(ctx.pathParam("userId")) }
+        assertThat(http.getBody("/path-param")).isEqualTo("path-param")
+        app.get("/:a/:A") { ctx -> ctx.result("${ctx.pathParam("a")}-${ctx.pathParam("A")}") }
+        assertThat(http.getBody("/a/B")).isEqualTo("a-B")
+
+        app.get("/:userId/") { ctx -> ctx.result(ctx.pathParam("userId")+"/") }
+        assertThat(http.getBody("/path-param/")).isEqualTo("path-param/")
+        app.get("/:a/:A/") { ctx -> ctx.result("${ctx.pathParam("a")}-${ctx.pathParam("A")}/") }
+        assertThat(http.getBody("/a/B/")).isEqualTo("a-B/")
+    }
+
+    @Test
+    fun `path-param values retain their casing`() = TestUtil.test(javalin) { app, http ->
+        app.get("/:path-param") { ctx -> ctx.result(ctx.pathParam("path-param")) }
+        app.get("/:path-param/") { ctx -> ctx.result(ctx.pathParam("path-param")+"/") }
+        assertThat(http.getBody("/SomeCamelCasedValue/")).isEqualTo("SomeCamelCasedValue/")
+        assertThat(http.getBody("/SomeCamelCasedValue/")).isEqualTo("SomeCamelCasedValue/")
+    }
+
+    @Test
+    fun `path regex works`() = TestUtil.test(javalin) { app, http ->
+        app.get("/:path-param/[0-9]+") { ctx -> ctx.result(ctx.pathParam("path-param")) }
+        app.get("/:path-param/[0-9]+/") { ctx -> ctx.result(ctx.pathParam("path-param")+"/") }
+        assertThat(http.getBody("/test/pathParam")).isEqualTo("Not found")
+        assertThat(http.getBody("/test/21")).isEqualTo("test")
+        assertThat(http.getBody("/test/pathParam/")).isEqualTo("Not found")
+        assertThat(http.getBody("/test/21/")).isEqualTo("test/")
+    }
+
+    @Test
+    fun `automatic slash prefixing works`() = TestUtil.test(javalin) { app, http ->
+        app.routes {
+            path("test") {
+                path(":id") { get { ctx -> ctx.result(ctx.pathParam("id")) } }
+                path(":id/") { get { ctx -> ctx.result(ctx.pathParam("id")+"/") } }
+                get { ctx -> ctx.result("test") }
+            }
+        }
+        assertThat(http.getBody("/test/path-param")).isEqualTo("path-param")
+        assertThat(http.getBody("/test/path-param/")).isEqualTo("path-param/")
+        assertThat(http.getBody("/test/")).isEqualTo("Not found")
+        assertThat(http.getBody("/test")).isEqualTo("test")
+    }
+
 }