Skip to content

chore: update protobuf to latest #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sgammon wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: update protobuf to latest #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/src/main/kotlin/dogacel/kotlinx/protobuf/gen/CodeGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dogacel.kotlinx.protobuf.gen

import com.google.protobuf.Descriptors
import com.google.protobuf.Descriptors.OneofDescriptor
import com.google.protobuf.compiler.PluginProtos
import com.squareup.kotlinpoet.ClassName
import com.squareup.kotlinpoet.CodeBlock
Expand Down Expand Up @@ -239,6 +240,9 @@ class CodeGenerator {
.defaultValue("%L", defaultValue)
}

private val OneofDescriptor.isSyntheticCompat: Boolean get() =
fields.size == 1 && !options.features.hasFieldPresence()

/**
* Generate a single class for the given [Descriptors.Descriptor]. Returns a [TypeSpec.Builder] so users
* can add additional code to the class.
Expand Down Expand Up @@ -269,7 +273,7 @@ class CodeGenerator {
// A trick to handle oneof fields. We need to make sure that only one of the fields is set.
// Validation is done in `init` block so objects in invalid states can't be initialized.
messageDescriptor.oneofs.forEach { oneOfDescriptor ->
if (!oneOfDescriptor.isSynthetic && oneOfDescriptor.fields.isNotEmpty()) {
if (!oneOfDescriptor.isSyntheticCompat && oneOfDescriptor.fields.isNotEmpty()) {
val codeSpec = CodeBlock.builder()
codeSpec.addStatement("require(")
codeSpec.indent()
Expand Down
11 changes: 8 additions & 3 deletions app/src/main/kotlin/dogacel/kotlinx/protobuf/gen/DefaultValues.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
package dogacel.kotlinx.protobuf.gen

import com.google.protobuf.Descriptors
import com.google.protobuf.Descriptors.FieldDescriptor
import com.squareup.kotlinpoet.CodeBlock
import com.squareup.kotlinpoet.TypeName

object DefaultValues {
// Indicate whether the field unconditionally has a value; true if the field is PROTO2 and marked `required`, or if
// the field has a default value when unset.
private val FieldDescriptor.unconditionallyHasValue: Boolean get() =
!isOptional || !options.features.hasFieldPresence()
Comment on lines +9 to +12
Copy link
Copy Markdown
Owner

@Dogacel Dogacel Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this works as intended.

In proto 3, having field presence = being marked as optional.

The method name was a little confusing, maybe we should name that something like hasFieldPresence()?

Copy link
Copy Markdown
Owner

@Dogacel Dogacel Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I wonder whether optional int32 a = 1 in proto3 and int32 a = 1 in edition 2023 both have hasFieldPresence set to true. Can you cross-check, write a test and confirm?


/**
* Get a default value for the given [Descriptors.FieldDescriptor].
*
Expand All @@ -14,7 +20,7 @@ object DefaultValues {
* @return default value of the given type
*/
fun defaultValueOf(
fieldDescriptor: Descriptors.FieldDescriptor,
fieldDescriptor: FieldDescriptor,
typeNames: Map<Descriptors.GenericDescriptor, TypeName> = mapOf(),
): Any? {
if (fieldDescriptor.realContainingOneof != null) {
Expand All @@ -28,10 +34,9 @@ object DefaultValues {
return CodeBlock.of("emptyList()")
}

if (fieldDescriptor.hasOptionalKeyword()) {
if (!fieldDescriptor.unconditionallyHasValue) {
return null
}

return when (fieldDescriptor.type) {
Descriptors.FieldDescriptor.Type.BOOL -> CodeBlock.of("false")
Descriptors.FieldDescriptor.Type.INT32 -> CodeBlock.of("0")
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/kotlin/dogacel/kotlinx/protobuf/gen/TypeNames.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ object TypeNames {
}

if (
fieldDescriptor.hasOptionalKeyword() ||
fieldDescriptor.isOptional ||
Copy link
Copy Markdown
Owner

@Dogacel Dogacel Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this causes a bug, we should update this to be unconditionallyHasValue as well.

Copy link
Copy Markdown
Owner

@Dogacel Dogacel Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe hasFieldPresence() is enough. Let's give it a try.

fieldDescriptor.type == Descriptors.FieldDescriptor.Type.MESSAGE ||
fieldDescriptor.realContainingOneof != null
) {
Expand Down
2 changes: 1 addition & 1 deletion examples/sample-grpc-server/src/main/kotlin/App.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class SampleServiceImpl : SampleService() {

override suspend fun add(addRequest: AddRequest): AddResponse {
return AddResponse(
result = addRequest.a + addRequest.b,
result = (addRequest.a ?: 0) + (addRequest.b ?: 0),
)
}
}
Expand Down
8 changes: 4 additions & 4 deletions generated-code-tests/src/main/kotlin/testgen/enums/enums.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import kotlinx.serialization.protobuf.ProtoNumber
@Serializable
public data class MessageWithEnum(
@ProtoNumber(number = 1)
public val id: Int = 0,
public val id: Int? = 0,
@ProtoNumber(number = 2)
public val testEnum: TestEnum = testgen.enums.TestEnum.FOO,
public val testEnum: TestEnum? = testgen.enums.TestEnum.FOO,
@ProtoNumber(number = 3)
public val aliasedEnum: AliasedEnum = testgen.enums.AliasedEnum.ALIAS_FOO,
public val aliasedEnum: AliasedEnum? = testgen.enums.AliasedEnum.ALIAS_FOO,
@ProtoNumber(number = 4)
public val nestedEnum: NestedEnum = testgen.enums.MessageWithEnum.NestedEnum.NESTED_FOO,
public val nestedEnum: NestedEnum? = testgen.enums.MessageWithEnum.NestedEnum.NESTED_FOO,
) {
@Serializable
public enum class NestedEnum {
Expand Down
4 changes: 2 additions & 2 deletions generated-code-tests/src/main/kotlin/testgen/google/protobuf/any.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import kotlinx.serialization.protobuf.ProtoNumber
@Serializable
public data class Any(
@ProtoNumber(number = 1)
public val typeUrl: String = "",
public val typeUrl: String? = "",
@ProtoNumber(number = 2)
public val `value`: ByteArray = byteArrayOf(),
public val `value`: ByteArray? = byteArrayOf(),
)
2 changes: 1 addition & 1 deletion generated-code-tests/src/main/kotlin/testgen/google/protobuf/struct.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import kotlinx.serialization.protobuf.ProtoNumber
@Serializable
public data class Struct(
@ProtoNumber(number = 1)
public val fields: Map<String, Value?> = emptyMap(),
public val fields: Map<String?, Value?> = emptyMap(),
)

@Serializable
Expand Down
42 changes: 21 additions & 21 deletions generated-code-tests/src/main/kotlin/testgen/maps/maps.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,48 +16,48 @@ import kotlinx.serialization.protobuf.ProtoNumber
@Serializable
public data class MapsMessage(
@ProtoNumber(number = 1)
public val mapInt32Int32: Map<Int, Int> = emptyMap(),
public val mapInt32Int32: Map<Int?, Int?> = emptyMap(),
@ProtoNumber(number = 2)
public val mapInt64Int64: Map<Long, Long> = emptyMap(),
public val mapInt64Int64: Map<Long?, Long?> = emptyMap(),
@ProtoNumber(number = 3)
public val mapUint32Uint32: Map<UInt, UInt> = emptyMap(),
public val mapUint32Uint32: Map<UInt?, UInt?> = emptyMap(),
@ProtoNumber(number = 4)
public val mapUint64Uint64: Map<ULong, ULong> = emptyMap(),
public val mapUint64Uint64: Map<ULong?, ULong?> = emptyMap(),
@ProtoNumber(number = 5)
public val mapSint32Sint32: Map<Int, Int> = emptyMap(),
public val mapSint32Sint32: Map<Int?, Int?> = emptyMap(),
@ProtoNumber(number = 6)
public val mapSint64Sint64: Map<Long, Long> = emptyMap(),
public val mapSint64Sint64: Map<Long?, Long?> = emptyMap(),
@ProtoNumber(number = 7)
public val mapFixed32Fixed32: Map<Int, Int> = emptyMap(),
public val mapFixed32Fixed32: Map<Int?, Int?> = emptyMap(),
@ProtoNumber(number = 8)
public val mapFixed64Fixed64: Map<Long, Long> = emptyMap(),
public val mapFixed64Fixed64: Map<Long?, Long?> = emptyMap(),
@ProtoNumber(number = 9)
public val mapSfixed32Sfixed32: Map<Int, Int> = emptyMap(),
public val mapSfixed32Sfixed32: Map<Int?, Int?> = emptyMap(),
@ProtoNumber(number = 10)
public val mapSfixed64Sfixed64: Map<Long, Long> = emptyMap(),
public val mapSfixed64Sfixed64: Map<Long?, Long?> = emptyMap(),
@ProtoNumber(number = 11)
public val mapInt32Float: Map<Int, Float> = emptyMap(),
public val mapInt32Float: Map<Int?, Float?> = emptyMap(),
@ProtoNumber(number = 12)
public val mapInt32Double: Map<Int, Double> = emptyMap(),
public val mapInt32Double: Map<Int?, Double?> = emptyMap(),
@ProtoNumber(number = 13)
public val mapBoolBool: Map<Boolean, Boolean> = emptyMap(),
public val mapBoolBool: Map<Boolean?, Boolean?> = emptyMap(),
@ProtoNumber(number = 14)
public val mapStringString: Map<String, String> = emptyMap(),
public val mapStringString: Map<String?, String?> = emptyMap(),
@ProtoNumber(number = 15)
public val mapStringBytes: Map<String, ByteArray> = emptyMap(),
public val mapStringBytes: Map<String?, ByteArray?> = emptyMap(),
@ProtoNumber(number = 16)
public val mapStringNestedMessage: Map<String, NestedMessage?> = emptyMap(),
public val mapStringNestedMessage: Map<String?, NestedMessage?> = emptyMap(),
@ProtoNumber(number = 17)
public val mapStringForeignMessage: Map<String, ForeignMessage?> = emptyMap(),
public val mapStringForeignMessage: Map<String?, ForeignMessage?> = emptyMap(),
@ProtoNumber(number = 18)
public val mapStringNestedEnum: Map<String, NestedEnum> = emptyMap(),
public val mapStringNestedEnum: Map<String?, NestedEnum?> = emptyMap(),
@ProtoNumber(number = 19)
public val mapStringForeignEnum: Map<String, ForeignEnum> = emptyMap(),
public val mapStringForeignEnum: Map<String?, ForeignEnum?> = emptyMap(),
) {
@Serializable
public data class NestedMessage(
@ProtoNumber(number = 1)
public val a: Int = 0,
public val a: Int? = 0,
@ProtoNumber(number = 2)
public val corecursive: MapsMessage? = null,
)
Expand All @@ -78,7 +78,7 @@ public data class MapsMessage(
@Serializable
public data class ForeignMessage(
@ProtoNumber(number = 1)
public val c: Int = 0,
public val c: Int? = 0,
)

@Serializable
Expand Down
12 changes: 2 additions & 10 deletions generated-code-tests/src/main/kotlin/testgen/messages/message_no_fields.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,15 @@ public class MessageNoFields() {
@Serializable
public data class SubMessageNoFieldsExtend(
@ProtoNumber(number = 1)
public val type: Type = testgen.messages.MessageNoFields.Type.UNKNOWN,
public val type: Type? = testgen.messages.MessageNoFields.Type.UNKNOWN,
)
}

@Serializable
public data class SubMessageOneofFields(
@ProtoNumber(number = 1)
public val someValue: Int? = null,
) {
init {
require(
listOfNotNull(
someValue,
).size <= 1
) { "Should only contain one of some_oneof." }
}
}
Comment on lines -34 to -42
Copy link
Copy Markdown
Owner

@Dogacel Dogacel Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder whether isSyntheticCompat works as intended in this situation or not. I don't see a reason why this block would be removed. It is not possible to set more than 1 one ofs.

)

@Serializable
public enum class Type {
Expand Down
6 changes: 3 additions & 3 deletions generated-code-tests/src/main/kotlin/testgen/messages/messages.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import kotlinx.serialization.protobuf.ProtoNumber
@Serializable
public data class MessagesMessage(
@ProtoNumber(number = 1)
public val id: String = "",
public val id: String? = "",
@ProtoNumber(number = 2)
public val optionalNestedMessage: NestedMessage? = null,
@ProtoNumber(number = 3)
Expand All @@ -17,7 +17,7 @@ public data class MessagesMessage(
@Serializable
public data class NestedMessage(
@ProtoNumber(number = 1)
public val a: Int = 0,
public val a: Int? = 0,
@ProtoNumber(number = 2)
public val corecursive: MessagesMessage? = null,
)
Expand All @@ -26,5 +26,5 @@ public data class MessagesMessage(
@Serializable
public data class ForeignMessage(
@ProtoNumber(number = 1)
public val c: Int = 0,
public val c: Int? = 0,
)
2 changes: 1 addition & 1 deletion generated-code-tests/src/main/kotlin/testgen/oneof/oneof.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public data class OneofMessage(
@Serializable
public data class NestedMessage(
@ProtoNumber(number = 1)
public val a: Int = 0,
public val a: Int? = 0,
@ProtoNumber(number = 2)
public val corecursive: OneofMessage? = null,
)
Expand Down
30 changes: 15 additions & 15 deletions generated-code-tests/src/main/kotlin/testgen/primitives/primitives.proto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,39 +18,39 @@ import kotlinx.serialization.protobuf.ProtoType
@Serializable
public data class PrimitivesMessage(
@ProtoNumber(number = 1)
public val optionalInt32: Int = 0,
public val optionalInt32: Int? = 0,
@ProtoNumber(number = 2)
public val optionalInt64: Long = 0L,
public val optionalInt64: Long? = 0L,
@ProtoNumber(number = 3)
public val optionalUint32: UInt = 0U,
public val optionalUint32: UInt? = 0U,
@ProtoNumber(number = 4)
public val optionalUint64: ULong = 0UL,
public val optionalUint64: ULong? = 0UL,
@ProtoNumber(number = 5)
@ProtoType(type = SIGNED)
public val optionalSint32: Int = 0,
public val optionalSint32: Int? = 0,
@ProtoNumber(number = 6)
@ProtoType(type = SIGNED)
public val optionalSint64: Long = 0L,
public val optionalSint64: Long? = 0L,
@ProtoNumber(number = 7)
@ProtoType(type = FIXED)
public val optionalFixed32: Int = 0,
public val optionalFixed32: Int? = 0,
@ProtoNumber(number = 8)
@ProtoType(type = FIXED)
public val optionalFixed64: Long = 0L,
public val optionalFixed64: Long? = 0L,
@ProtoNumber(number = 9)
@ProtoType(type = FIXED)
public val optionalSfixed32: Int = 0,
public val optionalSfixed32: Int? = 0,
@ProtoNumber(number = 10)
@ProtoType(type = FIXED)
public val optionalSfixed64: Long = 0L,
public val optionalSfixed64: Long? = 0L,
@ProtoNumber(number = 11)
public val optionalFloat: Float = 0.0f,
public val optionalFloat: Float? = 0.0f,
@ProtoNumber(number = 12)
public val optionalDouble: Double = 0.0,
public val optionalDouble: Double? = 0.0,
@ProtoNumber(number = 13)
public val optionalBool: Boolean = false,
public val optionalBool: Boolean? = false,
@ProtoNumber(number = 14)
public val optionalString: String = "",
public val optionalString: String? = "",
@ProtoNumber(number = 15)
public val optionalBytes: ByteArray = byteArrayOf(),
public val optionalBytes: ByteArray? = byteArrayOf(),
)
Loading