Fix protocol buffer enum schema generation (#1967)

fix protocol buffer schema generated from enum elements with 'ProtoNumber' annotation

Author: mogud

formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/schema/ProtoBufSchemaGenerator.kt

@@ -190,7 +190,7 @@ public object ProtoBufSchemaGenerator {
 
 
             val annotations = messageDescriptor.getElementAnnotations(index)
-            val number = annotations.filterIsInstance<ProtoNumber>().singleOrNull()?.number ?: index + 1
+            val number = annotations.filterIsInstance<ProtoNumber>().singleOrNull()?.number ?: (index + 1)
             if (!usedNumbers.add(number)) {
                 throw IllegalArgumentException("Field number $number is repeated in the class with serial name ${messageDescriptor.serialName}")
             }
@@ -319,19 +319,35 @@ public object ProtoBufSchemaGenerator {
         }
         val safeSerialName = removeLineBreaks(enumDescriptor.serialName)
         if (safeSerialName != enumName) {
-            append("// serial name '").append(enumName).appendLine('\'')
+            append("// serial name '").append(safeSerialName).appendLine('\'')
         }
 
         append("enum ").append(enumName).appendLine(" {")
 
-        enumDescriptor.elementDescriptors.forEachIndexed { number, element ->
+        val usedNumbers: MutableSet<Int> = mutableSetOf()
+        val duplicatedNumbers: MutableSet<Int> = mutableSetOf()
+        enumDescriptor.elementDescriptors.forEachIndexed { index, element ->
             val elementName = element.protobufEnumElementName
             elementName.checkIsValidIdentifier {
                 "The enum element name '$elementName' is invalid in the " +
                         "protobuf schema. Serial name of the enum class '${enumDescriptor.serialName}'"
             }
+
+            val annotations = enumDescriptor.getElementAnnotations(index)
+            val number = annotations.filterIsInstance<ProtoNumber>().singleOrNull()?.number ?: index
+            if (!usedNumbers.add(number)) {
+                duplicatedNumbers.add(number)
+            }
+
             append("  ").append(elementName).append(" = ").append(number).appendLine(';')
         }
+        if (duplicatedNumbers.isNotEmpty()) {
+            throw IllegalArgumentException(
+                "The class with serial name ${enumDescriptor.serialName} has duplicate " +
+                    "elements with numbers $duplicatedNumbers"
+            )
+        }
+
         appendLine('}')
     }
 

formats/protobuf/commonTest/src/kotlinx/serialization/protobuf/schema/SchemaValidationsTest.kt

@@ -3,7 +3,6 @@ package kotlinx.serialization.protobuf.schema
 import kotlinx.serialization.*
 import kotlinx.serialization.protobuf.*
 import kotlin.test.Test
-import kotlin.test.assertContains
 import kotlin.test.assertFailsWith
 
 class SchemaValidationsTest {
@@ -39,6 +38,33 @@ class SchemaValidationsTest {
         SECOND
     }
 
+    @Serializable
+    enum class EnumWithExplicitProtoNumberDuplicate {
+        @ProtoNumber(2)
+        FIRST,
+        @ProtoNumber(2)
+        SECOND,
+    }
+
+    @Serializable
+    enum class EnumWithImplicitProtoNumberDuplicate {
+        FIRST,
+        @ProtoNumber(0)
+        SECOND,
+    }
+
+    @Test
+    fun testExplicitDuplicateEnumElementProtoNumber() {
+        val descriptors = listOf(EnumWithExplicitProtoNumberDuplicate.serializer().descriptor)
+        assertFailsWith(IllegalArgumentException::class) { ProtoBufSchemaGenerator.generateSchemaText(descriptors) }
+    }
+
+    @Test
+    fun testImplicitDuplicateEnumElementProtoNumber() {
+        val descriptors = listOf(EnumWithImplicitProtoNumberDuplicate.serializer().descriptor)
+        assertFailsWith(IllegalArgumentException::class) { ProtoBufSchemaGenerator.generateSchemaText(descriptors) }
+    }
+
     @Test
     fun testInvalidEnumElementSerialName() {
         val descriptors = listOf(InvalidEnumElementName.serializer().descriptor)

formats/protobuf/jvmTest/resources/EnumWithProtoNumber.proto

@@ -0,0 +1,11 @@
+syntax = "proto2";
+
+package kotlinx.serialization.protobuf.schema.generator;
+
+// serial name 'kotlinx.serialization.protobuf.schema.GenerationTest.EnumWithProtoNumber'
+enum EnumWithProtoNumber {
+  ZERO = 0;
+  THREE = 3;
+  TWO = 2;
+  FIVE = 5;
+}

formats/protobuf/jvmTest/resources/common/schema.proto

@@ -131,6 +131,14 @@ message OptionalCollections {
   map<int32, int32> nullableOptionalMap = 8;
 }
 
+// serial name 'kotlinx.serialization.protobuf.schema.GenerationTest.EnumWithProtoNumber'
+enum EnumWithProtoNumber {
+  ZERO = 0;
+  THREE = 3;
+  TWO = 2;
+  FIVE = 5;
+}
+
 enum OverriddenEnumName {
   FIRST = 0;
   OverriddenElementName = 1;

formats/protobuf/jvmTest/src/kotlinx/serialization/protobuf/schema/GenerationTest.kt

@@ -25,6 +25,7 @@ internal val commonClasses = listOf(
     GenerationTest.LegacyMapHolder::class,
     GenerationTest.NullableNestedCollections::class,
     GenerationTest.OptionalCollections::class,
+    GenerationTest.EnumWithProtoNumber::class,
 )
 
 class GenerationTest {
@@ -180,6 +181,16 @@ class GenerationTest {
         val legacyMap: Map<List<Int>?, List<Int>?>
     )
 
+    @Serializable
+    enum class EnumWithProtoNumber {
+        ZERO,
+        @ProtoNumber(3)
+        THREE,
+        TWO,
+        @ProtoNumber(5)
+        FIVE,
+    }
+
     @Test
     fun testIndividuals() {
         assertSchemaForClass(OptionsClass::class, mapOf("java_package" to "api.proto", "java_outer_classname" to "Outer"))