Repository: Microsoft/thrifty (kotlin)

Issue:
Add option to not throw on unknown enum values
We would instead use the default value specified in thrift (if the field has one), or null for optional/unrequired fields.  We'd still have to throw for required fields without a default, but I'm OK with that - the design of enums in thrift isn't forward-compatible as is, and required fields doubly so.

See #84 for the conversation sparking this feature.

Pull request description:
Fixes #320

This is just a WIP which adds support only in the java codegen. If the
approach  seems good, tests will be added and also the support for
kotlin codegen

Patch:
diff --git a/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt b/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
index b48785bc3..d0792acd2 100644
--- a/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
+++ b/thrifty-compiler/src/main/kotlin/com/microsoft/thrifty/compiler/ThriftyCompiler.kt
@@ -256,6 +256,10 @@ class ThriftyCompiler {
                 .path(mustExist = true, canBeFile = true, canBeDir = false, mustBeReadable = true)
                 .multiple()
 
+        val failOnUnknownEnumValues by option("--fail-on-unknown-enum-values",
+                    help = "When set, unknown values found when decoding will throw an exception. Otherwise, it uses null/default values.")
+                .flag("--no-fail-on-unknown-enum-values", default = true)
+
         private val generatedAnnotationClassName: String? by lazy {
             when {
                 omitGeneratedAnnotations -> null
@@ -333,6 +337,7 @@ class ThriftyCompiler {
             gen.emitFileComment(!omitFileComments)
             gen.emitParcelable(emitParcelable)
             gen.emitGeneratedAnnotations(generatedAnnotationClassName)
+            gen.failOnUnknownEnumValues(failOnUnknownEnumValues)
 
             gen.generate(outputDirectory)
         }
@@ -363,6 +368,8 @@ class ThriftyCompiler {
                 gen.filePerNamespace()
             }
 
+            gen.failOnUnknownEnumValues(failOnUnknownEnumValues)
+
             listTypeName?.let { gen.listClassName(it) }
             setTypeName?.let { gen.setClassName(it) }
             mapTypeName?.let { gen.mapClassName(it) }
diff --git a/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/GenerateReaderVisitor.kt b/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/GenerateReaderVisitor.kt
index 7c574a452..635bb769c 100644
--- a/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/GenerateReaderVisitor.kt
+++ b/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/GenerateReaderVisitor.kt
@@ -51,7 +51,8 @@ internal open class GenerateReaderVisitor(
         private val resolver: TypeResolver,
         private val read: MethodSpec.Builder,
         private val fieldName: String,
-        private val fieldType: ThriftType
+        private val fieldType: ThriftType,
+        private val failOnUnknownEnumValues: Boolean = true
 ) : ThriftType.Visitor<Unit> {
 
     private val nameStack: Deque<String> = ArrayDeque<String>()
@@ -76,7 +77,13 @@ internal open class GenerateReaderVisitor(
     }
 
     protected open fun useReadValue(localName: String) {
-        read.addStatement("builder.\$N(\$N)", fieldName, localName)
+        if (failOnUnknownEnumValues || !fieldType.isEnum) {
+            read.addStatement("builder.\$N(\$N)", fieldName, localName)
+        } else {
+            read.beginControlFlow("if (\$N != null)", localName)
+            read.addStatement("builder.\$N(\$N)", fieldName, localName)
+            read.endControlFlow()
+        }
     }
 
     override fun visitBool(boolType: BuiltinType) {
@@ -122,14 +129,16 @@ internal open class GenerateReaderVisitor(
 
         read.addStatement("int \$L = protocol.readI32()", intName)
         read.addStatement("$1L $2N = $1L.findByValue($3L)", qualifiedJavaName, target, intName)
-        read.beginControlFlow("if (\$N == null)", target!!)
-        read.addStatement(
-                "throw new $1T($2T.PROTOCOL_ERROR, $3S + $4L)",
-                TypeNames.THRIFT_EXCEPTION,
-                TypeNames.THRIFT_EXCEPTION_KIND,
-                "Unexpected value for enum-type " + enumType.name + ": ",
-                intName)
-        read.endControlFlow()
+        if (failOnUnknownEnumValues) {
+            read.beginControlFlow("if (\$N == null)", target!!)
+            read.addStatement(
+                    "throw new $1T($2T.PROTOCOL_ERROR, $3S + $4L)",
+                    TypeNames.THRIFT_EXCEPTION,
+                    TypeNames.THRIFT_EXCEPTION_KIND,
+                    "Unexpected value for enum-type " + enumType.name + ": ",
+                    intName)
+            read.endControlFlow()
+        }
     }
 
     override fun visitList(listType: ListType) {
diff --git a/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/ThriftyCodeGenerator.kt b/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/ThriftyCodeGenerator.kt
index bddf83e1d..96aa50a43 100644
--- a/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/ThriftyCodeGenerator.kt
+++ b/thrifty-java-codegen/src/main/kotlin/com/microsoft.thrifty.gen/ThriftyCodeGenerator.kt
@@ -71,6 +71,7 @@ class ThriftyCodeGenerator {
     private var nullabilityAnnotationType: NullabilityAnnotationType = NullabilityAnnotationType.NONE
     private var emitParcelable: Boolean = false
     private var emitFileComment = true
+    private var failOnUnknownEnumValues = true
     private var generatedAnnotationType: ClassName? = null
     private val emitGeneratedAnnotations: Boolean
         get() = generatedAnnotationType != null
@@ -127,6 +128,11 @@ class ThriftyCodeGenerator {
         return this
     }
 
+    fun failOnUnknownEnumValues(failOnUnknownEnumValues: Boolean): ThriftyCodeGenerator {
+        this.failOnUnknownEnumValues = failOnUnknownEnumValues
+        return this
+    }
+
     fun generate(directory: Path) {
         generate { file ->
             file?.writeTo(directory)
@@ -620,9 +626,14 @@ class ThriftyCodeGenerator {
                 write.endControlFlow()
             }
 
-            // Read
+            val effectiveFailOnUnknownValues = if (tt.isEnum) {
+                failOnUnknownEnumValues || field.required
+            } else {
+                failOnUnknownEnumValues
+            }
+
             read.beginControlFlow("case \$L:", field.id)
-            GenerateReaderVisitor(typeResolver, read, fieldName, field.type.trueType).generate()
+            GenerateReaderVisitor(typeResolver, read, fieldName, tt, effectiveFailOnUnknownValues).generate()
             read.endControlFlow() // end case block
             read.addStatement("break")
         }
diff --git a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
index 894f792da..8b30e8308 100644
--- a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
+++ b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
@@ -125,6 +125,7 @@ class KotlinCodeGenerator(
     private var omitServiceClients: Boolean = false
     private var coroutineServiceClients: Boolean = false
     private var emitJvmName: Boolean = false
+    private var failOnUnknownEnumValues: Boolean = true
 
     private var listClassName: ClassName? = null
     private var setClassName: ClassName? = null
@@ -233,6 +234,10 @@ class KotlinCodeGenerator(
         this.emitJvmName = true
     }
 
+    fun failOnUnknownEnumValues(value: Boolean = true): KotlinCodeGenerator = apply {
+        this.failOnUnknownEnumValues = value
+    }
+
     private object NoTypeProcessor : KotlinTypeProcessor {
         override fun process(typeSpec: TypeSpec) = typeSpec
     }
@@ -971,14 +976,28 @@ class KotlinCodeGenerator(
                     addStatement("${field.id}·->·{⇥")
                     beginControlFlow("if (fieldMeta.typeId == %T.%L)", TType::class, fieldType.typeCodeName)
 
-                    generateReadCall(this, name, fieldType)
-
-                    if (builderType != null) {
-                        addStatement("builder.$name($name)")
+                    val effectiveFailOnUnknownValues = if (fieldType.isEnum) {
+                        failOnUnknownEnumValues || field.required
                     } else {
-                        addStatement("%N = $name", localFieldName(field))
+                        failOnUnknownEnumValues
                     }
+                    generateReadCall(this, name, fieldType, failOnUnknownEnumValues = effectiveFailOnUnknownValues)
 
+                    if (effectiveFailOnUnknownValues || !fieldType.isEnum) {
+                        if (builderType != null) {
+                            addStatement("builder.$name($name)")
+                        } else {
+                            addStatement("%N = $name", localFieldName(field))
+                        }
+                    } else if (builderType != null) {
+                        beginControlFlow("$name.let")
+                        addStatement("builder.$name(it)")
+                        endControlFlow()
+                    } else {
+                        beginControlFlow("$name.let")
+                        addStatement("%N = it", localFieldName(field))
+                        endControlFlow()
+                    }
                     nextControlFlow("else")
                     addStatement("%T.skip(protocol, fieldMeta.typeId)", ProtocolUtil::class)
                     endControlFlow()
@@ -1316,7 +1335,8 @@ class KotlinCodeGenerator(
             name: String,
             type: ThriftType,
             scope: Int = 0,
-            localNamePrefix: String = ""
+            localNamePrefix: String = "",
+            failOnUnknownEnumValues: Boolean = true
     ): CodeBlock.Builder {
         type.accept(object : ThriftType.Visitor<Unit> {
             override fun visitVoid(voidType: BuiltinType) {
@@ -1357,11 +1377,16 @@ class KotlinCodeGenerator(
 
             override fun visitEnum(enumType: EnumType) {
                 block.beginControlFlow("val $name = protocol.readI32().let")
-                block.addStatement(
-                        "%1T.findByValue(it) ?: throw %2T(%3T.PROTOCOL_ERROR, \"Unexpected·value·for·enum·type·%1T:·\$it\")",
-                        enumType.typeName,
-                        ThriftException::class,
-                        ThriftException.Kind::class)
+                if (failOnUnknownEnumValues) {
+                    block.addStatement(
+                            "%1T.findByValue(it) ?: " +
+                                    "throw %2T(%3T.PROTOCOL_ERROR, \"Unexpected·value·for·enum·type·%1T:·\$it\")",
+                            enumType.typeName,
+                            ThriftException::class,
+                            ThriftException.Kind::class)
+                } else {
+                    block.addStatement("%1T.findByValue(it)", enumType.typeName)
+                }
                 block.endControlFlow()
             }
 


Test patch:
diff --git a/thrifty-java-codegen/src/test/kotlin/com/microsoft/thrifty/gen/ThriftyCodeGeneratorTest.kt b/thrifty-java-codegen/src/test/kotlin/com/microsoft/thrifty/gen/ThriftyCodeGeneratorTest.kt
index feef3e386..f6478ab88 100644
--- a/thrifty-java-codegen/src/test/kotlin/com/microsoft/thrifty/gen/ThriftyCodeGeneratorTest.kt
+++ b/thrifty-java-codegen/src/test/kotlin/com/microsoft/thrifty/gen/ThriftyCodeGeneratorTest.kt
@@ -520,6 +520,87 @@ class ThriftyCodeGeneratorTest {
         assertThat(javaFile.toString()).isEqualTo(expected)
     }
 
+    @Test
+    fun structWithEnum() {
+        val thrift = """
+            namespace java structs.enums
+
+            enum TestEnum { FOO }
+
+            struct HasEnum {
+                1: optional TestEnum field = TestEnum.FOO;
+            }
+        """
+        val expected = """
+          case 1: {
+            if (field.typeId == TType.I32) {
+              int i32_0 = protocol.readI32();
+              structs.enums.TestEnum value = structs.enums.TestEnum.findByValue(i32_0);
+              if (value == null) {
+                throw new ThriftException(ThriftException.Kind.PROTOCOL_ERROR, "Unexpected value for enum-type TestEnum: " + i32_0);
+              }
+              builder.field(value);
+            } else {
+              ProtocolUtil.skip(protocol, field.typeId);
+            }
+          }
+          break;
+        """
+
+        val thriftFile = tmp.newFile("structs_enums.thrift")
+        val javaFile = compile(thriftFile, thrift)[1]
+
+        assertThat(javaFile.toString()).contains(expected)
+    }
+
+    @Test
+    fun structWithEnumAcceptUnknownValues() {
+        val thrift = """
+            namespace java structs.enums
+
+            enum TestEnum { FOO }
+
+            struct HasEnum {
+                1: optional TestEnum field1 = TestEnum.FOO;
+                2: required TestEnum field2 = TestEnum.FOO;
+            }
+        """
+        val expected = """
+          case 1: {
+            if (field.typeId == TType.I32) {
+              int i32_0 = protocol.readI32();
+              structs.enums.TestEnum value = structs.enums.TestEnum.findByValue(i32_0);
+              if (value != null) {
+                builder.field1(value);
+              }
+            } else {
+              ProtocolUtil.skip(protocol, field.typeId);
+            }
+          }
+          break;
+          case 2: {
+            if (field.typeId == TType.I32) {
+              int i32_0 = protocol.readI32();
+              structs.enums.TestEnum value = structs.enums.TestEnum.findByValue(i32_0);
+              if (value == null) {
+                throw new ThriftException(ThriftException.Kind.PROTOCOL_ERROR, "Unexpected value for enum-type TestEnum: " + i32_0);
+              }
+              builder.field2(value);
+            } else {
+              ProtocolUtil.skip(protocol, field.typeId);
+            }
+          }
+          break;
+        """
+
+        val thriftFile = tmp.newFile("structs_enums.thrift")
+        val schema = parse(thriftFile, thrift)
+        val gen = ThriftyCodeGenerator(schema).emitFileComment(false).failOnUnknownEnumValues(false)
+        val javaFile = gen.generateTypes()[1]
+
+        assertThat(javaFile.toString()).contains(expected)
+    }
+
     @Test
     fun mapsWithEnumKeysAndValues () {
         val thrift = """
diff --git a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
index 7ea49e8ab..9506ccfd7 100644
--- a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
+++ b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
@@ -34,6 +34,7 @@ import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.shouldNot
 import io.kotest.matchers.string.contain
+import io.kotest.matchers.string.shouldContain
 import org.junit.Ignore
 import org.junit.Rule
 import org.junit.Test
@@ -833,6 +834,122 @@ class KotlinCodeGeneratorTest {
         """.trimMargin())
     }
 
+    @Test
+    fun `enum fail on unknown value`() {
+        val thrift = """
+            |namespace kt test.struct
+            |
+            |enum TestEnum { FOO }
+            |
+            |struct HasEnum {
+            |  1: optional TestEnum field = TestEnum.FOO;
+            |}
+        """.trimMargin()
+
+        val expected = """
+          1 -> {
+            if (fieldMeta.typeId == TType.I32) {
+              val field = protocol.readI32().let {
+                TestEnum.findByValue(it) ?: throw
+                    ThriftException(ThriftException.Kind.PROTOCOL_ERROR,
+                    "Unexpected value for enum type TestEnum: ${'$'}it")
+              }
+              builder.field(field)
+            } else {
+              ProtocolUtil.skip(protocol, fieldMeta.typeId)
+            }
+          }"""
+
+        val file = generate(thrift)
+        file.single().toString() shouldContain expected
+    }
+
+    @Test
+    fun `enum don't fail on unknown value`() {
+        val thrift = """
+            |namespace kt test.struct
+            |
+            |enum TestEnum { FOO }
+            |
+            |struct HasEnum {
+            |  1: optional TestEnum field1 = TestEnum.FOO;
+            |  2: required TestEnum field2 = TestEnum.FOO;
+            |}
+        """.trimMargin()
+
+        val expected = """
+          1 -> {
+            if (fieldMeta.typeId == TType.I32) {
+              val field1 = protocol.readI32().let {
+                TestEnum.findByValue(it)
+              }
+              field1.let {
+                builder.field1(it)
+              }
+            } else {
+              ProtocolUtil.skip(protocol, fieldMeta.typeId)
+            }
+          }
+          2 -> {
+            if (fieldMeta.typeId == TType.I32) {
+              val field2 = protocol.readI32().let {
+                TestEnum.findByValue(it) ?: throw
+                    ThriftException(ThriftException.Kind.PROTOCOL_ERROR,
+                    "Unexpected value for enum type TestEnum: ${'$'}it")
+              }
+              builder.field2(field2)
+            } else {
+              ProtocolUtil.skip(protocol, fieldMeta.typeId)
+            }
+          }"""
+
+        val file = generate(thrift) { failOnUnknownEnumValues(false) }
+        file.single().toString() shouldContain expected
+    }
+
+    @Test
+    fun `enum don't fail on unknown value without builder`() {
+        val thrift = """
+            |namespace kt test.struct
+            |
+            |enum TestEnum { FOO }
+            |
+            |struct HasEnum {
+            |  1: optional TestEnum field1 = TestEnum.FOO;
+            |  2: required TestEnum field2 = TestEnum.FOO;
+            |}
+        """.trimMargin()
+
+        val expected = """
+          1 -> {
+            if (fieldMeta.typeId == TType.I32) {
+              val field1 = protocol.readI32().let {
+                TestEnum.findByValue(it)
+              }
+              field1.let {
+                _local_field1 = it
+              }
+            } else {
+              ProtocolUtil.skip(protocol, fieldMeta.typeId)
+            }
+          }
+          2 -> {
+            if (fieldMeta.typeId == TType.I32) {
+              val field2 = protocol.readI32().let {
+                TestEnum.findByValue(it) ?: throw
+                    ThriftException(ThriftException.Kind.PROTOCOL_ERROR,
+                    "Unexpected value for enum type TestEnum: ${'$'}it")
+              }
+              _local_field2 = field2
+            } else {
+              ProtocolUtil.skip(protocol, fieldMeta.typeId)
+            }
+          }"""
+
+        val file = generate(thrift) { failOnUnknownEnumValues(false); builderlessDataClasses() }
+        file.single().toString() shouldContain expected
+    }
+
     private fun generate(thrift: String, config: (KotlinCodeGenerator.() -> KotlinCodeGenerator)? = null): List<FileSpec> {
         val configOrDefault = config ?: { this }
         return KotlinCodeGenerator()