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

[Protobuf] Respect default values from official documentation #2831

Open
glureau opened this issue Oct 10, 2024 · 2 comments
Open

[Protobuf] Respect default values from official documentation #2831

glureau opened this issue Oct 10, 2024 · 2 comments
Labels

Comments

@glureau
Copy link

glureau commented Oct 10, 2024

(I did some research but not sure if I missed some documentation or discussions somewhere, sorry if it's a duplicate.)

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

My use-case is to generate correct protobuf3 messages so that other services (that may use other tools than kotlinx) are able to read properly.

A successful test as introduction:

    enum class Foo { A, B }

    @Serializable
    data class Bar(val foo: Foo = Foo.B)

    @Test
    fun defaultTest() { //
        val protobuf = ProtoBuf {
            encodeDefaults = false
        }
        val bar = Bar(Foo.B)
        val serialized = protobuf.encodeToByteArray(bar)
        assertEquals(0, serialized.size)
        val deserialized = protobuf.decodeFromByteArray<Bar>(serialized)
        assertEquals(bar, deserialized)
    }

This is not matching the official documentation as far as I understand: https://protobuf.dev/programming-guides/proto3/#default , especially those 2 quotes

For enums, the default value is the first defined enum value, which must be 0.

Also note that if a scalar message field is set to its default, the value will not be serialized on the wire.

It looks like we are mixing the Kotlin default values and what Protobuf considers the default values.
If this example is able to produce an empty ByteArray, decoding with the same protobuf from Java will lead to a Bar(Foo.A) instead of Bar(Foo.B).

This error is not limited to enums but all scalar types as mentioned in protobuf documentation (strings, bytes, bools, numeric types and enums).

Describe the solution you'd like

I'd like to have a serialization that respects the proto 3 format:

  • Default value in Kotlin code shouldn't be used when serializing/deserializing. Ktx should use the protobuf default values instead (ex: if Boolean is not set, then a false will be used, no matter the data class constructor signature).
  • For message fiels I presume null for nullable fields is the best things to do.

I consider the current encodeDefaults = false to be the best solution, given official doc "the value will not be serialized on the wire", and the fact that encoding default make it unusable with nullable fields. I presume it's ok to keep this flag for retro-compatibility if needed, but having a proper proto3 support is higher in my priority list.

@pdvrieze
Copy link
Contributor

@glureau It makes sense to add a mode to support it, but for compatibility reasons it wouldn't be valid to change the default behaviour. In terms of enums, the solution might be to "reorder" the enum fields so the default field will also be the one with index 0 (but again this changes the on-the-wire contents of the protobuf so may be incompatible).

@glureau
Copy link
Author

glureau commented Nov 13, 2024

This enum patch wouldn't be applicable on Strings or other scalar types unfortunately.

but for compatibility reasons it wouldn't be valid to change the default behaviour

I fully agree, I presume we could have a useKotlinDefaultValues = false when we want to support protobuf default values.
(I am currently implementing my own serialization lib to support this need, but I'd largely prefer Jetbrains to support a standard protobuf implementation and rely on it.)

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