Skip to content

Commit

Permalink
Fix missing inner enum import on operations
Browse files Browse the repository at this point in the history
Previously inner types of complex types were not imported properly on
operations. This resulted in Retrofit interfaces missing some imports.

This commit introduces methods to inspect and import the inner types
used as result types of operations.

Fixes #76
  • Loading branch information
cortinico committed Jan 1, 2020
1 parent 01fbfbf commit 54b94dd
Show file tree
Hide file tree
Showing 8 changed files with 249 additions and 26 deletions.
28 changes: 28 additions & 0 deletions plugin/src/main/java/com/yelp/codegen/KotlinGenerator.kt
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,26 @@ class KotlinGenerator : SharedCodegen() {
public override fun listTypeWrapper(listType: String, innerType: String) =
"$listType<$innerType>"

public override fun listTypeUnwrapper(baseType: String) = baseType
.replace(" ", "")
.removeSuffix(">")
.removePrefix("${typeMapping["list"]}<")

public override fun isListTypeWrapped(baseType: String) =
baseType.endsWith('>') && baseType.startsWith("${typeMapping["list"]}<")

@VisibleForTesting
public override fun mapTypeWrapper(mapType: String, innerType: String) =
"$mapType<${typeMapping["string"]}, $innerType>"

public override fun mapTypeUnwrapper(baseType: String) = baseType
.replace(" ", "")
.removeSuffix(">")
.removePrefix("${typeMapping["map"]}<${typeMapping["string"]},")

public override fun isMapTypeWrapped(baseType: String) =
baseType.endsWith('>') && baseType.startsWith("${typeMapping["map"]}<${typeMapping["string"]},")

@VisibleForTesting
public override fun nullableTypeWrapper(baseType: String) =
baseType.safeSuffix("?")
Expand Down Expand Up @@ -407,6 +423,7 @@ class KotlinGenerator : SharedCodegen() {
}

codegenOperation.imports.add("retrofit2.http.Headers")

codegenOperation.vendorExtensions[X_OPERATION_ID] = operation?.operationId
getHeadersToIgnore().forEach { headerName ->
ignoreHeaderParameter(headerName, codegenOperation)
Expand All @@ -417,6 +434,17 @@ class KotlinGenerator : SharedCodegen() {
codegenOperation.path = codegenOperation.path.removePrefix("/")
}
processTopLevelHeaders(codegenOperation)

// Let's make sure we import all the types, also the inner ones (see #76).
codegenOperation.responses.forEach {
if (it.dataType != null) {
val innerType = resolveInnerType(it.dataType)
if (innerType !in codegenOperation.imports) {
codegenOperation.imports.add(innerType)
}
}
}

return codegenOperation
}

Expand Down
48 changes: 48 additions & 0 deletions plugin/src/main/java/com/yelp/codegen/SharedCodegen.kt
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,24 @@ abstract class SharedCodegen : DefaultCodegen(), CodegenConfig {
return codegenModel.dataType
}

/**
* Resolve the inner type from a complex type. E.g:
* List<List<Int>> ---> Int
* Map<String, Map<String, List<Object>>> ---> Object
*/
internal tailrec fun resolveInnerType(
baseType: String
): String {
if (isListTypeWrapped(baseType)) {
return resolveInnerType(listTypeUnwrapper(baseType))
}
if (isMapTypeWrapped(baseType)) {
return resolveInnerType(mapTypeUnwrapper(baseType))
}

return baseType
}

/**
* Determine if the swagger operation consumes mutipart content.
*/
Expand Down Expand Up @@ -468,6 +486,21 @@ abstract class SharedCodegen : DefaultCodegen(), CodegenConfig {
*/
protected abstract fun listTypeWrapper(listType: String, innerType: String): String

/**
* Abstract function to unwrap a JSON Array type.
* @param baseType A JSON list type (e.g. `List<String>`)
* @return The unwrapped inner type (e.g. `String`).
* @see isListTypeWrapped
*/
protected abstract fun listTypeUnwrapper(baseType: String): String

/**
* Abstract function to check if a type is a JSON Array type.
* @param baseType A JSON type
* @return True if the type is a JSON Array type and can be safely unwrapped with [listTypeUnwrapper]
*/
protected abstract fun isListTypeWrapped(baseType: String): Boolean

/**
* Abstract function to create a type for a JSON Object (A Map from String to values).
* Please note that in JSON Maps have only Strings as keys.
Expand All @@ -478,6 +511,21 @@ abstract class SharedCodegen : DefaultCodegen(), CodegenConfig {
*/
protected abstract fun mapTypeWrapper(mapType: String, innerType: String): String

/**
* Abstract function to unwrap a JSON Map type.
* @param baseType A JSON map type (e.g. `Map<String, Item>`)
* @return The unwrapped inner type (e.g. `Item`).
* @see isMapTypeWrapped
*/
protected abstract fun mapTypeUnwrapper(baseType: String): String

/**
* Abstract function to check if a type is a JSON Map type.
* @param baseType A JSON type
* @return True if the type is a JSON Map type and can be safely unwrapped with [mapTypeUnwrapper]
*/
protected abstract fun isMapTypeWrapped(baseType: String): Boolean

/**
* Abstract function to create a type from a Nullable type.
* @param baseType A type (e.g. `Integer`).
Expand Down
27 changes: 1 addition & 26 deletions plugin/src/test/java/com/yelp/codegen/KotlinGeneratorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,31 +29,6 @@ class KotlinGeneratorTest {
assertEquals("com.yelp.test.apis", generator.apiPackage())
}

@Test
fun listTypeWrapper_withSimpleType() {
assertEquals("List<String>", KotlinGenerator().listTypeWrapper("List", "String"))
}

@Test
fun listTypeWrapper_withMultipleNesting() {
assertEquals("List<List<String>>", KotlinGenerator().listTypeWrapper("List", "List<String>"))
}

@Test
fun mapTypeWrapper_withSimpleType() {
assertEquals("Map<String, Any>", KotlinGenerator().mapTypeWrapper("Map", "Any"))
}

@Test
fun mapTypeWrapper_withMultipleNesting() {
assertEquals("HashMap<String, List<Any?>?>", KotlinGenerator().mapTypeWrapper("HashMap", "List<Any?>?"))
}

@Test
fun nullableTypeWrapper_withSimpleType() {
assertEquals("String?", KotlinGenerator().nullableTypeWrapper("String"))
}

@Test
fun escapeUnsafeCharacters_withNothingToEscape() {
assertEquals("Nothing", KotlinGenerator().escapeUnsafeCharacters("Nothing"))
Expand Down Expand Up @@ -154,7 +129,7 @@ class KotlinGeneratorTest {
val generator = KotlinGenerator()
generator.additionalProperties()[GROUP_ID] = "com.yelp"
generator.additionalProperties()[ARTIFACT_ID] = "test"
val sep:String = File.separator
val sep: String = File.separator
assertTrue(generator.modelFileFolder().endsWith("com${sep}yelp${sep}test${sep}models"))
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package com.yelp.codegen

import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test

class KotlinGenerator_TypeMappersTest {

@Test
fun listTypeWrapper_withSimpleType() {
assertEquals("List<String>", KotlinGenerator().listTypeWrapper("List", "String"))
}

@Test
fun listTypeWrapper_withMultipleNesting() {
assertEquals("List<List<String>>", KotlinGenerator().listTypeWrapper("List", "List<String>"))
}

@Test
fun listTypeUnwrapper_withSimpleType() {
assertEquals("String", KotlinGenerator().listTypeUnwrapper("String"))
}

@Test
fun listTypeUnwrapper_withListType() {
assertEquals("String", KotlinGenerator().listTypeUnwrapper("List<String>"))
}

@Test
fun isListTypeWrapped_withSimpleType() {
assertFalse(KotlinGenerator().isListTypeWrapped("String"))
}

@Test
fun isListTypeWrapped_withListType() {
assertTrue(KotlinGenerator().isListTypeWrapped("List<String>"))
}

@Test
fun isListTypeWrapped_withMapType() {
assertFalse(KotlinGenerator().isListTypeWrapped("Map<String, Int>"))
}

@Test
fun mapTypeWrapper_withSimpleType() {
assertEquals("Map<String, Any>", KotlinGenerator().mapTypeWrapper("Map", "Any"))
}

@Test
fun mapTypeWrapper_withMultipleNesting() {
assertEquals("HashMap<String, List<Any?>?>", KotlinGenerator().mapTypeWrapper("HashMap", "List<Any?>?"))
}

@Test
fun mapTypeUnwrapper_withSimpleType() {
assertEquals("String", KotlinGenerator().mapTypeUnwrapper("String"))
}

@Test
fun mapTypeUnwrapper_withMapType() {
assertEquals("Int", KotlinGenerator().mapTypeUnwrapper("Map<String, Int>"))
}

@Test
fun isMapTypeWrapped_withSimpleType() {
assertFalse(KotlinGenerator().isMapTypeWrapped("String"))
}

@Test
fun isMapTypeWrapped_withListType() {
assertFalse(KotlinGenerator().isMapTypeWrapped("List<String>"))
}

@Test
fun isMapTypeWrapped_withMapType() {
assertTrue(KotlinGenerator().isMapTypeWrapped("Map<String, Int>"))
}

@Test
fun nullableTypeWrapper_withSimpleType() {
assertEquals("String?", KotlinGenerator().nullableTypeWrapper("String"))
}

@Test
fun resolveInnerType_withSimpleType() {
assertEquals("String", KotlinGenerator().resolveInnerType("String"))
}

@Test
fun resolveInnerType_withListType() {
assertEquals("String", KotlinGenerator().resolveInnerType("List<String>"))
}

@Test
fun resolveInnerType_withMapType() {
assertEquals("Int", KotlinGenerator().resolveInnerType("Map<String, Int>"))
}

@Test
fun resolveInnerType_withComplexType() {
assertEquals("Int", KotlinGenerator().resolveInnerType("Map<String, Map<String, List<Map<String, Int>>>>"))
}
}
22 changes: 22 additions & 0 deletions samples/junit-tests/junit_tests_specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,28 @@
]
}
},
"/top_level_enum/nested": {
"get": {
"operationId": "get_top_level_enum_nested",
"responses": {
"200": {
"description": "",
"schema": {
"additionalProperties": {
"additionalProperties": {
"$ref": "#/definitions/top_level_enum"
},
"type": "object"
},
"type": "object"
}
}
},
"tags": [
"resource"
]
}
},
"/top_level_map/{size}": {
"get": {
"operationId": "get_top_level_map",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ interface ResourceApi {
@GET("/top_level_enum")
fun getTopLevelEnum(): Single<TopLevelEnum>

/**
* The endpoint is owned by junittests service owner
*/
@Headers(
"X-Operation-ID: get_top_level_enum_nested"
)
@GET("/top_level_enum/nested")
fun getTopLevelEnumNested(): Single<Map<String, Map<String, TopLevelEnum>>>

/**
* The endpoint is owned by junittests service owner
* @param size (required)
Expand Down
23 changes: 23 additions & 0 deletions samples/junit-tests/src/main/java/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,29 @@
]
}
},
"/top_level_enum/nested": {
"get": {
"operationId": "get_top_level_enum_nested",
"parameters": [],
"responses": {
"200": {
"description": "",
"schema": {
"additionalProperties": {
"additionalProperties": {
"$ref": "#/definitions/top_level_enum"
},
"type": "object"
},
"type": "object"
}
}
},
"tags": [
"resource"
]
}
},
"/top_level_map/{size}": {
"get": {
"operationId": "get_top_level_map",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,18 @@ class TopLevelEnumEndpointTest {
val returned = rule.getApi<ResourceApi>().getTopLevelEnum().blockingGet()
assertEquals(TopLevelEnum.VALUE1, returned)
}

@Test
fun topLevelEnumNestedEndpoint() {
rule.server.enqueue(MockResponse().setBody("""
{
"key1": {
"key2": "TOP_LEVEL_VALUE1"
}
}
""".trimIndent()))

val returned = rule.getApi<ResourceApi>().getTopLevelEnumNested().blockingGet()
assertEquals(TopLevelEnum.VALUE1, returned["key1"]?.get("key2"))
}
}

0 comments on commit 54b94dd

Please sign in to comment.