Skip to content
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.

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

Missing Feature - ProtoOneOf doesn't support complex objects #2718

Open
rotilho opened this issue Jun 16, 2024 · 5 comments
Open

Missing Feature - ProtoOneOf doesn't support complex objects #2718

rotilho opened this issue Jun 16, 2024 · 5 comments
Labels

Comments

@rotilho
Copy link
Contributor

rotilho commented Jun 16, 2024

What is your use-case and why do you need this feature?

@ProtoOneOf was introduced in the last release, allowing us to define polymorphic classes in a protobuf way. However, it seems we are missing a critical feature: support for complex children.

Describe the solution you'd like

kotlinx.serialization protobuf oneof supports complex types:

syntax = "proto2";

message AttoOpen {
  required string field1 = 1;
  required string field2 = 2;
}

message AttoSend {
  required string field1 = 1;
  required string field2 = 2;
}

message AttoTransaction {
  required string signature = 1;
  required string work = 2;
  oneof block {
    AttoOpen open = 3;
    AttoSend send = 4;
  }
}
@xiaozhikang0916
Copy link
Contributor

xiaozhikang0916 commented Jun 16, 2024

Sorry but I cannot get your point. If you are saying a custom message type in oneof field, it is currently supported in protobuf. You can check this test for example.

And your code seems not matching the protobuf declaration.

@rotilho
Copy link
Contributor Author

rotilho commented Jun 16, 2024

Hey @xiaozhikang0916 ! Nice to see you around. You are right, the test works with single field, however if the subclass has more than one field it will fail. I simplified above example to avoid confusion

@xiaozhikang0916
Copy link
Contributor

Your example proto can be described in kotlin like

import kotlinx.serialization.Serializable
import kotlinx.serialization.decodeFromHexString
import kotlinx.serialization.encodeToHexString
import kotlin.test.Test
import kotlin.test.assertEquals

class TestComplicatedOneOf {
    @Serializable
    data class AttoTransaction(
        @ProtoNumber(1) val sign: String,
        @ProtoNumber(2) val work: String,
        @ProtoOneOf val oneOf: IBlock,
    )

    @Serializable
    sealed interface IBlock

    @Serializable
    data class BlockOpen(@ProtoNumber(3) val value: AttoOpen) : IBlock

    @Serializable
    data class BlockSend(@ProtoNumber(4) val value: AttoSend) : IBlock

    @Serializable
    data class AttoOpen(@ProtoNumber(1) val field1: String, @ProtoNumber(2) val field2: String)

    @Serializable
    data class AttoSend(@ProtoNumber(1) val field1: String, @ProtoNumber(2) val field2: String)

    @Test
    fun test() {
        val attoTransactionOpen = AttoTransaction("sign1", "work1", BlockOpen(AttoOpen("atto", "open")))
        val hex = ProtoBuf.encodeToHexString(AttoTransaction.serializer(), attoTransactionOpen)
        println(hex)
        val restored = ProtoBuf.decodeFromHexString(AttoTransaction.serializer(), hex)
        assertEquals(attoTransactionOpen, restored)

        val attoTransactionSend = AttoTransaction("sign2", "work2", BlockSend(AttoSend("hello", "send")))
        val hex2 = ProtoBuf.encodeToHexString(AttoTransaction.serializer(), attoTransactionSend)
        println(hex2)
        val restored2 = ProtoBuf.decodeFromHexString(AttoTransaction.serializer(), hex2)
        assertEquals(attoTransactionSend, restored2)
    }
}

And this test passed as expected. Did you miss the BlockOpen and BlockSend?

@rotilho
Copy link
Contributor Author

rotilho commented Jun 22, 2024

Sorry for the delay, I haven't seen the notification.

I didn't realize that I should wrap the class... This definitely works and gives the correct Protobuf schema. However, I was under the impression the Kotlin definition would be similar to generated Protobuf Java classes (without the need for a wrapper class). This requirement also seems to break how JSON serialization of polymorphic serialization works; implementations are able to have more than one field.

@xiaozhikang0916
Copy link
Contributor

The Java class generated by proto plugin will generate a Case enum type for all oneof field, so some runtime checks like getXXXCase() == XXXCase or hasXXX() is required, but cannot provide information for the smart check.

On the other hand, oneof in proto says that only one field can be assigned and the last one wins, but kotlinx.serialization is open for devs to define their own data classes and serializers. If the oneof field is defined in the old-Java style, you may find multiple fields are assigned, and have no idea which one will be write to the wire.

Wrapper class here, may not be ideal, but should be the best way for the oneof field, as long as kotlin does not support union types yet.

This requirement also seems to break how JSON serialization of polymorphic serialization works

That is not true. @ProtoOneOf is a format specific helper annotation to tell ProtoBuf serializer to skip one class layer, and those data classes are still valid to be serialized and deserialized in other formats e.g. JSON, just like any other classes do, as if this annotation does not show up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants